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

[Alessandro Zucchelli <alessandro.zucchelli@bugseng.com>]

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.
-- 
Alessandro Zucchelli, B.Sc.

Software Engineer, BUGSENG (https://bugseng.com)