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
