| Message ID | ad49a2006a6f19c2db1ff5eabb9ffd666693c4c5.1715250761.git.alessandro.zucchelli@bugseng.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [XEN,v2] arm/mem_access: add conditional build of mem_access.c | expand |
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@bugseng.com> > --- > 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? 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 */ > > /* Cheers,
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@bugseng.com> >> --- >> 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.
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..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> 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;} + +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 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. Signed-off-by: Alessandro Zucchelli <alessandro.zucchelli@bugseng.com> --- 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(-)