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

[Jacob Pan <jacob.jun.pan@linux.intel.com>]

Hi Sean,

On Tue, 16 Apr 2024 17:39:42 -0700, Sean Christopherson <seanjc@google.com>
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@google.com> Suggested-by:
> > Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Jacob Pan
> > <jacob.jun.pan@linux.intel.com>
> > 
> > ---
> > 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