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,.
>>> update_sgx:
>>> - if (!(msr & FEAT_CTL_SGX_ENABLED) ||
>>> - !(msr & FEAT_CTL_SGX_LC_ENABLED) || !enable_sgx) {
>>> - if (enable_sgx)
>>> - pr_err_once("SGX disabled by BIOS\n");
>>> + if (!(msr & FEAT_CTL_SGX_ENABLED)) {
>>> + if (enable_sgx_kvm || enable_sgx_driver)
>>> + pr_err_once("SGX disabled by BIOS.\n");
>>> clear_cpu_cap(c, X86_FEATURE_SGX);
>>> + return;
>>> + }
>>
>>
>> Isn't there a pr_fmt here already? Won't these just look like:
>>
>> sgx: SGX disabled by BIOS.
>>
>> That seems a bit silly.
>
> Please see my reply to Sean's reply.
Got it. I was thinking this was in the SGX code, not in the generic CPU
setup code.
