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.
© 2016 - 2024 Red Hat, Inc.