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. 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.
