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

[Julien Grall <julien.grall.oss@xxxxxxxxx>]

On Mon, 27 Jan 2025 at 09:52, Michal Orzel <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>> 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