Re: [PATCH RFC 07/13] x86/irq: Add helpers for checking Intel PID

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

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