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

[Huang, Kai]

> 
> 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@xxxxxxxxx>
> > >
> > > 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.

I don't think update of Intel hash would be frequent, like you said all current processors with SGX support may should be using the same value.

Thus I am expecting I can run Intel SDK on KVM guest if LC feature flag is cleared.

Even Intel has updated the key, SDK should mention those impacted machine should run SDK starting from some new version, so SDK should continue to run.

> 
> 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.

I am not following. KVM can do whatever it wants, but it cannot change the fact that KVM guest cannot run intel enclave if platform's MSRs are configured to 3rd party and locked.

Or am I misunderstanding?

> 
> > 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?

One example is EPCM + EPC should be power of 2. And EPC migration (sudden loss of EPC). And if we apply some policy to restrict enclave during EINIT trap, etc.

But those are not the point. I should probably have not mentioned them.

> 
> > 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.

OK.

But IMO if what I mentioned above is correct, then that part should overweight the benefit you can get here.

Anyway who cares to do less/more thing in driver probe?

Thanks,
-Kai