Re: [PATCH v1 10/40] x86/cpu: Remove leaf 0x2 parsing loop and add helpers

["Ahmed S. Darwish" <darwi@linutronix.de>]

Hi Ingo,

On Tue, 04 Mar 2025, Ingo Molnar wrote:
> ...
> * Ahmed S. Darwish <darwi@linutronix.de> wrote:
> > +
> > +#include <linux/types.h>
> > +
> > +#include <asm/cpuid.h>
>
> So that header organization is a bit messy: if <asm/cpuid.h> is
> supposed to be the main header, why is there a <asm/cpuid/types.h>?
>
> I'd suggest we follow the FPU header structure:
>
>   starship:~/tip/arch/x86/include/asm/fpu> ls -l
>   total 48
>   -rw-rw-r-- 1 mingo mingo  5732 Feb 27 19:24 api.h
>   -rw-rw-r-- 1 mingo mingo   671 Feb 26 16:13 regset.h
>   -rw-rw-r-- 1 mingo mingo  2203 Feb 27 13:52 sched.h
>   -rw-rw-r-- 1 mingo mingo  1110 Feb 27 19:24 signal.h
>   -rw-rw-r-- 1 mingo mingo 14741 Feb 27 19:24 types.h
>   -rw-rw-r-- 1 mingo mingo   811 Feb 26 16:13 xcr.h
>   -rw-rw-r-- 1 mingo mingo  4401 Feb 27 23:01 xstate.h
>
> With <asm/cpuid/api.h> being the main header - established via a
> separate preparatory patch.
>
> This followup patch can then add <asm/cpuid/types.h> which will also be
> included in <asm/cpuid/api.h>.
>

Sounds sensible.  Thanks!

FYI, in our CPUID-model patch queue (the one after this), we have
something like:

    <asm/cpuid/>
    │
    ├── leaves.h   CPUID bitfields; auto-generated by x86-cpuid-db
    ├── data.h     Internal data structures for the model
    ├── api.h      The new CPUID-model API
    └── ops.h      The raw CPUID ops [Formerly <asm/cpuid.h>]

So doing this from within this PQ should fit nicely.

> > +
> > +/**
> > + * get_leaf_0x2_regs() - Return sanitized leaf 0x2 register output
> > + * @regs:	Output parameter
> > + *
> > + * Get leaf 0x2 register output and store it in @regs.  Invalid byte
> > + * descriptors returned by the hardware will be force set to zero (the
> > + * NULL cache/TLB descriptor) before returning them to the caller.
> > + */
> > +static inline void get_leaf_0x2_regs(union leaf_0x2_regs *regs)
>
> Please prefix all new cpuid API functions and types with cpuid_.
>

ACK.

> > +#define for_each_leaf_0x2_desc(regs, desc)				\
> > +	/* Skip the first byte as it is not a descriptor */		\
> > +	for (desc = &(regs).desc[1]; desc < &(regs).desc[16]; desc++)
>
> The comment line can come before the macro.
>

ACK.

> > +	get_leaf_0x2_regs(&regs);
> > +	for_each_leaf_0x2_desc(regs, desc)
> > +		intel_tlb_lookup(*desc);
>
> Nice interface otherwise.
>

Thanks!
Ahmed