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 for Android: free password hash cracker in your pocket

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

Message-ID: <Zh8aTitLwSYYlZW5@google.com>
Date: Tue, 16 Apr 2024 17:39:42 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Jacob Pan <jacob.jun.pan@...ux.intel.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
Subject: Re: [PATCH v2 03/13] x86/irq: Remove bitfields in posted interrupt descriptor

"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.

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