On Tue, 16 Aug 2022 at 17:23, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
>
> Hi, Ard,
>
> On Tue, Aug 16, 2022 at 9:27 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> >
> > On Fri, 17 Jun 2022 at 16:56, Huacai Chen <chenhuacai@xxxxxxxxxxx> wrote:
> > >
...
> > > diff --git a/arch/loongarch/kernel/efi.c b/arch/loongarch/kernel/efi.c
> > > index a50b60c587fa..42f7cfe9ab03 100644
> > > --- a/arch/loongarch/kernel/efi.c
> > > +++ b/arch/loongarch/kernel/efi.c
> > > @@ -22,19 +22,141 @@
> > >
> > > #include <asm/early_ioremap.h>
> > > #include <asm/efi.h>
> > > +#include <asm/tlb.h>
> > > #include <asm/loongson.h>
> > >
> > > static unsigned long efi_nr_tables;
> > > static unsigned long efi_config_table;
> > > +static unsigned long screen_info_table __initdata = EFI_INVALID_TABLE_ADDR;
> > >
> > > static efi_system_table_t *efi_systab;
> > > -static efi_config_table_type_t arch_tables[] __initdata = {{},};
> > > +static efi_config_table_type_t arch_tables[] __initdata = {
> > > + {LINUX_EFI_LARCH_SCREEN_INFO_TABLE_GUID, &screen_info_table, "SINFO"},
> > > + {},
> > > +};
> > > +
> > > +static void __init init_screen_info(void)
> > > +{
> > > + struct screen_info *si;
> > > +
> > > + if (screen_info_table == EFI_INVALID_TABLE_ADDR)
> > > + return;
> > > +
> > > + si = early_memremap_ro(screen_info_table, sizeof(*si));
> > > + if (!si) {
> > > + pr_err("Could not map screen_info config table\n");
> > > + return;
> > > + }
> > > + screen_info = *si;
> > > + early_memunmap(si, sizeof(*si));
> > > +
> > > + if (screen_info.orig_video_isVGA == VIDEO_TYPE_EFI)
> > > + memblock_reserve(screen_info.lfb_base, screen_info.lfb_size);
> > > +}
> > > +
> >
> > The above may be unnecessary - the EFI stub is part of the core kernel
> > image, so you can access screen_info directly (please refer to the
> > arm64 port for an example)
> Though we haven't supported zboot yet, but you are proposing for that.
> I think in that case we should use dynamic allocation?
>
No that makes no difference. The point is that the EFI stub and the
core kernel are the same image, so when the stub runs, the core
kernel's screen_info already exists in memory - the only thing you
need to do is make it accessible by adding it to image-vars.h
...
> > > +
> > > +/*
> > > + * set_virtual_map() - create a virtual mapping for the EFI memory map and call
> > > + * efi_set_virtual_address_map enter virtual for runtime service
> > > + *
> > > + * This function populates the virt_addr fields of all memory region descriptors
> > > + * in @memory_map whose EFI_MEMORY_RUNTIME attribute is set. Those descriptors
> > > + * are also copied to @runtime_map, and their total count is returned in @count.
> > > + */
> >
> > You mentioned before that this must be done in the core kernel and not
> > in the EFI stub, but I don't remember the reason.
> >
> > Can you add a comment here why the below conversions cannot be done by
> > the EFI stub? Doing this in the stub removes the need to set up a 1:1
> > mapping just for a single invocation of SetVirtualAddressMap(), so if
> > there is any way to move this into the stub, I would strongly prefer
> > it.
> In the current implementation of generic efistub, efi runtime is in a
> separate address space, but we want to map efi runtime in the kernel
> address space. So, even if we do SVAM in stub, we still need to modify
> some code.
That is fine.
> And if we do SVAM in the core kernel, we don't need to
> modify generic stub (as a side effect, this makes the non-EFI kernel
> be also able to use efi runtime).
>
> If use efi runtime in non-EFI kernel is unacceptable, and if we are
> free to modify the generic stub, then we can move SVAM to the stub.
>
We should only support EFI runtime services when booting via the EFI stub.
...
> > > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > > index 7aa4717cdcac..9e4645e5a5c0 100644
> > > --- a/drivers/firmware/efi/Kconfig
> > > +++ b/drivers/firmware/efi/Kconfig
> > > @@ -118,7 +118,7 @@ config EFI_GENERIC_STUB
> > >
> > > config EFI_ARMSTUB_DTB_LOADER
> > > bool "Enable the DTB loader"
> > > - depends on EFI_GENERIC_STUB && !RISCV
> > > + depends on EFI_GENERIC_STUB && !RISCV && !LOONGARCH
> > > default y
> > > help
> > > Select this config option to add support for the dtb= command
> >
> > Please make the initrd command line loader depend on !LOONGARCH.
> > systemd-boot already supports this, and GRUB patches are on the list
> > (the one you quoted above is part of the series that adds support for
> > it). Your QEMU/edk2 firmware port also implements support for the
> > LoadFile2 based method.
> I agree to not select "initrd command line loader" by default, but can
> we have a chance to select it even just for debugging? Because
> sometimes we want to load the kernel and initrd via the command line
> in the EFI shell.
>
The EFI shell has a 'initrd' command which implements the LoadFile2
protocol: please refer to
OvmfPkg/LinuxInitrdDynamicShellCommand/LinuxInitrdDynamicShellCommand.inf
and include it in your build of the UEFI shell. (It can be added to
the UEFI shell even when it runs on non-QEMU systems)
> >
> > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > > index d0537573501e..1588c61939e7 100644
> > > --- a/drivers/firmware/efi/libstub/Makefile
> > > +++ b/drivers/firmware/efi/libstub/Makefile
> > > @@ -26,6 +26,8 @@ cflags-$(CONFIG_ARM) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > $(call cc-option,-mno-single-pic-base)
> > > cflags-$(CONFIG_RISCV) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > -fpic
> > > +cflags-$(CONFIG_LOONGARCH) := $(subst $(CC_FLAGS_FTRACE),,$(KBUILD_CFLAGS)) \
> > > + -fpic
> > >
> > > cflags-$(CONFIG_EFI_GENERIC_STUB) += -I$(srctree)/scripts/dtc/libfdt
> > >
> > > @@ -70,6 +72,8 @@ lib-$(CONFIG_ARM) += arm32-stub.o
> > > lib-$(CONFIG_ARM64) += arm64-stub.o
> > > lib-$(CONFIG_X86) += x86-stub.o
> > > lib-$(CONFIG_RISCV) += riscv-stub.o
> > > +lib-$(CONFIG_LOONGARCH) += loongarch-stub.o
> > > +
> > > CFLAGS_arm32-stub.o := -DTEXT_OFFSET=$(TEXT_OFFSET)
> > >
> > > # Even when -mbranch-protection=none is set, Clang will generate a
> > > @@ -125,6 +129,12 @@ STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
> > > --prefix-symbols=__efistub_
> > > STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
> > >
> > > +# For LoongArch, keep all the symbols in .init section and make sure that no
> > > +# absolute symbols references doesn't exist.
> > > +STUBCOPY_FLAGS-$(CONFIG_LOONGARCH) += --prefix-alloc-sections=.init \
> > > + --prefix-symbols=__efistub_
> > > +STUBCOPY_RELOC-$(CONFIG_LOONGARCH) := R_LARCH_MARK_LA
> > > +
> > > $(obj)/%.stub.o: $(obj)/%.o FORCE
> > > $(call if_changed,stubcopy)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/efi-stub-helper.c b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > index 3d972061c1b0..f612cfceda22 100644
> > > --- a/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > +++ b/drivers/firmware/efi/libstub/efi-stub-helper.c
> > > @@ -21,7 +21,7 @@
> > > bool efi_nochunk;
> > > bool efi_nokaslr = !IS_ENABLED(CONFIG_RANDOMIZE_BASE);
> > > int efi_loglevel = CONSOLE_LOGLEVEL_DEFAULT;
> > > -bool efi_novamap;
> > > +bool efi_novamap = IS_ENABLED(CONFIG_LOONGARCH); /* LoongArch call svam() in kernel */
> > >
> > > static bool efi_noinitrd;
> > > static bool efi_nosoftreserve;
> > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
> > > index f515394cce6e..730b7bd21776 100644
> > > --- a/drivers/firmware/efi/libstub/efi-stub.c
> > > +++ b/drivers/firmware/efi/libstub/efi-stub.c
> > > @@ -40,9 +40,9 @@
> > >
> > > #ifdef CONFIG_ARM64
> > > # define EFI_RT_VIRTUAL_LIMIT DEFAULT_MAP_WINDOW_64
> > > -#elif defined(CONFIG_RISCV)
> > > +#elif defined(CONFIG_RISCV) || defined(CONFIG_LOONGARCH)
> > > # define EFI_RT_VIRTUAL_LIMIT TASK_SIZE_MIN
> > > -#else
> > > +#else /* Only if TASK_SIZE is a constant */
> > > # define EFI_RT_VIRTUAL_LIMIT TASK_SIZE
> > > #endif
> > >
> > > diff --git a/drivers/firmware/efi/libstub/loongarch-stub.c b/drivers/firmware/efi/libstub/loongarch-stub.c
> > > new file mode 100644
> > > index 000000000000..beee086d9950
> > > --- /dev/null
> > > +++ b/drivers/firmware/efi/libstub/loongarch-stub.c
> > > @@ -0,0 +1,88 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * Author: Yun Liu <liuyun@xxxxxxxxxxx>
> > > + * Huacai Chen <chenhuacai@xxxxxxxxxxx>
> > > + * Copyright (C) 2020-2022 Loongson Technology Corporation Limited
> > > + */
> > > +
> > > +#include <linux/efi.h>
> > > +#include <asm/efi.h>
> > > +#include <asm/addrspace.h>
> > > +#include "efistub.h"
> > > +
> > > +typedef void __noreturn (*kernel_entry_t)(bool efi, unsigned long fdt);
> > > +
> > > +extern int kernel_asize;
> > > +extern int kernel_fsize;
> > > +extern int kernel_offset;
> > > +extern kernel_entry_t kernel_entry;
> > > +
> > > +static efi_guid_t screen_info_guid = LINUX_EFI_LARCH_SCREEN_INFO_TABLE_GUID;
> > > +
> > > +struct screen_info *alloc_screen_info(void)
> > > +{
> > > + efi_status_t status;
> > > + struct screen_info *si;
> > > +
> > > + status = efi_bs_call(allocate_pool,
> > > + EFI_RUNTIME_SERVICES_DATA, sizeof(*si), (void **)&si);
> > > + if (status != EFI_SUCCESS)
> > > + return NULL;
> > > +
> > > + status = efi_bs_call(install_configuration_table, &screen_info_guid, si);
> > > + if (status == EFI_SUCCESS)
> > > + return si;
> > > +
> > > + efi_bs_call(free_pool, si);
> > > +
> > > + return NULL;
> > > +}
> > > +
> > > +void free_screen_info(struct screen_info *si)
> > > +{
> > > + if (!si)
> > > + return;
> > > +
> > > + efi_bs_call(install_configuration_table, &screen_info_guid, NULL);
> > > + efi_bs_call(free_pool, si);
> > > +}
> > > +
> >
> > As indicated above, I think you can adopt the arm64 approach here instead.
> >
> > > +efi_status_t check_platform_features(void)
> > > +{
> > > + /* Config Direct Mapping */
> > > + csr_write64(CSR_DMW0_INIT, LOONGARCH_CSR_DMWIN0);
> > > + csr_write64(CSR_DMW1_INIT, LOONGARCH_CSR_DMWIN1);
> > > +
> > > + return EFI_SUCCESS;
> > > +}
> > > +
> >
> > This code is not checking a platform feature so it does not belong here.
> >
> > The EFI stub code is an ordinary EFI app, and it runs in the execution
> > context provided by EFI. So why is this needed so early? Can you move
> > it into the kernel entry routine instead?
> This is useful once we use our own zboot implementation, maybe we
> don't need it with the new method you are proposing.
>
If this is part of your zboot implementation, please drop it for now.
Let's try using the generic EFI zboot instead - if we need to, we can
find a way to add it there.
But out of curiosity, why is this needed at all?
