Re: [PATCH 0/4] KVM: x86: Collect host state snapshots into a struct

[Sean Christopherson]

On Thu, Apr 25, 2024, Wei W Wang wrote:
> On Wednesday, April 24, 2024 6:15 AM, Sean Christopherson wrote:
> > Add a global "kvm_host" structure to hold various host values, e.g. for EFER,
> > XCR0, raw MAXPHYADDR etc., instead of having a bunch of one-off variables
> > that inevitably need to be exported, or in the case of shadow_phys_bits, are
> > buried in a random location and are awkward to use, leading to duplicate
> > code.
> 
> Looks good. How about applying similar improvements to the module
> parameters as well? I've changed the "enable_pmu" parameter as an example below:

Hmm, I don't hate the idea, but I don't think it would work as well in practice.

For kvm_host, all of the fields it contains were being namespace with "host_<asset>",

And the globals that became kvm_caps all had some variant of kvm_ or kvm_has_ as
a prefix.

For module params and other knobs, the thing being controlled is usually unique
to KVM, and often fairly self-descriptive, so we haven't had to namespace them
muc.  And we have params across kvm.ko, kvm-amd.ko, and kvm-intel.ko, which
sometimes weird splits in responsibilities, e.g. enable_apicv is defined by common
x86, but the module params themsleves are defined by SVM and VMX, and for SVM it's
an alias of avic.

> diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> index 77352a4abd87..a221ba7b546f 100644
> --- a/arch/x86/kvm/cpuid.c
> +++ b/arch/x86/kvm/cpuid.c
> @@ -1013,7 +1013,7 @@ static inline int __do_cpuid_func(struct kvm_cpuid_array *array, u32 function)
>                 union cpuid10_eax eax;
>                 union cpuid10_edx edx;
> 
> -               if (!enable_pmu || !static_cpu_has(X86_FEATURE_ARCH_PERFMON)) {
> +               if (!kvm_caps.enable_pmu || !static_cpu_has(X86_FEATURE_ARCH_PERFMON)) {

If we did try to collect module params in a struct, it should be a unique struct,
because they aren't pure capabilities or host values.

>                         entry->eax = entry->ebx = entry->ecx = entry->edx = 0;
>                         break;
>                 }