Re: [PATCH 4/4] KVM: VMX: Resurrect vmcs_conf sanitization for KVM-on-Hyper-V

[Vitaly Kuznetsov]

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