Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs

["Laszlo Ersek" <lersek@redhat.com>]

On 10/10/23 09:43, Ard Biesheuvel wrote:
> On Sun, 8 Oct 2023 at 17:39, Laszlo Ersek <lersek@redhat.com> 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_P5aOTQnM2ARYgR5WvKouvndMbX95XNmDsS0KTxMkMMw@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 <ardb+tianocore@kernel.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Leif Lindholm <quic_llindhol@quicinc.com>
>> Cc: Peter Maydell <peter.maydell@linaro.org>
>> Cc: Sami Mujawar <sami.mujawar@arm.com>
>>
> 
> Hello Laszlo,
> 
> Thanks for looking into this - a cleanup was overdue here.
> 
> 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
> 
> 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
> 
> This could be useful for debugging issues with the variable store etc.
> 
> Not saying this is something to address in this series, but I'd like
> to hear your take on this.
> 

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



-=-=-=-=-=-=-=-=-=-=-=-
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/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-