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>

(I've not Cc'd Peter on the patches themselves, but I'm Cc'ing him on
the cover letter.)

Thanks,
Laszlo

Laszlo Ersek (9):
  ArmVirtPkg: introduce FdtSerialPortAddressLib
  ArmVirtPkg/Fdt16550SerialPortHookLib: rebase to
    FdtSerialPortAddressLib
  ArmVirtPkg: adjust whitespace in block scope declarations
  ArmVirtPkg: adhere to the serial port selected by /chosen
    "stdout-path"
  ArmVirtPkg: store separate console and debug PL011 addresses in GUID
    HOB
  ArmVirtPkg: introduce DebugLibFdtPL011Uart Flash instance
  ArmVirtPkg: introduce DebugLibFdtPL011Uart RAM instance
  ArmVirtPkg: introduce DebugLibFdtPL011Uart DXE Runtime instance
  ArmVirtPkg: steer DebugLib output away from SerialPortLib+console
    traffic

 ArmVirtPkg/ArmVirt.dsc.inc                                                                    |  14 +-
 ArmVirtPkg/ArmVirtKvmTool.dsc                                                                 |  11 +
 ArmVirtPkg/ArmVirtPkg.dec                                                                     |   1 +
 ArmVirtPkg/ArmVirtXen.dsc                                                                     |  11 +
 ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h                                               |  15 +-
 ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h                                          |  83 +++++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf                         |  54 +++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf                           |  60 +++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf                    |  61 +++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c                                               | 107 ++++++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c                                                 | 124 ++++++++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h                                                 |  18 ++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c                                       |  27 +++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c                                             |  88 +++++++
 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h                                               |  39 +++
 ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c                 |  88 +------
 ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf               |   3 +-
 ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c                         |  90 +++----
 ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf                       |   3 +-
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                              |  20 +-
 ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c                          | 256 ++++++++++++++++++++
 ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf                        |  27 +++
 ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c                                            | 108 ++++++---
 ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf                                          |   2 +
 {MdePkg/Library/BaseDebugLibSerialPort => ArmVirtPkg/Library/DebugLibFdtPL011Uart}/DebugLib.c |  41 ++--
 25 files changed, 1128 insertions(+), 223 deletions(-)
 copy {MdePkg/Library/BaseDebugLibSerialPort => ArmVirtPkg/Library/DebugLibFdtPL011Uart}/DebugLib.c (89%)
 create mode 100644 ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c
 create mode 100644 ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h
 create mode 100644 ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
 create mode 100644 ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf


base-commit: 4ddd8ac3a29d9c5974a19f36c1dc5896d813dc6e


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109394): https://edk2.groups.io/g/devel/message/109394
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
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.

-- 
Ard.


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109475): https://edk2.groups.io/g/devel/message/109475
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/10/23 09:43, Ard Biesheuvel wrote:
> > 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 potential awkwardness here is that what the guest thinks about
the serial ports depends on the ACPI table fragments which QEMU
provides. EDK2 would need to edit the table fragment to remove any
mention of the second UART if it wanted to hide it from the kernel.
I don't know how hard that would be in EDK2.

(As far as I'm aware usually a boot via EDK2 doesn't pass the
dtb on to Linux, though I guess there's no reason it can't.)

From QEMU's point of view, we provide two UARTs to the guest, and we
don't really care whether that means one is used by EDK2 and one by
Linux, or both are used as getty terminals by Linux, or whether the
Linux guest uses one serial as a terminal and leaves the other to its
userspace programs  -- it's all just guest software to us :-)

[snip other technical stuff]

> All in all, I think the implementation would be quite a steep divergence
> from, or on top of, this patch set. :)

I agree with this and with Ard's "not something to address in this
series" comment above; it doesn't sound like this is something that
needs to hold up the patchset we have currently.

Does anybody have time to review Laszlo's code? It would be nice
to be able to get this into the next EDK2 release.

thanks
-- PMM


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110112): https://edk2.groups.io/g/devel/message/110112
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 10/26/23 16:21, Peter Maydell wrote:
> On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/10/23 09:43, Ard Biesheuvel wrote:
>>> 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 potential awkwardness here is that what the guest thinks about
> the serial ports depends on the ACPI table fragments which QEMU
> provides. EDK2 would need to edit the table fragment to remove any
> mention of the second UART if it wanted to hide it from the kernel.
> I don't know how hard that would be in EDK2.
> 
> (As far as I'm aware usually a boot via EDK2 doesn't pass the
> dtb on to Linux, though I guess there's no reason it can't.)
> 
> From QEMU's point of view, we provide two UARTs to the guest, and we
> don't really care whether that means one is used by EDK2 and one by
> Linux, or both are used as getty terminals by Linux, or whether the
> Linux guest uses one serial as a terminal and leaves the other to its
> userspace programs  -- it's all just guest software to us :-)
> 
> [snip other technical stuff]

Thanks, good point -- I wasn't aware of the ACPI impact.

We don't edit / patch QEMU's ACPI tables, ever. (Beyond obeying the ACPI
linker/loader script.) That's a principle we've upheld many times.
Whenever ACPI content needs to change, that implies a QEMU patch.

So, for this purpose, only the following could have a chance of working:

- Expose a new config option on the QEMU command line to the user,
regarding the intended use of the serial port(s). This could be of any
tolerable form (machine property, front-end (device) property, whatever
-- anything that QEMU reviewers can accept).

- In QEMU, generate both the DT and the ACPI tables accordingly. The
ACPI tables would have to immediately *not* contain the UART-to-hide (so
as to keep it secret from the guest OS). The DT at the same time would
still have to expose the "runtime DEBUG UART", because edk2 would have
to know where that UART was (and that it was meant specifically for OS
runtime debug output).

- Edk2 would have to patch the DT (we tend to do that already), because
(in some configs) we do forward the DT to the guest OS. This need for
patching could be lifted if QEMU adopted such a form of expression for
the "runtime DEBUG UART" that would be ignored by Linux out of the box.

> 
>> All in all, I think the implementation would be quite a steep divergence
>> from, or on top of, this patch set. :)
> 
> I agree with this and with Ard's "not something to address in this
> series" comment above; it doesn't sound like this is something that
> needs to hold up the patchset we have currently.

Right; I'd like to flush this one. The runtime debug UART seems to need
more joint pondering.

> 
> Does anybody have time to review Laszlo's code? It would be nice
> to be able to get this into the next EDK2 release.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110114): https://edk2.groups.io/g/devel/message/110114
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

  Hi,

> So, for this purpose, only the following could have a chance of working:
> 
> - Expose a new config option on the QEMU command line to the user,
> regarding the intended use of the serial port(s). This could be of any
> tolerable form (machine property, front-end (device) property, whatever
> -- anything that QEMU reviewers can accept).
> 
> - In QEMU, generate both the DT and the ACPI tables accordingly. The
> ACPI tables would have to immediately *not* contain the UART-to-hide (so
> as to keep it secret from the guest OS). The DT at the same time would
> still have to expose the "runtime DEBUG UART", because edk2 would have
> to know where that UART was (and that it was meant specifically for OS
> runtime debug output).
> 
> - Edk2 would have to patch the DT (we tend to do that already), because
> (in some configs) we do forward the DT to the guest OS. This need for
> patching could be lifted if QEMU adopted such a form of expression for
> the "runtime DEBUG UART" that would be ignored by Linux out of the box.

That approach looks best to me.  It allows the user to specify on the
qemu command line what the ports should be used for, which is IMHO the
most convenient option.  qemu generates the complete DSDT anyway, so
it's trivial to add/remove serial ports there, and for the DT we have
libfdt to do the work needed.

take care,
  Gerd



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110201): https://edk2.groups.io/g/devel/message/110201
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 26 Oct 2023 at 17:19, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 10/26/23 16:21, Peter Maydell wrote:
> > On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 10/10/23 09:43, Ard Biesheuvel wrote:
> >>> 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 potential awkwardness here is that what the guest thinks about
> > the serial ports depends on the ACPI table fragments which QEMU
> > provides. EDK2 would need to edit the table fragment to remove any
> > mention of the second UART if it wanted to hide it from the kernel.
> > I don't know how hard that would be in EDK2.
> >
> > (As far as I'm aware usually a boot via EDK2 doesn't pass the
> > dtb on to Linux, though I guess there's no reason it can't.)
> >
> > From QEMU's point of view, we provide two UARTs to the guest, and we
> > don't really care whether that means one is used by EDK2 and one by
> > Linux, or both are used as getty terminals by Linux, or whether the
> > Linux guest uses one serial as a terminal and leaves the other to its
> > userspace programs  -- it's all just guest software to us :-)
> >
> > [snip other technical stuff]
>
> Thanks, good point -- I wasn't aware of the ACPI impact.
>
> We don't edit / patch QEMU's ACPI tables, ever. (Beyond obeying the ACPI
> linker/loader script.) That's a principle we've upheld many times.
> Whenever ACPI content needs to change, that implies a QEMU patch.
>
> So, for this purpose, only the following could have a chance of working:
>
> - Expose a new config option on the QEMU command line to the user,
> regarding the intended use of the serial port(s). This could be of any
> tolerable form (machine property, front-end (device) property, whatever
> -- anything that QEMU reviewers can accept).
>
> - In QEMU, generate both the DT and the ACPI tables accordingly. The
> ACPI tables would have to immediately *not* contain the UART-to-hide (so
> as to keep it secret from the guest OS). The DT at the same time would
> still have to expose the "runtime DEBUG UART", because edk2 would have
> to know where that UART was (and that it was meant specifically for OS
> runtime debug output).
>
> - Edk2 would have to patch the DT (we tend to do that already), because
> (in some configs) we do forward the DT to the guest OS. This need for
> patching could be lifted if QEMU adopted such a form of expression for
> the "runtime DEBUG UART" that would be ignored by Linux out of the box.
>
> >
> >> All in all, I think the implementation would be quite a steep divergence
> >> from, or on top of, this patch set. :)
> >
> > I agree with this and with Ard's "not something to address in this
> > series" comment above; it doesn't sound like this is something that
> > needs to hold up the patchset we have currently.
>
> Right; I'd like to flush this one. The runtime debug UART seems to need
> more joint pondering.
>
> >
> > Does anybody have time to review Laszlo's code? It would be nice
> > to be able to get this into the next EDK2 release.
>

I'm happy for this to go in if it covers our needs.

Acked-by: Ard Biesheuvel <ardb@kernel.org>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110115): https://edk2.groups.io/g/devel/message/110115
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 10/26/23 17:21, Ard Biesheuvel wrote:
> On Thu, 26 Oct 2023 at 17:19, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 10/26/23 16:21, Peter Maydell wrote:
>>> On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 10/10/23 09:43, Ard Biesheuvel wrote:
>>>>> 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 potential awkwardness here is that what the guest thinks about
>>> the serial ports depends on the ACPI table fragments which QEMU
>>> provides. EDK2 would need to edit the table fragment to remove any
>>> mention of the second UART if it wanted to hide it from the kernel.
>>> I don't know how hard that would be in EDK2.
>>>
>>> (As far as I'm aware usually a boot via EDK2 doesn't pass the
>>> dtb on to Linux, though I guess there's no reason it can't.)
>>>
>>> From QEMU's point of view, we provide two UARTs to the guest, and we
>>> don't really care whether that means one is used by EDK2 and one by
>>> Linux, or both are used as getty terminals by Linux, or whether the
>>> Linux guest uses one serial as a terminal and leaves the other to its
>>> userspace programs  -- it's all just guest software to us :-)
>>>
>>> [snip other technical stuff]
>>
>> Thanks, good point -- I wasn't aware of the ACPI impact.
>>
>> We don't edit / patch QEMU's ACPI tables, ever. (Beyond obeying the ACPI
>> linker/loader script.) That's a principle we've upheld many times.
>> Whenever ACPI content needs to change, that implies a QEMU patch.
>>
>> So, for this purpose, only the following could have a chance of working:
>>
>> - Expose a new config option on the QEMU command line to the user,
>> regarding the intended use of the serial port(s). This could be of any
>> tolerable form (machine property, front-end (device) property, whatever
>> -- anything that QEMU reviewers can accept).
>>
>> - In QEMU, generate both the DT and the ACPI tables accordingly. The
>> ACPI tables would have to immediately *not* contain the UART-to-hide (so
>> as to keep it secret from the guest OS). The DT at the same time would
>> still have to expose the "runtime DEBUG UART", because edk2 would have
>> to know where that UART was (and that it was meant specifically for OS
>> runtime debug output).
>>
>> - Edk2 would have to patch the DT (we tend to do that already), because
>> (in some configs) we do forward the DT to the guest OS. This need for
>> patching could be lifted if QEMU adopted such a form of expression for
>> the "runtime DEBUG UART" that would be ignored by Linux out of the box.
>>
>>>
>>>> All in all, I think the implementation would be quite a steep divergence
>>>> from, or on top of, this patch set. :)
>>>
>>> I agree with this and with Ard's "not something to address in this
>>> series" comment above; it doesn't sound like this is something that
>>> needs to hold up the patchset we have currently.
>>
>> Right; I'd like to flush this one. The runtime debug UART seems to need
>> more joint pondering.
>>
>>>
>>> Does anybody have time to review Laszlo's code? It would be nice
>>> to be able to get this into the next EDK2 release.
>>
> 
> I'm happy for this to go in if it covers our needs.
> 
> Acked-by: Ard Biesheuvel <ardb@kernel.org>

Thank you, Ard! Merged as commit range 74c687cc2f2f..f945b72331d7 via
<https://github.com/tianocore/edk2/pull/4967>.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110141): https://edk2.groups.io/g/devel/message/110141
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

Hi,

On 26/10/2023 15:21, Peter Maydell wrote:
> On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 10/10/23 09:43, Ard Biesheuvel wrote:
>>> 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 potential awkwardness here is that what the guest thinks about
> the serial ports depends on the ACPI table fragments which QEMU
> provides. EDK2 would need to edit the table fragment to remove any
> mention of the second UART if it wanted to hide it from the kernel.
> I don't know how hard that would be in EDK2.

I am not sure if it would help EDK2 in this case. But we had a similar 
problem when adding support for ACPI in Xen. It was not trivial to 
remove the UART from the ACPI tables provided by the host. So we ended 
up to introduce the STAO table [1]. This is used to describe which 
device will be hidden to the OS.

Cheers,

[1] https://wiki.xenproject.org/images/0/02/Status-override-table.pdf

-- 
Julien Grall


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110107): https://edk2.groups.io/g/devel/message/110107
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 10/26/23 16:46, Julien Grall wrote:
> Hi,
> 
> On 26/10/2023 15:21, Peter Maydell wrote:
>> On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
>>> On 10/10/23 09:43, Ard Biesheuvel wrote:
>>>> 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 potential awkwardness here is that what the guest thinks about
>> the serial ports depends on the ACPI table fragments which QEMU
>> provides. EDK2 would need to edit the table fragment to remove any
>> mention of the second UART if it wanted to hide it from the kernel.
>> I don't know how hard that would be in EDK2.
> 
> I am not sure if it would help EDK2 in this case. But we had a similar
> problem when adding support for ACPI in Xen. It was not trivial to
> remove the UART from the ACPI tables provided by the host. So we ended
> up to introduce the STAO table [1]. This is used to describe which
> device will be hidden to the OS.
> 
> Cheers,
> 
> [1] https://wiki.xenproject.org/images/0/02/Status-override-table.pdf
> 

This is a very interesting document. It states the problem well, but the
solution Xen seems to have chosen is the opposite of QEMU's. The
document states that AML generation is difficult, so simple override
tables such as STAO are preferred. QEMU on the other hand has grown a
full-blown runtime AML generator, plus an "ACPI linker/loader script"
language that allows guest firmware to cross-link and install the "tree
of tables", also updating their checksums (under SeaBIOS), all the while
being completely blind to the actual contents of the tables.

This means that under QEMU, the sole source of "ACPI truth" is QEMU.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110119): https://edk2.groups.io/g/devel/message/110119
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On Thu, 26 Oct 2023 at 16:46, Julien Grall <julien@xen.org> wrote:
>
> Hi,
>
> On 26/10/2023 15:21, Peter Maydell wrote:
> > On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
> >> On 10/10/23 09:43, Ard Biesheuvel wrote:
> >>> 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 potential awkwardness here is that what the guest thinks about
> > the serial ports depends on the ACPI table fragments which QEMU
> > provides. EDK2 would need to edit the table fragment to remove any
> > mention of the second UART if it wanted to hide it from the kernel.
> > I don't know how hard that would be in EDK2.
>
> I am not sure if it would help EDK2 in this case. But we had a similar
> problem when adding support for ACPI in Xen. It was not trivial to
> remove the UART from the ACPI tables provided by the host. So we ended
> up to introduce the STAO table [1]. This is used to describe which
> device will be hidden to the OS.
>

I'd much rather have an implementation of the _STA method on those
devices where the underlying AML queries the fwcfg MMIO device


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110108): https://edk2.groups.io/g/devel/message/110108
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

On 10/26/23 16:55, Ard Biesheuvel wrote:
> On Thu, 26 Oct 2023 at 16:46, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 26/10/2023 15:21, Peter Maydell wrote:
>>> On Tue, 10 Oct 2023 at 16:33, Laszlo Ersek <lersek@redhat.com> wrote:
>>>> On 10/10/23 09:43, Ard Biesheuvel wrote:
>>>>> 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 potential awkwardness here is that what the guest thinks about
>>> the serial ports depends on the ACPI table fragments which QEMU
>>> provides. EDK2 would need to edit the table fragment to remove any
>>> mention of the second UART if it wanted to hide it from the kernel.
>>> I don't know how hard that would be in EDK2.
>>
>> I am not sure if it would help EDK2 in this case. But we had a similar
>> problem when adding support for ACPI in Xen. It was not trivial to
>> remove the UART from the ACPI tables provided by the host. So we ended
>> up to introduce the STAO table [1]. This is used to describe which
>> device will be hidden to the OS.
>>
> 
> I'd much rather have an implementation of the _STA method on those
> devices where the underlying AML queries the fwcfg MMIO device

Yes, this is possible too; the AML generated by QEMU need not be
constant, it can interact at OS runtime with QEMU. An example for this
is the VCPU hotplug register block. The hot(un)plug AML is super
sophisticated, it connects the guest kernel, QEMU (via the register
block) and the firmware (via raising SMIs).

One word of caution against accessing fw_cfg specifically, from AML:
fw_cfg is already exposed via guest sysfs (IIRC -- see
"drivers/firmware/qemu_fw_cfg.c"), because fw_cfg ended up being
"abused" (IMO) as a generic info channel from the host-side user to
guest OS + applications. (I call this "abuse" because "fw_cfg" literally
says "firmware configuration" in the name, and this particular use case
is anything *but* firmware configuration.) Accessing the same device
from AML may not be without risks.

But "non-fw_cfg platform registers" might work well with AML.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110123): https://edk2.groups.io/g/devel/message/110123
Mute This Topic: https://groups.io/mt/101834880/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/3901457/1787277/102458076/xyzzy [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-