Date: Thu, 11 Jan 2024 16:44:43 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/9] KVM: x86: Initialize guest cpu_caps based on guest CPUID

On Thu, Jan 04, 2024, Sean Christopherson wrote:
> 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:
 
> > > > 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).

Ok, the idea of having vcpu->arch.cpu_caps default to a KVM & GUEST is growing
on me.  There's a lurking bug in KVM that in some ways is due to lack of a per-vCPU,
KVM-enforced set of a features.  The bug is relatively benign (VMX passes through
CR4.FSGSBASE when it's not supported in the host), and easy to fix (incorporate
KVM-reserved CR4 bits into vcpu->arch.cr4_guest_rsvd_bits), but it really is
something that just shouldn't happen.  E.g. KVM's handling of EFER has a similar
lurking problem where __kvm_valid_efer() is unsafe to use without also consulting
efer_reserved_bits.

And after digging a bit more, I think I'm just being overly paranoid.  I'm fairly
certain the only exceptions are literally the few that I've called out (RDPID,
MOVBE, and MWAIT (which is only a problem because of a stupid quirk)).  I don't
yet have a firm plan on how to deal with the exceptions in a clean way, e.g. I'd
like to somehow have the "opt-out" code share the set of emulated features with
__do_cpuid_func_emulated().  One thought would be to add kvm_emulated_cpu_caps,
which would be *comically* wasteful, but might be worth the 90 bytes.

For v2, what if I post this more or less as-is, with a "convert to KVM & GUEST"
patch thrown in at the end as an RFC?  I want to do a lot more testing (and staring)
before committing to the conversion, and sadly I don't have anywhere near enough
cycles to do that right now.

Powered by blists - more mailing lists