| Message ID | 1ee69d2f0b9e9a704bf869e2b2700f88a8069343.1701270983.git.nicola.vetrini@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | address some violations of MISRA C Rule 8.4 | expand |
On Wed, 29 Nov 2023, Nicola Vetrini wrote: > No functional change. > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > --- > The preferred way to deviate is to use asmlinkage, but this modification is only > the consequence of NUMA on ARM (and possibly PPC) being a work in progress. > As stated in the comment above the textual deviation, first_valid_mfn will > likely then become static and there would be no need for the comment anymore. > This works towards having the analysis for this rule clean (i.e. no violations); > the interest in having a clean rule is that then it could be used to signal > newly introduced violations by making the analysis job fail. Please add this text as part of the commit message. It can be done on commit. With that: Acked-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/common/page_alloc.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c > index 9b5df74fddab..794d7689b179 100644 > --- a/xen/common/page_alloc.c > +++ b/xen/common/page_alloc.c > @@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list); > * first_valid_mfn is exported because it is use in ARM specific NUMA > * helpers. See comment in arch/arm/include/asm/numa.h. > */ > +/* SAF-1-safe */ > mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; > > struct bootmem_region { > -- > 2.34.1 >
On 01.12.2023 03:47, Stefano Stabellini wrote: > On Wed, 29 Nov 2023, Nicola Vetrini wrote: >> No functional change. >> >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> --- >> The preferred way to deviate is to use asmlinkage, but this modification is only >> the consequence of NUMA on ARM (and possibly PPC) being a work in progress. >> As stated in the comment above the textual deviation, first_valid_mfn will >> likely then become static and there would be no need for the comment anymore. >> This works towards having the analysis for this rule clean (i.e. no violations); >> the interest in having a clean rule is that then it could be used to signal >> newly introduced violations by making the analysis job fail. > > Please add this text as part of the commit message. It can be done on > commit. I assume you saw my reply on another of the patches in this series as to asmlinkage use on variables? IOW I think this paragraph would also need adjustment to account for that. > With that: > > Acked-by: Stefano Stabellini <sstabellini@kernel.org> > > >> --- >> xen/common/page_alloc.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c >> index 9b5df74fddab..794d7689b179 100644 >> --- a/xen/common/page_alloc.c >> +++ b/xen/common/page_alloc.c >> @@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list); >> * first_valid_mfn is exported because it is use in ARM specific NUMA >> * helpers. See comment in arch/arm/include/asm/numa.h. >> */ >> +/* SAF-1-safe */ >> mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; >> >> struct bootmem_region { >> -- >> 2.34.1 >>
On Fri, 1 Dec 2023, Jan Beulich wrote: > On 01.12.2023 03:47, Stefano Stabellini wrote: > > On Wed, 29 Nov 2023, Nicola Vetrini wrote: > >> No functional change. > >> > >> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > >> --- > >> The preferred way to deviate is to use asmlinkage, but this modification is only > >> the consequence of NUMA on ARM (and possibly PPC) being a work in progress. > >> As stated in the comment above the textual deviation, first_valid_mfn will > >> likely then become static and there would be no need for the comment anymore. > >> This works towards having the analysis for this rule clean (i.e. no violations); > >> the interest in having a clean rule is that then it could be used to signal > >> newly introduced violations by making the analysis job fail. > > > > Please add this text as part of the commit message. It can be done on > > commit. > > I assume you saw my reply on another of the patches in this series as to > asmlinkage use on variables? IOW I think this paragraph would also need > adjustment to account for that. I was going to ask you about that: reading your reply https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me what you are asking or suggesting as next step in regard to asmlinkage use on variables.
Hi, On 02/12/2023 04:03, Stefano Stabellini wrote: > On Fri, 1 Dec 2023, Jan Beulich wrote: >> On 01.12.2023 03:47, Stefano Stabellini wrote: >>> On Wed, 29 Nov 2023, Nicola Vetrini wrote: >>>> No functional change. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> The preferred way to deviate is to use asmlinkage, but this modification is only >>>> the consequence of NUMA on ARM (and possibly PPC) being a work in progress. >>>> As stated in the comment above the textual deviation, first_valid_mfn will >>>> likely then become static and there would be no need for the comment anymore. >>>> This works towards having the analysis for this rule clean (i.e. no violations); >>>> the interest in having a clean rule is that then it could be used to signal >>>> newly introduced violations by making the analysis job fail. >>> >>> Please add this text as part of the commit message. It can be done on >>> commit. >> >> I assume you saw my reply on another of the patches in this series as to >> asmlinkage use on variables? IOW I think this paragraph would also need >> adjustment to account for that. > > I was going to ask you about that: reading your reply > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me > what you are asking or suggesting as next step in regard to asmlinkage > use on variables. Regardless what Jan wrote there, I think it would be an abuse to add asmlinkage for first_valid_mfn because it is not used by assembly code. The same goes for the use of SAF-1-safe. This is defined as: "Functions and variables used only by asm modules do not need to have a visible declaration prior to their definition." Now, regarding the violation itself. AFAIU, this would be a violation only on x86 because Arm, PPC, RISC-V will all need the declaration as NUMA is not supported. For Arm, we may have NUMA support soon, but I doubt this will happen in the next couple of years for the others. Furthermore, I am sure this is not the only exported variable/function which may be used by some archictectures but not others. So I think it would be better to declare first_valid_mfn in a common header. Cheers,
On 02.12.2023 04:03, Stefano Stabellini wrote: > On Fri, 1 Dec 2023, Jan Beulich wrote: >> On 01.12.2023 03:47, Stefano Stabellini wrote: >>> On Wed, 29 Nov 2023, Nicola Vetrini wrote: >>>> No functional change. >>>> >>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>> --- >>>> The preferred way to deviate is to use asmlinkage, but this modification is only >>>> the consequence of NUMA on ARM (and possibly PPC) being a work in progress. >>>> As stated in the comment above the textual deviation, first_valid_mfn will >>>> likely then become static and there would be no need for the comment anymore. >>>> This works towards having the analysis for this rule clean (i.e. no violations); >>>> the interest in having a clean rule is that then it could be used to signal >>>> newly introduced violations by making the analysis job fail. >>> >>> Please add this text as part of the commit message. It can be done on >>> commit. >> >> I assume you saw my reply on another of the patches in this series as to >> asmlinkage use on variables? IOW I think this paragraph would also need >> adjustment to account for that. > > I was going to ask you about that: reading your reply > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me > what you are asking or suggesting as next step in regard to asmlinkage > use on variables. Either we need a separate attribute, or we need affirmation that calling convention attributes are ignored (and going to be going forward) for variables, or we need to resort to SAF-* comments. I'm not sure what's best (assuming the "affirm" wouldn't really be possible). Jan
On 2023-12-04 08:44, Jan Beulich wrote: > On 02.12.2023 04:03, Stefano Stabellini wrote: >> On Fri, 1 Dec 2023, Jan Beulich wrote: >>> On 01.12.2023 03:47, Stefano Stabellini wrote: >>>> On Wed, 29 Nov 2023, Nicola Vetrini wrote: >>>>> No functional change. >>>>> >>>>> Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >>>>> --- >>>>> The preferred way to deviate is to use asmlinkage, but this >>>>> modification is only >>>>> the consequence of NUMA on ARM (and possibly PPC) being a work in >>>>> progress. >>>>> As stated in the comment above the textual deviation, >>>>> first_valid_mfn will >>>>> likely then become static and there would be no need for the >>>>> comment anymore. >>>>> This works towards having the analysis for this rule clean (i.e. no >>>>> violations); >>>>> the interest in having a clean rule is that then it could be used >>>>> to signal >>>>> newly introduced violations by making the analysis job fail. >>>> >>>> Please add this text as part of the commit message. It can be done >>>> on >>>> commit. >>> >>> I assume you saw my reply on another of the patches in this series as >>> to >>> asmlinkage use on variables? IOW I think this paragraph would also >>> need >>> adjustment to account for that. >> >> I was going to ask you about that: reading your reply >> https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me >> what you are asking or suggesting as next step in regard to asmlinkage >> use on variables. > > Either we need a separate attribute, or we need affirmation that > calling > convention attributes are ignored (and going to be going forward) for > variables, or we need to resort to SAF-* comments. I'm not sure what's > best (assuming the "affirm" wouldn't really be possible). > Well, gcc does warn on unsupported attributes for the entity which are being dropped. This appears to be the case for calling convention attributes, as they are not listed in their documentation for variable attributes, but some more digging would be required to determine whether that's always the case.
On Mon, 4 Dec 2023, Nicola Vetrini wrote: > On 2023-12-04 08:44, Jan Beulich wrote: > > On 02.12.2023 04:03, Stefano Stabellini wrote: > > > On Fri, 1 Dec 2023, Jan Beulich wrote: > > > > On 01.12.2023 03:47, Stefano Stabellini wrote: > > > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote: > > > > > > No functional change. > > > > > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > > > > > --- > > > > > > The preferred way to deviate is to use asmlinkage, but this > > > > > > modification is only > > > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in > > > > > > progress. > > > > > > As stated in the comment above the textual deviation, > > > > > > first_valid_mfn will > > > > > > likely then become static and there would be no need for the comment > > > > > > anymore. > > > > > > This works towards having the analysis for this rule clean (i.e. no > > > > > > violations); > > > > > > the interest in having a clean rule is that then it could be used to > > > > > > signal > > > > > > newly introduced violations by making the analysis job fail. > > > > > > > > > > Please add this text as part of the commit message. It can be done on > > > > > commit. > > > > > > > > I assume you saw my reply on another of the patches in this series as to > > > > asmlinkage use on variables? IOW I think this paragraph would also need > > > > adjustment to account for that. > > > > > > I was going to ask you about that: reading your reply > > > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me > > > what you are asking or suggesting as next step in regard to asmlinkage > > > use on variables. > > > > Either we need a separate attribute, or we need affirmation that calling > > convention attributes are ignored (and going to be going forward) for > > variables, or we need to resort to SAF-* comments. I'm not sure what's > > best (assuming the "affirm" wouldn't really be possible). > > > > Well, gcc does warn on unsupported attributes for the entity which are being > dropped. This appears to be the case for calling convention attributes, as > they are not listed in their documentation for variable attributes, but some > more digging would be required to determine whether that's always the case. Given that I don't suppose we have many variables that need deviating (probably only 2-3 overall?) I think it is just easier to add a SAF comment.
On Sat, 2 Dec 2023, Julien Grall wrote: > Hi, > > On 02/12/2023 04:03, Stefano Stabellini wrote: > > On Fri, 1 Dec 2023, Jan Beulich wrote: > > > On 01.12.2023 03:47, Stefano Stabellini wrote: > > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote: > > > > > No functional change. > > > > > > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> > > > > > --- > > > > > The preferred way to deviate is to use asmlinkage, but this > > > > > modification is only > > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in > > > > > progress. > > > > > As stated in the comment above the textual deviation, first_valid_mfn > > > > > will > > > > > likely then become static and there would be no need for the comment > > > > > anymore. > > > > > This works towards having the analysis for this rule clean (i.e. no > > > > > violations); > > > > > the interest in having a clean rule is that then it could be used to > > > > > signal > > > > > newly introduced violations by making the analysis job fail. > > > > > > > > Please add this text as part of the commit message. It can be done on > > > > commit. > > > > > > I assume you saw my reply on another of the patches in this series as to > > > asmlinkage use on variables? IOW I think this paragraph would also need > > > adjustment to account for that. > > > > I was going to ask you about that: reading your reply > > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me > > what you are asking or suggesting as next step in regard to asmlinkage > > use on variables. > > Regardless what Jan wrote there, I think it would be an abuse to add > asmlinkage for first_valid_mfn because it is not used by assembly code. > > The same goes for the use of SAF-1-safe. This is defined as: > > "Functions and variables used only by asm modules do not need to have a > visible declaration prior to their definition." > > Now, regarding the violation itself. AFAIU, this would be a violation only on > x86 because Arm, PPC, RISC-V will all need the declaration as NUMA is not > supported. For Arm, we may have NUMA support soon, but I doubt this will > happen in the next couple of years for the others. > > Furthermore, I am sure this is not the only exported variable/function which > may be used by some archictectures but not others. So I think it would be > better to declare first_valid_mfn in a common header. That is true
On 2023-12-05 00:18, Stefano Stabellini wrote: > On Mon, 4 Dec 2023, Nicola Vetrini wrote: >> On 2023-12-04 08:44, Jan Beulich wrote: >> > On 02.12.2023 04:03, Stefano Stabellini wrote: >> > > On Fri, 1 Dec 2023, Jan Beulich wrote: >> > > > On 01.12.2023 03:47, Stefano Stabellini wrote: >> > > > > On Wed, 29 Nov 2023, Nicola Vetrini wrote: >> > > > > > No functional change. >> > > > > > >> > > > > > Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> >> > > > > > --- >> > > > > > The preferred way to deviate is to use asmlinkage, but this >> > > > > > modification is only >> > > > > > the consequence of NUMA on ARM (and possibly PPC) being a work in >> > > > > > progress. >> > > > > > As stated in the comment above the textual deviation, >> > > > > > first_valid_mfn will >> > > > > > likely then become static and there would be no need for the comment >> > > > > > anymore. >> > > > > > This works towards having the analysis for this rule clean (i.e. no >> > > > > > violations); >> > > > > > the interest in having a clean rule is that then it could be used to >> > > > > > signal >> > > > > > newly introduced violations by making the analysis job fail. >> > > > > >> > > > > Please add this text as part of the commit message. It can be done on >> > > > > commit. >> > > > >> > > > I assume you saw my reply on another of the patches in this series as to >> > > > asmlinkage use on variables? IOW I think this paragraph would also need >> > > > adjustment to account for that. >> > > >> > > I was going to ask you about that: reading your reply >> > > https://marc.info/?l=xen-devel&m=170142048615336 it is not clear to me >> > > what you are asking or suggesting as next step in regard to asmlinkage >> > > use on variables. >> > >> > Either we need a separate attribute, or we need affirmation that calling >> > convention attributes are ignored (and going to be going forward) for >> > variables, or we need to resort to SAF-* comments. I'm not sure what's >> > best (assuming the "affirm" wouldn't really be possible). >> > >> >> Well, gcc does warn on unsupported attributes for the entity which are >> being >> dropped. This appears to be the case for calling convention >> attributes, as >> they are not listed in their documentation for variable attributes, >> but some >> more digging would be required to determine whether that's always the >> case. > > Given that I don't suppose we have many variables that need deviating > (probably only 2-3 overall?) I think it is just easier to add a SAF > comment. I agree, but then given what Julien wrote in the thread, I'll see what I can do to avoid creating a new SAF entry.
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c index 9b5df74fddab..794d7689b179 100644 --- a/xen/common/page_alloc.c +++ b/xen/common/page_alloc.c @@ -258,6 +258,7 @@ static PAGE_LIST_HEAD(page_broken_list); * first_valid_mfn is exported because it is use in ARM specific NUMA * helpers. See comment in arch/arm/include/asm/numa.h. */ +/* SAF-1-safe */ mfn_t first_valid_mfn = INVALID_MFN_INITIALIZER; struct bootmem_region {
No functional change. Signed-off-by: Nicola Vetrini <nicola.vetrini@bugseng.com> --- The preferred way to deviate is to use asmlinkage, but this modification is only the consequence of NUMA on ARM (and possibly PPC) being a work in progress. As stated in the comment above the textual deviation, first_valid_mfn will likely then become static and there would be no need for the comment anymore. This works towards having the analysis for this rule clean (i.e. no violations); the interest in having a clean rule is that then it could be used to signal newly introduced violations by making the analysis job fail. --- xen/common/page_alloc.c | 1 + 1 file changed, 1 insertion(+)