Re: [PATCH v5 03/26] x86/hyperv: Update 'struct hv_enlightened_vmcs' definition

[Vitaly Kuznetsov <vkuznets@redhat.com>]

Sean Christopherson <seanjc@google.com> writes:

> We're talking about nested VMX, i.e. exposing TSC_SCALING to L1.  QEMU's CLX
> definition doesn't include TSC_SCALING.  In fact, none of QEMU's predefined CPU
> models supports TSC_SCALING, precisely because KVM didn't support exposing the
> feature to L1 until relatively recently.
>
> $ git grep VMX_SECONDARY_EXEC_TSC_SCALING
> target/i386/cpu.h:#define VMX_SECONDARY_EXEC_TSC_SCALING              0x02000000
> target/i386/kvm/kvm.c:    if (f[FEAT_VMX_SECONDARY_CTLS] &  VMX_SECONDARY_EXEC_TSC_SCALING) {

(sorry for my persistence but I still believe there are issues which we
won't be able to solve if we take the suggested approach).

You got me. Indeed, "vmx-tsc-scaling" feature is indeed not set for
named CPU models so my example was flawed. Let's swap it with
VMX_VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL /
VMX_VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL which a bunch of named models
have. So I do the same,

'-cpu CascadeLake-Sever,hv-evmcs'

on both the source host which knows about these eVMCS fields and the
destination host which doesn't.

First problem: CPUID. On the source host, we will have
CPUID.0x4000000A.EBX BIT(0) = 1, and "=0" on the destination. I don't
think we migrate CPUID data (can be wrong, though).

Second, assuming VMX feature MSRs are actually migrated, we must fail on
the destnation because VMX_VM_{ENTRY,EXIT}_LOAD_IA32_PERF_GLOBAL_CTRL is
trying to get set. We can do this in KVM but note: currently, KVM
filters guest reads but not host's so when you're trying to migrate from
a non-fixed KVM, VMX_VM_{ENTRY,EXIT}_LOAD_IA32_PERF_GLOBAL_CTRL are
actually present! So how do we distinguinsh in KVM between these two
cases, i.e. how do we know if
VMX_VM_{ENTRY,EXIT}_LOAD_IA32_PERF_GLOBAL_CTRL were filtered out on the
source (old kvm) or not (new KVM)?

...
>
> Because it's completely unnecessary, adds non-trivial maintenance burden to KVM,
> and requires explicit documentation to explain to userspace what "hv-evmcs-2022"
> means.
>
> It's unnecessary because if the user is concerned about eVMCS features showing up
> in the future, then they should do:
>
>   -cpu CascadeLake-Server,hv-evmcs,-vmx-tsc-scaling,-<any other VMX features not eVMCS-friendly>
>
> If QEMU wants to make that more user friendly, then define CascadeLake-Server-eVMCS
> or whatever so that the features that are unlikely be supported for eVMCS are off by
> default.

I completely agree that what I'm trying to achieve here could've been
done in QEMU from day 1 but we now have what we have: KVM silently
filtering out certain VMX features and zero indication to userspace
VMM whether filtering is being done or not (besides this
CPUID.0x4000000A.EBX BIT(0) bit but I'm not even sure we analyze
source's CPUID data upon migration).

>  This is no different than QEMU not including nested TSC_SCALING in any of
> the predefined models; the developers _know_ KVM doesn't widely support TSC_SCALING,
> so it was omitted, even though a real CLX CPU is guaranteed to support TSC_SCALING.
>

Out of curiosity, what happens if someone sends the following patch to
QEMU:

diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 1db1278a599b..2278f4522b44 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -3191,6 +3191,12 @@ static const X86CPUDefinition builtin_x86_defs[] = {
                   { "vmx-xsaves", "on" },
                   { /* end of list */ }
               },
+            { .version = 6,
+              .note = "ARCH_CAPABILITIES, EPT switching, XSAVES, no TSX, TSC_SCALING",
+              .props = (PropValue[]) {
+                  { "vmx-tsc-scaling", "on" },
+                  { /* end of list */ }
+              },
             },
             { /* end of list */ }
         }

Will Paolo remember about eVMCS and reject it?

> It's non-trivial maintenance for KVM because it would require defining new versions
> every time an eVMCS field is added, allowing userspace to specify and restrict
> features based on arbitrary versions, and do all of that without conflicting with
> whatever PV enumeration Microsoft adds.

The update at hand comes with a feature bit so no mater what we do, we
will need a new QEMU flag to support this feature bit. My suggestion was
just that we stretch its definition a bit and encode not only
PERF_GLOBAL_CTRL but all fields which were added. At the same time we
can switch to filtering host reads and failing host writes for what's
missing (and to do so we'll likely need to invert the logic and
explicitly list what eVMCS supports) so we're better prepared to the
next update.

-- 
Vitaly