Re: [PATCH] xen/arm: smmu-v3: Fix MISRA C 2012 Rule 1.3 violations

[xenia <burzalodowa@xxxxxxxxx>]

Hi Bertrand,

On 6/29/22 10:24, Bertrand Marquis wrote:
> Hi Xenia,
>
> > On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@xxxxxxxxx> wrote:
> >
> > The expression 1 << 31 produces undefined behaviour because the type of integer
> > constant 1 is (signed) int and the result of shifting 1 by 31 bits is not
> > representable in the (signed) int type.
> > Change the type of 1 to unsigned int by adding the U suffix.
> >
> > Signed-off-by: Xenia Ragiadakou <burzalodowa@xxxxxxxxx>
> > ---
> > Q_OVERFLOW_FLAG has already been fixed in upstream kernel code.
> > For GBPA_UPDATE I will submit a patch.
> >
> > xen/drivers/passthrough/arm/smmu-v3.c | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c 
> > b/xen/drivers/passthrough/arm/smmu-v3.c
> > index 1e857f915a..f2562acc38 100644
> > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > @@ -338,7 +338,7 @@ static int platform_get_irq_byname_optional(struct device 
> > *dev,
> > #define CR2_E2H                         (1 << 0)
> >
> > #define ARM_SMMU_GBPA                   0x44
> > -#define GBPA_UPDATE                    (1 << 31)
> > +#define GBPA_UPDATE                    (1U << 31)
> > #define GBPA_ABORT                      (1 << 20)
> >
> > #define ARM_SMMU_IRQ_CTRL               0x50
> > @@ -410,7 +410,7 @@ static int platform_get_irq_byname_optional(struct device 
> > *dev,
> >
> > #define Q_IDX(llq, p)                   ((p) & ((1 << (llq)->max_n_shift) - 1))
> > #define Q_WRP(llq, p)                   ((p) & (1 << (llq)->max_n_shift))
> Could also make sense to fix those 2 to be coherent.
According to the spec, the maximum value that max_n_shift can take is 19.
Hence, 1 << (llq)->max_n_shift cannot produce undefined behavior.

Personally, I have no problem to submit another patch that adds U/UL 
suffixes to all shifted integer constants in the file :) but ...
It seems that this driver has been ported from linux and this file still 
uses linux coding style, probably because deviations will reduce its 
maintainability.
Adding a U suffix to those two might be considered an unjustified 
deviation.
> > -#define Q_OVERFLOW_FLAG                        (1 << 31)
> > +#define Q_OVERFLOW_FLAG                        (1U << 31)
> > #define Q_OVF(p)                        ((p) & Q_OVERFLOW_FLAG)
> > #define Q_ENT(q, p)                     ((q)->base +                 \
> >                                          Q_IDX(&((q)->llq), p) * \
> Cheers
> Bertrand