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

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

[Alessandro Zucchelli]

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(-)

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 */
 
 /*
-- 
2.25.1

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

[Julien Grall]

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

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

[Alessandro Zucchelli]

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

[Nicola Vetrini]

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

[Jan Beulich]

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

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

[Julien Grall]

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,

-- 
Julien Grall

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

[Tamas K Lengyel]

On Fri, May 10, 2024 at 8:32 AM Alessandro Zucchelli
<alessandro.zucchelli@bugseng.com> 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 <alessandro.zucchelli@bugseng.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

[for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c

[Julien Grall]

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
> <alessandro.zucchelli@bugseng.com> 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 <alessandro.zucchelli@bugseng.com>
> 
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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: [for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c

[Oleksii K.]

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
> > <alessandro.zucchelli@bugseng.com> 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
> > > <alessandro.zucchelli@bugseng.com>
> > 
> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
> Oleksii, would you be happy if this patch is committed for 4.19?
Sure:
 Release-acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

> 
> 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

Re: [for-4.19] Re: [XEN PATCH v3] arm/mem_access: add conditional build of mem_access.c

[Julien Grall]

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
>>> <alessandro.zucchelli@bugseng.com> 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
>>>> <alessandro.zucchelli@bugseng.com>
>>>
>>> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>>
>> Oleksii, would you be happy if this patch is committed for 4.19?
> Sure:
>   Release-acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>

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