Re: [XEN PATCH] xen/mem_access: address violations of MISRA C: 2012 Rule 8.4

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Wed, May 8, 2024 at 10:02 AM Alessandro Zucchelli
<alessandro.zucchelli@xxxxxxxxxxx> 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@xxxxxxxxxxx>
> >>>> ---
> >>>>  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