| Message ID | ef5ffe3270ac193cc9f6069bdcff55525ebf17d0.1591873850.git.michal.simek@xilinx.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [v2] common: fdt: Remove additional 4k space for fdt allocation | expand |
On 6/11/20 1:10 PM, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@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 ?
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@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
On 6/11/20 5:10 AM, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@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.
On 6/11/20 5:10 AM, Michal Simek wrote: > From: Ashok Reddy Soma <ashok.reddy.soma@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:-)
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@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
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@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 --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);