linux-kernel - Re: [PATCH v2 48/49] KVM: x86: Add a macro for features that are synthesized into boot_cpu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Zo2n9VQ3nBuf1d3F@google.com>
Date: Tue, 9 Jul 2024 14:13:25 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: Maxim Levitsky <mlevitsk@...hat.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 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,
  }

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, then I'm definitely open to exploring alternative
formatting options.  But that's largely orthogonal to using macros instead of
individual function calls.