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

[Stefano Stabellini <sstabellini@xxxxxxxxxx>]

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?


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

Thanks!


> Do you expect larger MISRA changes in the SMMU driver?

I'll let Federico answer this one.