[PATCH] xen/arm: un-break build with clang

Stewart Hildebrand posted 1 patch 1 year, 6 months ago
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230525175115.110606-1-stewart.hildebrand@amd.com
There is a newer version of this series
xen/arch/arm/include/asm/lpae.h  | 4 ++++
xen/arch/arm/include/asm/setup.h | 8 ++++----
xen/include/xen/compiler.h       | 1 +
3 files changed, 9 insertions(+), 4 deletions(-)
[PATCH] xen/arm: un-break build with clang
Posted by Stewart Hildebrand 1 year, 6 months ago
clang doesn't like extern with __attribute__((__used__)):

  ./arch/arm/include/asm/setup.h:171:8: error: 'used' attribute ignored [-Werror,-Wignored-attributes]
  extern DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
         ^
  ./arch/arm/include/asm/lpae.h:273:29: note: expanded from macro 'DEFINE_BOOT_PAGE_TABLE'
  lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
                              ^
  ./include/xen/compiler.h:71:27: note: expanded from macro '__section'
  #define __section(s)      __used __attribute__((__section__(s)))
                            ^
  ./include/xen/compiler.h:104:39: note: expanded from macro '__used'
  #define __used         __attribute__((__used__))
                                        ^

Introduce a new EXTERN_DEFINE_BOOT_PAGE_TABLE macro without
__attribute__((__used__)) and use this for the relevant declarations.

Fixes: 1c78d76b67e1 ("xen/arm64: mm: Introduce helpers to prepare/enable/disable the identity mapping")
Signed-off-by: Stewart Hildebrand <stewart.hildebrand@amd.com>
---
I tested with clang 12 and clang 16

Here is my make command line:
make -j $(nproc) \
    clang=y \
    CC="clang --target=aarch64-none-linux-gnu -march=armv8a+nocrypto" \
    CXX="clang++ --target=aarch64-none-linux-gnu -march=armv8a+nocrypto" \
    HOSTCC=clang \
    HOSTCXX=clang++ \
    XEN_TARGET_ARCH=arm64 \
    CROSS_COMPILE=aarch64-none-linux-gnu- \
    dist-xen
---
 xen/arch/arm/include/asm/lpae.h  | 4 ++++
 xen/arch/arm/include/asm/setup.h | 8 ++++----
 xen/include/xen/compiler.h       | 1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/include/asm/lpae.h b/xen/arch/arm/include/asm/lpae.h
index 3fdd5d0de28e..294a8aa4bd30 100644
--- a/xen/arch/arm/include/asm/lpae.h
+++ b/xen/arch/arm/include/asm/lpae.h
@@ -273,6 +273,10 @@ lpae_t mfn_to_xen_entry(mfn_t mfn, unsigned int attr);
 lpae_t __aligned(PAGE_SIZE) __section(".data.page_aligned")                   \
     name[XEN_PT_LPAE_ENTRIES]
 
+#define EXTERN_DEFINE_BOOT_PAGE_TABLE(name)                                   \
+extern lpae_t __aligned(PAGE_SIZE) __no_used_section(".data.page_aligned")    \
+    name[XEN_PT_LPAE_ENTRIES]
+
 #define DEFINE_PAGE_TABLES(name, nr)                    \
 lpae_t __aligned(PAGE_SIZE) name[XEN_PT_LPAE_ENTRIES * (nr)]
 
diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
index 38e2ce255fcf..af53e58a6a07 100644
--- a/xen/arch/arm/include/asm/setup.h
+++ b/xen/arch/arm/include/asm/setup.h
@@ -168,13 +168,13 @@ u32 device_tree_get_u32(const void *fdt, int node,
 int map_range_to_domain(const struct dt_device_node *dev,
                         u64 addr, u64 len, void *data);
 
-extern DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
+EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
 
 #ifdef CONFIG_ARM_64
-extern DEFINE_BOOT_PAGE_TABLE(boot_first_id);
+EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_first_id);
 #endif
-extern DEFINE_BOOT_PAGE_TABLE(boot_second_id);
-extern DEFINE_BOOT_PAGE_TABLE(boot_third_id);
+EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_second_id);
+EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_third_id);
 
 /* Find where Xen will be residing at runtime and return a PT entry */
 lpae_t pte_of_xenaddr(vaddr_t);
diff --git a/xen/include/xen/compiler.h b/xen/include/xen/compiler.h
index 7d7ae2e5e4d9..70ba563e29c2 100644
--- a/xen/include/xen/compiler.h
+++ b/xen/include/xen/compiler.h
@@ -73,6 +73,7 @@
 #define __section(s)      __attribute__((__section__(s)))
 #endif
 #define __used_section(s) __used __attribute__((__section__(s)))
+#define __no_used_section(s) __attribute__((__section__(s)))
 #define __text_section(s) __attribute__((__section__(s)))
 
 #define __aligned(a) __attribute__((__aligned__(a)))
-- 
2.40.1
Re: [PATCH] xen/arm: un-break build with clang
Posted by Andrew Cooper 1 year, 6 months ago
On 25/05/2023 6:51 pm, Stewart Hildebrand wrote:
> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
> index 38e2ce255fcf..af53e58a6a07 100644
> --- a/xen/arch/arm/include/asm/setup.h
> +++ b/xen/arch/arm/include/asm/setup.h
> @@ -168,13 +168,13 @@ u32 device_tree_get_u32(const void *fdt, int node,
>  int map_range_to_domain(const struct dt_device_node *dev,
>                          u64 addr, u64 len, void *data);
>  
> -extern DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
> +EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_pgtable);

The problem is using DEFINE_$blah() when you mean DECLARE_$blah(). 
They're split everywhere else in Xen for good reason.

But the macro looks like pure obfuscation to start with.  It should just
be a simple

extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];

The declaration shouldn't have an alignment or section attribute on, and
deleting the macro makes the header easier to read.

~Andrew

Re: [PATCH] xen/arm: un-break build with clang
Posted by Stewart Hildebrand 1 year, 6 months ago
On 5/25/23 14:05, Andrew Cooper wrote:
> On 25/05/2023 6:51 pm, Stewart Hildebrand wrote:
>> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h
>> index 38e2ce255fcf..af53e58a6a07 100644
>> --- a/xen/arch/arm/include/asm/setup.h
>> +++ b/xen/arch/arm/include/asm/setup.h
>> @@ -168,13 +168,13 @@ u32 device_tree_get_u32(const void *fdt, int node,
>>  int map_range_to_domain(const struct dt_device_node *dev,
>>                          u64 addr, u64 len, void *data);
>>
>> -extern DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
>> +EXTERN_DEFINE_BOOT_PAGE_TABLE(boot_pgtable);
> 
> The problem is using DEFINE_$blah() when you mean DECLARE_$blah().
> They're split everywhere else in Xen for good reason.
> 
> But the macro looks like pure obfuscation to start with.  It should just
> be a simple
> 
> extern lpae_t boot_pgtable[XEN_PT_LPAE_ENTRIES];
> 
> The declaration shouldn't have an alignment or section attribute on, and
> deleting the macro makes the header easier to read.

This indeed makes much more sense. I will send v2 with simplified extern declarations.

To clarify, the definitions in xen/arch/arm/mm.c are to remain unchanged.