[PATCH RFC/RFT 0/3] efi/libstub: arm32: Remove dependency on dram_base

Thu Sep 10 19:32:37 EDT 2020

On Thu, Sep 10, 2020 at 7:08 AM Ard Biesheuvel <ardb at kernel.org> wrote:
>
> On Thu, 10 Sep 2020 at 13:04, Ard Biesheuvel <ardb at kernel.org> wrote:
> >
> > On Thu, 10 Sep 2020 at 04:34, Atish Patra <atishp at atishpatra.org> wrote:
> > >
> > > On Wed, Sep 9, 2020 at 2:44 PM Atish Patra <atishp at atishpatra.org> wrote:
> > > >
> > > > On Wed, Sep 9, 2020 at 1:52 PM Palmer Dabbelt <palmer at dabbelt.com> wrote:
> > > > >
> > > > > On Wed, 09 Sep 2020 08:16:20 PDT (-0700), ardb at kernel.org wrote:
> > > > > > Maxim reports boot failures on platforms that describe reserved memory
> > > > > > regions in DT that are disjoint from system DRAM, and which are converted
> > > > > > to EfiReservedMemory regions by the EFI subsystem in u-boot.
> > > > > >
> > > > > > As it turns out, the whole notion of discovering the base of DRAM is
> > > > > > problematic, and it would be better to simply rely on the EFI memory
> > > > > > allocation routines instead, and derive the FDT and initrd allocation
> > > > > > limits from the actual placement of the kernel (which is what defines
> > > > > > the start of the linear region anyway)
> > > > > >
> > > > > > Finally, we should be able to get rid of get_dram_base() entirely.
> > > > > > However, as RISC-V only just started using it, we will need to address
> > > > > > that at a later time.
> > > > >
> > > > > Looks like we're using dram_base to derive two argumets to
> > > > > efi_relocate_kernel(): the preferred load address and the minimum load address.
> > > > > I don't see any reason why we can't use the same PAGE_OFFSET-like logic that
> > > > > x86 uses for the minimum load address, but I don't think we have any mechanism
> > > > > like "struct boot_params" so we'd need to come up with something.
> > > > >
> > > >
> > > > As discussed in the other thread
> > > > (https://www.spinics.net/lists/linux-efi/msg20262.html),
> > > > we don't need to do anything special. efi_relocate_kernel can just
> > > > take preferred address as 0
> > > > so that efi_bs_alloc will fail and efi_low_alloc_above will be used to
> > > > allocate 2MB/4MB aligned address as per requirement.
> > > >
> > > > I don't think the other changes in this series will cause any issue
> > > > for RISC-V. I will test it and update anyways.
> > > >
> > > > > > Cc: Maxim Uvarov <maxim.uvarov at linaro.org>
> > > > > > Cc: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > > > > Cc: Atish Patra <atish.patra at wdc.com>
> > > > > > Cc: Palmer Dabbelt <palmer at dabbelt.com>
> > > > > > Cc: Jens Wiklander <jens.wiklander at linaro.org>
> > > > > > Cc: Francois Ozog <francois.ozog at linaro.org>
> > > > > > Cc: Etienne CARRIERE <etienne.carriere at st.com>
> > > > > > Cc: Takahiro Akashi <takahiro.akashi at linaro.org>
> > > > > > Cc: Patrice CHOTARD <patrice.chotard at st.com>
> > > > > > Cc: Sumit Garg <sumit.garg at linaro.org>
> > > > > > Cc: Grant Likely <Grant.Likely at arm.com>
> > > > > > Cc: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > > > > > Cc: Christophe Priouzeau <christophe.priouzeau at linaro.org>
> > > > > > Cc: Rouven Czerwinski <r.czerwinski at pengutronix.de>
> > > > > > Cc: Patrick DELAUNAY <patrick.delaunay at st.com>
> > > > > >
> > > > > > Ard Biesheuvel (3):
> > > > > >   efi/libstub: Export efi_low_alloc_above() to other units
> > > > > >   efi/libstub: Use low allocation for the uncompressed kernel
> > > > > >   efi/libstub: base FDT and initrd placement on image address not DRAM
> > > > > >     base
> > > > > >
> > > > > >  arch/arm/include/asm/efi.h                |   6 +-
> > > > > >  arch/arm64/include/asm/efi.h              |   2 +-
> > > > > >  drivers/firmware/efi/libstub/arm32-stub.c | 177 ++++----------------
> > > > > >  drivers/firmware/efi/libstub/efi-stub.c   |   2 +-
> > > > > >  drivers/firmware/efi/libstub/efistub.h    |   3 +
> > > > > >  drivers/firmware/efi/libstub/relocate.c   |   4 +-
> > > > > >  6 files changed, 47 insertions(+), 147 deletions(-)
> > > >
> > >
> > > I verified the above patches along with the following RISC-V specific changes.
> > >
> > > diff --git a/arch/riscv/include/asm/efi.h b/arch/riscv/include/asm/efi.h
> > > index 93c305a638f4..dd6ceea9d548 100644
> > > --- a/arch/riscv/include/asm/efi.h
> > > +++ b/arch/riscv/include/asm/efi.h
> > > @@ -37,7 +37,7 @@ static inline unsigned long
> > > efi_get_max_fdt_addr(unsigned long dram_base)
> > >  static inline unsigned long efi_get_max_initrd_addr(unsigned long dram_base,
> > >                                                     unsigned long image_addr)
> > >  {
> > > -       return dram_base + SZ_256M;
> > > +       return image_addr + SZ_256M;
> > >  }
> > >
> >
> > Ah yes, we need this change as well - this is a bit unfortunate since
> > that creates a conflict with the RISC-V tree.
> >
> > > --- a/drivers/firmware/efi/libstub/riscv-stub.c
> > > +++ b/drivers/firmware/efi/libstub/riscv-stub.c
> > > @@ -100,7 +100,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > >          */
> > >         preferred_addr = round_up(dram_base, MIN_KIMG_ALIGN) + MIN_KIMG_ALIGN;
> > >         status = efi_relocate_kernel(image_addr, kernel_size, *image_size,
> > > -                                    preferred_addr, MIN_KIMG_ALIGN, dram_base);
> > > +                                    0, MIN_KIMG_ALIGN, 0);
> > >
> > > FWIW: Tested-by: Atish Patra <atish.patra at wdc.com>
> >
> > Thanks for confirming.
>
> OK,
>
> So, just to annoy Palmer and you more than I already have up to this
> point: any chance we could do a final respin of the RISC-V code on top
> of these changes? They are important for ARM, and I would prefer these
> to be merged in a way that makes it easy to backport them to -stable
> if needed.
>

No worries. It's better to address these issues now rather than
patching it after the code is merged.
I will rebase and update the RISC-V patch series on top of this series
as per above discussion.

Should I also add a patch to remove get_dram_base() completely or are
you planning to do that ?

> So what I would suggest is:
> - I will create a new 'shared-efi' tag/stable branch containing the
> existing two patches, as well as these changes (in a slightly updated
> form)
> - Palmer creates a new topic branch in the riscv repo based on this
> shared tag, and applies the [updated] RISC-V patches on top
> - Palmer drops the current version of the riscv patches from
> riscv/for-next, and merges the topic branch into it instead.
>
> Again, sorry to be a pain, but I think this is the cleanest way to get
> these changes queued up for v5.10 without painting ourselves into a
> corner too much when it comes to future follow-up changes.

Sounds good to me. I will try to send a v8 early next week and let
palmer decide how he wants to proceed.

-- 
Regards,
Atish