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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

On Wed, 18 Oct 2023, Julien Grall wrote:
> Hi Stefano,
> 
> On 18/10/2023 01:55, Stefano Stabellini wrote:
> > On Tue, 17 Oct 2023, Julien Grall wrote:
> > > 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.
> > 
> > You have a good point. If we do MISRA for the SMMU then we might as well
> > fully convert the file to Xen. As a clarification, we can still look at
> > the fixes on the Linux driver and "port" security fixes and other key
> > patches such as workarounds for broken specific SMMU versions, but for
> > sure we wouldn't want to backport a new feature of the driver or code
> > refactoring / code improvements of the driver. But that probably is
> > already the case today?
> 
> Most likely yes, some features might be useful to backport. The main one I can
> think of is not sharing the stage-2 page-tables as there might be some issue
> to allow the CPU to access the GICv3 ITS doorbell (so it would have to be only
> mapped in the IOMMU page-tables).
> 
> The other one is stage-1 support.
> 
> Anyway, it is not clear whether we could use the same implementation as Linux.

Yeah.

In terms of next steps, what do you suggest?

Are you OK with keeping smmu.c out of exclude-list? And to go forward
with this patch?