Re: [XEN PATCH 03/13] xen/arm: address violations of MISRA C:2012 Directive 4.10

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

Hi,

On Mon, 28 Aug 2023 at 09:20, Simone Ballarin <simone.ballarin@xxxxxxxxxxx> wrote:

Add or move inclusion guards to address violations of
MISRA C:2012 Directive 4.10 ("Precautions shall be taken in order
to prevent the contents of a header file being included more than
once").

Inclusion guards must appear at the beginning of the headers
(comments are permitted anywhere).

Mechanical change.

Signed-off-by: Simone Ballarin <simone.ballarin@xxxxxxxxxxx>
---
 xen/arch/arm/efi/efi-boot.h          | 6 ++++++
 xen/arch/arm/include/asm/hypercall.h | 6 +++---
 xen/arch/arm/include/asm/iocap.h     | 6 +++---
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/xen/arch/arm/efi/efi-boot.h b/xen/arch/arm/efi/efi-boot.h
index 1c3640bb65..aba522ead5 100644
--- a/xen/arch/arm/efi/efi-boot.h
+++ b/xen/arch/arm/efi/efi-boot.h
@@ -3,6 +3,10 @@
  * is intended to be included by common/efi/boot.c _only_, and
  * therefore can define arch specific global variables.
  */
+
+#ifndef __ARM_EFI_EFI_BOOT_H__
+#define __ARM_EFI_EFI_BOOT_H__
+
 #include <xen/device_tree.h>
 #include <xen/libfdt/libfdt.h>
 #include <asm/setup.h>
@@ -1003,6 +1007,8 @@ static void __init efi_arch_flush_dcache_area(const void *vaddr, UINTN size)
     __flush_dcache_area(vaddr, size);
 }

+#endif /* __ARM_EFI_EFI_BOOT_H__*/
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/include/asm/hypercall.h b/xen/arch/arm/include/asm/hypercall.h
index ccd26c5184..4f4d96f1f2 100644
--- a/xen/arch/arm/include/asm/hypercall.h
+++ b/xen/arch/arm/include/asm/hypercall.h
@@ -1,10 +1,10 @@
+#ifndef __ASM_ARM_HYPERCALL_H__
+#define __ASM_ARM_HYPERCALL_H__
+
 #ifndef __XEN_HYPERCALL_H__
 #error "asm/hypercall.h should not be included directly - include xen/hypercall.h instead"
 #endif

-#ifndef __ASM_ARM_HYPERCALL_H__
-#define __ASM_ARM_HYPERCALL_H__
-

I understand that you are trying to fix a misra violation. However, this feels like it was done on purpose.

With the new change, you would not always check that the file were included at the correct place. I am not against this change but this ought to be explained.

 #include <public/domctl.h> /* for arch_do_domctl */

 long subarch_do_domctl(struct xen_domctl *domctl, struct domain *d,
diff --git a/xen/arch/arm/include/asm/iocap.h b/xen/arch/arm/include/asm/iocap.h
index 276fefbc59..4db1b16839 100644
--- a/xen/arch/arm/include/asm/iocap.h
+++ b/xen/arch/arm/include/asm/iocap.h
@@ -1,10 +1,10 @@
-#ifndef __X86_IOCAP_H__
-#define __X86_IOCAP_H__
+#ifndef __ASM_ARM_IOCAP_H__
+#define __ASM_ARM_IOCAP_H__

 #define cache_flush_permitted(d)                        \
     (!rangeset_is_empty((d)->iomem_caps))

-#endif
+#endif /* __ASM_ARM_IOCAP_H__ */

I don’t understand how this is related to the rest of the patch. You wrote that inclusion must appear first and this is the case here.

However the name is technically not correct. Is this really related to directive 4.10? If so, this should be clarified in the commit message. If not, then I think this should be in a separate commit.

Cheers,

 /*
  * Local variables:
--
2.34.1