diff mbox series

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

Message ID 20251209053130.407935-1-apatel@ventanamicro.com
State Accepted
Headers show
Series [v3] platform: generic: Keep some empty space in FDT passed to next stage | expand

Commit Message

Anup Patel Dec. 9, 2025, 5:31 a.m. UTC 
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@ventanamicro.com>
---
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(+)

Comments

Anup Patel Dec. 16, 2025, 2:45 p.m. UTC | #1 
On Tue, Dec 9, 2025 at 11:01 AM Anup Patel <apatel@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@ventanamicro.com>

Applied this patch to the riscv/opensbi repo.

Regards,
Anup

> ---
> 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
>
Samuel Holland Dec. 16, 2025, 3:55 p.m. UTC | #2 
Anup,

On 2025-12-16 8:45 AM, Anup Patel wrote:
> On Tue, Dec 9, 2025 at 11:01 AM Anup Patel <apatel@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@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.

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
>>
>
Anup Patel Dec. 16, 2025, 5:09 p.m. UTC | #3 
On Tue, Dec 16, 2025 at 9:25 PM Samuel Holland
<samuel.holland@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@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@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.
The original fix itself was incomplete. Adding memset() to
fdt_open_into() is a separate change. Feel free to send
patch for it.

Regards,
Anup

>
> 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
> >>
> >
>
Samuel Holland Dec. 16, 2025, 5:31 p.m. UTC | #4 
On 2025-12-16 11:09 AM, Anup Patel wrote:
> On Tue, Dec 16, 2025 at 9:25 PM Samuel Holland
> <samuel.holland@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@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@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
>>>>
>>>
>>
Anup Patel Dec. 16, 2025, 6:07 p.m. UTC | #5 
On Tue, Dec 16, 2025 at 11:01 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2025-12-16 11:09 AM, Anup Patel wrote:
> > On Tue, Dec 16, 2025 at 9:25 PM Samuel Holland
> > <samuel.holland@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@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@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.

You failed to understand my point. You original fix was already
incomplete since instead of packing the FDT it should have done
memset() of uninitialized space in FDT.

>
> > 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.

The original fix for packing FDT was already broken so don't
blame me for partially reverting.

Regards,
Anup
diff mbox series

Patch

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;
 }