Re: [XEN PATCH] xen/types: address Rule 10.1 for macro BITS_TO_LONGS

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.10.2023 15:23, Nicola Vetrini wrote:
> On 05/09/2023 09:31, Nicola Vetrini wrote:
>> Given its use in the declaration
>> 'DECLARE_BITMAP(features, IOMMU_FEAT_count)' the argument
>> 'bits' has essential type 'enum iommu_feature', which is not
>> allowed by the Rule as an operand to the addition operator.
>> Given that its value can be represented by a signed integer,
>> the explicit cast resolves the violation.
>>
>> Signed-off-by: Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>
>> ---
>>  xen/include/xen/types.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>> index aea259db1ef2..2ff8f243908d 100644
>> --- a/xen/include/xen/types.h
>> +++ b/xen/include/xen/types.h
>> @@ -23,7 +23,7 @@ typedef signed long ssize_t;
>>  typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>
>>  #define BITS_TO_LONGS(bits) \
>> -    (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>> +    (((int)(bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>  #define DECLARE_BITMAP(name,bits) \
>>      unsigned long name[BITS_TO_LONGS(bits)]
> 
> Revisiting this thread after a while: I did some digging and changing 
> the essential type of
> BITS_TO_LONGS to unsigned
> 
> #define BYTES_PER_LONG (_AC(1, U) << LONG_BYTEORDER)
> #define BITS_PER_LONG (BYTES_PER_LONG << 3)
> [...]
> #define BITS_TO_LONGS(bits) \
>      (((bits) + BITS_PER_LONG - 1U) / BITS_PER_LONG)
> #define DECLARE_BITMAP(name,bits) \
>      unsigned long name[BITS_TO_LONGS(bits)]
> 
> leads to a whole lot of issues due to the extensive usage of these 
> macros
> (BITS_TO_LONGS, BITS_PER_LONG) in comparisons with e.g. the macros 
> min/max.
> The comments made in this series to the patch do have value (e.g. 
> BITS_TO_LONGS should be
> expected to have only a positive argument), but ultimately the changes 
> required in order to
> do this and respect all other constraints (either in the form of MISRA 
> rules or gcc warnings)
> is far too broad to be tackled by a single patch.
> 
> Notable examples of such consequences:
> 
> There is a build error due to -Werror because of a pointer comparison at 
> line 469 of common/numa.c:
> i = min(PADDR_BITS, BITS_PER_LONG - 1);
> where
> #define PADDR_BITS              52
> 
> if x86's PADDR_BITS becomes unsigned, then other issues arise, such as:
> 
> xenheap_bits = min(flsl(mfn + 1) - 1 + PAGE_SHIFT, PADDR_BITS);
> 
> here the type of flsl is int, so either flsl should become unsigned too, 
> or the second
> argument should be suitably modified.

If PADDR_BITS was to gain a U suffix, I expect PAGE_SHIFT ought to as
well. Which would address the compiler complaint, but of course would
then still leave the left hand expression not aligned with Misra's
essential type system.

> In the end, the modification that solves a lot of violations (due to 
> this being inside an header, though it's a single place to be modified) 
> is this:
> 
> DECLARE_BITMAP(features, (int)IOMMU_FEAT_count)
> 
> If, as it has been argued, BITS_TO_LONGS really needs to become 
> unsigned, then a more general
> rethinking of the types involved needs to happen.

Well, yes, just that we'll need to find ways to make the changes gradually,
not all in one go. Even if not admitted to originally, I think it was
pretty clear that the Misra effort would lead to such.

Jan