[PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

*** WARNING *** ABI breakage.

Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
for SGX enclaves.  Past me didn't understand the roles and responsibilities
between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
being helpful by having KVM adjust the entries.

This is clearly an ABI breakage, but QEMU (tries to) do the right thing,
and AFAIK no other VMMs support SGX (yet), so I'm hoping we can excise the
ugly before userspace starts depending on the bad behavior.

Compile tested only (don't have an SGX system these days).

Note, QEMU commit 301e90675c ("target/i386: Enable support for XSAVES
based features") completely broke SGX by using allowed XSS instead of
XCR0, and no one has complained.  That gives me hope that this one will
go through as well.

I belive the QEMU fix is below.  I'll post a patch at some point unless
someone wants to do the dirty work and claim the patch as their own.

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 6576287e5b..f083ff4335 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -5718,8 +5718,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
         } else {
             *eax &= env->features[FEAT_SGX_12_1_EAX];
             *ebx &= 0; /* ebx reserve */
-            *ecx &= env->features[FEAT_XSAVE_XSS_LO];
-            *edx &= env->features[FEAT_XSAVE_XSS_HI];
+            *ecx &= env->features[FEAT_XSAVE_XCR0_LO];
+            *edx &= env->features[FEAT_XSAVE_XCR0_HI];
 
             /* FP and SSE are always allowed regardless of XSAVE/XCR0. */
             *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK;

Sean Christopherson (3):
  KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
    ECREATE
  KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
  KVM: x86: Open code supported XCR0 calculation in
    kvm_vcpu_after_set_cpuid()

 arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
 arch/x86/kvm/vmx/sgx.c |  3 ++-
 2 files changed, 12 insertions(+), 34 deletions(-)


base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
-- 
2.40.0.348.gf938b09366-goog

[PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Sean Christopherson]

Explicitly check the vCPU's supported XCR0 when determining whether or not
the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
XCR0, but that is rather subtle and KVM should not modify guest CPUID
except for modeling true runtime behavior (allowed XFRM is most definitely
not "runtime" behavior).

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/sgx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
index aa53c98034bf..362a31b19b0e 100644
--- a/arch/x86/kvm/vmx/sgx.c
+++ b/arch/x86/kvm/vmx/sgx.c
@@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
 	    (u32)attributes & ~sgx_12_1->eax ||
 	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
 	    (u32)xfrm & ~sgx_12_1->ecx ||
-	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
+	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
+	    xfrm & ~vcpu->arch.guest_supported_xcr0) {
 		kvm_inject_gp(vcpu, 0);
 		return 1;
 	}
-- 
2.40.0.348.gf938b09366-goog

[PATCH 2/3] KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)

[Sean Christopherson]

Drop KVM's manipulation of guest's CPUID.0x12.1 ECX and EDX, i.e. the
allowed XFRM of SGX enclaves, now that KVM explicitly checks the guest's
allowed XCR0 when emulating ECREATE.

Note, this could theoretically break a setup where userspace advertises
a "bad" XFRM and relies on KVM to provide a sane CPUID model, but QEMU
is the only known user of KVM SGX, and QEMU explicitly sets the SGX CPUID
XFRM subleaf based on the guest's XCR0.

Cc: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index 6972e0be60fa..d28c4fb14d43 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -253,7 +253,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 				       int nent)
 {
 	struct kvm_cpuid_entry2 *best;
-	u64 guest_supported_xcr0 = cpuid_get_supported_xcr0(entries, nent);
 
 	best = cpuid_entry2_find(entries, nent, 1, KVM_CPUID_INDEX_NOT_SIGNIFICANT);
 	if (best) {
@@ -292,21 +291,6 @@ static void __kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu, struct kvm_cpuid_e
 					   vcpu->arch.ia32_misc_enable_msr &
 					   MSR_IA32_MISC_ENABLE_MWAIT);
 	}
-
-	/*
-	 * Bits 127:0 of the allowed SECS.ATTRIBUTES (CPUID.0x12.0x1) enumerate
-	 * the supported XSAVE Feature Request Mask (XFRM), i.e. the enclave's
-	 * requested XCR0 value.  The enclave's XFRM must be a subset of XCRO
-	 * at the time of EENTER, thus adjust the allowed XFRM by the guest's
-	 * supported XCR0.  Similar to XCR0 handling, FP and SSE are forced to
-	 * '1' even on CPUs that don't support XSAVE.
-	 */
-	best = cpuid_entry2_find(entries, nent, 0x12, 0x1);
-	if (best) {
-		best->ecx &= guest_supported_xcr0 & 0xffffffff;
-		best->edx &= guest_supported_xcr0 >> 32;
-		best->ecx |= XFEATURE_MASK_FPSSE;
-	}
 }
 
 void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu)
-- 
2.40.0.348.gf938b09366-goog

[PATCH 3/3] KVM: x86: Open code supported XCR0 calculation in kvm_vcpu_after_set_cpuid()

[Sean Christopherson]

Drop cpuid_get_supported_xcr0() now that its bastardized usage in
__kvm_update_cpuid_runtime() is gone, and open code the logic in its sole
caller, kvm_vcpu_after_set_cpuid().

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/cpuid.c | 27 ++++++++++-----------------
 1 file changed, 10 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
index d28c4fb14d43..220eda4ab337 100644
--- a/arch/x86/kvm/cpuid.c
+++ b/arch/x86/kvm/cpuid.c
@@ -234,21 +234,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)
 {
@@ -323,8 +308,16 @@ 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);
+	/*
+	 * 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).
+	 */
+	best = kvm_find_cpuid_entry_index(vcpu, 0xd, 0);
+	if (!best)
+		vcpu->arch.guest_supported_xcr0 = 0;
+	else
+		vcpu->arch.guest_supported_xcr0 = (best->eax | ((u64)best->edx << 32)) &
+						  kvm_caps.supported_xcr0;
 
 	/*
 	 * FP+SSE can always be saved/restored via KVM_{G,S}ET_XSAVE, even if
-- 
2.40.0.348.gf938b09366-goog

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> *** WARNING *** ABI breakage.
> 
> Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> for SGX enclaves.  Past me didn't understand the roles and responsibilities
> between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> being helpful by having KVM adjust the entries.
> 
> This is clearly an ABI breakage, but QEMU (tries to) do the right thing,
> and AFAIK no other VMMs support SGX (yet), so I'm hoping we can excise the
> ugly before userspace starts depending on the bad behavior.
> 
> Compile tested only (don't have an SGX system these days).

I'll look into this, and at the meantime ...

> 
> Note, QEMU commit 301e90675c ("target/i386: Enable support for XSAVES
> based features") completely broke SGX by using allowed XSS instead of
> XCR0, and no one has complained.  That gives me hope that this one will
> go through as well.

...

Actually we got complain around half year ago:

https://github.com/gramineproject/gramine/issues/955#issuecomment-1272829510

> 
> I belive the QEMU fix is below.  I'll post a patch at some point unless
> someone wants to do the dirty work and claim the patch as their own.
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6576287e5b..f083ff4335 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5718,8 +5718,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          } else {
>              *eax &= env->features[FEAT_SGX_12_1_EAX];
>              *ebx &= 0; /* ebx reserve */
> -            *ecx &= env->features[FEAT_XSAVE_XSS_LO];
> -            *edx &= env->features[FEAT_XSAVE_XSS_HI];
> +            *ecx &= env->features[FEAT_XSAVE_XCR0_LO];
> +            *edx &= env->features[FEAT_XSAVE_XCR0_HI];
>  
>              /* FP and SSE are always allowed regardless of XSAVE/XCR0. */
>              *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK;

And since then Yang posted a patch to Qemu mailing list to fix:

https://lists.nongnu.org/archive/html/qemu-devel/2022-10/msg04990.html

I thought it had been merged, but it seems it hasn't :)

> 
> Sean Christopherson (3):
>   KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
>     ECREATE
>   KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
>   KVM: x86: Open code supported XCR0 calculation in
>     kvm_vcpu_after_set_cpuid()
> 
>  arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
>  arch/x86/kvm/vmx/sgx.c |  3 ++-
>  2 files changed, 12 insertions(+), 34 deletions(-)
> 
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> *** WARNING *** ABI breakage.
> 
> Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> for SGX enclaves.  Past me didn't understand the roles and responsibilities
> between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> being helpful by having KVM adjust the entries.

Actually I am not clear about this topic.

So the rule is KVM should never adjust CPUID entries passed from userspace?

What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
those CPUID entries to ensure there's no insane configuration?  My concern is if
we allow guest to be created with insane CPUID configurations, the guest can be
confused and behaviour unexpectedly.

> 
> This is clearly an ABI breakage, but QEMU (tries to) do the right thing,
> and AFAIK no other VMMs support SGX (yet), so I'm hoping we can excise the
> ugly before userspace starts depending on the bad behavior.

I wouldn't worry about userspace being broken, because, IIUC, such broken can
only happen when userspace doesn't do the right thing (i.e. it sets SGX CPUID
0x12,0x1 to have more bits than the XCR0).

> 
> Compile tested only (don't have an SGX system these days).
> 
> Note, QEMU commit 301e90675c ("target/i386: Enable support for XSAVES
> based features") completely broke SGX by using allowed XSS instead of
> XCR0, and no one has complained.  That gives me hope that this one will
> go through as well.
> 
> I belive the QEMU fix is below.  I'll post a patch at some point unless
> someone wants to do the dirty work and claim the patch as their own.
> 
> diff --git a/target/i386/cpu.c b/target/i386/cpu.c
> index 6576287e5b..f083ff4335 100644
> --- a/target/i386/cpu.c
> +++ b/target/i386/cpu.c
> @@ -5718,8 +5718,8 @@ void cpu_x86_cpuid(CPUX86State *env, uint32_t index, uint32_t count,
>          } else {
>              *eax &= env->features[FEAT_SGX_12_1_EAX];
>              *ebx &= 0; /* ebx reserve */
> -            *ecx &= env->features[FEAT_XSAVE_XSS_LO];
> -            *edx &= env->features[FEAT_XSAVE_XSS_HI];
> +            *ecx &= env->features[FEAT_XSAVE_XCR0_LO];
> +            *edx &= env->features[FEAT_XSAVE_XCR0_HI];
>  
>              /* FP and SSE are always allowed regardless of XSAVE/XCR0. */
>              *ecx |= XSTATE_FP_MASK | XSTATE_SSE_MASK;
> 
> Sean Christopherson (3):
>   KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for
>     ECREATE
>   KVM: x86: Don't adjust guest's CPUID.0x12.1 (allowed SGX enclave XFRM)
>   KVM: x86: Open code supported XCR0 calculation in
>     kvm_vcpu_after_set_cpuid()
> 
>  arch/x86/kvm/cpuid.c   | 43 ++++++++++--------------------------------
>  arch/x86/kvm/vmx/sgx.c |  3 ++-
>  2 files changed, 12 insertions(+), 34 deletions(-)
> 
> 
> base-commit: 27d6845d258b67f4eb3debe062b7dacc67e0c393
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Huang, Kai]

On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> Explicitly check the vCPU's supported XCR0 when determining whether or not
> the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
> guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
> XCR0, but that is rather subtle and KVM should not modify guest CPUID
> except for modeling true runtime behavior (allowed XFRM is most definitely
> not "runtime" behavior).
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/sgx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> index aa53c98034bf..362a31b19b0e 100644
> --- a/arch/x86/kvm/vmx/sgx.c
> +++ b/arch/x86/kvm/vmx/sgx.c
> @@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
>  	    (u32)attributes & ~sgx_12_1->eax ||
>  	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
>  	    (u32)xfrm & ~sgx_12_1->ecx ||
> -	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> +	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
> +	    xfrm & ~vcpu->arch.guest_supported_xcr0) {

Perhaps this change is needed even without patch 2?

This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0.  But
when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and
sgx_12_1->ecx is always set to 0x3.  

__handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit
check xfrm against guest_supported_xcr0, it seems guest can successfully run
ECREATE when it doesn't have CPUID 0xD?


>  		kvm_inject_gp(vcpu, 0);
>  		return 1;
>  	}
> -- 
> 2.40.0.348.gf938b09366-goog
> 

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Sean Christopherson]

On Wed, Apr 05, 2023, Huang, Kai wrote:
> On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > Explicitly check the vCPU's supported XCR0 when determining whether or not
> > the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
> > guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
> > XCR0, but that is rather subtle and KVM should not modify guest CPUID
> > except for modeling true runtime behavior (allowed XFRM is most definitely
> > not "runtime" behavior).
> > 
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >  arch/x86/kvm/vmx/sgx.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > index aa53c98034bf..362a31b19b0e 100644
> > --- a/arch/x86/kvm/vmx/sgx.c
> > +++ b/arch/x86/kvm/vmx/sgx.c
> > @@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
> >  	    (u32)attributes & ~sgx_12_1->eax ||
> >  	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
> >  	    (u32)xfrm & ~sgx_12_1->ecx ||
> > -	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> > +	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
> > +	    xfrm & ~vcpu->arch.guest_supported_xcr0) {
> 
> Perhaps this change is needed even without patch 2?
> 
> This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0.  But
> when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and
> sgx_12_1->ecx is always set to 0x3.

Hrm, that's a bug in this patch.  Drat.  More below.

> __handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit
> check xfrm against guest_supported_xcr0, it seems guest can successfully run
> ECREATE when it doesn't have CPUID 0xD?

ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE.  This exact scenario
is called out in the SDM:

  Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
    * XFRM[1:0] must be set to 0x3.
    * If the processor does support XSAVE, XFRM must contain a value that would
      be legal if loaded into XCR0.
    * If the processor does not support XSAVE, or if the system software has not
      enabled XSAVE, then XFRM[63:2] must be zero.

So the above needs to be either

	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)

or

	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)


I think I prefer the first one as I find it slightly more obvious that FP+SSE are
allowed in addition to the XCR0 bits.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

On Wed, Apr 05, 2023, Huang, Kai wrote:
> On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > *** WARNING *** ABI breakage.
> > 
> > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > being helpful by having KVM adjust the entries.
> 
> Actually I am not clear about this topic.
> 
> So the rule is KVM should never adjust CPUID entries passed from userspace?

Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.

> What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> those CPUID entries to ensure there's no insane configuration?  My concern is if
> we allow guest to be created with insane CPUID configurations, the guest can be
> confused and behaviour unexpectedly.

It is userspace's responsibility to provide a sane, correct setup.  The one
exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
unsupported virtual address width, the argument being that a malicious userspace
could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
VMCS field.

The reason for KVM punting to userspace is that it's all but impossible to define
what is/isn't sane.  A really good example would be an alternative we (Google)
considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
miss reserved bit #PFs.

Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
anticipation of eventual migration.  So long as userspace doesn't actually enumerate
memslots in the illegal address space, KVM would be able to treat such accesses as
emulated MMIO, and would only need to intercept #PF(RSVD).

Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
definitely qualifies as insane since it really can't work correctly, but in our
opinion it was far superior to running with allow_smaller_maxphyaddr=true.

And sane is not the same thing as architecturally legal.  AMX is a good example
of this.  It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but
not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG
in XCR0 without also enabling TILE_DATA.  KVM should arguably reject CPUID configs
with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100%
architecturally valid, if insane, CPUID configuration.  Ditto for nearly all of
the VMX control bits versus their CPUID counterparts.

And sometimes there are good reasons to run a VM with a truly insane configuration,
e.g. for testing purposes.

TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Huang, Kai]

On Wed, 2023-04-05 at 18:44 -0700, Sean Christopherson wrote:
> On Wed, Apr 05, 2023, Huang, Kai wrote:
> > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > Explicitly check the vCPU's supported XCR0 when determining whether or not
> > > the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
> > > guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
> > > XCR0, but that is rather subtle and KVM should not modify guest CPUID
> > > except for modeling true runtime behavior (allowed XFRM is most definitely
> > > not "runtime" behavior).
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---
> > >  arch/x86/kvm/vmx/sgx.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > > index aa53c98034bf..362a31b19b0e 100644
> > > --- a/arch/x86/kvm/vmx/sgx.c
> > > +++ b/arch/x86/kvm/vmx/sgx.c
> > > @@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
> > >  	    (u32)attributes & ~sgx_12_1->eax ||
> > >  	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
> > >  	    (u32)xfrm & ~sgx_12_1->ecx ||
> > > -	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> > > +	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
> > > +	    xfrm & ~vcpu->arch.guest_supported_xcr0) {
> > 
> > Perhaps this change is needed even without patch 2?
> > 
> > This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0.  But
> > when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and
> > sgx_12_1->ecx is always set to 0x3.
> 
> Hrm, that's a bug in this patch.  Drat.  More below.
> 
> > __handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit
> > check xfrm against guest_supported_xcr0, it seems guest can successfully run
> > ECREATE when it doesn't have CPUID 0xD?
> 
> ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE.  This exact scenario
> is called out in the SDM:
> 
>   Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
>     * XFRM[1:0] must be set to 0x3.
>     * If the processor does support XSAVE, XFRM must contain a value that would
>       be legal if loaded into XCR0.
>     * If the processor does not support XSAVE, or if the system software has not
>       enabled XSAVE, then XFRM[63:2] must be zero.
> 
> So the above needs to be either
> 
> 	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)
> 
> or
> 
> 	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)
> 
> 
> I think I prefer the first one as I find it slightly more obvious that FP+SSE are
> allowed in addition to the XCR0 bits.

The above check doesn't verify xfrm is a super set of 0x3.  I think we verify
that per SDM:

39.7.3 Processor Extended States and ENCLS[ECREATE]

The ECREATE leaf function of the ENCLS instruction enforces a number of
consistency checks described earlier. The execution of ENCLS[ECREATE] leaf
function results in a #GP(0) in any of the following cases:
  • SECS.ATTRIBUTES.XFRM[1:0] is not 3.
  • The processor does not support XSAVE and any of the following is true:
	— SECS.ATTRIBUTES.XFRM[63:2] is not 0.
	— SECS.SSAFRAMESIZE is 0.
  • The processor supports XSAVE and any of the following is true:
	— XSETBV would fault on an attempt to load XFRM into XCR0.
	— XFRM[63]=1.
	— The SSAFRAME is too small to hold required, enabled states ...


And in the ECREATE pseudo code, the relevant parts seem to be:

	(* Check lower 2 bits of XFRM are set *)
	IF ( ( DS:TMP_SECS.ATTRIBUTES.XFRM BitwiseAND 03H) ≠ 03H)
		THEN #GP(0); FI;

	IF (XFRM is illegal)
		THEN #GP(0); FI;

The first part is clear, but the second part is vague. 

I am not sure in hardware behaviour, whether XCR0 is actually checked in
ECREATE.  It's more likely XCRO is actually checked in EENTER.  

But I think it's just fine to also check against XCR0 here.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Zhi Wang]

On Wed, 5 Apr 2023 19:10:40 -0700
Sean Christopherson <seanjc@google.com> wrote:

> On Wed, Apr 05, 2023, Huang, Kai wrote:
> > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > *** WARNING *** ABI breakage.
> > > 
> > > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > > being helpful by having KVM adjust the entries.
> > 
> > Actually I am not clear about this topic.
> > 
> > So the rule is KVM should never adjust CPUID entries passed from userspace?
> 
> Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
> CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.
> 
> > What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> > those CPUID entries to ensure there's no insane configuration?  My concern is if
> > we allow guest to be created with insane CPUID configurations, the guest can be
> > confused and behaviour unexpectedly.
> 
> It is userspace's responsibility to provide a sane, correct setup.  The one
> exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
> unsupported virtual address width, the argument being that a malicious userspace
> could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
> VMCS field.
> 
> The reason for KVM punting to userspace is that it's all but impossible to define
> what is/isn't sane.  A really good example would be an alternative we (Google)
> considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
> migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
> miss reserved bit #PFs.
> 
> Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
> was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
> anticipation of eventual migration.  So long as userspace doesn't actually enumerate
> memslots in the illegal address space, KVM would be able to treat such accesses as
> emulated MMIO, and would only need to intercept #PF(RSVD).
> 
> Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
> definitely qualifies as insane since it really can't work correctly, but in our
> opinion it was far superior to running with allow_smaller_maxphyaddr=true.
> 
> And sane is not the same thing as architecturally legal.  AMX is a good example
> of this.  It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but
> not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG
> in XCR0 without also enabling TILE_DATA.  KVM should arguably reject CPUID configs
> with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100%
> architecturally valid, if insane, CPUID configuration.  Ditto for nearly all of
> the VMX control bits versus their CPUID counterparts.
> 
> And sometimes there are good reasons to run a VM with a truly insane configuration,
> e.g. for testing purposes.
> 
> TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
It would be nice to have a conclusion of what is "sane" and what is the
proper role for KVM, as firmware/TDX module is going to validate the "sane"
CPUID.

TDX/SNP requires the CPUID to be pre-configured and validated before creating
a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
SNP_LAUNCH_UPDATE in SNP).

IIUC according to what you mentioned, KVM should be treated like "CPUID box"
for QEMU and the checks in KVM is only to ensure the requirements of a chosen
one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.

As the valid CPUIDs have been decided when creating a CC guest, what should be
the proper behavior (basically any new checks?) of KVM for the later
SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
at least, because some KVM code paths rely on guest CPUID configuration.

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Sean Christopherson]

On Thu, Apr 06, 2023, Huang, Kai wrote:
> On Wed, 2023-04-05 at 18:44 -0700, Sean Christopherson wrote:
> > On Wed, Apr 05, 2023, Huang, Kai wrote:
> > > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > > Explicitly check the vCPU's supported XCR0 when determining whether or not
> > > > the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
> > > > guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
> > > > XCR0, but that is rather subtle and KVM should not modify guest CPUID
> > > > except for modeling true runtime behavior (allowed XFRM is most definitely
> > > > not "runtime" behavior).
> > > > 
> > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > ---
> > > >  arch/x86/kvm/vmx/sgx.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > > > index aa53c98034bf..362a31b19b0e 100644
> > > > --- a/arch/x86/kvm/vmx/sgx.c
> > > > +++ b/arch/x86/kvm/vmx/sgx.c
> > > > @@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
> > > >  	    (u32)attributes & ~sgx_12_1->eax ||
> > > >  	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
> > > >  	    (u32)xfrm & ~sgx_12_1->ecx ||
> > > > -	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> > > > +	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
> > > > +	    xfrm & ~vcpu->arch.guest_supported_xcr0) {
> > > 
> > > Perhaps this change is needed even without patch 2?
> > > 
> > > This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0.  But
> > > when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and
> > > sgx_12_1->ecx is always set to 0x3.
> > 
> > Hrm, that's a bug in this patch.  Drat.  More below.
> > 
> > > __handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit
> > > check xfrm against guest_supported_xcr0, it seems guest can successfully run
> > > ECREATE when it doesn't have CPUID 0xD?
> > 
> > ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE.  This exact scenario
> > is called out in the SDM:
> > 
> >   Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
> >     * XFRM[1:0] must be set to 0x3.
> >     * If the processor does support XSAVE, XFRM must contain a value that would
> >       be legal if loaded into XCR0.
> >     * If the processor does not support XSAVE, or if the system software has not
> >       enabled XSAVE, then XFRM[63:2] must be zero.
> > 
> > So the above needs to be either
> > 
> > 	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)
> > 
> > or
> > 
> > 	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)
> > 
> > 
> > I think I prefer the first one as I find it slightly more obvious that FP+SSE are
> > allowed in addition to the XCR0 bits.
> 
> The above check doesn't verify xfrm is a super set of 0x3.  I think we verify
> that per SDM:

Oooh, right.  It's not that FP+SSE are always allowed, it's that FP+SSE must always
be _set_.  So this?

		xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
		(xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE

> 39.7.3 Processor Extended States and ENCLS[ECREATE]
> 
> The ECREATE leaf function of the ENCLS instruction enforces a number of
> consistency checks described earlier. The execution of ENCLS[ECREATE] leaf
> function results in a #GP(0) in any of the following cases:
>   • SECS.ATTRIBUTES.XFRM[1:0] is not 3.
>   • The processor does not support XSAVE and any of the following is true:
> 	— SECS.ATTRIBUTES.XFRM[63:2] is not 0.
> 	— SECS.SSAFRAMESIZE is 0.
>   • The processor supports XSAVE and any of the following is true:
> 	— XSETBV would fault on an attempt to load XFRM into XCR0.
> 	— XFRM[63]=1.
> 	— The SSAFRAME is too small to hold required, enabled states ...
> 
> 
> And in the ECREATE pseudo code, the relevant parts seem to be:
> 
> 	(* Check lower 2 bits of XFRM are set *)
> 	IF ( ( DS:TMP_SECS.ATTRIBUTES.XFRM BitwiseAND 03H) ≠ 03H)
> 		THEN #GP(0); FI;
> 
> 	IF (XFRM is illegal)
> 		THEN #GP(0); FI;
> 
> The first part is clear, but the second part is vague. 
> 
> I am not sure in hardware behaviour, whether XCR0 is actually checked in
> ECREATE.  It's more likely XCRO is actually checked in EENTER.  
> 
> But I think it's just fine to also check against XCR0 here.

ECREATE doesn't check XCR0, it checks that XFRM represents a legal XCR0 values
for the platform, which in KVM is tracked as guest_supported_xcr0.

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Huang, Kai]

On Thu, 2023-04-06 at 12:12 -0700, Sean Christopherson wrote:
> On Thu, Apr 06, 2023, Huang, Kai wrote:
> > On Wed, 2023-04-05 at 18:44 -0700, Sean Christopherson wrote:
> > > On Wed, Apr 05, 2023, Huang, Kai wrote:
> > > > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > > > Explicitly check the vCPU's supported XCR0 when determining whether or not
> > > > > the XFRM for ECREATE is valid.  Checking CPUID works because KVM updates
> > > > > guest CPUID.0x12.1 to restrict the leaf to a subset of the guest's allowed
> > > > > XCR0, but that is rather subtle and KVM should not modify guest CPUID
> > > > > except for modeling true runtime behavior (allowed XFRM is most definitely
> > > > > not "runtime" behavior).
> > > > > 
> > > > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > > ---
> > > > >  arch/x86/kvm/vmx/sgx.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/arch/x86/kvm/vmx/sgx.c b/arch/x86/kvm/vmx/sgx.c
> > > > > index aa53c98034bf..362a31b19b0e 100644
> > > > > --- a/arch/x86/kvm/vmx/sgx.c
> > > > > +++ b/arch/x86/kvm/vmx/sgx.c
> > > > > @@ -175,7 +175,8 @@ static int __handle_encls_ecreate(struct kvm_vcpu *vcpu,
> > > > >  	    (u32)attributes & ~sgx_12_1->eax ||
> > > > >  	    (u32)(attributes >> 32) & ~sgx_12_1->ebx ||
> > > > >  	    (u32)xfrm & ~sgx_12_1->ecx ||
> > > > > -	    (u32)(xfrm >> 32) & ~sgx_12_1->edx) {
> > > > > +	    (u32)(xfrm >> 32) & ~sgx_12_1->edx ||
> > > > > +	    xfrm & ~vcpu->arch.guest_supported_xcr0) {
> > > > 
> > > > Perhaps this change is needed even without patch 2?
> > > > 
> > > > This is because when CPUID 0xD doesn't exist, guest_supported_xcr0 is 0.  But
> > > > when CPUID 0xD doesn't exist, IIUC currently KVM doesn't clear SGX in CPUID, and
> > > > sgx_12_1->ecx is always set to 0x3.
> > > 
> > > Hrm, that's a bug in this patch.  Drat.  More below.
> > > 
> > > > __handle_encls_ereate() doesn't check CPUID 0xD either, so w/o above explicit
> > > > check xfrm against guest_supported_xcr0, it seems guest can successfully run
> > > > ECREATE when it doesn't have CPUID 0xD?
> > > 
> > > ECREATE doesn't have a strict dependency on CPUID 0xD or XSAVE.  This exact scenario
> > > is called out in the SDM:
> > > 
> > >   Legal values for SECS.ATTRIBUTES.XFRM conform to these requirements:
> > >     * XFRM[1:0] must be set to 0x3.
> > >     * If the processor does support XSAVE, XFRM must contain a value that would
> > >       be legal if loaded into XCR0.
> > >     * If the processor does not support XSAVE, or if the system software has not
> > >       enabled XSAVE, then XFRM[63:2] must be zero.
> > > 
> > > So the above needs to be either
> > > 
> > > 	xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE)
> > > 
> > > or
> > > 
> > > 	(xfrm & ~XFEATURE_MASK_FPSSE & ~vcpu->arch.guest_supported_xcr0)
> > > 
> > > 
> > > I think I prefer the first one as I find it slightly more obvious that FP+SSE are
> > > allowed in addition to the XCR0 bits.
> > 
> > The above check doesn't verify xfrm is a super set of 0x3.  I think we verify
> > that per SDM:
> 
> Oooh, right.  It's not that FP+SSE are always allowed, it's that FP+SSE must always
> be _set_.  So this?
> 
> 		xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
> 		(xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE

Looks good.

I'll try to get some test done with this code change.

> 
> > 39.7.3 Processor Extended States and ENCLS[ECREATE]
> > 
> > The ECREATE leaf function of the ENCLS instruction enforces a number of
> > consistency checks described earlier. The execution of ENCLS[ECREATE] leaf
> > function results in a #GP(0) in any of the following cases:
> >   • SECS.ATTRIBUTES.XFRM[1:0] is not 3.
> >   • The processor does not support XSAVE and any of the following is true:
> > 	— SECS.ATTRIBUTES.XFRM[63:2] is not 0.
> > 	— SECS.SSAFRAMESIZE is 0.
> >   • The processor supports XSAVE and any of the following is true:
> > 	— XSETBV would fault on an attempt to load XFRM into XCR0.
> > 	— XFRM[63]=1.
> > 	— The SSAFRAME is too small to hold required, enabled states ...
> > 
> > 
> > And in the ECREATE pseudo code, the relevant parts seem to be:
> > 
> > 	(* Check lower 2 bits of XFRM are set *)
> > 	IF ( ( DS:TMP_SECS.ATTRIBUTES.XFRM BitwiseAND 03H) ≠ 03H)
> > 		THEN #GP(0); FI;
> > 
> > 	IF (XFRM is illegal)
> > 		THEN #GP(0); FI;
> > 
> > The first part is clear, but the second part is vague. 
> > 
> > I am not sure in hardware behaviour, whether XCR0 is actually checked in
> > ECREATE.  It's more likely XCRO is actually checked in EENTER.  
> > 
> > But I think it's just fine to also check against XCR0 here.
> 
> ECREATE doesn't check XCR0, it checks that XFRM represents a legal XCR0 values
> for the platform, which in KVM is tracked as guest_supported_xcr0.

Yes agreed.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> On Wed, 5 Apr 2023 19:10:40 -0700
> Sean Christopherson <seanjc@google.com> wrote:
> 
> > On Wed, Apr 05, 2023, Huang, Kai wrote:
> > > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > > *** WARNING *** ABI breakage.
> > > > 
> > > > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > > > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > > > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > > > being helpful by having KVM adjust the entries.
> > > 
> > > Actually I am not clear about this topic.
> > > 
> > > So the rule is KVM should never adjust CPUID entries passed from userspace?
> > 
> > Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
> > CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.
> > 
> > > What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> > > those CPUID entries to ensure there's no insane configuration?  My concern is if
> > > we allow guest to be created with insane CPUID configurations, the guest can be
> > > confused and behaviour unexpectedly.
> > 
> > It is userspace's responsibility to provide a sane, correct setup.  The one
> > exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
> > unsupported virtual address width, the argument being that a malicious userspace
> > could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
> > VMCS field.
> > 
> > The reason for KVM punting to userspace is that it's all but impossible to define
> > what is/isn't sane.  A really good example would be an alternative we (Google)
> > considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
> > migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
> > miss reserved bit #PFs.
> > 
> > Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
> > was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
> > anticipation of eventual migration.  So long as userspace doesn't actually enumerate
> > memslots in the illegal address space, KVM would be able to treat such accesses as
> > emulated MMIO, and would only need to intercept #PF(RSVD).
> > 
> > Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
> > definitely qualifies as insane since it really can't work correctly, but in our
> > opinion it was far superior to running with allow_smaller_maxphyaddr=true.
> > 
> > And sane is not the same thing as architecturally legal.  AMX is a good example
> > of this.  It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but
> > not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG
> > in XCR0 without also enabling TILE_DATA.  KVM should arguably reject CPUID configs
> > with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100%
> > architecturally valid, if insane, CPUID configuration.  Ditto for nearly all of
> > the VMX control bits versus their CPUID counterparts.
> > 
> > And sometimes there are good reasons to run a VM with a truly insane configuration,
> > e.g. for testing purposes.
> > 
> > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> 
> Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> It would be nice to have a conclusion of what is "sane" and what is the
> proper role for KVM, as firmware/TDX module is going to validate the "sane"
> CPUID.
> 
> TDX/SNP requires the CPUID to be pre-configured and validated before creating
> a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> SNP_LAUNCH_UPDATE in SNP).
> 
> IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.
> 
> As the valid CPUIDs have been decided when creating a CC guest, what should be
> the proper behavior (basically any new checks?) of KVM for the later
> SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> at least, because some KVM code paths rely on guest CPUID configuration.

For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
the SET_CPUID2 for TDX guest.

Not sure whether AMD counterpart can be done in similar way though. 

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Wed, 2023-04-05 at 19:10 -0700, Sean Christopherson wrote:
> On Wed, Apr 05, 2023, Huang, Kai wrote:
> > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > *** WARNING *** ABI breakage.
> > > 
> > > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > > being helpful by having KVM adjust the entries.
> > 
> > Actually I am not clear about this topic.
> > 
> > So the rule is KVM should never adjust CPUID entries passed from userspace?
> 
> Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
> CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.
> 
> > What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> > those CPUID entries to ensure there's no insane configuration?  My concern is if
> > we allow guest to be created with insane CPUID configurations, the guest can be
> > confused and behaviour unexpectedly.
> 
> It is userspace's responsibility to provide a sane, correct setup.  The one
> exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
> unsupported virtual address width, the argument being that a malicious userspace
> could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
> VMCS field.

Sorry could you elaborate an example of such attack? :)

> 
> The reason for KVM punting to userspace is that it's all but impossible to define
> what is/isn't sane.  A really good example would be an alternative we (Google)
> considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
> migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
> miss reserved bit #PFs.
> 
> Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
> was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
> anticipation of eventual migration.  So long as userspace doesn't actually enumerate
> memslots in the illegal address space, KVM would be able to treat such accesses as
> emulated MMIO, and would only need to intercept #PF(RSVD).
> 
> Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
> definitely qualifies as insane since it really can't work correctly, but in our
> opinion it was far superior to running with allow_smaller_maxphyaddr=true.

I guess everyone wants performance.

> 
> And sane is not the same thing as architecturally legal.  AMX is a good example
> of this.  It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but
> not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG
> in XCR0 without also enabling TILE_DATA.  KVM should arguably reject CPUID configs
> with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100%
> architecturally valid, if insane, CPUID configuration.  Ditto for nearly all of
> the VMX control bits versus their CPUID counterparts.
> 
> And sometimes there are good reasons to run a VM with a truly insane configuration,
> e.g. for testing purposes.
> 
> TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.

Agreed.  Thanks for the clarification.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

On Wed, Apr 12, 2023, Kai Huang wrote:
> On Wed, 2023-04-05 at 19:10 -0700, Sean Christopherson wrote:
> > On Wed, Apr 05, 2023, Huang, Kai wrote:
> > > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > > *** WARNING *** ABI breakage.
> > > > 
> > > > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > > > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > > > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > > > being helpful by having KVM adjust the entries.
> > > 
> > > Actually I am not clear about this topic.
> > > 
> > > So the rule is KVM should never adjust CPUID entries passed from userspace?
> > 
> > Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
> > CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.
> > 
> > > What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> > > those CPUID entries to ensure there's no insane configuration?  My concern is if
> > > we allow guest to be created with insane CPUID configurations, the guest can be
> > > confused and behaviour unexpectedly.
> > 
> > It is userspace's responsibility to provide a sane, correct setup.  The one
> > exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
> > unsupported virtual address width, the argument being that a malicious userspace
> > could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
> > VMCS field.
> 
> Sorry could you elaborate an example of such attack? :)

Hrm, I was going to say that userspace could shove a noncanonical address in
MSR_FS/GS_BASE and trigger an unexpected VM-Fail (VMX) or ??? behavior on VMLOAD
(I don't think SVM consistency checks FS/GS.base).  But is_noncanonical_address()
queries CR4.LA57, not the address width from CPUID.0x80000008, which makes sense
enumearing 57 bits of virtual address space on a CPU without LA57 would also allow
shoving a bad value into hardware.

So even that example is bogus, i.e. commit dd598091de4a ("KVM: x86: Warn if guest
virtual address space is not 48-bits") really shouldn't have gone in.

> > The reason for KVM punting to userspace is that it's all but impossible to define
> > what is/isn't sane.  A really good example would be an alternative we (Google)
> > considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
> > migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
> > miss reserved bit #PFs.
> > 
> > Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
> > was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
> > anticipation of eventual migration.  So long as userspace doesn't actually enumerate
> > memslots in the illegal address space, KVM would be able to treat such accesses as
> > emulated MMIO, and would only need to intercept #PF(RSVD).
> > 
> > Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
> > definitely qualifies as insane since it really can't work correctly, but in our
> > opinion it was far superior to running with allow_smaller_maxphyaddr=true.
> 
> I guess everyone wants performance.

Performance was a secondary concern, functional correctness was the main issue.
We were concerned that KVM would end up terminating healthy/sane guests due to
KVM's emulator being incomplete, i.e. if KVM failed to emulate an instruction in
the EPT violation handler when GPA > guest.MAXPHYADDR.  That, and SVM sets the
Accessed bit in the guest PTE before the NPT exit, i.e. KVM can't emulate a
smaller guest.MAXPHYADDR without creating an architectural violation from the
guest's perspective (a PTE with reserved bits should never set A/D bits).

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

On Wed, Apr 12, 2023, Kai Huang wrote:
> On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> > On Wed, 5 Apr 2023 19:10:40 -0700
> > Sean Christopherson <seanjc@google.com> wrote:
> > > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> > 
> > Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> > It would be nice to have a conclusion of what is "sane" and what is the
> > proper role for KVM, as firmware/TDX module is going to validate the "sane"
> > CPUID.
> > 
> > TDX/SNP requires the CPUID to be pre-configured and validated before creating
> > a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> > SNP_LAUNCH_UPDATE in SNP).
> > 
> > IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> > for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> > one is literally possible and correct. KVM should not care if the
> > combination, the usage of the chosen ones is insane or not, which gives
> > QEMU flexibility.
> > 
> > As the valid CPUIDs have been decided when creating a CC guest, what should be
> > the proper behavior (basically any new checks?) of KVM for the later
> > SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> > at least, because some KVM code paths rely on guest CPUID configuration.
> 
> For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
> manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
> the SET_CPUID2 for TDX guest.

It's been a long while since I looked at TDX's CPUID management, but IIRC ignoring
SET_CPUID2 is not an option becuase the TDH.MNG.INIT only allows leafs that are
known to the TDX Module, e.g. KVM's paravirt CPUID leafs can't be communicated via
TDH.MNG.INIT.  KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
could result in previously allowed input becoming invalid.

Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
leaf would effectively make _everything_ valid input.  If KVM were to rely solely
on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
that userspace provide a CPUID model that's consistent with what userspace provided
earlier.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> On Wed, Apr 12, 2023, Kai Huang wrote:
> > On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> > > On Wed, 5 Apr 2023 19:10:40 -0700
> > > Sean Christopherson <seanjc@google.com> wrote:
> > > > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> > > 
> > > Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> > > It would be nice to have a conclusion of what is "sane" and what is the
> > > proper role for KVM, as firmware/TDX module is going to validate the "sane"
> > > CPUID.
> > > 
> > > TDX/SNP requires the CPUID to be pre-configured and validated before creating
> > > a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> > > SNP_LAUNCH_UPDATE in SNP).
> > > 
> > > IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> > > for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> > > one is literally possible and correct. KVM should not care if the
> > > combination, the usage of the chosen ones is insane or not, which gives
> > > QEMU flexibility.
> > > 
> > > As the valid CPUIDs have been decided when creating a CC guest, what should be
> > > the proper behavior (basically any new checks?) of KVM for the later
> > > SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> > > at least, because some KVM code paths rely on guest CPUID configuration.
> > 
> > For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
> > manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
> > the SET_CPUID2 for TDX guest.
> 
> It's been a long while since I looked at TDX's CPUID management, but IIRC ignoring
> SET_CPUID2 is not an option becuase the TDH.MNG.INIT only allows leafs that are
> known to the TDX Module, e.g. KVM's paravirt CPUID leafs can't be communicated via
> TDH.MNG.INIT.  
> 

Oh yes.  I forgot this.

> KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> could result in previously allowed input becoming invalid.

How about only filtering out PV related CPUIDs when applying CPUIDs to
TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
anyway.

> 
> Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.

Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
more concrete result.  

> 
> Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> that userspace provide a CPUID model that's consistent with what userspace provided
> earlier.

So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
all vcpus are consistent (the same) in SET_CPUID{2}?

Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
when applying CPUIDs to TDH.MNG.INIT" approach works. 

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Zhi Wang]

On Wed, 12 Apr 2023 12:07:13 +0000
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Thu, 2023-04-06 at 13:01 +0300, Zhi Wang wrote:
> > On Wed, 5 Apr 2023 19:10:40 -0700
> > Sean Christopherson <seanjc@google.com> wrote:
> > 
> > > On Wed, Apr 05, 2023, Huang, Kai wrote:
> > > > On Tue, 2023-04-04 at 17:59 -0700, Sean Christopherson wrote:
> > > > > *** WARNING *** ABI breakage.
> > > > > 
> > > > > Stop adjusting the guest's CPUID info for the allowed XFRM (a.k.a. XCR0)
> > > > > for SGX enclaves.  Past me didn't understand the roles and responsibilities
> > > > > between userspace and KVM with respect to CPUID leafs, i.e. I thought I was
> > > > > being helpful by having KVM adjust the entries.
> > > > 
> > > > Actually I am not clear about this topic.
> > > > 
> > > > So the rule is KVM should never adjust CPUID entries passed from userspace?
> > > 
> > > Yes, except for true runtime entries where a CPUID leaf is dynamic based on other
> > > CPU state, e.g. CR4 bits, MISC_ENABLES in the MONITOR/MWAIT case, etc.
> > > 
> > > > What if the userspace passed the incorrect CPUID entries?  Should KVM sanitize
> > > > those CPUID entries to ensure there's no insane configuration?  My concern is if
> > > > we allow guest to be created with insane CPUID configurations, the guest can be
> > > > confused and behaviour unexpectedly.
> > > 
> > > It is userspace's responsibility to provide a sane, correct setup.  The one
> > > exception is that KVM rejects KVM_SET_CPUID{2} if userspace attempts to define an
> > > unsupported virtual address width, the argument being that a malicious userspace
> > > could attack KVM by coercing KVM into stuff a non-canonical address into e.g. a
> > > VMCS field.
> > > 
> > > The reason for KVM punting to userspace is that it's all but impossible to define
> > > what is/isn't sane.  A really good example would be an alternative we (Google)
> > > considered for the "smaller MAXPHYADDR" fiasco, the underlying problem being that
> > > migrating a vCPU with MAXPHYADDR=46 to a system with MAXPHYADDR=52 will incorrectly
> > > miss reserved bit #PFs.
> > > 
> > > Rather than teach KVM to try and deal with smaller MAXPHYADDRs, an idea we considered
> > > was to instead enumerate guest.MAXPHYADDR=52 on platforms with host.MAXPHYADDR=46 in
> > > anticipation of eventual migration.  So long as userspace doesn't actually enumerate
> > > memslots in the illegal address space, KVM would be able to treat such accesses as
> > > emulated MMIO, and would only need to intercept #PF(RSVD).
> > > 
> > > Circling back to "what's sane", enumerating guest.MAXPHYADDR > host.MAXPHYADDR
> > > definitely qualifies as insane since it really can't work correctly, but in our
> > > opinion it was far superior to running with allow_smaller_maxphyaddr=true.
> > > 
> > > And sane is not the same thing as architecturally legal.  AMX is a good example
> > > of this.  It's _technically_ legal to enumerate support for XFEATURE_TILE_CFG but
> > > not XFEATURE_TILE_DATA in CPUID, but illegal to actually try to enable TILE_CFG
> > > in XCR0 without also enabling TILE_DATA.  KVM should arguably reject CPUID configs
> > > with TILE_CFG but not TILE_DATA, and vice versa, but then KVM is rejecting a 100%
> > > architecturally valid, if insane, CPUID configuration.  Ditto for nearly all of
> > > the VMX control bits versus their CPUID counterparts.
> > > 
> > > And sometimes there are good reasons to run a VM with a truly insane configuration,
> > > e.g. for testing purposes.
> > > 
> > > TL;DR: trying to enforce "sane" CPUID/feature configuration is a gigantic can of worms.
> > 
> > Interesting point. I was digging the CPUID virtualization OF TDX/SNP.
> > It would be nice to have a conclusion of what is "sane" and what is the
> > proper role for KVM, as firmware/TDX module is going to validate the "sane"
> > CPUID.
> > 
> > TDX/SNP requires the CPUID to be pre-configured and validated before creating
> > a CC guest. (It is done via TDH.MNG.INIT in TDX and inserting a CPUID page in
> > SNP_LAUNCH_UPDATE in SNP).
> > 
> > IIUC according to what you mentioned, KVM should be treated like "CPUID box"
> > for QEMU and the checks in KVM is only to ensure the requirements of a chosen
> > one is literally possible and correct. KVM should not care if the combination, the usage of the chosen ones is insane or not, which gives QEMU flexibility.
> > 
> > As the valid CPUIDs have been decided when creating a CC guest, what should be
> > the proper behavior (basically any new checks?) of KVM for the later
> > SET_CPUID2? My gut feeling is KVM should know the "CPUID box" is reduced
> > at least, because some KVM code paths rely on guest CPUID configuration.
> 
> For TDX guest my preference is KVM to save all CPUID entries in TDH.MNG.INIT and
> manually make vcpu's CPUID point to the saved CPUIDs.  And then KVM just ignore
> the SET_CPUID2 for TDX guest.
> 
> Not sure whether AMD counterpart can be done in similar way though. 

I took a look on AMD SNP kernel[1], it supports host managing the CPUID
and firmware managing the CPUID. The host-managed CPUID is done via a GHCB
message call and it is going to be removed according to the SNP firmware ABI
spec:

7.1 CPUID Reporting
Note: This guest message may be removed in future versions as it is redundant with the CPUID page in SNP_LAUNCH_UPDATE. (See Section 8.17.)

So the style of CPUID virtualization of TDX and SNP will be aligned eventually.
Both will configure the supported CPUID for the firmware/TDX module before
creating a vCPU. 

[1] https://github.com/AMDESE/linux/blob/upmv10-host-snp-v8-rfc/arch/x86/kvm/svm/sev.c
[2] https://www.amd.com/system/files/TechDocs/56860.pdf

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Sean Christopherson]

On Thu, Apr 13, 2023, Kai Huang wrote:
> On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> > KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> > unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> > that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> > could result in previously allowed input becoming invalid.
> 
> How about only filtering out PV related CPUIDs when applying CPUIDs to
> TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
> anyway.

Nope, not going down that road.  Fool me once[*], shame on you.  Fool me twice,
shame on me :-)

Objections to hardware vendors defining PV interfaces aside, there exist leafs
that are neither PV related nor known to the TDX module, e.g. Centaur leafs.  I
think it's extremely unlikely (understatement) that anyone will want to expose
Centaur leafs to a TDX guest, but again I want to say out of the business of
telling userspace what is and isn't sane CPUID models.

[*] https://lore.kernel.org/all/20221210160046.2608762-6-chen.zhang@intel.com

> > Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> > becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> > leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> > on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.
> 
> Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
> more concrete result.  
> 
> > 
> > Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> > that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> > key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> > isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> > that userspace provide a CPUID model that's consistent with what userspace provided
> > earlier.
> 
> So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
> the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
> all vcpus are consistent (the same) in SET_CPUID{2}?

Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each
other, just that each vCPU is consistent with respect to what was reported to the
TDX Module.

> Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
> when applying CPUIDs to TDH.MNG.INIT" approach works. 

It's not complicated at all.  Walk through the leafs defined during TDH.MNG.INIT,
reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly.  Or has
the TDX spec changed and it's no longer that simple?

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Huang, Kai]

On Thu, 2023-04-13 at 15:48 -0700, Sean Christopherson wrote:
> On Thu, Apr 13, 2023, Kai Huang wrote:
> > On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> > > KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> > > unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> > > that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> > > could result in previously allowed input becoming invalid.
> > 
> > How about only filtering out PV related CPUIDs when applying CPUIDs to
> > TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
> > anyway.
> 
> Nope, not going down that road.  Fool me once[*], shame on you.  Fool me twice,
> shame on me :-)

Ah OK :)

> 
> Objections to hardware vendors defining PV interfaces aside, there exist leafs
> that are neither PV related nor known to the TDX module, e.g. Centaur leafs.  I
> think it's extremely unlikely (understatement) that anyone will want to expose
> Centaur leafs to a TDX guest, but again I want to say out of the business of
> telling userspace what is and isn't sane CPUID models.

Right.  There might be use case that TDX guest wants to use some CPUID which
isn't handled by the TDX module but purely by KVM.  We don't want to limit the
possibility.  Totally agree.

> 
> [*] https://lore.kernel.org/all/20221210160046.2608762-6-chen.zhang@intel.com
> 
> > > Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> > > becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> > > leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> > > on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.
> > 
> > Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
> > more concrete result.  
> > 
> > > 
> > > Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> > > that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> > > key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> > > isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> > > that userspace provide a CPUID model that's consistent with what userspace provided
> > > earlier.
> > 
> > So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
> > the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
> > all vcpus are consistent (the same) in SET_CPUID{2}?
> 
> Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each
> other, just that each vCPU is consistent with respect to what was reported to the
> TDX Module.

OK.  Fine to me.

> 
> > Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
> > when applying CPUIDs to TDH.MNG.INIT" approach works. 
> 
> It's not complicated at all.  Walk through the leafs defined during TDH.MNG.INIT,
> reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly.  Or has
> the TDX spec changed and it's no longer that simple?

No the module hasn't been changed, and yes it should be as simple as you said. 
I just had some first impression that handling CPUID in one IOCTL (TDH.MNG.INIT)
should be simpler than handling CPUID in two IOCTLs, but I guess this might not
be true :)

Anyway I agree with your suggestion.  Thanks.

Re: [PATCH 0/3] KVM: x86: SGX vs. XCR0 cleanups

[Zhi Wang]

On Fri, 14 Apr 2023 13:42:11 +0000
"Huang, Kai" <kai.huang@intel.com> wrote:

> On Thu, 2023-04-13 at 15:48 -0700, Sean Christopherson wrote:
> > On Thu, Apr 13, 2023, Kai Huang wrote:
> > > On Wed, 2023-04-12 at 08:22 -0700, Sean Christopherson wrote:
> > > > KVM's uAPI for initiating TDH.MNG.INIT could obviously filter out
> > > > unsupported leafs, but doing so would lead to potential ABI breaks, e.g. if a leaf
> > > > that KVM filters out becomes known to the TDX Module, then upgrading the TDX Module
> > > > could result in previously allowed input becoming invalid.
> > > 
> > > How about only filtering out PV related CPUIDs when applying CPUIDs to
> > > TDH.MNG.INIT?  I think we can assume they are not gonna be known to TDX module
> > > anyway.
> > 
> > Nope, not going down that road.  Fool me once[*], shame on you.  Fool me twice,
> > shame on me :-)
> 
> Ah OK :)
> 
> > 
> > Objections to hardware vendors defining PV interfaces aside, there exist leafs
> > that are neither PV related nor known to the TDX module, e.g. Centaur leafs.  I
> > think it's extremely unlikely (understatement) that anyone will want to expose
> > Centaur leafs to a TDX guest, but again I want to say out of the business of
> > telling userspace what is and isn't sane CPUID models.
> 
> Right.  There might be use case that TDX guest wants to use some CPUID which
> isn't handled by the TDX module but purely by KVM.  We don't want to limit the
> possibility.  Totally agree.
> 
> > 
> > [*] https://lore.kernel.org/all/20221210160046.2608762-6-chen.zhang@intel.com
> > 
> > > > Even if that weren't the case, ignoring KVM_SET_CPUID{2} would be a bad option
> > > > becuase it doesn't allow KVM to open behavior in the future, i.e. ignoring the
> > > > leaf would effectively make _everything_ valid input.  If KVM were to rely solely
> > > > on TDH.MNG.INIT, then KVM would want to completely disallow KVM_SET_CPUID{2}.
> > > 
> > > Right.  Disallowing SET_CPUID{2} probably is better, as it gives userspace a
> > > more concrete result.  
> > > 
> > > > 
> > > > Back to Zhi's question, the best thing to do for TDX and SNP is likely to require
> > > > that overlap between KVM_SET_CPUID{2} and the "trusted" CPUID be consistent.  The
> > > > key difference is that KVM would be enforcing consistency, not sanity.  I.e. KVM
> > > > isn't making arbitrary decisions on what is/isn't sane, KVM is simply requiring
> > > > that userspace provide a CPUID model that's consistent with what userspace provided
> > > > earlier.
> > > 
> > > So IIUC, you prefer to verifying the CPUIDs in SET_CPUID{2} are a super set of
> > > the CPUIDs provided in TDH.MNG.INIT?  And KVM manually verifies all CPUIDs for
> > > all vcpus are consistent (the same) in SET_CPUID{2}?
> > 
> > Yes, except KVM doesn't need to verify vCPUs are consistent with respect to each
> > other, just that each vCPU is consistent with respect to what was reported to the
> > TDX Module.
> 
> OK.  Fine to me.

I did some investigations and I think this approach would work on both TDX
and SNP, as both of them can let a CC guest handle the firmware-not-aware CPUID
in #VE or #VC. E.g. KVM paravirt CPUIDs. And we can factor out and re-use the
"checking-CPUID-is-equal" in KVM_SET_CPUID{2}. But I think TDX needs to
filter out the firmware-not-aware CPUIDs in TDH.MNG.INIT to pass the check?
(SNP firmware can adjust them automatically). I attached some details I found
in case you are interested in digging.

For TDX, KVM provides a CPUID table in TDH.MNG.INIT, and there are two polices
for the following CPUID virtualization: 1) TDX-module handle the CPUID
interception from a TD guest and emulated according to the CPUID table in
TDH.MNG.INIT. If TDX-module doesn't know this CPUID, #VE is injected 2) A TD
guest can request to handle the CPUID by itself via calling TDG.VP_CPUIDVE_SET.
Then a CPUID TD exit will be forwarded to the guest as #VE. The code snippet
of TDX module handling TD CPUID exit can be found here[1].

For SNP, userspace provides a CPUID table in SNP_LAUNCH_UPDATE with
PAGE_TYPE_CPUID. PSP will check and validate the CPUID in the table. It will be
part of the SNP metadata secrets, and passed to the guest later. A guest can
refer to the validated CPUID table when handling CPUID #VC, but can also handle
CPUIDs not in the table[2] (e.g. paravirt CPUID).


[1] https://downloadmirror.intel.com/738876/tdx-module-v1.0.01.01.zip/src/td_dispatcher/vm_exits/td_cpuid.c
[2] https://github.com/AMDESE/linux-svsm/blob/main/src/cpu/vc.rs#L571
> 
> > 
> > > Looks this is over-complicated, _if_ the "only filtering out PV related CPUIDs
> > > when applying CPUIDs to TDH.MNG.INIT" approach works. 
> > 
> > It's not complicated at all.  Walk through the leafs defined during TDH.MNG.INIT,
> > reject KVM_SET_CPUID if a leaf isn't present or doesn't match exactly.  Or has
> > the TDX spec changed and it's no longer that simple?
> 
> No the module hasn't been changed, and yes it should be as simple as you said. 
> I just had some first impression that handling CPUID in one IOCTL (TDH.MNG.INIT)
> should be simpler than handling CPUID in two IOCTLs, but I guess this might not
> be true :)
> 
> Anyway I agree with your suggestion.  Thanks.
> 

Re: [PATCH 1/3] KVM: VMX: Don't rely _only_ on CPUID to enforce XCR0 restrictions for ECREATE

[Huang, Kai]

> > 
> > Oooh, right.  It's not that FP+SSE are always allowed, it's that FP+SSE must always
> > be _set_.  So this?
> > 
> > 		xfrm & ~(vcpu->arch.guest_supported_xcr0 | XFEATURE_MASK_FPSSE) ||
> > 		(xfrm & XFEATURE_MASK_FPSSE) != XFEATURE_MASK_FPSSE
> 
> Looks good.
> 
> I'll try to get some test done with this code change.
> 

Tested this series with your above code change by running simple SGX app in the
guest.

For this particular case, tested with ECREATE with xfrm = 0x1 in the guest, and
guest can receive #GP. 

So for the entire series:

Reviewed-by: Kai Huang <kai.huang@intel.com>
Tested-by: Kai Huang <kai.huang@intel.com>


>