Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 29 Jun 2023 09:26:30 -0700
In-Reply-To: <9d9e9156-b9d9-86b4-9d20-77305e1e4d63@linux.intel.com>
Mime-Version: 1.0
References: <20230217231022.816138-1-seanjc@google.com> <20230217231022.816138-2-seanjc@google.com>
 <9d9e9156-b9d9-86b4-9d20-77305e1e4d63@linux.intel.com>
Message-ID: <ZJ2wtg5KJyhD3cUe@google.com>
Subject: Re: [PATCH 01/12] KVM: x86: Add a framework for enabling KVM-governed
 x86 features
From:   Sean Christopherson <seanjc@google.com>
To:     Binbin Wu <binbin.wu@linux.intel.com>
Cc:     Paolo Bonzini <pbonzini@redhat.com>,
        Vitaly Kuznetsov <vkuznets@redhat.com>, kvm@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jun 29, 2023, Binbin Wu wrote:
>=20
>=20
> On 2/18/2023 7:10 AM, Sean Christopherson wrote:
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm=
_host.h
> > index 792a6037047a..cd660de02f7b 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -835,6 +835,17 @@ struct kvm_vcpu_arch {
> >   	struct kvm_cpuid_entry2 *cpuid_entries;
> >   	struct kvm_hypervisor_cpuid kvm_cpuid;
> > +	/*
> > +	 * Track whether or not the guest is allowed to use features that are
> > +	 * governed by KVM, where "governed" means KVM needs to manage state
> > +	 * and/or explicitly enable the feature in hardware.  Typically, but
> > +	 * not always, governed features can be used by the guest if and only
> > +	 * if both KVM and userspace want to expose the feature to the guest.
> > +	 */
> > +	struct {
> > +		u32 enabled;
> Although there are some guidances/preconditions of using the framework,
> is it possible that u32 will be ran out quickly after people starts to us=
e
> the framework?

It's definitely possible.  And there's no reason to limit this to a u32, I =
really
have no idea why I did that.=20

Ah, it's because "struct kvm_vcpu_arch" is defined in arch/x86/include/asm/=
kvm_host.h,
and I didn't want to expose governed_features.h in arch/x86/include/asm.  H=
rm,
that's really annoying.

Aha!  A better workaround for that conudrum would be to define an explicit =
"max"
and use that, with a FIXME to call out that this really should use
KVM_NR_GOVERNED_FEATURES directly.  I have aspirations of moving kvm_host.h=
 to
arch/<arch>/kvm, at which point this can be cleaned up by declaring "enum
kvm_governed_features" in kvm_host.h (though it'll likely be named somethin=
g
like kvm_arch.h at that point).

	/*
	 * FIXME: Drop this macro and use KVM_NR_GOVERNED_FEATURES directly
	 * when "struct kvm_vcpu_arch" is no longer defined in an
	 * arch/x86/include/asm header.  The max is mostly arbitrary, i.e.
	 * can be increased as necessary.
	 */
#define KVM_MAX_NR_GOVERNED_FEATURES BITS_PER_LONG

	/*
	 * Track whether or not the guest is allowed to use features that are
	 * governed by KVM, where "governed" means KVM needs to manage state
	 * and/or explicitly enable the feature in hardware.  Typically, but
	 * not always, governed features can be used by the guest if and only
	 * if both KVM and userspace want to expose the feature to the guest.
	 */
	struct {
		DECLARE_BITMAP(enabled, KVM_MAX_NR_GOVERNED_FEATURES);
	} governed_features;


> Of course, I noticed there is build=EF=BF=BD bug check on the length, it =
should be
> OK to increase the length when needed.

> > +static __always_inline int kvm_governed_feature_index(unsigned int x86=
_feature)
> > +{
> > +	switch (x86_feature) {
> > +#define KVM_GOVERNED_FEATURE(x) case x: return KVM_GOVERNED_##x;
> > +#include "governed_features.h"
> > +	default:
> > +		return -1;
> > +	}
> > +}
> > +
> > +static __always_inline int kvm_is_governed_feature(unsigned int x86_fe=
ature)
> Is it better to use bool instead of int?

Yes, this definitely should return a bool.

Thanks!