On Wed, Jan 27, 2021, Kai Huang wrote:
> 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.
SGX driver doesn't need to be changed, just this core feat_ctl.c code.
> 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?
Yep, adding a check on SGX1 is definitely not mandatory for this series.
