Re: [PATCH v19,RESEND 08/27] x86/cpu/intel: Detect SGX support and update caps appropriately

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Tue, Mar 26, 2019 at 05:17:40AM -0700, Huang, Kai wrote:
> On Wed, 2019-03-20 at 18:21 +0200, Jarkko Sakkinen wrote:
> > From: Sean Christopherson <sean.j.christopherson@intel.com>
> > 
> > Similar to other large Intel features such as VMX and TXT, SGX must be
> > explicitly enabled in IA32_FEATURE_CONTROL MSR to be truly usable.
> > Clear all SGX related capabilities if SGX is not fully enabled in
> > IA32_FEATURE_CONTROL or if the SGX1 instruction set isn't supported
> > (impossible on bare metal, theoretically possible in a VM if the VMM is
> > doing something weird).
> > 
> > Like SGX itself, SGX Launch Control must be explicitly enabled via a
> > flag in IA32_FEATURE_CONTROL. Clear the SGX_LC capability if Launch
> > Control is not fully enabled (or obviously if SGX itself is disabled).
> > 
> > Note that clearing X86_FEATURE_SGX_LC creates a bit of a conundrum
> > regarding the SGXLEPUBKEYHASH MSRs, as it may be desirable to read the
> > MSRs even if they are not writable, e.g. to query the configured key,
> > but clearing the capability leaves no breadcrum for discerning whether
> > or not the MSRs exist.  But, such usage will be rare (KVM is the only
> > known case at this time) and not performance critical, so it's not
> > unreasonable to require the use of rdmsr_safe().  Clearing the cap bit
> > eliminates the need for an additional flag to track whether or not
> > Launch Control is truly enabled, which is what we care about the vast
> > majority of the time.
> 
> [Resend. Somehow my last reply doesn't show up in my mailbox so not sure whether I sent it
> successfully or not. Sorry if you receving duplicated mails.]
> 
> However this is not consistent with HW behavior. If LC feature flag is not present, then MSRs should
> have hash of Intel's key, which is not always the case here, when you expose SGX to KVM. Enclave in
> KVM guest will get unexpected EINIT error when launing Intel enclave, if on HW MSRs are configured
> to 3rd party value but locked to readonly.

Intel doesn't have a singular key.  The internal reset value of the LE
pubkey hash MSRs is micro-architectural, i.e. can change without warning
on any given processor.  All current processors with SGX support may use
the same reset value, but it's not something that customers should/can
rely on.

That being said, this in no way impacts KVM's ability to virtualize SGX,
e.g. KVM can directly do CPUID and {RD,WR}MSR to probe the capabilities
of the platform as needed.

> My opition is we already have enough cases that violates HW behavior in
> SGX virtualization, let's not have one more.

What are the other cases?

> Besides, why do we "need an additional flag to track whether or not
> Launch Control is truly enabled"? Doesn't driver only need to know whether
> MSRs are writable?

Yes, and that's why we're overloading X86_FEATURE_SGX_LC to be set if
and only if SGX_LC is supported *and* enabled, e.g. so that the kernel
can simply check X86_FEATURE_SGX_LC without having to also probe the MSRs.