Sean Christopherson <seanjc@xxxxxxxxxx> writes:
> Revert back to clearing VM_{ENTRY,EXIT}_LOAD_IA32_PERF_GLOBAL_CTRL in KVM's
> golden VMCS config, as applying the workaround during vCPU creation is
> pointless and broken. KVM *unconditionally* clears the controls in the
> values returned by vmx_vmentry_ctrl() and vmx_vmexit_ctrl(), as KVM loads
> PERF_GLOBAL_CTRL if and only if its necessary to do so. E.g. if KVM wants
> to run the guest with the same PERF_GLOBAL_CTRL as the host, then there's
> no need to re-load the MSR on entry and exit.
Agreed with 'pointless' part ...
>
> Even worse, the buggy commit failed to apply the erratum where it's
> actually needed, add_atomic_switch_msr(). As a result, KVM completely
> ignores the erratum for all intents and purposes, i.e. uses the flawed
> VMCS controls to load PERF_GLOBAL_CTRL.
... and with 'broken'. I think the original commit was never tested on
these old CPUs which need it in the first place.
>
> To top things off, the patch was intended to be dropped, as the premise
> of an L1 VMM being able to pivot on FMS is flawed, and KVM can (and now
> does) fully emulate the controls in software. Simply revert the commit,
> as all upstream supported kernels that have the buggy commit should also
> have commit f4c93d1a0e71 ("KVM: nVMX: Always emulate PERF_GLOBAL_CTRL
> VM-Entry/VM-Exit controls"), i.e. the (likely theoretical) live migration
> concern is a complete non-issue.
It seems that both f4c93d1a0e71 and 9d78d6fb186b landed in 6.1. In
theory, the former could've broken some L1s (not necessarily KVMs) by
exposing PERF_GLOBAL_CTRL where it was previously missing but then
it would've been a VMM problem which doesn't sanitize controls and
passes everything KVM exposes to the guest unfiltered.
>
> Opportunistically drop the manual "kvm: " scope from the warning about
> the erratum, as KVM now uses pr_fmt() to provide the correct scope (v6.1
> kernels and earlier don't, but the erratum only applies to CPUs that are
> 15+ years old; it's not worth a separate patch).
>
> This reverts commit 9d78d6fb186bc4aff41b5d6c4726b76649d3cb53.
Reviewed-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>
> Link: https://lore.kernel.org/all/YtnZmCutdd5tpUmz@xxxxxxxxxx
> Fixes: 9d78d6fb186b ("KVM: VMX: Move LOAD_IA32_PERF_GLOBAL_CTRL errata handling out of setup_vmcs_config()")
> Cc: stable@xxxxxxxxxxxxxxx
> Cc: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
> Cc: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
> ---
>
> Found by inspection, verified by hacking cpu_has_perf_global_ctrl_bug() to
> unconditionally return true, and warning if add_atomic_switch_msr_special()
> was called with for PERF_GLOBAL_GTRL.
s,GTRL,CTRL, even though it's below '---' :-)
>
> arch/x86/kvm/vmx/vmx.c | 54 ++++++++++++++++--------------------------
> 1 file changed, 21 insertions(+), 33 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index d28618e9277e..92fee5e8a3c7 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2551,28 +2551,6 @@ 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 errata where VM Exit may incorrectly clear
> - * IA32_PERF_GLOBAL_CTRL[34:32]. Work around the errata by using the
> - * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL.
> - */
> -static bool cpu_has_perf_global_ctrl_bug(void)
> -{
> - switch (boot_cpu_data.x86_vfm) {
> - case INTEL_NEHALEM_EP: /* AAK155 */
> - case INTEL_NEHALEM: /* AAP115 */
> - case INTEL_WESTMERE: /* AAT100 */
> - case INTEL_WESTMERE_EP: /* BC86,AAY89,BD102 */
> - case INTEL_NEHALEM_EX: /* BA97 */
> - return true;
> - default:
> - break;
> - }
> -
> - return false;
> -}
> -
> static int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt, u32 msr, u32 *result)
> {
> u32 vmx_msr_low, vmx_msr_high;
> @@ -2732,6 +2710,27 @@ static 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.
> + */
> + switch (boot_cpu_data.x86_vfm) {
> + case INTEL_NEHALEM_EP: /* AAK155 */
> + case INTEL_NEHALEM: /* AAP115 */
> + case INTEL_WESTMERE: /* AAT100 */
> + case INTEL_WESTMERE_EP: /* BC86,AAY89,BD102 */
> + case INTEL_NEHALEM_EX: /* BA97 */
> + _vmentry_control &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
> + _vmexit_control &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
> + pr_warn_once("VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL "
> + "does not work properly. Using workaround\n");
> + break;
> + default:
> + break;
> + }
> +
> rdmsrl(MSR_IA32_VMX_BASIC, basic_msr);
>
> /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
> @@ -4422,9 +4421,6 @@ 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;
> }
>
> @@ -4442,10 +4438,6 @@ 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);
> @@ -8400,10 +8392,6 @@ __init int vmx_hardware_setup(void)
> if (setup_vmcs_config(&vmcs_config, &vmx_capability) < 0)
> return -EIO;
>
> - if (cpu_has_perf_global_ctrl_bug())
> - pr_warn_once("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);
>
>
> base-commit: adc218676eef25575469234709c2d87185ca223a
--
Vitaly
