| Message ID | 20250128094002.145755-2-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm CONFIG_PHYS_ADDR_T_32=y fixes | expand |
Hi Michal, Julien, > On 28 Jan 2025, at 09:40, Michal Orzel <michal.orzel@amd.com> wrote: > > On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed: > common/device-tree/bootfdt.c: In function 'build_assertions': > ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)" > 47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) > | ^~~~~~~~~~~~~~ > common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON' > 31 | BUILD_BUG_ON(alignof(struct membanks) != 8); > > When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long, > therefore the struct membanks alignment is 4B and not 8B. The check is > there to ensure the struct membanks and struct membank, which is a > member of the former, are equally aligned. Therefore modify the check to > compare alignments obtained via alignof not to rely on hardcoded > values. > > Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> > --- > Changes in v2: > - modify the check to test against alignment of struct membank > --- > xen/common/device-tree/bootfdt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c > index 47386d4fffea..529c91e603ab 100644 > --- a/xen/common/device-tree/bootfdt.c > +++ b/xen/common/device-tree/bootfdt.c > @@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void) > */ > BUILD_BUG_ON((offsetof(struct membanks, bank) != > offsetof(struct meminfo, bank))); > - /* Ensure "struct membanks" is 8-byte aligned */ > - BUILD_BUG_ON(alignof(struct membanks) != 8); > + /* Ensure "struct membanks" and "struct membank" are equally aligned */ > + BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); Apologies for not catching the issue for your v1, probably I didn't understand very well the test itself, why are we checking that the structure have the same alignment? I see we check the offset of `(struct membanks).bank` against `(struct meminfo|struct shared_meminfo).bank`, isn't that enough? For sure I’m missing something... Anyway I tested this: Tested-by: Luca Fancellu <luca.fancellu@arm.com> > } > > static bool __init device_tree_node_is_available(const void *fdt, int node) > -- > 2.25.1 > >
On 28/01/2025 11:34, Luca Fancellu wrote: > > > Hi Michal, Julien, > >> On 28 Jan 2025, at 09:40, Michal Orzel <michal.orzel@amd.com> wrote: >> >> On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed: >> common/device-tree/bootfdt.c: In function 'build_assertions': >> ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)" >> 47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) >> | ^~~~~~~~~~~~~~ >> common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON' >> 31 | BUILD_BUG_ON(alignof(struct membanks) != 8); >> >> When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long, >> therefore the struct membanks alignment is 4B and not 8B. The check is >> there to ensure the struct membanks and struct membank, which is a >> member of the former, are equally aligned. Therefore modify the check to >> compare alignments obtained via alignof not to rely on hardcoded >> values. >> >> Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") >> Signed-off-by: Michal Orzel <michal.orzel@amd.com> >> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> >> --- >> Changes in v2: >> - modify the check to test against alignment of struct membank >> --- >> xen/common/device-tree/bootfdt.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c >> index 47386d4fffea..529c91e603ab 100644 >> --- a/xen/common/device-tree/bootfdt.c >> +++ b/xen/common/device-tree/bootfdt.c >> @@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void) >> */ >> BUILD_BUG_ON((offsetof(struct membanks, bank) != >> offsetof(struct meminfo, bank))); >> - /* Ensure "struct membanks" is 8-byte aligned */ >> - BUILD_BUG_ON(alignof(struct membanks) != 8); >> + /* Ensure "struct membanks" and "struct membank" are equally aligned */ >> + BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); > > Apologies for not catching the issue for your v1, probably I didn't understand very well the test itself, > why are we checking that the structure have the same alignment? > I see we check the offset of `(struct membanks).bank` against `(struct meminfo|struct shared_meminfo).bank`, > isn't that enough? > For sure I’m missing something... In my understanding it's to prevent issues when casting between these structures. It's not that FAM (flexible array member) requires the same alignment but if you consider casting this member to a type with a stricter alignment requirement you may encounter alignment issues. > > Anyway I tested this: > > Tested-by: Luca Fancellu <luca.fancellu@arm.com> > ~Michal
On Tue, 28 Jan 2025 at 06:40, Michal Orzel <michal.orzel@amd.com> wrote: > On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed: > common/device-tree/bootfdt.c: In function 'build_assertions': > ./include/xen/macros.h:47:31: error: static assertion failed: > "!(alignof(struct membanks) != 8)" > 47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond > ")"); }) > | ^~~~~~~~~~~~~~ > common/device-tree/bootfdt.c:31:5: note: in expansion of macro > 'BUILD_BUG_ON' > 31 | BUILD_BUG_ON(alignof(struct membanks) != 8); > > When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long, > therefore the struct membanks alignment is 4B and not 8B. The check is > there to ensure the struct membanks and struct membank, which is a > member of the former, are equally aligned. Therefore modify the check to > compare alignments obtained via alignof not to rely on hardcoded > values. > > Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory > bank structures") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> Reviewed-by: Julien Grall <julien@xen.org> Cheers, > --- > Changes in v2: > - modify the check to test against alignment of struct membank > --- > xen/common/device-tree/bootfdt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/xen/common/device-tree/bootfdt.c > b/xen/common/device-tree/bootfdt.c > index 47386d4fffea..529c91e603ab 100644 > --- a/xen/common/device-tree/bootfdt.c > +++ b/xen/common/device-tree/bootfdt.c > @@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void) > */ > BUILD_BUG_ON((offsetof(struct membanks, bank) != > offsetof(struct meminfo, bank))); > - /* Ensure "struct membanks" is 8-byte aligned */ > - BUILD_BUG_ON(alignof(struct membanks) != 8); > + /* Ensure "struct membanks" and "struct membank" are equally aligned > */ > + BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); > } > > static bool __init device_tree_node_is_available(const void *fdt, int > node) > -- > 2.25.1 > >
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c index 47386d4fffea..529c91e603ab 100644 --- a/xen/common/device-tree/bootfdt.c +++ b/xen/common/device-tree/bootfdt.c @@ -27,8 +27,8 @@ static void __init __maybe_unused build_assertions(void) */ BUILD_BUG_ON((offsetof(struct membanks, bank) != offsetof(struct meminfo, bank))); - /* Ensure "struct membanks" is 8-byte aligned */ - BUILD_BUG_ON(alignof(struct membanks) != 8); + /* Ensure "struct membanks" and "struct membank" are equally aligned */ + BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); } static bool __init device_tree_node_is_available(const void *fdt, int node)
On Arm32, when CONFIG_PHYS_ADDR_T_32 is set, a build failure is observed: common/device-tree/bootfdt.c: In function 'build_assertions': ./include/xen/macros.h:47:31: error: static assertion failed: "!(alignof(struct membanks) != 8)" 47 | #define BUILD_BUG_ON(cond) ({ _Static_assert(!(cond), "!(" #cond ")"); }) | ^~~~~~~~~~~~~~ common/device-tree/bootfdt.c:31:5: note: in expansion of macro 'BUILD_BUG_ON' 31 | BUILD_BUG_ON(alignof(struct membanks) != 8); When CONFIG_PHYS_ADDR_T_32 is set, paddr_t is defined as unsigned long, therefore the struct membanks alignment is 4B and not 8B. The check is there to ensure the struct membanks and struct membank, which is a member of the former, are equally aligned. Therefore modify the check to compare alignments obtained via alignof not to rely on hardcoded values. Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") Signed-off-by: Michal Orzel <michal.orzel@amd.com> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in v2: - modify the check to test against alignment of struct membank --- xen/common/device-tree/bootfdt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)