Re: [RFC PATCH 04/23] x86/cpufeatures: Add SGX1 and SGX2 sub-features

[Sean Christopherson <seanjc@google.com>]

On Thu, Jan 07, 2021, Kai Huang wrote:
> On Wed, 6 Jan 2021 14:21:39 -0800 Dave Hansen wrote:
> > On 1/6/21 2:12 PM, Kai Huang wrote:
> > > On Wed, 6 Jan 2021 11:39:46 -0800 Dave Hansen wrote:
> > >> On 1/5/21 5:55 PM, Kai Huang wrote:
> > >>> --- a/arch/x86/kernel/cpu/feat_ctl.c
> > >>> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> > >>> @@ -97,6 +97,8 @@ static void clear_sgx_caps(void)
> > >>>  {
> > >>>  	setup_clear_cpu_cap(X86_FEATURE_SGX);
> > >>>  	setup_clear_cpu_cap(X86_FEATURE_SGX_LC);
> > >>> +	setup_clear_cpu_cap(X86_FEATURE_SGX1);
> > >>> +	setup_clear_cpu_cap(X86_FEATURE_SGX2);
> > >>>  }
> > >> Logically, I think you want this *after* the "Allow SGX virtualization
> > >> without Launch Control support" patch.  As it stands, this will totally
> > >> disable SGX (including virtualization) if launch control is unavailable.
> > >>
> > > To me it is better to be here, since clear_sgx_caps(), which disables SGX
> > > totally, should logically clear all SGX feature bits, no matter later patch's
> > > behavior. So when new SGX bits are introduced, clear_sgx_caps() should clear
> > > them too. Otherwise the logic of this patch (adding new SGX feature bits) is
> > > not complete IMHO.
> > > 
> > > And actually in later patch "Allow SGX virtualization without Launch Control
> > > support", a new clear_sgx_lc() is added, and is called when LC is not
> > > available but SGX virtualization is enabled, to make sure only SGX_LC bit is
> > > cleared in this case. I don't quite understand why we need to clear SGX1 and
> > > SGX2 in clear_sgx_caps() after the later patch.
> > 
> > I was talking about patch ordering.  It could be argued that this goes
> > after the content of patch 05/23.  Please _consider_ changing the ordering.
> > 
> > If that doesn't work for some reason, please at least call out in the
> > changelog that it leaves a temporarily funky situation.
> > 
> 
> The later patch currently uses SGX1 bit, which is the reason that this patch
> needs be before later patch.
> 
> Sean,
> 
> I think it is OK to remove SGX1 bit check in later patch, since I have
> never seen a machine with SGX bit in CPUID, but w/o SGX1.

The SGX1 check is "needed" to handle the case where SGX is supported but was
soft-disabled, e.g. because software disable a machine check bank by writing an
MCi_CTL MSR.

> If we remove SGX1 bit check in later, we can put this patch after the later
> patch.
> 
> Do you have comment here? If you are OK, I'll remove SGX1 bit check in later
> patch and reorder the patch.

Hmm, I'm not sure why the SGX driver was merged without explicitly checking for
SGX1 support.  I'm pretty sure we had an explicit SGX1 check in the driver path
at some point.  My guess is that the SGX1 change ended up in the KVM series
through a mishandled rebase.

Moving the check later won't break anything that's not already broken.  But,
arguably checking SGX1 is a bug fix of sorts, e.g. to guard against broken
firmware, and should go in as a standalone patch destined for stable.  The
kernel can't prevent SGX from being soft-disabled after boot, but IMO it should
cleanly handle the case where SGX was soft-disabled _before_ boot.