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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi Xenia,

> On 29 Jun 2022, at 09:55, xenia <burzalodowa@xxxxxxxxx> wrote:
> 
> 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.

I agree with that but my preference would be to not rely on deep analysis on 
the code for those kind of cases and use 1U whenever possible.

What other maintainers think on this ?

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

At this stage the SMMU code is starting to deviate a lot from Linux so it will 
not be possible to update it easily to sync with latest linux code.
And the decision should be are we fixing it or should we fully skip this file 
saying that it is coming from linux and should not be fixed.
We started to have a discussion during the FuSa meeting on this but we need to 
come up with a conclusion per file.

On this one I would say keeping it with linux code style and near from the 
linux one is not needed.
Rahul, Julien, Stefano: what do you think here ?

Cheers
Bertrand

>>> -#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