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.marq...@arm.com> wrote:
Hi Xenia,
On 29 Jun 2022, at 09:55, xenia <burzalod...@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 <burzalod...@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 <burzalod...@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
