On Wed, May 8, 2024 at 12:26 PM Julien Grall <jul...@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.zucche...@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.zucche...@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
