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.
