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

[Bertrand Marquis <Bertrand.Marquis@xxxxxxx>]

Hi,

In fact the patch was committed before we started this discussion as Rahul R-b 
was enough.
But I would still be interested to have other maintainers view on this.

> On 29 Jun 2022, at 10:03, Bertrand Marquis <Bertrand.Marquis@xxxxxxx> wrote:
> 
> 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

Cheers
Bertrand