From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id A75B3C54EE9 for ; Mon, 19 Sep 2022 15:09:12 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229568AbiISPJL (ORCPT ); Mon, 19 Sep 2022 11:09:11 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56200 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229522AbiISPJK (ORCPT ); Mon, 19 Sep 2022 11:09:10 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 932242A410 for ; Mon, 19 Sep 2022 08:09:09 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 1DC23614B3 for ; Mon, 19 Sep 2022 15:09:09 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 7A99EC433C1 for ; Mon, 19 Sep 2022 15:09:08 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1663600148; bh=L2PvyrR/OZWJTFXPX/Q4dsbml9IKn0Gx6ZddXYNs8Lk=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=rebCJRboqTkQhT11UXACbNbLVBPYyqeWo74qo+8OD85zETVbCxoYtmmWwIiTpKZte ojo4lN8kC6Z7iSTO3UkDhrqNPgoH9NiiQ2eP6vka96W0JhFieYfpD3vHSYtsPWm1gF 7pYg2nEt6sbBbHHU7ukC3NQ5ExJDUrp+5vDiGGO/HG0AKy4SgS4v6FE0hXJSHwxJPh K2hjloErPmjbUFa0NOt9fED1zhdtkcR3W8CHgWDJ/uCr1a17hj4QcNu9Fxc2mHaIQX YYlET2GRoT6AV15UZgRegQenm9NM3x9N4ctkUjZFpIt1KhWmbi9SNlVyuOQZML5J7F aMMcKoU7tiG8g== Received: by mail-vs1-f52.google.com with SMTP id p4so12037016vsa.9 for ; Mon, 19 Sep 2022 08:09:08 -0700 (PDT) X-Gm-Message-State: ACrzQf2sSaU/BNp6nPTcBau2R4Ngt5oCKtEPZ3Vd5aIp8PoL973b44sQ 56vT3FXjVky1kjnKUkLbjiY2XNCgmDq5H8zYO0g= X-Google-Smtp-Source: AMsMyM4s6SR0LC4Yf74FuSChbh0IrvX8w8NLEv6Sy414stl1b8GzvcyuNLkL39ZvJTzdZHxq1LXXDdGq/X7uX14jTME= X-Received: by 2002:a67:d491:0:b0:398:1bbc:bc85 with SMTP id g17-20020a67d491000000b003981bbcbc85mr6219564vsj.59.1663600147323; Mon, 19 Sep 2022 08:09:07 -0700 (PDT) MIME-Version: 1.0 References: <20220918213544.2176249-1-ardb@kernel.org> <20220918213544.2176249-12-ardb@kernel.org> In-Reply-To: From: Huacai Chen Date: Mon, 19 Sep 2022 23:08:54 +0800 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 11/12] efi/loongarch: libstub: remove dependency on flattened DT To: Ard Biesheuvel Cc: linux-efi , loongarch@lists.linux.dev, "Russell King (Oracle)" , Arnd Bergmann , Ilias Apalodimas , Huacai Chen , Xi Ruoyao Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-efi@vger.kernel.org On Mon, Sep 19, 2022 at 10:44 PM Ard Biesheuvel wrote: > > On Mon, 19 Sept 2022 at 16:43, Huacai Chen wrote: > > > > On Mon, Sep 19, 2022 at 10:32 PM Ard Biesheuvel wrote: > > > > > > On Mon, 19 Sept 2022 at 16:25, Huacai Chen wrote: > > > > > > > > Hi, Ard, > > > > > > > > On Mon, Sep 19, 2022 at 8:27 PM Ard Biesheuvel wrote: > > > > > > > > > > On Mon, 19 Sept 2022 at 14:15, Huacai Chen wrote: > > > > > > > > > > > > On Mon, Sep 19, 2022 at 8:11 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 13:58, Huacai Chen wrote: > > > > > > > > > > > > > > > > On Mon, Sep 19, 2022 at 7:21 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 13:15, Huacai Chen wrote: > > > > > > > > > > > > > > > > > > > > On Mon, Sep 19, 2022 at 6:49 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 12:47, Huacai Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 19, 2022 at 6:37 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 12:33, Huacai Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, Ard, > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 19, 2022 at 2:22 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 08:06, Huacai Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, Ard, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Sep 19, 2022 at 1:15 PM Ard Biesheuvel wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Mon, 19 Sept 2022 at 03:58, Huacai Chen wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi, Ard, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I think the parameters passed to the core kernel need to be discussed. > > > > > > > > > > > > > > > > > > The old way (so-called old world): > > > > > > > > > > > > > > > > > > a0=argc, a1=argv, a1=bpi > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The current way (so-called new world): > > > > > > > > > > > > > > > > > > a0=efi boot flag, a1=fdt pointer > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > The new way (in this patchset): > > > > > > > > > > > > > > > > > > a0=efi boot flag, a1=systemtable, a2=cmdline > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > I prefer to use the current way, there are 3 reasons: > > > > > > > > > > > > > > > > > > 1, both acpi system and dt system can use the same parameters passing method; > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > DT systems will use this too. The distinction is between EFI boot and > > > > > > > > > > > > > > > > > non-EFI boot. We *really* should keep these separate, given the > > > > > > > > > > > > > > > > > experience on ARM, where other projects invent ways to pass those > > > > > > > > > > > > > > > > > values to the kernel without going through the stub. > > > > > > > > > > > > > > > > In the last patch I see: > > > > > > > > > > > > > > > > + void *fdt_ptr = early_memremap_ro(fw_arg1, SZ_64K); > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > + early_init_dt_scan(fdt_ptr); > > > > > > > > > > > > > > > > + early_init_fdt_reserve_self(); > > > > > > > > > > > > > > > > + > > > > > > > > > > > > > > > > clear_bit(EFI_BOOT, &efi.flags); > > > > > > > > > > > > > > > > So I suppose for the DT system that means a0=efi boot flag, a1=fdt > > > > > > > > > > > > > > > > pointer, a2=cmdline? Then it is not exactly the same as the ACPI > > > > > > > > > > > > > > > > system, but similar. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > No, for non-EFI DT boot, the command line is passed via the DT, so > > > > > > > > > > > > > > > a0=0x0 (non-efi), a1=DT, a2=0x0 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Do you intend to support non-EFI DT boot by the way? > > > > > > > > > > > > > > I think we needn't support non-EFI DT boot, so a0=efi boot flag, > > > > > > > > > > > > > > a1=systemtable, a2=cmdline is just OK (or maybe we can exchange a1/a2, > > > > > > > > > > > > > > which looks similar to the old-world). > > > > > > > > > > > > > > > > > > > > > > > > > > Excellent. If non-EFI boot is not supported, we can drop the code that > > > > > > > > > > > > > deals with that. > > > > > > > > > > > > > > > > > > > > > > > > > > > But I have another question: is > > > > > > > > > > > > > > it early enough to get DT from systemtable for DT boot (in the current > > > > > > > > > > > > > > way DT is the earliest thing)? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > If you rely on DT only for the hardware description, then loading it > > > > > > > > > > > > > from efi_init() should be fine. > > > > > > > > > > > > OK, then please drop patch #12 at this time. It can be added when we > > > > > > > > > > > > add Loongson-2K support. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > OK > > > > > > > > > > > > > > > > > > > > > > > > > > I don't think it makes sense to allow efi=novamap on LoongArch, given > > > > > > > > > > > > > > > that we cannot make use of the runtime services that way. So in my > > > > > > > > > > > > > > > code, efi_novamap is set to false unconditionally. > > > > > > > > > > > > > > Emm, I prefer to support "efi=novamap", the core kernel has already > > > > > > > > > > > > > > supported "noefi" to disable runtime, I don't want to hack > > > > > > > > > > > > > > efi_novamap. So please keep the efi boot flag, thanks. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Fair enough. Do you have any uses for efi_novamap in mind? > > > > > > > > > > > > I usually use "efi=novamap" in EFI shell to see whether it can work > > > > > > > > > > > > well without runtime. And I think I will modify this patch these days > > > > > > > > > > > > because in another thread you said that this series cannot boot. > > > > > > > > > > > > > > > > > > > > > > It works fine now. > > > > > > > > > > > > > > > > > > > > > > However, clearing the EFI_BOOT flag is not the correct way to disable > > > > > > > > > > > runtime services only. And note that we also have efi=noruntime - does > > > > > > > > > > > that currently work on LoongArch? > > > > > > > > > > OK, but we don't need to add a new "efi=noruntime" parameter, I can > > > > > > > > > > use "noefi" instead. > > > > > > > > > > > > > > > > > > OK > > > > > > > > > > > > > > > > > > > But I think support "efi=novamap" is not a bad > > > > > > > > > > idea (maybe I misunderstood its usage). > > > > > > > > > > > > > > > > > > > > > > > > > > > > It basically exists to deal with broken EFI firmware on ARM laptops > > > > > > > > > that were only intended to run Windows. Windows calls > > > > > > > > > SetVirtualAddressMap() *after* creating the new mappings, and some > > > > > > > > > broken firmware drivers will access those regions too early. > > > > > > > > > > > > > > > > > > On Linux, we install the mapping first, and then much later, we > > > > > > > > > actually create the mappings and only activate them on a single CPU > > > > > > > > > while the EFI runtime service call is in progress. > > > > > > > > > > > > > > > > > > In any case, I will reinstate the efi_novamap logic for now - we can > > > > > > > > > always revisit it later. > > > > > > > > OK, now I think there are no big problems. Only some bikesheddings: > > > > > > > > 1, Please use "a0=efi boot flag, a1=cmdline a2=systemtable" to pass > > > > > > > > boot information, it looks most similar to the old-world, and we can > > > > > > > > distinguish old-world/new-world by judging whether a0 is greater than > > > > > > > > 1. > > > > > > > > 2, I prefer "early return" in efi_init, i.e., if (boot_memmap == > > > > > > > > EFI_INVALID_TABLE_ADDR) then return immediately, this makes > > > > > > > > indentation look better. > > > > > > > > 3, Declare "cmdline" out of the if() condition in init_environ() looks better. > > > > > > > > 4, I prefer "kernel_addr" rather than "image_addr" in efi_boot_kernel(). > > > > > > > > 5, It seems that one line is enough for the last statement in efi_boot_kernel(). > > > > > > > > > > > > > > > > > > > > > > OK > > > > > > > > > > > > > > > Hope this series will be stable as soon as possible, our kexec/kdump > > > > > > > > work needs to adjust because of this change. :) > > > > > > > > > > > > > > > > > > > > > > Do you have a link to those patches? > > > > > > There is a snapshot for patches pending for 6.1: > > > > > > https://github.com/loongson/linux/commits/loongarch-next > > > > > > > > > > Thanks. I already spotted an issue there which is exactly the kind of > > > > > thing I am trying to avoid with this series. > > > > > > > > > > diff --git a/arch/loongarch/kernel/mem.c b/arch/loongarch/kernel/mem.c > > > > > index 7423361b0ebc9b..c68c88e40cc0b9 100644 > > > > > --- a/arch/loongarch/kernel/mem.c > > > > > +++ b/arch/loongarch/kernel/mem.c > > > > > @@ -61,4 +62,9 @@ void __init memblock_init(void) > > > > > > > > > > /* Reserve the initrd */ > > > > > reserve_initrd_mem(); > > > > > + > > > > > + /* Main reserved memory for the elf core head */ > > > > > + early_init_fdt_scan_reserved_mem(); > > > > > + /* Parse linux,usable-memory-range for crash dump kernel */ > > > > > + early_init_dt_check_for_usable_mem_range(); > > > > > } > > > > > > > > > > So here, we are adding support for DT memory reservations, which kdump > > > > > apparently needs. > > > > > > > > > > However, this parsing of the DT not only happens on kexec boot: all > > > > > ACPI and DT kernels will now honour FDT memory reservations passed in > > > > > via a DT when booting the first kernel, and external projects may use > > > > > this and start to depend on this. > > > > > > > > > > Once something like this hits the upstream kernel, it is *very* > > > > > difficult to change or remove. > > > > > > > > > > If you only need to pass the usable memory range for kcrash/kdump, > > > > > it's probably better to use a command line option for that, instead of > > > > > relying on DT memory reservations. > > > > Thank you for your suggestion, but we found some trouble when handle > > > > initrd in kexec. > > > > In current implementation, we generate a fdt in kexec-tools, then fill > > > > "linux,usable-memory-range", "linux,elfcorehdr" and initrd information > > > > in it. After this series, we can use "mem=xxx" "elfcorehdr=" in > > > > cmdline, but how to handle initrd information which is stored in a > > > > system table? > > > > > > > > > > There are two options: > > > - use initrdmem= on the command line > > This is not a good way, even if the second kernel handle "initrdmem=", > > it will conflict with the config table. > > > > It will not conflict - initrdmem= supersedes the INITRD table because > early param passing happens after efi_init(). > > > > - update the INITRD config table in memory (i.e., update the base and > > > size to refer to the new initrd image) > > This way seems practicable, but we don't know how to handle it in > > kexec-tools. :( > > > > Good point. Let me think a bit about this. OK, then let's go back to this series itself. :) I have seen the latest code in your git repo, I don't think we need efi_disable_rt. Instead, setting/clearing EFI_BOOT according to a0 as before seems reasonable. If efi_novamap breaks something, I can accept "set efi_novamap to false" in the previous version, or just ignore its value in the efistub, but a0 should clearly be the indicator of EFI_BOOT. Huacai