Hi Thomas,
On Wed, 06 Dec 2023 20:02:58 +0100, Thomas Gleixner <tglx@xxxxxxxxxxxxx>
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
