| Message ID | 20200909151623.16153-1-ardb@kernel.org |
|---|---|
| Headers | show |
| Series | efi/libstub: arm32: Remove dependency on dram_base | expand |
On Wed, 9 Sep 2020 at 18:26, Grant Likely <grant.likely@arm.com> wrote: > > > > On 09/09/2020 16:16, Ard Biesheuvel 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 reasonable to me. Presumably all special cases (platform specific > spin tables, etc) are covered as reserved in the UEFI memory map, correct? > Yes. The only reason the code is as it is today is for a proprietary HP Inc. platform that had a bootservicesdata allocation at the base of DRAM of 8 KiB, but this should now be covered in the same way as any other reserved region living in the window below TEXT_OFFSET. (Note that the current logic is flawed in any case: even though boot services regions are released to the OS at ExitBootServices(), there are types of EFI configuration tables that keep their contents in BsData regions as well, and those may get clobbered by the decompressed image) In summary, I am not expecting these changes to regress any platforms we care about (famous last words), and if it works, we can start removing the dram base handling code altogether (which currently gets called on arm64 as well, even though we never use the result)
On Wed, Sep 9, 2020 at 1:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Wed, 09 Sep 2020 08:16:20 PDT (-0700), ardb@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@linaro.org> > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > > Cc: Atish Patra <atish.patra@wdc.com> > > Cc: Palmer Dabbelt <palmer@dabbelt.com> > > Cc: Jens Wiklander <jens.wiklander@linaro.org> > > Cc: Francois Ozog <francois.ozog@linaro.org> > > Cc: Etienne CARRIERE <etienne.carriere@st.com> > > Cc: Takahiro Akashi <takahiro.akashi@linaro.org> > > Cc: Patrice CHOTARD <patrice.chotard@st.com> > > Cc: Sumit Garg <sumit.garg@linaro.org> > > Cc: Grant Likely <Grant.Likely@arm.com> > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > Cc: Christophe Priouzeau <christophe.priouzeau@linaro.org> > > Cc: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > Cc: Patrick DELAUNAY <patrick.delaunay@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(-)
On Thu, 10 Sep 2020 at 04:34, Atish Patra <atishp@atishpatra.org> wrote: > > On Wed, Sep 9, 2020 at 2:44 PM Atish Patra <atishp@atishpatra.org> wrote: > > > > On Wed, Sep 9, 2020 at 1:52 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > > > > > On Wed, 09 Sep 2020 08:16:20 PDT (-0700), ardb@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@linaro.org> > > > > Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> > > > > Cc: Atish Patra <atish.patra@wdc.com> > > > > Cc: Palmer Dabbelt <palmer@dabbelt.com> > > > > Cc: Jens Wiklander <jens.wiklander@linaro.org> > > > > Cc: Francois Ozog <francois.ozog@linaro.org> > > > > Cc: Etienne CARRIERE <etienne.carriere@st.com> > > > > Cc: Takahiro Akashi <takahiro.akashi@linaro.org> > > > > Cc: Patrice CHOTARD <patrice.chotard@st.com> > > > > Cc: Sumit Garg <sumit.garg@linaro.org> > > > > Cc: Grant Likely <Grant.Likely@arm.com> > > > > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > > > > Cc: Christophe Priouzeau <christophe.priouzeau@linaro.org> > > > > Cc: Rouven Czerwinski <r.czerwinski@pengutronix.de> > > > > Cc: Patrick DELAUNAY <patrick.delaunay@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@wdc.com> Thanks for confirming.
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. Cc: Maxim Uvarov <maxim.uvarov@linaro.org> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de> Cc: Atish Patra <atish.patra@wdc.com> Cc: Palmer Dabbelt <palmer@dabbelt.com> Cc: Jens Wiklander <jens.wiklander@linaro.org> Cc: Francois Ozog <francois.ozog@linaro.org> Cc: Etienne CARRIERE <etienne.carriere@st.com> Cc: Takahiro Akashi <takahiro.akashi@linaro.org> Cc: Patrice CHOTARD <patrice.chotard@st.com> Cc: Sumit Garg <sumit.garg@linaro.org> Cc: Grant Likely <Grant.Likely@arm.com> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Christophe Priouzeau <christophe.priouzeau@linaro.org> Cc: Rouven Czerwinski <r.czerwinski@pengutronix.de> Cc: Patrick DELAUNAY <patrick.delaunay@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(-)