Re: [XEN PATCH v2] arm/mem_access: add conditional build of mem_access.c

[Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>]

On 2024-05-09 12:52, Julien Grall wrote:
> Hi,
>
> On 09/05/2024 11:39, Alessandro Zucchelli wrote:
> > In order to comply to MISRA C:2012 Rule 8.4 for ARM asm/mem_access.h 
> > in
> > the case where MEM_ACCESS=n stubs are needed to allow the conditional
> > compilation of the users of this header.
> I think you need to update the commit message given ...
>
> > Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@xxxxxxxxxxx>
> > ---
> > Changes from v1:
> > Reverted preprocessor conditional changes to xen/mem_access.h;
> > added conditional build for xen/mem_access.c;
> > provided stubs for asm/mem_access.h functions.
> > ---
> >   xen/arch/arm/Makefile                 | 2 +-
> >   xen/arch/arm/include/asm/mem_access.h | 9 +++++++++
> >   2 files changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
> > index 7b1350e2ef..45dc29ea53 100644
> > --- a/xen/arch/arm/Makefile
> > +++ b/xen/arch/arm/Makefile
> > @@ -37,7 +37,7 @@ obj-$(CONFIG_IOREQ_SERVER) += ioreq.o
> >   obj-y += irq.o
> >   obj-y += kernel.init.o
> >   obj-$(CONFIG_LIVEPATCH) += livepatch.o
> > -obj-y += mem_access.o
> > +obj-$(CONFIG_MEM_ACCESS) += mem_access.o
> ... this not only adding stub.
>
> >   obj-y += mm.o
> >   obj-y += monitor.o
> >   obj-y += p2m.o
> > diff --git a/xen/arch/arm/include/asm/mem_access.h 
> > b/xen/arch/arm/include/asm/mem_access.h
> > index 35ed0ad154..2f73172e39 100644
> > --- a/xen/arch/arm/include/asm/mem_access.h
> > +++ b/xen/arch/arm/include/asm/mem_access.h
> > @@ -17,6 +17,7 @@
> >   #ifndef _ASM_ARM_MEM_ACCESS_H
> >   #define _ASM_ARM_MEM_ACCESS_H
> >   +#include <xen/types.h>
> Can you explain why this is needed?
Without the inclusion of xen/types header NULL would be undefined.

> Style: Newline here please.
>
> >   static inline
> >   bool p2m_mem_access_emulate_check(struct vcpu *v,
> >                                     const struct vm_event_st *rsp)
> > @@ -35,12 +36,20 @@ static inline bool 
> > p2m_mem_access_sanity_check(struct domain *d)
> >    * Send mem event based on the access. Boolean return value 
> > indicates if trap
> >    * needs to be injected into guest.
> >    */
> > +#ifdef CONFIG_MEM_ACCESS
> >   bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct 
> > npfec npfec);
> >     struct page_info*
> >   p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
> >                                     const struct vcpu *v);
> > +#else
> >   +static inline bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, 
> > const struct npfec npfec) {return false;}
> Coding style: The line is well over the 80 characters limit. Also we 
> don't tend to provide the implementation of the stub in the single line 
> if it is more than {}. So "{return false;}" should look like:
> {
>    return false;
> }
>
> > +
> > +static inline struct page_info*
> > +p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag,
> > +                                  const struct vcpu *v) {return 
> > NULL;}
> Ditto.
>
> > +#endif /*CONFIG_MEM_ACCESS*/
> >   #endif /* _ASM_ARM_MEM_ACCESS_H */
> >     /*
Thanks for the feedback, I will soon provide the new version of the 
patch with the requested stylistic changes and a clearer description.
--
Alessandro Zucchelli, B.Sc.

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