| Message ID | 20220629150625.238286-17-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: VMX: Support TscScaling and EnclsExitingBitmap with eVMCS + use vmcs_config for L1 VMX MSRs | expand |
On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: > > SECONDARY_EXEC_ENCLS_EXITING is conditionally added to the 'optional' > checklist in setup_vmcs_config() but there's little value in doing so. > First, as the control is optional, we can always check for its > presence, no harm done. Second, the only real value cpu_has_sgx() check > gives is that on the CPUs which support SECONDARY_EXEC_ENCLS_EXITING but > don't support SGX, the control is not getting enabled. It's highly unlikely > such CPUs exist but it's possible that some hypervisors expose broken vCPU > models. > > Preserve cpu_has_sgx() check but filter the result of adjust_vmx_controls() > instead of the input. > > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/vmx.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 89a3bbafa5af..e32d91006b80 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2528,9 +2528,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > SECONDARY_EXEC_PT_CONCEAL_VMX | > SECONDARY_EXEC_ENABLE_VMFUNC | > SECONDARY_EXEC_BUS_LOCK_DETECTION | > - SECONDARY_EXEC_NOTIFY_VM_EXITING; > - if (cpu_has_sgx()) > - opt2 |= SECONDARY_EXEC_ENCLS_EXITING; > + SECONDARY_EXEC_NOTIFY_VM_EXITING | > + SECONDARY_EXEC_ENCLS_EXITING; > + > if (adjust_vmx_controls(min2, opt2, > MSR_IA32_VMX_PROCBASED_CTLS2, > &_cpu_based_2nd_exec_control) < 0) > @@ -2577,6 +2577,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > vmx_cap->vpid = 0; > } > > + if (!cpu_has_sgx()) > + _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING; NYC, but why is there a leading underscore here? > if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) { > u64 opt3 = TERTIARY_EXEC_IPI_VIRT; > > -- > 2.35.3 > Reviewed-by: Jim Mattson <jmattson@google.com>
Jim Mattson <jmattson@google.com> writes: > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> SECONDARY_EXEC_ENCLS_EXITING is conditionally added to the 'optional' >> checklist in setup_vmcs_config() but there's little value in doing so. >> First, as the control is optional, we can always check for its >> presence, no harm done. Second, the only real value cpu_has_sgx() check >> gives is that on the CPUs which support SECONDARY_EXEC_ENCLS_EXITING but >> don't support SGX, the control is not getting enabled. It's highly unlikely >> such CPUs exist but it's possible that some hypervisors expose broken vCPU >> models. >> >> Preserve cpu_has_sgx() check but filter the result of adjust_vmx_controls() >> instead of the input. >> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 89a3bbafa5af..e32d91006b80 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -2528,9 +2528,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, >> SECONDARY_EXEC_PT_CONCEAL_VMX | >> SECONDARY_EXEC_ENABLE_VMFUNC | >> SECONDARY_EXEC_BUS_LOCK_DETECTION | >> - SECONDARY_EXEC_NOTIFY_VM_EXITING; >> - if (cpu_has_sgx()) >> - opt2 |= SECONDARY_EXEC_ENCLS_EXITING; >> + SECONDARY_EXEC_NOTIFY_VM_EXITING | >> + SECONDARY_EXEC_ENCLS_EXITING; >> + >> if (adjust_vmx_controls(min2, opt2, >> MSR_IA32_VMX_PROCBASED_CTLS2, >> &_cpu_based_2nd_exec_control) < 0) >> @@ -2577,6 +2577,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, >> vmx_cap->vpid = 0; >> } >> >> + if (!cpu_has_sgx()) >> + _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING; > > NYC, but why is there a leading underscore here? No idea to be honest, this goes way back to 2007 when setup_vmcs_config() was introduced: commit 1c3d14fe0ab75337a3f6c06b6bc18bcbc2b3d0bc Author: Yang, Sheng <sheng.yang@intel.com> Date: Sun Jul 29 11:07:42 2007 +0300 KVM: VMX: Improve the method of writing vmcs control > >> if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) { >> u64 opt3 = TERTIARY_EXEC_IPI_VIRT; >> >> -- >> 2.35.3 >> > Reviewed-by: Jim Mattson <jmattson@google.com> > Thanks!
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 89a3bbafa5af..e32d91006b80 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2528,9 +2528,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, SECONDARY_EXEC_PT_CONCEAL_VMX | SECONDARY_EXEC_ENABLE_VMFUNC | SECONDARY_EXEC_BUS_LOCK_DETECTION | - SECONDARY_EXEC_NOTIFY_VM_EXITING; - if (cpu_has_sgx()) - opt2 |= SECONDARY_EXEC_ENCLS_EXITING; + SECONDARY_EXEC_NOTIFY_VM_EXITING | + SECONDARY_EXEC_ENCLS_EXITING; + if (adjust_vmx_controls(min2, opt2, MSR_IA32_VMX_PROCBASED_CTLS2, &_cpu_based_2nd_exec_control) < 0) @@ -2577,6 +2577,9 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, vmx_cap->vpid = 0; } + if (!cpu_has_sgx()) + _cpu_based_2nd_exec_control &= ~SECONDARY_EXEC_ENCLS_EXITING; + if (_cpu_based_exec_control & CPU_BASED_ACTIVATE_TERTIARY_CONTROLS) { u64 opt3 = TERTIARY_EXEC_IPI_VIRT;
SECONDARY_EXEC_ENCLS_EXITING is conditionally added to the 'optional' checklist in setup_vmcs_config() but there's little value in doing so. First, as the control is optional, we can always check for its presence, no harm done. Second, the only real value cpu_has_sgx() check gives is that on the CPUs which support SECONDARY_EXEC_ENCLS_EXITING but don't support SGX, the control is not getting enabled. It's highly unlikely such CPUs exist but it's possible that some hypervisors expose broken vCPU models. Preserve cpu_has_sgx() check but filter the result of adjust_vmx_controls() instead of the input. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/vmx.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)