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