mbox series

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

Message ID 20230503160838.3412617-1-seanjc@google.com
Headers
Series KVM: x86: SGX vs. XCR0 cleanups |

Message

Sean Christopherson May 3, 2023, 4:08 p.m. UTC 
  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 change, but QEMU does the right thing and AFAIK no
other VMMs support SGX (yet), so I'm hopeful/confident that we can excise
the ugly before userspace starts depending on the bad behavior.
 
v2:
 - Collect reviews/testing. [Kai]
 - Require FP+SSE to always be set in XFRM, and exempt them from the XFRM
   vs. XCR0 check. [Kai]

v1: https://lore.kernel.org/all/20230405005911.423699-1-seanjc@google.com

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 | 11 +++++++++--
 2 files changed, 19 insertions(+), 35 deletions(-)


base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5

Comments

Paolo Bonzini May 19, 2023, 5:54 p.m. UTC | #1 
On 5/3/23 18:08, Sean Christopherson wrote:
> 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 change, but QEMU does the right thing and AFAIK no
> other VMMs support SGX (yet), so I'm hopeful/confident that we can excise
> the ugly before userspace starts depending on the bad behavior.
>   
> v2:
>   - Collect reviews/testing. [Kai]
>   - Require FP+SSE to always be set in XFRM, and exempt them from the XFRM
>     vs. XCR0 check. [Kai]
> 
> v1: https://lore.kernel.org/all/20230405005911.423699-1-seanjc@google.com
> 
> 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 | 11 +++++++++--
>   2 files changed, 19 insertions(+), 35 deletions(-)
> 
> 
> base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5

Queued, thanks.  But why patch 3?  Small functions are nice and remove 
the need to remember what is in EDX:EAX of CPUID[0xD,0].

Paolo
Sean Christopherson May 19, 2023, 8:57 p.m. UTC | #2 
On Fri, May 19, 2023, Paolo Bonzini wrote:
> On 5/3/23 18:08, Sean Christopherson wrote:
> > 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 change, but QEMU does the right thing and AFAIK no
> > other VMMs support SGX (yet), so I'm hopeful/confident that we can excise
> > the ugly before userspace starts depending on the bad behavior.
> > v2:
> >   - Collect reviews/testing. [Kai]
> >   - Require FP+SSE to always be set in XFRM, and exempt them from the XFRM
> >     vs. XCR0 check. [Kai]
> > 
> > v1: https://lore.kernel.org/all/20230405005911.423699-1-seanjc@google.com
> > 
> > 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 | 11 +++++++++--
> >   2 files changed, 19 insertions(+), 35 deletions(-)
> > 
> > 
> > base-commit: 5c291b93e5d665380dbecc6944973583f9565ee5
> 
> Queued, thanks.  But why patch 3?

I want to guard against future misuse of calculating the support XCR0 before the
CPUID update is complete.  I suppose I could have done this:

  static u64 guest_cpuid_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;
  }

but I don't really see the point since there should only ever be one caller,
e.g. unlike cpuid_query_maxphyaddr() which needs a non-zero default.

> Small functions are nice and remove the need to remember what is in EDX:EAX
> of CPUID[0xD,0].

Hmm, yes and no.  Specifically, I dislike single-use helpers that bury unintuitive
details in the helper, e.g. in this case, filtering the raw iguest CPUID with KVM's
kvm_caps.supported_xcr0.  Communicating that in the name of the function so that
they're are no surprises is often more difficult than just open coding things.