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: <cbcb80ee5be13d78390ff6f4a1a3c58fc849e311.camel@redhat.com>
Date: Thu, 21 Nov 2024 22:17:57 -0500
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 22/49] KVM: x86: Add a macro to precisely handle
 aliased 0x1.EDX CPUID features

On Wed, 2024-09-11 at 08:37 -0700, Sean Christopherson wrote:
> 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.
> 

Hi!

I am stating my point again: Treating 8000_0001 leaf aliases as regular CPUID features means that
we don't need common code to deal with this, and thus when someone reads the common code
(and this is the thing I care about the most) that someone won't need to dig up the info
about what these aliases are. 

I for example didn't knew about them because these aliases are basically a result of AMD redoing 
some things in the spec their way when they just released first 64-bit extensions.
I didn't follow the x86 ISA closely back then (I only had 32 bit systems to play with).

Best regards,
	Maxim Levitsky