From: Zeng Guang <guang.zeng@intel.com>
To: "Christopherson,, Sean" <seanjc@google.com>,
Paolo Bonzini <pbonzini@redhat.com>,
Vitaly Kuznetsov <vkuznets@redhat.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Maxim Levitsky <mlevitsk@redhat.com>
Subject: Re: [PATCH v2 12/21] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled"
Date: Mon, 14 Aug 2023 14:28:59 +0800 [thread overview]
Message-ID: <fa15cd52-b10a-6aad-d63f-3d809d16f591@intel.com> (raw)
In-Reply-To: <20230729011608.1065019-13-seanjc@google.com>
On 7/29/2023 9:15 AM, Sean Christopherson wrote:
> Use the governed feature framework to track if XSAVES is "enabled", i.e.
> if XSAVES can be used by the guest. Add a comment in the SVM code to
> explain the very unintuitive logic of deliberately NOT checking if XSAVES
> is enumerated in the guest CPUID model.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
> arch/x86/kvm/governed_features.h | 1 +
> arch/x86/kvm/svm/svm.c | 17 ++++++++++++++---
> arch/x86/kvm/vmx/vmx.c | 32 ++++++++++++++++++--------------
> arch/x86/kvm/x86.c | 4 ++--
> 4 files changed, 35 insertions(+), 19 deletions(-)
>
> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h
> index b29c15d5e038..b896a64e4ac3 100644
> --- a/arch/x86/kvm/governed_features.h
> +++ b/arch/x86/kvm/governed_features.h
> @@ -6,6 +6,7 @@ BUILD_BUG()
> #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x)
>
> KVM_GOVERNED_X86_FEATURE(GBPAGES)
> +KVM_GOVERNED_X86_FEATURE(XSAVES)
>
> #undef KVM_GOVERNED_X86_FEATURE
> #undef KVM_GOVERNED_FEATURE
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 64092df06f94..d5f8cb402eb7 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4204,9 +4204,20 @@ static void svm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> struct vcpu_svm *svm = to_svm(vcpu);
> struct kvm_cpuid_entry2 *best;
>
> - vcpu->arch.xsaves_enabled = guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> - boot_cpu_has(X86_FEATURE_XSAVE) &&
> - boot_cpu_has(X86_FEATURE_XSAVES);
> + /*
> + * SVM doesn't provide a way to disable just XSAVES in the guest, KVM
> + * can only disable all variants of by disallowing CR4.OSXSAVE from
> + * being set. As a result, if the host has XSAVE and XSAVES, and the
> + * guest has XSAVE enabled, the guest can execute XSAVES without
> + * faulting. Treat XSAVES as enabled in this case regardless of
> + * whether it's advertised to the guest so that KVM context switches
> + * XSS on VM-Enter/VM-Exit. Failure to do so would effectively give
> + * the guest read/write access to the host's XSS.
> + */
> + if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> + boot_cpu_has(X86_FEATURE_XSAVES) &&
> + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> + kvm_governed_feature_set(vcpu, X86_FEATURE_XSAVES);
>
> /* Update nrips enabled cache */
> svm->nrips_enabled = kvm_cpu_cap_has(X86_FEATURE_NRIPS) &&
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index a0a47be2feed..3100ed62615c 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -4518,16 +4518,19 @@ vmx_adjust_secondary_exec_control(struct vcpu_vmx *vmx, u32 *exec_control,
> * based on a single guest CPUID bit, with a dedicated feature bit. This also
> * verifies that the control is actually supported by KVM and hardware.
> */
> -#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
> -({ \
> - bool __enabled; \
> - \
> - if (cpu_has_vmx_##name()) { \
> - __enabled = guest_cpuid_has(&(vmx)->vcpu, \
> - X86_FEATURE_##feat_name); \
> - vmx_adjust_secondary_exec_control(vmx, exec_control, \
> - SECONDARY_EXEC_##ctrl_name, __enabled, exiting); \
> - } \
> +#define vmx_adjust_sec_exec_control(vmx, exec_control, name, feat_name, ctrl_name, exiting) \
> +({ \
> + struct kvm_vcpu *__vcpu = &(vmx)->vcpu; \
> + bool __enabled; \
> + \
> + if (cpu_has_vmx_##name()) { \
> + if (kvm_is_governed_feature(X86_FEATURE_##feat_name)) \
> + __enabled = guest_can_use(__vcpu, X86_FEATURE_##feat_name); \
> + else \
> + __enabled = guest_cpuid_has(__vcpu, X86_FEATURE_##feat_name); \
> + vmx_adjust_secondary_exec_control(vmx, exec_control, SECONDARY_EXEC_##ctrl_name,\
> + __enabled, exiting); \
> + } \
> })
>
> /* More macro magic for ENABLE_/opt-in versus _EXITING/opt-out controls. */
> @@ -4587,10 +4590,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
> if (!enable_pml || !atomic_read(&vcpu->kvm->nr_memslots_dirty_logging))
> exec_control &= ~SECONDARY_EXEC_ENABLE_PML;
>
> - if (cpu_has_vmx_xsaves())
> - vmx_adjust_secondary_exec_control(vmx, &exec_control,
> - SECONDARY_EXEC_ENABLE_XSAVES,
> - vcpu->arch.xsaves_enabled, false);
> + vmx_adjust_sec_exec_feature(vmx, &exec_control, xsaves, XSAVES);
>
> /*
> * RDPID is also gated by ENABLE_RDTSCP, turn on the control if either
> @@ -4609,6 +4609,7 @@ static u32 vmx_secondary_exec_control(struct vcpu_vmx *vmx)
> SECONDARY_EXEC_ENABLE_RDTSCP,
> rdpid_or_rdtscp_enabled, false);
> }
> +
> vmx_adjust_sec_exec_feature(vmx, &exec_control, invpcid, INVPCID);
>
> vmx_adjust_sec_exec_exiting(vmx, &exec_control, rdrand, RDRAND);
> @@ -7722,6 +7723,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> boot_cpu_has(X86_FEATURE_XSAVE) &&
> guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> guest_cpuid_has(vcpu, X86_FEATURE_XSAVES);
> + if (boot_cpu_has(X86_FEATURE_XSAVE) &&
> + guest_cpuid_has(vcpu, X86_FEATURE_XSAVE))
> + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_XSAVES);
>
> vmx_setup_uret_msrs(vmx);
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 5a14378ed4e1..201fa957ce9a 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1012,7 +1012,7 @@ void kvm_load_guest_xsave_state(struct kvm_vcpu *vcpu)
> if (vcpu->arch.xcr0 != host_xcr0)
> xsetbv(XCR_XFEATURE_ENABLED_MASK, vcpu->arch.xcr0);
>
> - if (vcpu->arch.xsaves_enabled &&
> + if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
> vcpu->arch.ia32_xss != host_xss)
> wrmsrl(MSR_IA32_XSS, vcpu->arch.ia32_xss);
> }
> @@ -1043,7 +1043,7 @@ void kvm_load_host_xsave_state(struct kvm_vcpu *vcpu)
> if (vcpu->arch.xcr0 != host_xcr0)
> xsetbv(XCR_XFEATURE_ENABLED_MASK, host_xcr0);
>
> - if (vcpu->arch.xsaves_enabled &&
> + if (guest_can_use(vcpu, X86_FEATURE_XSAVES) &&
> vcpu->arch.ia32_xss != host_xss)
> wrmsrl(MSR_IA32_XSS, host_xss);
> }
"xsaves_enabled" can be removed from struct kvm_vcpu_arch as VMX/SVM doesn't reference it anymore.
next prev parent reply other threads:[~2023-08-14 6:30 UTC|newest]
Thread overview: 27+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-29 1:15 [PATCH v2 00/21] KVM: x86: Add "governed" X86_FEATURE framework Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 01/21] KVM: nSVM: Check instead of asserting on nested TSC scaling support Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 02/21] KVM: nSVM: Load L1's TSC multiplier based on L1 state, not L2 state Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 03/21] KVM: nSVM: Use the "outer" helper for writing multiplier to MSR_AMD64_TSC_RATIO Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 04/21] KVM: SVM: Clean up preemption toggling related " Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 05/21] KVM: x86: Always write vCPU's current TSC offset/ratio in vendor hooks Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 06/21] KVM: nSVM: Skip writes to MSR_AMD64_TSC_RATIO if guest state isn't loaded Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 07/21] KVM: x86: Add a framework for enabling KVM-governed x86 features Sean Christopherson
2023-08-14 4:43 ` Zeng Guang
2023-08-14 17:20 ` Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 08/21] KVM: x86/mmu: Use KVM-governed feature framework to track "GBPAGES enabled" Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 09/21] KVM: VMX: Recompute "XSAVES enabled" only after CPUID update Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 10/21] KVM: VMX: Check KVM CPU caps, not just VMX MSR support, for XSAVE enabling Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 11/21] KVM: VMX: Rename XSAVES control to follow KVM's preferred "ENABLE_XYZ" Sean Christopherson
2023-07-29 1:15 ` [PATCH v2 12/21] KVM: x86: Use KVM-governed feature framework to track "XSAVES enabled" Sean Christopherson
2023-08-14 6:28 ` Zeng Guang [this message]
2023-07-29 1:16 ` [PATCH v2 13/21] KVM: nVMX: Use KVM-governed feature framework to track "nested VMX enabled" Sean Christopherson
2023-08-14 8:11 ` Yuan Yao
2023-07-29 1:16 ` [PATCH v2 14/21] KVM: nSVM: Use KVM-governed feature framework to track "NRIPS enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 15/21] KVM: nSVM: Use KVM-governed feature framework to track "TSC scaling enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 16/21] KVM: nSVM: Use KVM-governed feature framework to track "vVM{SAVE,LOAD} enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 17/21] KVM: nSVM: Use KVM-governed feature framework to track "LBRv enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 18/21] KVM: nSVM: Use KVM-governed feature framework to track "Pause Filter enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 19/21] KVM: nSVM: Use KVM-governed feature framework to track "vGIF enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 20/21] KVM: nSVM: Use KVM-governed feature framework to track "vNMI enabled" Sean Christopherson
2023-07-29 1:16 ` [PATCH v2 21/21] KVM: x86: Disallow guest CPUID lookups when IRQs are disabled Sean Christopherson
2023-08-04 0:40 ` [PATCH v2 00/21] KVM: x86: Add "governed" X86_FEATURE framework Sean Christopherson
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=fa15cd52-b10a-6aad-d63f-3d809d16f591@intel.com \
--to=guang.zeng@intel.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mlevitsk@redhat.com \
--cc=pbonzini@redhat.com \
--cc=seanjc@google.com \
--cc=vkuznets@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox