Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2

[Julien Grall <julien@xxxxxxx>]

Hi Stefano,

On 16/10/2023 21:47, Stefano Stabellini wrote:
> On Mon, 16 Oct 2023, Bertrand Marquis wrote:
> > > On 16 Oct 2023, at 15:38, Julien Grall <julien@xxxxxxx> wrote:
> > >
> > >
> > >
> > > On 16/10/2023 14:31, Bertrand Marquis wrote:
> > > > Hi Julien,
> > > Hi Bertrand,
> > >
> > > > > On 16 Oct 2023, at 11:07, Julien Grall <julien@xxxxxxx> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On 13/10/2023 16:24, Federico Serafini wrote:
> > > > > > Add missing parameter names, no functional change.
> > > > > > Signed-off-by: Federico Serafini <federico.serafini@xxxxxxxxxxx>
> > > > > > ---
> > > > > >   xen/drivers/passthrough/arm/smmu.c | 6 +++---
> > > > > This file is using the Linux coding style because it is imported from Linux. I 
> > > > > was under the impression we would exclude such file for now.
> > > > >
> > > > > Looking at exclude-list.json, it doesn't seem to be present. I think this patch 
> > > > > should be replaced with adding a line in execlude-list.json.
> > > > I think that during one of the discussions we said that this file already 
> > > > deviated quite a lot from the status in Linux and we wanted to turn it to Xen 
> > > > coding style in the future hence it is not listed in the exclude file.
> > > AFAIK the SMMUv{1, 2} code didn't deviate very much from Linux. I can't tell 
> > > about the SMMUv3.
> > True that i had the v3 code in mind, we might not want to do that for v1/2 you 
> > are right.
> >
> > > > At the end having a working smmu might be critical in a safety context so it 
> > > > will make sense to also check this part of xen.
> > > I don't buy this argument right now. We have files in exclude-lists.json that I 
> > > would consider critical for Xen to run (e.g. common/bitmap.c, common/libfdt). 
> > > So if you want to use this argument then we need to move critical components of 
> > > Xen out of the exclusion list.
> > I am sure we will at some point for runtime code but we cannot do everything on 
> > the first shot.
> > I was not meaning to do that now but that it is something to consider.
> Things that are in exclude-lists.json are source files that come from
> other projects and also change very rarely. The argument that we don't
> do MISRA C on the files in exclude-lists.json, it is not because those
> files are unimportant, but because they change only once every many
> years.
Interesting. I would have thought this would be a condition to do MISRA 
as the cost to port a patch would increase a bit but this is one time 
cost every many years. Whereas code like the SMMU are still actively 
developped. And in particular for SMMUv2 we tried to stick close to 
Linux to help backport. So this would be a reason to initially exclude 
it from MISRA.
> Of course the least we rely on exclude-lists.json the better.
>
> For smmu.c, looking at the git history I think it is more actively
> worked on than other files such as lib/rbtree.c or common/bitmap.c.
> Given that backports from Linux to smmu.c are not straightforward anyway
> (please correct me if I am wrong) then I think we should not add smmu.c
> to exclude-lists.json and do MISRA for smmu.c.
I haven't done any recently. But if they are already not 
straightforward, then adding MISRA on top is not really to make it 
better. So I think if you want to do MISRA for the SMMU, then we need to 
fully convert it to Xen and abandon the idea to backport from Linux.
This would also make the code looks nicer as at the moment this contains 
wrapper just to stay as close as possible to Linux.
As a side note, the change here looks fairly self-contained. So I don't 
expect a major impact and therefore would not block this. This may not 
be the case for more complex one. Hence why I wanted to exclude it.
Do you expect larger MISRA changes in the SMMU driver?

Cheers,

--
Julien Grall