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.