| Message ID | 199886f6ba1f2d5701eabd080b4f9723fc28f4b9.1697207038.git.federico.serafini@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: address some violations of MISRA C:2012 Rule 8.2 | expand |
On Fri, 13 Oct 2023, Federico Serafini wrote: > Add missing parameter names, no functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Hi, On 13/10/2023 16:24, Federico Serafini wrote: > Add missing parameter names, no functional change. > > Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> > --- > 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. Cheers,
Hi Julien, > On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >> --- >> 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. 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. @Rahul: do you agree ? Cheers Bertrand > > Cheers, > > -- > Julien Grall
On 16/10/2023 14:31, Bertrand Marquis wrote: > Hi Julien, Hi Bertrand, >> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>> --- >>> 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. > 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. Cheers,
Hi Julien, > On 16 Oct 2023, at 15:38, Julien Grall <julien@xen.org> wrote: > > > > On 16/10/2023 14:31, Bertrand Marquis wrote: >> Hi Julien, > > Hi Bertrand, > >>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>>> --- >>>> 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. Cheers Bertrand > > Cheers, > > -- > Julien Grall
Hi Bertrand > On 16 Oct 2023, at 2:31 pm, Bertrand Marquis <bertrand.marquis@arm.com> wrote: > > Hi Julien, > >> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>> --- >>> 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. > 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. > > @Rahul: do you agree ? Yes, you are right current SMMUv3 code already deviates quite a lot from the status of Linux because Xen handles the command queue in a different way than the way Linux handles it. More detailed can be found at the start of the SMMUv3 file. I am pasting it here also. * 5. Latest version of the Linux SMMUv3 code implements the commands queue * access functions based on atomic operations implemented in Linux. * Atomic functions used by the commands queue access functions are not * implemented in XEN therefore we decided to port the earlier version * of the code. Atomic operations are introduced to fix the bottleneck of * the SMMU command queue insertion operation. A new algorithm for * inserting commands into the queue is introduced, which is * lock-free on the fast-path. * Consequence of reverting the patch is that the command queue insertion * will be slow for large systems as spinlock will be used to serializes * accesses from all CPUs to the single queue supported by the hardware. * Once the proper atomic operations will be available in XEN the driver * can be updated. Anyway because of above reason backporting SMMUv3 Linux driver patches to Xen are not straight forward. If making smmu-v3.c to Xen coding style helps in safety context I am okay with that. Regards, Rahul
On Mon, 16 Oct 2023, Bertrand Marquis wrote: > > On 16 Oct 2023, at 15:38, Julien Grall <julien@xen.org> wrote: > > > > > > > > On 16/10/2023 14:31, Bertrand Marquis wrote: > >> Hi Julien, > > > > Hi Bertrand, > > > >>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> > >>>> --- > >>>> 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. 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. On the other hand, if we think that doing MISRA for smmu.c is going to make backports a lot harder, and we think that we want to do backports "often" (every year or every couple of years) then maybe we shouldn't do MISRA for smmu.c after all.
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@xen.org> wrote: >>> >>> >>> >>> On 16/10/2023 14:31, Bertrand Marquis wrote: >>>> Hi Julien, >>> >>> Hi Bertrand, >>> >>>>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>>>>> --- >>>>>> 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,
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@xen.org> wrote: > > > > > > > > > > > > > > > > On 16/10/2023 14:31, Bertrand Marquis wrote: > > > > > Hi Julien, > > > > > > > > Hi Bertrand, > > > > > > > > > > On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> > > > > > > > --- > > > > > > > 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.
On 18/10/23 02: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@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 16/10/2023 14:31, Bertrand Marquis wrote: >>>>>> Hi Julien, >>>>> >>>>> Hi Bertrand, >>>>> >>>>>>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>>>>>>> --- >>>>>>>> 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. > There are more than 500 violations of accepted rules involving the SMMU files under xen/drivers/passthrough/arm/ and half of them come from smmu.c. Those are violations of the series 10 and the essential type system, so I'll definitely expect larger changes to SMMU driver. Furthermore, there is approximately the same amount of violations involving MISRA rules which have yet to be discussed.
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@xen.org> wrote: >>>>> >>>>> >>>>> >>>>> On 16/10/2023 14:31, Bertrand Marquis wrote: >>>>>> Hi Julien, >>>>> >>>>> Hi Bertrand, >>>>> >>>>>>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>>>>>>> --- >>>>>>>> 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. Cheers,
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@xen.org> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On 16/10/2023 14:31, Bertrand Marquis wrote: > > > > > > > Hi Julien, > > > > > > > > > > > > Hi Bertrand, > > > > > > > > > > > > > > On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> > > > > > > > > > --- > > > > > > > > > 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?
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@xen.org> wrote: >>>>>>> >>>>>>> >>>>>>> >>>>>>> On 16/10/2023 14:31, Bertrand Marquis wrote: >>>>>>>> Hi Julien, >>>>>>> >>>>>>> Hi Bertrand, >>>>>>> >>>>>>>>> On 16 Oct 2023, at 11:07, Julien Grall <julien@xen.org> 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@bugseng.com> >>>>>>>>>> --- >>>>>>>>>> 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,
diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c index 71799064f8..11fc1d22ef 100644 --- a/xen/drivers/passthrough/arm/smmu.c +++ b/xen/drivers/passthrough/arm/smmu.c @@ -277,8 +277,8 @@ static void iommu_group_put(struct iommu_group *group) } static void iommu_group_set_iommudata(struct iommu_group *group, - struct arm_smmu_master_cfg *cfg, - void (*releasefn)(void *)) + struct arm_smmu_master_cfg *cfg, + void (*releasefn)(void *data)) { /* TODO: Store the releasefn for the PCI */ ASSERT(releasefn == NULL); @@ -2082,7 +2082,7 @@ static int arm_smmu_add_device(struct device *dev) struct arm_smmu_device *smmu; struct arm_smmu_master_cfg *cfg; struct iommu_group *group; - void (*releasefn)(void *) = NULL; + void (*releasefn)(void *data) = NULL; int ret; smmu = find_smmu_for_device(dev);
Add missing parameter names, no functional change. Signed-off-by: Federico Serafini <federico.serafini@bugseng.com> --- xen/drivers/passthrough/arm/smmu.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)