linux-kernel - Re: [PATCH v4] x86/paravirt: Disable virt spinlock on bare metal

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: <ZqueWhX6lqS+1vwg@chenyu5-mobl2>
Date: Thu, 1 Aug 2024 22:40:26 +0800
From: Chen Yu <yu.c.chen@...el.com>
To: maobibo <maobibo@...ngson.cn>
CC: Dave Hansen <dave.hansen@...ux.intel.com>, Thomas Gleixner
	<tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, Borislav Petkov
	<bp@...en8.de>, "H. Peter Anvin" <hpa@...or.com>, Arnd Bergmann
	<arnd@...db.de>, <virtualization@...ts.linux.dev>,
	<linux-kernel@...r.kernel.org>, Juergen Gross <jgross@...e.com>, "Nikolay
 Borisov" <nik.borisov@...e.com>, Qiuxu Zhuo <qiuxu.zhuo@...el.com>, "Prem
 Nath Dey" <prem.nath.dey@...el.com>, Xiaoping Zhou <xiaoping.zhou@...el.com>
Subject: Re: [PATCH v4] x86/paravirt: Disable virt spinlock on bare metal

Hi Bibo,

On 2024-08-01 at 16:00:19 +0800, maobibo wrote:
> Chenyu,
> 
> I do not know much about x86, just give some comments(probably incorrected)
> from the code.
> 
> On 2024/7/29 下午2:52, Chen Yu wrote:
> > X86_FEATURE_HYPERVISOR         Y    Y    Y     N
> > CONFIG_PARAVIRT_SPINLOCKS      Y    Y    N     Y/N
> > PV spinlock                    Y    N    N     Y/N
> > 
> > virt_spin_lock_key             N    N    Y     N
> > 
> > -DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key);
> > +DECLARE_STATIC_KEY_FALSE(virt_spin_lock_key);
> 
> @@ -87,7 +87,7 @@ static inline bool virt_spin_lock(struct qspinlock *lock)
>  {
>         int val;
> 
> -       if (!static_branch_likely(&virt_spin_lock_key))
> +       if (!static_branch_unlikely(&virt_spin_lock_key))
>                 return false;
> 
> Do we need change it with static_branch_unlikely() if value of key is false
> by fault?

My understanding is that, firstly, whether it is likely() or unlikely()
depends on the 'expected' value of the key, rather than its default
initialized value. The compiler can arrange the if 'jump' condition to
avoid the overhead of branch jump(to keep the instruction pipeline)
as much as possible. Secondly, before this patch, the 'expected' value
of virt_spin_lock_key is 'true', so I'm not sure if we should change
its behavior. Although it seems that in most VM guest, with para-virt
spinlock enabled, this key should be false at most time, but just in
case of any regression...

> >   /*
> >    * Shortcut for the queued_spin_lock_slowpath() function that allows
> > diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
> > index 5358d43886ad..fec381533555 100644
> > --- a/arch/x86/kernel/paravirt.c
> > +++ b/arch/x86/kernel/paravirt.c
> > @@ -51,13 +51,12 @@ DEFINE_ASM_FUNC(pv_native_irq_enable, "sti", .noinstr.text);
> >   DEFINE_ASM_FUNC(pv_native_read_cr2, "mov %cr2, %rax", .noinstr.text);
> >   #endif
> > -DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
> > +DEFINE_STATIC_KEY_FALSE(virt_spin_lock_key);
> >   void __init native_pv_lock_init(void)
> >   {
> > -	if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) &&
> > -	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > -		static_branch_disable(&virt_spin_lock_key);
> > +	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
> > +		static_branch_enable(&virt_spin_lock_key);
> >   }
> 
> From my point, the sentence static_branch_disable(&virt_spin_lock_key) can
> be removed in file arch/x86/xen/spinlock.c and arch/x86/kernel/kvm.c, since
> function native_smp_prepare_boot_cpu() is already called by
> xen_smp_prepare_boot_cpu() and kvm_smp_prepare_boot_cpu().
>

The key is enabled by native_smp_prepare_boot_cpu() for VM guest as
the initial value(default to true). And later either arch/x86/xen/spinlock.c
or arch/x86/kernel/kvm.c disable this key in a on-demand manner.

thanks,
Chenyu