diff mbox series

[4/6] kvm: svm: Enumerate XSAVES in guest CPUID when it is available to the guest

Message ID 20191009004142.225377-4-aaronlewis@google.com (mailing list archive)
State New, archived
Headers show
Series [1/6] KVM: VMX: Remove unneeded check for X86_FEATURE_XSAVE | expand

Commit Message

Aaron Lewis Oct. 9, 2019, 12:41 a.m. UTC 
Add the function guest_cpuid_set() to allow a bit in the guest cpuid to
be set.  This is complementary to the guest_cpuid_clear() function.

Also, set the XSAVES bit in the guest cpuid if the host has the same bit
set and guest has XSAVE bit set.  This is to ensure that XSAVES will be
enumerated in the guest CPUID if XSAVES can be used in the guest.

Reviewed-by: Jim Mattson <jmattson@google.com>
Signed-off-by: Aaron Lewis <aaronlewis@google.com>
---
 arch/x86/kvm/cpuid.h | 9 +++++++++
 arch/x86/kvm/svm.c   | 4 ++++
 2 files changed, 13 insertions(+)

Comments

Yang, Weijiang Oct. 9, 2019, 1:42 a.m. UTC | #1 
On Tue, Oct 08, 2019 at 05:41:40PM -0700, Aaron Lewis wrote:
> Add the function guest_cpuid_set() to allow a bit in the guest cpuid to
> be set.  This is complementary to the guest_cpuid_clear() function.
> 
> Also, set the XSAVES bit in the guest cpuid if the host has the same bit
> set and guest has XSAVE bit set.  This is to ensure that XSAVES will be
> enumerated in the guest CPUID if XSAVES can be used in the guest.
> 
> Reviewed-by: Jim Mattson <jmattson@google.com>
> Signed-off-by: Aaron Lewis <aaronlewis@google.com>
> ---
>  arch/x86/kvm/cpuid.h | 9 +++++++++
>  arch/x86/kvm/svm.c   | 4 ++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
> index d78a61408243..420ceea02fd1 100644
> --- a/arch/x86/kvm/cpuid.h
> +++ b/arch/x86/kvm/cpuid.h
> @@ -113,6 +113,15 @@ static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x8
>  		*reg &= ~bit(x86_feature);
>  }
>  
> +static __always_inline void guest_cpuid_set(struct kvm_vcpu *vcpu, unsigned x86_feature)
> +{
> +	int *reg;
> +
> +	reg = guest_cpuid_get_register(vcpu, x86_feature);
> +	if (reg)
> +		*reg |= ~bit(x86_feature);
> +}
> +
Sounds like it's to set the bit, is it: *reg |= bit(x86_feature)?
>  static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu)
>  {
>  	struct kvm_cpuid_entry2 *best;
> diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
> index 65223827c675..2522a467bbc0 100644
> --- a/arch/x86/kvm/svm.c
> +++ b/arch/x86/kvm/svm.c
> @@ -5887,6 +5887,10 @@ static void svm_cpuid_update(struct kvm_vcpu *vcpu)
>  {
>  	struct vcpu_svm *svm = to_svm(vcpu);
>  
> +	if (guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> +	    boot_cpu_has(X86_FEATURE_XSAVES))
> +		guest_cpuid_set(vcpu, X86_FEATURE_XSAVES);
> +
>  	/* Update nrips enabled cache */
>  	svm->nrips_enabled = !!guest_cpuid_has(&svm->vcpu, X86_FEATURE_NRIPS);
>  
> -- 
> 2.23.0.581.g78d2f28ef7-goog
Paolo Bonzini Oct. 9, 2019, 6:32 a.m. UTC | #2 
On 09/10/19 03:42, Yang Weijiang wrote:
> +	if (guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> +	    boot_cpu_has(X86_FEATURE_XSAVES))
> +		guest_cpuid_set(vcpu, X86_FEATURE_XSAVES);
> +

This is incorrect, as it would cause a change in the guest ABI when
migrating from an XSAVES-enabled processor to one that doesn't have it.

As long as IA32_XSS is 0, XSAVES is indistinguishable from XSAVEC, so
it's okay if the guest "tries" to run it despite the bit being clear in
CPUID.

Paolo
Jim Mattson Oct. 9, 2019, 11:35 p.m. UTC | #3 
On Tue, Oct 8, 2019 at 11:32 PM Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 09/10/19 03:42, Yang Weijiang wrote:
> > +     if (guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
> > +         boot_cpu_has(X86_FEATURE_XSAVES))
> > +             guest_cpuid_set(vcpu, X86_FEATURE_XSAVES);
> > +
>
> This is incorrect, as it would cause a change in the guest ABI when
> migrating from an XSAVES-enabled processor to one that doesn't have it.
>
> As long as IA32_XSS is 0, XSAVES is indistinguishable from XSAVEC, so
> it's okay if the guest "tries" to run it despite the bit being clear in
> CPUID.

My bad. When Aaron and I were discussing this, I expressed concern
about guest behavior on a future day when (a) AMD supports a non-zero
IA32_XSS, and (b) Linux supports an intercepting non-zero IA32_XSS.
One could argue that if XSAVES isn't enumerated, the guest gets what
it deserves for trying it anyway. Or, one could argue that the
decision to swap guest/host IA32_XSS values on VM-entry/VM-exit
shouldn't be predicated on whether or not the guest CPUID enumerates
XSAVES. I'm going to suggest the latter. How would you feel about
having {vmx,svm}_cpuid_update set a boolean in kvm_vcpu_arch that
indicates whether or not the guest can execute XSAVES, regardless of
what is enumerated in the guest CPUID? The wrmsr(IA32_XSS) in the
VM-entry/VM-exit path would then query that boolean rather than
guest_cpuid_has(XSAVES).
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index d78a61408243..420ceea02fd1 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -113,6 +113,15 @@  static __always_inline void guest_cpuid_clear(struct kvm_vcpu *vcpu, unsigned x8
 		*reg &= ~bit(x86_feature);
 }
 
+static __always_inline void guest_cpuid_set(struct kvm_vcpu *vcpu, unsigned x86_feature)
+{
+	int *reg;
+
+	reg = guest_cpuid_get_register(vcpu, x86_feature);
+	if (reg)
+		*reg |= ~bit(x86_feature);
+}
+
 static inline bool guest_cpuid_is_amd(struct kvm_vcpu *vcpu)
 {
 	struct kvm_cpuid_entry2 *best;
diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 65223827c675..2522a467bbc0 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -5887,6 +5887,10 @@  static void svm_cpuid_update(struct kvm_vcpu *vcpu)
 {
 	struct vcpu_svm *svm = to_svm(vcpu);
 
+	if (guest_cpuid_has(vcpu, X86_FEATURE_XSAVE) &&
+	    boot_cpu_has(X86_FEATURE_XSAVES))
+		guest_cpuid_set(vcpu, X86_FEATURE_XSAVES);
+
 	/* Update nrips enabled cache */
 	svm->nrips_enabled = !!guest_cpuid_has(&svm->vcpu, X86_FEATURE_NRIPS);