| Message ID | a3d4e07433932624266ac9b675daf0b70734696d.1714405386.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [XEN] xen/mem_access: address violations of MISRA C: 2012 Rule 8.4 | expand |
On 29.04.2024 17:45, Alessandro Zucchelli wrote: > Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), > allowing asm/mem_access.h to be included in all ARM build configurations. > This is to address the violation of MISRA C: 2012 Rule 8.4 which states: > "A compatible declaration shall be visible when an object or function > with external linkage is defined". Functions p2m_mem_access_check > and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not > defined in ARM builds don't have visible declarations in the file > containing their definitions. > > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> > --- > xen/include/xen/mem_access.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h > index 87d93b31f6..ec0630677d 100644 > --- a/xen/include/xen/mem_access.h > +++ b/xen/include/xen/mem_access.h > @@ -33,7 +33,7 @@ > */ > struct vm_event_st; > > -#ifdef CONFIG_MEM_ACCESS > +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) > #include <asm/mem_access.h> > #endif This doesn't look quite right. If Arm supports mem-access, why would it not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, then those would better move here, thus eliminating the need for a per-arch stub header (see what was e.g. done for numa.h). This way RISC-V and PPC (and whatever is to come) would then be taken care of as well. Jan
On 2024-04-29 17:58, Jan Beulich wrote: > On 29.04.2024 17:45, Alessandro Zucchelli wrote: >> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), >> allowing asm/mem_access.h to be included in all ARM build >> configurations. >> This is to address the violation of MISRA C: 2012 Rule 8.4 which >> states: >> "A compatible declaration shall be visible when an object or function >> with external linkage is defined". Functions p2m_mem_access_check >> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not >> defined in ARM builds don't have visible declarations in the file >> containing their definitions. >> >> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> >> --- >> xen/include/xen/mem_access.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/xen/include/xen/mem_access.h >> b/xen/include/xen/mem_access.h >> index 87d93b31f6..ec0630677d 100644 >> --- a/xen/include/xen/mem_access.h >> +++ b/xen/include/xen/mem_access.h >> @@ -33,7 +33,7 @@ >> */ >> struct vm_event_st; >> >> -#ifdef CONFIG_MEM_ACCESS >> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) >> #include <asm/mem_access.h> >> #endif > > This doesn't look quite right. If Arm supports mem-access, why would it > not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, > then > those would better move here, thus eliminating the need for a per-arch > stub header (see what was e.g. done for numa.h). This way RISC-V and > PPC > (and whatever is to come) would then be taken care of as well. > ARM does support mem-access, so I don't think this is akin to the changes done to handle numa.h. ARM also allows users to set MEM_ACCESS=n (e.g. xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, the implementation file mem_access.c is compiled unconditionally in ARM's makefile, hence why the violation was spotted. This is a bit unusual, so I was also hoping to get some feedback from mem-access maintainers as to why this discrepancy from x86 exists. I probably should have also included some ARM maintainers as well, so I'm going to loop them in now. An alternative option I think is to make the compilation of arm's mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and common).
Hi, On 03/05/2024 08:09, Alessandro Zucchelli wrote: > On 2024-04-29 17:58, Jan Beulich wrote: >> On 29.04.2024 17:45, Alessandro Zucchelli wrote: >>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), >>> allowing asm/mem_access.h to be included in all ARM build >>> configurations. >>> This is to address the violation of MISRA C: 2012 Rule 8.4 which states: >>> "A compatible declaration shall be visible when an object or function >>> with external linkage is defined". Functions p2m_mem_access_check >>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not >>> defined in ARM builds don't have visible declarations in the file >>> containing their definitions. >>> >>> Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> >>> --- >>> xen/include/xen/mem_access.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h >>> index 87d93b31f6..ec0630677d 100644 >>> --- a/xen/include/xen/mem_access.h >>> +++ b/xen/include/xen/mem_access.h >>> @@ -33,7 +33,7 @@ >>> */ >>> struct vm_event_st; >>> >>> -#ifdef CONFIG_MEM_ACCESS >>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) >>> #include <asm/mem_access.h> >>> #endif >> >> This doesn't look quite right. If Arm supports mem-access, why would it >> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, then >> those would better move here, thus eliminating the need for a per-arch >> stub header (see what was e.g. done for numa.h). This way RISC-V and PPC >> (and whatever is to come) would then be taken care of as well. >> > ARM does support mem-access, so I don't think this is akin to the > changes done to handle numa.h. > ARM also allows users to set MEM_ACCESS=n (e.g. > xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, > the implementation file mem_access.c is compiled unconditionally in > ARM's makefile, hence why the violation was spotted. > This is a bit unusual, so I was also hoping to get some feedback from > mem-access maintainers as to why this discrepancy from x86 exists. I > probably should have also included some ARM maintainers as well, so I'm > going to loop them in now. > > An alternative option I think is to make the compilation of arm's > mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and common). I can't think of a reason to have mem_access.c unconditional compiled in. So I think it should be conditional like on x86. Cheers,
On 2024-05-03 11:32, Julien Grall wrote: > Hi, > > On 03/05/2024 08:09, Alessandro Zucchelli wrote: >> On 2024-04-29 17:58, Jan Beulich wrote: >>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: >>>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), >>>> allowing asm/mem_access.h to be included in all ARM build >>>> configurations. >>>> This is to address the violation of MISRA C: 2012 Rule 8.4 which >>>> states: >>>> "A compatible declaration shall be visible when an object or >>>> function >>>> with external linkage is defined". Functions p2m_mem_access_check >>>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not >>>> defined in ARM builds don't have visible declarations in the file >>>> containing their definitions. >>>> >>>> Signed-off-by: Alessandro Zucchelli >>>> <alessandro.zucchelli@bugseng.com> >>>> --- >>>> xen/include/xen/mem_access.h | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/xen/include/xen/mem_access.h >>>> b/xen/include/xen/mem_access.h >>>> index 87d93b31f6..ec0630677d 100644 >>>> --- a/xen/include/xen/mem_access.h >>>> +++ b/xen/include/xen/mem_access.h >>>> @@ -33,7 +33,7 @@ >>>> */ >>>> struct vm_event_st; >>>> >>>> -#ifdef CONFIG_MEM_ACCESS >>>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) >>>> #include <asm/mem_access.h> >>>> #endif >>> >>> This doesn't look quite right. If Arm supports mem-access, why would >>> it >>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, >>> then >>> those would better move here, thus eliminating the need for a >>> per-arch >>> stub header (see what was e.g. done for numa.h). This way RISC-V and >>> PPC >>> (and whatever is to come) would then be taken care of as well. >>> >> ARM does support mem-access, so I don't think this is akin to the >> changes done to handle numa.h. >> ARM also allows users to set MEM_ACCESS=n (e.g. >> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, >> the implementation file mem_access.c is compiled unconditionally in >> ARM's makefile, hence why the violation was spotted. >> This is a bit unusual, so I was also hoping to get some feedback from >> mem-access maintainers as to why this discrepancy from x86 exists. I >> probably should have also included some ARM maintainers as well, so >> I'm going to loop them in now. >> >> An alternative option I think is to make the compilation of arm's >> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and >> common). > > I can't think of a reason to have mem_access.c unconditional compiled > in. So I think it should be conditional like on x86. Hi, attempting to build ARM with a configuration where MEM_ACCESS=n and mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as there are other pieces of code that call some mem_access.c functions (p2m_mem_access_check_and_get_page, p2m_mem_access_check). In a Matrix chat Julien was suggesting adding stubs for the functions for this use case.
On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> wrote: > > On 2024-05-03 11:32, Julien Grall wrote: > > Hi, > > > > On 03/05/2024 08:09, Alessandro Zucchelli wrote: > >> On 2024-04-29 17:58, Jan Beulich wrote: > >>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: > >>>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), > >>>> allowing asm/mem_access.h to be included in all ARM build > >>>> configurations. > >>>> This is to address the violation of MISRA C: 2012 Rule 8.4 which > >>>> states: > >>>> "A compatible declaration shall be visible when an object or > >>>> function > >>>> with external linkage is defined". Functions p2m_mem_access_check > >>>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not > >>>> defined in ARM builds don't have visible declarations in the file > >>>> containing their definitions. > >>>> > >>>> Signed-off-by: Alessandro Zucchelli > >>>> <alessandro.zucchelli@bugseng.com> > >>>> --- > >>>> xen/include/xen/mem_access.h | 2 +- > >>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/xen/include/xen/mem_access.h > >>>> b/xen/include/xen/mem_access.h > >>>> index 87d93b31f6..ec0630677d 100644 > >>>> --- a/xen/include/xen/mem_access.h > >>>> +++ b/xen/include/xen/mem_access.h > >>>> @@ -33,7 +33,7 @@ > >>>> */ > >>>> struct vm_event_st; > >>>> > >>>> -#ifdef CONFIG_MEM_ACCESS > >>>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) > >>>> #include <asm/mem_access.h> > >>>> #endif > >>> > >>> This doesn't look quite right. If Arm supports mem-access, why would > >>> it > >>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, > >>> then > >>> those would better move here, thus eliminating the need for a > >>> per-arch > >>> stub header (see what was e.g. done for numa.h). This way RISC-V and > >>> PPC > >>> (and whatever is to come) would then be taken care of as well. > >>> > >> ARM does support mem-access, so I don't think this is akin to the > >> changes done to handle numa.h. > >> ARM also allows users to set MEM_ACCESS=n (e.g. > >> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, > >> the implementation file mem_access.c is compiled unconditionally in > >> ARM's makefile, hence why the violation was spotted. > >> This is a bit unusual, so I was also hoping to get some feedback from > >> mem-access maintainers as to why this discrepancy from x86 exists. I > >> probably should have also included some ARM maintainers as well, so > >> I'm going to loop them in now. > >> > >> An alternative option I think is to make the compilation of arm's > >> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and > >> common). > > > > I can't think of a reason to have mem_access.c unconditional compiled > > in. So I think it should be conditional like on x86. > > Hi, > attempting to build ARM with a configuration where MEM_ACCESS=n and > mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as > there are other pieces of code that call some mem_access.c functions > (p2m_mem_access_check_and_get_page, p2m_mem_access_check). > In a Matrix chat Julien was suggesting adding stubs for the functions > for this use case. Perhaps just wrap the callers into #ifdef CONFIG_MEM_ACCESS blocks? Tamas
Hi Tamas, On 08/05/2024 17:01, Tamas K Lengyel wrote: > On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli > <alessandro.zucchelli@bugseng.com> wrote: >> >> On 2024-05-03 11:32, Julien Grall wrote: >>> Hi, >>> >>> On 03/05/2024 08:09, Alessandro Zucchelli wrote: >>>> On 2024-04-29 17:58, Jan Beulich wrote: >>>>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: >>>>>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), >>>>>> allowing asm/mem_access.h to be included in all ARM build >>>>>> configurations. >>>>>> This is to address the violation of MISRA C: 2012 Rule 8.4 which >>>>>> states: >>>>>> "A compatible declaration shall be visible when an object or >>>>>> function >>>>>> with external linkage is defined". Functions p2m_mem_access_check >>>>>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not >>>>>> defined in ARM builds don't have visible declarations in the file >>>>>> containing their definitions. >>>>>> >>>>>> Signed-off-by: Alessandro Zucchelli >>>>>> <alessandro.zucchelli@bugseng.com> >>>>>> --- >>>>>> xen/include/xen/mem_access.h | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/xen/include/xen/mem_access.h >>>>>> b/xen/include/xen/mem_access.h >>>>>> index 87d93b31f6..ec0630677d 100644 >>>>>> --- a/xen/include/xen/mem_access.h >>>>>> +++ b/xen/include/xen/mem_access.h >>>>>> @@ -33,7 +33,7 @@ >>>>>> */ >>>>>> struct vm_event_st; >>>>>> >>>>>> -#ifdef CONFIG_MEM_ACCESS >>>>>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) >>>>>> #include <asm/mem_access.h> >>>>>> #endif >>>>> >>>>> This doesn't look quite right. If Arm supports mem-access, why would >>>>> it >>>>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, >>>>> then >>>>> those would better move here, thus eliminating the need for a >>>>> per-arch >>>>> stub header (see what was e.g. done for numa.h). This way RISC-V and >>>>> PPC >>>>> (and whatever is to come) would then be taken care of as well. >>>>> >>>> ARM does support mem-access, so I don't think this is akin to the >>>> changes done to handle numa.h. >>>> ARM also allows users to set MEM_ACCESS=n (e.g. >>>> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, >>>> the implementation file mem_access.c is compiled unconditionally in >>>> ARM's makefile, hence why the violation was spotted. >>>> This is a bit unusual, so I was also hoping to get some feedback from >>>> mem-access maintainers as to why this discrepancy from x86 exists. I >>>> probably should have also included some ARM maintainers as well, so >>>> I'm going to loop them in now. >>>> >>>> An alternative option I think is to make the compilation of arm's >>>> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and >>>> common). >>> >>> I can't think of a reason to have mem_access.c unconditional compiled >>> in. So I think it should be conditional like on x86. >> >> Hi, >> attempting to build ARM with a configuration where MEM_ACCESS=n and >> mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as >> there are other pieces of code that call some mem_access.c functions >> (p2m_mem_access_check_and_get_page, p2m_mem_access_check). >> In a Matrix chat Julien was suggesting adding stubs for the functions >> for this use case. > > Perhaps just wrap the callers into #ifdef CONFIG_MEM_ACCESS blocks? In Xen, we tend prefer stubs over #ifdef-ing code blocks. I would rather use this approach here too. Cheers,
On Wed, May 8, 2024 at 12:26 PM Julien Grall <julien@xen.org> wrote: > > Hi Tamas, > > On 08/05/2024 17:01, Tamas K Lengyel wrote: > > On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli > > <alessandro.zucchelli@bugseng.com> wrote: > >> > >> On 2024-05-03 11:32, Julien Grall wrote: > >>> Hi, > >>> > >>> On 03/05/2024 08:09, Alessandro Zucchelli wrote: > >>>> On 2024-04-29 17:58, Jan Beulich wrote: > >>>>> On 29.04.2024 17:45, Alessandro Zucchelli wrote: > >>>>>> Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), > >>>>>> allowing asm/mem_access.h to be included in all ARM build > >>>>>> configurations. > >>>>>> This is to address the violation of MISRA C: 2012 Rule 8.4 which > >>>>>> states: > >>>>>> "A compatible declaration shall be visible when an object or > >>>>>> function > >>>>>> with external linkage is defined". Functions p2m_mem_access_check > >>>>>> and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not > >>>>>> defined in ARM builds don't have visible declarations in the file > >>>>>> containing their definitions. > >>>>>> > >>>>>> Signed-off-by: Alessandro Zucchelli > >>>>>> <alessandro.zucchelli@bugseng.com> > >>>>>> --- > >>>>>> xen/include/xen/mem_access.h | 2 +- > >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>>>>> > >>>>>> diff --git a/xen/include/xen/mem_access.h > >>>>>> b/xen/include/xen/mem_access.h > >>>>>> index 87d93b31f6..ec0630677d 100644 > >>>>>> --- a/xen/include/xen/mem_access.h > >>>>>> +++ b/xen/include/xen/mem_access.h > >>>>>> @@ -33,7 +33,7 @@ > >>>>>> */ > >>>>>> struct vm_event_st; > >>>>>> > >>>>>> -#ifdef CONFIG_MEM_ACCESS > >>>>>> +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) > >>>>>> #include <asm/mem_access.h> > >>>>>> #endif > >>>>> > >>>>> This doesn't look quite right. If Arm supports mem-access, why would > >>>>> it > >>>>> not set MEM_ACCESS=y? Whereas if it's only stubs that Arm supplies, > >>>>> then > >>>>> those would better move here, thus eliminating the need for a > >>>>> per-arch > >>>>> stub header (see what was e.g. done for numa.h). This way RISC-V and > >>>>> PPC > >>>>> (and whatever is to come) would then be taken care of as well. > >>>>> > >>>> ARM does support mem-access, so I don't think this is akin to the > >>>> changes done to handle numa.h. > >>>> ARM also allows users to set MEM_ACCESS=n (e.g. > >>>> xen/arch/arm/configs/tiny64_defconfig) and builds just fine; however, > >>>> the implementation file mem_access.c is compiled unconditionally in > >>>> ARM's makefile, hence why the violation was spotted. > >>>> This is a bit unusual, so I was also hoping to get some feedback from > >>>> mem-access maintainers as to why this discrepancy from x86 exists. I > >>>> probably should have also included some ARM maintainers as well, so > >>>> I'm going to loop them in now. > >>>> > >>>> An alternative option I think is to make the compilation of arm's > >>>> mem_access.c conditional on CONFIG_MEM_ACCESS (as for x86/mm and > >>>> common). > >>> > >>> I can't think of a reason to have mem_access.c unconditional compiled > >>> in. So I think it should be conditional like on x86. > >> > >> Hi, > >> attempting to build ARM with a configuration where MEM_ACCESS=n and > >> mem_access.c is conditioned on CONFIG_MEM_ACCESS results in a fail as > >> there are other pieces of code that call some mem_access.c functions > >> (p2m_mem_access_check_and_get_page, p2m_mem_access_check). > >> In a Matrix chat Julien was suggesting adding stubs for the functions > >> for this use case. > > > > Perhaps just wrap the callers into #ifdef CONFIG_MEM_ACCESS blocks? > > In Xen, we tend prefer stubs over #ifdef-ing code blocks. I would rather > use this approach here too. I was looking at arch/x86/hvm/hvm.c for examples on how MEM_PAGING and MEM_SHARING calls are handled and those were ifdef'd. I have no preference for one vs the other, both work. Tamas
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h index 87d93b31f6..ec0630677d 100644 --- a/xen/include/xen/mem_access.h +++ b/xen/include/xen/mem_access.h @@ -33,7 +33,7 @@ */ struct vm_event_st; -#ifdef CONFIG_MEM_ACCESS +#if defined(CONFIG_MEM_ACCESS) || defined(CONFIG_ARM) #include <asm/mem_access.h> #endif
Change #ifdef CONFIG_MEM_ACCESS by OR-ing defined(CONFIG_ARM), allowing asm/mem_access.h to be included in all ARM build configurations. This is to address the violation of MISRA C: 2012 Rule 8.4 which states: "A compatible declaration shall be visible when an object or function with external linkage is defined". Functions p2m_mem_access_check and p2m_mem_access_check_and_get_page when CONFIG_MEM_ACCESS is not defined in ARM builds don't have visible declarations in the file containing their definitions. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- xen/include/xen/mem_access.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)