linux-kernel - Re: [PATCH] KVM: VMX: Micro-optimize vmexit time when not exposing PMU

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]

Date:   Thu, 12 Mar 2020 11:36:19 +0100
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Wanpeng Li <kernellwp@...il.com>, linux-kernel@...r.kernel.org,
        kvm@...r.kernel.org
Cc:     Paolo Bonzini <pbonzini@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>
Subject: Re: [PATCH] KVM: VMX: Micro-optimize vmexit time when not exposing PMU

Wanpeng Li <kernellwp@...il.com> writes:

> From: Wanpeng Li <wanpengli@...cent.com>
>
> PMU is not exposed to guest by most of cloud providers since the bad performance 
> of PMU emulation and security concern. However, it calls perf_guest_switch_get_msrs()
> and clear_atomic_switch_msr() unconditionally even if PMU is not exposed to the 
> guest before each vmentry. 
>
> ~1.28% vmexit time reduced can be observed by kvm-unit-tests/vmexit.flat on my 
> SKX server.
>
> Before patch:
> vmcall 1559
>
> After patch:
> vmcall 1539
>
> Signed-off-by: Wanpeng Li <wanpengli@...cent.com>
> ---
>  arch/x86/kvm/vmx/vmx.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 40b1e61..fd526c8 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -6441,6 +6441,9 @@ static void atomic_switch_perf_msrs(struct vcpu_vmx *vmx)
>  	int i, nr_msrs;
>  	struct perf_guest_switch_msr *msrs;
>  
> +	if (!vcpu_to_pmu(&vmx->vcpu)->version)
> +		return;
> +
>  	msrs = perf_guest_get_msrs(&nr_msrs);
>  
>  	if (!msrs)

Personally, I'd prefer this to be expressed as

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 40b1e6138cd5..ace92076c90f 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -6567,7 +6567,9 @@ static void vmx_vcpu_run(struct kvm_vcpu *vcpu)
 
        pt_guest_enter(vmx);
 
-       atomic_switch_perf_msrs(vmx);
+       if (vcpu_to_pmu(&vmx->vcpu)->version)
+               atomic_switch_perf_msrs(vmx);
+
        atomic_switch_umwait_control_msr(vmx);
 
        if (enable_preemption_timer)

(which will likely produce the same code as atomic_switch_perf_msrs() is
likely inlined).

Also, (not knowing much about PMU), is
"vcpu_to_pmu(&vmx->vcpu)->version" check correct?

E.g. in intel_is_valid_msr() correct for Intel PMU or is it stated
somewhere that it is generic rule?

Also, speaking about cloud providers and the 'micro' nature of this
optimization, would it rather make sense to introduce a static branch
(the policy to disable vPMU is likely to be host wide, right)?

-- 
Vitaly