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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

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@xxxxxxxxxxx>
> > ---
> >   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. 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

[1] 
https://lore.kernel.org/xen-devel/6495ba58bda01eae1f4baa46096424eb@xxxxxxxxxxx/
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)