Re: [PATCH v6 04/11] x86: define IA32_FEATUE_CONTROL.SGX_LC

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

On Tue, 2017-11-28 at 23:55 +0200, Jarkko Sakkinen wrote:
> On Tue, Nov 28, 2017 at 01:33:14PM -0800, Sean Christopherson wrote:
> > 
> > On Tue, 2017-11-28 at 23:24 +0200, Jarkko Sakkinen wrote:
> > > 
> > > On Tue, Nov 28, 2017 at 10:53:24PM +0200, Jarkko Sakkinen wrote:
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > So, maybe something like this?
> > > > > 
> > > > >     After SGX is activated[1] the IA32_SGXLEPUBKEYHASHn MSRs are writable
> > > > >     if and only if SGX_LC is set in the IA32_FEATURE_CONTROL MSR and the
> > > > >     IA32_FEATURE_CONTROL MSR is locked, otherwise they are read-only.
> > > > > 
> > > > >     For example, firmware can allow the OS to change the launch enclave
> > > > >     root key by setting IA32_FEATURE_CONTROL.SGX_LC, and thus give the
> > > > >     OS complete control over the enclaves it runs.  Alternatively,
> > > > >     firmware can clear IA32_FEATURE_CONTROL.SGX_LC to lock down the root
> > > > >     key and restrict the OS to running enclaves signed with the root key
> > > > >     or whitelisted/trusted by a launch enclave (which must be signed with
> > > > >     the root key).
> > > > > 
> > > > >     [1] SGX related bits in IA32_FEATURE_CONTROL cannot be set until SGX
> > > > >         is activated, e.g. by firmware.  SGX activation is triggered by
> > > > >         setting bit 0 in MSR 0x7a.  Until SGX is activated, the LE hash
> > > > >         MSRs are writable, e.g. to allow firmware to lock down the LE
> > > > >         root key with a non-Intel value.
> > > > Thanks I'll use this as a basis and move most of the crappy commit
> > > > message to the commit (with some editing) that defines the MSRs.
> > > Not sure after all if I'm following this.
> > > 
> > > IA32_FEATURE_CONTROL[17] contols whether the MSRs are writable or not
> > > after the feature control MSR is locked. SGX_LC means just that the
> > > CPU supports the launch configuration.
> > > 
> > > /Jarkko
> > My comments were referring to improving the commit message for defining
> > IA32_FEATURE_CONTROL.SGX_LC, i.e. bit 17, not the CPUID bit.
> My bad but SGX_LC is referring here to the CPUID bit.
> 
> In SGX chapters there is no specific name for IA32_FEATURE_CONTROL[17].
> I would call it something else than SGX_LC. Maybe SGX_LC_WRITABLE.
> 
> /Jarkko

What about SGX_LC_ENABLE?  The title in the MSR section of the SDM is
"SGX Launch Control Enable", and it's more consistent with the other
bits defined in feature control.  I'd also prefer that name for the
actual #define too, SGX_LAUNCH_CONTROL_ENABLE is overly verbose IMO.