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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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

So we need either (int) or (unsigned int). The question is which one
between the two, and theoretically (unsigned int) is better but due to
the reasons above (int) is the simplest choice.

Yes, instead of the cast we can also add a SAF-x comment, which refers
to a deviation that says something along the lines "we could fix this
with a cast but we prefer a deviation because it makes the code easier
to read".

In general my personal preference would be to use a cast, because we
shouldn't implicitly convert enums to integers. However, in this
specific case where there is int vs. unsigned int discussion, I can see
that also SAF-x could be a way to go.