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

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

--
2.28.0

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
>

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

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.

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.