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
