Sean Christopherson <seanjc@xxxxxxxxxx> 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@xxxxxxxxxx/)
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
