diff mbox series

[v6,09/25] KVM: x86: Rework cpuid_get_supported_xcr0() to operate on vCPU data

Message ID 20230914063325.85503-10-weijiang.yang@intel.com (mailing list archive)
State New, archived
Headers show
Series Enable CET Virtualization | expand

Commit Message

Yang, Weijiang Sept. 14, 2023, 6:33 a.m. UTC 
From: Sean Christopherson <seanjc@google.com>

Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
state, i.e. on a vCPU's CPUID state.  Prior to commit 275a87244ec8 ("KVM:
x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM
incorrectly fudged guest CPUID at runtime, which in turn necessitated
massaging the incoming CPUID state for KVM_SET_CPUID{2} so as not to run
afoul of kvm_cpuid_check_equal().

Opportunistically move the helper below kvm_update_cpuid_runtime() to make
it harder to repeat the mistake of querying supported XCR0 for runtime
updates.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
---
 arch/x86/kvm/cpuid.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

Comments

Maxim Levitsky Oct. 31, 2023, 5:46 p.m. UTC | #1 
On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> From: Sean Christopherson <seanjc@google.com>
> 
> Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> state, i.e. on a vCPU's CPUID state.  Prior to commit 275a87244ec8 ("KVM:
> x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM
> incorrectly fudged guest CPUID at runtime,
Can you explain how commit 275a87244ec8 relates to this patch?


>  which in turn necessitated
> massaging the incoming CPUID state for KVM_SET_CPUID{2} so as not to run
> afoul of kvm_cpuid_check_equal().

Can you link the commit that added this 'massaging' and explain on how this relates to this patch?

Can you explain what is the problem that this patch is trying to solve?


Is it really allowed in x86 spec to have different supported mask of XCR0 bits
on different CPUs (assuming all CPUs of the same type)?

If true, does KVM supports it?

Assuming that the answer to the above questions is no, won't this patch make it easier
to break this rule and thus make it easier to introduce a bug?

Best regards,
	Maxim Levitsky

> 
> Opportunistically move the helper below kvm_update_cpuid_runtime() to make
> it harder to repeat the mistake of querying supported XCR0 for runtime
> updates.

> 
> No functional change intended.
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Yang Weijiang <weijiang.yang@intel.com>
> ---
>  arch/x86/kvm/cpuid.c | 33 ++++++++++++++++-----------------
>  1 file changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 0544e30b4946..7c3e4a550ca7 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -247,21 +247,6 @@ void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
>  		vcpu->arch.pv_cpuid.features = best->eax;
>  }
>  
> -/*
> - * Calculate guest's supported XCR0 taking into account guest CPUID data and
> - * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
> - */
> -static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent)
> -{
> -	struct kvm_cpuid_entry2 *best;
> -
> -	best = cpuid_entry2_find(entries, nent, 0xd, 0);
> -	if (!best)
> -		return 0;
> -
> -	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
> -}
> -
>  static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
>  				       int nent)
>  {
> @@ -312,6 +297,21 @@ void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
>  }
>  EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
>  
> +/*
> + * Calculate guest's supported XCR0 taking into account guest CPUID data and
> + * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
> + */
> +static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu)
> +{
> +	struct kvm_cpuid_entry2 *best;
> +
> +	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0);
> +	if (!best)
> +		return 0;
> +
> +	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
> +}
> +
>  static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
>  {
>  	struct kvm_cpuid_entry2 *entry;
> @@ -357,8 +357,7 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
>  		kvm_apic_set_version(vcpu);
>  	}
>  
> -	vcpu->arch.guest_supported_xcr0 =
> -		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
> +	vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu);
>  
>  	/*
>  	 * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if
Sean Christopherson Nov. 1, 2023, 2:41 p.m. UTC | #2 
On Tue, Oct 31, 2023, Maxim Levitsky wrote:
> On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > 
> > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> > state, i.e. on a vCPU's CPUID state.  Prior to commit 275a87244ec8 ("KVM:
> > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM
> > incorrectly fudged guest CPUID at runtime,
> Can you explain how commit 275a87244ec8 relates to this patch?
>
> > which in turn necessitated massaging the incoming CPUID state for
> > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().
> 
> Can you link the commit that added this 'massaging' and explain on how this
> relates to this patch?

It's commit 275a87244ec8, which is right above.  I think the missing part is an
explicit call out that the massaging used cpuid_get_supported_xcr0() with the
incoming "struct kvm_cpuid_entry2", i.e. without a "struct kvm_vcpu".

> Can you explain what is the problem that this patch is trying to solve?

Is this better?

--
Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
state, i.e. on a vCPU's CPUID state, now that the only usage of the helper
is to retrieve a vCPU's already-set CPUID.

Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1
(allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at
runtime, which in turn necessitated massaging the incoming CPUID state for
KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().  I.e.
KVM also invoked cpuid_get_supported_xcr0() with the incoming CPUID state,
and thus without an explicit vCPU object.
--

> Is it really allowed in x86 spec to have different supported mask of XCR0 bits
> on different CPUs (assuming all CPUs of the same type)?

Yes, nothing in the SDM explicitly states that all cores in have identical feature
sets.  And "assuming all CPUs of the same type" isn't really a valid constraint
because it's very doable to put different SKUs into a multi-socket system.

Intel even (somewhat inadvertantly) kinda sorta shipped such CPUs, as Alder Lake
P-cores support AVX512 but E-cores do not, and IIRC early (pre-production?) BIOS
didn't disable AVX512 on the P-Cores, i.e. software could observe cores with and
without AVX512.  That quickly got fixed because it confused software, but until
Intel squashed AVX512 entirely with a microcode update, disabling E-Cores in BIOS
would effectively enable AVX512 on the remaining P-Cores.

And it's not XCR0-related, but PMUs on Alder Lake (and all Intel hybrid CPUs) are
truly heterogenous.  It's a mess for virtualization, but concrete proof that there
are no architectural guarantees regarding homogeneity of feature sets.

> If true, does KVM supports it?

Yes.  Whether or not that's a good thing is definitely debatle, bug KVM's ABI for
a very long time has allowed userspace to expose whatever it wants via KVM_SET_CPUID.

Getting (guest) software to play nice is an entirely different matter, but exposing
heterogenous vCPUs isn't an architectural violation.
Maxim Levitsky Nov. 2, 2023, 6:25 p.m. UTC | #3 
On Wed, 2023-11-01 at 07:41 -0700, Sean Christopherson wrote:
> On Tue, Oct 31, 2023, Maxim Levitsky wrote:
> > On Thu, 2023-09-14 at 02:33 -0400, Yang Weijiang wrote:
> > > From: Sean Christopherson <seanjc@google.com>
> > > 
> > > Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> > > state, i.e. on a vCPU's CPUID state.  Prior to commit 275a87244ec8 ("KVM:
> > > x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)"), KVM
> > > incorrectly fudged guest CPUID at runtime,
> > Can you explain how commit 275a87244ec8 relates to this patch?
> > 
> > > which in turn necessitated massaging the incoming CPUID state for
> > > KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().
> > 
> > Can you link the commit that added this 'massaging' and explain on how this
> > relates to this patch?
> 
> It's commit 275a87244ec8, which is right above.  I think the missing part is an
> explicit call out that the massaging used cpuid_get_supported_xcr0() with the
> incoming "struct kvm_cpuid_entry2", i.e. without a "struct kvm_vcpu".
> 
> > Can you explain what is the problem that this patch is trying to solve?
> 
> Is this better?
> 
> --
> Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU
> state, i.e. on a vCPU's CPUID state, now that the only usage of the helper
> is to retrieve a vCPU's already-set CPUID.
> 
> Prior to commit 275a87244ec8 ("KVM: x86: Don't adjust guest's CPUID.0x12.1
> (allowed SGX enclave XFRM)"), KVM incorrectly fudged guest CPUID at
> runtime, which in turn necessitated massaging the incoming CPUID state for
> KVM_SET_CPUID{2} so as not to run afoul of kvm_cpuid_check_equal().  I.e.
> KVM also invoked cpuid_get_supported_xcr0() with the incoming CPUID state,
> and thus without an explicit vCPU object.

Ah, I understand you. I incorrectly assumed that KVM doesn't allow different CPUID
on different vCPUs, while the actual restriction that was recently placed was
to not allow changing a vCPU's CPUID, once a vCPU was in a guest mode once.

I also understand what you mean in regard to the commit 275a87244ec8 but IMHO
this part of the commit message is only adding to the confusion.

I think that this will be a better commit message:


"Rework and rename cpuid_get_supported_xcr0() to explicitly operate on vCPU state
i.e. on a vCPU's CPUID state.

This is needed because KVM permits different vCPUs to have different CPUIDs, 
and thus it is valid for each vCPU to have different set of supported bits in the XCR0"

> --
> 
> > Is it really allowed in x86 spec to have different supported mask of XCR0 bits
> > on different CPUs (assuming all CPUs of the same type)?
> 
> Yes, nothing in the SDM explicitly states that all cores in have identical feature
> sets.  And "assuming all CPUs of the same type" isn't really a valid constraint
> because it's very doable to put different SKUs into a multi-socket system.
> 
> Intel even (somewhat inadvertantly) kinda sorta shipped such CPUs, as Alder Lake
> P-cores support AVX512 but E-cores do not, and IIRC early (pre-production?) BIOS
> didn't disable AVX512 on the P-Cores, i.e. software could observe cores with and
> without AVX512.  That quickly got fixed because it confused software, but until
> Intel squashed AVX512 entirely with a microcode update, disabling E-Cores in BIOS
> would effectively enable AVX512 on the remaining P-Cores.

Yea, sure I know about this, that's why I said "same type". I just was under the impression
that KVM doesn't 'officially' support heterogeneous vCPUs and recently added a check
to ensure that all vCPUs have the same CPUID. Now I understand.

> 
> And it's not XCR0-related, but PMUs on Alder Lake (and all Intel hybrid CPUs) are
> truly heterogenous.  It's a mess for virtualization, but concrete proof that there
> are no architectural guarantees regarding homogeneity of feature sets.
> 
> > If true, does KVM supports it?
> 
> Yes.  Whether or not that's a good thing is definitely debatle, bug KVM's ABI for
> a very long time has allowed userspace to expose whatever it wants via KVM_SET_CPUID.

> 
> Getting (guest) software to play nice is an entirely different matter, but exposing
> heterogenous vCPUs isn't an architectural violation.
> 

Best regards,
	Maxim Levitsky
diff mbox series

Patch

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 0544e30b4946..7c3e4a550ca7 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -247,21 +247,6 @@  void kvm_update_pv_runtime(struct kvm_vcpu *vcpu)
 		vcpu->arch.pv_cpuid.features = best->eax;
 }
 
-/*
- * Calculate guest's supported XCR0 taking into account guest CPUID data and
- * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
- */
-static u64 cpuid_get_supported_xcr0(struct kvm_cpuid_entry2 *entries, int nent)
-{
-	struct kvm_cpuid_entry2 *best;
-
-	best = cpuid_entry2_find(entries, nent, 0xd, 0);
-	if (!best)
-		return 0;
-
-	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
-}
-
 static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_entry2 *entries,
 				       int nent)
 {
@@ -312,6 +297,21 @@  void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
 }
 EXPORT_SYMBOL_GPL(kvm_update_cpuid_runtime);
 
+/*
+ * Calculate guest's supported XCR0 taking into account guest CPUID data and
+ * KVM's supported XCR0 (comprised of host's XCR0 and KVM_SUPPORTED_XCR0).
+ */
+static u64 vcpu_get_supported_xcr0(struct kvm_vcpu *vcpu)
+{
+	struct kvm_cpuid_entry2 *best;
+
+	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0);
+	if (!best)
+		return 0;
+
+	return (best->eax | ((u64)best->edx << 32)) & kvm_caps.supported_xcr0;
+}
+
 static bool kvm_cpuid_has_hyperv(struct kvm_cpuid_entry2 *entries, int nent)
 {
 	struct kvm_cpuid_entry2 *entry;
@@ -357,8 +357,7 @@  static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
 		kvm_apic_set_version(vcpu);
 	}
 
-	vcpu->arch.guest_supported_xcr0 =
-		cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
+	vcpu->arch.guest_supported_xcr0 = vcpu_get_supported_xcr0(vcpu);
 
 	/*
 	 * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if