Re: [XEN PATCH][for-4.19 8/9] xen/types: address Rule 10.1 for DECLARE_BITMAP use

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

On 10/10/2023 12:53, Julien Grall wrote:
> On 10/10/2023 02:09, Stefano Stabellini wrote:
>> On Mon, 9 Oct 2023, Julien Grall wrote:
>>> On 07/10/2023 02:04, Stefano Stabellini wrote:
>>>> On Fri, 6 Oct 2023, Julien Grall wrote:
>>>>> Hi Nicola,
>>>>> 
>>>>> On 06/10/2023 11:10, Nicola Vetrini wrote:
>>>>>> On 06/10/2023 11:34, Julien Grall wrote:
>>>>>>> Hi Nicola,
>>>>>>> 
>>>>>>> On 06/10/2023 09:26, 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
>>>>>>>> in macro 'BITS_TO_LONGS'.
>>>>>>>> 
>>>>>>>> A comment in BITS_TO_LONGS is added to make it clear that
>>>>>>>> values passed are meant to be positive.
>>>>>>> 
>>>>>>> I am confused. If the value is meant to be positive. Then...
>>>>>>> 
>>>>>>>> 
>>>>>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com>
>>>>>>>> ---
>>>>>>>>     xen/include/xen/iommu.h | 2 +-
>>>>>>>>     xen/include/xen/types.h | 1 +
>>>>>>>>     2 files changed, 2 insertions(+), 1 deletion(-)
>>>>>>>> 
>>>>>>>> diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h
>>>>>>>> index 0e747b0bbc1c..34aa0b9b5b81 100644
>>>>>>>> --- a/xen/include/xen/iommu.h
>>>>>>>> +++ b/xen/include/xen/iommu.h
>>>>>>>> @@ -360,7 +360,7 @@ struct domain_iommu {
>>>>>>>>     #endif
>>>>>>>>           /* Features supported by the IOMMU */
>>>>>>>> -    DECLARE_BITMAP(features, IOMMU_FEAT_count);
>>>>>>>> +    DECLARE_BITMAP(features, (int)IOMMU_FEAT_count);
>>>>>>> 
>>>>>>> ... why do we cast to (int) rather than (unsigned int)? Also, I 
>>>>>>> think
>>>>>>> this cast deserve a comment on top because this is not a very 
>>>>>>> obvious
>>>>>>> one.
>>>>>>> 
>>>>>>>>           /* Does the guest share HAP mapping with the IOMMU? */
>>>>>>>>         bool hap_pt_share;
>>>>>>>> diff --git a/xen/include/xen/types.h b/xen/include/xen/types.h
>>>>>>>> index aea259db1ef2..936e83d333a0 100644
>>>>>>>> --- a/xen/include/xen/types.h
>>>>>>>> +++ b/xen/include/xen/types.h
>>>>>>>> @@ -22,6 +22,7 @@ typedef signed long ssize_t;
>>>>>>>>       typedef __PTRDIFF_TYPE__ ptrdiff_t;
>>>>>>>>     +/* Users of this macro are expected to pass a positive 
>>>>>>>> value */
>>>>>>>>     #define BITS_TO_LONGS(bits) \
>>>>>>>>         (((bits)+BITS_PER_LONG-1)/BITS_PER_LONG)
>>>>>>>>     #define DECLARE_BITMAP(name,bits) \
>>>>>>> 
>>>>>>> Cheers,
>>>>>> 
>>>>>> See [1] for the reason why I did so. I should have mentioned that 
>>>>>> in the
>>>>>> commit notes, sorry.
>>>>>> In short, making BITS_TO_LONGS essentially unsigned would cause a
>>>>>> cascade of
>>>>>> build errors and
>>>>>> possibly other essential type violations.
>>>>> Can you share some of the errors?
>>>>> 
>>>>>> If this is to be fixed that way, the effort required
>>>>>> is far greater. Either way, a comment on top of can be added, 
>>>>>> along the
>>>>>> lines of:
>>>>>> 
>>>>>> Leaving this as an enum would violate MISRA C:2012 Rule 10.1
>>>>> 
>>>>> I read this as you are simply trying to silence your tool. I think 
>>>>> this
>>>>> the
>>>>> wrong approach as you are now making the code confusing for the 
>>>>> reader
>>>>> (you
>>>>> pass a signed int to a function that is supposed to take a positive
>>>>> number).
>>>>> 
>>>>> I appreciate that this will result to more violations at the 
>>>>> beginning.
>>>>> But
>>>>> the whole point of MISRA is to make the code better.
>>>>> 
>>>>> If this is too complex to solve now, then a possible option is to 
>>>>> deviate
>>>>> for
>>>>> the time being.
>>>> 
>>>> I agree on everything Julien's wrote and I was about to suggest to 
>>>> use a
>>>> SAF comment to suppress the warning because it is clearer than a int
>>>> cast.
>>>> 
>>>> But then I realized that even if BITS_TO_LONGS was fixed, wouldn't 
>>>> still
>>>> we have a problem because IOMMU_FEAT_count is an enum?
>>>> 
>>>> Is it the case that IOMMU_FEAT_count would have to be cast 
>>>> regardless,
>>>> either to int or unsigned int or whatever to be used in 
>>>> DECLARE_BITMAP?
>>>> 
>>>> 
>>>> So we have 2 problems here: one problem is DECLARE_BITMAP taking int
>>>> instead of unsigned int, and another problem is IOMMU_FEAT_count 
>>>> being
>>>> an enum.
>>>> 
>>>> If I got it right, then I would go with the cast to int (like done 
>>>> in
>>>> this patch) with a decent comment on top of it.
>>> 
>>> I might be missing something here. But why should we use cast rather 
>>> than /*
>>> SAF-X */ just above? I would have expected we wanted to highlight the
>>> violation rather than hiding it.
>> 
>> My understanding is that the cast is required when converting an enum
>> type to an integer type and vice versa. The idea is that we shouldn't 
>> do
>> implicit conversions as they are error prone, only explicit 
>> conversions
>> are allowed between enum and integers.
> 
> We have a lot of places in Xen using implicit conversion from enum to
> integer (for instance in the P2M code for p2m_type_t). Does ECLAIR
> report all of them? If not, then why?
> 

Re-replying here, since in the other reply I didn't address your concern 
fully:
yes, there are more than a few places where this comes up for Rule 10.1, 
especially
in x86 code. In theory a cast is not the only option to bring the code 
into compliance,
but the specific solution should be checked on a case-by-case basis.

The main aim of the series on R10.1 is to deviate or fix the main 
offenders in terms
of violations with as little effort as possible, to have a more 
manageable analysis
result (in my branch, with some patches yet to be submitted I'm down to 
a few violations
on ARM and ~100 on x86).

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