* [PATCH 1/4] KVM: nVMX: Sanitize primary processor-based VM-execution controls with eVMCS too
2022-10-18 10:09 [PATCH 0/4] KVM: VMX: nVMX: Make eVMCS enablement more robust Vitaly Kuznetsov
@ 2022-10-18 10:09 ` Vitaly Kuznetsov
2022-10-18 10:09 ` [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check Vitaly Kuznetsov
` (2 subsequent siblings)
3 siblings, 0 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-18 10:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
The only unsupported primary processor-based VM-execution control at the
moment is CPU_BASED_ACTIVATE_TERTIARY_CONTROLS and KVM doesn't expose it
in nested VMX feature MSRs anyway (see nested_vmx_setup_ctls_msrs())
but in preparation to inverting "unsupported with eVMCS" checks (and
for completeness) it's better to sanitize MSR_IA32_VMX_PROCBASED_CTLS/
MSR_IA32_VMX_TRUE_PROCBASED_CTLS too.
No functional change intended.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/vmx/evmcs.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index d8b23c96d627..337783675731 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -368,6 +368,7 @@ enum evmcs_revision {
enum evmcs_ctrl_type {
EVMCS_EXIT_CTRLS,
EVMCS_ENTRY_CTRLS,
+ EVMCS_EXEC_CTRL,
EVMCS_2NDEXEC,
EVMCS_PINCTRL,
EVMCS_VMFUNC,
@@ -381,6 +382,9 @@ static const u32 evmcs_unsupported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
[EVMCS_ENTRY_CTRLS] = {
[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMENTRY_CTRL,
},
+ [EVMCS_EXEC_CTRL] = {
+ [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_EXEC_CTRL,
+ },
[EVMCS_2NDEXEC] = {
[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_2NDEXEC,
},
@@ -441,6 +445,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
unsupported_ctrls |= VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
ctl_high &= ~unsupported_ctrls;
break;
+ case MSR_IA32_VMX_PROCBASED_CTLS:
+ case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
+ ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_EXEC_CTRL);
+ break;
case MSR_IA32_VMX_PROCBASED_CTLS2:
ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_2NDEXEC);
break;
@@ -468,6 +476,10 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
vmcs12->pin_based_vm_exec_control)))
return -EINVAL;
+ if (CC(!nested_evmcs_is_valid_controls(EVMCS_EXEC_CTRL,
+ vmcs12->cpu_based_vm_exec_control)))
+ return -EINVAL;
+
if (CC(!nested_evmcs_is_valid_controls(EVMCS_2NDEXEC,
vmcs12->secondary_vm_exec_control)))
return -EINVAL;
--
2.37.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
2022-10-18 10:09 [PATCH 0/4] KVM: VMX: nVMX: Make eVMCS enablement more robust Vitaly Kuznetsov
2022-10-18 10:09 ` [PATCH 1/4] KVM: nVMX: Sanitize primary processor-based VM-execution controls with eVMCS too Vitaly Kuznetsov
@ 2022-10-18 10:09 ` Vitaly Kuznetsov
2022-10-26 23:18 ` Sean Christopherson
2022-10-18 10:09 ` [PATCH 3/4] KVM: nVMX: Prepare to sanitize tertiary execution controls with eVMCS Vitaly Kuznetsov
2022-10-18 10:10 ` [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V Vitaly Kuznetsov
3 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-18 10:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
need to be adjusted to avoid the situation when the feature is exposed
to the guest but there's no corresponding eVMCS field[s] for it. This
is not obvious and fragile. Invert 'unsupported by eVMCSv1' check and
make it 'supported by eVMCSv1' instead, this way it's much harder to
make a mistake. New features will get added to EVMCS1_SUPPORTED_*
defines when the corresponding fields are added to eVMCS definition.
No functional change intended. EVMCS1_SUPPORTED_* defines are composed
by taking KVM_{REQUIRED,OPTIONAL}_VMX_ defines and filtering out what
was previously known as EVMCS1_UNSUPPORTED_*.
From all the controls, SECONDARY_EXEC_TSC_SCALING requires special
handling as it's actually present in eVMCSv1 definition but is not
currently supported for Hyper-V-on-KVM, just for KVM-on-Hyper-V. As
evmcs_supported_ctrls will be used for both scenarios, just add it
there instead of EVMCS1_SUPPORTED_2NDEXEC.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/vmx/evmcs.c | 42 +++++++++----------
arch/x86/kvm/vmx/evmcs.h | 90 +++++++++++++++++++++++++++++++++-------
2 files changed, 96 insertions(+), 36 deletions(-)
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 337783675731..0f031d27741a 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -375,32 +375,32 @@ enum evmcs_ctrl_type {
NR_EVMCS_CTRLS,
};
-static const u32 evmcs_unsupported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
+static const u32 evmcs_supported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
[EVMCS_EXIT_CTRLS] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMEXIT_CTRL,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMEXIT_CTRL,
},
[EVMCS_ENTRY_CTRLS] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMENTRY_CTRL,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMENTRY_CTRL,
},
[EVMCS_EXEC_CTRL] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_EXEC_CTRL,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_EXEC_CTRL,
},
[EVMCS_2NDEXEC] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_2NDEXEC,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_2NDEXEC & ~SECONDARY_EXEC_TSC_SCALING,
},
[EVMCS_PINCTRL] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_PINCTRL,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_PINCTRL,
},
[EVMCS_VMFUNC] = {
- [EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMFUNC,
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMFUNC,
},
};
-static u32 evmcs_get_unsupported_ctls(enum evmcs_ctrl_type ctrl_type)
+static u32 evmcs_get_supported_ctls(enum evmcs_ctrl_type ctrl_type)
{
enum evmcs_revision evmcs_rev = EVMCSv1_LEGACY;
- return evmcs_unsupported_ctrls[ctrl_type][evmcs_rev];
+ return evmcs_supported_ctrls[ctrl_type][evmcs_rev];
}
static bool evmcs_has_perf_global_ctrl(struct kvm_vcpu *vcpu)
@@ -424,7 +424,7 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
{
u32 ctl_low = (u32)*pdata;
u32 ctl_high = (u32)(*pdata >> 32);
- u32 unsupported_ctrls;
+ u32 supported_ctrls;
/*
* Hyper-V 2016 and 2019 try using these features even when eVMCS
@@ -433,31 +433,31 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
switch (msr_index) {
case MSR_IA32_VMX_EXIT_CTLS:
case MSR_IA32_VMX_TRUE_EXIT_CTLS:
- unsupported_ctrls = evmcs_get_unsupported_ctls(EVMCS_EXIT_CTRLS);
+ supported_ctrls = evmcs_get_supported_ctls(EVMCS_EXIT_CTRLS);
if (!evmcs_has_perf_global_ctrl(vcpu))
- unsupported_ctrls |= VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
- ctl_high &= ~unsupported_ctrls;
+ supported_ctrls &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
+ ctl_high &= supported_ctrls;
break;
case MSR_IA32_VMX_ENTRY_CTLS:
case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
- unsupported_ctrls = evmcs_get_unsupported_ctls(EVMCS_ENTRY_CTRLS);
+ supported_ctrls = evmcs_get_supported_ctls(EVMCS_ENTRY_CTRLS);
if (!evmcs_has_perf_global_ctrl(vcpu))
- unsupported_ctrls |= VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
- ctl_high &= ~unsupported_ctrls;
+ supported_ctrls &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
+ ctl_high &= supported_ctrls;
break;
case MSR_IA32_VMX_PROCBASED_CTLS:
case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
- ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_EXEC_CTRL);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_EXEC_CTRL);
break;
case MSR_IA32_VMX_PROCBASED_CTLS2:
- ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_2NDEXEC);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_2NDEXEC);
break;
case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
case MSR_IA32_VMX_PINBASED_CTLS:
- ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_PINCTRL);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_PINCTRL);
break;
case MSR_IA32_VMX_VMFUNC:
- ctl_low &= ~evmcs_get_unsupported_ctls(EVMCS_VMFUNC);
+ ctl_low &= evmcs_get_supported_ctls(EVMCS_VMFUNC);
break;
}
@@ -467,7 +467,7 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
static bool nested_evmcs_is_valid_controls(enum evmcs_ctrl_type ctrl_type,
u32 val)
{
- return !(val & evmcs_get_unsupported_ctls(ctrl_type));
+ return !(val & ~evmcs_get_supported_ctls(ctrl_type));
}
int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 6f746ef3c038..4c351f334446 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -46,22 +46,82 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
* Currently unsupported in KVM:
* GUEST_IA32_RTIT_CTL = 0x00002814,
*/
-#define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
- PIN_BASED_VMX_PREEMPTION_TIMER)
-#define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
-#define EVMCS1_UNSUPPORTED_2NDEXEC \
- (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | \
- SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES | \
- SECONDARY_EXEC_APIC_REGISTER_VIRT | \
- SECONDARY_EXEC_ENABLE_PML | \
- SECONDARY_EXEC_ENABLE_VMFUNC | \
- SECONDARY_EXEC_SHADOW_VMCS | \
+#define EVMCS1_SUPPORTED_PINCTRL \
+ (PIN_BASED_ALWAYSON_WITHOUT_TRUE_MSR | \
+ PIN_BASED_EXT_INTR_MASK | \
+ PIN_BASED_NMI_EXITING | \
+ PIN_BASED_VIRTUAL_NMIS)
+
+#define EVMCS1_SUPPORTED_EXEC_CTRL \
+ (CPU_BASED_ALWAYSON_WITHOUT_TRUE_MSR | \
+ CPU_BASED_HLT_EXITING | \
+ CPU_BASED_CR3_LOAD_EXITING | \
+ CPU_BASED_CR3_STORE_EXITING | \
+ CPU_BASED_UNCOND_IO_EXITING | \
+ CPU_BASED_MOV_DR_EXITING | \
+ CPU_BASED_USE_TSC_OFFSETTING | \
+ CPU_BASED_MWAIT_EXITING | \
+ CPU_BASED_MONITOR_EXITING | \
+ CPU_BASED_INVLPG_EXITING | \
+ CPU_BASED_RDPMC_EXITING | \
+ CPU_BASED_INTR_WINDOW_EXITING | \
+ CPU_BASED_CR8_LOAD_EXITING | \
+ CPU_BASED_CR8_STORE_EXITING | \
+ CPU_BASED_RDTSC_EXITING | \
+ CPU_BASED_TPR_SHADOW | \
+ CPU_BASED_USE_IO_BITMAPS | \
+ CPU_BASED_MONITOR_TRAP_FLAG | \
+ CPU_BASED_USE_MSR_BITMAPS | \
+ CPU_BASED_NMI_WINDOW_EXITING | \
+ CPU_BASED_PAUSE_EXITING | \
+ CPU_BASED_ACTIVATE_SECONDARY_CONTROLS)
+
+#define EVMCS1_SUPPORTED_2NDEXEC \
+ (SECONDARY_EXEC_VIRTUALIZE_X2APIC_MODE | \
+ SECONDARY_EXEC_WBINVD_EXITING | \
+ SECONDARY_EXEC_ENABLE_VPID | \
+ SECONDARY_EXEC_ENABLE_EPT | \
+ SECONDARY_EXEC_UNRESTRICTED_GUEST | \
+ SECONDARY_EXEC_DESC | \
+ SECONDARY_EXEC_ENABLE_RDTSCP | \
+ SECONDARY_EXEC_ENABLE_INVPCID | \
+ SECONDARY_EXEC_XSAVES | \
+ SECONDARY_EXEC_RDSEED_EXITING | \
+ SECONDARY_EXEC_RDRAND_EXITING | \
SECONDARY_EXEC_TSC_SCALING | \
- SECONDARY_EXEC_PAUSE_LOOP_EXITING)
-#define EVMCS1_UNSUPPORTED_VMEXIT_CTRL \
- (VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
-#define EVMCS1_UNSUPPORTED_VMENTRY_CTRL (0)
-#define EVMCS1_UNSUPPORTED_VMFUNC (VMX_VMFUNC_EPTP_SWITCHING)
+ SECONDARY_EXEC_ENABLE_USR_WAIT_PAUSE | \
+ SECONDARY_EXEC_PT_USE_GPA | \
+ SECONDARY_EXEC_PT_CONCEAL_VMX | \
+ SECONDARY_EXEC_BUS_LOCK_DETECTION | \
+ SECONDARY_EXEC_NOTIFY_VM_EXITING | \
+ SECONDARY_EXEC_ENCLS_EXITING)
+
+#define EVMCS1_SUPPORTED_VMEXIT_CTRL \
+ (VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR | \
+ VM_EXIT_SAVE_DEBUG_CONTROLS | \
+ VM_EXIT_ACK_INTR_ON_EXIT | \
+ VM_EXIT_HOST_ADDR_SPACE_SIZE | \
+ VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | \
+ VM_EXIT_SAVE_IA32_PAT | \
+ VM_EXIT_LOAD_IA32_PAT | \
+ VM_EXIT_SAVE_IA32_EFER | \
+ VM_EXIT_LOAD_IA32_EFER | \
+ VM_EXIT_CLEAR_BNDCFGS | \
+ VM_EXIT_PT_CONCEAL_PIP | \
+ VM_EXIT_CLEAR_IA32_RTIT_CTL)
+
+#define EVMCS1_SUPPORTED_VMENTRY_CTRL \
+ (VM_ENTRY_ALWAYSON_WITHOUT_TRUE_MSR | \
+ VM_ENTRY_LOAD_DEBUG_CONTROLS | \
+ VM_ENTRY_IA32E_MODE | \
+ VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL | \
+ VM_ENTRY_LOAD_IA32_PAT | \
+ VM_ENTRY_LOAD_IA32_EFER | \
+ VM_ENTRY_LOAD_BNDCFGS | \
+ VM_ENTRY_PT_CONCEAL_PIP | \
+ VM_ENTRY_LOAD_IA32_RTIT_CTL)
+
+#define EVMCS1_SUPPORTED_VMFUNC (0)
struct evmcs_field {
u16 offset;
--
2.37.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
2022-10-18 10:09 ` [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check Vitaly Kuznetsov
@ 2022-10-26 23:18 ` Sean Christopherson
2022-10-27 11:14 ` Vitaly Kuznetsov
0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-10-26 23:18 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
> need to be adjusted to avoid the situation when the feature is exposed
> to the guest but there's no corresponding eVMCS field[s] for it. This
> is not obvious and fragile.
Eh, either way is fragile, the only difference is what goes wrong when it breaks.
At the risk of making this overly verbose, what about requiring developers to
explicitly define whether or not a new control is support? E.g. keep the
EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.
That way the eVMCS "supported" controls don't need to include the ALWAYSON
controls, and anytime someone adds a new control, they'll have to stop and think
about eVMCS.
I think we'll still want (need?) the runtime sanitization, but this might allow
catching at least some cases without needing to wait until a control actually gets
exposed.
E.g. possibly with more macro magic to reduce the boilerplate
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index d8b23c96d627..190932edcc02 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -422,6 +422,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
u32 ctl_high = (u32)(*pdata >> 32);
u32 unsupported_ctrls;
+ BUILD_BUG_ON((EVMCS1_SUPPORTED_PINCTRL | EVMCS1_UNSUPPORTED_PINCTRL) !=
+ (KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL |
+ KVM_OPTIONAL_VMX_PIN_BASED_VM_EXEC_CONTROL));
+
/*
* Hyper-V 2016 and 2019 try using these features even when eVMCS
* is enabled but there are no corresponding fields.
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 6f746ef3c038..58d77afe9d57 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -48,6 +48,11 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
*/
#define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
PIN_BASED_VMX_PREEMPTION_TIMER)
+#define EVMCS1_SUPPORTED_PINCTRL \
+ (PIN_BASED_EXT_INTR_MASK | \
+ PIN_BASED_NMI_EXITING | \
+ PIN_BASED_VIRTUAL_NMIS)
+
#define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
#define EVMCS1_UNSUPPORTED_2NDEXEC \
(SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | \
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
2022-10-26 23:18 ` Sean Christopherson
@ 2022-10-27 11:14 ` Vitaly Kuznetsov
2022-10-27 21:35 ` Sean Christopherson
0 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-27 11:14 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
Sean Christopherson <seanjc@google.com> writes:
> On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
>> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
>> need to be adjusted to avoid the situation when the feature is exposed
>> to the guest but there's no corresponding eVMCS field[s] for it. This
>> is not obvious and fragile.
>
> Eh, either way is fragile, the only difference is what goes wrong when it breaks.
>
> At the risk of making this overly verbose, what about requiring developers to
> explicitly define whether or not a new control is support? E.g. keep the
> EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
> feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.
>
> That way the eVMCS "supported" controls don't need to include the ALWAYSON
> controls, and anytime someone adds a new control, they'll have to stop and think
> about eVMCS.
Is this a good thing or a bad one? :-) I'm not against being extra
verbose but adding a new feature to EVMCS1_SUPPORTED_* (even when there
is a corresponding field) requires testing or a
evmcs_has_perf_global_ctrl()-like story may happen and such testing
would require access to Windows/Hyper-V images. This sounds like an
extra burden for contributors. IMO it's OK if new features are
mechanically added to EVMCS1_UNSUPPORTED_* on the grounds that it
wasn't tested but then it's not much different from "unsupported by
default" (my approach). So I'm on the fence here.
>
> I think we'll still want (need?) the runtime sanitization, but this might allow
> catching at least some cases without needing to wait until a control actually gets
> exposed.
>
> E.g. possibly with more macro magic to reduce the boilerplate
>
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index d8b23c96d627..190932edcc02 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -422,6 +422,10 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
> u32 ctl_high = (u32)(*pdata >> 32);
> u32 unsupported_ctrls;
>
> + BUILD_BUG_ON((EVMCS1_SUPPORTED_PINCTRL | EVMCS1_UNSUPPORTED_PINCTRL) !=
> + (KVM_REQUIRED_VMX_PIN_BASED_VM_EXEC_CONTROL |
> + KVM_OPTIONAL_VMX_PIN_BASED_VM_EXEC_CONTROL));
> +
> /*
> * Hyper-V 2016 and 2019 try using these features even when eVMCS
> * is enabled but there are no corresponding fields.
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index 6f746ef3c038..58d77afe9d57 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -48,6 +48,11 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
> */
> #define EVMCS1_UNSUPPORTED_PINCTRL (PIN_BASED_POSTED_INTR | \
> PIN_BASED_VMX_PREEMPTION_TIMER)
> +#define EVMCS1_SUPPORTED_PINCTRL \
> + (PIN_BASED_EXT_INTR_MASK | \
> + PIN_BASED_NMI_EXITING | \
> + PIN_BASED_VIRTUAL_NMIS)
> +
> #define EVMCS1_UNSUPPORTED_EXEC_CTRL (CPU_BASED_ACTIVATE_TERTIARY_CONTROLS)
> #define EVMCS1_UNSUPPORTED_2NDEXEC \
> (SECONDARY_EXEC_VIRTUAL_INTR_DELIVERY | \
>
--
Vitaly
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check
2022-10-27 11:14 ` Vitaly Kuznetsov
@ 2022-10-27 21:35 ` Sean Christopherson
0 siblings, 0 replies; 10+ messages in thread
From: Sean Christopherson @ 2022-10-27 21:35 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
On Thu, Oct 27, 2022, Vitaly Kuznetsov wrote:
> Sean Christopherson <seanjc@google.com> writes:
>
> > On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
> >> When a new feature gets implemented in KVM, EVMCS1_UNSUPPORTED_* defines
> >> need to be adjusted to avoid the situation when the feature is exposed
> >> to the guest but there's no corresponding eVMCS field[s] for it. This
> >> is not obvious and fragile.
> >
> > Eh, either way is fragile, the only difference is what goes wrong when it breaks.
> >
> > At the risk of making this overly verbose, what about requiring developers to
> > explicitly define whether or not a new control is support? E.g. keep the
> > EVMCS1_UNSUPPORTED_* and then add compile-time assertions to verify that every
> > feature that is REQUIRED | OPTIONAL is SUPPORTED | UNSUPPORTED.
> >
> > That way the eVMCS "supported" controls don't need to include the ALWAYSON
> > controls, and anytime someone adds a new control, they'll have to stop and think
> > about eVMCS.
>
> Is this a good thing or a bad one? :-) I'm not against being extra
> verbose but adding a new feature to EVMCS1_SUPPORTED_* (even when there
> is a corresponding field) requires testing or a
> evmcs_has_perf_global_ctrl()-like story may happen and such testing
> would require access to Windows/Hyper-V images. This sounds like an
> extra burden for contributors. IMO it's OK if new features are
> mechanically added to EVMCS1_UNSUPPORTED_* on the grounds that it
> wasn't tested but then it's not much different from "unsupported by
> default" (my approach). So I'm on the fence here.
Yeah, I was hoping the compile-time asserts would buy us full protection, i.e. I
was hoping to avoid the sanitization, but I don't see a way to handle the case
where Hyper-V starts advertising a feature that was previously unsupported :-(
I'm a-ok going with SUPPORTED only, I'm on the fence too.
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 3/4] KVM: nVMX: Prepare to sanitize tertiary execution controls with eVMCS
2022-10-18 10:09 [PATCH 0/4] KVM: VMX: nVMX: Make eVMCS enablement more robust Vitaly Kuznetsov
2022-10-18 10:09 ` [PATCH 1/4] KVM: nVMX: Sanitize primary processor-based VM-execution controls with eVMCS too Vitaly Kuznetsov
2022-10-18 10:09 ` [PATCH 2/4] KVM: nVMX: Invert 'unsupported by eVMCSv1' check Vitaly Kuznetsov
@ 2022-10-18 10:09 ` Vitaly Kuznetsov
2022-10-18 10:10 ` [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V Vitaly Kuznetsov
3 siblings, 0 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-18 10:09 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
In preparation to restoring vmcs_conf sanitization for KVM-on-Hyper-V,
(and for completeness) add tertiary VM-execution controls to
'evmcs_supported_ctrls'.
No functional change intended as KVM doesn't yet expose
MSR_IA32_VMX_PROCBASED_CTLS3 to its guests.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/vmx/evmcs.c | 4 ++++
arch/x86/kvm/vmx/evmcs.h | 2 ++
2 files changed, 6 insertions(+)
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 0f031d27741a..47f6d1cbd428 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -370,6 +370,7 @@ enum evmcs_ctrl_type {
EVMCS_ENTRY_CTRLS,
EVMCS_EXEC_CTRL,
EVMCS_2NDEXEC,
+ EVMCS_3RDEXEC,
EVMCS_PINCTRL,
EVMCS_VMFUNC,
NR_EVMCS_CTRLS,
@@ -388,6 +389,9 @@ static const u32 evmcs_supported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
[EVMCS_2NDEXEC] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_2NDEXEC & ~SECONDARY_EXEC_TSC_SCALING,
},
+ [EVMCS_3RDEXEC] = {
+ [EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_3RDEXEC,
+ },
[EVMCS_PINCTRL] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_PINCTRL,
},
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 4c351f334446..205b5b467617 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -96,6 +96,8 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
SECONDARY_EXEC_NOTIFY_VM_EXITING | \
SECONDARY_EXEC_ENCLS_EXITING)
+#define EVMCS1_SUPPORTED_3RDEXEC (0ULL)
+
#define EVMCS1_SUPPORTED_VMEXIT_CTRL \
(VM_EXIT_ALWAYSON_WITHOUT_TRUE_MSR | \
VM_EXIT_SAVE_DEBUG_CONTROLS | \
--
2.37.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V
2022-10-18 10:09 [PATCH 0/4] KVM: VMX: nVMX: Make eVMCS enablement more robust Vitaly Kuznetsov
` (2 preceding siblings ...)
2022-10-18 10:09 ` [PATCH 3/4] KVM: nVMX: Prepare to sanitize tertiary execution controls with eVMCS Vitaly Kuznetsov
@ 2022-10-18 10:10 ` Vitaly Kuznetsov
2022-10-26 23:34 ` Sean Christopherson
3 siblings, 1 reply; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-18 10:10 UTC (permalink / raw)
To: kvm, Paolo Bonzini
Cc: Sean Christopherson, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
Commit 9bcb90650e31 ("KVM: VMX: Get rid of eVMCS specific VMX controls
sanitization") dropped 'vmcs_conf' sanitization for KVM-on-Hyper-V because
there's no known Hyper-V version which would expose a feature
unsupported in eVMCS in VMX feature MSRs. This works well for all
currently existing Hyper-V version, however, future Hyper-V versions
may add features which are supported by KVM and are currently missing
in eVMCSv1 definition (e.g. APIC virtualization, PML,...). When this
happens, existing KVMs will get broken. With the inverted 'unsupported
by eVMCSv1' checks, we can resurrect vmcs_conf sanitization and make
KVM future proof.
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
arch/x86/kvm/vmx/evmcs.c | 80 +++++++++++++++++++++++++++++++++++-----
arch/x86/kvm/vmx/evmcs.h | 1 +
arch/x86/kvm/vmx/vmx.c | 5 +++
3 files changed, 76 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 47f6d1cbd428..89d7b9537ada 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) "kvm/hyper-v: " fmt
+
#include <linux/errno.h>
#include <linux/smp.h>
@@ -362,6 +364,7 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
enum evmcs_revision {
EVMCSv1_LEGACY,
+ EVMCSv1_STRICT,
NR_EVMCS_REVISIONS,
};
@@ -379,31 +382,36 @@ enum evmcs_ctrl_type {
static const u32 evmcs_supported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
[EVMCS_EXIT_CTRLS] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMEXIT_CTRL,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_VMEXIT_CTRL,
},
[EVMCS_ENTRY_CTRLS] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMENTRY_CTRL,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_VMENTRY_CTRL,
},
[EVMCS_EXEC_CTRL] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_EXEC_CTRL,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_EXEC_CTRL,
},
[EVMCS_2NDEXEC] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_2NDEXEC & ~SECONDARY_EXEC_TSC_SCALING,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_2NDEXEC,
},
[EVMCS_3RDEXEC] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_3RDEXEC,
},
[EVMCS_PINCTRL] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_PINCTRL,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_PINCTRL,
},
[EVMCS_VMFUNC] = {
[EVMCSv1_LEGACY] = EVMCS1_SUPPORTED_VMFUNC,
+ [EVMCSv1_STRICT] = EVMCS1_SUPPORTED_VMFUNC,
},
};
-static u32 evmcs_get_supported_ctls(enum evmcs_ctrl_type ctrl_type)
+static u32 evmcs_get_supported_ctls(enum evmcs_ctrl_type ctrl_type,
+ enum evmcs_revision evmcs_rev)
{
- enum evmcs_revision evmcs_rev = EVMCSv1_LEGACY;
-
return evmcs_supported_ctrls[ctrl_type][evmcs_rev];
}
@@ -437,31 +445,37 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
switch (msr_index) {
case MSR_IA32_VMX_EXIT_CTLS:
case MSR_IA32_VMX_TRUE_EXIT_CTLS:
- supported_ctrls = evmcs_get_supported_ctls(EVMCS_EXIT_CTRLS);
+ supported_ctrls = evmcs_get_supported_ctls(EVMCS_EXIT_CTRLS,
+ EVMCSv1_LEGACY);
if (!evmcs_has_perf_global_ctrl(vcpu))
supported_ctrls &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
ctl_high &= supported_ctrls;
break;
case MSR_IA32_VMX_ENTRY_CTLS:
case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
- supported_ctrls = evmcs_get_supported_ctls(EVMCS_ENTRY_CTRLS);
+ supported_ctrls = evmcs_get_supported_ctls(EVMCS_ENTRY_CTRLS,
+ EVMCSv1_LEGACY);
if (!evmcs_has_perf_global_ctrl(vcpu))
supported_ctrls &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
ctl_high &= supported_ctrls;
break;
case MSR_IA32_VMX_PROCBASED_CTLS:
case MSR_IA32_VMX_TRUE_PROCBASED_CTLS:
- ctl_high &= evmcs_get_supported_ctls(EVMCS_EXEC_CTRL);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_EXEC_CTRL,
+ EVMCSv1_LEGACY);
break;
case MSR_IA32_VMX_PROCBASED_CTLS2:
- ctl_high &= evmcs_get_supported_ctls(EVMCS_2NDEXEC);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_2NDEXEC,
+ EVMCSv1_LEGACY);
break;
case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
case MSR_IA32_VMX_PINBASED_CTLS:
- ctl_high &= evmcs_get_supported_ctls(EVMCS_PINCTRL);
+ ctl_high &= evmcs_get_supported_ctls(EVMCS_PINCTRL,
+ EVMCSv1_LEGACY);
break;
case MSR_IA32_VMX_VMFUNC:
- ctl_low &= evmcs_get_supported_ctls(EVMCS_VMFUNC);
+ ctl_low &= evmcs_get_supported_ctls(EVMCS_VMFUNC,
+ EVMCSv1_LEGACY);
break;
}
@@ -471,7 +485,7 @@ void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *
static bool nested_evmcs_is_valid_controls(enum evmcs_ctrl_type ctrl_type,
u32 val)
{
- return !(val & ~evmcs_get_supported_ctls(ctrl_type));
+ return !(val & ~evmcs_get_supported_ctls(ctrl_type, EVMCSv1_LEGACY));
}
int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
@@ -511,6 +525,52 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
return 0;
}
+#if IS_ENABLED(CONFIG_HYPERV)
+/*
+ * KVM on Hyper-V always uses the newest known eVMCSv1 revision, the assumption
+ * is: in case a feature has corresponding fields in eVMCS described and it was
+ * exposed in VMX feature MSRs, KVM is free to use it. Warn if KVM meets a
+ * feature which has no corresponding eVMCS field, this likely means that KVM
+ * needs to be updated.
+ */
+#define evmcs_check_vmcs_conf32(field, ctrl) \
+ { \
+ u32 supported, unsupported32; \
+ \
+ supported = evmcs_get_supported_ctls(ctrl, EVMCSv1_STRICT); \
+ unsupported32 = vmcs_conf->field & ~supported; \
+ if (unsupported32) { \
+ pr_warn_once(#field " unsupported with eVMCS: 0x%x\n", \
+ unsupported32); \
+ vmcs_conf->field &= supported; \
+ } \
+ }
+
+#define evmcs_check_vmcs_conf64(field, ctrl) \
+ { \
+ u32 supported; \
+ u64 unsupported64; \
+ \
+ supported = evmcs_get_supported_ctls(ctrl, EVMCSv1_STRICT); \
+ unsupported64 = vmcs_conf->field & ~supported; \
+ if (unsupported64) { \
+ pr_warn_once(#field " unsupported with eVMCS: 0x%llx\n",\
+ unsupported64); \
+ vmcs_conf->field &= supported; \
+ } \
+ }
+
+__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
+{
+ evmcs_check_vmcs_conf32(cpu_based_exec_ctrl, EVMCS_EXEC_CTRL);
+ evmcs_check_vmcs_conf32(pin_based_exec_ctrl, EVMCS_PINCTRL);
+ evmcs_check_vmcs_conf32(cpu_based_2nd_exec_ctrl, EVMCS_2NDEXEC);
+ evmcs_check_vmcs_conf64(cpu_based_3rd_exec_ctrl, EVMCS_3RDEXEC);
+ evmcs_check_vmcs_conf32(vmentry_ctrl, EVMCS_ENTRY_CTRLS);
+ evmcs_check_vmcs_conf32(vmexit_ctrl, EVMCS_EXIT_CTRLS);
+}
+#endif
+
int nested_enable_evmcs(struct kvm_vcpu *vcpu,
uint16_t *vmcs_version)
{
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 205b5b467617..300e50d52042 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -271,6 +271,7 @@ static inline void evmcs_load(u64 phys_addr)
vp_ap->enlighten_vmentry = 1;
}
+__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
#else /* !IS_ENABLED(CONFIG_HYPERV) */
static __always_inline void evmcs_write64(unsigned long field, u64 value) {}
static inline void evmcs_write32(unsigned long field, u32 value) {}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9dba04b6b019..7fd21b1fae1d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2720,6 +2720,11 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
vmcs_conf->vmentry_ctrl = _vmentry_control;
vmcs_conf->misc = misc_msr;
+#if IS_ENABLED(CONFIG_HYPERV)
+ if (enlightened_vmcs)
+ evmcs_sanitize_exec_ctrls(vmcs_conf);
+#endif
+
return 0;
}
--
2.37.3
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V
2022-10-18 10:10 ` [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V Vitaly Kuznetsov
@ 2022-10-26 23:34 ` Sean Christopherson
2022-10-27 11:26 ` Vitaly Kuznetsov
0 siblings, 1 reply; 10+ messages in thread
From: Sean Christopherson @ 2022-10-26 23:34 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
> @@ -362,6 +364,7 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>
> enum evmcs_revision {
> EVMCSv1_LEGACY,
> + EVMCSv1_STRICT,
"strict" isn't really the right word, this is more like "raw" or "unfiltered",
because unless I'm misunderstanding the intent, this will always track KVM's
bleeding edge, i.e. everything that KVM can possibly enable.
And in that case, we can avoid bikeshedding the name becase bouncing through
evmcs_supported_ctrls is unnecessary, just use the #defines directly. And then
you can just fold the one (or two) #defines from patch 3 into this path.
> @@ -511,6 +525,52 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_HYPERV)
> +/*
> + * KVM on Hyper-V always uses the newest known eVMCSv1 revision, the assumption
> + * is: in case a feature has corresponding fields in eVMCS described and it was
> + * exposed in VMX feature MSRs, KVM is free to use it. Warn if KVM meets a
> + * feature which has no corresponding eVMCS field, this likely means that KVM
> + * needs to be updated.
> + */
> +#define evmcs_check_vmcs_conf32(field, ctrl) \
> + { \
> + u32 supported, unsupported32; \
> + \
> + supported = evmcs_get_supported_ctls(ctrl, EVMCSv1_STRICT); \
> + unsupported32 = vmcs_conf->field & ~supported; \
> + if (unsupported32) { \
> + pr_warn_once(#field " unsupported with eVMCS: 0x%x\n", \
> + unsupported32); \
> + vmcs_conf->field &= supported; \
> + } \
> + }
> +
> +#define evmcs_check_vmcs_conf64(field, ctrl) \
> + { \
> + u32 supported; \
> + u64 unsupported64; \
Channeling my inner Morpheus: Stop trying to use macros and use macros! :-D
---
arch/x86/kvm/vmx/evmcs.c | 34 ++++++++++++++++++++++++++++++++++
arch/x86/kvm/vmx/evmcs.h | 2 ++
arch/x86/kvm/vmx/vmx.c | 5 +++++
3 files changed, 41 insertions(+)
diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index 337783675731..f7f8eafeecf7 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -1,5 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
+#define pr_fmt(fmt) "kvm/hyper-v: " fmt
+
#include <linux/errno.h>
#include <linux/smp.h>
@@ -507,6 +509,38 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
return 0;
}
+#if IS_ENABLED(CONFIG_HYPERV)
+/*
+ * KVM on Hyper-V always uses the newest known eVMCSv1 revision, the assumption
+ * is: in case a feature has corresponding fields in eVMCS described and it was
+ * exposed in VMX feature MSRs, KVM is free to use it. Warn if KVM meets a
+ * feature which has no corresponding eVMCS field, this likely means that KVM
+ * needs to be updated.
+ */
+#define evmcs_check_vmcs_conf(field, ctrl) \
+do { \
+ typeof(vmcs_conf->field) unsupported; \
+ \
+ unsupported = vmcs_conf->field & EVMCS1_UNSUPPORTED_ ## ctrl; \
+ if (unsupported) { \
+ pr_warn_once(#field " unsupported with eVMCS: 0x%llx\n",\
+ (u64)unsupported); \
+ vmcs_conf->field &= ~unsupported; \
+ } \
+} \
+while (0)
+
+__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
+{
+ evmcs_check_vmcs_conf(cpu_based_exec_ctrl, EXEC_CTRL);
+ evmcs_check_vmcs_conf(pin_based_exec_ctrl, PINCTRL);
+ evmcs_check_vmcs_conf(cpu_based_2nd_exec_ctrl, 2NDEXEC);
+ evmcs_check_vmcs_conf(cpu_based_3rd_exec_ctrl, 3RDEXEC);
+ evmcs_check_vmcs_conf(vmentry_ctrl, VMENTRY_CTRL);
+ evmcs_check_vmcs_conf(vmexit_ctrl, VMEXIT_CTRL);
+}
+#endif
+
int nested_enable_evmcs(struct kvm_vcpu *vcpu,
uint16_t *vmcs_version)
{
diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
index 6f746ef3c038..bc795c6e9059 100644
--- a/arch/x86/kvm/vmx/evmcs.h
+++ b/arch/x86/kvm/vmx/evmcs.h
@@ -58,6 +58,7 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
SECONDARY_EXEC_SHADOW_VMCS | \
SECONDARY_EXEC_TSC_SCALING | \
SECONDARY_EXEC_PAUSE_LOOP_EXITING)
+#define EVMCS1_UNSUPPORTED_3RDEXEC (~0ULL)
#define EVMCS1_UNSUPPORTED_VMEXIT_CTRL \
(VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
#define EVMCS1_UNSUPPORTED_VMENTRY_CTRL (0)
@@ -209,6 +210,7 @@ static inline void evmcs_load(u64 phys_addr)
vp_ap->enlighten_vmentry = 1;
}
+__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
#else /* !IS_ENABLED(CONFIG_HYPERV) */
static __always_inline void evmcs_write64(unsigned long field, u64 value) {}
static inline void evmcs_write32(unsigned long field, u32 value) {}
diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index 9dba04b6b019..7fd21b1fae1d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2720,6 +2720,11 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
vmcs_conf->vmentry_ctrl = _vmentry_control;
vmcs_conf->misc = misc_msr;
+#if IS_ENABLED(CONFIG_HYPERV)
+ if (enlightened_vmcs)
+ evmcs_sanitize_exec_ctrls(vmcs_conf);
+#endif
+
return 0;
}
base-commit: 5b6b6bcc0ef138b55fdd17dc8f9d43dfd26f8bd7
--
^ permalink raw reply related [flat|nested] 10+ messages in thread* Re: [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V
2022-10-26 23:34 ` Sean Christopherson
@ 2022-10-27 11:26 ` Vitaly Kuznetsov
0 siblings, 0 replies; 10+ messages in thread
From: Vitaly Kuznetsov @ 2022-10-27 11:26 UTC (permalink / raw)
To: Sean Christopherson
Cc: kvm, Paolo Bonzini, Wanpeng Li, Jim Mattson, Maxim Levitsky,
linux-hyperv, linux-kernel
Sean Christopherson <seanjc@google.com> writes:
> On Tue, Oct 18, 2022, Vitaly Kuznetsov wrote:
>> @@ -362,6 +364,7 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>>
>> enum evmcs_revision {
>> EVMCSv1_LEGACY,
>> + EVMCSv1_STRICT,
>
> "strict" isn't really the right word, this is more like "raw" or "unfiltered",
> because unless I'm misunderstanding the intent, this will always track KVM's
> bleeding edge, i.e. everything that KVM can possibly enable.
>
Yes, it's unclear from the patch but this is a pre-requisite to exposing
'updated' eVMCSv1 controls (e.g. TSC scaling) for Hyper-V-on-KVM
case. Previously (https://lore.kernel.org/kvm/20220824030138.3524159-10-seanjc@google.com/)
we called it 'ENFORCED' but I misremembered and called it 'strict'.
> And in that case, we can avoid bikeshedding the name becase bouncing through
> evmcs_supported_ctrls is unnecessary, just use the #defines directly. And then
> you can just fold the one (or two) #defines from patch 3 into this path.
>
Defines can be used directly indeed and 'strict/enforcing/...'
discussion can happen when we finally come to exposing 'updated'
controls (hope we're almost there).
>> @@ -511,6 +525,52 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>> return 0;
>> }
>>
>> +#if IS_ENABLED(CONFIG_HYPERV)
>> +/*
>> + * KVM on Hyper-V always uses the newest known eVMCSv1 revision, the assumption
>> + * is: in case a feature has corresponding fields in eVMCS described and it was
>> + * exposed in VMX feature MSRs, KVM is free to use it. Warn if KVM meets a
>> + * feature which has no corresponding eVMCS field, this likely means that KVM
>> + * needs to be updated.
>> + */
>> +#define evmcs_check_vmcs_conf32(field, ctrl) \
>> + { \
>> + u32 supported, unsupported32; \
>> + \
>> + supported = evmcs_get_supported_ctls(ctrl, EVMCSv1_STRICT); \
>> + unsupported32 = vmcs_conf->field & ~supported; \
>> + if (unsupported32) { \
>> + pr_warn_once(#field " unsupported with eVMCS: 0x%x\n", \
>> + unsupported32); \
>> + vmcs_conf->field &= supported; \
>> + } \
>> + }
>> +
>> +#define evmcs_check_vmcs_conf64(field, ctrl) \
>> + { \
>> + u32 supported; \
>> + u64 unsupported64; \
>
> Channeling my inner Morpheus: Stop trying to use macros and use macros! :-D
>
> ---
> arch/x86/kvm/vmx/evmcs.c | 34 ++++++++++++++++++++++++++++++++++
> arch/x86/kvm/vmx/evmcs.h | 2 ++
> arch/x86/kvm/vmx/vmx.c | 5 +++++
> 3 files changed, 41 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 337783675731..f7f8eafeecf7 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -1,5 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
>
> +#define pr_fmt(fmt) "kvm/hyper-v: " fmt
> +
> #include <linux/errno.h>
> #include <linux/smp.h>
>
> @@ -507,6 +509,38 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_HYPERV)
> +/*
> + * KVM on Hyper-V always uses the newest known eVMCSv1 revision, the assumption
> + * is: in case a feature has corresponding fields in eVMCS described and it was
> + * exposed in VMX feature MSRs, KVM is free to use it. Warn if KVM meets a
> + * feature which has no corresponding eVMCS field, this likely means that KVM
> + * needs to be updated.
> + */
> +#define evmcs_check_vmcs_conf(field, ctrl) \
> +do { \
> + typeof(vmcs_conf->field) unsupported; \
> + \
> + unsupported = vmcs_conf->field & EVMCS1_UNSUPPORTED_ ## ctrl; \
> + if (unsupported) { \
> + pr_warn_once(#field " unsupported with eVMCS: 0x%llx\n",\
> + (u64)unsupported); \
> + vmcs_conf->field &= ~unsupported; \
> + } \
> +} \
> +while (0)
> +
> +__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf)
> +{
> + evmcs_check_vmcs_conf(cpu_based_exec_ctrl, EXEC_CTRL);
> + evmcs_check_vmcs_conf(pin_based_exec_ctrl, PINCTRL);
> + evmcs_check_vmcs_conf(cpu_based_2nd_exec_ctrl, 2NDEXEC);
> + evmcs_check_vmcs_conf(cpu_based_3rd_exec_ctrl, 3RDEXEC);
> + evmcs_check_vmcs_conf(vmentry_ctrl, VMENTRY_CTRL);
> + evmcs_check_vmcs_conf(vmexit_ctrl, VMEXIT_CTRL);
> +}
> +#endif
> +
> int nested_enable_evmcs(struct kvm_vcpu *vcpu,
> uint16_t *vmcs_version)
> {
> diff --git a/arch/x86/kvm/vmx/evmcs.h b/arch/x86/kvm/vmx/evmcs.h
> index 6f746ef3c038..bc795c6e9059 100644
> --- a/arch/x86/kvm/vmx/evmcs.h
> +++ b/arch/x86/kvm/vmx/evmcs.h
> @@ -58,6 +58,7 @@ DECLARE_STATIC_KEY_FALSE(enable_evmcs);
> SECONDARY_EXEC_SHADOW_VMCS | \
> SECONDARY_EXEC_TSC_SCALING | \
> SECONDARY_EXEC_PAUSE_LOOP_EXITING)
> +#define EVMCS1_UNSUPPORTED_3RDEXEC (~0ULL)
> #define EVMCS1_UNSUPPORTED_VMEXIT_CTRL \
> (VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)
> #define EVMCS1_UNSUPPORTED_VMENTRY_CTRL (0)
> @@ -209,6 +210,7 @@ static inline void evmcs_load(u64 phys_addr)
> vp_ap->enlighten_vmentry = 1;
> }
>
> +__init void evmcs_sanitize_exec_ctrls(struct vmcs_config *vmcs_conf);
> #else /* !IS_ENABLED(CONFIG_HYPERV) */
> static __always_inline void evmcs_write64(unsigned long field, u64 value) {}
> static inline void evmcs_write32(unsigned long field, u32 value) {}
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 9dba04b6b019..7fd21b1fae1d 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -2720,6 +2720,11 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
> vmcs_conf->vmentry_ctrl = _vmentry_control;
> vmcs_conf->misc = misc_msr;
>
> +#if IS_ENABLED(CONFIG_HYPERV)
> + if (enlightened_vmcs)
> + evmcs_sanitize_exec_ctrls(vmcs_conf);
> +#endif
> +
> return 0;
> }
>
>
> base-commit: 5b6b6bcc0ef138b55fdd17dc8f9d43dfd26f8bd7
--
Vitaly
^ permalink raw reply [flat|nested] 10+ messages in thread