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.
