From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 3EE57AC1548 for ; Tue, 10 Oct 2023 15:33:29 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=qXGEZeaDJPC1IVmooTSJ4MjL1EO6kDraInXITrHppgY=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1696952007; v=1; b=TnTcCzNf+S0OtBnf2T6Ldd89MdLgvWW1/V1NHIqYYR3MRIzW+rpbefHdTvLPcs36Jc8XWyYt fUgzWSpaOIWSshZzQxGgftF3sRnyIbkihgZHcTYYaX+euLO+RUo7IsU0vFBgeqtaqlYgSb7XfTV fC88Nvw20GAVLItoijo+32o0= X-Received: by 127.0.0.2 with SMTP id qAyFYY7687511xKuGH9NsK4X; Tue, 10 Oct 2023 08:33:27 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.94964.1696952007064081462 for ; Tue, 10 Oct 2023 08:33:27 -0700 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-682-YZeAbFTqNYWS-wQxxsv2ww-1; Tue, 10 Oct 2023 11:33:12 -0400 X-MC-Unique: YZeAbFTqNYWS-wQxxsv2ww-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4C3BA382254A; Tue, 10 Oct 2023 15:33:12 +0000 (UTC) X-Received: from [10.39.192.135] (unknown [10.39.192.135]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C0E461C060DF; Tue, 10 Oct 2023 15:33:10 +0000 (UTC) Message-ID: <35314dd9-3705-d322-4137-f4708d420e3e@redhat.com> Date: Tue, 10 Oct 2023 17:33:09 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs To: devel@edk2.groups.io, ardb@kernel.org Cc: Ard Biesheuvel , Gerd Hoffmann , Julien Grall , Leif Lindholm , Peter Maydell , Sami Mujawar References: <20231008153912.175941-1-lersek@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: 1jDznMh33SZcKl3fvshRJYl6x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=TnTcCzNf; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 10/10/23 09:43, Ard Biesheuvel wrote: > On Sun, 8 Oct 2023 at 17:39, Laszlo Ersek wrote: >> >> This ArmVirtPkg series can be fetched from: >> >> repo: https://pagure.io/lersek/edk2.git >> branch: armvirt-dual-serial @ 65ee08413595 >> >> The series does the following: >> >> - It centralizes (and cleans up) two FDT parsing actions, namely looking >> up all serial ports, and looking up the /chosen "stdout-path" serial >> port, in a new library class and instance. >> >> - It rebases Fdt16550SerialPortHookLib, EarlyFdtPL011SerialPortLib and >> PlatformPeiLib to the new library. >> >> - If QEMU specifies just one PL011 UART, then this patch set is >> unobservable from the outside. >> >> - If QEMU specifies (at least) two PL011 UARTs, then we distinguish a >> "chosen" one, and a (first) "non-chosen" one: >> >> - Both EarlyFdtPL011SerialPortLib, and (PlatformPeiLib + >> FdtPL011SerialPortLib), target the "chosen" PL011. The consequence >> of this is that (a) direct SerialPortLib traffic, (b) the dependent >> SerialIo (SerialDxe) protocol traffic, and (c) the dependent UEFI >> console traffic, all occcur on the same PL011, and do so regardless >> of the firmware phase. Furthermore, (d) the Linux serial console >> traffic is directed to the same PL011 as well. In total, the >> "chosen" PL011 UART becomes "the console", covering both firmware >> and Linux. >> >> - Three new DebugLib instances -- namely Flash, RAM, and DXE Runtime >> instances of "DebugLibFdtPL011Uart" -- target the (first) >> "non-chosen" PL011. The consequence is that DebugLib output is >> hermetically separated from the above-mentioned console, mirroring >> the isa-debugcon situation with x86 OVMF. >> >> Peter's QEMU patch set that this series interoperates with is at: >> >> repo: https://git.linaro.org/people/pmaydell/qemu-arm.git >> branch: uart-edk-investigation @ 66bff4241bf8 >> >> See the larger background, and my detailed test results -- using >> "ArmVirtQemu.dsc" -- in the following thread: >> >> EDK2 ArmVirtQemu behaviour with multiple UARTs >> http://mid.mail-archive.com/CAFEAcA_P5aOTQnM2ARYgR5WvKouvndMbX95XNmDsS= 0KTxMkMMw@mail.gmail.com >> https://listman.redhat.com/archives/edk2-devel-archive/2023-September/= 068241.html >> https://edk2.groups.io/g/devel/message/108941 >> >> For my testing, I rebased Peter's set on more recent QEMU commit >> 36e9aab3c569. Also, importantly, Peter's last patch 66bff4241bf8 ("virt: >> Reverse order of UART dtb nodes", 2023-09-21) is *indifferent* regarding >> my test results (which shows that the ordering of the two PL011 UARTs in >> the DTB does not matter, with this edk2 series applied). See more on >> that in the above-noted thread. >> >> "ArmVirtKvmTool.dsc" and "ArmVirtXen.dsc" are not supposed to be visibly >> affected by this series; I test-built them, and checked the library >> resolutions before/after in their build report files (no change). >> Runtime regression testing with these platforms would be welcome. >> >> I also test-built "ArmVirtCloudHv.dsc" and "ArmVirtQemuKernel.dsc". >> Those *are* supposed to receive the same feature, but I couldn't / >> didn't boot them, respectively. >> >> I've formatted the patches with "--find-copies-harder", because (a) that >> makes for an easier reading, and (b) leaves the patches applicable from >> the list. The base commit is noted at the end of this message. >> >> Cc: Ard Biesheuvel >> Cc: Gerd Hoffmann >> Cc: Julien Grall >> Cc: Leif Lindholm >> Cc: Peter Maydell >> Cc: Sami Mujawar >> >=20 > Hello Laszlo, >=20 > Thanks for looking into this - a cleanup was overdue here. >=20 > I will take a look in more detail later, but one thing that occurred > to me when reading this overview is that having a separate DEBUG > serial port would permit us to >=20 > a) remove it from the DT ... as in, hide it from Linux, I assume? > b) add a runtime mapping for it > c) keep using it after ExitBootServices >=20 > This could be useful for debugging issues with the variable store etc. >=20 > Not saying this is something to address in this series, but I'd like > to hear your take on this. >=20 Sounds like a useful feature. I see four challenges: (1) We'd have to coordinate it with Peter. If we hide any one of the serial ports from Linux, that may not be what QEMU intends for Linux to happen. Linux currently ties getties to all serial ports -- via the serial* aliases, IIUC. Thus, some "positive identification" in the DT could be necessary (i.e., that edk2 was welcome to hide that port from Linux). The current concept is to identify the chosen port for direct SerialPortLib + SerialDxe + UEFI console purposes, and the "first non-chosen port" for DEBUG. If we got some "positive identification" in the DT, then "first non-chosen port" for DEBUG would not be good enough. And the "positive identification" logic would affect all callers of FdtSerialGetPorts(), and the library interface itself may have to reworked (for remaining useful), probably. (2) We'd have to find a good "home" (like a new, "initializing", driver?) for adding the runtime MMIO mapping, and for modifying the DT in the DXE phase. Examples for the latter are: - "ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c"; which is linked via NULL class resolution into "EmbeddedPkg/RealTimeClockRuntimeDxe" - the logic we reverted -- from ArmVirtPkg/PciHostBridgeDxe, at the time -- in commit 29589acf1010 ("ArmVirtPkg/PciHostBridgeDxe: don't set linux,pci-probe-only DT property", 2016-09-02). I don't have an idea for what driver should host this DT-tweaking logic. Hooking it as a small library into (say) SerialDxe via NULL class resolution feels weird, because SerialDxe is supposed to deal with the *chosen* port (via SerialPortLib), and not with any one of the non-chosen ones. And, we don't have a "debug port driver". (3) For setting the "status" property of the affected PL011 node to "disabled" (for hiding it from Linux), we'd have to identify that node by NodeId, for FDT_CLIENT_PROTOCOL.SetNodeProperty(). After the present patch set, the DXE phase knows the address of the "debug PL011" from the (extended) "EarlyPL011BaseAddress" GUID HOB. But FDT_CLIENT_PROTOCOL.SetNodeProperty() cannot consume a base address. So, wherever we put the new FDT_CLIENT_PROTOCOL-based logic, it would likely have to contain a separate loop, with FindNextCompatibleNode(), for locating the debug PL011 once again (and once found, for disabling it). This is not optimal; the whole idea of the "EarlyPL011BaseAddress" GUID HOB is to cache the relevant PL011 characteristics once we have writeable RAM, and not to traverse the DT (not even via FDT_CLIENT_PROTOCOL) again for the sake of serial ports. However, device tree NodeId's are not stable references, as far as I know, and FDT_CLIENT_PROTOCOL does expose some functions for modifying the DT (which we do call). Thus, assuming we consider NodeId's stable only during a "read-only traversal", we couldn't "cache" any NodeId in advance (in PlatformPeiLib, where we populate the GUID HOB). We couldn't avoid a loop with FindNextCompatibleNode() -- we'd have to duplicate at least some of the logic that we use for filling in the GUID HOB, in PlatformPeiLib. (4) In the DXE runtime DebugLib instance, we'd have to convert the address of the debug PL011. Not necessarily complicated, just something to remember. All in all, I think the implementation would be quite a steep divergence from, or on top of, this patch set. :) --*-- BTW, I've had a different (independent?) extension in mind, on top of this series. At some point we could switch the policy to: - one PL011: console yes, DEBUG *discarded* - two PL011: console on chosen, DEBUG on the other The policy change is the "discarded" part; currently that part reads "intermixed with console". This policy change would be justified for the following reason: right now (some) downstreams build two ArmVirtQemu binaries, one verbose and one silent. The verbose one is good for debugging, but boots slowly, because PL011 (MMIO) traps are expensive. The silent one boots fast, but is not as good for debugging. With support for two PL011's present, we could ship just one ArmVirtQemu binary: a verbose one. If the domain were booted with one PL011, the DEBUG output could be discarded (as opposed to including it with the console IO), thereby making the boot fast. With two PL011s, the user would get pristine console IO, separate from pristine debug output, plus a slow boot. The point is that both of these boot modes / VM configs would be available with a single firmware binary. So this latter idea might not be difficult to implement on top of this series, I assume. I imagine the distinction between the ports would remain the same, we'd just set the debug port address to zero, rather than aliasing the console port address, in case there was just one port. Additionally, we'd discard any debug output destined for the zero address. This is of course a compat-breaking change, because people used to just one PL011 UART would suddenly lose their DEBUG output (as opposed to it being intermixed with console IO). People wanting DEBUG output going forward would have to modify their domain configs (add the second PL011), and handle separate debug log files on the host side. Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109498): https://edk2.groups.io/g/devel/message/109498 Mute This Topic: https://groups.io/mt/101834880/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-