Message-ID: <aKMvTrrKYgJNWX8L@lx-t490>
Date: Mon, 18 Aug 2025 15:49:02 +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 Fri, 15 Aug 2025, Sean Christopherson wrote:
>
> IMO, the end result is quite misleading for leaves with "repeated" entries.  0xd
> in particular will be bizarre due to its array starting at ECX=2.  E.g.
> cpuid_subleaf_index() straight up won't work (without lying) due to hardcoding
> '0' as the subleaf.
>
> #define cpuid_subleaf_index(_cpuinfo, _leaf, _idx)			\
> ({									\
> 	__cpuid_assert_leaf_has_dynamic_subleaves(_cpuinfo, _leaf);	\
> 	__cpuid_table_subleaf_idx(&(_cpuinfo)->cpuid, _leaf, 0, _idx);	\
>                                                              ^
>                                                              |
>
> And then the usage would be similarly bizarre, e.g.
>
> 	for (i = XFEATURE_YMM; i < ARRAY_SIZE(xstate_sizes); i++) {
> 		struct cpuid_xstate_sizes *xs = &xstate_sizes[i];
> 		struct cpuid_0xd_2 *c = cpuid_subleaf_index(..., 0xD, i - 2);
>
> 		...
> 	}
>
> Even the cases where the array starts at '0' look weird:
>
> 	const struct leaf_0x4_0 *regs = cpuid_subleaf_index(c, 0x4, index);
>
> because the code is obviously not grabbing CPUID(0x4).0.
>
> And the subleaf_index naming is also weird, because they're essentially the same
> thing, e.g. the SDM refers to "sub-leaf index" for more than just the repeated
> cases.
>
> Rather than define the structures names using an explicit starting subleaf, what
> if the structures and APIs explicitly reference 'n' as the subleaf?  That would
> communicate that the struct represents a repeated subleaf, explicitly tie the API
> to that structure, and would provide macro/function names that don't make the
> reader tease out the subtle usage of "index".
>
> And then instead of just the array size, capture the start:end of the repeated
> subleaf so that the caller doesn't need to manually do the math.
>
> E.g.
>
> 	const struct leaf_0x4_n *regs = cpuid_subleaf_n(c, 0x4, index);
>
> 	struct cpuid_0xd_n *c = cpuid_subleaf_n(..., 0xD, i);
>

Thanks a lot for all these remarks.  I was indeed struggling with good
names for the array indexing case, so the above was really helpful.

In this v4 iteration, I did not add a CPUID parser API for the cases
where the subleaves are repeated but do not start from zero.  Not for any
technical reason, but to avoid having kernel APIs with no call sites.

What I internally have is:

  /**
   * __cpuid_subleaf_index() - Access parsed CPUID data at runtime subleaf index
   * @_cpuinfo:	CPU capability structure reference ('struct cpuinfo_x86')
   * @_leaf:	CPUID leaf in compile-time 0xN format; e.g. 0x4, 0x8000001d
   * @_subleaf:	CPUID subleaf in compile-time decimal format; e.g. 0, 1, 3
   * @_idx:	Index within CPUID(@_leaf).@...bleaf output storage array.
   *		Unlike @_leaf and @_subleaf, this index value can be provided
   *		dynamically.
   * ...
   */
  #define __cpuid_subleaf_index(_cpuinfo, _leaf, _subleaf, @_idx)	\
  ...

Thus, the arch/x86/kvm/cpuid.c loop you quoted would be tokenized as:

    const struct leaf_0xd_2 *ld;
    for (int i = 0; i < ARRAY_SIZE(xstate_sizes) - XFEATURE_YMM; i++) {
        ...
        ld = __cpuid_subleaf_index(c, 0xd, 2, i);
	                           |   |   |  └───────┐
                                   |   |   └────────┐ |
                                   |   └─────────┐  | |
                                   *             *  * v
        ld =                      &c.cpuid.leaf_0xd_2[i];
        ...
    }

I actually agree that, in this case, types like "struct leaf_0xd_2" are
deceiving... Let's divide the problem in two.

Easy case: Subleaves start repeating from subleaf = 0
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This would be the CPUID leaves:

    x86-cpuid-db/db/xml (tip)> git grep 'id="0" array='
    leaf_04.xml:        <subleaf id="0" array="32">
    leaf_0b.xml:        <subleaf id="0" array="2">
    leaf_18.xml:        <subleaf id="0" array="32">
    leaf_1b.xml:        <subleaf id="0" array="32">
    leaf_1f.xml:        <subleaf id="0" array="6">
    leaf_8000001d.xml:	<subleaf id="0" array="32">
    leaf_80000026.xml:	<subleaf id="0" array="4">

For example, patch 24/34 ("x86/cacheinfo: Use parsed CPUID(0x4)"):

    static int
    intel_fill_cpuid4_info(struct cpuinfo_x86 *c, int index, ...)
    {
	const struct leaf_0x4_0 *regs = cpuid_subleaf_index(c, 0x4, index);
	...
    }

In that case, we might actually generate leaf data types in the form:

    struct leaf_0x4_n { ... };

And have the access macros called cpuid_subleaf_n():

    static int
    intel_fill_cpuid4_info(struct cpuinfo_x86 *c, int index, ...) {
    {
	const struct leaf_0x4_n *regs = cpuid_subleaf_n(c, 0x4, index);
	...
    }

I think this indeed looks much much better.

I will do that for v5 of the model.

Hard case: Subleaves start repeating from subleaf > 0
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This would be the CPUID leaves:

    x86-cpuid-db/db/xml (tip)> git grep 'id="[1-9][0-9]*" array='

    leaf_0d.xml:    <subleaf id="2" array="62">
    leaf_10.xml:    <subleaf id="1" array="2">
    leaf_12.xml:    <subleaf id="2" array="30">
    leaf_17.xml:    <subleaf id="1" array="3">

For something like CPUID(0xd), I cannot just blindly define a 'struct
cpuid_0xd_n' data type.  We already have:

    struct leaf_0xd_0 { ... };
    struct leaf_0xd_1 { ... };
    struct leaf_0xd_2 { ... };

and they all have different bitfields.  A similar case exist for
CPUID(0x10), CPUID(0x12), and CPUID(0x17).

But, we can still have:

    struct leaf_0xd_0	{ ... };
    struct leaf_0xd_1	{ ... };
    struct leaf_0xd_2_n	{ ... };

With this, the CPUID(0x12) call site at kernel/cpu/sgx/main.c can change
from:

    u32 eax, ebx, ecx, edx;

    for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
        ...
	cpuid_count(SGX_CPUID, i + SGX_CPUID_EPC, &eax, ...);
    }

to:

    const struct leaf_0x12_2_n *l;

    for (i = 0; i < ARRAY_SIZE(sgx_epc_sections); i++) {
        ...
	l = __cpuid_subleaf_n(0x12, 2, i);
    }

And the aforementioned KVM snippet would be:

    const struct leaf_0xd_2_n *l;

    for (int i = 0; i < ARRAY_SIZE(xstate_sizes) - XFEATURE_YMM; i++) {
        l = __cpuid_subleaf_n(c, 0xd, 2, i);
    }

I'm open for better names than __cpuid_subleaf_n() for this.  The only
constraint with any suggested name is that I need to have 0xd and 2 both
passed as CPP literals.

API Summary
~~~~~~~~~~~

For CPUID leaves with static subleaves:

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

For CPUID leaves with dynamic subleaves:

    cpuid_subleaf_n(_cpuinfo, _leaf, _idx)
    __cpuid_subleaf_n(_cpuinfo, _leaf, _subleaf, _idx)

Sounds good?

>
> Tangentially related, having to manually specific count=1 to CPUID_LEAF() for
> the common case is also ugly.  If a dedicated CPUID_LEAF_N() macro is added to
> specificy the start:end of the range, then CPUID_LEAF() can just hardcode the
> count to '1'.  E.g.
>
> struct cpuid_leaves {
> 	CPUID_LEAF(0x0,		 	0);
> 	CPUID_LEAF(0x1,			0);
> 	CPUID_LEAF(0x2,		 	0);
> 	CPUID_LEAF_N(0x4,		0,	7);
> 	CPUID_LEAF(0xd,			0);
> 	CPUID_LEAF(0xd,			1);
> 	CPUID_LEAF_N(0xd,		2,	63);
> 	CPUID_LEAF(0x16,	 	0);
> 	CPUID_LEAF(0x80000000,		0);
> 	CPUID_LEAF(0x80000005,		0);
> 	CPUID_LEAF(0x80000006,		0);
> 	CPUID_LEAF_N(0x8000001d,	0,	7);
> };
>

This is much better; will do.

Thanks!

--
Ahmed S. Darwish
Linutronix GmbH

Powered by blists - more mailing lists