Re: [PATCH 1/6] x86/cpu-policy: Drop build time cross-checks of featureset sizes

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.05.2023 17:06, Andrew Cooper wrote:
> On 09/05/2023 5:15 pm, Jan Beulich wrote:
>> On 09.05.2023 17:59, Andrew Cooper wrote:
>>> On 09/05/2023 3:28 pm, Jan Beulich wrote:
>>>> On 09.05.2023 15:04, Andrew Cooper wrote:
>>>>> On 08/05/2023 7:47 am, Jan Beulich wrote:
>>>>>> On 04.05.2023 21:39, Andrew Cooper wrote:
>>>>>>> These BUILD_BUG_ON()s exist to cover the curious absence of a 
>>>>>>> diagnostic for
>>>>>>> code which looks like:
>>>>>>>
>>>>>>>   uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>
>>>>>>> However, GCC 12 at least does now warn for this:
>>>>>>>
>>>>>>>   foo.c:1:24: error: excess elements in array initializer [-Werror]
>>>>>>>     884 | uint32_t foo[1] = { 1, 2, 3 };
>>>>>>>         |                        ^
>>>>>>>   foo.c:1:24: note: (near initialization for 'foo')
>>>>>> I'm pretty sure all gcc versions we support diagnose such cases. In turn
>>>>>> the arrays in question don't have explicit dimensions at their
>>>>>> definition sites, and hence they derive their dimensions from their
>>>>>> initializers. So the build-time-checks are about the arrays in fact
>>>>>> obtaining the right dimensions, i.e. the initializers being suitable.
>>>>>>
>>>>>> With the core part of the reasoning not being applicable, I'm afraid I
>>>>>> can't even say "okay with an adjusted description".
>>>>> Now I'm extra confused.
>>>>>
>>>>> I put those BUILD_BUG_ON()'s in because I was not getting a diagnostic
>>>>> when I was expecting one, and there was a bug in the original featureset
>>>>> work caused by this going wrong.
>>>>>
>>>>> But godbolt seems to agree that even GCC 4.1 notices.
>>>>>
>>>>> Maybe it was some other error (C file not seeing the header properly?)
>>>>> which disappeared across the upstream review?
>>>> Or maybe, by mistake, too few initializer fields? But what exactly it
>>>> was probably doesn't matter. If this patch is to stay (see below), some
>>>> different description will be needed anyway (or the change be folded
>>>> into the one actually invalidating those BUILD_BUG_ON()s).
>>>>
>>>>> Either way, these aren't appropriate, and need deleting before patch 5,
>>>>> because the check is no longer valid when a featureset can be longer
>>>>> than the autogen length.
>>>> Well, they need deleting if we stick to the approach chosen there right
>>>> now. If we switched to my proposed alternative, they better would stay.
>>> Given that all versions of GCC do warn, I don't see any justification
>>> for them to stay.
>> All versions warn when the variable declarations / definitions have a
>> dimension specified, and then there are excess initializers. Yet none
>> of the five affected arrays have a dimension specified in their
>> definitions.
>>
>> Even if dimensions were added, we'd then have only covered half of
>> what the BUILD_BUG_ON()s cover right now: There could then be fewer
>> than intended initializer fields, and things may still be screwed. I
>> think it was for this very reason why BUILD_BUG_ON() was chosen.
> 
> ???
> 
> The dimensions already exist, as proved by the fact GCC can spot the
> violation.

Where? Quoting cpu-policy.c:

const uint32_t known_features[] = INIT_KNOWN_FEATURES;

static const uint32_t __initconst pv_max_featuremask[] = INIT_PV_MAX_FEATURES;
static const uint32_t hvm_shadow_max_featuremask[] = 
INIT_HVM_SHADOW_MAX_FEATURES;
static const uint32_t __initconst hvm_hap_max_featuremask[] =
    INIT_HVM_HAP_MAX_FEATURES;
static const uint32_t __initconst pv_def_featuremask[] = INIT_PV_DEF_FEATURES;
static const uint32_t __initconst hvm_shadow_def_featuremask[] =
    INIT_HVM_SHADOW_DEF_FEATURES;
static const uint32_t __initconst hvm_hap_def_featuremask[] =
    INIT_HVM_HAP_DEF_FEATURES;
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;

I notice that known_features[], as an exception, has its dimension declared
in cpuid.h.

> On the other hand, zero extending a featureset is explicitly how they're
> supposed to work.  How do you think Xapi has coped with migration
> compatibility over the years, not to mention the microcode changes which
> lengthen a featureset?
> 
> So no, there was never any problem with constructs of the form uint32_t
> foo[10] = { 1, } in the first place.
> 
> The BUILD_BUG_ON()s therefore serve no purpose at all.

As per above the very minimum would be to accompany their dropping with
adding of explicitly specified dimensions for all the static arrays. I'm
not entirely certain about the other side (the zero-extension), but I'd
likely end up simply trusting you on that.

Jan