Re: [for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
Hi Oleksii, On 23/05/2024 09:04, Oleksii K. wrote: On Wed, 2024-05-22 at 21:50 +0100, Julien Grall wrote: Hi, Adding Oleksii as the release manager. On 22/05/2024 19:27, Tamas K Lengyel wrote: On Fri, May 10, 2024 at 8:32 AM 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 as well and provide stubs in asm/mem_access.h for the users of this header. Signed-off-by: Alessandro Zucchelli Acked-by: Tamas K Lengyel Oleksii, would you be happy if this patch is committed for 4.19? Sure: Release-acked-by: Oleksii Kurochko Thanks. It is now committed. BTW, do you want to be release-ack every bug until the hard code freeze? Or would you be fine to levea the decision to the maintainers? I would prefer to leave the decision to the maintainers. Ok. I will keep it in mind for the bug fixes until the hard code. Cheers, -- Julien Grall
Re: [for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
Hi Julien, On Wed, 2024-05-22 at 21:50 +0100, Julien Grall wrote: > Hi, > > Adding Oleksii as the release manager. > > On 22/05/2024 19:27, Tamas K Lengyel wrote: > > On Fri, May 10, 2024 at 8:32 AM 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 > > > as well and provide stubs in asm/mem_access.h for the users of > > > this > > > header. > > > > > > Signed-off-by: Alessandro Zucchelli > > > > > > > Acked-by: Tamas K Lengyel > > Oleksii, would you be happy if this patch is committed for 4.19? Sure: Release-acked-by: Oleksii Kurochko > > BTW, do you want to be release-ack every bug until the hard code > freeze? > Or would you be fine to levea the decision to the maintainers? I would prefer to leave the decision to the maintainers. ~ Oleksii
[for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
Hi, Adding Oleksii as the release manager. On 22/05/2024 19:27, Tamas K Lengyel wrote: On Fri, May 10, 2024 at 8:32 AM 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 as well and provide stubs in asm/mem_access.h for the users of this header. Signed-off-by: Alessandro Zucchelli Acked-by: Tamas K Lengyel Oleksii, would you be happy if this patch is committed for 4.19? BTW, do you want to be release-ack every bug until the hard code freeze? Or would you be fine to levea the decision to the maintainers? Cheers, -- Julien Grall
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
On Fri, May 10, 2024 at 8:32 AM 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 > as well and provide stubs in asm/mem_access.h for the users of this > header. > > Signed-off-by: Alessandro Zucchelli Acked-by: Tamas K Lengyel
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
On 2024-05-10 22:59, Julien Grall wrote:
Hi,
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,
since Julien is ok with the patch, with the commit message he proposed,
I think this needs an R-by or an A-by in order to commit for 4.19.
--
Nicola Vetrini, BSc
Software Engineer, BUGSENG srl (https://bugseng.com)
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
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
+
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,
--
Julien Grall
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
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
> +
> 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
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
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,
--
Alessandro Zucchelli, B.Sc.
Software Engineer, BUGSENG (https://bugseng.com)
Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
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,
--
Julien Grall
[XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c
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
---
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(-)
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
+
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 */
/*
--
2.25.1
