1. Patchew
  2. Xen
  3. View series

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

Alessandro Zucchelli posted 1 patch 6 months, 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/ad49a2006a6f19c2db1ff5eabb9ffd666693c4c5.1715250761.git.alessandro.zucchelli@bugseng.com
There is a newer version of this series
xen/arch/arm/Makefile                 | 2 +-
xen/arch/arm/include/asm/mem_access.h | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
[XEN PATCH v2] arm/mem_access: add conditional build of mem_access.c
Posted by Alessandro Zucchelli 6 months, 2 weeks ago 
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(-)

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 */
 
 /*
-- 
2.25.1
Re: [XEN PATCH v2] arm/mem_access: add conditional build of mem_access.c
Posted by Julien Grall 6 months, 2 weeks ago 
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,

-- 
Julien Grall
Re: [XEN PATCH v2] arm/mem_access: add conditional build of mem_access.c
Posted by Alessandro Zucchelli 6 months, 2 weeks ago 
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.
-- 
Alessandro Zucchelli, B.Sc.

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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.