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

Sean Christopherson
2022-07-21T21:59:09Z

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>
}