Re: [PATCH 5/6] x86/cpu-policy: Disentangle X86_NR_FEAT and FEATURESET_NR_ENTRIES

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.05.2023 21:39, Andrew Cooper wrote:
> When adding new words to a featureset, there is a reasonable amount of
> boilerplate and it is preforable to split the addition into multiple patches.
> 
> GCC 12 spotted a real (transient) error which occurs when splitting additions
> like this.  Right now, FEATURESET_NR_ENTRIES is dynamically generated from the
> highest numeric XEN_CPUFEATURE() value, and can be less than what the
> FEATURESET_* constants suggest the length of a featureset bitmap ought to be.
> 
> This causes the policy <-> featureset converters to genuinely access
> out-of-bounds on the featureset array.
> 
> Rework X86_NR_FEAT to be related to FEATURESET_* alone, allowing it
> specifically to grow larger than FEATURESET_NR_ENTRIES.
> 
> Reported-by: Jan Beulich <jbeulich@xxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

While, like you, I could live with the previous patch even if I don't
particularly like it, I'm not convinced of the route you take here. Can't
we instead improve build-time checking, so the issue spotted late in the
build by gcc12 can be spotted earlier and/or be connected better to the
underlying reason?

One idea I have would deal with another aspect I don't like about our
present XEN_CPUFEATURE() as well: The *32 that's there in every use of
the macro. How about

XEN_CPUFEATURE(FSRCS,        10, 12) /*A  Fast Short REP CMPSB/SCASB */

as the common use and e.g.

XEN_CPUFEATURE(16)

or (if that ends up easier in gen-cpuid-py and/or the public header)
something like

XEN_CPUFEATURE(, 16, )

as the placeholder required for (at least trailing) unpopulated slots? Of
course the macro used may also be one of a different name, which may even
be necessary to keep the public header reasonably simple; maybe as much
as avoiding use of compiler extensions there. (This would then mean to
leave alone XEN_CPUFEATURE(), and my secondary adjustment would perhaps
become an independent change to make.)

> To preempt what I expect will be the first review question, no FEATURESET_*
> can't become an enumeration, because the constants undergo token concatination
> in the preprocess as part of making DECL_BITFIELD() work.

Just as a remark: I had trouble understanding this. Which was a result of
you referring to token concatenation being the problem (which is fine when
the results are enumerators), when really the issue is with the result of
the concatenation wanting to be expanded to a literal number.

Then again - do CPUID_BITFIELD_<n> really need to be named that way?
Couldn't they equally well be CPUID_BITFIELD_1d, CPUID_BITFIELD_e1c, and
alike, thus removing the need for intermediate macro expansion?

Jan