Message-ID: <aKR3zjxsM7xQZ_g8@lx-t490>
Date: Tue, 19 Aug 2025 15:10:38 +0200
From: "Ahmed S. Darwish" <darwi@...utronix.de>
To: Sean Christopherson <seanjc@...gle.com>
Cc: Borislav Petkov <bp@...en8.de>, Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Andrew Cooper <andrew.cooper3@...rix.com>,
	"H. Peter Anvin" <hpa@...or.com>,
	David Woodhouse <dwmw2@...radead.org>,
	Peter Zijlstra <peterz@...radead.org>,
	Sohil Mehta <sohil.mehta@...el.com>,
	John Ogness <john.ogness@...utronix.de>, x86@...nel.org,
	x86-cpuid@...ts.linux.dev, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 07/34] x86/cpuid: Introduce a centralized CPUID data
 model

Hi,

On Mon, 18 Aug 2025, Sean Christopherson wrote:
>
> Not sure "cleaner" is the right word, but if you really want to add
> compile-time sanity checks, you could put the actual definitions in a
> dedicated header file that's included multiple times without any #ifdef
> guards.  Once to define "struct cpuid_leaves", and a second time to
> define global metadata for each leaf, e.g. the first/last subleaf in a
> dynamic range.
>
...
> @@ -432,7 +432,10 @@ static inline u32 cpuid_base_hypervisor(const char *sig, u32 leaves)
>  #define cpuid_subleaf_index(_cpuinfo, _leaf, _idx)			\
>  ({									\
>  	__cpuid_assert_leaf_has_dynamic_subleaves(_cpuinfo, _leaf);	\
> -	__cpuid_table_subleaf_idx(&(_cpuinfo)->cpuid, _leaf, 0, _idx);	\
> +	BUILD_BUG_ON(__builtin_constant_p(_idx) &&			\
> +		     ((_idx) < CPUID_LEAF_ ## _leaf ## _N_FIRST ||	\
> +		      (_idx) > CPUID_LEAF_ ## _leaf ## _N_LAST));	\
> +	__cpuid_table_subleaf_idx(&(_cpuinfo)->cpuid, _leaf, n, _idx);	\
>  })
>
...
> -#define CPUID_LEAF(_leaf, _subleaf, _count)			\
> -	__CPUID_LEAF(leaf_ ## _leaf ## _ ## _subleaf, _count)
> +#define CPUID_LEAF(_leaf, _subleaf)			\
> +	__CPUID_LEAF(leaf_ ## _leaf ## _ ## _subleaf, 1)
> +
> +#define CPUID_LEAF_N(_leaf, _first, _last)		\
> +	__CPUID_LEAF(leaf_ ## _leaf ## _n, _last - _first + 1)
> +
> +
>
...
> +
> +#undef CPUID_LEAF
> +#undef CPUID_LEAF_N
> +#define CPUID_LEAF(_leaf, _subleaf)
> +#define CPUID_LEAF_N(_leaf, _first, _last)		\
> +	CPUID_LEAF_ ## _leaf ## _N_FIRST = _first,	\
> +	CPUID_LEAF_ ## _leaf ## _N_LAST  = _last,
> +
> +enum cpuid_dynamic_leaf_ranges {
> +#include "leaf_defs.h"
>  };
...

Thanks a lot for jumping-in and posting code on top; really appreciated!

Yes, I'm doing a new 'x86-cpuid-db' release now.

It will generate <asm/cpuid/api/leaf_types.h> in below form; showing only
the dynamic CPUID leaves here:

    struct leaf_0x4_n { ... };

    #define LEAF_0x4_SUBLEAF_N_FIRST		0
    #define LEAF_0x4_SUBLEAF_N_LAST		31

    struct leaf_0xb_n { ... };

    #define LEAF_0xb_SUBLEAF_N_FIRST		0
    #define LEAF_0xb_SUBLEAF_N_LAST		1

    struct leaf_0xd_n { ... };

    #define LEAF_0xd_SUBLEAF_N_FIRST		2
    #define LEAF_0xd_SUBLEAF_N_LAST		62

    struct leaf_0x10_n { ... };

    #define LEAF_0x10_SUBLEAF_N_FIRST		1
    #define LEAF_0x10_SUBLEAF_N_LAST		2

    struct leaf_0x12_n { ... };

    #define LEAF_0x12_SUBLEAF_N_FIRST		2
    #define LEAF_0x12_SUBLEAF_N_LAST		9

    // ...

    struct leaf_0x8000001d_n { ... };

    #define LEAF_0x8000001d_SUBLEAF_N_FIRST	0
    #define LEAF_0x8000001d_SUBLEAF_N_LAST	31

    struct leaf_0x80000026_n { ... };

    #define LEAF_0x80000026_SUBLEAF_N_FIRST	0
    #define LEAF_0x80000026_SUBLEAF_N_LAST	3

IMO, these are a clean set of symbols.  They are in the same header file
because Ingo would like to keep the header structure simple (and 100%
agreement from my side; especially not to hit any ugly generated files
version mismatches later).  That is:

    arch/x86/include/asm/cpuid/
    ├── api.h
    ├── leaf_types.h				// One auto-generated file
    └── types.h

The _SUBLEAF_N_FIRST/LAST symbols reflect below CPUID XML database lines:

    leaf_04.xml:        <subleaf id="0" last="31">
    leaf_0b.xml:        <subleaf id="0" last="1">
    leaf_0d.xml:        <subleaf id="2" last="62">
    leaf_10.xml:        <subleaf id="1" last="2">
    leaf_12.xml:        <subleaf id="2" last="9">
    ...
    leaf_8000001d.xml:	<subleaf id="0" last="31">
    leaf_80000026.xml:	<subleaf id="0" last="3">

In current release, the last="" attribute is called array="", but that's
an implementation detail. The new attribute name is much more clear
(thanks for the _FIRST/_LAST suggestion); it fits current semantics.

Then, everything else, like the snippet you posted (slightly adapted):

> +	BUILD_BUG_ON(__builtin_constant_p(_idx) &&			\
> +		     ((_idx) < LEAF_ ## _leaf ## SUBLEAF_N_FIRST ||	\
> +		      (_idx) > LEAF_ ## _leaf ## SUBLEAF__N_LAST));	\

would work perfectly out of the box.

To give freedom to the Linux Kernel code though, the kernel's own per-CPU
CPUID tables can have storage areas less than

    LEAF_M_SUBLEAF_N_LAST - LEAF_M_SUBLEAF_N_FIST + 1

For example:

    struct leaf_0x0_0           leaf_0x0_0[1];
    ...
    struct leaf_0x4_0           leaf_0x4_0[8];		// 8 < 32
    ...
    struct leaf_0x8000001d_0    leaf_0x8000001d_0[8];	// 8 < 32

That should be OK, and is by design.  The BUILD_BUG_ON() snippet above
can easily be adapted with a ARRAY_SIZE().

After all that (which is not much extra atually, thanks to the XML symbol
generation), we can have pretty nice compile- and run-time safety for the
new API:

    cpuid_subleaf_n(_cpuinfo, _leaf, _idx)

The other CPUID parser APIs:

    cpuid_leaf(_cpuinfo, _leaf)
    cpuid_subleaf(_cpuinfo, _leaf, _subleaf)

already have total compile-time safety due to the zero-casting.

This is all is pretty neat.  Thanks again!

Kind regards,
Ahmed

Powered by blists - more mailing lists