Received-SPF: pass (google.com: domain of linux-kernel+bounces-17409-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Thu, 4 Jan 2024 18:13:11 -0800
In-Reply-To: <daaf098a6219310b6b1c1e3dc147fbb7e48b6f54.camel@redhat.com>
Precedence: bulk
Mime-Version: 1.0
References: <20231110235528.1561679-1-seanjc@google.com> <20231110235528.1561679-4-seanjc@google.com>
 <3ad69657ba8e1b19d150db574193619cf0cb34df.camel@redhat.com>
 <ZWk8IMZamuemfwXG@google.com> <daaf098a6219310b6b1c1e3dc147fbb7e48b6f54.camel@redhat.com>
Message-ID: <ZZdlt2Dm36VF4WL6@google.com>
Subject: Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest CPUID
From: Sean Christopherson <seanjc@google.com>
To: Maxim Levitsky <mlevitsk@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>, kvm@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="us-ascii"

On Thu, Dec 21, 2023, Maxim Levitsky wrote:
> On Thu, 2023-11-30 at 17:51 -0800, Sean Christopherson wrote:
> > On Sun, Nov 19, 2023, Maxim Levitsky wrote:
> > > On Fri, 2023-11-10 at 15:55 -0800, Sean Christopherson wrote:
> > > > +/*
> > > > + * This isn't truly "unsafe", but all callers except kvm_cpu_after_set_cpuid()
> > > > + * should use __cpuid_entry_get_reg(), which provides compile-time validation
> > > > + * of the input.
> > > > + */
> > > > +static u32 cpuid_get_reg_unsafe(struct kvm_cpuid_entry2 *entry, u32 reg)
> > > > +{
> > > > +	switch (reg) {
> > > > +	case CPUID_EAX:
> > > > +		return entry->eax;
> > > > +	case CPUID_EBX:
> > > > +		return entry->ebx;
> > > > +	case CPUID_ECX:
> > > > +		return entry->ecx;
> > > > +	case CPUID_EDX:
> > > > +		return entry->edx;
> > > > +	default:
> > > > +		WARN_ON_ONCE(1);
> > > > +		return 0;
> > > > +	}
> > > > +}
> > 
> > ...
> > 
> > > >  static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > >  {
> > > >  	struct kvm_lapic *apic = vcpu->arch.apic;
> > > >  	struct kvm_cpuid_entry2 *best;
> > > >  	bool allow_gbpages;
> > > > +	int i;
> > > >  
> > > > -	memset(vcpu->arch.cpu_caps, 0, sizeof(vcpu->arch.cpu_caps));
> > > > +	BUILD_BUG_ON(ARRAY_SIZE(reverse_cpuid) != NR_KVM_CPU_CAPS);
> > > > +
> > > > +	/*
> > > > +	 * Reset guest capabilities to userspace's guest CPUID definition, i.e.
> > > > +	 * honor userspace's definition for features that don't require KVM or
> > > > +	 * hardware management/support (or that KVM simply doesn't care about).
> > > > +	 */
> > > > +	for (i = 0; i < NR_KVM_CPU_CAPS; i++) {
> > > > +		const struct cpuid_reg cpuid = reverse_cpuid[i];
> > > > +
> > > > +		best = kvm_find_cpuid_entry_index(vcpu, cpuid.function, cpuid.index);
> > > > +		if (best)
> > > > +			vcpu->arch.cpu_caps[i] = cpuid_get_reg_unsafe(best, cpuid.reg);
> > > 
> > > Why not just use __cpuid_entry_get_reg? 
> > > 
> > > cpuid.reg comes from read/only 'reverse_cpuid' anyway, and in fact
> > > it seems that all callers of __cpuid_entry_get_reg, take the reg value from
> > > x86_feature_cpuid() which also takes it from 'reverse_cpuid'.
> > > 
> > > So if the compiler is smart enough to not complain in these cases, I don't
> > > see why this case is different.
> > 
> > It's because the input isn't a compile-time constant, and so the BUILD_BUG() in
> > the default path will fire. 
> >  All of the compile-time assertions in reverse_cpuid.h
> > rely on the feature being a constant value, which allows the compiler to optimize
> > away the dead paths, i.e. turn __cpuid_entry_get_reg()'s switch statement into
> > simple pointer arithmetic and thus omit the BUILD_BUG() code.
> 
> In the above code, assuming that the compiler really treats the reverse_cpuid
> as const (if that assumption is not true, then all uses of __cpuid_entry_get_reg
> are also not compile time constant either),

It's not so much the compiler treating something as const as it is the compiler
generating code that resolves the relevant inputs to compile-time constants.

> then the 'reg' value depends only on 'i', and therefore for each iteration of
> the loop, the compiler does know the compile time value of the 'reg', and so
> it can easily prove that 'default' case in __cpuid_entry_get_reg can't be
> reached, and thus eliminate that BUILD_BUG().

A compiler _could_ know, but as above what truly matters is what code the compiler
actually generates.  E.g. all helpers are tagged __always_inline to prevent the
compiler from uninlining the functions (thanks, KASAN), at which point the code
of the non-inline function is no longer dealing with compile-time constants and
so the BUILD_BUG_ON() doesn't get eliminated.

For the loop, while all values are indeed constant, the compiler may choose to
generate a loop instead of unrolling everything.  A sufficiently clever compiler
could still detect that nothing in the loop can hit the "default" case, but in
practice neither clang nor gcc does that level of optimization, at least not yet.

> > > Also why not to initialize guest_caps = host_caps & userspace_cpuid?
> > > 
> > > If this was the default we won't need any guest_cpu_cap_restrict and such,
> > > instead it will just work.
> > 
> > Hrm, I definitely like the idea.  Unfortunately, unless we do an audit of all
> > ~120 uses of guest_cpuid_has(), restricting those based on kvm_cpu_caps might
> > break userspace.
> 
> 120 uses is not that bad, IMHO it is worth it - we won't need to deal with that
> in the future.
> 
> How about a compromise - you change the patches such as it will be possible
> to remove these cases one by one, and also all new cases will be fully
> automatic?

Hrm, I'm not necessarily opposed to that, but I don't think we should go partway
unless we are 100% confident that changing the default to "use guest CPUID ANDed
with KVM capabilities" is the best end state, *and* that someone will actually
have the bandwidth to do the work soon-ish so that KVM isn't in a half-baked
state for months on end.  Even then, my preference would definitely be to switch
everything in one go.

And automatically handling new features would only be feasible for entirely new
leafs.  E.g. X86_FEATURE_RDPID is buried in CPUID.0x7.0x0.ECX, so to automatically
handle new features KVM would need to set the default guest_caps for all bits
*except* RDPID, at which point we're again building a set of features that need
to opt-out.

> > To be fair, the manual lists predate the governed features.
> 
> 100% agree, however the point of governed features was to simplify this list,
> the point of this patch set is to simplify these lists and yet they still remain,
> more or less untouched, and we will still need to maintain them.
> 
> Again I do think that governed features and/or this patchset are better than
> the mess that was there before, but a part of me wants to fully get rid of
> this mess instead of just making it a bit more beautiful. 

Oh, I would love to get rid of the mess too, I _completely_ getting rid of the
mess isn't realistic.  There are guaranteed to be exceptions to the rule, whether
the rule is "use guest CPUID by default" or "use guest CPUID constrained by KVM
capabilities by default".

I.e. there will always be some amount of manual messiness, the question is which
default behavior would yield the smallest mess.  My gut agrees with you, that
defaulting to "guest & KVM" would yield the fewest exceptions.  But as above,
I think we're better off doing the switch as an all-or-nothing things (where "all"
means within a single series, not within a single patch).