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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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