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.
>
>
Hi Dave,
After second thinking, if I understand you correctly, the "funky situation" you
are talking about is, w/o patch "Allow SGX virtualization without Launch
Control Support", SGX virtualization is disabled too if LC is not available in
hardware, but in previous patches (basically patch 3 "Introduce virtual EPC
for use by KVM guests"), we have been treating SGX virtualization can be
enabled?
In this case, clearing SGX1 and SGX2 bits before or after "Allow SGX
virtualization without Launch Control support" patch doesn't make difference,
since KVM should always check SGX bit first.
So a better way is to put "Allow SGX virtualization without Launch Control
Support" at the beginning of this series? If so, the Kconfig
X86_SGX_VIRTUALIZATION needs to be in separate patch at the very beginning.
Does above make sense?
