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

[Julien Grall <julien@xxxxxxx>]

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@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?
In Xen, we tend prefer stubs over #ifdef-ing code blocks. I would rather 
use this approach here too.
Cheers,

--
Julien Grall