Re: [PATCH] LoongArch: Add efistub booting support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi, Ard,

On Wed, Aug 17, 2022 at 3:00 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
>
> On Wed, 17 Aug 2022 at 08:43, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> >
> > Hi, Ard,
> >
> > On Tue, Aug 16, 2022 at 11:32 PM Ard Biesheuvel <ardb@xxxxxxxxxx> wrote:
> > >
> > > On Tue, 16 Aug 2022 at 17:23, Huacai Chen <chenhuacai@xxxxxxxxxx> wrote:
> > > >
> ...
> > > >
> > >
> > > 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
> > Emm,  in ARM64,
> > #define alloc_screen_info(x...)         &screen_info
> >
> > So screen_info is a global variable in the core kernel. For the zboot
> > case (our own implementation, not sure about the proposing new
> > method), efistub may be able to fill this info, but while
> > decompressing, screen_info will be overwritten. I think.
> >
>
> Right. So you can drop it then.
OK, then can we rename LINUX_EFI_ARM_SCREEN_INFO_TABLE_GUID to
LINUX_EFI_SCREEN_INFO_TABLE_GUID and avoid define a dedicated GUID for
each arch?

>
> >
> > >
> > > ...
> > > > > > +
> > > > > > +/*
> > > > > > + * 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.
> > OK, I will move SVAM to the stub.
> >
>
> OK
>
> > >
> > > ...
> > > > > > 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)
> > OK, then we will disable "initrd command line loader".
> >
>
> OK
>
> > > > > 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?
> > My mistake, the real reason of configuring DMW in stub is that the
> > address of real_kernel_entry() is a kernel va, not a efi va (which is
> > the same as pa).
> >
>
> That means you can move this code to efi_enter_kernel(), no?
Yes, we can move to efi_enter_kernel(), thank you.

Huacai



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux