On Tue, 19 Dec 2023 at 07:40, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
>
> Hi, Ard and Yao,
>
> The logic of this patch seems right, but I see this in arm64 code:
>
> if (image->image_base != _text) {
> efi_err("FIRMWARE BUG: efi_loaded_image_t::image_base
> has bogus value\n");
> image->image_base = _text;
> }
>
> Does this mean image_base is unreliable?
>
Only if you use an old distro fork of GRUB. Those would 'repurpose'
the LoadImage protocol created by the firmware to load GRUB by
updating some fields and passing the same protocol to the kernel as
the kernel's loaded image, but failed to set the image_base correctly.
I don't think this is a concern for LoongArch.
> The arm64 code is introduced in commit c2136dceba9a329e997ccce3277
> ("efi/libstub/arm64: Avoid image_base value from efi_loaded_image"),
> and I don't know whether LoongArch has similar problems.
>
> Huacai
>
> On Tue, Dec 19, 2023 at 1:36 PM <wangyao@xxxxxxxxxx> wrote:
> >
> > From: Wang Yao <wangyao@xxxxxxxxxx>
> >
> > Replace kernel_offset with image_base to positon the image file that has
> > been loaded by UEFI or GRUB.
> >
Why?
When you write a commit log, focus on the why not on the how (we can
all see the 'how' by reading the patch itself)
> > Signed-off-by: Wang Yao <wangyao@xxxxxxxxxx>
> > ---
> > arch/loongarch/include/asm/efi.h | 2 --
> > arch/loongarch/kernel/head.S | 1 -
> > arch/loongarch/kernel/image-vars.h | 1 -
> > arch/loongarch/kernel/vmlinux.lds.S | 1 -
> > drivers/firmware/efi/libstub/loongarch-stub.c | 9 +++++----
> > drivers/firmware/efi/libstub/loongarch-stub.h | 4 ++++
> > drivers/firmware/efi/libstub/loongarch.c | 6 ++++--
> > 7 files changed, 13 insertions(+), 11 deletions(-)
> > create mode 100644 drivers/firmware/efi/libstub/loongarch-stub.h
> >
> > diff --git a/arch/loongarch/include/asm/efi.h b/arch/loongarch/include/asm/efi.h
> > index 91d81f9730ab..eddc8e79b3fa 100644
> > --- a/arch/loongarch/include/asm/efi.h
> > +++ b/arch/loongarch/include/asm/efi.h
> > @@ -32,6 +32,4 @@ static inline unsigned long efi_get_kimg_min_align(void)
> >
> > #define EFI_KIMG_PREFERRED_ADDRESS PHYSADDR(VMLINUX_LOAD_ADDRESS)
> >
> > -unsigned long kernel_entry_address(unsigned long kernel_addr);
> > -
> > #endif /* _ASM_LOONGARCH_EFI_H */
> > diff --git a/arch/loongarch/kernel/head.S b/arch/loongarch/kernel/head.S
> > index 53b883db0786..0ecab4216392 100644
> > --- a/arch/loongarch/kernel/head.S
> > +++ b/arch/loongarch/kernel/head.S
> > @@ -34,7 +34,6 @@ pe_header:
> >
> > SYM_DATA(kernel_asize, .long _kernel_asize);
> > SYM_DATA(kernel_fsize, .long _kernel_fsize);
> > -SYM_DATA(kernel_offset, .long _kernel_offset);
> >
> > #endif
> >
> > diff --git a/arch/loongarch/kernel/image-vars.h b/arch/loongarch/kernel/image-vars.h
> > index 5087416b9678..41ddcf56d21c 100644
> > --- a/arch/loongarch/kernel/image-vars.h
> > +++ b/arch/loongarch/kernel/image-vars.h
> > @@ -11,7 +11,6 @@ __efistub_strcmp = strcmp;
> > __efistub_kernel_entry = kernel_entry;
> > __efistub_kernel_asize = kernel_asize;
> > __efistub_kernel_fsize = kernel_fsize;
> > -__efistub_kernel_offset = kernel_offset;
> > #if defined(CONFIG_EFI_EARLYCON) || defined(CONFIG_SYSFB)
> > __efistub_screen_info = screen_info;
> > #endif
> > diff --git a/arch/loongarch/kernel/vmlinux.lds.S b/arch/loongarch/kernel/vmlinux.lds.S
> > index bb2ec86f37a8..a5d0cd2035da 100644
> > --- a/arch/loongarch/kernel/vmlinux.lds.S
> > +++ b/arch/loongarch/kernel/vmlinux.lds.S
> > @@ -143,7 +143,6 @@ SECTIONS
> > _kernel_fsize = _edata - _text;
> > _kernel_vsize = _end - __initdata_begin;
> > _kernel_rsize = _edata - __initdata_begin;
> > - _kernel_offset = kernel_offset - _text;
> > #endif
> >
> > .gptab.sdata : {
> > diff --git a/drivers/firmware/efi/libstub/loongarch-stub.c b/drivers/firmware/efi/libstub/loongarch-stub.c
> > index d6ec5d4b8dbe..736b6aae323d 100644
> > --- a/drivers/firmware/efi/libstub/loongarch-stub.c
> > +++ b/drivers/firmware/efi/libstub/loongarch-stub.c
> > @@ -8,10 +8,10 @@
> > #include <asm/efi.h>
> > #include <asm/addrspace.h>
> > #include "efistub.h"
> > +#include "loongarch-stub.h"
> >
> > extern int kernel_asize;
> > extern int kernel_fsize;
> > -extern int kernel_offset;
> > extern int kernel_entry;
> >
> > efi_status_t handle_kernel_image(unsigned long *image_addr,
> > @@ -24,7 +24,7 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > efi_status_t status;
> > unsigned long kernel_addr = 0;
> >
> > - kernel_addr = (unsigned long)&kernel_offset - kernel_offset;
> > + kernel_addr = (unsigned long)image->image_base;
> >
> > status = efi_relocate_kernel(&kernel_addr, kernel_fsize, kernel_asize,
> > EFI_KIMG_PREFERRED_ADDRESS, efi_get_kimg_min_align(), 0x0);
> > @@ -35,9 +35,10 @@ efi_status_t handle_kernel_image(unsigned long *image_addr,
> > return status;
> > }
> >
> > -unsigned long kernel_entry_address(unsigned long kernel_addr)
> > +unsigned long kernel_entry_address(unsigned long kernel_addr,
> > + efi_loaded_image_t *image)
> > {
> > - unsigned long base = (unsigned long)&kernel_offset - kernel_offset;
> > + unsigned long base = (unsigned long)image->image_base;
> >
> > return (unsigned long)&kernel_entry - base + kernel_addr;
> > }
> > diff --git a/drivers/firmware/efi/libstub/loongarch-stub.h b/drivers/firmware/efi/libstub/loongarch-stub.h
> > new file mode 100644
> > index 000000000000..cd015955a015
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/loongarch-stub.h
> > @@ -0,0 +1,4 @@
> > +/* SPDX-License-Identifier: GPL-2.0-only */
> > +
> > +unsigned long kernel_entry_address(unsigned long kernel_addr,
> > + efi_loaded_image_t *image);
> > diff --git a/drivers/firmware/efi/libstub/loongarch.c b/drivers/firmware/efi/libstub/loongarch.c
> > index 0e0aa6cda73f..684c9354637c 100644
> > --- a/drivers/firmware/efi/libstub/loongarch.c
> > +++ b/drivers/firmware/efi/libstub/loongarch.c
> > @@ -8,6 +8,7 @@
> > #include <asm/efi.h>
> > #include <asm/addrspace.h>
> > #include "efistub.h"
> > +#include "loongarch-stub.h"
> >
> > typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long cmdline,
> > unsigned long systab);
> > @@ -37,7 +38,8 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map, void *priv)
> > return EFI_SUCCESS;
> > }
> >
> > -unsigned long __weak kernel_entry_address(unsigned long kernel_addr)
> > +unsigned long __weak kernel_entry_address(unsigned long kernel_addr,
> > + efi_loaded_image_t *image)
> > {
> > return *(unsigned long *)(kernel_addr + 8) - VMLINUX_LOAD_ADDRESS + kernel_addr;
> > }
> > @@ -73,7 +75,7 @@ efi_status_t efi_boot_kernel(void *handle, efi_loaded_image_t *image,
> > csr_write64(CSR_DMW0_INIT, LOONGARCH_CSR_DMWIN0);
> > csr_write64(CSR_DMW1_INIT, LOONGARCH_CSR_DMWIN1);
> >
> > - real_kernel_entry = (void *)kernel_entry_address(kernel_addr);
> > + real_kernel_entry = (void *)kernel_entry_address(kernel_addr, image);
> >
> > real_kernel_entry(true, (unsigned long)cmdline_ptr,
> > (unsigned long)efi_system_table);
> > --
> > 2.27.0
> >
