| Message ID | 20220629150625.238286-24-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: > > As a preparation to reusing the result of setup_vmcs_config() for setting > up nested VMX control MSRs, move LOAD_IA32_PERF_GLOBAL_CTRL errata handling > to vmx_vmexit_ctrl()/vmx_vmentry_ctrl() and print the warning from > hardware_setup(). While it seems reasonable to not expose > LOAD_IA32_PERF_GLOBAL_CTRL controls to L1 hypervisor on buggy CPUs, > such change would inevitably break live migration from older KVMs > where the controls are exposed. Keep the status quo for know, L1 hypervisor > itself is supposed to take care of the errata. It can only do that if L1 doesn't lie about the model. This is why F/M/S checks are, in general, evil. > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > --- > arch/x86/kvm/vmx/vmx.c | 62 ++++++++++++++++++++++++++---------------- > 1 file changed, 38 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index fb58b0be953d..5f7ef1f8d2c6 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -2416,6 +2416,31 @@ static bool cpu_has_sgx(void) > return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); > } > > +/* > + * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they > + * can't be used due to an errata where VM Exit may incorrectly clear Nit: erratum (singular), or drop the 'an' to refer to errata (plural). > + * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the Nit: workaround (one word) is a noun. The verb form is "work around." > + * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. > + */ > +static bool cpu_has_perf_global_ctrl_bug(void) > +{ > + if (boot_cpu_data.x86 == 0x6) { > + switch (boot_cpu_data.x86_model) { > + case 26: /* AAK155 */ > + case 30: /* AAP115 */ > + case 37: /* AAT100 */ > + case 44: /* BC86,AAY89,BD102 */ > + case 46: /* BA97 */ Nit: Replace decimal model numbers with mnemonics. See https://lore.kernel.org/kvm/20220629222221.986645-1-jmattson@google.com/. > + return true; > + default: > + break; > + } > + } > + > + return false; > +} Is it worth either (a) memoizing the result, or (b) toggling a static branch? Or am I prematurely optimizing? > + > + > static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, > u32 msr, u32 *result) > { > @@ -2572,30 +2597,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, > _vmexit_control &= ~x_ctrl; > } > > - /* > - * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they > - * can't be used due to an errata where VM Exit may incorrectly clear > - * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the > - * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. > - */ > - if (boot_cpu_data.x86 == 0x6) { > - switch (boot_cpu_data.x86_model) { > - case 26: /* AAK155 */ > - case 30: /* AAP115 */ > - case 37: /* AAT100 */ > - case 44: /* BC86,AAY89,BD102 */ > - case 46: /* BA97 */ > - _vmentry_control &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; > - _vmexit_control &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; > - pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " > - "does not work properly. Using workaround\n"); > - break; > - default: > - break; > - } > - } > - > - > rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high); > > /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */ > @@ -4188,6 +4189,10 @@ static u32 vmx_vmentry_ctrl(void) > VM_ENTRY_LOAD_IA32_EFER | > VM_ENTRY_IA32E_MODE); > > + > + if (cpu_has_perf_global_ctrl_bug()) > + vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; > + > return vmentry_ctrl; > } > > @@ -4202,6 +4207,10 @@ static u32 vmx_vmexit_ctrl(void) > if (vmx_pt_mode_is_system()) > vmexit_ctrl &= ~(VM_EXIT_PT_CONCEAL_PIP | > VM_EXIT_CLEAR_IA32_RTIT_CTL); > + > + if (cpu_has_perf_global_ctrl_bug()) > + vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; > + > /* Loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically */ > return vmexit_ctrl & > ~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER); > @@ -8117,6 +8126,11 @@ static __init int hardware_setup(void) > if (setup_vmcs_config(&vmcs_config, &vmx_capability) < 0) > return -EIO; > > + if (cpu_has_perf_global_ctrl_bug()) { > + pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " > + "does not work properly. Using workaround\n"); > + } > + > if (boot_cpu_has(X86_FEATURE_NX)) > kvm_enable_efer_bits(EFER_NX); > > -- > 2.35.3 >
Jim Mattson <jmattson@google.com> writes: > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote: >> >> As a preparation to reusing the result of setup_vmcs_config() for setting >> up nested VMX control MSRs, move LOAD_IA32_PERF_GLOBAL_CTRL errata handling >> to vmx_vmexit_ctrl()/vmx_vmentry_ctrl() and print the warning from >> hardware_setup(). While it seems reasonable to not expose >> LOAD_IA32_PERF_GLOBAL_CTRL controls to L1 hypervisor on buggy CPUs, >> such change would inevitably break live migration from older KVMs >> where the controls are exposed. Keep the status quo for know, L1 hypervisor >> itself is supposed to take care of the errata. > > It can only do that if L1 doesn't lie about the model. This is why > F/M/S checks are, in general, evil. > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> >> --- >> arch/x86/kvm/vmx/vmx.c | 62 ++++++++++++++++++++++++++---------------- >> 1 file changed, 38 insertions(+), 24 deletions(-) >> >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index fb58b0be953d..5f7ef1f8d2c6 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -2416,6 +2416,31 @@ static bool cpu_has_sgx(void) >> return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); >> } >> >> +/* >> + * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they >> + * can't be used due to an errata where VM Exit may incorrectly clear > > Nit: erratum (singular), or drop the 'an' to refer to errata (plural). > >> + * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the > > Nit: workaround (one word) is a noun. The verb form is "work around." > Sure, but I'm not exactly certain which commit to blame here as I have options: Fixes: 8bf00a529967 ("KVM: VMX: add support for switching of PERF_GLOBAL_CTRL") where it was introduced or Fixes: bb3541f175a9 ("KVM: x86: Fix typos") Fixes: c73da3fcab43 ("KVM: VMX: Properly handle dynamic VM Entry/Exit controls") where it was preserved :-) >> + * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. >> + */ >> +static bool cpu_has_perf_global_ctrl_bug(void) >> +{ >> + if (boot_cpu_data.x86 == 0x6) { >> + switch (boot_cpu_data.x86_model) { >> + case 26: /* AAK155 */ >> + case 30: /* AAP115 */ >> + case 37: /* AAT100 */ >> + case 44: /* BC86,AAY89,BD102 */ >> + case 46: /* BA97 */ > > Nit: Replace decimal model numbers with mnemonics. See > https://lore.kernel.org/kvm/20220629222221.986645-1-jmattson@google.com/. > I'm going to steal your patch and put it to my series (as I don't see it in kvm/queue yet). >> + return true; >> + default: >> + break; >> + } >> + } >> + >> + return false; >> +} > > Is it worth either (a) memoizing the result, or (b) toggling a static > branch? Or am I prematurely optimizing? > (Unless I missed something) besides hardware_setup(), cpu_has_perf_global_ctrl_bug() is only called (twice) from: vmx_vcpu_reset() __vmx_vcpu_reset() init_vmcs() vmx_vmentry_ctrl() vmx_vmexit_ctrl() this shouldn't happen very often so I guess we can leave it un-optimized. Also, we're only reading boot_cpu_data.x86/ boot_cpu_data.x86_model here, this should be cheap). >> + >> + >> static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, >> u32 msr, u32 *result) >> { >> @@ -2572,30 +2597,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, >> _vmexit_control &= ~x_ctrl; >> } >> >> - /* >> - * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they >> - * can't be used due to an errata where VM Exit may incorrectly clear >> - * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the >> - * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. >> - */ >> - if (boot_cpu_data.x86 == 0x6) { >> - switch (boot_cpu_data.x86_model) { >> - case 26: /* AAK155 */ >> - case 30: /* AAP115 */ >> - case 37: /* AAT100 */ >> - case 44: /* BC86,AAY89,BD102 */ >> - case 46: /* BA97 */ >> - _vmentry_control &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; >> - _vmexit_control &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; >> - pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " >> - "does not work properly. Using workaround\n"); >> - break; >> - default: >> - break; >> - } >> - } >> - >> - >> rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high); >> >> /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */ >> @@ -4188,6 +4189,10 @@ static u32 vmx_vmentry_ctrl(void) >> VM_ENTRY_LOAD_IA32_EFER | >> VM_ENTRY_IA32E_MODE); >> >> + >> + if (cpu_has_perf_global_ctrl_bug()) >> + vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; >> + >> return vmentry_ctrl; >> } >> >> @@ -4202,6 +4207,10 @@ static u32 vmx_vmexit_ctrl(void) >> if (vmx_pt_mode_is_system()) >> vmexit_ctrl &= ~(VM_EXIT_PT_CONCEAL_PIP | >> VM_EXIT_CLEAR_IA32_RTIT_CTL); >> + >> + if (cpu_has_perf_global_ctrl_bug()) >> + vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; >> + >> /* Loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically */ >> return vmexit_ctrl & >> ~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER); >> @@ -8117,6 +8126,11 @@ static __init int hardware_setup(void) >> if (setup_vmcs_config(&vmcs_config, &vmx_capability) < 0) >> return -EIO; >> >> + if (cpu_has_perf_global_ctrl_bug()) { >> + pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " >> + "does not work properly. Using workaround\n"); >> + } >> + >> if (boot_cpu_has(X86_FEATURE_NX)) >> kvm_enable_efer_bits(EFER_NX); >> >> -- >> 2.35.3 >> >
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index fb58b0be953d..5f7ef1f8d2c6 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -2416,6 +2416,31 @@ static bool cpu_has_sgx(void) return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0)); } +/* + * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they + * can't be used due to an errata where VM Exit may incorrectly clear + * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the + * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. + */ +static bool cpu_has_perf_global_ctrl_bug(void) +{ + if (boot_cpu_data.x86 == 0x6) { + switch (boot_cpu_data.x86_model) { + case 26: /* AAK155 */ + case 30: /* AAP115 */ + case 37: /* AAT100 */ + case 44: /* BC86,AAY89,BD102 */ + case 46: /* BA97 */ + return true; + default: + break; + } + } + + return false; +} + + static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32 msr, u32 *result) { @@ -2572,30 +2597,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf, _vmexit_control &= ~x_ctrl; } - /* - * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they - * can't be used due to an errata where VM Exit may incorrectly clear - * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the - * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL. - */ - if (boot_cpu_data.x86 == 0x6) { - switch (boot_cpu_data.x86_model) { - case 26: /* AAK155 */ - case 30: /* AAP115 */ - case 37: /* AAT100 */ - case 44: /* BC86,AAY89,BD102 */ - case 46: /* BA97 */ - _vmentry_control &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; - _vmexit_control &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; - pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " - "does not work properly. Using workaround\n"); - break; - default: - break; - } - } - - rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high); /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */ @@ -4188,6 +4189,10 @@ static u32 vmx_vmentry_ctrl(void) VM_ENTRY_LOAD_IA32_EFER | VM_ENTRY_IA32E_MODE); + + if (cpu_has_perf_global_ctrl_bug()) + vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL; + return vmentry_ctrl; } @@ -4202,6 +4207,10 @@ static u32 vmx_vmexit_ctrl(void) if (vmx_pt_mode_is_system()) vmexit_ctrl &= ~(VM_EXIT_PT_CONCEAL_PIP | VM_EXIT_CLEAR_IA32_RTIT_CTL); + + if (cpu_has_perf_global_ctrl_bug()) + vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL; + /* Loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically */ return vmexit_ctrl & ~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER); @@ -8117,6 +8126,11 @@ static __init int hardware_setup(void) if (setup_vmcs_config(&vmcs_config, &vmx_capability) < 0) return -EIO; + if (cpu_has_perf_global_ctrl_bug()) { + pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL " + "does not work properly. Using workaround\n"); + } + if (boot_cpu_has(X86_FEATURE_NX)) kvm_enable_efer_bits(EFER_NX);
As a preparation to reusing the result of setup_vmcs_config() for setting up nested VMX control MSRs, move LOAD_IA32_PERF_GLOBAL_CTRL errata handling to vmx_vmexit_ctrl()/vmx_vmentry_ctrl() and print the warning from hardware_setup(). While it seems reasonable to not expose LOAD_IA32_PERF_GLOBAL_CTRL controls to L1 hypervisor on buggy CPUs, such change would inevitably break live migration from older KVMs where the controls are exposed. Keep the status quo for know, L1 hypervisor itself is supposed to take care of the errata. Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- arch/x86/kvm/vmx/vmx.c | 62 ++++++++++++++++++++++++++---------------- 1 file changed, 38 insertions(+), 24 deletions(-)