| Message ID | 20200312180416.6679-1-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Condition ENCLS-exiting enabling on CPU support for SGX1 | expand |
Sean Christopherson <sean.j.christopherson@intel.com> writes: > Enable ENCLS-exiting (and thus set vmcs.ENCLS_EXITING_BITMAP) only if > the CPU supports SGX1. Per Intel's SDM, all ENCLS leafs #UD if SGX1 > is not supported[*], i.e. intercepting ENCLS to inject a #UD is > unnecessary. > > Avoiding ENCLS-exiting even when it is reported as supported by the CPU > works around a reported issue where SGX is "hard" disabled after an S3 > suspend/resume cycle, i.e. CPUID.0x7.SGX=0 and the VMCS field/control > are enumerated as unsupported. While the root cause of the S3 issue is > unknown, it's definitely _not_ a KVM (or kernel) bug, i.e. this is a > workaround for what is most likely a hardware or firmware issue. As a > bonus side effect, KVM saves a VMWRITE when first preparing vmcs01 and > vmcs02. > > Query CPUID directly instead of going through cpu_data() or cpu_has() to > ensure KVM is trapping ENCLS when it's supported in hardware, e.g. even > if X86_FEATURE_SGX1 (which doesn't yet exist in upstream) were disabled > by the kernel/user. I would prefer this paragraph to become a comment right above cpu_has_sgx() or I bet we'll be getting a lot of 'avoid open-coding boot_cpu_has() ...' patches in the future. > > Note, SGX must be disabled in BIOS to take advantage of this workaround > > [*] The additional ENCLS CPUID check on SGX1 exists so that SGX can be > globally "soft" disabled post-reset, e.g. if #MC bits in MCi_CTL are > cleared. Soft disabled meaning disabling SGX without clearing the > primary CPUID bit (in leaf 0x7) and without poking into non-SGX > CPU paths, e.g. for the VMCS controls. > > Fixes: 0b665d304028 ("KVM: vmx: Inject #UD for SGX ENCLS instruction in guest") > Reported-by: Toni Spets <toni.spets@iki.fi> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > > This seems somewhat premature given that we don't yet know if the observed > behavior is a logic bug, a one off manufacturing defect, firmware specific, > etc... On the other hand, the change is arguably an optimization > irrespective of using it as a workaround. > > arch/x86/kvm/vmx/vmx.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 40b1e6138cd5..50cab98382e7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2338,6 +2338,11 @@ static void hardware_disable(void) > kvm_cpu_vmxoff(); > } > > +static bool cpu_has_sgx(void) > +{ > + return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); > +} > + > static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, > u32 msr, u32 *result) > { > @@ -2418,8 +2423,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE | > SECONDARY_EXEC_PT_USE_GPA | > SECONDARY_EXEC_PT_CONCEAL_VMX | > - SECONDARY_EXEC_ENABLE_VMFUNC | > - SECONDARY_EXEC_ENCLS_EXITING; > + SECONDARY_EXEC_ENABLE_VMFUNC; > + if (cpu_has_sgx()) > + opt2 |= SECONDARY_EXEC_ENCLS_EXITING; > if (adjust_vmx_controls(min2, opt2, > MSR_IA32_VMX_PROCBASED_CTLS2, > &_cpu_based_2nd_exec_control) < 0) Reviewed-by: Vitaly Kuznetsov <vkuznets@redhat.com>
On 12/03/20 19:04, Sean Christopherson wrote: > Enable ENCLS-exiting (and thus set vmcs.ENCLS_EXITING_BITMAP) only if > the CPU supports SGX1. Per Intel's SDM, all ENCLS leafs #UD if SGX1 > is not supported[*], i.e. intercepting ENCLS to inject a #UD is > unnecessary. > > Avoiding ENCLS-exiting even when it is reported as supported by the CPU > works around a reported issue where SGX is "hard" disabled after an S3 > suspend/resume cycle, i.e. CPUID.0x7.SGX=0 and the VMCS field/control > are enumerated as unsupported. While the root cause of the S3 issue is > unknown, it's definitely _not_ a KVM (or kernel) bug, i.e. this is a > workaround for what is most likely a hardware or firmware issue. As a > bonus side effect, KVM saves a VMWRITE when first preparing vmcs01 and > vmcs02. > > Query CPUID directly instead of going through cpu_data() or cpu_has() to > ensure KVM is trapping ENCLS when it's supported in hardware, e.g. even > if X86_FEATURE_SGX1 (which doesn't yet exist in upstream) were disabled > by the kernel/user. > > Note, SGX must be disabled in BIOS to take advantage of this workaround > > [*] The additional ENCLS CPUID check on SGX1 exists so that SGX can be > globally "soft" disabled post-reset, e.g. if #MC bits in MCi_CTL are > cleared. Soft disabled meaning disabling SGX without clearing the > primary CPUID bit (in leaf 0x7) and without poking into non-SGX > CPU paths, e.g. for the VMCS controls. > > Fixes: 0b665d304028 ("KVM: vmx: Inject #UD for SGX ENCLS instruction in guest") > Reported-by: Toni Spets <toni.spets@iki.fi> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > > This seems somewhat premature given that we don't yet know if the observed > behavior is a logic bug, a one off manufacturing defect, firmware specific, > etc... On the other hand, the change is arguably an optimization > irrespective of using it as a workaround. > > arch/x86/kvm/vmx/vmx.c | 10 ++++++++-- > 1 file changed, 8 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 40b1e6138cd5..50cab98382e7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2338,6 +2338,11 @@ static void hardware_disable(void) > kvm_cpu_vmxoff(); > } > > +static bool cpu_has_sgx(void) > +{ > + return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); > +} > + > static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, > u32 msr, u32 *result) > { > @@ -2418,8 +2423,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE | > SECONDARY_EXEC_PT_USE_GPA | > SECONDARY_EXEC_PT_CONCEAL_VMX | > - SECONDARY_EXEC_ENABLE_VMFUNC | > - SECONDARY_EXEC_ENCLS_EXITING; > + SECONDARY_EXEC_ENABLE_VMFUNC; > + if (cpu_has_sgx()) > + opt2 |= SECONDARY_EXEC_ENCLS_EXITING; > if (adjust_vmx_controls(min2, opt2, > MSR_IA32_VMX_PROCBASED_CTLS2, > &_cpu_based_2nd_exec_control) < 0) > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 40b1e6138cd5..50cab98382e7 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2338,6 +2338,11 @@ static void hardware_disable(void) kvm_cpu_vmxoff(); } +static bool cpu_has_sgx(void) +{ + return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); +} + static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32 msr, u32 *result) { @@ -2418,8 +2423,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE | SECONDARY_EXEC_PT_USE_GPA | SECONDARY_EXEC_PT_CONCEAL_VMX | - SECONDARY_EXEC_ENABLE_VMFUNC | - SECONDARY_EXEC_ENCLS_EXITING; + SECONDARY_EXEC_ENABLE_VMFUNC; + if (cpu_has_sgx()) + opt2 |= SECONDARY_EXEC_ENCLS_EXITING; if (adjust_vmx_controls(min2, opt2, MSR_IA32_VMX_PROCBASED_CTLS2, &_cpu_based_2nd_exec_control) < 0)
Enable ENCLS-exiting (and thus set vmcs.ENCLS_EXITING_BITMAP) only if the CPU supports SGX1. Per Intel's SDM, all ENCLS leafs #UD if SGX1 is not supported[*], i.e. intercepting ENCLS to inject a #UD is unnecessary. Avoiding ENCLS-exiting even when it is reported as supported by the CPU works around a reported issue where SGX is "hard" disabled after an S3 suspend/resume cycle, i.e. CPUID.0x7.SGX=0 and the VMCS field/control are enumerated as unsupported. While the root cause of the S3 issue is unknown, it's definitely _not_ a KVM (or kernel) bug, i.e. this is a workaround for what is most likely a hardware or firmware issue. As a bonus side effect, KVM saves a VMWRITE when first preparing vmcs01 and vmcs02. Query CPUID directly instead of going through cpu_data() or cpu_has() to ensure KVM is trapping ENCLS when it's supported in hardware, e.g. even if X86_FEATURE_SGX1 (which doesn't yet exist in upstream) were disabled by the kernel/user. Note, SGX must be disabled in BIOS to take advantage of this workaround [*] The additional ENCLS CPUID check on SGX1 exists so that SGX can be globally "soft" disabled post-reset, e.g. if #MC bits in MCi_CTL are cleared. Soft disabled meaning disabling SGX without clearing the primary CPUID bit (in leaf 0x7) and without poking into non-SGX CPU paths, e.g. for the VMCS controls. Fixes: 0b665d304028 ("KVM: vmx: Inject #UD for SGX ENCLS instruction in guest") Reported-by: Toni Spets <toni.spets@iki.fi> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com> --- This seems somewhat premature given that we don't yet know if the observed behavior is a logic bug, a one off manufacturing defect, firmware specific, etc... On the other hand, the change is arguably an optimization irrespective of using it as a workaround. arch/x86/kvm/vmx/vmx.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)