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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240417110131.4aaf1d66@jacob-builder>
Date: Wed, 17 Apr 2024 11:01:31 -0700
From: Jacob Pan <jacob.jun.pan@...ux.intel.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: LKML <linux-kernel@...r.kernel.org>, X86 Kernel <x86@...nel.org>, Peter
 Zijlstra <peterz@...radead.org>, iommu@...ts.linux.dev, Thomas Gleixner
 <tglx@...utronix.de>, Lu Baolu <baolu.lu@...ux.intel.com>,
 kvm@...r.kernel.org, Dave Hansen <dave.hansen@...el.com>, Joerg Roedel
 <joro@...tes.org>, "H. Peter Anvin" <hpa@...or.com>, Borislav Petkov
 <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>, Paul Luse
 <paul.e.luse@...el.com>, Dan Williams <dan.j.williams@...el.com>, Jens
 Axboe <axboe@...nel.dk>, Raj Ashok <ashok.raj@...el.com>, Kevin Tian
 <kevin.tian@...el.com>, maz@...nel.org, Robin Murphy
 <robin.murphy@....com>, jim.harris@...sung.com, a.manzanares@...sung.com,
 Bjorn Helgaas <helgaas@...nel.org>, guang.zeng@...el.com,
 robert.hoo.linux@...il.com, jacob.jun.pan@...ux.intel.com,
 oliver.sang@...el.com
Subject: Re: [PATCH v2 03/13] x86/irq: Remove bitfields in posted interrupt
 descriptor

Hi Sean,

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