| 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
| ||
|
Date: Mon, 25 Mar 2019 08:47:05 -0700
From: Sean Christopherson <sean.j.christopherson@...el.com>
To: Xiaoyao Li <xiaoyao.li@...ux.intel.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>,
Radim Krčmář <rkrcmar@...hat.com>,
linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
"H. Peter Anvin" <hpa@...or.com>, x86@...nel.org,
chao.gao@...el.com
Subject: Re: [PATCH v3 2/2] x86/vmx: optimize MSR_MISC_FEATURES_ENABLES switch
On Mon, Mar 25, 2019 at 04:06:50PM +0800, Xiaoyao Li wrote:
> KVM needs to switch MSR_MISC_FEATURES_ENABLES between host and guest in
> every pcpu/vcpu context switch. Since WRMSR is expensive, this patch tries
> to save cycles by avoiding WRMSR MSR_MISC_FEATURES_ENABLES whenever possible.
>
> If host's value is zero, nothing needs to be done, since guest can use
> kvm emulated cpuid faulting.
>
> If host's value is non-zero, it need not clear MSR_MISC_FEATURES_ENABLES
> unconditionally. We can use hardware cpuid faulting if guest's
> value is equal to host'value, thus avoid WRMSR
> MSR_MISC_FEATURES_ENABLES.
>
> Since hardware cpuid faulting takes higher priority than CPUID vm exit,
> it should be updated to hardware while guest wrmsr and hardware cpuid
> faulting is used for guest.
>
> Signed-off-by: Xiaoyao Li <xiaoyao.li@...ux.intel.com>
> ---
> arch/x86/include/asm/kvm_host.h | 2 ++
> arch/x86/kvm/vmx/vmx.c | 13 ++++++++++---
> arch/x86/kvm/x86.c | 15 ++++++++++++---
> 3 files changed, 24 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 2c53df4a5a2a..9bcb444b903d 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -1343,6 +1343,8 @@ void kvm_lmsw(struct kvm_vcpu *vcpu, unsigned long msw);
> void kvm_get_cs_db_l_bits(struct kvm_vcpu *vcpu, int *db, int *l);
> int kvm_set_xcr(struct kvm_vcpu *vcpu, u32 index, u64 xcr);
>
> +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data);
> +
> int kvm_get_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
> int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr);
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 65aa947947ba..73bb11f74b36 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -1035,10 +1035,10 @@ static void vmx_prepare_guest_misc_features_enables(struct vcpu_vmx *vmx)
> {
> u64 msrval = this_cpu_read(msr_misc_features_shadow);
>
> - if (!msrval)
> + if (!msrval || msrval == vmx->vcpu.arch.msr_misc_features_enables)
> return;
>
> - wrmsrl(MSR_MISC_FEATURES_ENABLES, 0ULL);
> + wrmsrl(MSR_MISC_FEATURES_ENABLES, vmx->vcpu.arch.msr_misc_features_enables);
> }
>
> void vmx_prepare_switch_to_guest(struct kvm_vcpu *vcpu)
> @@ -1136,7 +1136,7 @@ static void vmx_load_host_misc_features_enables(struct vcpu_vmx *vmx)
> {
> u64 msrval = this_cpu_read(msr_misc_features_shadow);
>
> - if (!msrval)
> + if (!msrval || msrval == vmx->vcpu.arch.msr_misc_features_enables)
> return;
>
> wrmsrl(MSR_MISC_FEATURES_ENABLES, msrval);
> @@ -2043,6 +2043,13 @@ static int vmx_set_msr(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> else
> vmx->pt_desc.guest.addr_a[index / 2] = data;
> break;
> + case MSR_MISC_FEATURES_ENABLES:
> + if (!kvm_supported_msr_misc_features_enables(vcpu, data))
> + return 1;
> + if (this_cpu_read(msr_misc_features_shadow) && vmx->loaded_cpu_state)
> + wrmsrl(MSR_MISC_FEATURES_ENABLES, data);
> + vcpu->arch.msr_misc_features_enables = data;
> + break;
> case MSR_TSC_AUX:
> if (!msr_info->host_initiated &&
> !guest_cpuid_has(vcpu, X86_FEATURE_RDTSCP))
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ad1df965574e..d2af90422a51 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2449,6 +2449,17 @@ static void record_steal_time(struct kvm_vcpu *vcpu)
> &vcpu->arch.st.steal, sizeof(struct kvm_steal_time));
> }
>
> +int kvm_supported_msr_misc_features_enables(struct kvm_vcpu *vcpu, u64 data)
The name kvm_supported_msr_misc_features_enables() is a bit confusing,
e.g. I expected it to return a bitmask of the supported features. Maybe
kvm_misc_features_enables_msr_valid() to be consistent with
vmx_feature_control_msr_valid()?
> +{
> + if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
> + (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
> + !supports_cpuid_fault(vcpu)))
> + return 0;
> + else
> + return 1;
No need for the if-else, you can simply do:
return (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) ||
(data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
!supports_cpuid_fault(vcpu));
You could even shorten it to:
return (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT) ||
(data && !supports_cpuid_fault(vcpu));
although that might be getting a bit too cute.
> +}
> +EXPORT_SYMBOL_GPL(kvm_supported_msr_misc_features_enables);
> +
> int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> {
> bool pr = false;
> @@ -2669,9 +2680,7 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, struct msr_data *msr_info)
> vcpu->arch.msr_platform_info = data;
> break;
> case MSR_MISC_FEATURES_ENABLES:
> - if (data & ~MSR_MISC_FEATURES_ENABLES_CPUID_FAULT ||
> - (data & MSR_MISC_FEATURES_ENABLES_CPUID_FAULT &&
> - !supports_cpuid_fault(vcpu)))
> + if (!kvm_supported_msr_misc_features_enables(vcpu, data))
> return 1;
> vcpu->arch.msr_misc_features_enables = data;
> break;
> --
> 2.19.1
>
Powered by blists - more mailing lists