[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [XEN PATCH 10/10] arm/smmu: address violation of MISRA C:2012 Rule 8.2
Hi,
On 19/10/2023 01:21, Stefano Stabellini wrote:
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?
Federico suggested that there are hundreds of violation. I feel like we
should convert the SMMU driver to Xen coding style, clean-it up and then
handle any MISRA violations.
I would still be ok to accept small MISRA fix first though.
Cheers,
--
Julien Grall
|
