Re: [PATCH 01/12] KVM: x86: Add a framework for enabling KVM-governed x86 features

[Sean Christopherson <seanjc@google.com>]

On Thu, Jun 29, 2023, Binbin Wu wrote:
> 
> 
> 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 use
> 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. 

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.  Hrm,
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 something
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� 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_feature)
> Is it better to use bool instead of int?

Yes, this definitely should return a bool.

Thanks!