Received-SPF: pass (google.com: domain of linux-kernel+bounces-40847-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Fri, 26 Jan 2024 15:31:08 -0800
From: Jacob Pan <jacob.jun.pan@linux.intel.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: LKML <linux-kernel@vger.kernel.org>, X86 Kernel <x86@kernel.org>,
 iommu@lists.linux.dev, Lu Baolu <baolu.lu@linux.intel.com>,
 kvm@vger.kernel.org, Dave Hansen <dave.hansen@intel.com>, Joerg Roedel
 <joro@8bytes.org>, "H. Peter Anvin" <hpa@zytor.com>, Borislav Petkov
 <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>, Raj Ashok
 <ashok.raj@intel.com>, "Tian, Kevin" <kevin.tian@intel.com>,
 maz@kernel.org, peterz@infradead.org, seanjc@google.com, Robin Murphy
 <robin.murphy@arm.com>, jacob.jun.pan@linux.intel.com
Subject: Re: [PATCH RFC 07/13] x86/irq: Add helpers for checking Intel PID
Message-ID: <20240126153108.2a6bbb9e@jacob-builder>
In-Reply-To: <87il5bupb1.ffs@tglx>
References: <20231112041643.2868316-1-jacob.jun.pan@linux.intel.com>
 <20231112041643.2868316-8-jacob.jun.pan@linux.intel.com>
 <87il5bupb1.ffs@tglx>
Organization: OTC
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Hi Thomas,

On Wed, 06 Dec 2023 20:02:58 +0100, Thomas Gleixner <tglx@linutronix.de>
wrote:

> On Sat, Nov 11 2023 at 20:16, Jacob Pan wrote:
> 
> That 'Intel PID' in the subject line sucks. What's wrong with writing
> things out?
> 
>        x86/irq: Add accessors for posted interrupt descriptors
> 
will do.

> Hmm?
> 
> > Intel posted interrupt descriptor (PID) stores pending interrupts in its
> > posted interrupt requests (PIR) bitmap.
> >
> > Add helper functions to check individual vector status and the entire
> > bitmap.
> >
> > They are used for interrupt migration and runtime demultiplexing posted
> > MSI vectors.  
> 
> This is all backwards.
> 
>   Posted interrupts are controlled by and pending interrupts are marked in
>   the posted interrupt descriptor. The upcoming support for host side
>   posted interrupts requires accessors to check for pending vectors.
> 
>   Add ....
> 
> >  #ifdef CONFIG_X86_POSTED_MSI
> > +/*
> > + * Not all external vectors are subject to interrupt remapping, e.g.
> > IOMMU's
> > + * own interrupts. Here we do not distinguish them since those vector
> > bits in
> > + * PIR will always be zero.
> > + */
> > +static inline bool is_pi_pending_this_cpu(unsigned int vector)  
> 
> Can you please use a proper name space pi_.....() instead of this
> is_...() muck which is horrible to grep for. It's documented ....
> 
good idea, will do.

> > +{
> > +	struct pi_desc *pid;
> > +
> > +	if (WARN_ON(vector > NR_VECTORS || vector <
> > FIRST_EXTERNAL_VECTOR))
> > +		return false;  
> 
> Haha. So much about your 'can use the full vector space' dreams .... And
> WARN_ON_ONCE() please.
> 
yes, will do. Not enough motivation for the full vector space.

> > +
> > +	pid = this_cpu_ptr(&posted_interrupt_desc);  
> 
> Also this can go into the declaration line.
will do

> 
> > +
> > +	return (pid->pir[vector >> 5] & (1 << (vector % 32)));  
> 
>   __test_bit() perhaps?
> 
> > +}  
> 
> > +static inline bool is_pir_pending(struct pi_desc *pid)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < 4; i++) {
> > +		if (pid->pir_l[i])
> > +			return true;
> > +	}
> > +
> > +	return false;  
> 
> This is required because pi_is_pir_empty() is checking the other way
> round, right?
> 
This function is not needed anymore in the next version. I was thinking
performance is better if we bail out while encountering the first set bit.

> > +}
> > +
> >  extern void intel_posted_msi_init(void);
> >  
> >  #else
> > +static inline bool is_pi_pending_this_cpu(unsigned int vector) {return
> > false; }  
> 
> lacks space before 'return'
> 
will fix.
> > +
> >  static inline void intel_posted_msi_init(void) {};
> >  
> >  #endif /* X86_POSTED_MSI */  


Thanks,

Jacob