Received-SPF: pass (google.com: domain of linux-kernel+bounces-158705-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Thu, 25 Apr 2024 07:24:25 -0700
In-Reply-To: <DS0PR11MB6373E404A16BD8CC55128FDFDC172@DS0PR11MB6373.namprd11.prod.outlook.com>
Precedence: bulk
Mime-Version: 1.0
References: <20240423221521.2923759-1-seanjc@google.com> <DS0PR11MB6373E404A16BD8CC55128FDFDC172@DS0PR11MB6373.namprd11.prod.outlook.com>
Message-ID: <ZipnmfPs0dP3fRUE@google.com>
Subject: Re: [PATCH 0/4] KVM: x86: Collect host state snapshots into a struct
From: Sean Christopherson <seanjc@google.com>
To: Wei W Wang <wei.w.wang@intel.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, "kvm@vger.kernel.org" <kvm@vger.kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="us-ascii"

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