Re: [RFC PATCH v6 07/36] KVM: nVMX: Refactor unsupported eVMCS controls logic to use 2-d array

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> From: Vitaly Kuznetsov <vkuznets@redhat.com>
>
> Refactor the handling of unsupported eVMCS to use a 2-d array to store
> the set of unsupported controls.  KVM's handling of eVMCS is completely
> broken as there is no way for userspace to query which features are
> unsupported, nor does KVM prevent userspace from attempting to enable
> unsupported features.  A future commit will remedy that by filtering and
> enforcing unsupported features when eVMCS, but that needs to be opt-in
> from userspace to avoid breakage, i.e. KVM needs to maintain its legacy
> behavior by snapshotting the exact set of controls that are currently
> (un)supported by eVMCS.
>
> No functional change intended.
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> [sean: split to standalone patch, write changelog]
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/vmx/evmcs.c | 60 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 50 insertions(+), 10 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 9139c70b6008..10fc0be49f96 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -345,6 +345,45 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> +enum evmcs_revision {
> +	EVMCSv1_LEGACY,
> +	NR_EVMCS_REVISIONS,
> +};
> +
> +enum evmcs_ctrl_type {
> +	EVMCS_EXIT_CTRLS,
> +	EVMCS_ENTRY_CTRLS,
> +	EVMCS_2NDEXEC,
> +	EVMCS_PINCTRL,
> +	EVMCS_VMFUNC,
> +	NR_EVMCS_CTRLS,
> +};
> +
> +static const u32 evmcs_unsupported_ctrls[NR_EVMCS_CTRLS][NR_EVMCS_REVISIONS] = {
> +	[EVMCS_EXIT_CTRLS] = {
> +		[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMEXIT_CTRL | VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
> +	},
> +	[EVMCS_ENTRY_CTRLS] = {
> +		[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMENTRY_CTRL | VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
> +	},
> +	[EVMCS_2NDEXEC] = {
> +		[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_2NDEXEC | SECONDARY_EXEC_TSC_SCALING,

By the time of this patch, VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL, SECONDARY_EXEC_TSC_SCALING are
still in 'EVMCS1_UNSUPPORTED_*' lists.

> +	},
> +	[EVMCS_PINCTRL] = {
> +		[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_PINCTRL,
> +	},
> +	[EVMCS_VMFUNC] = {
> +		[EVMCSv1_LEGACY] = EVMCS1_UNSUPPORTED_VMFUNC,
> +	},
> +};
> +
> +static u32 evmcs_get_unsupported_ctls(enum evmcs_ctrl_type ctrl_type)
> +{
> +	enum evmcs_revision evmcs_rev = EVMCSv1_LEGACY;
> +
> +	return evmcs_unsupported_ctrls[ctrl_type][evmcs_rev];
> +}
> +
>  void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata)
>  {
>  	u32 ctl_low = (u32)*pdata;
> @@ -357,21 +396,21 @@ void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata)
>  	switch (msr_index) {
>  	case MSR_IA32_VMX_EXIT_CTLS:
>  	case MSR_IA32_VMX_TRUE_EXIT_CTLS:
> -		ctl_high &= ~EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
> +		ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_EXIT_CTRLS);
>  		break;
>  	case MSR_IA32_VMX_ENTRY_CTLS:
>  	case MSR_IA32_VMX_TRUE_ENTRY_CTLS:
> -		ctl_high &= ~EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
> +		ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_ENTRY_CTRLS);
>  		break;
>  	case MSR_IA32_VMX_PROCBASED_CTLS2:
> -		ctl_high &= ~EVMCS1_UNSUPPORTED_2NDEXEC;
> +		ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_2NDEXEC);
>  		break;
>  	case MSR_IA32_VMX_TRUE_PINBASED_CTLS:
>  	case MSR_IA32_VMX_PINBASED_CTLS:
> -		ctl_high &= ~EVMCS1_UNSUPPORTED_PINCTRL;
> +		ctl_high &= ~evmcs_get_unsupported_ctls(EVMCS_PINCTRL);
>  		break;
>  	case MSR_IA32_VMX_VMFUNC:
> -		ctl_low &= ~EVMCS1_UNSUPPORTED_VMFUNC;
> +		ctl_low &= ~evmcs_get_unsupported_ctls(EVMCS_VMFUNC);
>  		break;
>  	}
>  
> @@ -384,7 +423,7 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>  	u32 unsupp_ctl;
>  
>  	unsupp_ctl = vmcs12->pin_based_vm_exec_control &
> -		EVMCS1_UNSUPPORTED_PINCTRL;
> +		evmcs_get_unsupported_ctls(EVMCS_PINCTRL);
>  	if (unsupp_ctl) {
>  		trace_kvm_nested_vmenter_failed(
>  			"eVMCS: unsupported pin-based VM-execution controls",
> @@ -393,7 +432,7 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>  	}
>  
>  	unsupp_ctl = vmcs12->secondary_vm_exec_control &
> -		EVMCS1_UNSUPPORTED_2NDEXEC;
> +		evmcs_get_unsupported_ctls(EVMCS_2NDEXEC);
>  	if (unsupp_ctl) {
>  		trace_kvm_nested_vmenter_failed(
>  			"eVMCS: unsupported secondary VM-execution controls",
> @@ -402,7 +441,7 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>  	}
>  
>  	unsupp_ctl = vmcs12->vm_exit_controls &
> -		EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
> +		evmcs_get_unsupported_ctls(EVMCS_EXIT_CTRLS);
>  	if (unsupp_ctl) {
>  		trace_kvm_nested_vmenter_failed(
>  			"eVMCS: unsupported VM-exit controls",
> @@ -411,7 +450,7 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>  	}
>  
>  	unsupp_ctl = vmcs12->vm_entry_controls &
> -		EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
> +		evmcs_get_unsupported_ctls(EVMCS_ENTRY_CTRLS);
>  	if (unsupp_ctl) {
>  		trace_kvm_nested_vmenter_failed(
>  			"eVMCS: unsupported VM-entry controls",
> @@ -419,7 +458,8 @@ int nested_evmcs_check_controls(struct vmcs12 *vmcs12)
>  		ret = -EINVAL;
>  	}
>  
> -	unsupp_ctl = vmcs12->vm_function_control & EVMCS1_UNSUPPORTED_VMFUNC;
> +	unsupp_ctl = vmcs12->vm_function_control &
> +		evmcs_get_unsupported_ctls(EVMCS_VMFUNC);
>  	if (unsupp_ctl) {
>  		trace_kvm_nested_vmenter_failed(
>  			"eVMCS: unsupported VM-function controls",

-- 
Vitaly