Re: [PATCH v4 09/25] KVM: VMX: nVMX: Support TSC scaling and PERF_GLOBAL_CTRL with enlightened VMCS

[Sean Christopherson <seanjc@google.com>]

Nit of all nits, just "KVM: nVMX:" in the shortlog.

On Thu, Jul 14, 2022, Vitaly Kuznetsov wrote:
> Enlightened VMCS v1 got updated and now includes the required fields
> for TSC scaling and loading PERF_GLOBAL_CTRL upon VMENTER/VMEXIT
> features. For Hyper-V on KVM enablement, KVM can just observe VMX control
> MSRs and use the features (with or without eVMCS) when
> possible. Hyper-V on KVM case is trickier because of live migration:
> the new features require explicit enablement from VMM to not break
> it. Luckily, the updated eVMCS revision comes with a feature bit in
> CPUID.0x4000000A.EBX.

Mostly out of curiosity, what happens if the VMM parrots back the results of
kvm_get_hv_cpuid()?

> Reviewed-by: Maxim Levitsky <mlevitsk@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  arch/x86/kvm/hyperv.c     |  2 +-
>  arch/x86/kvm/vmx/evmcs.c  | 88 +++++++++++++++++++++++++++++++--------
>  arch/x86/kvm/vmx/evmcs.h  | 17 ++------
>  arch/x86/kvm/vmx/nested.c |  2 +-
>  arch/x86/kvm/vmx/vmx.c    |  2 +-
>  5 files changed, 78 insertions(+), 33 deletions(-)
> 
> diff --git a/arch/x86/kvm/hyperv.c b/arch/x86/kvm/hyperv.c
> index a8e4944ca110..995d3ab1443e 100644
> --- a/arch/x86/kvm/hyperv.c
> +++ b/arch/x86/kvm/hyperv.c
> @@ -2552,7 +2552,7 @@ int kvm_get_hv_cpuid(struct kvm_vcpu *vcpu, struct kvm_cpuid2 *cpuid,
>  		case HYPERV_CPUID_NESTED_FEATURES:
>  			ent->eax = evmcs_ver;
>  			ent->eax |= HV_X64_NESTED_MSR_BITMAP;
> -
> +			ent->ebx |= HV_X64_NESTED_EVMCS1_2022_UPDATE;
>  			break;
>  
>  		case HYPERV_CPUID_SYNDBG_VENDOR_AND_MAX_FUNCTIONS:
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index 8bea5dea0341..52a53debd806 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -368,7 +368,60 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
>  
> -void nested_evmcs_filter_control_msr(u32 msr_index, u64 *pdata)
> +enum evmcs_v1_revision {
> +	EVMCSv1_2016,
> +	EVMCSv1_2022,
> +};

Why bother with the enums?  They don't make life any easier, e.g. if there's a
2023 update then it's easy to do:

		unsupported = <baseline>;
		if (!has_evmcs_2022_features)
			unsupported |= <2022 features>;
		if (!has_evmcs_2023_features)
			unsupported |= <2023 features>;
		return unsupported;

diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
index b5cfbf7d487b..7b348a03e096 100644
--- a/arch/x86/kvm/vmx/evmcs.c
+++ b/arch/x86/kvm/vmx/evmcs.c
@@ -355,11 +355,6 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
        return 0;
 }

-enum evmcs_v1_revision {
-       EVMCSv1_2016,
-       EVMCSv1_2022,
-};
-
 enum evmcs_unsupported_ctrl_type {
        EVMCS_EXIT_CTLS,
        EVMCS_ENTRY_CTLS,
@@ -372,29 +367,29 @@ static u32 evmcs_get_unsupported_ctls(struct kvm_vcpu *vcpu,
                                      enum evmcs_unsupported_ctrl_type ctrl_type)
 {
        struct kvm_vcpu_hv *hv_vcpu = to_hv_vcpu(vcpu);
-       enum evmcs_v1_revision evmcs_rev = EVMCSv1_2016;
+       bool has_evmcs_2022_features;

        if (!hv_vcpu)
                return 0;

-       if (hv_vcpu->cpuid_cache.nested_ebx & HV_X64_NESTED_EVMCS1_2022_UPDATE)
-               evmcs_rev = EVMCSv1_2022;
+       has_evmcs_2022_features = hv_vcpu->cpuid_cache.nested_ebx &
+                                 HV_X64_NESTED_EVMCS1_2022_UPDATE;

        switch (ctrl_type) {
        case EVMCS_EXIT_CTLS:
-               if (evmcs_rev == EVMCSv1_2016)
+               if (!has_evmcs_2022_features)
                        return EVMCS1_UNSUPPORTED_VMEXIT_CTRL |
                                VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
                else
                        return EVMCS1_UNSUPPORTED_VMEXIT_CTRL;
        case EVMCS_ENTRY_CTLS:
-               if (evmcs_rev == EVMCSv1_2016)
+               if (!has_evmcs_2022_features)
                        return EVMCS1_UNSUPPORTED_VMENTRY_CTRL |
                                VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
                else
                        return EVMCS1_UNSUPPORTED_VMENTRY_CTRL;
        case EVMCS_2NDEXEC:
-               if (evmcs_rev == EVMCSv1_2016)
+               if (!has_evmcs_2022_features)
                        return EVMCS1_UNSUPPORTED_2NDEXEC |
                                SECONDARY_EXEC_TSC_SCALING;
                else

> +
> +enum evmcs_unsupported_ctrl_type {
> +	EVMCS_EXIT_CTLS,
> +	EVMCS_ENTRY_CTLS,
> +	EVMCS_2NDEXEC,
> +	EVMCS_PINCTRL,
> +	EVMCS_VMFUNC,
> +};

Same question here, it just makes life more difficult.  I.e. do

  static u32 evmcs_get_unsupported_ctls(struct kvm_vcpu *vcpu, int msr_index)

and then

  void nested_evmcs_filter_control_msr(struct kvm_vcpu *vcpu, u32 msr_index, u64 *pdata)
  {
	u32 ctl_low = (u32)*pdata;
	u32 ctl_high = (u32)(*pdata >> 32);

	/*
	 * Hyper-V 2016 and 2019 try using unsupported features when eVMCS is
	 * enabled but there are no corresponding fields.
	 */
	ctl_high &= ~evmcs_get_unsupported_ctls(vcpu, msr_index);

	*pdata = ctl_low | ((u64)ctl_high << 32);
  }


  int nested_evmcs_check_controls(struct kvm_vcpu *vcpu, struct vmcs12 *vmcs12)
  {
	int ret = 0;
	u32 unsupp_ctl;

	unsupp_ctl = vmcs12->pin_based_vm_exec_control &
		evmcs_get_unsupported_ctls(vcpu, MSR_IA32_VMX_TRUE_PINBASED_CTLS);

	<and so on and so forth>
  }