* [edk2-devel] [PATCH 1/9] ArmVirtPkg: introduce FdtSerialPortAddressLib
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 2/9] ArmVirtPkg/Fdt16550SerialPortHookLib: rebase to FdtSerialPortAddressLib Laszlo Ersek
` (8 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
Introduce a new library class + instance for:
- collecting serial port base addresses from the device tree,
- collecting the /chosen stdout-path serial port base address from the
device tree.
The logic is loosely based on the following functions:
- SerialPortGetBaseAddress()
[ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c]
- PlatformPeim() [ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c]
- GetSerialConsolePortAddress()
[ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c]
which are going to be converted to clients of the new library later.
Copyright notices from those other files are preserved.
The new library fixes the following warts, found by reading the existent
code:
- Neither of the three functions check whether the "reg" property exists.
(This may be implicitly checked when they compare the property size to
16.)
- GetSerialConsolePortAddress() uses ScanMem8() for locating a colon (":")
node path separator in "stdout-path", when AsciiStrStr() could work just
as fine. While ScanMem8() is likely faster, "stdout-path" is presumably
very short, and ScanMem8() introduces an extra lib class dependency
(namely BaseMemoryLib).
- If ScanMem8() fails to locate a colon in "stdout-path", then
GetSerialConsolePortAddress() re-measures the length of the whole
"stdout-path" property. This is conceptually (if not performance-wise)
disturbing, because we know the whole size of the "stdout-path" property
from the property lookup just before, so we only need to subtract the
NUL-terminator for learning the length.
- GetSerialConsolePortAddress() does not check if the first (or only) node
path inside the "stdout-path" property is empty. (Not a big deal, the
subsequent alias resolution should simply fail.)
- GetSerialConsolePortAddress() does not verify if the node path retrieved
(and potentially alias-resolved) from "stdout-path" can be located in
the device tree; it assumes it.
- Code is duplicated (of course) between SerialPortGetBaseAddress() and
PlatformPeim(), but more surprisingly, all three functions embed the
same code for verifying the "status" property of the serial port node,
and for checking and reading its "reg" property.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/ArmVirtPkg.dec | 1 +
ArmVirtPkg/ArmVirt.dsc.inc | 1 +
ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h | 83 +++++++
ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf | 27 +++
ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c | 256 ++++++++++++++++++++
5 files changed, 368 insertions(+)
diff --git a/ArmVirtPkg/ArmVirtPkg.dec b/ArmVirtPkg/ArmVirtPkg.dec
index 4645c91a8375..0f2d7873279f 100644
--- a/ArmVirtPkg/ArmVirtPkg.dec
+++ b/ArmVirtPkg/ArmVirtPkg.dec
@@ -27,6 +27,7 @@ [Includes.common]
[LibraryClasses]
ArmVirtMemInfoLib|Include/Library/ArmVirtMemInfoLib.h
+ FdtSerialPortAddressLib|Include/Library/FdtSerialPortAddressLib.h
[Guids.common]
gArmVirtTokenSpaceGuid = { 0x0B6F5CA7, 0x4F53, 0x445A, { 0xB7, 0x6E, 0x2E, 0x36, 0x5B, 0x80, 0x63, 0x66 } }
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 3f7bac6bf33a..0d7115525497 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -121,6 +121,7 @@ [LibraryClasses.common]
# ARM PL011 UART Driver
PL011UartLib|ArmPlatformPkg/Library/PL011UartLib/PL011UartLib.inf
SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
+ FdtSerialPortAddressLib|ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
PeCoffExtraActionLib|ArmPkg/Library/DebugPeCoffExtraActionLib/DebugPeCoffExtraActionLib.inf
#PeCoffExtraActionLib|MdePkg/Library/BasePeCoffExtraActionLibNull/BasePeCoffExtraActionLibNull.inf
diff --git a/ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h b/ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h
new file mode 100644
index 000000000000..3d4c911f58aa
--- /dev/null
+++ b/ArmVirtPkg/Include/Library/FdtSerialPortAddressLib.h
@@ -0,0 +1,83 @@
+/** @file
+ Determine the base addresses of serial ports from the Device Tree.
+
+ Copyright (C) Red Hat
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef FDT_SERIAL_PORT_ADDRESS_LIB_H_
+#define FDT_SERIAL_PORT_ADDRESS_LIB_H_
+
+#include <Base.h>
+
+typedef struct {
+ UINTN NumberOfPorts;
+ UINT64 BaseAddress[2];
+} FDT_SERIAL_PORTS;
+
+/**
+ Collect the first ARRAY_SIZE (Ports->BaseAddress) serial ports into Ports from
+ DeviceTree.
+
+ @param[in] DeviceTree The flat device tree (FDT) to scan.
+
+ @param[in] Compatible Look for Compatible in the "compatible" property of the
+ scanned nodes.
+
+ @param[out] Ports On successful return, Ports->NumberOfPorts contains the
+ number of serial ports found; it is (a) positive and
+ (b) at most ARRAY_SIZE (Ports->BaseAddress). If the FDT
+ had more serial ports, those are not reported. On
+ error, the contents of Ports are indeterminate.
+
+ @retval RETURN_INVALID_PARAMETER DeviceTree does not point to a valid FDT
+ header.
+
+ @retval RETURN_NOT_FOUND No compatible and enabled serial port has
+ been found.
+
+ @retval RETURN_SUCCESS At least one compatible and enabled serial
+ port has been found; Ports has been filled
+ in.
+**/
+RETURN_STATUS
+EFIAPI
+FdtSerialGetPorts (
+ IN CONST VOID *DeviceTree,
+ IN CONST CHAR8 *Compatible,
+ OUT FDT_SERIAL_PORTS *Ports
+ );
+
+/**
+ Fetch the base address of the serial port identified in the "stdout-path"
+ property of the "/chosen" node in DeviceTree.
+
+ @param[in] DeviceTree The flat device tree (FDT) to scan.
+
+ @param[out] BaseAddress On success, the base address of the preferred serial
+ port (to be used as console). On error, BaseAddress
+ is not modified.
+
+ @retval RETURN_INVALID_PARAMETER DeviceTree does not point to a valid FDT
+ header.
+
+ @retval RETURN_NOT_FOUND No enabled console port has been found.
+
+ @retval RETURN_PROTOCOL_ERROR The first (or only) node path in the
+ "stdout-path" property is an empty string.
+
+ @retval RETURN_PROTOCOL_ERROR The console port has been found in the FDT,
+ but its base address is not correctly
+ represented.
+
+ @retval RETURN_SUCCESS BaseAddress has been populated.
+**/
+RETURN_STATUS
+EFIAPI
+FdtSerialGetConsolePort (
+ IN CONST VOID *DeviceTree,
+ OUT UINT64 *BaseAddress
+ );
+
+#endif
diff --git a/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf b/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
new file mode 100644
index 000000000000..ae6d0d374b80
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.inf
@@ -0,0 +1,27 @@
+## @file
+# Determine the base addresses of serial ports from the Device Tree.
+#
+# Copyright (C) Red Hat
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 1.27
+ BASE_NAME = FdtSerialPortAddressLib
+ FILE_GUID = AEBE813B-25EA-40E5-95C4-2B864FE1E951
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = FdtSerialPortAddressLib
+
+[Sources]
+ FdtSerialPortAddressLib.c
+
+[Packages]
+ ArmVirtPkg/ArmVirtPkg.dec
+ EmbeddedPkg/EmbeddedPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ BaseLib
+ FdtLib
diff --git a/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c b/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
new file mode 100644
index 000000000000..f6508e09898a
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtSerialPortAddressLib/FdtSerialPortAddressLib.c
@@ -0,0 +1,256 @@
+/** @file
+ Determine the base addresses of serial ports from the Device Tree.
+
+ Copyright (C) Red Hat
+ Copyright (c) 2011 - 2023, Arm Ltd. All rights reserved.<BR>
+ Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+ Copyright (c) 2014 - 2020, Linaro Ltd. All rights reserved.<BR>
+ Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/BaseLib.h>
+#include <Library/FdtSerialPortAddressLib.h>
+#include <libfdt.h>
+
+/**
+ Read the "reg" property of Node in DeviceTree as a UINT64 base address.
+
+ @param[in] DeviceTree The flat device tree (FDT) to scan.
+
+ @param[in] Node The node to read the "reg" property of.
+
+ @param[out] BaseAddress On success, the base address read out of Node's "reg"
+ property. On error, not modified.
+
+ @retval RETURN_DEVICE_ERROR Node has a "status" property with value
+ different from "okay".
+
+ @retval RETURN_NOT_FOUND Node does not have a "reg" property.
+
+ @retval RETURN_BAD_BUFFER_SIZE The size of Node's "reg" property is not 16
+ bytes.
+
+ @retval RETURN_SUCCESS BaseAddress has been populated.
+**/
+STATIC
+RETURN_STATUS
+GetBaseAddress (
+ IN CONST VOID *DeviceTree,
+ IN INT32 Node,
+ OUT UINT64 *BaseAddress
+ )
+{
+ CONST CHAR8 *NodeStatus;
+ CONST VOID *RegProp;
+ INT32 PropSize;
+
+ NodeStatus = fdt_getprop (DeviceTree, Node, "status", NULL);
+ if ((NodeStatus != NULL) && (AsciiStrCmp (NodeStatus, "okay") != 0)) {
+ return RETURN_DEVICE_ERROR;
+ }
+
+ RegProp = fdt_getprop (DeviceTree, Node, "reg", &PropSize);
+ if (RegProp == NULL) {
+ return RETURN_NOT_FOUND;
+ }
+
+ if (PropSize != 16) {
+ return RETURN_BAD_BUFFER_SIZE;
+ }
+
+ *BaseAddress = fdt64_to_cpu (ReadUnaligned64 (RegProp));
+ return RETURN_SUCCESS;
+}
+
+/**
+ Collect the first ARRAY_SIZE (Ports->BaseAddress) serial ports into Ports from
+ DeviceTree.
+
+ @param[in] DeviceTree The flat device tree (FDT) to scan.
+
+ @param[in] Compatible Look for Compatible in the "compatible" property of the
+ scanned nodes.
+
+ @param[out] Ports On successful return, Ports->NumberOfPorts contains the
+ number of serial ports found; it is (a) positive and
+ (b) at most ARRAY_SIZE (Ports->BaseAddress). If the FDT
+ had more serial ports, those are not reported. On
+ error, the contents of Ports are indeterminate.
+
+ @retval RETURN_INVALID_PARAMETER DeviceTree does not point to a valid FDT
+ header.
+
+ @retval RETURN_NOT_FOUND No compatible and enabled serial port has
+ been found.
+
+ @retval RETURN_SUCCESS At least one compatible and enabled serial
+ port has been found; Ports has been filled
+ in.
+**/
+RETURN_STATUS
+EFIAPI
+FdtSerialGetPorts (
+ IN CONST VOID *DeviceTree,
+ IN CONST CHAR8 *Compatible,
+ OUT FDT_SERIAL_PORTS *Ports
+ )
+{
+ INT32 Node;
+
+ if (fdt_check_header (DeviceTree) != 0) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ Ports->NumberOfPorts = 0;
+ Node = fdt_next_node (DeviceTree, 0, NULL);
+ while ((Node > 0) &&
+ (Ports->NumberOfPorts < ARRAY_SIZE (Ports->BaseAddress)))
+ {
+ CONST CHAR8 *CompatProp;
+ INT32 PropSize;
+
+ CompatProp = fdt_getprop (DeviceTree, Node, "compatible", &PropSize);
+ if (CompatProp != NULL) {
+ CONST CHAR8 *CompatItem;
+
+ CompatItem = CompatProp;
+ while ((CompatItem < CompatProp + PropSize) &&
+ (AsciiStrCmp (CompatItem, Compatible) != 0))
+ {
+ CompatItem += AsciiStrLen (CompatItem) + 1;
+ }
+
+ if (CompatItem < CompatProp + PropSize) {
+ RETURN_STATUS Status;
+ UINT64 BaseAddress;
+
+ Status = GetBaseAddress (DeviceTree, Node, &BaseAddress);
+ if (!RETURN_ERROR (Status)) {
+ Ports->BaseAddress[Ports->NumberOfPorts++] = BaseAddress;
+ }
+ }
+ }
+
+ Node = fdt_next_node (DeviceTree, Node, NULL);
+ }
+
+ return Ports->NumberOfPorts > 0 ? RETURN_SUCCESS : RETURN_NOT_FOUND;
+}
+
+/**
+ Fetch the base address of the serial port identified in the "stdout-path"
+ property of the "/chosen" node in DeviceTree.
+
+ @param[in] DeviceTree The flat device tree (FDT) to scan.
+
+ @param[out] BaseAddress On success, the base address of the preferred serial
+ port (to be used as console). On error, BaseAddress
+ is not modified.
+
+ @retval RETURN_INVALID_PARAMETER DeviceTree does not point to a valid FDT
+ header.
+
+ @retval RETURN_NOT_FOUND No enabled console port has been found.
+
+ @retval RETURN_PROTOCOL_ERROR The first (or only) node path in the
+ "stdout-path" property is an empty string.
+
+ @retval RETURN_PROTOCOL_ERROR The console port has been found in the FDT,
+ but its base address is not correctly
+ represented.
+
+ @retval RETURN_SUCCESS BaseAddress has been populated.
+**/
+RETURN_STATUS
+EFIAPI
+FdtSerialGetConsolePort (
+ IN CONST VOID *DeviceTree,
+ OUT UINT64 *BaseAddress
+ )
+{
+ INT32 ChosenNode;
+ CONST CHAR8 *StdoutPathProp;
+ INT32 PropSize;
+ CONST CHAR8 *StdoutPathEnd;
+ UINTN StdoutPathLength;
+ INT32 ConsoleNode;
+ RETURN_STATUS Status;
+
+ if (fdt_check_header (DeviceTree) != 0) {
+ return RETURN_INVALID_PARAMETER;
+ }
+
+ ChosenNode = fdt_path_offset (DeviceTree, "/chosen");
+ if (ChosenNode < 0) {
+ return RETURN_NOT_FOUND;
+ }
+
+ StdoutPathProp = fdt_getprop (
+ DeviceTree,
+ ChosenNode,
+ "stdout-path",
+ &PropSize
+ );
+ if (StdoutPathProp == NULL) {
+ return RETURN_NOT_FOUND;
+ }
+
+ //
+ // If StdoutPathProp contains a colon (":"), then the colon terminates the
+ // path we're interested in.
+ //
+ StdoutPathEnd = AsciiStrStr (StdoutPathProp, ":");
+ if (StdoutPathEnd == NULL) {
+ StdoutPathLength = PropSize - 1;
+ } else {
+ StdoutPathLength = StdoutPathEnd - StdoutPathProp;
+ }
+
+ if (StdoutPathLength == 0) {
+ return RETURN_PROTOCOL_ERROR;
+ }
+
+ if (StdoutPathProp[0] == '/') {
+ //
+ // StdoutPathProp starts with an absolute node path.
+ //
+ ConsoleNode = fdt_path_offset_namelen (
+ DeviceTree,
+ StdoutPathProp,
+ (INT32)StdoutPathLength
+ );
+ } else {
+ //
+ // StdoutPathProp starts with an alias.
+ //
+ CONST CHAR8 *ResolvedStdoutPath;
+
+ ResolvedStdoutPath = fdt_get_alias_namelen (
+ DeviceTree,
+ StdoutPathProp,
+ (INT32)StdoutPathLength
+ );
+ if (ResolvedStdoutPath == NULL) {
+ return RETURN_NOT_FOUND;
+ }
+
+ ConsoleNode = fdt_path_offset (DeviceTree, ResolvedStdoutPath);
+ }
+
+ if (ConsoleNode < 0) {
+ return RETURN_NOT_FOUND;
+ }
+
+ Status = GetBaseAddress (DeviceTree, ConsoleNode, BaseAddress);
+ switch (Status) {
+ case RETURN_NOT_FOUND:
+ case RETURN_BAD_BUFFER_SIZE:
+ return RETURN_PROTOCOL_ERROR;
+ case RETURN_SUCCESS:
+ return RETURN_SUCCESS;
+ default:
+ return RETURN_NOT_FOUND;
+ }
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109391): https://edk2.groups.io/g/devel/message/109391
Mute This Topic: https://groups.io/mt/101834875/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 2/9] ArmVirtPkg/Fdt16550SerialPortHookLib: rebase to FdtSerialPortAddressLib
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 1/9] ArmVirtPkg: introduce FdtSerialPortAddressLib Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 3/9] ArmVirtPkg: adjust whitespace in block scope declarations Laszlo Ersek
` (7 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
This is only a refactoring; the patch is not supposed to cause any
observable change.
Build-tested only (with "ArmVirtKvmTool.dsc").
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf | 3 +-
ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c | 88 +-------------------
2 files changed, 4 insertions(+), 87 deletions(-)
diff --git a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf
index 007a45eca2a6..22aba53d9b48 100644
--- a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf
+++ b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf
@@ -22,12 +22,11 @@ [Sources]
[LibraryClasses]
BaseLib
PcdLib
- FdtLib
+ FdtSerialPortAddressLib
HobLib
[Packages]
ArmVirtPkg/ArmVirtPkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
MdeModulePkg/MdeModulePkg.dec
MdePkg/MdePkg.dec
diff --git a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
index c1b81920214b..03d28b9282ea 100644
--- a/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
+++ b/ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.c
@@ -17,90 +17,7 @@
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
#include <Library/PlatformHookLib.h>
-#include <libfdt.h>
-
-/** Get the UART base address of the console serial-port from the DT.
-
- This function fetches the node referenced in the "stdout-path"
- property of the "chosen" node and returns the base address of
- the console UART.
-
- @param [in] Fdt Pointer to a Flattened Device Tree (Fdt).
- @param [out] SerialConsoleAddress If success, contains the base address
- of the console serial-port.
-
- @retval EFI_SUCCESS The function completed successfully.
- @retval EFI_NOT_FOUND Console serial-port info not found in DT.
- @retval EFI_INVALID_PARAMETER Invalid parameter.
-**/
-STATIC
-EFI_STATUS
-EFIAPI
-GetSerialConsolePortAddress (
- IN CONST VOID *Fdt,
- OUT UINT64 *SerialConsoleAddress
- )
-{
- CONST CHAR8 *Prop;
- INT32 PropSize;
- CONST CHAR8 *Path;
- INT32 PathLen;
- INT32 ChosenNode;
- INT32 SerialConsoleNode;
- INT32 Len;
- CONST CHAR8 *NodeStatus;
- CONST UINT64 *RegProperty;
-
- if ((Fdt == NULL) || (fdt_check_header (Fdt) != 0)) {
- return EFI_INVALID_PARAMETER;
- }
-
- // The "chosen" node resides at the root of the DT. Fetch it.
- ChosenNode = fdt_path_offset (Fdt, "/chosen");
- if (ChosenNode < 0) {
- return EFI_NOT_FOUND;
- }
-
- Prop = fdt_getprop (Fdt, ChosenNode, "stdout-path", &PropSize);
- if (PropSize < 0) {
- return EFI_NOT_FOUND;
- }
-
- // Determine the actual path length, as a colon terminates the path.
- Path = ScanMem8 (Prop, PropSize, ':');
- if (Path == NULL) {
- PathLen = AsciiStrLen (Prop);
- } else {
- PathLen = Path - Prop;
- }
-
- // Aliases cannot start with a '/', so it must be the actual path.
- if (Prop[0] == '/') {
- SerialConsoleNode = fdt_path_offset_namelen (Fdt, Prop, PathLen);
- } else {
- // Lookup the alias, as this contains the actual path.
- Path = fdt_get_alias_namelen (Fdt, Prop, PathLen);
- if (Path == NULL) {
- return EFI_NOT_FOUND;
- }
-
- SerialConsoleNode = fdt_path_offset (Fdt, Path);
- }
-
- NodeStatus = fdt_getprop (Fdt, SerialConsoleNode, "status", &Len);
- if ((NodeStatus != NULL) && (AsciiStrCmp (NodeStatus, "okay") != 0)) {
- return EFI_NOT_FOUND;
- }
-
- RegProperty = fdt_getprop (Fdt, SerialConsoleNode, "reg", &Len);
- if (Len != 16) {
- return EFI_INVALID_PARAMETER;
- }
-
- *SerialConsoleAddress = fdt64_to_cpu (ReadUnaligned64 (RegProperty));
-
- return EFI_SUCCESS;
-}
+#include <Library/FdtSerialPortAddressLib.h>
/** Platform hook to retrieve the 16550 UART base address from the platform
Device tree and store it in PcdSerialRegisterBase.
@@ -108,6 +25,7 @@ GetSerialConsolePortAddress (
@retval RETURN_SUCCESS Success.
@retval RETURN_INVALID_PARAMETER A parameter was invalid.
@retval RETURN_NOT_FOUND Serial port information not found.
+ @retval RETURN_PROTOCOL_ERROR Invalid information in the Device Tree.
**/
RETURN_STATUS
@@ -129,7 +47,7 @@ PlatformHookSerialPortInitialize (
return RETURN_NOT_FOUND;
}
- Status = GetSerialConsolePortAddress (DeviceTreeBase, &SerialConsoleAddress);
+ Status = FdtSerialGetConsolePort (DeviceTreeBase, &SerialConsoleAddress);
if (RETURN_ERROR (Status)) {
return Status;
}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109400): https://edk2.groups.io/g/devel/message/109400
Mute This Topic: https://groups.io/mt/101834887/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 3/9] ArmVirtPkg: adjust whitespace in block scope declarations
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 1/9] ArmVirtPkg: introduce FdtSerialPortAddressLib Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 2/9] ArmVirtPkg/Fdt16550SerialPortHookLib: rebase to FdtSerialPortAddressLib Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 4/9] ArmVirtPkg: adhere to the serial port selected by /chosen "stdout-path" Laszlo Ersek
` (6 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
I strongly dislike when *small* local variable declaration changes are
muddled by whitespace changes. When that happens, a reviewer can choose
from two suboptimal options: display the patch with "git show -b", which
creates confusion in *other* parts of the patch, or display the patch with
just "git show", which then produces an unjustifiedly large hunk for the
sequence of declarations.
For avoiding that in subsequent patches, adjust some whitespace in this
patch in isolation. Functionally this is a no-op; "git show -b" produces
empty output.
Note that uncrustify is (of course) unhappy with this patch, but that's
fine -- this patch is in the middle of a series, and by the end of the
series (which is where uncrustify is run in CI) the whitespace is going to
be tight.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 18 +++++-----
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 38 ++++++++++----------
2 files changed, 28 insertions(+), 28 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index 9d71f369570f..5718b02977df 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -44,15 +44,15 @@ SerialPortInitialize (
VOID
)
{
- VOID *Hob;
- RETURN_STATUS Status;
- CONST UINT64 *UartBase;
- UINTN SerialBaseAddress;
- UINT64 BaudRate;
- UINT32 ReceiveFifoDepth;
- EFI_PARITY_TYPE Parity;
- UINT8 DataBits;
- EFI_STOP_BITS_TYPE StopBits;
+ VOID *Hob;
+ RETURN_STATUS Status;
+ CONST UINT64 *UartBase;
+ UINTN SerialBaseAddress;
+ UINT64 BaudRate;
+ UINT32 ReceiveFifoDepth;
+ EFI_PARITY_TYPE Parity;
+ UINT8 DataBits;
+ EFI_STOP_BITS_TYPE StopBits;
if (mSerialBaseAddress != 0) {
return RETURN_SUCCESS;
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index 8d9dcf504dc6..1b43d77dd120 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -37,25 +37,25 @@ PlatformPeim (
VOID
)
{
- VOID *Base;
- VOID *NewBase;
- UINTN FdtSize;
- UINTN FdtPages;
- UINT64 *FdtHobData;
- UINT64 *UartHobData;
- INT32 Node, Prev;
- INT32 Parent, Depth;
- CONST CHAR8 *Compatible;
- CONST CHAR8 *CompItem;
- CONST CHAR8 *NodeStatus;
- INT32 Len;
- INT32 RangesLen;
- INT32 StatusLen;
- CONST UINT64 *RegProp;
- CONST UINT32 *RangesProp;
- UINT64 UartBase;
- UINT64 TpmBase;
- EFI_STATUS Status;
+ VOID *Base;
+ VOID *NewBase;
+ UINTN FdtSize;
+ UINTN FdtPages;
+ UINT64 *FdtHobData;
+ UINT64 *UartHobData;
+ INT32 Node, Prev;
+ INT32 Parent, Depth;
+ CONST CHAR8 *Compatible;
+ CONST CHAR8 *CompItem;
+ CONST CHAR8 *NodeStatus;
+ INT32 Len;
+ INT32 RangesLen;
+ INT32 StatusLen;
+ CONST UINT64 *RegProp;
+ CONST UINT32 *RangesProp;
+ UINT64 UartBase;
+ UINT64 TpmBase;
+ EFI_STATUS Status;
Base = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
ASSERT (Base != NULL);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109392): https://edk2.groups.io/g/devel/message/109392
Mute This Topic: https://groups.io/mt/101834876/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 4/9] ArmVirtPkg: adhere to the serial port selected by /chosen "stdout-path"
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (2 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 3/9] ArmVirtPkg: adjust whitespace in block scope declarations Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 5/9] ArmVirtPkg: store separate console and debug PL011 addresses in GUID HOB Laszlo Ersek
` (5 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
Convert both EarlyFdtPL011SerialPortLib and PlatformPeiLib at the same
time to clients of FdtSerialPortAddressLib (so that both "early" and
"late" serial output continue going to a common serial port). If the
device tree specifies just one serial port, this conversion makes no
difference, but if there are multiple ports, the output is written to the
port identified by /chosen "stdout-path".
In this patch, DebugLib output is not separated yet from the UEFI console.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf | 3 +-
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 1 +
ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 90 +++++++-------------
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 42 ++++-----
4 files changed, 56 insertions(+), 80 deletions(-)
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
index 32b2d337d412..f47692f06a95 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
@@ -22,11 +22,10 @@ [Sources.common]
[LibraryClasses]
PL011UartLib
PcdLib
- FdtLib
+ FdtSerialPortAddressLib
[Packages]
MdePkg/MdePkg.dec
- EmbeddedPkg/EmbeddedPkg.dec
ArmPlatformPkg/ArmPlatformPkg.dec
ArmVirtPkg/ArmVirtPkg.dec
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
index 3f97ef080520..08a8f23bb449 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
@@ -34,6 +34,7 @@ [LibraryClasses]
DebugLib
HobLib
FdtLib
+ FdtSerialPortAddressLib
PcdLib
PeiServicesLib
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
index c34021243210..dc5459b4ce66 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
@@ -15,7 +15,7 @@
#include <Library/PcdLib.h>
#include <Library/PL011UartLib.h>
#include <Library/SerialPortLib.h>
-#include <libfdt.h>
+#include <Library/FdtSerialPortAddressLib.h>
RETURN_STATUS
EFIAPI
@@ -56,74 +56,48 @@ SerialPortGetBaseAddress (
UINT8 DataBits;
EFI_STOP_BITS_TYPE StopBits;
VOID *DeviceTreeBase;
- INT32 Node, Prev;
- INT32 Len;
- CONST CHAR8 *Compatible;
- CONST CHAR8 *NodeStatus;
- CONST CHAR8 *CompatibleItem;
- CONST UINT64 *RegProperty;
- UINTN UartBase;
+ FDT_SERIAL_PORTS Ports;
+ UINT64 UartBase;
RETURN_STATUS Status;
DeviceTreeBase = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
- if ((DeviceTreeBase == NULL) || (fdt_check_header (DeviceTreeBase) != 0)) {
+ if (DeviceTreeBase == NULL) {
+ return 0;
+ }
+
+ Status = FdtSerialGetPorts (DeviceTreeBase, "arm,pl011", &Ports);
+ if (RETURN_ERROR (Status)) {
return 0;
}
//
- // Enumerate all FDT nodes looking for a PL011 and capture its base address
+ // Default to the first port found, but (if there are multiple ports) allow
+ // the "/chosen" node to override it. Note that if FdtSerialGetConsolePort()
+ // fails, it does not modify UartBase.
//
- for (Prev = 0; ; Prev = Node) {
- Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
- if (Node < 0) {
- break;
- }
-
- Compatible = fdt_getprop (DeviceTreeBase, Node, "compatible", &Len);
- if (Compatible == NULL) {
- continue;
- }
-
- //
- // Iterate over the NULL-separated items in the compatible string
- //
- for (CompatibleItem = Compatible; CompatibleItem < Compatible + Len;
- CompatibleItem += 1 + AsciiStrLen (CompatibleItem))
- {
- if (AsciiStrCmp (CompatibleItem, "arm,pl011") == 0) {
- NodeStatus = fdt_getprop (DeviceTreeBase, Node, "status", &Len);
- if ((NodeStatus != NULL) && (AsciiStrCmp (NodeStatus, "okay") != 0)) {
- continue;
- }
-
- RegProperty = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
- if (Len != 16) {
- return 0;
- }
-
- UartBase = (UINTN)fdt64_to_cpu (ReadUnaligned64 (RegProperty));
+ UartBase = Ports.BaseAddress[0];
+ if (Ports.NumberOfPorts > 1) {
+ FdtSerialGetConsolePort (DeviceTreeBase, &UartBase);
+ }
- BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
- ReceiveFifoDepth = 0; // Use the default value for Fifo depth
- Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
- DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
- StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
+ BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
+ ReceiveFifoDepth = 0; // Use the default value for Fifo depth
+ Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
+ DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
+ StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
- Status = PL011UartInitializePort (
- UartBase,
- FixedPcdGet32 (PL011UartClkInHz),
- &BaudRate,
- &ReceiveFifoDepth,
- &Parity,
- &DataBits,
- &StopBits
- );
- if (!EFI_ERROR (Status)) {
- return UartBase;
- }
- }
- }
+ Status = PL011UartInitializePort (
+ UartBase,
+ FixedPcdGet32 (PL011UartClkInHz),
+ &BaudRate,
+ &ReceiveFifoDepth,
+ &Parity,
+ &DataBits,
+ &StopBits
+ );
+ if (!RETURN_ERROR (Status)) {
+ return UartBase;
}
return 0;
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index 1b43d77dd120..d5dcc7cbfd52 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -14,6 +14,7 @@
#include <Library/HobLib.h>
#include <Library/PcdLib.h>
#include <Library/PeiServicesLib.h>
+#include <Library/FdtSerialPortAddressLib.h>
#include <libfdt.h>
#include <Guid/EarlyPL011BaseAddress.h>
@@ -43,17 +44,15 @@ PlatformPeim (
UINTN FdtPages;
UINT64 *FdtHobData;
UINT64 *UartHobData;
+ FDT_SERIAL_PORTS Ports;
INT32 Node, Prev;
INT32 Parent, Depth;
CONST CHAR8 *Compatible;
CONST CHAR8 *CompItem;
- CONST CHAR8 *NodeStatus;
INT32 Len;
INT32 RangesLen;
- INT32 StatusLen;
CONST UINT64 *RegProp;
CONST UINT32 *RangesProp;
- UINT64 UartBase;
UINT64 TpmBase;
EFI_STATUS Status;
@@ -75,6 +74,24 @@ PlatformPeim (
ASSERT (UartHobData != NULL);
*UartHobData = 0;
+ Status = FdtSerialGetPorts (Base, "arm,pl011", &Ports);
+ if (!EFI_ERROR (Status)) {
+ UINT64 UartBase;
+
+ //
+ // Default to the first port found, but (if there are multiple ports) allow
+ // the "/chosen" node to override it. Note that if FdtSerialGetConsolePort()
+ // fails, it does not modify UartBase.
+ //
+ UartBase = Ports.BaseAddress[0];
+ if (Ports.NumberOfPorts > 1) {
+ FdtSerialGetConsolePort (Base, &UartBase);
+ }
+
+ DEBUG ((DEBUG_INFO, "%a: PL011 UART @ 0x%lx\n", __func__, UartBase));
+ *UartHobData = UartBase;
+ }
+
TpmBase = 0;
//
@@ -100,23 +117,8 @@ PlatformPeim (
for (CompItem = Compatible; CompItem != NULL && CompItem < Compatible + Len;
CompItem += 1 + AsciiStrLen (CompItem))
{
- if (AsciiStrCmp (CompItem, "arm,pl011") == 0) {
- NodeStatus = fdt_getprop (Base, Node, "status", &StatusLen);
- if ((NodeStatus != NULL) && (AsciiStrCmp (NodeStatus, "okay") != 0)) {
- continue;
- }
-
- RegProp = fdt_getprop (Base, Node, "reg", &Len);
- ASSERT (Len == 16);
-
- UartBase = fdt64_to_cpu (ReadUnaligned64 (RegProp));
-
- DEBUG ((DEBUG_INFO, "%a: PL011 UART @ 0x%lx\n", __func__, UartBase));
-
- *UartHobData = UartBase;
- break;
- } else if (FeaturePcdGet (PcdTpm2SupportEnabled) &&
- (AsciiStrCmp (CompItem, "tcg,tpm-tis-mmio") == 0))
+ if (FeaturePcdGet (PcdTpm2SupportEnabled) &&
+ (AsciiStrCmp (CompItem, "tcg,tpm-tis-mmio") == 0))
{
RegProp = fdt_getprop (Base, Node, "reg", &Len);
ASSERT (Len == 8 || Len == 16);
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109396): https://edk2.groups.io/g/devel/message/109396
Mute This Topic: https://groups.io/mt/101834882/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 5/9] ArmVirtPkg: store separate console and debug PL011 addresses in GUID HOB
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (3 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 4/9] ArmVirtPkg: adhere to the serial port selected by /chosen "stdout-path" Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 6/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart Flash instance Laszlo Ersek
` (4 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
PlatformPeiLib produces the EarlyPL011BaseAddress GUID HOB, and
FdtPL011SerialPortLib consumes it. Extend the HOB such that it also carry
the base address of the PL011 UART meant for DebugLib usage -- namely the
first UART that is *not* designated by the /chosen node's "stdout-path"
property. Implement this policy in PlatformPeiLib.
Note that as far as the SerialPortLib+console UART is concerned, this
patch makes no difference. That selection remains consistent with the
pre-patch state, and therefore consistent with EarlyFdtPL011SerialPortLib.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h | 15 ++++-
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf | 1 +
ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c | 4 +-
ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c | 58 +++++++++++++++-----
4 files changed, 61 insertions(+), 17 deletions(-)
diff --git a/ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h b/ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h
index 492cbbcb1599..43b106f3ff04 100644
--- a/ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h
+++ b/ArmVirtPkg/Include/Guid/EarlyPL011BaseAddress.h
@@ -1,6 +1,6 @@
/** @file
- GUID for the HOB that caches the base address of the PL011 serial port, for
- when PCD access is not available.
+ GUID for the HOB that caches the base address(es) of the PL011 serial port(s),
+ for when PCD access is not available.
Copyright (C) 2014, Red Hat, Inc.
@@ -18,4 +18,15 @@
extern EFI_GUID gEarlyPL011BaseAddressGuid;
+typedef struct {
+ //
+ // for SerialPortLib and console IO
+ //
+ UINT64 ConsoleAddress;
+ //
+ // for DebugLib; may equal ConsoleAddress if there's only one PL011 UART
+ //
+ UINT64 DebugAddress;
+} EARLY_PL011_BASE_ADDRESS;
+
#endif
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
index 08a8f23bb449..b867d8bb895e 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf
@@ -31,6 +31,7 @@ [FeaturePcd]
gArmVirtTokenSpaceGuid.PcdTpm2SupportEnabled
[LibraryClasses]
+ BaseMemoryLib
DebugLib
HobLib
FdtLib
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index 5718b02977df..20e29e3f57f4 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -46,7 +46,7 @@ SerialPortInitialize (
{
VOID *Hob;
RETURN_STATUS Status;
- CONST UINT64 *UartBase;
+ CONST EARLY_PL011_BASE_ADDRESS *UartBase;
UINTN SerialBaseAddress;
UINT64 BaudRate;
UINT32 ReceiveFifoDepth;
@@ -70,7 +70,7 @@ SerialPortInitialize (
UartBase = GET_GUID_HOB_DATA (Hob);
- SerialBaseAddress = (UINTN)*UartBase;
+ SerialBaseAddress = (UINTN)UartBase->ConsoleAddress;
if (SerialBaseAddress == 0) {
Status = RETURN_NOT_FOUND;
goto Failed;
diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index d5dcc7cbfd52..7ab4aa2d6bb9 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -9,6 +9,7 @@
#include <PiPei.h>
+#include <Library/BaseMemoryLib.h>
#include <Library/MemoryAllocationLib.h>
#include <Library/DebugLib.h>
#include <Library/HobLib.h>
@@ -43,7 +44,7 @@ PlatformPeim (
UINTN FdtSize;
UINTN FdtPages;
UINT64 *FdtHobData;
- UINT64 *UartHobData;
+ EARLY_PL011_BASE_ADDRESS *UartHobData;
FDT_SERIAL_PORTS Ports;
INT32 Node, Prev;
INT32 Parent, Depth;
@@ -72,24 +73,55 @@ PlatformPeim (
UartHobData = BuildGuidHob (&gEarlyPL011BaseAddressGuid, sizeof *UartHobData);
ASSERT (UartHobData != NULL);
- *UartHobData = 0;
+ SetMem (UartHobData, sizeof *UartHobData, 0);
Status = FdtSerialGetPorts (Base, "arm,pl011", &Ports);
if (!EFI_ERROR (Status)) {
- UINT64 UartBase;
+ if (Ports.NumberOfPorts == 1) {
+ //
+ // Just one UART; direct both SerialPortLib+console and DebugLib to it.
+ //
+ UartHobData->ConsoleAddress = Ports.BaseAddress[0];
+ UartHobData->DebugAddress = Ports.BaseAddress[0];
+ } else {
+ UINT64 ConsoleAddress;
- //
- // Default to the first port found, but (if there are multiple ports) allow
- // the "/chosen" node to override it. Note that if FdtSerialGetConsolePort()
- // fails, it does not modify UartBase.
- //
- UartBase = Ports.BaseAddress[0];
- if (Ports.NumberOfPorts > 1) {
- FdtSerialGetConsolePort (Base, &UartBase);
+ Status = FdtSerialGetConsolePort (Base, &ConsoleAddress);
+ if (EFI_ERROR (Status)) {
+ //
+ // At least two UARTs; but failed to get the console preference. Use the
+ // first UART for SerialPortLib+console, and the second one for
+ // DebugLib.
+ //
+ UartHobData->ConsoleAddress = Ports.BaseAddress[0];
+ UartHobData->DebugAddress = Ports.BaseAddress[1];
+ } else {
+ //
+ // At least two UARTs; and console preference available. Use the
+ // preferred UART for SerialPortLib+console, and *another* UART for
+ // DebugLib.
+ //
+ UartHobData->ConsoleAddress = ConsoleAddress;
+ if (ConsoleAddress == Ports.BaseAddress[0]) {
+ UartHobData->DebugAddress = Ports.BaseAddress[1];
+ } else {
+ UartHobData->DebugAddress = Ports.BaseAddress[0];
+ }
+ }
}
- DEBUG ((DEBUG_INFO, "%a: PL011 UART @ 0x%lx\n", __func__, UartBase));
- *UartHobData = UartBase;
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: PL011 UART (console) @ 0x%lx\n",
+ __func__,
+ UartHobData->ConsoleAddress
+ ));
+ DEBUG ((
+ DEBUG_INFO,
+ "%a: PL011 UART (debug) @ 0x%lx\n",
+ __func__,
+ UartHobData->DebugAddress
+ ));
}
TpmBase = 0;
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109393): https://edk2.groups.io/g/devel/message/109393
Mute This Topic: https://groups.io/mt/101834879/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 6/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart Flash instance
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (4 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 5/9] ArmVirtPkg: store separate console and debug PL011 addresses in GUID HOB Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 7/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart RAM instance Laszlo Ersek
` (3 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
Introduce three new DebugLib instances, forked from
MdePkg/Library/BaseDebugLibSerialPort. All three instances rely on
PL011UartLib rather than SerialPortLib so that they can customize the
PL011 UART that the debug messages are written to. All three instances
direct the debug output to the first such PL011 UART that *differs* from
the one specified in the Device Tree's /chosen node's "stdout-path"
property.
From these, DebugLibFdtPL011UartFlash mirrors EarlyFdtPL011SerialPortLib:
it parses the initial Device Tree, and initializes the UART -- a UART
different from EarlyFdtPL011SerialPortLib's -- for every message written.
Suitable for SEC, PEI_CORE, PEIM.
(Note that OVMF uses a similar set of dedicated DebugLib instances
(PlatformDebugLibIoPort) for logging to the (x86-only) isa-debugcon device
from various firmware phases.)
The contexts in which these DebugLib instances run are identical to those
in which the corresponding SerialPortLib instances run. The particular
original dependency chain is
BaseDebugLibSerialPort (SEC, PEI_CORE, PEIM)
EarlyFdtPL011SerialPortLib
PcdDeviceTreeInitialBaseAddress
FdtSerialPortAddressLib
PL011UartLib
and the new dependency chain is
DebugLibFdtPL011UartFlash (SEC, PEI_CORE, PEIM)
PcdDeviceTreeInitialBaseAddress
FdtSerialPortAddressLib
PL011UartLib
Note that EarlyFdtPL011SerialPortLib remains in use (just not via
BaseDebugLibSerialPort), namely for direct SerialPortLib calls from SEC,
PEI_CORE, PEIM. See for example commit 56035d1c8b25
("ArmPlatformPkg/PrePeiCore: Print the firmware version early in boot",
2022-10-25).
The ArmVirtPkg DSC files will be switched to the new library instances in
a separate patch.
This patch is worth viewing with "git show --find-copies-harder".
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf | 54 ++++++++++
ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h | 39 +++++++
{MdePkg/Library/BaseDebugLibSerialPort => ArmVirtPkg/Library/DebugLibFdtPL011Uart}/DebugLib.c | 41 +++-----
ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c | 107 ++++++++++++++++++++
4 files changed, 216 insertions(+), 25 deletions(-)
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
new file mode 100644
index 000000000000..7870ca2ae47f
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
@@ -0,0 +1,54 @@
+## @file
+# DebugLib instance that produces debug output directly via PL011UartLib.
+#
+# If there are at least two PL011 UARTs in the device tree, and the /chosen
+# node's "stdout-path" property references one PL011 UART, then both raw
+# SerialPortLib IO, and -- via SerialDxe -- UEFI console IO, will occur on that
+# UART; and this DebugLib instance will produce output on a *different* UART.
+#
+# This instance is suitable for modules that may run from flash or RAM.
+#
+# Copyright (C) Red Hat
+#
+# SPDX-License-Identifier: BSD-2-Clause-Patent
+##
+
+[Defines]
+ INF_VERSION = 1.27
+ BASE_NAME = DebugLibFdtPL011UartFlash
+ FILE_GUID = 43A4C56B-D071-4CE0-A157-9D59E6161DEC
+ MODULE_TYPE = BASE
+ VERSION_STRING = 1.0
+ LIBRARY_CLASS = DebugLib|SEC PEI_CORE PEIM
+
+[Sources]
+ DebugLib.c
+ Flash.c
+ Write.h
+
+[Packages]
+ ArmPlatformPkg/ArmPlatformPkg.dec
+ ArmVirtPkg/ArmVirtPkg.dec
+ MdePkg/MdePkg.dec
+
+[LibraryClasses]
+ BaseLib
+ BaseMemoryLib
+ DebugPrintErrorLevelLib
+ FdtSerialPortAddressLib # Flash.c
+ PL011UartLib
+ PcdLib
+ PrintLib
+
+[Pcd]
+ gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress # Flash.c
+ gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
+ gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask
+ gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel
+
+[FixedPcd]
+ gArmPlatformTokenSpaceGuid.PL011UartClkInHz
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
+ gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h
new file mode 100644
index 000000000000..2cf610676423
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Write.h
@@ -0,0 +1,39 @@
+/** @file
+ Declare DebugLibFdtPL011UartWrite(), for abstracting PL011 UART initialization
+ differences between flash- vs. RAM-based modules.
+
+ Copyright (C) Red Hat
+ Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2012 - 2014, ARM Ltd. All rights reserved.
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef DEBUG_LIB_FDT_PL011_UART_WRITE_H_
+#define DEBUG_LIB_FDT_PL011_UART_WRITE_H_
+
+/**
+ (Copied from SerialPortWrite() in "MdePkg/Include/Library/SerialPortLib.h" at
+ commit c4547aefb3d0, with the Buffer non-nullity assertion removed:)
+
+ Write data from buffer to serial device.
+
+ Writes NumberOfBytes data bytes from Buffer to the serial device.
+ The number of bytes actually written to the serial device is returned.
+ If the return value is less than NumberOfBytes, then the write operation failed.
+ If NumberOfBytes is zero, then return 0.
+
+ @param Buffer Pointer to the data buffer to be written.
+ @param NumberOfBytes Number of bytes to written to the serial device.
+
+ @retval 0 NumberOfBytes is 0.
+ @retval >0 The number of bytes written to the serial device.
+ If this value is less than NumberOfBytes, then the write operation failed.
+**/
+UINTN
+DebugLibFdtPL011UartWrite (
+ IN UINT8 *Buffer,
+ IN UINTN NumberOfBytes
+ );
+
+#endif
diff --git a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLib.c
similarity index 89%
copy from MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
copy to ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLib.c
index bd5686947712..0da84ba8d263 100644
--- a/MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLib.c
@@ -1,15 +1,20 @@
/** @file
- Base Debug library instance base on Serial Port library.
- It uses PrintLib to send debug messages to serial port device.
+ Originally copied from "MdePkg/Library/BaseDebugLibSerialPort/DebugLib.c" at
+ commit f36e1ec1f0a5, and customized for:
- NOTE: If the Serial Port library enables hardware flow control, then a call
- to DebugPrint() or DebugAssert() may hang if writes to the serial port are
- being blocked. This may occur if a key(s) are pressed in a terminal emulator
- used to monitor the DEBUG() and ASSERT() messages.
+ - RAM vs. flash dependent PL011 UART initialization,
+ - direct PL011 UART access, with the base address taken from the device tree
+ such that the debug output be separate from the SerialPortLib / UEFI console
+ traffic.
+
+ Both of these customizations are hidden behind DebugLibFdtPL011UartWrite(),
+ which replaces SerialPortWrite().
+
+ Copyright (C) Red Hat
Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+
SPDX-License-Identifier: BSD-2-Clause-Patent
-
**/
#include <Base.h>
@@ -18,9 +23,10 @@
#include <Library/PrintLib.h>
#include <Library/PcdLib.h>
#include <Library/BaseMemoryLib.h>
-#include <Library/SerialPortLib.h>
#include <Library/DebugPrintErrorLevelLib.h>
+#include "Write.h"
+
//
// Define the maximum debug and assert message length that this library supports
//
@@ -32,21 +38,6 @@
//
VA_LIST mVaListNull;
-/**
- The constructor function initialize the Serial Port Library
-
- @retval EFI_SUCCESS The constructor always returns RETURN_SUCCESS.
-
-**/
-RETURN_STATUS
-EFIAPI
-BaseDebugLibSerialPortConstructor (
- VOID
- )
-{
- return SerialPortInitialize ();
-}
-
/**
Prints a debug message to the debug output device if the specified error level is enabled.
@@ -128,7 +119,7 @@ DebugPrintMarker (
//
// Send the print string to a Serial Port
//
- SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
+ DebugLibFdtPL011UartWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
}
/**
@@ -224,7 +215,7 @@ DebugAssert (
//
// Send the print string to the Console Output device
//
- SerialPortWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
+ DebugLibFdtPL011UartWrite ((UINT8 *)Buffer, AsciiStrLen (Buffer));
//
// Generate a Breakpoint, DeadLoop, or NOP based on PCD settings
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c
new file mode 100644
index 000000000000..a624e0860d10
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Flash.c
@@ -0,0 +1,107 @@
+/** @file
+ Define DebugLibFdtPL011UartWrite() for modules that may run from flash or RAM.
+
+ Copyright (C) Red Hat
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Library/FdtSerialPortAddressLib.h>
+#include <Library/PL011UartLib.h>
+#include <Library/PcdLib.h>
+
+#include "Write.h"
+
+/**
+ (Copied from SerialPortWrite() in "MdePkg/Include/Library/SerialPortLib.h" at
+ commit c4547aefb3d0, with the Buffer non-nullity assertion removed:)
+
+ Write data from buffer to serial device.
+
+ Writes NumberOfBytes data bytes from Buffer to the serial device.
+ The number of bytes actually written to the serial device is returned.
+ If the return value is less than NumberOfBytes, then the write operation failed.
+ If NumberOfBytes is zero, then return 0.
+
+ @param Buffer Pointer to the data buffer to be written.
+ @param NumberOfBytes Number of bytes to written to the serial device.
+
+ @retval 0 NumberOfBytes is 0.
+ @retval >0 The number of bytes written to the serial device.
+ If this value is less than NumberOfBytes, then the write operation failed.
+**/
+UINTN
+DebugLibFdtPL011UartWrite (
+ IN UINT8 *Buffer,
+ IN UINTN NumberOfBytes
+ )
+{
+ CONST VOID *DeviceTree;
+ RETURN_STATUS Status;
+ FDT_SERIAL_PORTS Ports;
+ UINT64 DebugAddress;
+ UINT64 BaudRate;
+ UINT32 ReceiveFifoDepth;
+ EFI_PARITY_TYPE Parity;
+ UINT8 DataBits;
+ EFI_STOP_BITS_TYPE StopBits;
+
+ DeviceTree = (VOID *)(UINTN)PcdGet64 (PcdDeviceTreeInitialBaseAddress);
+ if (DeviceTree == NULL) {
+ return 0;
+ }
+
+ Status = FdtSerialGetPorts (DeviceTree, "arm,pl011", &Ports);
+ if (RETURN_ERROR (Status)) {
+ return 0;
+ }
+
+ if (Ports.NumberOfPorts == 1) {
+ //
+ // Just one UART; direct DebugLib to it.
+ //
+ DebugAddress = Ports.BaseAddress[0];
+ } else {
+ UINT64 ConsoleAddress;
+
+ Status = FdtSerialGetConsolePort (DeviceTree, &ConsoleAddress);
+ if (EFI_ERROR (Status)) {
+ //
+ // At least two UARTs; but failed to get the console preference. Use the
+ // second UART for DebugLib.
+ //
+ DebugAddress = Ports.BaseAddress[1];
+ } else {
+ //
+ // At least two UARTs; and console preference available. Use the first
+ // such UART for DebugLib that *differs* from ConsoleAddress.
+ //
+ if (ConsoleAddress == Ports.BaseAddress[0]) {
+ DebugAddress = Ports.BaseAddress[1];
+ } else {
+ DebugAddress = Ports.BaseAddress[0];
+ }
+ }
+ }
+
+ BaudRate = (UINTN)FixedPcdGet64 (PcdUartDefaultBaudRate);
+ ReceiveFifoDepth = 0; // Use the default value for Fifo depth
+ Parity = (EFI_PARITY_TYPE)FixedPcdGet8 (PcdUartDefaultParity);
+ DataBits = FixedPcdGet8 (PcdUartDefaultDataBits);
+ StopBits = (EFI_STOP_BITS_TYPE)FixedPcdGet8 (PcdUartDefaultStopBits);
+
+ Status = PL011UartInitializePort (
+ (UINTN)DebugAddress,
+ FixedPcdGet32 (PL011UartClkInHz),
+ &BaudRate,
+ &ReceiveFifoDepth,
+ &Parity,
+ &DataBits,
+ &StopBits
+ );
+ if (RETURN_ERROR (Status)) {
+ return 0;
+ }
+
+ return PL011UartWrite ((UINTN)DebugAddress, Buffer, NumberOfBytes);
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109395): https://edk2.groups.io/g/devel/message/109395
Mute This Topic: https://groups.io/mt/101834881/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 7/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart RAM instance
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (5 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 6/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart Flash instance Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 8/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart DXE Runtime instance Laszlo Ersek
` (2 subsequent siblings)
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
Introduce three new DebugLib instances, forked from
MdePkg/Library/BaseDebugLibSerialPort. All three instances rely on
PL011UartLib rather than SerialPortLib so that they can customize the
PL011 UART that the debug messages are written to. All three instances
direct the debug output to the first such PL011 UART that *differs* from
the one specified in the Device Tree's /chosen node's "stdout-path"
property.
From these, DebugLibFdtPL011UartRam mirrors FdtPL011SerialPortLib: it
relies on the EarlyPL011BaseAddress GUID HOB, and initializes the UART --
a UART different from FdtPL011SerialPortLib's -- only once in the lifetime
of the containing module. Suitable for module types that can only execute
from RAM (i.e., all types different from SEC, PEI_CORE, PEIM), except
DXE_RUNTIME_DRIVER.
(Note that OVMF uses a similar set of dedicated DebugLib instances
(PlatformDebugLibIoPort) for logging to the (x86-only) isa-debugcon device
from various firmware phases.)
The contexts in which these DebugLib instances run are identical to those
in which the corresponding SerialPortLib instances run. The particular
original dependency chain is
BaseDebugLibSerialPort (not SEC, PEI_CORE, PEIM, DXE_RUNTIME_DRIVER)
FdtPL011SerialPortLib
gEarlyPL011BaseAddressGuid
HobLib
PL011UartLib
and the new dependency chain is
DebugLibFdtPL011UartRam (not SEC, PEI_CORE, PEIM, DXE_RUNTIME_DRIVER)
gEarlyPL011BaseAddressGuid
HobLib
PL011UartLib
Note that FdtPL011SerialPortLib remains in use (just not via
BaseDebugLibSerialPort); for instance by MdeModulePkg/Universal/SerialDxe,
which produces the SerialIo protocol, underlying the UEFI console.
The ArmVirtPkg DSC files will be switched to the new library instances in
a separate patch.
This patch is worth viewing with "git show --find-copies-harder".
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/DebugLibFdtPL011Uart/{DebugLibFdtPL011UartFlash.inf => DebugLibFdtPL011UartRam.inf} | 20 ++--
ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h | 18 +++
ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c | 124 ++++++++++++++++++++
ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c | 27 +++++
4 files changed, 182 insertions(+), 7 deletions(-)
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
similarity index 67%
copy from ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
copy to ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
index 7870ca2ae47f..a5f4c2d80a3c 100644
--- a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
@@ -6,7 +6,8 @@
# SerialPortLib IO, and -- via SerialDxe -- UEFI console IO, will occur on that
# UART; and this DebugLib instance will produce output on a *different* UART.
#
-# This instance is suitable for modules that may run from flash or RAM.
+# This instance is suitable for modules that can only run from RAM (except
+# DXE_RUNTIME_DRIVER).
#
# Copyright (C) Red Hat
#
@@ -15,15 +16,18 @@
[Defines]
INF_VERSION = 1.27
- BASE_NAME = DebugLibFdtPL011UartFlash
- FILE_GUID = 43A4C56B-D071-4CE0-A157-9D59E6161DEC
+ BASE_NAME = DebugLibFdtPL011UartRam
+ FILE_GUID = 0584DE55-9C4C-49C1-ADA0-F62C9C1F3600
MODULE_TYPE = BASE
VERSION_STRING = 1.0
- LIBRARY_CLASS = DebugLib|SEC PEI_CORE PEIM
+ LIBRARY_CLASS = DebugLib|DXE_CORE SMM_CORE MM_CORE_STANDALONE DXE_DRIVER DXE_SMM_DRIVER SMM_DRIVER MM_STANDALONE UEFI_DRIVER UEFI_APPLICATION
+ CONSTRUCTOR = DebugLibFdtPL011UartRamConstructor
[Sources]
DebugLib.c
- Flash.c
+ Ram.c
+ Ram.h
+ RamNonRuntime.c
Write.h
[Packages]
@@ -35,13 +39,12 @@ [LibraryClasses]
BaseLib
BaseMemoryLib
DebugPrintErrorLevelLib
- FdtSerialPortAddressLib # Flash.c
+ HobLib # Ram.c
PL011UartLib
PcdLib
PrintLib
[Pcd]
- gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress # Flash.c
gEfiMdePkgTokenSpaceGuid.PcdDebugClearMemoryValue
gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask
gEfiMdePkgTokenSpaceGuid.PcdFixedDebugPrintErrorLevel
@@ -52,3 +55,6 @@ [FixedPcd]
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+
+[Guids]
+ gEarlyPL011BaseAddressGuid # Ram.c
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h
new file mode 100644
index 000000000000..8c1ef52b4dba
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.h
@@ -0,0 +1,18 @@
+/** @file
+ Declare the variables that modules that can only run from RAM use for
+ remembering initialization status.
+
+ Copyright (C) Red Hat
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef DEBUG_LIB_FDT_PL011_UART_RAM_H_
+#define DEBUG_LIB_FDT_PL011_UART_RAM_H_
+
+#include <Base.h>
+
+extern UINTN mDebugLibFdtPL011UartAddress;
+extern RETURN_STATUS mDebugLibFdtPL011UartPermanentStatus;
+
+#endif
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c
new file mode 100644
index 000000000000..bc5be015bded
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Ram.c
@@ -0,0 +1,124 @@
+/** @file
+ Define DebugLibFdtPL011UartWrite() for modules that can only run from RAM.
+
+ Copyright (C) Red Hat
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Uefi/UefiBaseType.h>
+#include <Uefi/UefiMultiPhase.h>
+#include <Pi/PiBootMode.h>
+#include <Pi/PiHob.h>
+#include <Library/HobLib.h>
+#include <Library/PL011UartLib.h>
+#include <Library/PcdLib.h>
+#include <Guid/EarlyPL011BaseAddress.h>
+
+#include "Ram.h"
+#include "Write.h"
+
+UINTN mDebugLibFdtPL011UartAddress;
+RETURN_STATUS mDebugLibFdtPL011UartPermanentStatus = RETURN_SUCCESS;
+
+/**
+ Statefully initialize both the library instance and the debug PL011 UART.
+**/
+STATIC
+RETURN_STATUS
+Initialize (
+ VOID
+ )
+{
+ CONST VOID *Hob;
+ CONST EARLY_PL011_BASE_ADDRESS *UartBase;
+ RETURN_STATUS Status;
+ UINTN DebugAddress;
+ UINT64 BaudRate;
+ UINT32 ReceiveFifoDepth;
+ EFI_PARITY_TYPE Parity;
+ UINT8 DataBits;
+ EFI_STOP_BITS_TYPE StopBits;
+
+ if (mDebugLibFdtPL011UartAddress != 0) {
+ return RETURN_SUCCESS;
+ }
+
+ if (RETURN_ERROR (mDebugLibFdtPL011UartPermanentStatus)) {
+ return mDebugLibFdtPL011UartPermanentStatus;
+ }
+
+ Hob = GetFirstGuidHob (&gEarlyPL011BaseAddressGuid);
+ if ((Hob == NULL) || (GET_GUID_HOB_DATA_SIZE (Hob) != sizeof *UartBase)) {
+ Status = RETURN_NOT_FOUND;
+ goto Failed;
+ }
+
+ UartBase = GET_GUID_HOB_DATA (Hob);
+
+ DebugAddress = (UINTN)UartBase->DebugAddress;
+ if (DebugAddress == 0) {
+ Status = RETURN_NOT_FOUND;
+ goto Failed;
+ }
+
+ BaudRate = (UINTN)PcdGet64 (PcdUartDefaultBaudRate);
+ ReceiveFifoDepth = 0; // Use the default value for Fifo depth
+ Parity = (EFI_PARITY_TYPE)PcdGet8 (PcdUartDefaultParity);
+ DataBits = PcdGet8 (PcdUartDefaultDataBits);
+ StopBits = (EFI_STOP_BITS_TYPE)PcdGet8 (PcdUartDefaultStopBits);
+
+ Status = PL011UartInitializePort (
+ DebugAddress,
+ FixedPcdGet32 (PL011UartClkInHz),
+ &BaudRate,
+ &ReceiveFifoDepth,
+ &Parity,
+ &DataBits,
+ &StopBits
+ );
+ if (RETURN_ERROR (Status)) {
+ goto Failed;
+ }
+
+ mDebugLibFdtPL011UartAddress = DebugAddress;
+ return RETURN_SUCCESS;
+
+Failed:
+ mDebugLibFdtPL011UartPermanentStatus = Status;
+ return Status;
+}
+
+/**
+ (Copied from SerialPortWrite() in "MdePkg/Include/Library/SerialPortLib.h" at
+ commit c4547aefb3d0, with the Buffer non-nullity assertion removed:)
+
+ Write data from buffer to serial device.
+
+ Writes NumberOfBytes data bytes from Buffer to the serial device.
+ The number of bytes actually written to the serial device is returned.
+ If the return value is less than NumberOfBytes, then the write operation failed.
+ If NumberOfBytes is zero, then return 0.
+
+ @param Buffer Pointer to the data buffer to be written.
+ @param NumberOfBytes Number of bytes to written to the serial device.
+
+ @retval 0 NumberOfBytes is 0.
+ @retval >0 The number of bytes written to the serial device.
+ If this value is less than NumberOfBytes, then the write operation failed.
+**/
+UINTN
+DebugLibFdtPL011UartWrite (
+ IN UINT8 *Buffer,
+ IN UINTN NumberOfBytes
+ )
+{
+ RETURN_STATUS Status;
+
+ Status = Initialize ();
+ if (RETURN_ERROR (Status)) {
+ return 0;
+ }
+
+ return PL011UartWrite (mDebugLibFdtPL011UartAddress, Buffer, NumberOfBytes);
+}
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c
new file mode 100644
index 000000000000..715d3400ddd9
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/RamNonRuntime.c
@@ -0,0 +1,27 @@
+/** @file
+ Provide an empty lib instance constructor for modules that can only run from
+ RAM but are not DXE_RUNTIME_DRIVER modules.
+
+ This ensures that e.g. any HobLib constructor is ordered correctly. (The
+ DXE_CORE calls constructors late, but the DXE_CORE HobLib instance needs no
+ construction anyway.)
+
+ Copyright (C) Red Hat
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Base.h>
+
+/**
+ Empty library instance constructor, only for ensuring the connectivity of the
+ constructor dependency graph.
+**/
+RETURN_STATUS
+EFIAPI
+DebugLibFdtPL011UartRamConstructor (
+ VOID
+ )
+{
+ return RETURN_SUCCESS;
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109399): https://edk2.groups.io/g/devel/message/109399
Mute This Topic: https://groups.io/mt/101834885/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 8/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart DXE Runtime instance
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (6 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 7/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart RAM instance Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-08 15:39 ` [edk2-devel] [PATCH 9/9] ArmVirtPkg: steer DebugLib output away from SerialPortLib+console traffic Laszlo Ersek
2023-10-10 7:43 ` [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Ard Biesheuvel
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel; +Cc: Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm, Sami Mujawar
Introduce three new DebugLib instances, forked from
MdePkg/Library/BaseDebugLibSerialPort. All three instances rely on
PL011UartLib rather than SerialPortLib so that they can customize the
PL011 UART that the debug messages are written to. All three instances
direct the debug output to the first such PL011 UART that *differs* from
the one specified in the Device Tree's /chosen node's "stdout-path"
property.
From these, DxeRuntimeDebugLibFdtPL011Uart is identical to
DebugLibFdtPL011UartRam, with the addition that UART access is permanently
disabled when the containing DXE_RUNTIME_DRIVER module is notified about
exiting boot services.
The contexts in which these DebugLib instances run are identical to those
in which the corresponding SerialPortLib instances run. The particular
original dependency chain is
DxeRuntimeDebugLibSerialPort (DXE_RUNTIME_DRIVER)
FdtPL011SerialPortLib
gEarlyPL011BaseAddressGuid
HobLib
PL011UartLib
and the new dependency chain is
DxeRuntimeDebugLibFdtPL011Uart (DXE_RUNTIME_DRIVER)
gEarlyPL011BaseAddressGuid
HobLib
PL011UartLib
The ArmVirtPkg DSC files will be switched to the new library instances in
a separate patch.
This patch is worth viewing with "git show --find-copies-harder".
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Leif Lindholm <quic_llindhol@quicinc.com>
Cc: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/Library/DebugLibFdtPL011Uart/{DebugLibFdtPL011UartRam.inf => DxeRuntimeDebugLibFdtPL011Uart.inf} | 17 ++--
ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c | 88 ++++++++++++++++++++
2 files changed, 97 insertions(+), 8 deletions(-)
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf
similarity index 71%
copy from ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
copy to ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf
index a5f4c2d80a3c..84e9dbae221b 100644
--- a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf
@@ -6,8 +6,8 @@
# SerialPortLib IO, and -- via SerialDxe -- UEFI console IO, will occur on that
# UART; and this DebugLib instance will produce output on a *different* UART.
#
-# This instance is suitable for modules that can only run from RAM (except
-# DXE_RUNTIME_DRIVER).
+# This instance is suitable for DXE_RUNTIME_DRIVER modules. When exiting boot
+# services, UART access is stopped.
#
# Copyright (C) Red Hat
#
@@ -16,18 +16,19 @@
[Defines]
INF_VERSION = 1.27
- BASE_NAME = DebugLibFdtPL011UartRam
- FILE_GUID = 0584DE55-9C4C-49C1-ADA0-F62C9C1F3600
- MODULE_TYPE = BASE
+ BASE_NAME = DxeRuntimeDebugLibFdtPL011Uart
+ FILE_GUID = 8A6E0972-81B5-4FF4-BB24-A07748415947
+ MODULE_TYPE = DXE_RUNTIME_DRIVER
VERSION_STRING = 1.0
- LIBRARY_CLASS = DebugLib|DXE_CORE SMM_CORE MM_CORE_STANDALONE DXE_DRIVER DXE_SMM_DRIVER SMM_DRIVER MM_STANDALONE UEFI_DRIVER UEFI_APPLICATION
- CONSTRUCTOR = DebugLibFdtPL011UartRamConstructor
+ LIBRARY_CLASS = DebugLib|DXE_RUNTIME_DRIVER
+ CONSTRUCTOR = DxeRuntimeDebugLibFdtPL011UartConstructor
+ DESTRUCTOR = DxeRuntimeDebugLibFdtPL011UartDestructor
[Sources]
DebugLib.c
Ram.c
Ram.h
- RamNonRuntime.c
+ Runtime.c
Write.h
[Packages]
diff --git a/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c
new file mode 100644
index 000000000000..de7144739c4c
--- /dev/null
+++ b/ArmVirtPkg/Library/DebugLibFdtPL011Uart/Runtime.c
@@ -0,0 +1,88 @@
+/** @file
+ Permanently disable the library instance in DXE_RUNTIME_DRIVER modules when
+ exiting boot services.
+
+ Copyright (C) Red Hat
+ Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.<BR>
+ Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#include <Uefi/UefiSpec.h>
+
+#include "Ram.h"
+
+STATIC EFI_EVENT mExitBootServicesEvent;
+
+/**
+ Notification function that is triggered when the boot service
+ ExitBootServices() is called.
+
+ @param[in] Event Event whose notification function is being invoked. Here,
+ unused.
+
+ @param[in] Context The pointer to the notification function's context, which
+ is implementation-dependent. Here, unused.
+**/
+STATIC
+VOID
+EFIAPI
+ExitBootServicesNotify (
+ IN EFI_EVENT Event,
+ IN VOID *Context
+ )
+{
+ mDebugLibFdtPL011UartAddress = 0;
+ mDebugLibFdtPL011UartPermanentStatus = RETURN_ABORTED;
+}
+
+/**
+ Library instance constructor, registering ExitBootServicesNotify().
+
+ @param[in] ImageHandle The firmware-allocated handle for the EFI image.
+
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS The operation completed successfully.
+
+ @return Error codes propagated from CreateEvent(); the
+ registration of ExitBootServicesNotify() failed.
+**/
+EFI_STATUS
+EFIAPI
+DxeRuntimeDebugLibFdtPL011UartConstructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return SystemTable->BootServices->CreateEvent (
+ EVT_SIGNAL_EXIT_BOOT_SERVICES,
+ TPL_CALLBACK,
+ ExitBootServicesNotify,
+ NULL /* NotifyContext */,
+ &mExitBootServicesEvent
+ );
+}
+
+/**
+ Library instance destructor, deregistering ExitBootServicesNotify().
+
+ @param[in] ImageHandle The firmware-allocated handle for the EFI image.
+
+ @param[in] SystemTable A pointer to the EFI System Table.
+
+ @retval EFI_SUCCESS Library instance tear-down complete.
+
+ @return Error codes propagated from CloseEvent(); the
+ deregistration of ExitBootServicesNotify() failed.
+**/
+EFI_STATUS
+EFIAPI
+DxeRuntimeDebugLibFdtPL011UartDestructor (
+ IN EFI_HANDLE ImageHandle,
+ IN EFI_SYSTEM_TABLE *SystemTable
+ )
+{
+ return SystemTable->BootServices->CloseEvent (mExitBootServicesEvent);
+}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109398): https://edk2.groups.io/g/devel/message/109398
Mute This Topic: https://groups.io/mt/101834884/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* [edk2-devel] [PATCH 9/9] ArmVirtPkg: steer DebugLib output away from SerialPortLib+console traffic
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (7 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 8/9] ArmVirtPkg: introduce DebugLibFdtPL011Uart DXE Runtime instance Laszlo Ersek
@ 2023-10-08 15:39 ` Laszlo Ersek
2023-10-10 7:43 ` [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Ard Biesheuvel
9 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-08 15:39 UTC (permalink / raw)
To: devel
Cc: Ard Biesheuvel, Gerd Hoffmann, Julien Grall, Leif Lindholm,
Sami Mujawar
For the RELEASE target, all ArmVirtPkg DSCs inherit BaseDebugLibNull from
"ArmVirt.dsc.inc"; keep that.
For NOOPT and DEBUG:
- switch the lib class resolution pair (BaseDebugLibSerialPort +
FdtPL011SerialPortLib) that is set as the default for all module types
in "ArmVirt.dsc.inc" to DebugLibFdtPL011UartRam;
- switch the lib class resolution pair (BaseDebugLibSerialPort +
EarlyFdtPL011SerialPortLib) that is set as an override for SEC,
PEI_CORE, PEIM modules in "ArmVirt.dsc.inc" to
DebugLibFdtPL011UartFlash;
- switch the lib class resolution pair (DxeRuntimeDebugLibSerialPort +
FdtPL011SerialPortLib) that is set as an override for DXE_RUNTIME_DRIVER
modules in "ArmVirt.dsc.inc" to DxeRuntimeDebugLibFdtPL011Uart;
- mask all of the above DebugLib class resolution changes in
"ArmVirtKvmTool.dsc", because "ArmVirtKvmTool.dsc" uses
BaseSerialPortLib16550 rather than PL011 UARTs,
- mask all of the above DebugLib class resolution changes in
"ArmVirtXen.dsc" too, because "ArmVirtXen.dsc" uses
XenConsoleSerialPortLib rather than PL011 UARTs.
I regression-tested this change for "ArmVirtKvmTool.dsc" and
"ArmVirtXen.dsc" by building them for both DEBUG and RELEASE, both before
the patch and after, and comparing the edk2 build report files (focusing
on lib class resolutions). There are no changes.
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: Sami Mujawar <sami.mujawar@arm.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
ArmVirtPkg/ArmVirt.dsc.inc | 13 +++++++++++--
ArmVirtPkg/ArmVirtKvmTool.dsc | 11 +++++++++++
ArmVirtPkg/ArmVirtXen.dsc | 11 +++++++++++
3 files changed, 33 insertions(+), 2 deletions(-)
diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index 0d7115525497..4ed86b979e1a 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -37,7 +37,7 @@ [LibraryClasses.common]
!if $(TARGET) == RELEASE
DebugLib|MdePkg/Library/BaseDebugLibNull/BaseDebugLibNull.inf
!else
- DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+ DebugLib|ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartRam.inf
!endif
DebugPrintErrorLevelLib|MdePkg/Library/BaseDebugPrintErrorLevelLib/BaseDebugPrintErrorLevelLib.inf
@@ -189,6 +189,9 @@ [LibraryClasses.common.SEC]
PeiServicesLib|MdePkg/Library/PeiServicesLib/PeiServicesLib.inf
PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
MemoryAllocationLib|MdePkg/Library/PeiMemoryAllocationLib/PeiMemoryAllocationLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
+!endif
[LibraryClasses.common.PEI_CORE]
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -204,6 +207,9 @@ [LibraryClasses.common.PEI_CORE]
PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
+!endif
[LibraryClasses.common.PEIM]
PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf
@@ -219,6 +225,9 @@ [LibraryClasses.common.PEIM]
PeiServicesTablePointerLib|ArmPkg/Library/PeiServicesTablePointerLib/PeiServicesTablePointerLib.inf
SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|ArmVirtPkg/Library/DebugLibFdtPL011Uart/DebugLibFdtPL011UartFlash.inf
+!endif
[LibraryClasses.common.DXE_CORE]
HobLib|MdePkg/Library/DxeCoreHobLib/DxeCoreHobLib.inf
@@ -246,7 +255,7 @@ [LibraryClasses.common.DXE_RUNTIME_DRIVER]
MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
!if $(TARGET) != RELEASE
- DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+ DebugLib|ArmVirtPkg/Library/DebugLibFdtPL011Uart/DxeRuntimeDebugLibFdtPL011Uart.inf
!endif
VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolicyLibRuntimeDxe.inf
diff --git a/ArmVirtPkg/ArmVirtKvmTool.dsc b/ArmVirtPkg/ArmVirtKvmTool.dsc
index 00b6c64d1c16..f50d53bf158e 100644
--- a/ArmVirtPkg/ArmVirtKvmTool.dsc
+++ b/ArmVirtPkg/ArmVirtKvmTool.dsc
@@ -77,6 +77,9 @@ [LibraryClasses.common]
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/Fdt16550SerialPortHookLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+!endif
HwInfoParserLib|DynamicTablesPkg/Library/FdtHwInfoParserLib/FdtHwInfoParserLib.inf
DynamicPlatRepoLib|DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
@@ -88,6 +91,14 @@ [LibraryClasses.common.SEC, LibraryClasses.common.PEI_CORE, LibraryClasses.commo
PciExpressLib|MdePkg/Library/BasePciExpressLib/BasePciExpressLib.inf
PlatformHookLib|ArmVirtPkg/Library/Fdt16550SerialPortHookLib/EarlyFdt16550SerialPortHookLib.inf
SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+!endif
+
+[LibraryClasses.common.DXE_RUNTIME_DRIVER]
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
[LibraryClasses.common.UEFI_DRIVER]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
diff --git a/ArmVirtPkg/ArmVirtXen.dsc b/ArmVirtPkg/ArmVirtXen.dsc
index f44c1857c961..f0d15b823b9f 100644
--- a/ArmVirtPkg/ArmVirtXen.dsc
+++ b/ArmVirtPkg/ArmVirtXen.dsc
@@ -29,6 +29,9 @@ [Defines]
[LibraryClasses]
SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+!endif
RealTimeClockLib|OvmfPkg/Library/XenRealTimeClockLib/XenRealTimeClockLib.inf
XenHypercallLib|OvmfPkg/Library/XenHypercallLib/XenHypercallLib.inf
@@ -52,6 +55,11 @@ [LibraryClasses]
TpmMeasurementLib|MdeModulePkg/Library/TpmMeasurementLibNull/TpmMeasurementLibNull.inf
TpmPlatformHierarchyLib|SecurityPkg/Library/PeiDxeTpmPlatformHierarchyLibNull/PeiDxeTpmPlatformHierarchyLib.inf
+[LibraryClasses.common.DXE_RUNTIME_DRIVER]
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
+!endif
+
[LibraryClasses.common.UEFI_DRIVER]
UefiScsiLib|MdePkg/Library/UefiScsiLib/UefiScsiLib.inf
@@ -147,6 +155,9 @@ [Components.common]
PrePiHobListPointerLib|ArmPlatformPkg/Library/PrePiHobListPointerLib/PrePiHobListPointerLib.inf
MemoryAllocationLib|EmbeddedPkg/Library/PrePiMemoryAllocationLib/PrePiMemoryAllocationLib.inf
SerialPortLib|OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.inf
+!if $(TARGET) != RELEASE
+ DebugLib|MdePkg/Library/BaseDebugLibSerialPort/BaseDebugLibSerialPort.inf
+!endif
}
#
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#109397): https://edk2.groups.io/g/devel/message/109397
Mute This Topic: https://groups.io/mt/101834883/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-08 15:39 [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Laszlo Ersek
` (8 preceding siblings ...)
2023-10-08 15:39 ` [edk2-devel] [PATCH 9/9] ArmVirtPkg: steer DebugLib output away from SerialPortLib+console traffic Laszlo Ersek
@ 2023-10-10 7:43 ` Ard Biesheuvel
2023-10-10 15:33 ` Laszlo Ersek
9 siblings, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-10-10 7:43 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, Ard Biesheuvel, Gerd Hoffmann, Julien Grall, Leif Lindholm,
Peter Maydell, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-10 7:43 ` [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs Ard Biesheuvel
@ 2023-10-10 15:33 ` Laszlo Ersek
2023-10-26 14:21 ` Peter Maydell
0 siblings, 1 reply; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-10 15:33 UTC (permalink / raw)
To: devel, ardb
Cc: Ard Biesheuvel, Gerd Hoffmann, Julien Grall, Leif Lindholm,
Peter Maydell, Sami Mujawar
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]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-10 15:33 ` Laszlo Ersek
@ 2023-10-26 14:21 ` Peter Maydell
2023-10-26 14:46 ` Julien Grall
2023-10-26 15:19 ` Laszlo Ersek
0 siblings, 2 replies; 21+ messages in thread
From: Peter Maydell @ 2023-10-26 14:21 UTC (permalink / raw)
To: Laszlo Ersek
Cc: devel, ardb, Ard Biesheuvel, Gerd Hoffmann, Julien Grall,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 14:21 ` Peter Maydell
@ 2023-10-26 14:46 ` Julien Grall
2023-10-26 14:55 ` Ard Biesheuvel
2023-10-26 15:30 ` Laszlo Ersek
2023-10-26 15:19 ` Laszlo Ersek
1 sibling, 2 replies; 21+ messages in thread
From: Julien Grall @ 2023-10-26 14:46 UTC (permalink / raw)
To: Peter Maydell, Laszlo Ersek
Cc: devel, ardb, Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm,
Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 14:46 ` Julien Grall
@ 2023-10-26 14:55 ` Ard Biesheuvel
2023-10-26 15:36 ` Laszlo Ersek
2023-10-26 15:30 ` Laszlo Ersek
1 sibling, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-10-26 14:55 UTC (permalink / raw)
To: Julien Grall
Cc: Peter Maydell, Laszlo Ersek, devel, Ard Biesheuvel, Gerd Hoffmann,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 14:55 ` Ard Biesheuvel
@ 2023-10-26 15:36 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-26 15:36 UTC (permalink / raw)
To: Ard Biesheuvel, Julien Grall
Cc: Peter Maydell, devel, Ard Biesheuvel, Gerd Hoffmann,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 14:46 ` Julien Grall
2023-10-26 14:55 ` Ard Biesheuvel
@ 2023-10-26 15:30 ` Laszlo Ersek
1 sibling, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-26 15:30 UTC (permalink / raw)
To: Julien Grall, Peter Maydell
Cc: devel, ardb, Ard Biesheuvel, Gerd Hoffmann, Leif Lindholm,
Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 14:21 ` Peter Maydell
2023-10-26 14:46 ` Julien Grall
@ 2023-10-26 15:19 ` Laszlo Ersek
2023-10-26 15:21 ` Ard Biesheuvel
2023-10-27 10:57 ` Gerd Hoffmann
1 sibling, 2 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-26 15:19 UTC (permalink / raw)
To: Peter Maydell
Cc: devel, ardb, Ard Biesheuvel, Gerd Hoffmann, Julien Grall,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 15:19 ` Laszlo Ersek
@ 2023-10-26 15:21 ` Ard Biesheuvel
2023-10-26 19:00 ` Laszlo Ersek
2023-10-27 10:57 ` Gerd Hoffmann
1 sibling, 1 reply; 21+ messages in thread
From: Ard Biesheuvel @ 2023-10-26 15:21 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Peter Maydell, devel, Ard Biesheuvel, Gerd Hoffmann, Julien Grall,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 15:21 ` Ard Biesheuvel
@ 2023-10-26 19:00 ` Laszlo Ersek
0 siblings, 0 replies; 21+ messages in thread
From: Laszlo Ersek @ 2023-10-26 19:00 UTC (permalink / raw)
To: devel, ardb
Cc: Peter Maydell, Ard Biesheuvel, Gerd Hoffmann, Julien Grall,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [edk2-devel] [PATCH 0/9] ArmVirtPkg: support two PL011 UARTs
2023-10-26 15:19 ` Laszlo Ersek
2023-10-26 15:21 ` Ard Biesheuvel
@ 2023-10-27 10:57 ` Gerd Hoffmann
1 sibling, 0 replies; 21+ messages in thread
From: Gerd Hoffmann @ 2023-10-27 10:57 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Peter Maydell, devel, ardb, Ard Biesheuvel, Julien Grall,
Leif Lindholm, Sami Mujawar
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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 21+ messages in thread