Jim Mattson [off-list ref] writes:

On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov [off-list ref] wrote:

quoted

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.

quoted

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).

quoted

+ * 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 :-)

quoted

+ * 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/ (local).

I'm going to steal your patch and put it to my series (as I don't see it
in kvm/queue yet).

quoted

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

quoted

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

-- 
Vitaly