Re: [PATCH v3 03/27] KVM: VMX: Add support for the secondary VM exit controls

[Sean Christopherson <seanjc@google.com>]

[-- Attachment #1: Type: text/plain, Size: 3427 bytes --]

On Tue, Oct 22, 2024, Xin Li wrote:
> > > > > 		_vmentry_control &= ~n_ctrl;
> > > > > 		_vmexit_control &= ~x_ctrl;
> > > > 
> > > > w/ patch 4, VM_EXIT_ACTIVATE_SECONDARY_CONTROLS is cleared if FRED fails in the
> > > > consistent check. this means, all features in the secondary vm-exit controls
> > > > are removed. it is overkill.
> > > 
> > > Good catch!
> > > 
> > > > 
> > > > I prefer to maintain a separate table for the secondary VM-exit controls:
> > > > 
> > > >    	struct {
> > > >    		u32 entry_control;
> > > >    		u64 exit2_control;
> > > > 	} const vmcs_entry_exit2_pairs[] = {
> > > > 		{ VM_ENTRY_LOAD_IA32_FRED, SECONDARY_VM_EXIT_SAVE_IA32_FRED |
> > > > 					   SECONDARY_VM_EXIT_LOAD_IA32_FRED},
> > > > 	};
> > > > 
> > > > 	for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit2_pairs); i++) {
> > > > 	...
> > > > 	}
> > > 
> > > Hmm, I prefer one table, as it's more straight forward.

Heh, that's debatable.  Also, calling these triplets is *very* misleading.

> > One table is fine if we can fix the issue and improve readability. The three
> > nested if() statements hurts readability.
> 
> You're right!  Let's try to make it clearer.

I agree with Chao, two tables provides better separation, which makes it easier
to follow what's going on, and avoids "polluting" every entry with empty fields.

If it weren't for the new controls supporting 64 unique bits, and the need to
clear bits in KVM's controls, it'd be trivial to extract processing to a helper
function.  But, it's easy enough to solve that conundrum by using a macro instead
of a function.  And as a bonus, a macro allows for adding compile-time assertions
to detect typos, e.g. can detect if KVM passes in secondary controls (u64) pairs
with the primary controls (u32) variable.

I'll post the attached patch shortly.  I verified it works as expected with a
simulated "bad" FRED CPU.

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index c9e5576d99d0..4717d48eabe8 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2621,6 +2621,7 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
        u32 _vmentry_control = 0;
        u64 basic_msr;
        u64 misc_msr;
+       u64 _vmexit2_control = BIT_ULL(1);
 
        /*
         * LOAD/SAVE_DEBUG_CONTROLS are absent because both are mandatory.
@@ -2638,6 +2639,13 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
                { VM_ENTRY_LOAD_IA32_RTIT_CTL,          VM_EXIT_CLEAR_IA32_RTIT_CTL },
        };
 
+       struct {
+               u32 entry_control;
+               u64 exit_control;
+       } const vmcs_entry_exit2_pairs[] = {
+               { 0x00800000,                           BIT_ULL(0) | BIT_ULL(1) },
+       };
+
        memset(vmcs_conf, 0, sizeof(*vmcs_conf));
 
        if (adjust_vmx_controls(KVM_REQUIRED_VMX_CPU_BASED_VM_EXEC_CONTROL,
@@ -2728,6 +2736,12 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
                                       _vmentry_control, _vmexit_control))
                return -EIO;
 
+       if (vmx_check_entry_exit_pairs(vmcs_entry_exit2_pairs,
+                                      _vmentry_control, _vmexit2_control))
+               return -EIO;
+
+       WARN_ON_ONCE(_vmexit2_control);
+
        /*
         * 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

[-- Attachment #2: 0001-KVM-VMX-Extract-checks-on-entry-exit-control-pairs-t.patch --]
[-- Type: text/x-diff, Size: 4751 bytes --]

From b1def684c93990d1a62c169bb23706137b96b727 Mon Sep 17 00:00:00 2001
From: Sean Christopherson <seanjc@google.com>
Date: Tue, 25 Feb 2025 09:10:32 -0800
Subject: [PATCH] KVM: VMX: Extract checks on entry/exit control pairs to a
 helper macro

Extract the checking of entry/exit pairs to a helper macro so that the
code can be reused to process the upcoming "secondary" exit controls (the
primary exit controls field is out of bits).  Use a macro instead of a
function to support different sized variables (all secondary exit controls
will be optional and so the MSR doesn't have the fixed-0/fixed-1 split).
Taking the largest size as input is trivial, but handling the modification
of KVM's to-be-used controls is much trickier, e.g. would require bitmap
games to clear bits from a 32-bit bitmap vs. a 64-bit bitmap.

Opportunistically add sanity checks to ensure the size of the controls
match (yay, macro!), e.g. to detect bugs where KVM passes in the pairs for
primary exit controls, but its variable for the secondary exit controls.

To help users triage mismatches, print the control bits that are checked,
not just the actual value.  For the foreseeable future, that provides
enough information for a user to determine which fields mismatched.  E.g.
until secondary entry controls comes along, all entry bits and thus all
error messages are guaranteed to be unique.

To avoid returning from a macro, which can get quite dangerous, simply
process all pairs even if error_on_inconsistent_vmcs_config is set.  The
speed at which KVM rejects module load is not at all interesting.

Keep the error message a "once" printk, even though it would be nice to
print out all mismatching pairs.  In practice, the most likely scenario is
that a single pair will be mismatch on all CPUs.  Printing all mismatches
generates redundant messages in that situation, and can be extremely noisy
on systems with large numbers of CPUs.  If a CPU has multiple mismatches,
not printing every bad pair is the least of the user's concerns.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/vmx/vmx.c | 48 +++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index b71392989609..c9e5576d99d0 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2582,6 +2582,34 @@ static u64 adjust_vmx_controls64(u64 ctl_opt, u32 msr)
 	return  ctl_opt & allowed;
 }
 
+#define vmx_check_entry_exit_pairs(pairs, entry_controls, exit_controls)	\
+({										\
+	int i, r = 0;								\
+										\
+	BUILD_BUG_ON(sizeof(pairs[0].entry_control) != sizeof(entry_controls));	\
+	BUILD_BUG_ON(sizeof(pairs[0].exit_control)  != sizeof(exit_controls));	\
+										\
+	for (i = 0; i < ARRAY_SIZE(pairs); i++) {				\
+		typeof(entry_controls) n_ctrl = pairs[i].entry_control;		\
+		typeof(exit_controls) x_ctrl = pairs[i].exit_control;		\
+										\
+		if (!(entry_controls & n_ctrl) == !(exit_controls & x_ctrl))	\
+			continue;						\
+										\
+		pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, " 		\
+			     "entry = %llx (%llx), exit = %llx (%llx)\n",	\
+			    (u64)(entry_controls & n_ctrl), (u64)n_ctrl,	\
+			    (u64)(exit_controls & x_ctrl), (u64)x_ctrl);	\
+										\
+		if (error_on_inconsistent_vmcs_config)				\
+			r = -EIO;						\
+										\
+		entry_controls &= ~n_ctrl;					\
+		exit_controls &= ~x_ctrl;					\
+	}									\
+	r;									\
+})
+
 static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 			     struct vmx_capability *vmx_cap)
 {
@@ -2593,7 +2621,6 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 	u32 _vmentry_control = 0;
 	u64 basic_msr;
 	u64 misc_msr;
-	int i;
 
 	/*
 	 * LOAD/SAVE_DEBUG_CONTROLS are absent because both are mandatory.
@@ -2697,22 +2724,9 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 				&_vmentry_control))
 		return -EIO;
 
-	for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_pairs); i++) {
-		u32 n_ctrl = vmcs_entry_exit_pairs[i].entry_control;
-		u32 x_ctrl = vmcs_entry_exit_pairs[i].exit_control;
-
-		if (!(_vmentry_control & n_ctrl) == !(_vmexit_control & x_ctrl))
-			continue;
-
-		pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
-			     _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
-
-		if (error_on_inconsistent_vmcs_config)
-			return -EIO;
-
-		_vmentry_control &= ~n_ctrl;
-		_vmexit_control &= ~x_ctrl;
-	}
+	if (vmx_check_entry_exit_pairs(vmcs_entry_exit_pairs,
+				       _vmentry_control, _vmexit_control))
+		return -EIO;
 
 	/*
 	 * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they

base-commit: fed48e2967f402f561d80075a20c5c9e16866e53
-- 
2.48.1.658.g4767266eb4-goog