linux-kernel - Re: [PATCH v2 22/49] KVM: x86: Add a macro to precisely handle aliased 0x1.EDX CPUID features

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: <ZuG5ULBjfQ3hv_Jb@google.com>
Date: Wed, 11 Sep 2024 08:37:52 -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 22/49] KVM: x86: Add a macro to precisely handle
 aliased 0x1.EDX CPUID features

On Tue, Sep 10, 2024, Maxim Levitsky wrote:
> On Mon, 2024-08-05 at 15:00 -0700, Sean Christopherson wrote:
> > If we go with ALIASED_F() (or ALIASED_8000_0001_F()), then that macro is all that
> > is needed, and it's bulletproof.  E.g. there is no KVM_X86_FEATURE_FPU_ALIAS that
> > can be queried, and thus no need to be ensure it's defined in cpuid.c and #undef'd
> > after its use.
> > 
> > Hmm, I supposed we could harden the aliased feature usage in the same way as the
> > ALIASED_F(), e.g.
> > 
> >   #define __X86_FEATURE_8000_0001_ALIAS(feature)				\
> >   ({										\
> > 	BUILD_BUG_ON(__feature_leaf(X86_FEATURE_##name) != CPUID_1_EDX);	\
> > 	BUILD_BUG_ON(kvm_cpu_cap_init_in_progress != CPUID_8000_0001_EDX);	\
> > 	(feature + (CPUID_8000_0001_EDX - CPUID_1_EDX) * 32);			\
> >   })
> > 
> > If something tries to use an X86_FEATURE_*_ALIAS outside if kvm_cpu_cap_init(),
> > it would need to define and set kvm_cpu_cap_init_in_progress, i.e. would really
> > have to try to mess up.
> > 
> > Effectively the only differences are that KVM would have ~10 or so more lines of
> > code to define the X86_FEATURE_*_ALIAS macros, and that the usage would look like:
> > 
> > 	VIRTUALIZED_F(FPU_ALIAS)
> > 
> > versus
> > 
> > 	ALIASED_F(FPU)
> 
> 
> This is exactly my point. I want to avoid profiliation of the _F macros, because
> later, we will need to figure out what each of them (e.g ALIASED_F) does.
> 
> A whole leaf alias, is once in x86 arch life misfeature, and it is very likely that
> Intel/AMD won't add more such aliases.
> 
> Why VIRTUALIZED_F though, it wasn't in the patch series? Normal F() should be enough
> IMHO.

I'm a-ok with F(), I simply thought there was a desire for more verbosity across
the board.

> > At that point, I'm ok with defining each alias, though I honestly still don't
> > understand the motivation for defining single-use macros.
> > 
> 
> The idea is that nobody will need to look at these macros
> (e.g__X86_FEATURE_8000_0001_ALIAS() and its usages), because it's clear what
> they do, they just define few extra CPUID features that nobody really cares
> about.
> 
> ALIASED_F() on the other hand is yet another _F macro() and we will need,
> once again and again to figure out why it is there, what it does, etc.

That seems easily solved by naming the macro ALIASED_8000_0001_F().  I don't see
how that's any less clear than __X86_FEATURE_8000_0001_ALIAS(), and as above,
there are several advantages to defining the alias in the context of the leaf
builder.