| Message ID | 20230525175115.110606-1-stewart.hildebrand@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm: un-break build with clang | expand |
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
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.
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)))
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(-)