linux-kernel - Re: [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV spinlocks

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: <a789fb32-3830-e36b-f648-d070c742384f@oracle.com>
Date:   Thu, 3 Oct 2019 18:30:43 +0800
From:   Zhenzhong Duan <zhenzhong.duan@...cle.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     linux-kernel@...r.kernel.org, vkuznets@...hat.com,
        linux-hyperv@...r.kernel.org, kvm@...r.kernel.org,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        "H. Peter Anvin" <hpa@...or.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krcmar <rkrcmar@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Will Deacon <will@...nel.org>
Subject: Re: [PATCH v3 1/4] x86/kvm: Add "nopvspin" parameter to disable PV
 spinlocks

On 2019/10/3 1:10, Sean Christopherson wrote:

> On Mon, Sep 30, 2019 at 08:44:36PM +0800, Zhenzhong Duan wrote:
>> There are cases where a guest tries to switch spinlocks to bare metal
>> behavior (e.g. by setting "xen_nopvspin" on XEN platform and
>> "hv_nopvspin" on HYPER_V).
>>
>> That feature is missed on KVM, add a new parameter "nopvspin" to disable
>> PV spinlocks for KVM guest.
>>
>> This new parameter is also used to replace "xen_nopvspin" and
>> "hv_nopvspin".
> This is confusing as there are no Xen or Hyper-V changes in this patch.
> Please make it clear that you're talking about future patches, e.g.:
>
>    The new 'nopvspin' parameter will also replace Xen and Hyper-V specific
>    parameters in future patches.

Will fix

>
>> The global variable pvspin isn't defined as __initdata as it's used at
>> runtime by XEN guest.
> Same comment as above regarding what this patch is doing versus what will
> be done in the future.  Arguably you should even mark it __initdata in
> this patch and deal with conflict in the Xen patch, e.g. use it only to
> set the existing xen_pvspin variable.

Will fix

>
>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@...cle.com>

......snip

>>   /**
>> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
>> index e820568..a4f108d 100644
>> --- a/arch/x86/kernel/kvm.c
>> +++ b/arch/x86/kernel/kvm.c
>> @@ -842,6 +842,13 @@ void __init kvm_spinlock_init(void)
>>   	if (num_possible_cpus() == 1)
>>   		return;
>>   
>> +	if (!pvspin) {
>> +		pr_info("PV spinlocks disabled\n");
>> +		static_branch_disable(&virt_spin_lock_key);
>> +		return;
>> +	}
>> +	pr_info("PV spinlocks enabled\n");
> These prints could be confusing as KVM also disables PV spinlocks when it
> sees KVM_HINTS_REALTIME.

What about below:

pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");

Or you prefer separate print for each disabling like below?

         /* Does host kernel support KVM_FEATURE_PV_UNHALT? */
         if (!kvm_para_has_feature(KVM_FEATURE_PV_UNHALT)) {
                 pr_info("PV spinlocks disabled, KVM_FEATURE_PV_UNHALT feature needed.\n");
                 return;
         }

         if (kvm_para_has_hint(KVM_HINTS_REALTIME)) {
                 pr_info("PV spinlocks disabled, having non-preemption hints.\n");
                 return;
         }

         /* Don't use the pvqspinlock code if there is only 1 vCPU. */
         if (num_possible_cpus() == 1) {
                 pr_info("PV spinlocks disabled on UP.\n");
                 return;
         }
	if (!pvspin) {
		pr_info("PV spinlocks disabled forced by \"nopvspin\" parameter.\n");
		static_branch_disable(&virt_spin_lock_key);
		return;
	}
	pr_info("PV spinlocks enabled\n");

>
>> +
>>   	__pv_init_lock_hash();
>>   	pv_ops.lock.queued_spin_lock_slowpath = __pv_queued_spin_lock_slowpath;
>>   	pv_ops.lock.queued_spin_unlock =
>> diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
>> index 2473f10..945b510 100644
>> --- a/kernel/locking/qspinlock.c
>> +++ b/kernel/locking/qspinlock.c
>> @@ -580,4 +580,11 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
>>   #include "qspinlock_paravirt.h"
>>   #include "qspinlock.c"
>>   
>> +bool pvspin = true;
> This can be __ro_after_init, or probably better __initdata and have Xen
> snapshot the value for its use case.

I will use __initdata

>
> Personal preference: I'd invert the bool and name it nopvspin to make it
> easier to connect the variable to the kernel param.

OK, will do that.  Thanks for review for all the patches.

Zhenzhong