| Message ID | 20180303144813.28169-1-xypron.glpk@gmx.de |
|---|---|
| State | Superseded, archived |
| Delegated to: | Alexander Graf |
| Headers | show |
| Series | [U-Boot,1/1] efi_loader: efi_allocate_pages is too restrictive | expand |
On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: > When running on the sandbox the stack is not necessarily at a higher memory > address than the highest free memory. > > There is no reason why the checking of the highest memory address should be > more restrictive for EFI_ALLOCATE_ANY_PAGES than for > EFI_ALLOCATE_MAX_ADDRESS. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > lib/efi_loader/efi_memory.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index cc271e0709d..0efbb973231 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type, > switch (type) { > case EFI_ALLOCATE_ANY_PAGES: > /* Any page */ > - addr = efi_find_free_memory(len, gd->start_addr_sp); > + addr = efi_find_free_memory(len, (uint64_t)-1); This will break on systems that do not map high address space into the linear map (IIRC nvidia systems had that issue). Alex
On 03/09/2018 01:48 PM, Alexander Graf wrote: > On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: >> When running on the sandbox the stack is not necessarily at a higher >> memory >> address than the highest free memory. >> >> There is no reason why the checking of the highest memory address >> should be >> more restrictive for EFI_ALLOCATE_ANY_PAGES than for >> EFI_ALLOCATE_MAX_ADDRESS. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> lib/efi_loader/efi_memory.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >> index cc271e0709d..0efbb973231 100644 >> --- a/lib/efi_loader/efi_memory.c >> +++ b/lib/efi_loader/efi_memory.c >> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int >> memory_type, >> switch (type) { >> case EFI_ALLOCATE_ANY_PAGES: >> /* Any page */ >> - addr = efi_find_free_memory(len, gd->start_addr_sp); >> + addr = efi_find_free_memory(len, (uint64_t)-1); > > This will break on systems that do not map high address space into the > linear map (IIRC nvidia systems had that issue). > The memory map is also passed on to the operating system when booting. If a memory reservation is missing for any board it has to be fixed in the board or driver files, cf. sunxi: video: mark framebuffer as EFI reserved memory https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about gd->start_addr_sp. So if the memory map is incomplete the current code may fail. Keeping things as they are is not a viable option. Could you, please, identify for which Nvidia system a problem was reported? Then we can add a call to efi_add_memory_map() for the board. Best regards Heinrich
On 03/09/2018 04:58 PM, Heinrich Schuchardt wrote: > On 03/09/2018 01:48 PM, Alexander Graf wrote: >> On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: >>> When running on the sandbox the stack is not necessarily at a higher >>> memory >>> address than the highest free memory. >>> >>> There is no reason why the checking of the highest memory address >>> should be >>> more restrictive for EFI_ALLOCATE_ANY_PAGES than for >>> EFI_ALLOCATE_MAX_ADDRESS. >>> >>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>> --- >>> lib/efi_loader/efi_memory.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>> index cc271e0709d..0efbb973231 100644 >>> --- a/lib/efi_loader/efi_memory.c >>> +++ b/lib/efi_loader/efi_memory.c >>> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int >>> memory_type, >>> switch (type) { >>> case EFI_ALLOCATE_ANY_PAGES: >>> /* Any page */ >>> - addr = efi_find_free_memory(len, gd->start_addr_sp); >>> + addr = efi_find_free_memory(len, (uint64_t)-1); >> This will break on systems that do not map high address space into the >> linear map (IIRC nvidia systems had that issue). >> > The memory map is also passed on to the operating system when booting. > If a memory reservation is missing for any board it has to be fixed in > the board or driver files, cf. > > sunxi: video: mark framebuffer as EFI reserved memory > https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm > > For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about > gd->start_addr_sp. So if the memory map is incomplete the current code > may fail. Keeping things as they are is not a viable option. > > Could you, please, identify for which Nvidia system a problem was > reported? Then we can add a call to efi_add_memory_map() for the board. Git blame points to this commit. I guess -1 should do the same thing then, true. Andreas, would you see any reason -1 will not work? Alex commit dede284d1ce9f9d9e79a5114fe7eb814fec07679 Author: Andreas Färber <afaerber@suse.de> Date: Wed Apr 13 14:04:38 2016 +0200 efi_loader: Handle memory overflows jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be zero. Handle this by either avoiding ram_top or by using the same type as ram_top to reverse the overflow effect. Cc: Alexander Graf <agraf@suse.de> Signed-off-by: Andreas Färber <afaerber@suse.de> Reviewed-by: Alexander Graf <agraf@suse.de> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index df995858ed..71a3d19269 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -220,7 +220,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type, switch (type) { case 0: /* Any page */ - addr = efi_find_free_memory(len, gd->ram_top); + addr = efi_find_free_memory(len, gd->start_addr_sp); if (!addr) { r = EFI_NOT_FOUND; break; @@ -320,9 +320,9 @@ efi_status_t efi_get_memory_map(unsigned long *memory_map_size, int efi_memory_init(void) { - uint64_t runtime_start, runtime_end, runtime_pages; - uint64_t uboot_start, uboot_pages; - uint64_t uboot_stack_size = 16 * 1024 * 1024; + unsigned long runtime_start, runtime_end, runtime_pages; + unsigned long uboot_start, uboot_pages; + unsigned long uboot_stack_size = 16 * 1024 * 1024; int i; /* Add RAM */
On 03/09/2018 05:19 PM, Alexander Graf wrote: > On 03/09/2018 04:58 PM, Heinrich Schuchardt wrote: >> On 03/09/2018 01:48 PM, Alexander Graf wrote: >>> On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: >>>> When running on the sandbox the stack is not necessarily at a higher >>>> memory >>>> address than the highest free memory. >>>> >>>> There is no reason why the checking of the highest memory address >>>> should be >>>> more restrictive for EFI_ALLOCATE_ANY_PAGES than for >>>> EFI_ALLOCATE_MAX_ADDRESS. >>>> >>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>> --- >>>> lib/efi_loader/efi_memory.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>>> index cc271e0709d..0efbb973231 100644 >>>> --- a/lib/efi_loader/efi_memory.c >>>> +++ b/lib/efi_loader/efi_memory.c >>>> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int >>>> memory_type, >>>> switch (type) { >>>> case EFI_ALLOCATE_ANY_PAGES: >>>> /* Any page */ >>>> - addr = efi_find_free_memory(len, gd->start_addr_sp); >>>> + addr = efi_find_free_memory(len, (uint64_t)-1); >>> This will break on systems that do not map high address space into the >>> linear map (IIRC nvidia systems had that issue). >>> >> The memory map is also passed on to the operating system when booting. >> If a memory reservation is missing for any board it has to be fixed in >> the board or driver files, cf. >> >> sunxi: video: mark framebuffer as EFI reserved memory >> https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm >> >> For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about >> gd->start_addr_sp. So if the memory map is incomplete the current code >> may fail. Keeping things as they are is not a viable option. >> >> Could you, please, identify for which Nvidia system a problem was >> reported? Then we can add a call to efi_add_memory_map() for the board. > > Git blame points to this commit. I guess -1 should do the same thing > then, true. > > Andreas, would you see any reason -1 will not work? Are we talking about this line: arch/arm/mach-tegra/board2.c:317: gd->pci_ram_top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size; Regards Heinrich > > > Alex > > commit dede284d1ce9f9d9e79a5114fe7eb814fec07679 > Author: Andreas Färber <afaerber@suse.de> > Date: Wed Apr 13 14:04:38 2016 +0200 > > efi_loader: Handle memory overflows > > jetson-tk1 has 2 GB of RAM at 0x80000000, causing gd->ram_top to be > zero. > Handle this by either avoiding ram_top or by using the same type as > ram_top to reverse the overflow effect. > > Cc: Alexander Graf <agraf@suse.de> > Signed-off-by: Andreas Färber <afaerber@suse.de> > Reviewed-by: Alexander Graf <agraf@suse.de> > > diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c > index df995858ed..71a3d19269 100644 > --- a/lib/efi_loader/efi_memory.c > +++ b/lib/efi_loader/efi_memory.c > @@ -220,7 +220,7 @@ efi_status_t efi_allocate_pages(int type, int > memory_type, > switch (type) { > case 0: > /* Any page */ > - addr = efi_find_free_memory(len, gd->ram_top); > + addr = efi_find_free_memory(len, gd->start_addr_sp); > if (!addr) { > r = EFI_NOT_FOUND; > break; > @@ -320,9 +320,9 @@ efi_status_t efi_get_memory_map(unsigned long > *memory_map_size, > > int efi_memory_init(void) > { > - uint64_t runtime_start, runtime_end, runtime_pages; > - uint64_t uboot_start, uboot_pages; > - uint64_t uboot_stack_size = 16 * 1024 * 1024; > + unsigned long runtime_start, runtime_end, runtime_pages; > + unsigned long uboot_start, uboot_pages; > + unsigned long uboot_stack_size = 16 * 1024 * 1024; > int i; > > /* Add RAM */ > > > >
On 03/09/2018 05:35 PM, Heinrich Schuchardt wrote: > On 03/09/2018 05:19 PM, Alexander Graf wrote: >> On 03/09/2018 04:58 PM, Heinrich Schuchardt wrote: >>> On 03/09/2018 01:48 PM, Alexander Graf wrote: >>>> On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: >>>>> When running on the sandbox the stack is not necessarily at a higher >>>>> memory >>>>> address than the highest free memory. >>>>> >>>>> There is no reason why the checking of the highest memory address >>>>> should be >>>>> more restrictive for EFI_ALLOCATE_ANY_PAGES than for >>>>> EFI_ALLOCATE_MAX_ADDRESS. >>>>> >>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>> --- >>>>> lib/efi_loader/efi_memory.c | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c >>>>> index cc271e0709d..0efbb973231 100644 >>>>> --- a/lib/efi_loader/efi_memory.c >>>>> +++ b/lib/efi_loader/efi_memory.c >>>>> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int >>>>> memory_type, >>>>> switch (type) { >>>>> case EFI_ALLOCATE_ANY_PAGES: >>>>> /* Any page */ >>>>> - addr = efi_find_free_memory(len, gd->start_addr_sp); >>>>> + addr = efi_find_free_memory(len, (uint64_t)-1); >>>> This will break on systems that do not map high address space into the >>>> linear map (IIRC nvidia systems had that issue). >>>> >>> The memory map is also passed on to the operating system when booting. >>> If a memory reservation is missing for any board it has to be fixed in >>> the board or driver files, cf. >>> >>> sunxi: video: mark framebuffer as EFI reserved memory >>> https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm >>> >>> For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about >>> gd->start_addr_sp. So if the memory map is incomplete the current code >>> may fail. Keeping things as they are is not a viable option. >>> >>> Could you, please, identify for which Nvidia system a problem was >>> reported? Then we can add a call to efi_add_memory_map() for the board. >> Git blame points to this commit. I guess -1 should do the same thing >> then, true. >> >> Andreas, would you see any reason -1 will not work? > Are we talking about this line: > > arch/arm/mach-tegra/board2.c:317: > gd->pci_ram_top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size; pci_ram_top != ram_top, no? :) I'd rather assume it's this one: /* * Most hardware on 64-bit Tegra is still restricted to DMA to the lower * 32-bits of the physical address space. Cap the maximum usable RAM area * at 4 GiB to avoid DMA buffers from being allocated beyond the 32-bit * boundary that most devices can address. Also, don't let U-Boot use any * carve-out, as mentioned above. * * This function is called before dram_init_banksize(), so we can't simply * return gd->bd->bi_dram[1].start + gd->bd->bi_dram[1].size. */ ulong board_get_usable_ram_top(ulong total_size) { return CONFIG_SYS_SDRAM_BASE + usable_ram_size_below_4g(); } But the real problem is that ram_top of 0 is perfectly valid for 32bit systems. Also ram_top may be used by platforms (like tegra again) to ensure that we don't use addresses >32bit for DMA. I guess the real solution to that would be to enable bounce buffers for tegra though. But we should make sure we don't regress tegra support, so we need to enable bounce buffers first for tegra and then allow the allocation to walk the full address space. Alex
On 03/12/2018 11:48 AM, Alexander Graf wrote: > On 03/09/2018 05:35 PM, Heinrich Schuchardt wrote: >> On 03/09/2018 05:19 PM, Alexander Graf wrote: >>> On 03/09/2018 04:58 PM, Heinrich Schuchardt wrote: >>>> On 03/09/2018 01:48 PM, Alexander Graf wrote: >>>>> On 03/03/2018 03:48 PM, Heinrich Schuchardt wrote: >>>>>> When running on the sandbox the stack is not necessarily at a higher >>>>>> memory >>>>>> address than the highest free memory. >>>>>> >>>>>> There is no reason why the checking of the highest memory address >>>>>> should be >>>>>> more restrictive for EFI_ALLOCATE_ANY_PAGES than for >>>>>> EFI_ALLOCATE_MAX_ADDRESS. >>>>>> >>>>>> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >>>>>> --- >>>>>> lib/efi_loader/efi_memory.c | 2 +- >>>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/lib/efi_loader/efi_memory.c >>>>>> b/lib/efi_loader/efi_memory.c >>>>>> index cc271e0709d..0efbb973231 100644 >>>>>> --- a/lib/efi_loader/efi_memory.c >>>>>> +++ b/lib/efi_loader/efi_memory.c >>>>>> @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int >>>>>> memory_type, >>>>>> switch (type) { >>>>>> case EFI_ALLOCATE_ANY_PAGES: >>>>>> /* Any page */ >>>>>> - addr = efi_find_free_memory(len, gd->start_addr_sp); >>>>>> + addr = efi_find_free_memory(len, (uint64_t)-1); >>>>> This will break on systems that do not map high address space into the >>>>> linear map (IIRC nvidia systems had that issue). >>>>> >>>> The memory map is also passed on to the operating system when booting. >>>> If a memory reservation is missing for any board it has to be fixed in >>>> the board or driver files, cf. >>>> >>>> sunxi: video: mark framebuffer as EFI reserved memory >>>> https://lists.denx.de/pipermail/u-boot/2018-March/321820.htm >>>> >>>> For type = EFI_ALLOCATE_MAX_ADDRESS we don't care about >>>> gd->start_addr_sp. So if the memory map is incomplete the current code >>>> may fail. Keeping things as they are is not a viable option. >>>> >>>> Could you, please, identify for which Nvidia system a problem was >>>> reported? Then we can add a call to efi_add_memory_map() for the board. >>> Git blame points to this commit. I guess -1 should do the same thing >>> then, true. >>> >>> Andreas, would you see any reason -1 will not work? >> Are we talking about this line: >> >> arch/arm/mach-tegra/board2.c:317: >> gd->pci_ram_top = gd->bd->bi_dram[0].start + gd->bd->bi_dram[0].size; > > pci_ram_top != ram_top, no? :) > > I'd rather assume it's this one: > > /* > * Most hardware on 64-bit Tegra is still restricted to DMA to the lower > * 32-bits of the physical address space. Cap the maximum usable RAM area > * at 4 GiB to avoid DMA buffers from being allocated beyond the 32-bit > * boundary that most devices can address. Also, don't let U-Boot use any > * carve-out, as mentioned above. > * > * This function is called before dram_init_banksize(), so we can't simply > * return gd->bd->bi_dram[1].start + gd->bd->bi_dram[1].size. > */ > ulong board_get_usable_ram_top(ulong total_size) > { > return CONFIG_SYS_SDRAM_BASE + usable_ram_size_below_4g(); > } > > But the real problem is that ram_top of 0 is perfectly valid for 32bit > systems. But where is the problem? If ram_top == 0 on a 32bit system efi_memory_init() will create a memory map for the high memory reaching from start_addr_sp - uboot_stack_size to 0xffffffff marked as EFI_LOADER_DATA. So efi_allocate_pages will never touch that memory even if we start our search at 0xffffffffffffffff. And on a 32 bit system we will not have any memory map beyond 0xffffffff. So essentially in this case the patch does not lead to a different outcome of the search for free pages. Best regards Heinrich > > Also ram_top may be used by platforms (like tegra again) to ensure that > we don't use addresses >32bit for DMA. I guess the real solution to that > would be to enable bounce buffers for tegra though. But we should make > sure we don't regress tegra support, so we need to enable bounce buffers > first for tegra and then allow the allocation to walk the full address > space. > > > Alex > > _______________________________________________ > U-Boot mailing list > U-Boot@lists.denx.de > https://lists.denx.de/listinfo/u-boot
diff --git a/lib/efi_loader/efi_memory.c b/lib/efi_loader/efi_memory.c index cc271e0709d..0efbb973231 100644 --- a/lib/efi_loader/efi_memory.c +++ b/lib/efi_loader/efi_memory.c @@ -294,7 +294,7 @@ efi_status_t efi_allocate_pages(int type, int memory_type, switch (type) { case EFI_ALLOCATE_ANY_PAGES: /* Any page */ - addr = efi_find_free_memory(len, gd->start_addr_sp); + addr = efi_find_free_memory(len, (uint64_t)-1); if (!addr) { r = EFI_NOT_FOUND; break;
When running on the sandbox the stack is not necessarily at a higher memory address than the highest free memory. There is no reason why the checking of the highest memory address should be more restrictive for EFI_ALLOCATE_ANY_PAGES than for EFI_ALLOCATE_MAX_ADDRESS. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- lib/efi_loader/efi_memory.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)