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
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 ....
> +{
> + 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.
> +
> + pid = this_cpu_ptr(&posted_interrupt_desc);
Also this can go into the declaration line.
> +
> + 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?
> +}
> +
> 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'
> +
> static inline void intel_posted_msi_init(void) {};
>
> #endif /* X86_POSTED_MSI */
Hi Thomas,
On Wed, 06 Dec 2023 20:02:58 +0100, Thomas Gleixner <[email protected]>
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