[PATCH 1/1] efi/libstub: DRAM base calculation

[PATCH 1/1] efi/libstub: DRAM base calculation

[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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Maxim Uvarov]

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
>

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Heinrich Schuchardt]

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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Maxim Uvarov]

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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Maxim Uvarov]

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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Ard Biesheuvel]

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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Maxim Uvarov]

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.

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Ard Biesheuvel]

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.

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Maxim Uvarov]

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.

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Atish Patra]

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

Re: [PATCH 1/1] efi/libstub: DRAM base calculation

[Ard Biesheuvel]

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.