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
