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

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

[Xenia Ragiadakou]

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@gmail.com>
---
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))
-#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) *	\
-- 
2.34.1

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

[Rahul Singh]

Hi Xenia,

> On 28 Jun 2022, at 4:08 pm, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>

Reviewed-by: Rahul Singh <rahul.singh@arm.com>

Regards,
Rahul
> ---
> 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))
> -#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) *	\
> -- 
> 2.34.1
> 

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

[Bertrand Marquis]

Hi Xenia,

> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
> ---
> 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.

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

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

[xenia]

Hi Bertrand,

On 6/29/22 10:24, Bertrand Marquis wrote:
> Hi Xenia,
>
>> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
>> ---
>> 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
>

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

[Bertrand Marquis]

Hi Xenia,

> On 29 Jun 2022, at 09:55, xenia <burzalodowa@gmail.com> wrote:
> 
> Hi Bertrand,
> 
> On 6/29/22 10:24, Bertrand Marquis wrote:
>> Hi Xenia,
>> 
>>> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
>>> ---
>>> 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

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

[Bertrand Marquis]

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@arm.com> wrote:
> 
> Hi Xenia,
> 
>> On 29 Jun 2022, at 09:55, xenia <burzalodowa@gmail.com> wrote:
>> 
>> Hi Bertrand,
>> 
>> On 6/29/22 10:24, Bertrand Marquis wrote:
>>> Hi Xenia,
>>> 
>>>> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
>>>> ---
>>>> 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

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

[Julien Grall]

On 29/06/2022 15:10, Bertrand Marquis wrote:
> Hi,

Hi Bertrand,

> In fact the patch was committed before we started this discussion as Rahul R-b was enough.

It was probably merged a bit too fast. When there are multiple 
maintainers responsible for the code, I tend to prefer to wait a bit 
just in case there are extra comments.

> But I would still be interested to have other maintainers view on this.

Rahul and you are the official maintainers for that code. I think 
Stefano and I are only CCed because we maintain the Arm code 
(get_maintainers.pl doesn't automatically remove maintainers from upper 
directory).

> 
>> On 29 Jun 2022, at 10:03, Bertrand Marquis <Bertrand.Marquis@arm.com> wrote:
>>
>> Hi Xenia,
>>
>>> On 29 Jun 2022, at 09:55, xenia <burzalodowa@gmail.com> wrote:
>>>
>>> Hi Bertrand,
>>>
>>> On 6/29/22 10:24, Bertrand Marquis wrote:
>>>> Hi Xenia,
>>>>
>>>>> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 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@gmail.com>
>>>>> ---
>>>>> 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 ?

In general, I prefer if we use 1U (or 1UL/1ULL) so it is clearer that 
the code is correct and consistent (I always find odd when we use 1U << 
31 but 1 << 20).

In this case, even if you use 1U, it is not really clear whether 
max_n_shift can be greater than 31. That said, I would not suggest to 
use ULL unless this is strictly necessary.

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

This can be solved by sending patch to Linux as well. This will also 
help Linux to reduce the number MISRA violations (I guess SMMUv3 will be 
part of the safety certification scope).

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

I will address both point separately.

In general, we don't want to mix coding style within a file. As the file 
started with the Linux one, then we should keep it like that. Otherwise, 
you will end up with a mix and it will be difficult for the contributor 
to know how to write new code. That said, I would not necessarily 
consider using "1 << ..." as part of the code style we want to keep.

This brings to the second part (i.e. keeping the code near Linux). Linux 
has a much larger user/developper base than us. Therefore there are more 
chance for them to find bugs and fix them. By diverging, then we could 
end up to add new bugs and also increase the maintainance.

I have tried both way with the SMMUv{1,2} driver. The first attempt was 
to fully adapt the code to Xen (coding style...). But this made 
difficult to keep track of bugs. A few months we removed it completely 
and then tried to keep as close as to Linux. Since then Linux reworked 
the IOMMU subsystem, so port needs to be adapted. It is more difficult 
but likely less than if we had our own port.

So overall, I think it was beneficials for our version of the SMMU{v1, 
v2} drivers to be close to Linux. I haven't looked very close to the 
SMMUv3 to state whether we should stay close or not.

>> Rahul, Julien, Stefano: what do you think here ?

Rahul and you are the maintainers. I can share my preference/experience, 
but I think this is your call on how you want to maintain the driver.

Cheers,

-- 
Julien Grall

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

[Xenia Ragiadakou]

On 6/29/22 18:02, Julien Grall wrote:
> On 29/06/2022 15:10, Bertrand Marquis wrote:
>> Hi,
> 
> Hi Bertrand,
> 
>> In fact the patch was committed before we started this discussion as 
>> Rahul R-b was enough.
> 
> It was probably merged a bit too fast. When there are multiple 
> maintainers responsible for the code, I tend to prefer to wait a bit 
> just in case there are extra comments.
> 
>> But I would still be interested to have other maintainers view on this.
> 
> Rahul and you are the official maintainers for that code. I think 
> Stefano and I are only CCed because we maintain the Arm code 
> (get_maintainers.pl doesn't automatically remove maintainers from upper 
> directory).
> 
>>
>>> On 29 Jun 2022, at 10:03, Bertrand Marquis <Bertrand.Marquis@arm.com> 
>>> wrote:
>>>
>>> Hi Xenia,
>>>
>>>> On 29 Jun 2022, at 09:55, xenia <burzalodowa@gmail.com> wrote:
>>>>
>>>> Hi Bertrand,
>>>>
>>>> On 6/29/22 10:24, Bertrand Marquis wrote:
>>>>> Hi Xenia,
>>>>>
>>>>>> On 28 Jun 2022, at 16:08, Xenia Ragiadakou <burzalodowa@gmail.com> 
>>>>>> 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@gmail.com>
>>>>>> ---
>>>>>> 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 ?
> 
> In general, I prefer if we use 1U (or 1UL/1ULL) so it is clearer that 
> the code is correct and consistent (I always find odd when we use 1U << 
> 31 but 1 << 20).
> 
> In this case, even if you use 1U, it is not really clear whether 
> max_n_shift can be greater than 31. That said, I would not suggest to 
> use ULL unless this is strictly necessary.
> 
>>>
>>>>
>>>> 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.
> 
> This can be solved by sending patch to Linux as well. This will also 
> help Linux to reduce the number MISRA violations (I guess SMMUv3 will be 
> part of the safety certification scope).

Linux relies on -fno-strict-overflow (implementation-defined behavior). 
Nevertheless, a patch changing all of them to BIT() has some chances to 
get accepted.

> 
>>>
>>> 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.
> 
> I will address both point separately.
> 
> In general, we don't want to mix coding style within a file. As the file 
> started with the Linux one, then we should keep it like that. Otherwise, 
> you will end up with a mix and it will be difficult for the contributor 
> to know how to write new code. That said, I would not necessarily 
> consider using "1 << ..." as part of the code style we want to keep.
> 
> This brings to the second part (i.e. keeping the code near Linux). Linux 
> has a much larger user/developper base than us. Therefore there are more 
> chance for them to find bugs and fix them. By diverging, then we could 
> end up to add new bugs and also increase the maintainance.
> 
> I have tried both way with the SMMUv{1,2} driver. The first attempt was 
> to fully adapt the code to Xen (coding style...). But this made 
> difficult to keep track of bugs. A few months we removed it completely 
> and then tried to keep as close as to Linux. Since then Linux reworked 
> the IOMMU subsystem, so port needs to be adapted. It is more difficult 
> but likely less than if we had our own port.
> 
> So overall, I think it was beneficials for our version of the SMMU{v1, 
> v2} drivers to be close to Linux. I haven't looked very close to the 
> SMMUv3 to state whether we should stay close or not.
> 
>>> Rahul, Julien, Stefano: what do you think here ?
> 
> Rahul and you are the maintainers. I can share my preference/experience, 
> but I think this is your call on how you want to maintain the driver.
> 
> Cheers,
> 

-- 
Xenia