Re: [PATCH] Revert "KVM: VMX: Move LOAD_IA32_PERF_GLOBAL_CTRL errata handling out of setup_vmcs_config()"

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> 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@redhat.com>

>
> Link: https://lore.kernel.org/all/YtnZmCutdd5tpUmz@google.com
> Fixes: 9d78d6fb186b ("KVM: VMX: Move LOAD_IA32_PERF_GLOBAL_CTRL errata handling out of setup_vmcs_config()")
> Cc: stable@vger.kernel.org
> Cc: Vitaly Kuznetsov <vkuznets@redhat.com>
> Cc: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>
> 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