* [PATCH 1/1] efi/libstub: DRAM base calculation @ 2020-09-04 15:50 Heinrich Schuchardt 2020-09-07 7:00 ` Maxim Uvarov 2020-09-09 8:17 ` Ard Biesheuvel 0 siblings, 2 replies; 11+ messages in thread From: Heinrich Schuchardt @ 2020-09-04 15:50 UTC (permalink / raw) To: Ard Biesheuvel Cc: Ingo Molnar, Arvind Sankar, linux-efi, linux-kernel, Maxim Uvarov, Jens Wiklander, Heinrich Schuchardt In the memory map the regions with the lowest addresses may be of type EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the rest of the memory. So for calculating the maximum loading address for the device tree and the initial ramdisk image these reserved areas should not be taken into account. Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> --- drivers/firmware/efi/libstub/efi-stub.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c index c2484bf75c5d..13058ac75765 100644 --- a/drivers/firmware/efi/libstub/efi-stub.c +++ b/drivers/firmware/efi/libstub/efi-stub.c @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) map.map_end = map.map + map_size; for_each_efi_memory_desc_in_map(&map, md) { - if (md->attribute & EFI_MEMORY_WB) { + if (md->attribute & EFI_MEMORY_WB && + md->type != EFI_RESERVED_TYPE) { if (membase > md->phys_addr) membase = md->phys_addr; } -- 2.28.0 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-04 15:50 [PATCH 1/1] efi/libstub: DRAM base calculation Heinrich Schuchardt @ 2020-09-07 7:00 ` Maxim Uvarov 2020-09-07 8:30 ` Heinrich Schuchardt 2020-09-09 8:17 ` Ard Biesheuvel 1 sibling, 1 reply; 11+ messages in thread From: Maxim Uvarov @ 2020-09-07 7:00 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Ard Biesheuvel, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, Ilias Apalodimas On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > In the memory map the regions with the lowest addresses may be of type > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > rest of the memory. So for calculating the maximum loading address for the > device tree and the initial ramdisk image these reserved areas should not > be taken into account. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index c2484bf75c5d..13058ac75765 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > map.map_end = map.map + map_size; > > for_each_efi_memory_desc_in_map(&map, md) { > - if (md->attribute & EFI_MEMORY_WB) { > + if (md->attribute & EFI_MEMORY_WB && > + md->type != EFI_RESERVED_TYPE) { shouldn't the type here be CONVENTIONAL? > if (membase > md->phys_addr) > membase = md->phys_addr; > } > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-07 7:00 ` Maxim Uvarov @ 2020-09-07 8:30 ` Heinrich Schuchardt 2020-09-07 10:21 ` Maxim Uvarov 0 siblings, 1 reply; 11+ messages in thread From: Heinrich Schuchardt @ 2020-09-07 8:30 UTC (permalink / raw) To: Maxim Uvarov Cc: Ard Biesheuvel, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, Ilias Apalodimas On 07.09.20 09:00, Maxim Uvarov wrote: > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> In the memory map the regions with the lowest addresses may be of type >> EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the >> rest of the memory. So for calculating the maximum loading address for the >> device tree and the initial ramdisk image these reserved areas should not >> be taken into account. >> >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> >> --- >> drivers/firmware/efi/libstub/efi-stub.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c >> index c2484bf75c5d..13058ac75765 100644 >> --- a/drivers/firmware/efi/libstub/efi-stub.c >> +++ b/drivers/firmware/efi/libstub/efi-stub.c >> @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) >> map.map_end = map.map + map_size; >> >> for_each_efi_memory_desc_in_map(&map, md) { >> - if (md->attribute & EFI_MEMORY_WB) { >> + if (md->attribute & EFI_MEMORY_WB && >> + md->type != EFI_RESERVED_TYPE) { > > shouldn't the type here be CONVENTIONAL? In 32bit ARM reserve_kernel_base() the following are considered: * EFI_LOADER_CODE * EFI_LOADER_DATA * EFI_BOOT_SERVICES_CODE * EFI_BOOT_SERVICES_DATA * EFI_CONVENTIONAL_MEMORY What I have not yet fully understood is why Linux on ARM 32bit tries to put the kernel into the first 128 MiB. Cf. handle_kernel_image() in drivers/firmware/efi/libstub/arm32-stub.c. According to the comments this is due to some implementation detail in the Linux zImage decompressor and not required by UEFI or the hardware: Verify that the DRAM base address is compatible with the ARM boot protocol, which determines the base of DRAM by masking off the low 27 bits of the address at which the zImage is loaded. These assumptions are made by the decompressor, before any memory map is available. Best regards Heinrich > >> if (membase > md->phys_addr) >> membase = md->phys_addr; >> } >> -- >> 2.28.0 >> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-07 8:30 ` Heinrich Schuchardt @ 2020-09-07 10:21 ` Maxim Uvarov 2020-09-07 15:42 ` Maxim Uvarov 0 siblings, 1 reply; 11+ messages in thread From: Maxim Uvarov @ 2020-09-07 10:21 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Ard Biesheuvel, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, Ilias Apalodimas On Mon, 7 Sep 2020 at 11:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 07.09.20 09:00, Maxim Uvarov wrote: > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> In the memory map the regions with the lowest addresses may be of type > >> EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > >> rest of the memory. So for calculating the maximum loading address for the > >> device tree and the initial ramdisk image these reserved areas should not > >> be taken into account. > >> > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > >> --- > >> drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > >> index c2484bf75c5d..13058ac75765 100644 > >> --- a/drivers/firmware/efi/libstub/efi-stub.c > >> +++ b/drivers/firmware/efi/libstub/efi-stub.c > >> @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > >> map.map_end = map.map + map_size; > >> > >> for_each_efi_memory_desc_in_map(&map, md) { > >> - if (md->attribute & EFI_MEMORY_WB) { > >> + if (md->attribute & EFI_MEMORY_WB && > >> + md->type != EFI_RESERVED_TYPE) { > > > > shouldn't the type here be CONVENTIONAL? > > In 32bit ARM reserve_kernel_base() the following are considered: > > * EFI_LOADER_CODE > * EFI_LOADER_DATA > * EFI_BOOT_SERVICES_CODE > * EFI_BOOT_SERVICES_DATA > * EFI_CONVENTIONAL_MEMORY > > What I have not yet fully understood is why Linux on ARM 32bit tries to > put the kernel into the first 128 MiB. Cf. handle_kernel_image() in > drivers/firmware/efi/libstub/arm32-stub.c. > Are you looking to the latest kernel? https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/libstub/arm32-stub.c?h=v5.9-rc4#n211 efi_bs_call(allocate_pages,..) is only for EFI_CONVENTIONAL_MEMORY. > According to the comments this is due to some implementation detail in > the Linux zImage decompressor and not required by UEFI or the hardware: > > Verify that the DRAM base address is compatible with the ARM > boot protocol, which determines the base of DRAM by masking > off the low 27 bits of the address at which the zImage is > loaded. These assumptions are made by the decompressor, > before any memory map is available. I think that is also fixed with: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/efi/libstub/arm32-stub.c?h=v5.9-rc4&id=d0f9ca9be11f25ef4151195eab7ea36d136084f6 Maxim. > > Best regards > > Heinrich > > > > >> if (membase > md->phys_addr) > >> membase = md->phys_addr; > >> } > >> -- > >> 2.28.0 > >> > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-07 10:21 ` Maxim Uvarov @ 2020-09-07 15:42 ` Maxim Uvarov 0 siblings, 0 replies; 11+ messages in thread From: Maxim Uvarov @ 2020-09-07 15:42 UTC (permalink / raw) To: Heinrich Schuchardt Cc: Ard Biesheuvel, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, Ilias Apalodimas Tested both original and (md->type == EFI_CONVENTIONAL_MEMORY) versions - they fix qemu v7 boot under qemu. I think the second version is more correct. Regards, Maxim. On Mon, 7 Sep 2020 at 13:21, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > On Mon, 7 Sep 2020 at 11:31, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > On 07.09.20 09:00, Maxim Uvarov wrote: > > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > >> > > >> In the memory map the regions with the lowest addresses may be of type > > >> EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > >> rest of the memory. So for calculating the maximum loading address for the > > >> device tree and the initial ramdisk image these reserved areas should not > > >> be taken into account. > > >> > > >> Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > >> --- > > >> drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > >> 1 file changed, 2 insertions(+), 1 deletion(-) > > >> > > >> diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > >> index c2484bf75c5d..13058ac75765 100644 > > >> --- a/drivers/firmware/efi/libstub/efi-stub.c > > >> +++ b/drivers/firmware/efi/libstub/efi-stub.c > > >> @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > >> map.map_end = map.map + map_size; > > >> > > >> for_each_efi_memory_desc_in_map(&map, md) { > > >> - if (md->attribute & EFI_MEMORY_WB) { > > >> + if (md->attribute & EFI_MEMORY_WB && > > >> + md->type != EFI_RESERVED_TYPE) { > > > > > > shouldn't the type here be CONVENTIONAL? > > > > In 32bit ARM reserve_kernel_base() the following are considered: > > > > * EFI_LOADER_CODE > > * EFI_LOADER_DATA > > * EFI_BOOT_SERVICES_CODE > > * EFI_BOOT_SERVICES_DATA > > * EFI_CONVENTIONAL_MEMORY > > > > What I have not yet fully understood is why Linux on ARM 32bit tries to > > put the kernel into the first 128 MiB. Cf. handle_kernel_image() in > > drivers/firmware/efi/libstub/arm32-stub.c. > > > > Are you looking to the latest kernel? > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/firmware/efi/libstub/arm32-stub.c?h=v5.9-rc4#n211 > efi_bs_call(allocate_pages,..) is only for EFI_CONVENTIONAL_MEMORY. > > > According to the comments this is due to some implementation detail in > > the Linux zImage decompressor and not required by UEFI or the hardware: > > > > Verify that the DRAM base address is compatible with the ARM > > boot protocol, which determines the base of DRAM by masking > > off the low 27 bits of the address at which the zImage is > > loaded. These assumptions are made by the decompressor, > > before any memory map is available. > > I think that is also fixed with: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/efi/libstub/arm32-stub.c?h=v5.9-rc4&id=d0f9ca9be11f25ef4151195eab7ea36d136084f6 > > Maxim. > > > > > Best regards > > > > Heinrich > > > > > > > >> if (membase > md->phys_addr) > > >> membase = md->phys_addr; > > >> } > > >> -- > > >> 2.28.0 > > >> > > ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-04 15:50 [PATCH 1/1] efi/libstub: DRAM base calculation Heinrich Schuchardt 2020-09-07 7:00 ` Maxim Uvarov @ 2020-09-09 8:17 ` Ard Biesheuvel 2020-09-09 10:43 ` Maxim Uvarov 2020-09-09 20:36 ` Atish Patra 1 sibling, 2 replies; 11+ messages in thread From: Ard Biesheuvel @ 2020-09-09 8:17 UTC (permalink / raw) To: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt Cc: Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Maxim Uvarov, Jens Wiklander (+ Atish, Palmer) On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > In the memory map the regions with the lowest addresses may be of type > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > rest of the memory. So for calculating the maximum loading address for the > device tree and the initial ramdisk image these reserved areas should not > be taken into account. > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > --- > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index c2484bf75c5d..13058ac75765 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > map.map_end = map.map + map_size; > > for_each_efi_memory_desc_in_map(&map, md) { > - if (md->attribute & EFI_MEMORY_WB) { > + if (md->attribute & EFI_MEMORY_WB && > + md->type != EFI_RESERVED_TYPE) { > if (membase > md->phys_addr) > membase = md->phys_addr; > } > -- > 2.28.0 > This is not the right fix - on RPi2, for instance, which has some reserved memory at the base of DRAM, this change will result in the first 16 MB of memory to be wasted. What I would prefer to do is get rid of get_dram_base() entirely - arm64 does not use its return value in the first place, and for ARM, the only reason we need it is so that we can place the uncompressed kernel image as low in memory as possible, and there are probably better ways to do that. RISC-V just started using it too, but only passes it from handle_kernel_image() to efi_relocate_kernel(), and afaict, passing 0x0 there instead would not cause any problems. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-09 8:17 ` Ard Biesheuvel @ 2020-09-09 10:43 ` Maxim Uvarov 2020-09-09 10:46 ` Ard Biesheuvel 2020-09-09 20:36 ` Atish Patra 1 sibling, 1 reply; 11+ messages in thread From: Maxim Uvarov @ 2020-09-09 10:43 UTC (permalink / raw) To: Ard Biesheuvel Cc: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, François Ozog On Wed, 9 Sep 2020 at 11:17, Ard Biesheuvel <ardb@kernel.org> wrote: > > (+ Atish, Palmer) > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > In the memory map the regions with the lowest addresses may be of type > > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > rest of the memory. So for calculating the maximum loading address for the > > device tree and the initial ramdisk image these reserved areas should not > > be taken into account. > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > --- > > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > index c2484bf75c5d..13058ac75765 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > map.map_end = map.map + map_size; > > > > for_each_efi_memory_desc_in_map(&map, md) { > > - if (md->attribute & EFI_MEMORY_WB) { > > + if (md->attribute & EFI_MEMORY_WB && > > + md->type != EFI_RESERVED_TYPE) { > > if (membase > md->phys_addr) > > membase = md->phys_addr; > > } > > -- > > 2.28.0 > > > > This is not the right fix - on RPi2, for instance, which has some > reserved memory at the base of DRAM, this change will result in the > first 16 MB of memory to be wasted. > In the EFI memmap provided to the kernel efi stub it will be 2 regions. First is EFI_RESERVED and second is EFI_CONVENTIONAL_MEMORY. Even if they follow each other. And for_each_efi_memory_desc_in_map will just return the second one. Do not see where the problem is here. > What I would prefer to do is get rid of get_dram_base() entirely - > arm64 does not use its return value in the first place, and for ARM, > the only reason we need it is so that we can place the uncompressed > kernel image as low in memory as possible, and there are probably > better ways to do that. RISC-V just started using it too, but only > passes it from handle_kernel_image() to efi_relocate_kernel(), and > afaict, passing 0x0 there instead would not cause any problems. For prior 5.8 kernels there was limitation for maximum address to unpack the kernel. As I understand that was copy-pasted from x86 code, and now is missing in 5.9. That is why the suggestion was to point dram_base to the region where it's possible to allocate. I.e. I assume that patch was created not to the latest kernel. Removing the upper allocation limit should work here. Maxim. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-09 10:43 ` Maxim Uvarov @ 2020-09-09 10:46 ` Ard Biesheuvel 2020-09-09 11:04 ` Maxim Uvarov 0 siblings, 1 reply; 11+ messages in thread From: Ard Biesheuvel @ 2020-09-09 10:46 UTC (permalink / raw) To: Maxim Uvarov Cc: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, François Ozog On Wed, 9 Sep 2020 at 13:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > On Wed, 9 Sep 2020 at 11:17, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > (+ Atish, Palmer) > > > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > In the memory map the regions with the lowest addresses may be of type > > > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > > rest of the memory. So for calculating the maximum loading address for the > > > device tree and the initial ramdisk image these reserved areas should not > > > be taken into account. > > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > --- > > > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > > index c2484bf75c5d..13058ac75765 100644 > > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > > map.map_end = map.map + map_size; > > > > > > for_each_efi_memory_desc_in_map(&map, md) { > > > - if (md->attribute & EFI_MEMORY_WB) { > > > + if (md->attribute & EFI_MEMORY_WB && > > > + md->type != EFI_RESERVED_TYPE) { > > > if (membase > md->phys_addr) > > > membase = md->phys_addr; > > > } > > > -- > > > 2.28.0 > > > > > > > This is not the right fix - on RPi2, for instance, which has some > > reserved memory at the base of DRAM, this change will result in the > > first 16 MB of memory to be wasted. > > > In the EFI memmap provided to the kernel efi stub it will be 2 > regions. First is EFI_RESERVED and second is EFI_CONVENTIONAL_MEMORY. > Even if they follow each other. > And for_each_efi_memory_desc_in_map will just return the second one. > Do not see where the problem is here. > The base of DRAM will no longer start at a 16 MB aligned address on RPi, and so it will round up to the next 16 MB, wasting the space in between. > > What I would prefer to do is get rid of get_dram_base() entirely - > > arm64 does not use its return value in the first place, and for ARM, > > the only reason we need it is so that we can place the uncompressed > > kernel image as low in memory as possible, and there are probably > > better ways to do that. RISC-V just started using it too, but only > > passes it from handle_kernel_image() to efi_relocate_kernel(), and > > afaict, passing 0x0 there instead would not cause any problems. > > For prior 5.8 kernels there was limitation for maximum address to > unpack the kernel. As I understand that was copy-pasted from x86 code, > and now is missing in 5.9. What code are you referring to here? > That is why the suggestion was to point > dram_base to the region where it's possible to allocate. I.e. I assume > that > patch was created not to the latest kernel. Removing the upper > allocation limit should work here. > As I pointed out, this will regress other platforms. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-09 10:46 ` Ard Biesheuvel @ 2020-09-09 11:04 ` Maxim Uvarov 0 siblings, 0 replies; 11+ messages in thread From: Maxim Uvarov @ 2020-09-09 11:04 UTC (permalink / raw) To: Ard Biesheuvel Cc: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Jens Wiklander, François Ozog On Wed, 9 Sep 2020 at 13:47, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Wed, 9 Sep 2020 at 13:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote: > > > > On Wed, 9 Sep 2020 at 11:17, Ard Biesheuvel <ardb@kernel.org> wrote: > > > > > > (+ Atish, Palmer) > > > > > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > > > In the memory map the regions with the lowest addresses may be of type > > > > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > > > rest of the memory. So for calculating the maximum loading address for the > > > > device tree and the initial ramdisk image these reserved areas should not > > > > be taken into account. > > > > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > --- > > > > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > > > index c2484bf75c5d..13058ac75765 100644 > > > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > > > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > > > map.map_end = map.map + map_size; > > > > > > > > for_each_efi_memory_desc_in_map(&map, md) { > > > > - if (md->attribute & EFI_MEMORY_WB) { > > > > + if (md->attribute & EFI_MEMORY_WB && > > > > + md->type != EFI_RESERVED_TYPE) { > > > > if (membase > md->phys_addr) > > > > membase = md->phys_addr; > > > > } > > > > -- > > > > 2.28.0 > > > > > > > > > > This is not the right fix - on RPi2, for instance, which has some > > > reserved memory at the base of DRAM, this change will result in the > > > first 16 MB of memory to be wasted. > > > > > In the EFI memmap provided to the kernel efi stub it will be 2 > > regions. First is EFI_RESERVED and second is EFI_CONVENTIONAL_MEMORY. > > Even if they follow each other. > > And for_each_efi_memory_desc_in_map will just return the second one. > > Do not see where the problem is here. > > > > The base of DRAM will no longer start at a 16 MB aligned address on > RPi, and so it will round up to the next 16 MB, wasting the space in > between. > Ok. > > > What I would prefer to do is get rid of get_dram_base() entirely - > > > arm64 does not use its return value in the first place, and for ARM, > > > the only reason we need it is so that we can place the uncompressed > > > kernel image as low in memory as possible, and there are probably > > > better ways to do that. RISC-V just started using it too, but only > > > passes it from handle_kernel_image() to efi_relocate_kernel(), and > > > afaict, passing 0x0 there instead would not cause any problems. > > > > For prior 5.8 kernels there was limitation for maximum address to > > unpack the kernel. As I understand that was copy-pasted from x86 code, > > and now is missing in 5.9. > > What code are you referring to here? > to this commit: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/firmware/efi/libstub/arm32-stub.c?h=v5.9-rc4&id=d0f9ca9be11f25ef4151195eab7ea36d136084f6 > > That is why the suggestion was to point > > dram_base to the region where it's possible to allocate. I.e. I assume > > that > > patch was created not to the latest kernel. Removing the upper > > allocation limit should work here. > > > > As I pointed out, this will regress other platforms. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-09 8:17 ` Ard Biesheuvel 2020-09-09 10:43 ` Maxim Uvarov @ 2020-09-09 20:36 ` Atish Patra 2020-09-10 10:03 ` Ard Biesheuvel 1 sibling, 1 reply; 11+ messages in thread From: Atish Patra @ 2020-09-09 20:36 UTC (permalink / raw) To: Ard Biesheuvel Cc: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Maxim Uvarov, Jens Wiklander On Wed, Sep 9, 2020 at 1:17 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > (+ Atish, Palmer) > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > In the memory map the regions with the lowest addresses may be of type > > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > rest of the memory. So for calculating the maximum loading address for the > > device tree and the initial ramdisk image these reserved areas should not > > be taken into account. > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > --- > > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > index c2484bf75c5d..13058ac75765 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > map.map_end = map.map + map_size; > > > > for_each_efi_memory_desc_in_map(&map, md) { > > - if (md->attribute & EFI_MEMORY_WB) { > > + if (md->attribute & EFI_MEMORY_WB && > > + md->type != EFI_RESERVED_TYPE) { > > if (membase > md->phys_addr) > > membase = md->phys_addr; > > } > > -- > > 2.28.0 > > > > This is not the right fix - on RPi2, for instance, which has some > reserved memory at the base of DRAM, this change will result in the > first 16 MB of memory to be wasted. > > What I would prefer to do is get rid of get_dram_base() entirely - > arm64 does not use its return value in the first place, and for ARM, > the only reason we need it is so that we can place the uncompressed > kernel image as low in memory as possible, and there are probably > better ways to do that. RISC-V just started using it too, but only > passes it from handle_kernel_image() to efi_relocate_kernel(), and > afaict, passing 0x0 there instead would not cause any problems. Yes. Passing 0x0 to efi_relocate_kernel will result in a failure in efi_bs_call and it will fallback to efi_low_alloc_above which will try to assign the lowest possible available memory with 2MB alignment restriction. The only disadvantage is an extra unnecessary call to UEFI firmware which should be okay as it is not in the hotpath. -- Regards, Atish ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/1] efi/libstub: DRAM base calculation 2020-09-09 20:36 ` Atish Patra @ 2020-09-10 10:03 ` Ard Biesheuvel 0 siblings, 0 replies; 11+ messages in thread From: Ard Biesheuvel @ 2020-09-10 10:03 UTC (permalink / raw) To: Atish Patra Cc: Heinrich Schuchardt, Atish Patra, Palmer Dabbelt, Ingo Molnar, Arvind Sankar, linux-efi, Linux Kernel Mailing List, Maxim Uvarov, Jens Wiklander On Wed, 9 Sep 2020 at 23:37, Atish Patra <atishp@atishpatra.org> wrote: > > On Wed, Sep 9, 2020 at 1:17 AM Ard Biesheuvel <ardb@kernel.org> wrote: > > > > (+ Atish, Palmer) > > > > On Fri, 4 Sep 2020 at 18:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > > > > > In the memory map the regions with the lowest addresses may be of type > > > EFI_RESERVED_TYPE. The reserved areas may be discontinuous relative to the > > > rest of the memory. So for calculating the maximum loading address for the > > > device tree and the initial ramdisk image these reserved areas should not > > > be taken into account. > > > > > > Signed-off-by: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > --- > > > drivers/firmware/efi/libstub/efi-stub.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > > index c2484bf75c5d..13058ac75765 100644 > > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > > @@ -106,7 +106,8 @@ static unsigned long get_dram_base(void) > > > map.map_end = map.map + map_size; > > > > > > for_each_efi_memory_desc_in_map(&map, md) { > > > - if (md->attribute & EFI_MEMORY_WB) { > > > + if (md->attribute & EFI_MEMORY_WB && > > > + md->type != EFI_RESERVED_TYPE) { > > > if (membase > md->phys_addr) > > > membase = md->phys_addr; > > > } > > > -- > > > 2.28.0 > > > > > > > This is not the right fix - on RPi2, for instance, which has some > > reserved memory at the base of DRAM, this change will result in the > > first 16 MB of memory to be wasted. > > > > What I would prefer to do is get rid of get_dram_base() entirely - > > arm64 does not use its return value in the first place, and for ARM, > > the only reason we need it is so that we can place the uncompressed > > kernel image as low in memory as possible, and there are probably > > better ways to do that. RISC-V just started using it too, but only > > passes it from handle_kernel_image() to efi_relocate_kernel(), and > > afaict, passing 0x0 there instead would not cause any problems. > > Yes. Passing 0x0 to efi_relocate_kernel will result in a failure in > efi_bs_call and it will fallback to > efi_low_alloc_above which will try to assign the lowest possible > available memory with 2MB alignment restriction. > The only disadvantage is an extra unnecessary call to UEFI firmware > which should be okay as it is not in the hotpath. > The point is really that get_dram_base() does a similar call to GetMemoryMap() under the hood, so once we remove it, the worst case still does that once (in efi_low_alloc_above() invoked from efi_relocate_kernel) whereas the optimal case will no longer call it at all. ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-09-10 10:03 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-09-04 15:50 [PATCH 1/1] efi/libstub: DRAM base calculation Heinrich Schuchardt 2020-09-07 7:00 ` Maxim Uvarov 2020-09-07 8:30 ` Heinrich Schuchardt 2020-09-07 10:21 ` Maxim Uvarov 2020-09-07 15:42 ` Maxim Uvarov 2020-09-09 8:17 ` Ard Biesheuvel 2020-09-09 10:43 ` Maxim Uvarov 2020-09-09 10:46 ` Ard Biesheuvel 2020-09-09 11:04 ` Maxim Uvarov 2020-09-09 20:36 ` Atish Patra 2020-09-10 10:03 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).