Re: [PATCH v2 08/30] xen/x86: Mask out unknown features from Xen's capabilities

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 12/02/16 17:14, Jan Beulich wrote:
>>>> On 12.02.16 at 17:48, <andrew.cooper3@citrix.com> wrote:
>> On 12/02/16 16:43, Jan Beulich wrote:
>>>>>> On 05.02.16 at 14:42, <andrew.cooper3@citrix.com> wrote:
>>>> --- /dev/null
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -0,0 +1,19 @@
>>>> +#include <xen/lib.h>
>>>> +#include <asm/cpuid.h>
>>>> +
>>>> +const uint32_t known_features[] = INIT_KNOWN_FEATURES;
>>>> +
>>>> +static void __maybe_unused build_assertions(void)
>>>> +{
>>>> +    BUILD_BUG_ON(ARRAY_SIZE(known_features) != FSCAPINTS);
>>> This is sort of redundant with ...
>>>
>>>> --- /dev/null
>>>> +++ b/xen/include/asm-x86/cpuid.h
>>>> @@ -0,0 +1,24 @@
>>>> +#ifndef __X86_CPUID_H__
>>>> +#define __X86_CPUID_H__
>>>> +
>>>> +#include <asm/cpuid-autogen.h>
>>>> +
>>>> +#define FSCAPINTS FEATURESET_NR_ENTRIES
>>>> +
>>>> +#ifndef __ASSEMBLY__
>>>> +#include <xen/types.h>
>>>> +
>>>> +extern const uint32_t known_features[FSCAPINTS];
>>> ... the use of FSCAPINTS here. You'd catch more mistakes if you
>>> just used [] here.
>> Not quite.
>>
>> The extern gives an explicit size so other translation units can use
>> ARRAY_SIZE().
> True.
>
>> Without the BUILD_BUG_ON(), const uint32_t known_features[] can actually
>> be longer than FSCAPINTS, and everything compiles fine.
>>
>> The BUILD_BUG_ON() were introduced following an off-by-one error
>> generating INIT_KNOWN_FEATURES, where ARRAY_SIZE(known_features) was
>> different in this translation unit than all others.
> But what if INIT_KNOWN_FEATURES inits fewer than the intended
> number of elements. The remaining array members will be zero, sure,
> but I think such a condition would suggest a mistake elsewhere, and
> hence might be worth flagging.

In principle, implicit zero extending is ok.

In practice, the autogen script explicitly zero extends the identifier
to the intended number of words.

~Andrew