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

[Sean Christopherson]

On Tue, Mar 26, 2019 at 02:25:52PM -0700, Huang, Kai wrote:
> > 
> > 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?

What does that have to do with this patch?  The only thing this patch does
is clear a *software* bit that says "SGX LC is enabled" so that the kernel
can make the reasonable assumption that the MSRs are writable when
X86_FEATURE_SGX_LC=1.

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

Again, that's a micro-architecture detail due to the use of range registers
to carve out the EPC.  The only thing the SDM states regarding size and
alignment is:

  The EPC is divided into EPC pages. An EPC page is 4KB in size and always
  aligned on a 4KB boundary.

> And EPC migration (sudden loss of EPC).

Unfortunate, yes, but it does not violate the architecture in any way.
You can argue that it adds a wrinkle that isn't explicitly documented
in the SDM, but virtualization is full of things like that.

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

Half dozen of one, six of the other.  If we clear the flag, then KVM has
to manually probe the MSRs.  If we don't clear the flag, then the driver
has to manually probe the MSRs.

Alternatively we could add another flag, but then we end up with
X86_FEATURE_SGX_LC and X86_FEATURE_SGX_LE_PUBKEY_HASH_WRITABLE, or maybe
X86_FEATURE_SGX_LC_REALLY_TRULY_ENABLED.

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

Exactly.  KVM will probe the MSRs once during its own loading.