Revert "KVM: VMX: Move LOAD_IA32_PERF_GLOBAL_CTRL errata handling out of setup_vmcs_config()"

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

Posted by Sean Christopherson 2 days, 14 hours ago

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.

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.

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.

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.

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.

 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
-- 
2.47.0.338.g60cca15819-goog

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

Posted by Paolo Bonzini 1 day, 15 hours ago

Queued, thanks.

Paolo

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

Posted by Vitaly Kuznetsov 2 days, 4 hours ago

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