Re: [for-4.20][PATCH 1/2] device-tree: bootfdt: Fix build issue when CONFIG_PHYS_ADDR_T_32=y

[Michal Orzel <michal.orzel@xxxxxxx>]

On 27/01/2025 17:27, Julien Grall wrote:
>       
> 
> 
> 
> 
> On Mon, 27 Jan 2025 at 09:52, Michal Orzel <michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>> wrote:
> 
> 
> 
>     On 27/01/2025 12:19, Julien Grall wrote:
>     >       
>     >
>     >
>     >
>     >
>     > On Mon, 27 Jan 2025 at 07:46, Michal Orzel <michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx> <mailto:michal.orzel@xxxxxxx 
> <mailto:michal.orzel@xxxxxxx>>> 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