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

[Julien Grall <julien@xxxxxxx>]

On 10/10/2023 02:09, Stefano Stabellini wrote:
> 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.
We have a lot of places in Xen using implicit conversion from enum to 
integer (for instance in the P2M code for p2m_type_t). Does ECLAIR 
report all of them? If not, then why?
> 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.
See above. I'd like to understand whether we are going to sprinkle the 
code with cast. If so, I am afraid I am against this solution.
Cheers,

--
Julien Grall