"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. > /* > * 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. > > 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;
