From: Xin Li <xin3.li@intel.com>
Always load the secondary VM exit controls to prepare for FRED enabling.
Extend the VM exit/entry consistency check framework to accommodate this
newly added secondary VM exit controls.
Signed-off-by: Xin Li <xin3.li@intel.com>
Signed-off-by: Xin Li (Intel) <xin@zytor.com>
Tested-by: Shan Kang <shan.kang@intel.com>
Reviewed-by: Chao Gao <chao.gao@intel.com>
---
Change since v2:
* Do FRED controls consistency checks in the VM exit/entry consistency
check framework (Sean Christopherson).
Change since v1:
* Always load the secondary VM exit controls (Sean Christopherson).
---
arch/x86/include/asm/msr-index.h | 1 +
arch/x86/include/asm/vmx.h | 3 ++
arch/x86/kvm/vmx/capabilities.h | 9 +++++-
arch/x86/kvm/vmx/vmcs.h | 1 +
arch/x86/kvm/vmx/vmx.c | 51 ++++++++++++++++++++++++++------
arch/x86/kvm/vmx/vmx.h | 7 ++++-
6 files changed, 61 insertions(+), 11 deletions(-)
diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
index 3ae84c3b8e6d..95b6e2749256 100644
--- a/arch/x86/include/asm/msr-index.h
+++ b/arch/x86/include/asm/msr-index.h
@@ -1178,6 +1178,7 @@
#define MSR_IA32_VMX_TRUE_ENTRY_CTLS 0x00000490
#define MSR_IA32_VMX_VMFUNC 0x00000491
#define MSR_IA32_VMX_PROCBASED_CTLS3 0x00000492
+#define MSR_IA32_VMX_EXIT_CTLS2 0x00000493
/* Resctrl MSRs: */
/* - Intel: */
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index f7fd4369b821..57a37ea06a17 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -106,6 +106,7 @@
#define VM_EXIT_CLEAR_BNDCFGS 0x00800000
#define VM_EXIT_PT_CONCEAL_PIP 0x01000000
#define VM_EXIT_CLEAR_IA32_RTIT_CTL 0x02000000
+#define VM_EXIT_ACTIVATE_SECONDARY_CONTROLS 0x80000000
#define VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR 0x00036dff
@@ -258,6 +259,8 @@ enum vmcs_field {
TERTIARY_VM_EXEC_CONTROL_HIGH = 0x00002035,
PID_POINTER_TABLE = 0x00002042,
PID_POINTER_TABLE_HIGH = 0x00002043,
+ SECONDARY_VM_EXIT_CONTROLS = 0x00002044,
+ SECONDARY_VM_EXIT_CONTROLS_HIGH = 0x00002045,
GUEST_PHYSICAL_ADDRESS = 0x00002400,
GUEST_PHYSICAL_ADDRESS_HIGH = 0x00002401,
VMCS_LINK_POINTER = 0x00002800,
diff --git a/arch/x86/kvm/vmx/capabilities.h b/arch/x86/kvm/vmx/capabilities.h
index cb6588238f46..e8f3ad0f79ee 100644
--- a/arch/x86/kvm/vmx/capabilities.h
+++ b/arch/x86/kvm/vmx/capabilities.h
@@ -59,8 +59,9 @@ struct vmcs_config {
u32 cpu_based_exec_ctrl;
u32 cpu_based_2nd_exec_ctrl;
u64 cpu_based_3rd_exec_ctrl;
- u32 vmexit_ctrl;
u32 vmentry_ctrl;
+ u32 vmexit_ctrl;
+ u64 secondary_vmexit_ctrl;
u64 misc;
struct nested_vmx_msrs nested;
};
@@ -136,6 +137,12 @@ static inline bool cpu_has_tertiary_exec_ctrls(void)
CPU_BASED_ACTIVATE_TERTIARY_CONTROLS;
}
+static inline bool cpu_has_secondary_vmexit_ctrls(void)
+{
+ return vmcs_config.vmexit_ctrl &
+ VM_EXIT_ACTIVATE_SECONDARY_CONTROLS;
+}
+
static inline bool cpu_has_vmx_virtualize_apic_accesses(void)
{
return vmcs_config.cpu_based_2nd_exec_ctrl &
diff --git a/arch/x86/kvm/vmx/vmcs.h b/arch/x86/kvm/vmx/vmcs.h
index b25625314658..ae152a9d1963 100644
--- a/arch/x86/kvm/vmx/vmcs.h
+++ b/arch/x86/kvm/vmx/vmcs.h
@@ -47,6 +47,7 @@ struct vmcs_host_state {
struct vmcs_controls_shadow {
u32 vm_entry;
u32 vm_exit;
+ u64 secondary_vm_exit;
u32 pin;
u32 exec;
u32 secondary_exec;
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 3f6257d88ded..ec548c75c3ef 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2606,6 +2606,7 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
u32 _cpu_based_2nd_exec_control = 0;
u64 _cpu_based_3rd_exec_control = 0;
u32 _vmexit_control = 0;
+ u64 _secondary_vmexit_control = 0;
u32 _vmentry_control = 0;
u64 basic_msr;
u64 misc_msr;
@@ -2619,7 +2620,8 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
struct {
u32 entry_control;
u32 exit_control;
- } const vmcs_entry_exit_pairs[] = {
+ u64 exit_2nd_control;
+ } const vmcs_entry_exit_triplets[] = {
{ VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL, VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL },
{ VM_ENTRY_LOAD_IA32_PAT, VM_EXIT_LOAD_IA32_PAT },
{ VM_ENTRY_LOAD_IA32_EFER, VM_EXIT_LOAD_IA32_EFER },
@@ -2713,21 +2715,43 @@ 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))
+ if (_vmexit_control & VM_EXIT_ACTIVATE_SECONDARY_CONTROLS)
+ _secondary_vmexit_control =
+ adjust_vmx_controls64(KVM_OPTIONAL_VMX_SECONDARY_VM_EXIT_CONTROLS,
+ MSR_IA32_VMX_EXIT_CTLS2);
+
+ for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_triplets); i++) {
+ u32 n_ctrl = vmcs_entry_exit_triplets[i].entry_control;
+ u32 x_ctrl = vmcs_entry_exit_triplets[i].exit_control;
+ u64 x_ctrl_2 = vmcs_entry_exit_triplets[i].exit_2nd_control;
+ bool has_n = n_ctrl && ((_vmentry_control & n_ctrl) == n_ctrl);
+ bool has_x = x_ctrl && ((_vmexit_control & x_ctrl) == x_ctrl);
+ bool has_x_2 = x_ctrl_2 && ((_secondary_vmexit_control & x_ctrl_2) == x_ctrl_2);
+
+ if (x_ctrl_2) {
+ /* Only activate secondary VM exit control bit should be set */
+ if ((_vmexit_control & x_ctrl) == VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
+ if (has_n == has_x_2)
+ continue;
+ } else {
+ /* The feature should not be supported in any control */
+ if (!has_n && !has_x && !has_x_2)
+ continue;
+ }
+ } else if (has_n == has_x) {
continue;
+ }
- pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
- _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
+ pr_warn_once("Inconsistent VM-Entry/VM-Exit triplet, entry = %x, exit = %x, secondary_exit = %llx\n",
+ _vmentry_control & n_ctrl, _vmexit_control & x_ctrl,
+ _secondary_vmexit_control & x_ctrl_2);
if (error_on_inconsistent_vmcs_config)
return -EIO;
_vmentry_control &= ~n_ctrl;
_vmexit_control &= ~x_ctrl;
+ _secondary_vmexit_control &= ~x_ctrl_2;
}
rdmsrl(MSR_IA32_VMX_BASIC, basic_msr);
@@ -2757,8 +2781,9 @@ static int setup_vmcs_config(struct vmcs_config *vmcs_conf,
vmcs_conf->cpu_based_exec_ctrl = _cpu_based_exec_control;
vmcs_conf->cpu_based_2nd_exec_ctrl = _cpu_based_2nd_exec_control;
vmcs_conf->cpu_based_3rd_exec_ctrl = _cpu_based_3rd_exec_control;
- vmcs_conf->vmexit_ctrl = _vmexit_control;
vmcs_conf->vmentry_ctrl = _vmentry_control;
+ vmcs_conf->vmexit_ctrl = _vmexit_control;
+ vmcs_conf->secondary_vmexit_ctrl = _secondary_vmexit_control;
vmcs_conf->misc = misc_msr;
#if IS_ENABLED(CONFIG_HYPERV)
@@ -4449,6 +4474,11 @@ static u32 vmx_vmexit_ctrl(void)
~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER);
}
+static u64 vmx_secondary_vmexit_ctrl(void)
+{
+ return vmcs_config.secondary_vmexit_ctrl;
+}
+
void vmx_refresh_apicv_exec_ctrl(struct kvm_vcpu *vcpu)
{
struct vcpu_vmx *vmx = to_vmx(vcpu);
@@ -4799,6 +4829,9 @@ static void init_vmcs(struct vcpu_vmx *vmx)
vm_exit_controls_set(vmx, vmx_vmexit_ctrl());
+ if (cpu_has_secondary_vmexit_ctrls())
+ secondary_vm_exit_controls_set(vmx, vmx_secondary_vmexit_ctrl());
+
/* 22.2.1, 20.8.1 */
vm_entry_controls_set(vmx, vmx_vmentry_ctrl());
diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
index 2325f773a20b..cf3a6c116634 100644
--- a/arch/x86/kvm/vmx/vmx.h
+++ b/arch/x86/kvm/vmx/vmx.h
@@ -507,7 +507,11 @@ static inline u8 vmx_get_rvi(void)
VM_EXIT_LOAD_IA32_EFER | \
VM_EXIT_CLEAR_BNDCFGS | \
VM_EXIT_PT_CONCEAL_PIP | \
- VM_EXIT_CLEAR_IA32_RTIT_CTL)
+ VM_EXIT_CLEAR_IA32_RTIT_CTL | \
+ VM_EXIT_ACTIVATE_SECONDARY_CONTROLS)
+
+#define KVM_REQUIRED_VMX_SECONDARY_VM_EXIT_CONTROLS (0)
+#define KVM_OPTIONAL_VMX_SECONDARY_VM_EXIT_CONTROLS (0)
#define KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL \
(PIN_BASED_EXT_INTR_MASK | \
@@ -612,6 +616,7 @@ static __always_inline void lname##_controls_clearbit(struct vcpu_vmx *vmx, u##b
}
BUILD_CONTROLS_SHADOW(vm_entry, VM_ENTRY_CONTROLS, 32)
BUILD_CONTROLS_SHADOW(vm_exit, VM_EXIT_CONTROLS, 32)
+BUILD_CONTROLS_SHADOW(secondary_vm_exit, SECONDARY_VM_EXIT_CONTROLS, 64)
BUILD_CONTROLS_SHADOW(pin, PIN_BASED_VM_EXEC_CONTROL, 32)
BUILD_CONTROLS_SHADOW(exec, CPU_BASED_VM_EXEC_CONTROL, 32)
BUILD_CONTROLS_SHADOW(secondary_exec, SECONDARY_VM_EXEC_CONTROL, 32)
--
2.46.2>@@ -2713,21 +2715,43 @@ 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))
>+ if (_vmexit_control & VM_EXIT_ACTIVATE_SECONDARY_CONTROLS)
>+ _secondary_vmexit_control =
>+ adjust_vmx_controls64(KVM_OPTIONAL_VMX_SECONDARY_VM_EXIT_CONTROLS,
>+ MSR_IA32_VMX_EXIT_CTLS2);
>+
>+ for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_triplets); i++) {
>+ u32 n_ctrl = vmcs_entry_exit_triplets[i].entry_control;
>+ u32 x_ctrl = vmcs_entry_exit_triplets[i].exit_control;
>+ u64 x_ctrl_2 = vmcs_entry_exit_triplets[i].exit_2nd_control;
>+ bool has_n = n_ctrl && ((_vmentry_control & n_ctrl) == n_ctrl);
>+ bool has_x = x_ctrl && ((_vmexit_control & x_ctrl) == x_ctrl);
>+ bool has_x_2 = x_ctrl_2 && ((_secondary_vmexit_control & x_ctrl_2) == x_ctrl_2);
>+
>+ if (x_ctrl_2) {
>+ /* Only activate secondary VM exit control bit should be set */
>+ if ((_vmexit_control & x_ctrl) == VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
>+ if (has_n == has_x_2)
>+ continue;
>+ } else {
>+ /* The feature should not be supported in any control */
>+ if (!has_n && !has_x && !has_x_2)
>+ continue;
>+ }
>+ } else if (has_n == has_x) {
> continue;
>+ }
>
>- pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
>- _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
>+ pr_warn_once("Inconsistent VM-Entry/VM-Exit triplet, entry = %x, exit = %x, secondary_exit = %llx\n",
>+ _vmentry_control & n_ctrl, _vmexit_control & x_ctrl,
>+ _secondary_vmexit_control & x_ctrl_2);
>
> if (error_on_inconsistent_vmcs_config)
> return -EIO;
>
> _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.
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++) {
...
}
>+ _secondary_vmexit_control &= ~x_ctrl_2;
> }
>
> rdmsrl(MSR_IA32_VMX_BASIC, basic_msr);
On 10/21/2024 1:28 AM, Chao Gao wrote:
>> + for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_triplets); i++) {
>> + u32 n_ctrl = vmcs_entry_exit_triplets[i].entry_control;
>> + u32 x_ctrl = vmcs_entry_exit_triplets[i].exit_control;
>> + u64 x_ctrl_2 = vmcs_entry_exit_triplets[i].exit_2nd_control;
>> + bool has_n = n_ctrl && ((_vmentry_control & n_ctrl) == n_ctrl);
>> + bool has_x = x_ctrl && ((_vmexit_control & x_ctrl) == x_ctrl);
>> + bool has_x_2 = x_ctrl_2 && ((_secondary_vmexit_control & x_ctrl_2) == x_ctrl_2);
>> +
>> + if (x_ctrl_2) {
>> + /* Only activate secondary VM exit control bit should be set */
>> + if ((_vmexit_control & x_ctrl) == VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
>> + if (has_n == has_x_2)
>> + continue;
>> + } else {
>> + /* The feature should not be supported in any control */
>> + if (!has_n && !has_x && !has_x_2)
>> + continue;
>> + }
>> + } else if (has_n == has_x) {
>> continue;
>> + }
>>
>> - pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
>> - _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
>> + pr_warn_once("Inconsistent VM-Entry/VM-Exit triplet, entry = %x, exit = %x, secondary_exit = %llx\n",
>> + _vmentry_control & n_ctrl, _vmexit_control & x_ctrl,
>> + _secondary_vmexit_control & x_ctrl_2);
>>
>> if (error_on_inconsistent_vmcs_config)
>> return -EIO;
>>
>> _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.
>
>> + _secondary_vmexit_control &= ~x_ctrl_2;
>> }
>>
>> rdmsrl(MSR_IA32_VMX_BASIC, basic_msr);
On Mon, Oct 21, 2024 at 10:03:45AM -0700, Xin Li wrote:
>On 10/21/2024 1:28 AM, Chao Gao wrote:
>> > + for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_triplets); i++) {
>> > + u32 n_ctrl = vmcs_entry_exit_triplets[i].entry_control;
>> > + u32 x_ctrl = vmcs_entry_exit_triplets[i].exit_control;
>> > + u64 x_ctrl_2 = vmcs_entry_exit_triplets[i].exit_2nd_control;
>> > + bool has_n = n_ctrl && ((_vmentry_control & n_ctrl) == n_ctrl);
>> > + bool has_x = x_ctrl && ((_vmexit_control & x_ctrl) == x_ctrl);
>> > + bool has_x_2 = x_ctrl_2 && ((_secondary_vmexit_control & x_ctrl_2) == x_ctrl_2);
>> > +
>> > + if (x_ctrl_2) {
>> > + /* Only activate secondary VM exit control bit should be set */
>> > + if ((_vmexit_control & x_ctrl) == VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
>> > + if (has_n == has_x_2)
>> > + continue;
>> > + } else {
>> > + /* The feature should not be supported in any control */
>> > + if (!has_n && !has_x && !has_x_2)
>> > + continue;
>> > + }
>> > + } else if (has_n == has_x) {
>> > continue;
>> > + }
>> >
>> > - pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
>> > - _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
>> > + pr_warn_once("Inconsistent VM-Entry/VM-Exit triplet, entry = %x, exit = %x, secondary_exit = %llx\n",
>> > + _vmentry_control & n_ctrl, _vmexit_control & x_ctrl,
>> > + _secondary_vmexit_control & x_ctrl_2);
>> >
>> > if (error_on_inconsistent_vmcs_config)
>> > return -EIO;
>> >
>> > _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.
One table is fine if we can fix the issue and improve readability. The three
nested if() statements hurts readability.
I just thought using two tables would eliminate the need for any if() statements.
>>>> _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.
>
> 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 just thought using two tables would eliminate the need for any if() statements.
>
One more thing, IIUC, Sean prefers to keep
VM_EXIT_ACTIVATE_SECONDARY_CONTROLS set if it's allowed to be set and
even bits in the 2nd VM exit controls are all 0. I may be able to make
it simpler.
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
© 2016 - 2026 Red Hat, Inc.