On Tue, 26 Jan 2021 16:18:31 -0800 Dave Hansen wrote:
> On 1/26/21 3:56 PM, Kai Huang wrote:
> > On Tue, 26 Jan 2021 08:26:21 -0800 Dave Hansen wrote:
> >> On 1/26/21 1:30 AM, Kai Huang wrote:
> >>> --- a/arch/x86/kernel/cpu/feat_ctl.c
> >>> +++ b/arch/x86/kernel/cpu/feat_ctl.c
> >>> @@ -105,7 +105,8 @@ early_param("nosgx", nosgx);
> >>> void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> >>> {
> >>> bool tboot = tboot_enabled();
> >>> - bool enable_sgx;
> >>> + bool enable_vmx;
> >>> + bool enable_sgx_any, enable_sgx_kvm, enable_sgx_driver;
> >>> u64 msr;
> >>>
> >>> if (rdmsrl_safe(MSR_IA32_FEAT_CTL, &msr)) {
> >>> @@ -114,13 +115,22 @@ void init_ia32_feat_ctl(struct cpuinfo_x86 *c)
> >>> return;
> >>> }
> >>>
> >>> + enable_vmx = cpu_has(c, X86_FEATURE_VMX) &&
> >>> + IS_ENABLED(CONFIG_KVM_INTEL);
> >>
> >> The reason it's called 'enable_sgx' below is because this code is
> >> actually going to "enable sgx". This code does not "enable vmx". That
> >> makes this a badly-named variable. "vmx_enabled" or "vmx_available"
> >> would be better.
> >
> > It will also try to enable VMX if feature control MSR is not locked by BIOS.
> > Please see below code:
>
> Ahh, I forgot this is non-SGX code. It's mucking with all kinds of
> other stuff in the same MSR. Oh, well, I guess that's what you get for
> dumping a bunch of refactoring in the same patch as the new code.
>
>
> >>> - enable_sgx = cpu_has(c, X86_FEATURE_SGX) &&
> >>> - cpu_has(c, X86_FEATURE_SGX_LC) &&
> >>> - IS_ENABLED(CONFIG_X86_SGX);
> >>> + enable_sgx_any = cpu_has(c, X86_FEATURE_SGX) &&
> >>> + cpu_has(c, X86_FEATURE_SGX1) &&
> >>> + IS_ENABLED(CONFIG_X86_SGX);
> >>
> >> The X86_FEATURE_SGX1 check seems to have snuck in here. Why?
> >
> > Please see my reply to Sean's reply.
>
> ... yes, so you're breaking out the fix into a separate patch,.
For the separate patch to fix SGX1 check, if I understand correctly, SGX driver
should be changed too. I feel I am not the best person to do it. Jarkko or Sean
is.
So I'll remove SGX1 here in the next version, but I won't include another
patch to fix the SGX1 logic. If Jarkko or Sean sent out that patch, and it is
merged quickly, I can rebase on top of that.
Does this make sense?
