Re: [PATCH v2 03/13] x86/irq: Remove bitfields in posted interrupt descriptor

From: Jacob Pan
Date: Wed Apr 17 2024 - 13:57:08 EST


Hi Sean,

On Tue, 16 Apr 2024 17:39:42 -0700, Sean Christopherson <seanjc@xxxxxxxxxx>
wrote:

> "KVM" here would be nice too.
>
> On Fri, Apr 05, 2024, Jacob Pan wrote:
> > Mixture of bitfields and types is weird and really not intuitive, remove
> > bitfields and use typed data exclusively.
> >
> > Link:
> > https://lore.kernel.org/all/20240404101735.402feec8@jacob-builder/T/#mf66e34a82a48f4d8e2926b5581eff59a122de53a
> > Suggested-by: Sean Christopherson <seanjc@xxxxxxxxxx> Suggested-by:
> > Thomas Gleixner <tglx@xxxxxxxxxxxxx> Signed-off-by: Jacob Pan
> > <jacob.jun.pan@xxxxxxxxxxxxxxx>
> >
> > ---
> > v2:
> > - Replace bitfields, no more mix.
> > ---
> > arch/x86/include/asm/posted_intr.h | 10 +---------
> > arch/x86/kvm/vmx/posted_intr.c | 4 ++--
> > arch/x86/kvm/vmx/vmx.c | 2 +-
> > 3 files changed, 4 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/posted_intr.h
> > b/arch/x86/include/asm/posted_intr.h index acf237b2882e..c682c41d4e44
> > 100644 --- a/arch/x86/include/asm/posted_intr.h
> > +++ b/arch/x86/include/asm/posted_intr.h
> > @@ -15,17 +15,9 @@ struct pi_desc {
> > };
> > union {
> > struct {
> > - /* bit 256 - Outstanding Notification
> > */
> > - u16 on : 1,
> > - /* bit 257 - Suppress Notification */
> > - sn : 1,
> > - /* bit 271:258 - Reserved */
> > - rsvd_1 : 14;
> > - /* bit 279:272 - Notification Vector */
> > + u16 notif_ctrl; /* Suppress and
> > outstanding bits */ u8 nv;
> > - /* bit 287:280 - Reserved */
> > u8 rsvd_2;
> > - /* bit 319:288 - Notification
> > Destination */ u32 ndst;
> > };
> > u64 control;
> > diff --git a/arch/x86/kvm/vmx/posted_intr.c
> > b/arch/x86/kvm/vmx/posted_intr.c index af662312fd07..592dbb765675 100644
> > --- a/arch/x86/kvm/vmx/posted_intr.c
> > +++ b/arch/x86/kvm/vmx/posted_intr.c
> > @@ -107,7 +107,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int
> > cpu)
> > * handle task migration (@cpu != vcpu->cpu).
> > */
> > new.ndst = dest;
> > - new.sn = 0;
> > + new.notif_ctrl &= ~POSTED_INTR_SN;
>
> At the risk of creating confusing, would it make sense to add
> double-underscore, non-atomic versions of the set/clear helpers for ON
> and SN?
>
> I can't tell if that's a net positive versus open coding clear() and
> set() here and below.
IMHO, we can add non-atomic helpers when we have more than one user for
each operation.

I do have a stupid bug here, it should be:
- new.notif_ctrl &= ~POSTED_INTR_SN;
+ new.notif_ctrl &= ~BIT(POSTED_INTR_SN);
Same as below.

Thanks to Oliver(LKP kvm self test). I didn't catch that in my VFIO device
assignment test.

>
> > /*
> > * Restore the notification vector; in the blocking
> > case, the @@ -157,7 +157,7 @@ static void
> > pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
> > &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
> > raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
> > - WARN(pi_desc->sn, "PI descriptor SN field set before
> > blocking");
> > + WARN(pi_desc->notif_ctrl & POSTED_INTR_SN, "PI descriptor SN
> > field set before blocking");
>
> This can use pi_test_sn(), as test_bit() isn't atomic, i.e. doesn't incur
> a LOCK.
make sense. will do.

> >
> > old.control = READ_ONCE(pi_desc->control);
> > do {
> > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> > index d94bb069bac9..50580bbfba5d 100644
> > --- a/arch/x86/kvm/vmx/vmx.c
> > +++ b/arch/x86/kvm/vmx/vmx.c
> > @@ -4843,7 +4843,7 @@ static void __vmx_vcpu_reset(struct kvm_vcpu
> > *vcpu)
> > * or POSTED_INTR_WAKEUP_VECTOR.
> > */
> > vmx->pi_desc.nv = POSTED_INTR_VECTOR;
> > - vmx->pi_desc.sn = 1;
> > + vmx->pi_desc.notif_ctrl |= POSTED_INTR_SN;


Thanks,

Jacob