On 27/01/2025 17:27, Julien Grall wrote:
>
>
>
>
>
> On Mon, 27 Jan 2025 at 09:52, Michal Orzel <[email protected]
> <mailto:[email protected]>> wrote:
>
>
>
> On 27/01/2025 12:19, Julien Grall wrote:
> >
> >
> >
> >
> >
> > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <[email protected]
> <mailto:[email protected]> <mailto:[email protected]
> <mailto:[email protected]>>> 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
