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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 08/05/2023 8:45 am, Jan Beulich wrote:
> 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.

It's the route you tentatively agreed to in
https://lore.kernel.org/xen-devel/a282c338-98ab-6c3f-314b-267a5a82bad1@xxxxxxxx/

> 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?

I don't understand what you mean by this.  For the transient period of
time, Xen's idea of a featureset *is* longer the autogen idea, hence the
work in this patch to decouple the two.

>
> 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.)

Honestly, I don't want to hide the *32 part of the expression.  This
logic is already magic enough.

If we were to do something like this, I don't see what's wrong with just
having the value as a regular define at the end anyway.

One way or another with this approach, something needs updating in the
tail of cpufeatureset.h, and gen-cpuid.py can easily parse for a
specific named constant, and it will be less magic than overloading
XEN_CPUFEATURE().

>> 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?

gen-cpuid.py doesn't know the short names; only Xen does, which is why
the expansion needs to know the name->word mapping.

I suppose this can be fixed, but it will require more magic comments and
more parsing to achieve.

~Andrew