[PATCH v3] platform: generic: Keep some empty space in FDT passed to next stage

[Samuel Holland]

On 2025-12-16 11:09 AM, Anup Patel wrote:
> On Tue, Dec 16, 2025 at 9:25 PM Samuel Holland
> <samuel.holland at sifive.com> wrote:
>>
>> Anup,
>>
>> On 2025-12-16 8:45 AM, Anup Patel wrote:
>>> On Tue, Dec 9, 2025 at 11:01 AM Anup Patel <apatel at ventanamicro.com> wrote:
>>>>
>>>> Leaving no empty space in the FDT passed to the next booting stage
>>>> causes the following U-Boot crash on Ventana internal platforms:
>>>>
>>>> Unhandled exception: Load access fault
>>>> EPC: 00000000fffa6372 RA: 00000000fffa7418 TVAL: 0001746174730068
>>>> EPC: 0000000080245372 RA: 0000000080246418 reloc adjusted
>>>>
>>>> SP:  00000000fef38440 GP:  00000000fef40e60 TP:  0000000000000000
>>>> T0:  00000000fef40a70 T1:  000000000000ff00 T2:  0000000000000000
>>>> S0:  00000000fffc17a8 S1:  00000000fef38d40 A0:  7375746174730068
>>>> A1:  00000000fffc17a8 A2:  0000000000000010 A3:  0000000000000010
>>>> A4:  0000000000000000 A5:  00000000fffc17b8 A6:  0000000000ff0000
>>>> A7:  000000000000b100 S2:  0000000000000000 S3:  0000000000000001
>>>> S4:  00000000fef38d40 S5:  7375746174730068 S6:  0000000000000000
>>>> S7:  00000000fef4eef0 S8:  00000000fef4ef90 S9:  0000000000000000
>>>> S10: 0000000000000000 S11: 00000000fef4efc0 T3:  00000000fef40ea8
>>>> T4:  0000000000ff0000 T5:  00000000fef40a60 T6:  00000000fef40a6c
>>>>
>>>> To address the above issue, keep some minimal empty space in the
>>>> FDT instead of no empty space.
>>>>
>>>> Fixes: bbe9a23060e9 ("platform: generic: Pack the FDT after applying fixups")
>>>> Signed-off-by: Anup Patel <apatel at ventanamicro.com>
>>>
>>> Applied this patch to the riscv/opensbi repo.
>>
>> Again, this patch reintroduces the bug that was fixed by the "Fixes:" commit.
>> You can't just say "somebody will add the memset() somewhere later" and ignore
>> the bug you are introducing. You need to add the memset() *now* along with the
>> fdt_open_into() call.
> 
> Yes, but that fix broke U-boot booting.

And it is entirely possible to fix U-boot booting without breaking other users
(cvw, etc.) in the process. Yes, the original commit exposed a bug in U-Boot.
Bugs happen; we can fix them or work around them. But needing to fix one bug is
not license to intentionally introduce more bugs.

> The original fix itself was incomplete. Adding memset() to
> fdt_open_into() is a separate change. Feel free to send
> patch for it.

If you want adding memset() to fdt_open_into() to be a separate change, then
that change must be merged *first*, before this patch, so it is then safe to add
the call to fdt_open_into(). Because now you have broken bisectability for the
affected platforms. The uninitialized memory issue was completely fixed by the
original patch, and OpenSBI could boot on cvw as of that commit; now it is
broken again, and that is a regression.

Regards,
Samuel

>>>> ---
>>>> Changes since v2:
>>>> - Fix comments in generic_final_init()
>>>> Changes since v1:
>>>> - Add Kconfig option for amount of empty space in FDT with default
>>>>   value as 4KB
>>>> ---
>>>>  platform/generic/Kconfig    | 5 +++++
>>>>  platform/generic/platform.c | 3 +++
>>>>  2 files changed, 8 insertions(+)
>>>>
>>>> diff --git a/platform/generic/Kconfig b/platform/generic/Kconfig
>>>> index 25b8886b..b1808012 100644
>>>> --- a/platform/generic/Kconfig
>>>> +++ b/platform/generic/Kconfig
>>>> @@ -23,6 +23,11 @@ config PLATFORM_GENERIC_MINOR_VER
>>>>         range 0 65535
>>>>         default 1
>>>>
>>>> +config PLATFORM_GENERIC_FDT_EMPTY_SPACE
>>>> +       int "Platform FDT empty space (KB)"
>>>> +       range 0 1024
>>>> +       default 4
>>>> +
>>>>  config PLATFORM_ALLWINNER_D1
>>>>         bool "Allwinner D1 support"
>>>>         depends on FDT_IRQCHIP_PLIC
>>>> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
>>>> index e66f99fa..01fa03e0 100644
>>>> --- a/platform/generic/platform.c
>>>> +++ b/platform/generic/platform.c
>>>> @@ -245,7 +245,10 @@ int generic_final_init(bool cold_boot)
>>>>         fdt_fixups(fdt);
>>>>         fdt_domain_fixup(fdt);
>>>>
>>>> +       /* Set the empty space in FDT based on kconfig option */
>>>>         fdt_pack(fdt);
>>>> +       fdt_open_into(fdt, fdt, fdt_totalsize(fdt) +
>>>> +                     CONFIG_PLATFORM_GENERIC_FDT_EMPTY_SPACE * 1024);
>>>>
>>>>         return 0;
>>>>  }
>>>> --
>>>> 2.43.0
>>>>
>>>
>>