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

[Nicola Vetrini <nicola.vetrini@xxxxxxxxxxx>]

On 07/10/2023 03: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@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?
Indeed. MISRA interprets enums as unordered sets of constants.

> 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?
Correct. See also my reply to Julien's message: right now 
value-preserving conversions
do not generate violations, but a non-value preserving conversion from 
unsigned to signed
will generate one.

> 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.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)