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 12:26 PM Julien Grall <julien@xxxxxxx> 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@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.

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