Re: [PATCH v3 00/57] KVM: x86: CPUID overhaul, fixes, and caching

[Maxim Levitsky <mlevitsk@redhat.com>]

On Wed, 2024-11-27 at 17:33 -0800, Sean Christopherson wrote:
> The super short TL;DR: snapshot all X86_FEATURE_* flags that KVM cares
> about so that all queries against guest capabilities are "fast", e.g. don't
> require manual enabling or judgment calls as to where a feature needs to be
> fast.
> 
> The guest_cpu_cap_* nomenclature follows the existing kvm_cpu_cap_*
> except for a few (maybe just one?) cases where guest cpu_caps need APIs
> that kvm_cpu_caps don't.  In theory, the similar names will make this
> approach more intuitive.
> 
> This series also adds more hardening, e.g. to assert at compile-time if a
> feature flag is passed to the wrong word.  It also sets the stage for even
> more hardening in the future, as tracking all KVM-supported features allows
> shoving known vs. used features into arrays at compile time, which can then
> be checked for consistency irrespective of hardware support.  E.g. allows
> detecting if KVM is checking a feature without advertising it to userspace.
> This extra hardening is future work; I have it mostly working, but it's ugly
> and requires a runtime check to process the generated arrays.
> 
> There are *multiple* potentially breaking changes in this series (in for a
> penny, in for a pound).  However, I don't expect any fallout for real world
> VMMs because the ABI changes either disallow things that couldn't possibly
> have worked in the first place, or are following in the footsteps of other
> behaviors, e.g. KVM advertises x2APIC, which is 100% dependent on an in-kernel
> local APIC.
> 
>  * Disallow stuffing CPUID-dependent guest CR4 features before setting guest
>    CPUID.
>  * Disallow KVM_CAP_X86_DISABLE_EXITS after vCPU creation
>  * Reject disabling of MWAIT/HLT interception when not allowed
>  * Advertise TSC_DEADLINE_TIMER in KVM_GET_SUPPORTED_CPUID.
>  * Advertise HYPERVISOR in KVM_GET_SUPPORTED_CPUID
> 
> Validated the flag rework by comparing the output of KVM_GET_SUPPORTED_CPUID
> (and the emulated version) at the beginning and end of the series, on AMD
> and Intel hosts that should support almost every feature known to KVM.
> 
> Maxim, I did my best to incorporate all of your feedback, and when we
> disagreed, I tried to find an approach that I we can hopefully both live
> with, at least until someone comes up with a better idea.
> 
> I _think_ the only suggestion that I "rejected" entirely is the existence
> of ALIASED_1_EDX_F.  I responded to the previous thread, definitely feel
> free to continue the conversation there (or here).
> 
> If I missed something you care strongly about, please holler!

Hi,

I did go over this patch series, I don't think I have anything to add,
there are still things I disagree, especially the F* macros, IMHO this
makes the code less readable.

So if you want to merge this, I won't object.

Thanks,
Best regards,
	Maxim Levitsky

> 
> v3:
>  - Collect more reviews.
>  - Too many to list.
>  
> v2:
>  - Collect a few reviews (though I dropped several due to the patches changing
>    significantly).
>  - Incorporate KVM's support into the vCPU's cpu_caps. [Maxim]
>  - A massive pile of new patches.
> 
> Sean Christopherson (57):
>   KVM: x86: Use feature_bit() to clear CONSTANT_TSC when emulating CPUID
>   KVM: x86: Limit use of F() and SF() to
>     kvm_cpu_cap_{mask,init_kvm_defined}()
>   KVM: x86: Do all post-set CPUID processing during vCPU creation
>   KVM: x86: Explicitly do runtime CPUID updates "after" initial setup
>   KVM: x86: Account for KVM-reserved CR4 bits when passing through CR4
>     on VMX
>   KVM: selftests: Update x86's set_sregs_test to match KVM's CPUID
>     enforcement
>   KVM: selftests: Assert that vcpu->cpuid is non-NULL when getting CPUID
>     entries
>   KVM: selftests: Refresh vCPU CPUID cache in __vcpu_get_cpuid_entry()
>   KVM: selftests: Verify KVM stuffs runtime CPUID OS bits on CR4 writes
>   KVM: x86: Move __kvm_is_valid_cr4() definition to x86.h
>   KVM: x86/pmu: Drop now-redundant refresh() during init()
>   KVM: x86: Drop now-redundant MAXPHYADDR and GPA rsvd bits from vCPU
>     creation
>   KVM: x86: Disallow KVM_CAP_X86_DISABLE_EXITS after vCPU creation
>   KVM: x86: Reject disabling of MWAIT/HLT interception when not allowed
>   KVM: x86: Drop the now unused KVM_X86_DISABLE_VALID_EXITS
>   KVM: selftests: Fix a bad TEST_REQUIRE() in x86's KVM PV test
>   KVM: selftests: Update x86's KVM PV test to match KVM's disabling
>     exits behavior
>   KVM: x86: Zero out PV features cache when the CPUID leaf is not
>     present
>   KVM: x86: Don't update PV features caches when enabling enforcement
>     capability
>   KVM: x86: Do reverse CPUID sanity checks in __feature_leaf()
>   KVM: x86: Account for max supported CPUID leaf when getting raw host
>     CPUID
>   KVM: x86: Unpack F() CPUID feature flag macros to one flag per line of
>     code
>   KVM: x86: Rename kvm_cpu_cap_mask() to kvm_cpu_cap_init()
>   KVM: x86: Add a macro to init CPUID features that are 64-bit only
>   KVM: x86: Add a macro to precisely handle aliased 0x1.EDX CPUID
>     features
>   KVM: x86: Handle kernel- and KVM-defined CPUID words in a single
>     helper
>   KVM: x86: #undef SPEC_CTRL_SSBD in cpuid.c to avoid macro collisions
>   KVM: x86: Harden CPU capabilities processing against out-of-scope
>     features
>   KVM: x86: Add a macro to init CPUID features that ignore host kernel
>     support
>   KVM: x86: Add a macro to init CPUID features that KVM emulates in
>     software
>   KVM: x86: Swap incoming guest CPUID into vCPU before massaging in
>     KVM_SET_CPUID2
>   KVM: x86: Clear PV_UNHALT for !HLT-exiting only when userspace sets
>     CPUID
>   KVM: x86: Remove unnecessary caching of KVM's PV CPUID base
>   KVM: x86: Always operate on kvm_vcpu data in cpuid_entry2_find()
>   KVM: x86: Move kvm_find_cpuid_entry{,_index}() up near
>     cpuid_entry2_find()
>   KVM: x86: Remove all direct usage of cpuid_entry2_find()
>   KVM: x86: Advertise TSC_DEADLINE_TIMER in KVM_GET_SUPPORTED_CPUID
>   KVM: x86: Advertise HYPERVISOR in KVM_GET_SUPPORTED_CPUID
>   KVM: x86: Rename "governed features" helpers to use "guest_cpu_cap"
>   KVM: x86: Replace guts of "governed" features with comprehensive
>     cpu_caps
>   KVM: x86: Initialize guest cpu_caps based on guest CPUID
>   KVM: x86: Extract code for generating per-entry emulated CPUID
>     information
>   KVM: x86: Treat MONTIOR/MWAIT as a "partially emulated" feature
>   KVM: x86: Initialize guest cpu_caps based on KVM support
>   KVM: x86: Avoid double CPUID lookup when updating MWAIT at runtime
>   KVM: x86: Drop unnecessary check that cpuid_entry2_find() returns
>     right leaf
>   KVM: x86: Update OS{XSAVE,PKE} bits in guest CPUID irrespective of
>     host support
>   KVM: x86: Update guest cpu_caps at runtime for dynamic CPUID-based
>     features
>   KVM: x86: Shuffle code to prepare for dropping guest_cpuid_has()
>   KVM: x86: Replace (almost) all guest CPUID feature queries with
>     cpu_caps
>   KVM: x86: Drop superfluous host XSAVE check when adjusting guest
>     XSAVES caps
>   KVM: x86: Add a macro for features that are synthesized into
>     boot_cpu_data
>   KVM: x86: Pull CPUID capabilities from boot_cpu_data only as needed
>   KVM: x86: Rename "SF" macro to "SCATTERED_F"
>   KVM: x86: Explicitly track feature flags that require vendor enabling
>   KVM: x86: Explicitly track feature flags that are enabled at runtime
>   KVM: x86: Use only local variables (no bitmask) to init kvm_cpu_caps
> 
>  Documentation/virt/kvm/api.rst                |  10 +-
>  arch/x86/include/asm/kvm_host.h               |  47 +-
>  arch/x86/kvm/cpuid.c                          | 967 ++++++++++++------
>  arch/x86/kvm/cpuid.h                          | 128 +--
>  arch/x86/kvm/governed_features.h              |  22 -
>  arch/x86/kvm/hyperv.c                         |   2 +-
>  arch/x86/kvm/lapic.c                          |   4 +-
>  arch/x86/kvm/mmu.h                            |   2 +-
>  arch/x86/kvm/mmu/mmu.c                        |   4 +-
>  arch/x86/kvm/pmu.c                            |   1 -
>  arch/x86/kvm/reverse_cpuid.h                  |  23 +-
>  arch/x86/kvm/smm.c                            |  10 +-
>  arch/x86/kvm/svm/nested.c                     |  22 +-
>  arch/x86/kvm/svm/pmu.c                        |   8 +-
>  arch/x86/kvm/svm/sev.c                        |  21 +-
>  arch/x86/kvm/svm/svm.c                        |  46 +-
>  arch/x86/kvm/svm/svm.h                        |   4 +-
>  arch/x86/kvm/vmx/hyperv.h                     |   2 +-
>  arch/x86/kvm/vmx/nested.c                     |  18 +-
>  arch/x86/kvm/vmx/pmu_intel.c                  |   4 +-
>  arch/x86/kvm/vmx/sgx.c                        |  14 +-
>  arch/x86/kvm/vmx/vmx.c                        |  61 +-
>  arch/x86/kvm/x86.c                            | 153 ++-
>  arch/x86/kvm/x86.h                            |   6 +-
>  include/uapi/linux/kvm.h                      |   4 -
>  .../selftests/kvm/include/x86_64/processor.h  |  18 +-
>  .../selftests/kvm/x86_64/kvm_pv_test.c        |  38 +-
>  .../selftests/kvm/x86_64/set_sregs_test.c     |  63 +-
>  28 files changed, 1017 insertions(+), 685 deletions(-)
>  delete mode 100644 arch/x86/kvm/governed_features.h
> 
> 
> base-commit: 4d911c7abee56771b0219a9fbf0120d06bdc9c14