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

[Michal Orzel <michal.orzel@xxxxxxx>]

On 28/01/2025 11:34, Luca Fancellu wrote:
> 
> 
> Hi Michal, Julien,
> 
>> On 28 Jan 2025, at 09:40, Michal Orzel <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 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@xxxxxxx>
>> Release-Acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>> ---
>> 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@xxxxxxx>
> 

~Michal