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 16:47, 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?
I don't have that build log at hand, but you can reproduce them by 
defining
#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)

and then fiddle a bit with the signedness of the constants in
xen/arch/x86/include/asm/page-bits.h.

They are essentially pointer comparison errors in the instances of 'min' 
and 'max'
that involve those macros or macros derived from those, which make the 
build fail because
they (rightfully) trigger a warning from gcc.

> > 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.
>
> Cheers,
You have a point in that is sort of hides a deeper issue that is 
constituted by the
signedness of the macro, but I'd suggest adding to this patch a comment 
explaining what
needs to be done, rather than a deviation comment that hides the 
violations. The reason
is that in the former case, if you put an unsigned argument too big to 
fit in an integer
it will generate a new report, while if a negative argument is used, 
there is a warning
comment on the macro definition (not an ideal countermeasure, though).

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