diff mbox series

[v2] common: fdt: Remove additional 4k space for fdt allocation

Message ID ef5ffe3270ac193cc9f6069bdcff55525ebf17d0.1591873850.git.michal.simek@xilinx.com
State Superseded
Headers show
Series [v2] common: fdt: Remove additional 4k space for fdt allocation | expand

Commit Message

Michal Simek June 11, 2020, 11:10 a.m. UTC
From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>

There is no technical reason to add additional 4k space for FDT. This space
is completely unused and just increase memory requirements. This is
problematic on systems with limited memory resources as Xilinx Zynq
CSE/ZynqMP mini and Versal mini configurations.

The patch is removing additional 4k space and also increasing alignment to
64 to be aligned with 64bit systems.

EFI code is using copy_fdt() which copy FDT to different location.
And all boot commands in case of using U-Boot's FDT pointed by
$fdtcontroladdr are copying FDT to different locations by
image_setup_libfdt().
That's why in proper flow none should modified DTB used by U-Boot that's
why there is no need for additional space.

Signed-off-by: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
Signed-off-by: Michal Simek <michal.simek at xilinx.com>
---

Changes in v2:
- Change subject (was: common: Add Kconfig option for FDT mem alignment)
- Remove Kconfig symbol
- Extend description

I have tested it on zcu104

---
 common/board_f.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Marek Vasut June 11, 2020, 11:24 a.m. UTC | #1
On 6/11/20 1:10 PM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
> 
> There is no technical reason to add additional 4k space for FDT.

Could it be that this is needed for adjusting the FDT early on ?
Michal Simek June 11, 2020, 12:07 p.m. UTC | #2
On 11. 06. 20 13:24, Marek Vasut wrote:
> On 6/11/20 1:10 PM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
>>
>> There is no technical reason to add additional 4k space for FDT.
> 
> Could it be that this is needed for adjusting the FDT early on ?
> 

It really depends how early. fdt_totalsize is called in reserve_fdt().
It means if this is done before then you are working with already
updated size and there is no need to add any space.
And switch/copy is done by reloc_fdt from board_f.c.

And in your case it is really question if 4k additional is enough.
Because you can have a need to use more then 4k.

Thanks,
Michal
Stephen Warren June 12, 2020, 5 p.m. UTC | #3
On 6/11/20 5:10 AM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
> 
> There is no technical reason to add additional 4k space for FDT. This space
> is completely unused and just increase memory requirements. This is
> problematic on systems with limited memory resources as Xilinx Zynq
> CSE/ZynqMP mini and Versal mini configurations.

I haven't looked into whether it's useful to reserve extra space yet.
But I have to say I find it extremely unlikely that 4K space usage is
problematic for a system large enough to run U-Boot, unless this is part
of a much larger push to trim memory usage.

> The patch is removing additional 4k space and also increasing alignment to
> 64 to be aligned with 64bit systems.

> diff --git a/common/board_f.c b/common/board_f.c

> -		gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
> +		gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob), 64);

So IIRC the alignment parameter is bytes not bits. I don't follow why a
64-bit system would need 64-byte alignment? IIRC, the stack alignment
requirement is only 16 bytes, so the presumably isn't what this change
is trying to fix.

I'd suggest making the alignment-change and extra-size-removal patches
separate patches so that any issues caused by the change can easily be
tracked to one of the separate logical changes by git bisect etc.
Stephen Warren June 12, 2020, 7:28 p.m. UTC | #4
On 6/11/20 5:10 AM, Michal Simek wrote:
> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
> 
> There is no technical reason to add additional 4k space for FDT. This space
> is completely unused and just increase memory requirements. This is
> problematic on systems with limited memory resources as Xilinx Zynq
> CSE/ZynqMP mini and Versal mini configurations.

I tried to work out where this additional space came from, but can't
find any obvious clues why it's there.

The extra 4k was originally introduced in x86-specific code by:

commit f697d528caba0c30382b7269fd36f1111e51810d
    x86: Support relocation of FDT on start-up
+int copy_fdt_to_ram(void)
+               fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);

... and later copied to generic code by:

commit 1938f4a5b62fc03c52b47697a89b2bb47b77c90c (HEAD)
    Introduce generic pre-relocation board_f.c
+static int reserve_fdt(void)
+               gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) +
0x1000, 32);

However, there's no obvious comment re: why 4k was added. I wonder if
it's anything to do with 4k==page size and hence different MMU
permissions for the DTB and any data following it? That's purely a guess
though, since I don't see anything in those patches messing with the MMU
any differently for the DTB.

So I guess if this patch passes testing, it's good:-)
Heinrich Schuchardt June 12, 2020, 10:56 p.m. UTC | #5
On 6/12/20 9:28 PM, Stephen Warren wrote:
> On 6/11/20 5:10 AM, Michal Simek wrote:
>> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
>>
>> There is no technical reason to add additional 4k space for FDT. This space
>> is completely unused and just increase memory requirements. This is
>> problematic on systems with limited memory resources as Xilinx Zynq
>> CSE/ZynqMP mini and Versal mini configurations.
>
> I tried to work out where this additional space came from, but can't
> find any obvious clues why it's there.
>
> The extra 4k was originally introduced in x86-specific code by:
>
> commit f697d528caba0c30382b7269fd36f1111e51810d
>     x86: Support relocation of FDT on start-up
> +int copy_fdt_to_ram(void)
> +               fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
>
> ... and later copied to generic code by:
>
> commit 1938f4a5b62fc03c52b47697a89b2bb47b77c90c (HEAD)
>     Introduce generic pre-relocation board_f.c
> +static int reserve_fdt(void)
> +               gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) +
> 0x1000, 32);
>
> However, there's no obvious comment re: why 4k was added. I wonder if
> it's anything to do with 4k==page size and hence different MMU
> permissions for the DTB and any data following it? That's purely a guess
> though, since I don't see anything in those patches messing with the MMU> any differently for the DTB.

Target $(obj)/%.dtb.S in scripts/Makefile.lib only guarantees 16 byte
alignment of embedded device trees. It does not make any sense to
provide extra 4KiB in the case of CONFIG_OF_EMBED=y and not for
CONFIG_OF_EMBED=n.

I do not see a need for using ALIGN for the fdt size.

>
> So I guess if this patch passes testing, it's good:-)
>

Which test will check that we do not have any buffer overruns due to
increasing the device tree size before we copy the device tree in a boot
command?

Best regards

Heinrich
Stephen Warren June 12, 2020, 11:07 p.m. UTC | #6
On 6/12/20 4:56 PM, Heinrich Schuchardt wrote:
> On 6/12/20 9:28 PM, Stephen Warren wrote:
>> On 6/11/20 5:10 AM, Michal Simek wrote:
>>> From: Ashok Reddy Soma <ashok.reddy.soma at xilinx.com>
>>>
>>> There is no technical reason to add additional 4k space for FDT. This space
>>> is completely unused and just increase memory requirements. This is
>>> problematic on systems with limited memory resources as Xilinx Zynq
>>> CSE/ZynqMP mini and Versal mini configurations.
>>
>> I tried to work out where this additional space came from, but can't
>> find any obvious clues why it's there.
>>
>> The extra 4k was originally introduced in x86-specific code by:
>>
>> commit f697d528caba0c30382b7269fd36f1111e51810d
>>     x86: Support relocation of FDT on start-up
>> +int copy_fdt_to_ram(void)
>> +               fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
>>
>> ... and later copied to generic code by:
>>
>> commit 1938f4a5b62fc03c52b47697a89b2bb47b77c90c (HEAD)
>>     Introduce generic pre-relocation board_f.c
>> +static int reserve_fdt(void)
>> +               gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) +
>> 0x1000, 32);
>>
>> However, there's no obvious comment re: why 4k was added. I wonder if
>> it's anything to do with 4k==page size and hence different MMU
>> permissions for the DTB and any data following it? That's purely a guess
>> though, since I don't see anything in those patches messing with the MMU> any differently for the DTB.
> 
> Target $(obj)/%.dtb.S in scripts/Makefile.lib only guarantees 16 byte
> alignment of embedded device trees. It does not make any sense to
> provide extra 4KiB in the case of CONFIG_OF_EMBED=y and not for
> CONFIG_OF_EMBED=n.
> 
> I do not see a need for using ALIGN for the fdt size.
> 
>> So I guess if this patch passes testing, it's good:-)
> 
> Which test will check that we do not have any buffer overruns due to
> increasing the device tree size before we copy the device tree in a boot
> command?

We'd need to boot U-Boot on all the boards it supports. In other words,
apply it early during a release cycle and watch for any fallout when
people test it I guess.
diff mbox series

Patch

diff --git a/common/board_f.c b/common/board_f.c
index 01194eaa0e4d..7e99b2425a62 100644
--- a/common/board_f.c
+++ b/common/board_f.c
@@ -537,7 +537,7 @@  static int reserve_fdt(void)
 	 * will be relocated with other data.
 	 */
 	if (gd->fdt_blob) {
-		gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob) + 0x1000, 32);
+		gd->fdt_size = ALIGN(fdt_totalsize(gd->fdt_blob), 64);
 
 		gd->start_addr_sp = reserve_stack_aligned(gd->fdt_size);
 		gd->new_fdt = map_sysmem(gd->start_addr_sp, gd->fdt_size);