Re: [XEN PATCH 5/9] x86/cpu-policy: address violations of MISRA C Rule 10.1

[Nicola Vetrini <nicola.vetrini@bugseng.com>]

On 06/10/2023 19:57, Andrew Cooper wrote:
> On 06/10/2023 9:26 am, Nicola Vetrini wrote:
>> The COUNT_LEAVES macro is introduced to avoid using an essentially
>> boolean value in a subtraction.
>> 
>> No functional change.
>> 
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>> ---
>>  xen/include/xen/lib/x86/cpu-policy.h | 13 +++++++------
>>  1 file changed, 7 insertions(+), 6 deletions(-)
>> 
>> diff --git a/xen/include/xen/lib/x86/cpu-policy.h 
>> b/xen/include/xen/lib/x86/cpu-policy.h
>> index bab3eecda6c1..700993cc67e8 100644
>> --- a/xen/include/xen/lib/x86/cpu-policy.h
>> +++ b/xen/include/xen/lib/x86/cpu-policy.h
>> @@ -95,17 +95,18 @@ const char *x86_cpuid_vendor_to_str(unsigned int 
>> vendor);
>>  #define CPUID_GUEST_NR_EXTD       MAX(CPUID_GUEST_NR_EXTD_INTEL, \
>>                                        CPUID_GUEST_NR_EXTD_AMD)
>> 
>> +#define COUNT_LEAVES(X) ((X) - ((X) ? 1 : 0))
>>  /*
>>   * Maximum number of leaves a struct cpu_policy turns into when 
>> serialised for
>>   * interaction with the toolstack.  (Sum of all leaves in each union, 
>> less the
>>   * entries in basic which sub-unions hang off of.)
>>   */
>> -#define CPUID_MAX_SERIALISED_LEAVES                     \
>> -    (CPUID_GUEST_NR_BASIC +                             \
>> -     CPUID_GUEST_NR_FEAT   - !!CPUID_GUEST_NR_FEAT +    \
>> -     CPUID_GUEST_NR_CACHE  - !!CPUID_GUEST_NR_CACHE +   \
>> -     CPUID_GUEST_NR_TOPO   - !!CPUID_GUEST_NR_TOPO +    \
>> -     CPUID_GUEST_NR_XSTATE - !!CPUID_GUEST_NR_XSTATE +  \
>> +#define CPUID_MAX_SERIALISED_LEAVES         \
>> +    (CPUID_GUEST_NR_BASIC +                 \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_FEAT) +    \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_CACHE) +   \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_TOPO) +    \
>> +     COUNT_LEAVES(CPUID_GUEST_NR_XSTATE) +  \
>>       CPUID_GUEST_NR_EXTD + 2 /* hv_limit and hv2_limit */ )
> 
> This may not have been a MISRA-approved calculation, but encapsulating
> it like this breaks any ability to follow what's going on.
> 
> CPUID data in x86 is mostly a sparse 1-D array (BASIC, EXTD, HV 
> blocks),
> but a couple of elements in the BASIC array have arrays themselves.
> 
> The struct is laid out for O(1) access, so you can't just say
> sizeof(struct) / sizeof(element).  The BASIC array has elements (0x4,
> 0x7, 0xb, 0xd) which hold no data because there's actually an array
> elsewhere containing all the data.
> 
> So logically, it's:
> 
> (BASIC + (FEAT - 1) + (CACHE - 1) + (TOPO - 1) + (XSTATE - 1)) + EXTD + 
> 2
> 
> And in practice I'd far rather express it with a plain -1 than a -
> !!NR_, if the latter isn't an option.
> 
> Presumably MISRA would be happy with that?
> 
> If so, I can submit a patch.  There's also a typo in that the comment
> that wants fixing.
> 
> ~Andrew

Yes, that should be fine. I'll be happy to test that.

-- 
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)