| Message ID | b3f03c4f5a78b86b01750f10bb0cebcdb2fd35cc.1715265720.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [XEN,v3] arm/mem_access: add conditional build of mem_access.c | expand |
Hi, On 10/05/2024 13:32, Alessandro Zucchelli wrote: > In order to comply to MISRA C:2012 Rule 8.4 for ARM the following > changes are done: > revert preprocessor conditional changes to xen/mem_access.h which > had it build unconditionally, add conditional build for xen/mem_access.c I am afraid, I don't understand this one as you don't seem to modify xen/mem_access.h. Is this meant to be part of the changelog? You also don't seem to mention the change in Makefile. This is the one I was asking for in the previous version. So what about: "xen/arm: mem_access: Conditionally compile mem_access.c Commit 634cfc8beb ("Make MEM_ACCESS configurable") intended to make MEM_ACCESS configurable on Arm to reduce the code size when the user doesn't need it. However, this didn't cover the arch specific code. None of the code in arm/mem_access.c is necessary when MEM_ACCESS=n, so it can be compiled out. This will require to provide some stub for functions called by the common code. This is also fixing violation of the MISRA C:2012 Rule 8.4 reported by ECLAIR. " The patch itself loks good so once we agree on the commit message, then I am happy to update it on commit. Cheers,
On 2024-05-10 22:59, Julien Grall wrote: > Hi, > > On 10/05/2024 13:32, Alessandro Zucchelli wrote: >> In order to comply to MISRA C:2012 Rule 8.4 for ARM the following >> changes are done: >> revert preprocessor conditional changes to xen/mem_access.h which >> had it build unconditionally, add conditional build for >> xen/mem_access.c > > I am afraid, I don't understand this one as you don't seem to modify > xen/mem_access.h. Is this meant to be part of the changelog? > > You also don't seem to mention the change in Makefile. This is the one > I was asking for in the previous version. So what about: > > "xen/arm: mem_access: Conditionally compile mem_access.c > > Commit 634cfc8beb ("Make MEM_ACCESS configurable") intended to make > MEM_ACCESS configurable on Arm to reduce the code size when the user > doesn't need it. > > However, this didn't cover the arch specific code. None of the code in > arm/mem_access.c is necessary when MEM_ACCESS=n, so it can be compiled > out. This will require to provide some stub for functions called by the > common code. > > This is also fixing violation of the MISRA C:2012 Rule 8.4 reported by > ECLAIR. > " > > The patch itself loks good so once we agree on the commit message, then > I am happy to update it on commit. Hi, Thanks for the feedback, > > I am afraid, I don't understand this one as you don't seem to modify > xen/mem_access.h. Is this meant to be part of the changelog? > you are right, this should be part of the changelog as it referes to the revert of a previous patch's changes. I approve of the commit message you provided. Cheers,
On 10.05.2024 14:32, Alessandro Zucchelli wrote: > --- a/xen/arch/arm/include/asm/mem_access.h > +++ b/xen/arch/arm/include/asm/mem_access.h > @@ -17,6 +17,8 @@ > #ifndef _ASM_ARM_MEM_ACCESS_H > #define _ASM_ARM_MEM_ACCESS_H > > +#include <xen/types.h> > + > static inline > bool p2m_mem_access_emulate_check(struct vcpu *v, > const struct vm_event_st *rsp) > @@ -35,12 +37,28 @@ 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; > +} > + > +static inline struct page_info* > +p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag, > + const struct vcpu *v) > +{ > + return NULL; > +} > > +#endif /*CONFIG_MEM_ACCESS*/ Why would each arch need to repeat these stubs? IOW why would they not live in xen/mem_access.h? Jan
Hi Jan, On 14/05/2024 09:10, Jan Beulich wrote: > On 10.05.2024 14:32, Alessandro Zucchelli wrote: >> --- a/xen/arch/arm/include/asm/mem_access.h >> +++ b/xen/arch/arm/include/asm/mem_access.h >> @@ -17,6 +17,8 @@ >> #ifndef _ASM_ARM_MEM_ACCESS_H >> #define _ASM_ARM_MEM_ACCESS_H >> >> +#include <xen/types.h> >> + >> static inline >> bool p2m_mem_access_emulate_check(struct vcpu *v, >> const struct vm_event_st *rsp) >> @@ -35,12 +37,28 @@ 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; >> +} >> + >> +static inline struct page_info* >> +p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag, >> + const struct vcpu *v) >> +{ >> + return NULL; >> +} >> >> +#endif /*CONFIG_MEM_ACCESS*/ > > Why would each arch need to repeat these stubs? IOW why would they not > live in xen/mem_access.h? Because they are not used by nor defined in common code. It is pure coincidence they are named the same. If at some point, some code can be shared, then yes I would agree it could be common. Cheers,
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 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..abac8032fc 100644 --- a/xen/arch/arm/include/asm/mem_access.h +++ b/xen/arch/arm/include/asm/mem_access.h @@ -17,6 +17,8 @@ #ifndef _ASM_ARM_MEM_ACCESS_H #define _ASM_ARM_MEM_ACCESS_H +#include <xen/types.h> + static inline bool p2m_mem_access_emulate_check(struct vcpu *v, const struct vm_event_st *rsp) @@ -35,12 +37,28 @@ 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; +} + +static inline struct page_info* +p2m_mem_access_check_and_get_page(vaddr_t gva, unsigned long flag, + const struct vcpu *v) +{ + return NULL; +} +#endif /*CONFIG_MEM_ACCESS*/ #endif /* _ASM_ARM_MEM_ACCESS_H */ /*
In order to comply to MISRA C:2012 Rule 8.4 for ARM the following changes are done: revert preprocessor conditional changes to xen/mem_access.h which had it build unconditionally, add conditional build for xen/mem_access.c as well and provide stubs in asm/mem_access.h for the users of this header. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- Changes from v2: Stylistic changes to code aimed to respect xen's coding guidelines. --- 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 | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-)