Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 21 Jul 2022 22:56:24 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vitaly Kuznetsov <vkuznets@redhat.com>
Cc:     kvm@vger.kernel.org, Paolo Bonzini <pbonzini@redhat.com>,
        Anirudh Rayabharam <anrayabh@linux.microsoft.com>,
        Wanpeng Li <wanpengli@tencent.com>,
        Jim Mattson <jmattson@google.com>,
        Maxim Levitsky <mlevitsk@redhat.com>,
        linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 21/25] KVM: VMX: Move LOAD_IA32_PERF_GLOBAL_CTRL
 errata handling out of setup_vmcs_config()
Message-ID: <YtnZmCutdd5tpUmz@google.com>
References: <20220714091327.1085353-1-vkuznets@redhat.com>
 <20220714091327.1085353-22-vkuznets@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220714091327.1085353-22-vkuznets@redhat.com>
Precedence: bulk

On Thu, Jul 14, 2022, Vitaly Kuznetsov 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

s/know/now

> itself is supposed to take care of the errata.

Except the errata are based on FMS and the FMS exposed to the L1 hypervisor may
not be the real FMS.

But that's moot, because they _should_ be fully emulated by KVM anyways; KVM
runs L2 with a MSR value modified by perf, not the raw MSR value requested by L1.

Of course KVM screws things up and fails to clear the flag in entry controls...
All exit controls are emulated so at least KVM gets those right.

Untested, but I believe KVM the fix is:

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index d0e781c7ac72..76926147b672 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -2357,7 +2357,8 @@ static void prepare_vmcs02_early(struct vcpu_vmx *vmx, struct loaded_vmcs *vmcs0
         * we can avoid VMWrites during vmx_set_efer().
         */
        exec_control = __vm_entry_controls_get(vmcs01);
-       exec_control |= vmcs12->vm_entry_controls;
+       exec_control |= (vmcs12->vm_entry_controls &
+                        ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL);
        exec_control &= ~(VM_ENTRY_IA32E_MODE | VM_ENTRY_LOAD_IA32_EFER);
        if (cpu_has_load_ia32_efer()) {
                if (guest_efer & EFER_LMA)

> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> 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 2dff5b94c535..e462e5b9c0a1 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 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)
> +{
> +	if (boot_cpu_data.x86 == 0x6) {
> +		switch (boot_cpu_data.x86_model) {
> +		case INTEL_FAM6_NEHALEM_EP:	/* AAK155 */
> +		case INTEL_FAM6_NEHALEM:	/* AAP115 */
> +		case INTEL_FAM6_WESTMERE:	/* AAT100 */
> +		case INTEL_FAM6_WESTMERE_EP:	/* BC86,AAY89,BD102 */
> +		case INTEL_FAM6_NEHALEM_EX:	/* 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 INTEL_FAM6_NEHALEM_EP:	/* AAK155 */
> -		case INTEL_FAM6_NEHALEM:	/* AAP115 */
> -		case INTEL_FAM6_WESTMERE:	/* AAT100 */
> -		case INTEL_FAM6_WESTMERE_EP:	/* BC86,AAY89,BD102 */
> -		case INTEL_FAM6_NEHALEM_EX:	/* 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. */
> @@ -4184,6 +4185,10 @@ static u32 vmx_vmentry_ctrl(void)
>  			  VM_ENTRY_LOAD_IA32_EFER |
>  			  VM_ENTRY_IA32E_MODE);
>  
> +

Extra line.

> +	if (cpu_has_perf_global_ctrl_bug())
> +		vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
> +
>  	return vmentry_ctrl;
>  }
>  
> @@ -4198,6 +4203,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);
> @@ -8113,6 +8122,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()) {

Curly braces not needed.

> +		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
>