Message-ID: <5d4d3eb81170ccf31f41a672121670ae4194b80a.camel@redhat.com>
Date: Wed, 24 Jul 2024 14:04:56 -0400
From: Maxim Levitsky <mlevitsk@...hat.com>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Paolo Bonzini <pbonzini@...hat.com>, Vitaly Kuznetsov
 <vkuznets@...hat.com>,  kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 Hou Wenlong <houwenlong.hwl@...group.com>, Kechen Lu <kechenl@...dia.com>,
 Oliver Upton <oliver.upton@...ux.dev>, Binbin Wu
 <binbin.wu@...ux.intel.com>, Yang Weijiang <weijiang.yang@...el.com>,
 Robert Hoo <robert.hoo.linux@...il.com>
Subject: Re: [PATCH v2 48/49] KVM: x86: Add a macro for features that are
 synthesized into boot_cpu_data

On Tue, 2024-07-09 at 14:13 -0700, Sean Christopherson wrote:
> On Thu, Jul 04, 2024, Maxim Levitsky wrote:
> > On Fri, 2024-05-17 at 10:39 -0700, Sean Christopherson wrote:
> > > Add yet another CPUID macro, this time for features that the host kernel
> > > synthesizes into boot_cpu_data, i.e. that the kernel force sets even in
> > > situations where the feature isn't reported by CPUID.  Thanks to the
> > > macro shenanigans of kvm_cpu_cap_init(), such features can now be handled
> > > in the core CPUID framework, i.e. don't need to be handled out-of-band and
> > > thus without as many guardrails.
> > > 
> > > Adding a dedicated macro also helps document what's going on, e.g. the
> > > calls to kvm_cpu_cap_check_and_set() are very confusing unless the reader
> > > knows exactly how kvm_cpu_cap_init() generates kvm_cpu_caps (and even
> > > then, it's far from obvious).
> > > 
> > > Signed-off-by: Sean Christopherson <seanjc@...gle.com>
> > > ---
> 
> ...
> 
> > Now that you added the final F_* macro, let's list all of them:
> > 
> > #define F(name)							\
> > 
> > /* Scattered Flag - For features that are scattered by cpufeatures.h. */
> > #define SF(name)						\
> > 
> > /* Features that KVM supports only on 64-bit kernels. */
> > #define X86_64_F(name)						\
> > 
> > /*
> >  * Raw Feature - For features that KVM supports based purely on raw host CPUID,
> >  * i.e. that KVM virtualizes even if the host kernel doesn't use the feature.
> >  * Simply force set the feature in KVM's capabilities, raw CPUID support will
> >  * be factored in by __kvm_cpu_cap_mask().
> >  */
> > #define RAW_F(name)						\
> > 
> > /*
> >  * Emulated Feature - For features that KVM emulates in software irrespective
> >  * of host CPU/kernel support.
> >  */
> > #define EMUL_F(name)						\
> > 
> > /*
> >  * Synthesized Feature - For features that are synthesized into boot_cpu_data,
> >  * i.e. may not be present in the raw CPUID, but can still be advertised to
> >  * userspace.  Primarily used for mitigation related feature flags.
> >  */
> > #define SYN_F(name)						\
> > 
> > /*
> >  * Aliased Features - For features in 0x8000_0001.EDX that are duplicates of
> >  * identical 0x1.EDX features, and thus are aliased from 0x1 to 0x8000_0001.
> >  */
> > #define AF(name)								\
> > 
> > /*
> >  * VMM Features - For features that KVM "supports" in some capacity, i.e. that
> >  * KVM may query, but that are never advertised to userspace.  E.g. KVM allows
> >  * userspace to enumerate MONITOR+MWAIT support to the guest, but the MWAIT
> >  * feature flag is never advertised to userspace because MONITOR+MWAIT aren't
> >  * virtualized by hardware, can't be faithfully emulated in software (KVM
> >  * emulates them as NOPs), and allowing the guest to execute them natively
> >  * requires enabling a per-VM capability.
> >  */
> > #define VMM_F(name)								\
> > 
> > 
> > Honestly, I already somewhat lost in what each of those macros means even
> > when reading the comments, which might indicate that a future reader might
> > also have a hard time understanding those.
> > 
> > I now support even more the case of setting each feature bit in a separate
> > statement as I explained in an earlier patch.
> > 
> > What do you think?
> 
> I completely agree that there are an absurd number of flavors of features, but
> I don't see how using separate statement eliminates any of that complexity.  The
> complexity comes from the fact that KVM actually has that many different ways and
> combinations for advertising and enumerating CPUID-based features.
> 
> Ignoring for the moment that "vmm" and "aliased" could be avoided for any approach,
> if we go with statements, we'll still have
> 
>   kvm_cpu_cap_init{,passthrough,emulated,synthesized,aliased,vmm,only64}()
> 
> or if the flavor is an input/enum,
> 
>   enum kvm_cpuid_feature_type {
>   	NORMAL,
> 	PASSTHROUGH,
> 	EMULATED,
> 	SYNTHESIZED,
> 	ALIASED,
> 	VMM,
> 	ONLY_64,
>   }

It doesn't have to be like that - something more compact can be done,
plus bitmask of various flags can be used.

> 
> I.e. we'll still need the same functionality and comments, it would simply be
> dressed up differently.

> 
> If the underlying concern is that the macro names are too terse, and/or getting
> one feature per line is desirable, 

I indeed have these concerns and more:

These are my concerns

1. Macro names are indeed too terse, and hard to figure out, even after looking
at the macro source.
This wasn't a problem before this patch series.

2. One feature per line would be very nice, it is much more readable, especially
when features have various 'modifiers'.
This wasn't such a problem before this patch series, because we just had features 'or'ed,
but having one feature per line would be a good thing to have even before this patch series.

3. Feature bitmap 'or'ing of macro's output after this patch series became very confusing, 
now that macros do various side things.

In fact VMM_F confuses the user even more, because it doesn't even contribute to the
feature mask at all.

It was OK before the patch series.

Technically of course I am not opposed to have the 'kvm_cpu_cap_init' or whatever we name
it, to remain a macro, it is probably even desirable to have it as a macro, but it is OK,
as long as it is just a macro which doesn't evaluate to anything and thus looks
like a function call.

Best regards,
	Maxim Levitsky


> then I'm definitely open to exploring alternative
> formatting options.  But that's largely orthogonal to using macros instead of
> individual function calls.
> 

Powered by blists - more mailing lists