| Message ID | 20250127104556.175641-2-michal.orzel@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/arm CONFIG_PHYS_ADDR_T_32=y fixes | expand |
On Mon, 27 Jan 2025 at 07:46, 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. Fix it. Usually, we add a BUILD_BUG_ON when other parts of the code rely on a specific property (in this case alignment). Can you explain in the commit message why the new check is still ok? Cheers, > > Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory > bank structures") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- > 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..511700ccc2ba 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" is paddr aligned */ > + BUILD_BUG_ON(alignof(struct membanks) != sizeof(paddr_t)); > } > > static bool __init device_tree_node_is_available(const void *fdt, int > node) > -- > 2.25.1 > >
On 27/01/2025 12:19, Julien Grall wrote: > > > > > > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com <mailto: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. Fix it. > > > Usually, we add a BUILD_BUG_ON when other parts of the code rely on a specific property (in this case alignment). Can you explain in the commit message why the new check is still ok? Well, the change itself reflects the change in alignment requirement. When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B. On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B. AFAICT you requested this check back then, because struct membanks contains flexible array member 'bank' of type struct membank. The alignment requirement of struct membanks becomes the requirement of struct membank whose largest type is paddr_t. Let me know how you would like to have this written in commit msg. ~Michal
Hi Michal, > On 27 Jan 2025, at 10:45, 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. Fix it. > > Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") > Signed-off-by: Michal Orzel <michal.orzel@amd.com> > --- Apart from Julien’s comments for which I’ll leave you both to agree on a solution, I’ve reproduced the issue and tested that your change is fixing it and it’s not breaking a different setup (e.g. 64 bit). Reviewed-by: Luca Fancellu <luca.fancellu@arm.com> Tested-by: Luca Fancellu <luca.fancellu@arm.com>
On Mon, 27 Jan 2025 at 09:52, Michal Orzel <michal.orzel@amd.com> wrote: > > > On 27/01/2025 12:19, Julien Grall wrote: > > > > > > > > > > > > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com > <mailto: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. Fix it. > > > > > > Usually, we add a BUILD_BUG_ON when other parts of the code rely on a > specific property (in this case alignment). Can you explain in the commit > message why the new check is still ok? > Well, the change itself reflects the change in alignment requirement. > When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B. > On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B. > > AFAICT you requested this check back then, because struct membanks > contains flexible array member 'bank' of type struct membank. > The alignment requirement of struct membanks becomes the requirement of > struct membank whose largest type is paddr_t. Reading this, it sounds like you want to check against the alignment of « struct membank ». This is because the structure could gain a 64-bit field in the future and this would not be caught by the BUILD_BUG_ON. > Let me know how you would like to have this written in commit msg. I think it needs to be rephrased to make clear the alignment of struct membanks should be the same as struct membank. Cheers, > > ~Michal > >
On 27/01/2025 17:27, Julien Grall wrote: > > > > > > On Mon, 27 Jan 2025 at 09:52, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@amd.com>> wrote: > > > > On 27/01/2025 12:19, Julien Grall wrote: > > > > > > > > > > > > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@amd.com <mailto:michal.orzel@amd.com> <mailto:michal.orzel@amd.com <mailto: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. Fix it. > > > > > > Usually, we add a BUILD_BUG_ON when other parts of the code rely on a specific property (in this case alignment). Can you explain in the commit message why the new check is still ok? > Well, the change itself reflects the change in alignment requirement. > When paddr_t is 64b (Arm64, Arm32 with PA=40b) the alignment is 8B. > On Arm32 with PA=32b, the alignment is 4B because paddr_t is 4B. > > AFAICT you requested this check back then, because struct membanks contains flexible array member 'bank' of type struct membank. > The alignment requirement of struct membanks becomes the requirement of struct membank whose largest type is paddr_t. > > > Reading this, it sounds like you want to check against the alignment of « struct membank ». This is because the structure could gain a 64-bit field in the future and this would not be caught by the BUILD_BUG_ON. > > > Let me know how you would like to have this written in commit msg. > > > I think it needs to be rephrased to make clear the alignment of struct membanks should be the same as struct membank. Shouldn't this check therefore be changed to: BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); ~Michal
Hi Michal, On Mon, 27 Jan 2025 at 14:15, Michal Orzel <michal.orzel@amd.com> wrote: > > I think it needs to be rephrased to make clear the alignment of struct > membanks should be the same as struct membank. > Shouldn't this check therefore be changed to: > BUILD_BUG_ON(alignof(struct membanks) != alignof(struct membank)); Yes it should be changed. Cheers, > > ~Michal >
diff --git a/xen/common/device-tree/bootfdt.c b/xen/common/device-tree/bootfdt.c index 47386d4fffea..511700ccc2ba 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" is paddr aligned */ + BUILD_BUG_ON(alignof(struct membanks) != sizeof(paddr_t)); } 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. Fix it. Fixes: 2209c1e35b47 ("xen/arm: Introduce a generic way to access memory bank structures") Signed-off-by: Michal Orzel <michal.orzel@amd.com> --- xen/common/device-tree/bootfdt.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)