[RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

Serial port output is useful when debugging UEFI runtime services in OS runtime.
The patches are trying to provide a handy method to enable runtime serial port
debug for ArmVirtQemu.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Julien Grall <julien.grall@arm.com>

Heyi Guo (3):
  MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
  ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
  ArmVirtQemu: enable runtime debug by build flag

 ArmVirtPkg/ArmVirt.dsc.inc                                                          |   6 ++
 ArmVirtPkg/ArmVirtQemu.dsc                                                          |  13 +++
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                |   5 +
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                    |  29 ++++--
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h                    |  27 +++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf                  |   3 +
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c              |  27 +++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c             | 104 ++++++++++++++++++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf           |  58 +++++++++++
 MdeModulePkg/MdeModulePkg.dec                                                       |   6 ++
 MdeModulePkg/MdeModulePkg.uni                                                       |   6 ++
 MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   |   2 +-
 MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf |   7 +-
 13 files changed, 279 insertions(+), 14 deletions(-)
 create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
 create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
 create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
 create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf

-- 
1.8.3.1

[RFC 1/3] MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug

[Heyi Guo]

Serial port message is useful when we are debugging UEFI runtime
services, so add a feature PCD to enable runtime serial debug.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao Wu <hao.a.wu@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 MdeModulePkg/MdeModulePkg.dec                                                       | 6 ++++++
 MdeModulePkg/MdeModulePkg.uni                                                       | 6 ++++++
 MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   | 2 +-
 MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf | 7 ++++---
 4 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2130bc..19953cc 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -777,6 +777,12 @@
   # @Prompt Enable StatusCode via Serial port.
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE|BOOLEAN|0x00010022
 
+  ## Indicates if StatusCode is reported via Serial port at runtime; mainly for debug purpose<BR><BR>
+  #   TRUE  - Reports StatusCode via Serial port at runtime.<BR>
+  #   FALSE - Does not report StatusCode via Serial port at runtime.<BR>
+  # @Prompt Enable StatusCode via Serial port at runtime
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerialRuntime|FALSE|BOOLEAN|0x00010024
+
   ## Indicates if StatusCode is stored in memory.
   #  The memory is boot time memory in PEI Phase and is runtime memory in DXE Phase.<BR><BR>
   #   TRUE  - Stores StatusCode in memory.<BR>
diff --git a/MdeModulePkg/MdeModulePkg.uni b/MdeModulePkg/MdeModulePkg.uni
index 787fdf2..122fc44 100644
--- a/MdeModulePkg/MdeModulePkg.uni
+++ b/MdeModulePkg/MdeModulePkg.uni
@@ -759,6 +759,12 @@
                                                                                         "TRUE  - Reports StatusCode via Serial port.<BR>\n"
                                                                                         "FALSE - Does not report StatusCode via Serial port.<BR>"
 
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdStatusCodeUseSerialRuntime_PROMPT  #language en-US "Enable StatusCode via Serial port at runtime"
+
+#string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdStatusCodeUseSerialRuntime_HELP  #language en-US "Indicates if StatusCode is reported via Serial port at runtime.<BR><BR>\n"
+                                                                                        "TRUE  - Reports StatusCode via Serial port at runtime.<BR>\n"
+                                                                                        "FALSE - Does not report StatusCode via Serial port at runtime.<BR>"
+
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdStatusCodeUseMemory_PROMPT  #language en-US "Enable StatusCode via memory"
 
 #string STR_gEfiMdeModulePkgTokenSpaceGuid_PcdStatusCodeUseMemory_HELP  #language en-US "Indicates if StatusCode is stored in memory. The memory is boot time memory in PEI Phase and is runtime memory in DXE Phase.<BR><BR>\n"
diff --git a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
index e47e083..2142cfc 100644
--- a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
+++ b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c
@@ -35,7 +35,7 @@ UnregisterBootTimeHandlers (
   IN VOID             *Context
   )
 {
-  if (FeaturePcdGet (PcdStatusCodeUseSerial)) {
+  if (FeaturePcdGet (PcdStatusCodeUseSerial) && !FeaturePcdGet (PcdStatusCodeUseSerialRuntime)) {
     mRscHandlerProtocol->Unregister (SerialStatusCodeReportWorker);
   }
 }
diff --git a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
index a0a37a4..cf25b22 100644
--- a/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
+++ b/MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
@@ -64,9 +64,10 @@
   gEfiRscHandlerProtocolGuid                    ## CONSUMES
 
 [FeaturePcd]
-  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeReplayIn  ## CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory ## CONSUMES
-  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeReplayIn         ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory        ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial        ## CONSUMES
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerialRuntime ## CONSUMES
 
 [Pcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeMemorySize |128| gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory   ## SOMETIMES_CONSUMES
-- 
1.8.3.1

[RFC 2/3] ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib

[Heyi Guo]

We add a runtime instance inf into the same directory for code reuse.
The different pieces of code are put into *Common.c and *Runtime.c.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Julien Grall <julien.grall@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c          |  29 ++++--
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h          |  27 +++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf        |   3 +
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c    |  27 +++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c   | 104 ++++++++++++++++++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf |  58 +++++++++++
 6 files changed, 238 insertions(+), 10 deletions(-)

diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
index 2f10fb7..51e1909 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c
@@ -3,9 +3,10 @@
 
   Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
   Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
-  Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR>
   Copyright (c) 2014, Red Hat, Inc.<BR>
   Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
 
   This program and the accompanying materials
   are licensed and made available under the terms and conditions of the BSD License
@@ -28,17 +29,10 @@
 #include <Pi/PiHob.h>
 #include <Library/HobLib.h>
 #include <Guid/EarlyPL011BaseAddress.h>
+#include "FdtPL011SerialPortLib.h"
 
-STATIC UINTN mSerialBaseAddress;
 
-RETURN_STATUS
-EFIAPI
-SerialPortInitialize (
-  VOID
-  )
-{
-  return RETURN_SUCCESS;
-}
+UINTN mSerialBaseAddress;
 
 /**
 
@@ -90,6 +84,21 @@ FdtPL011SerialPortLibInitialize (
            );
 }
 
+RETURN_STATUS
+EFIAPI
+SerialPortInitialize (
+  VOID
+  )
+{
+  if (mSerialBaseAddress == 0) {
+    FdtPL011SerialPortLibInitialize ();
+  }
+
+  (VOID) SerialPortRuntimePrepare ();
+
+  return RETURN_SUCCESS;
+}
+
 /**
   Write data to serial device.
 
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
new file mode 100644
index 0000000..25989ef
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
@@ -0,0 +1,27 @@
+/** @file
+
+  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2014, Red Hat, Inc.<BR>
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#ifndef _FDT_PL011_SERIAL_PORT_LIB_H_
+#define _FDT_PL011_SERIAL_PORT_LIB_H_
+
+extern UINTN mSerialBaseAddress;
+
+RETURN_STATUS SerialPortRuntimePrepare (VOID);
+
+#endif
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
index 0b06797..050c965 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf
@@ -3,6 +3,8 @@
 #  Component description file for PL011SerialPortLib module
 #
 #  Copyright (c) 2011-2015, ARM Ltd. All rights reserved.<BR>
+#  Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR>
+#  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
 #
 #  This program and the accompanying materials
 #  are licensed and made available under the terms and conditions of the BSD License
@@ -25,6 +27,7 @@
 
 [Sources.common]
   FdtPL011SerialPortLib.c
+  FdtPL011SerialPortLibCommon.c
 
 [LibraryClasses]
   PL011UartLib
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
new file mode 100644
index 0000000..68fa6a2
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
@@ -0,0 +1,27 @@
+/** @file
+  Serial I/O Port library functions with base address discovered from FDT
+
+  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2014, Red Hat, Inc.<BR>
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Base.h>
+#include "FdtPL011SerialPortLib.h"
+
+RETURN_STATUS SerialPortRuntimePrepare (VOID)
+{
+  return RETURN_SUCCESS;
+}
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
new file mode 100644
index 0000000..6ea1d02
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
@@ -0,0 +1,104 @@
+/** @file
+  Serial I/O Port library functions with base address discovered from FDT
+
+  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2012 - 2013, ARM Ltd. All rights reserved.<BR>
+  Copyright (c) 2014 - 2019, Linaro Ltd. All rights reserved.<BR>
+  Copyright (c) 2014, Red Hat, Inc.<BR>
+  Copyright (c) 2015, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
+
+  This program and the accompanying materials
+  are licensed and made available under the terms and conditions of the BSD License
+  which accompanies this distribution.  The full text of the license may be found at
+  http://opensource.org/licenses/bsd-license.php
+
+  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+**/
+
+#include <Uefi.h>
+#include <Guid/EventGroup.h>
+#include <Library/DxeServicesTableLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/UefiRuntimeLib.h>
+#include "FdtPL011SerialPortLib.h"
+
+STATIC EFI_EVENT mSerialVirtualAddressChangeEvent = NULL;
+
+RETURN_STATUS
+EFIAPI
+SerialPortLibDestructor (
+    IN EFI_HANDLE        ImageHandle,
+    IN EFI_SYSTEM_TABLE  *SystemTable
+    )
+{
+
+  if (!mSerialVirtualAddressChangeEvent) {
+    return RETURN_SUCCESS;
+  }
+
+  return gBS->CloseEvent (mSerialVirtualAddressChangeEvent);
+}
+
+VOID
+EFIAPI
+SerialVirtualAddressChangeCallBack (
+  IN EFI_EVENT        Event,
+  IN VOID             *Context
+  )
+{
+  EfiConvertPointer (0, (VOID **) &mSerialBaseAddress);
+}
+
+
+RETURN_STATUS SerialPortRuntimePrepare (VOID)
+{
+  RETURN_STATUS                   Status;
+  UINT64                          Length = SIZE_4KB;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR Desc = {0};
+
+  // For the serial port register space length is only 4KB, we don't need to
+  // check if the descriptor is large enough to cover the space.
+  Status = gDS->GetMemorySpaceDescriptor(mSerialBaseAddress, &Desc);
+  if (EFI_ERROR (Status) ||
+      Desc.GcdMemoryType == EfiGcdMemoryTypeNonExistent) {
+    Status = gDS->AddMemorySpace (
+        EfiGcdMemoryTypeMemoryMappedIo,
+        mSerialBaseAddress,
+        Length,
+        EFI_MEMORY_UC | EFI_MEMORY_RUNTIME
+    );
+    if (EFI_ERROR (Status)) {
+      return Status;
+    }
+    Desc.Attributes = EFI_MEMORY_UC;
+  }
+
+  if ((Desc.Attributes & EFI_MEMORY_RUNTIME) == 0) {
+    Desc.Attributes |= EFI_MEMORY_RUNTIME;
+    Status = gDS->SetMemorySpaceAttributes (
+        mSerialBaseAddress,
+        Length,
+        Desc.Attributes
+        );
+    if(EFI_ERROR (Status)){
+      return Status;
+    }
+  }
+
+  Status = gBS->CreateEventEx (
+      EVT_NOTIFY_SIGNAL,
+      TPL_NOTIFY,
+      SerialVirtualAddressChangeCallBack,
+      NULL,
+      &gEfiEventVirtualAddressChangeGuid,
+      &mSerialVirtualAddressChangeEvent
+      );
+  if (EFI_ERROR (Status)) {
+    mSerialVirtualAddressChangeEvent = NULL;
+  }
+
+  return Status;
+}
diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
new file mode 100644
index 0000000..a3f87bd
--- /dev/null
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
@@ -0,0 +1,58 @@
+#/** @file
+#
+#  Component description file for PL011SerialPortLib module
+#
+#  Copyright (c) 2011-2015, ARM Ltd. All rights reserved.<BR>
+#  Copyright (c) 2019, Linaro Ltd. All rights reserved.<BR>
+#  Copyright (c) 2019, Huawei Technologies Co., Ltd. All rights reserved.<BR>
+#
+#  This program and the accompanying materials
+#  are licensed and made available under the terms and conditions of the BSD License
+#  which accompanies this distribution.  The full text of the license may be found at
+#  http://opensource.org/licenses/bsd-license.php
+#
+#  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = FdtPL011SerialPortLibRuntime
+  FILE_GUID                      = 83afbb90-38c6-11e9-aeab-203db202c950
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = SerialPortLib|DXE_RUNTIME_DRIVER
+  DESTRUCTOR                     = SerialPortLibDestructor
+
+[Sources.common]
+  FdtPL011SerialPortLib.c
+  FdtPL011SerialPortLibRuntime.c
+
+[LibraryClasses]
+  DxeServicesTableLib
+  HobLib
+  PL011UartLib
+  UefiBootServicesTableLib
+  UefiRuntimeLib
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  ArmPlatformPkg/ArmPlatformPkg.dec
+  ArmVirtPkg/ArmVirtPkg.dec
+  ArmPkg/ArmPkg.dec
+
+[FixedPcd]
+  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate
+  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits
+  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity
+  gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits
+  gArmPlatformTokenSpaceGuid.PL011UartClkInHz
+
+[Guids]
+  gEarlyPL011BaseAddressGuid
+
+[Depex]
+  gEfiCpuArchProtocolGuid
-- 
1.8.3.1

[RFC 3/3] ArmVirtQemu: enable runtime debug by build flag

[Heyi Guo]

Introduce a build definition "RT_DEBUG" to enable runtime debug, so
that we can have a handy method for UEFI runtime services debug.

This build flag only applies to DEBUG build; the reason for not
enabling it for DEBUG build by default is the related modules have
some dependencies, so "DEBUG(())" won't print any anything until the
status code handler driver has run.

User needs to specify "-D RT_DEBUG=TRUE"; "-D RT_DEBUG" is not enough
for we use !if $(RT_DEBUG)==TRUE in the code.

Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Julien Grall <julien.grall@arm.com>
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 ArmVirtPkg/ArmVirt.dsc.inc           |  6 ++++++
 ArmVirtPkg/ArmVirtQemu.dsc           | 13 +++++++++++++
 ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc |  5 +++++
 3 files changed, 24 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index d172a08..26984d6 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -240,8 +240,14 @@
   MemoryAllocationLib|MdePkg/Library/UefiMemoryAllocationLib/UefiMemoryAllocationLib.inf
   CapsuleLib|MdeModulePkg/Library/DxeCapsuleLibNull/DxeCapsuleLibNull.inf
 !if $(TARGET) != RELEASE
+!if $(RT_DEBUG) == TRUE
+  SerialPortLib|ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
+  ReportStatusCodeLib|MdeModulePkg/Library/RuntimeDxeReportStatusCodeLib/RuntimeDxeReportStatusCodeLib.inf
+  DebugLib|MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
+!else
   DebugLib|MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
 !endif
+!endif
 
 !if $(SECURE_BOOT_ENABLE) == TRUE
   BaseCryptLib|CryptoPkg/Library/BaseCryptLib/RuntimeCryptLib.inf
diff --git a/ArmVirtPkg/ArmVirtQemu.dsc b/ArmVirtPkg/ArmVirtQemu.dsc
index d703317..6309903 100644
--- a/ArmVirtPkg/ArmVirtQemu.dsc
+++ b/ArmVirtPkg/ArmVirtQemu.dsc
@@ -37,6 +37,7 @@
   DEFINE SECURE_BOOT_ENABLE      = FALSE
   DEFINE NETWORK_IP6_ENABLE      = FALSE
   DEFINE HTTP_BOOT_ENABLE        = FALSE
+  DEFINE RT_DEBUG                = FALSE
 
 !include ArmVirtPkg/ArmVirt.dsc.inc
 
@@ -93,6 +94,10 @@
 
   gEfiMdeModulePkgTokenSpaceGuid.PcdTurnOffUsbLegacySupport|TRUE
 
+!if $(TARGET) != RELEASE && $(RT_DEBUG) == TRUE
+  gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerialRuntime|TRUE
+!endif
+
 [PcdsFixedAtBuild.common]
 !if $(ARCH) == AARCH64
   gArmTokenSpaceGuid.PcdVFPEnabled|1
@@ -159,7 +164,11 @@
   gEfiSecurityPkgTokenSpaceGuid.PcdRemovableMediaImageVerificationPolicy|0x04
 !endif
 
+!if $(TARGET) != RELEASE && $(RT_DEBUG) == TRUE
+  gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|7
+!else
   gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|3
+!endif
 
 [PcdsFixedAtBuild.AARCH64]
   # Clearing BIT0 in this PCD prevents installing a 32-bit SMBIOS entry point,
@@ -263,6 +272,10 @@
       PcdLib|MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf
   }
 
+!if $(TARGET) != RELEASE && $(RT_DEBUG) == TRUE
+  MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
+!endif
+
   #
   # Architectural Protocols
   #
diff --git a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
index 098d40b..bf86e61 100644
--- a/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
+++ b/ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc
@@ -46,6 +46,11 @@ READ_LOCK_STATUS   = TRUE
 
   INF MdeModulePkg/Core/Dxe/DxeMain.inf
   INF MdeModulePkg/Universal/PCD/Dxe/Pcd.inf
+
+!if $(TARGET) != RELEASE && $(RT_DEBUG) == TRUE
+  INF MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf
+!endif
+
   INF ArmVirtPkg/VirtioFdtDxe/VirtioFdtDxe.inf
   INF ArmVirtPkg/FdtClientDxe/FdtClientDxe.inf
   INF ArmVirtPkg/HighMemDxe/HighMemDxe.inf
-- 
1.8.3.1

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Ard Biesheuvel]

On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>
> Serial port output is useful when debugging UEFI runtime services in OS runtime.
> The patches are trying to provide a handy method to enable runtime serial port
> debug for ArmVirtQemu.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Julien Grall <julien.grall@arm.com>
>
> Heyi Guo (3):
>   MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>   ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>   ArmVirtQemu: enable runtime debug by build flag
>

Hello Heyi,

We have had this discussion before, and IIRC, the proposed solution
was to use status codes.

I'm not sure how that is supposed to work though - hopefully Laszlo or
one of the MdeModulePkg maintainers can elaborate.



>  ArmVirtPkg/ArmVirt.dsc.inc                                                          |   6 ++
>  ArmVirtPkg/ArmVirtQemu.dsc                                                          |  13 +++
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                |   5 +
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                    |  29 ++++--
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h                    |  27 +++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf                  |   3 +
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c              |  27 +++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c             | 104 ++++++++++++++++++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf           |  58 +++++++++++
>  MdeModulePkg/MdeModulePkg.dec                                                       |   6 ++
>  MdeModulePkg/MdeModulePkg.uni                                                       |   6 ++
>  MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   |   2 +-
>  MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf |   7 +-
>  13 files changed, 279 insertions(+), 14 deletions(-)
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
>
> --
> 1.8.3.1
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

+Peter, for the last few paragraphs

On 02/28/19 13:10, Ard Biesheuvel wrote:
> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>
>> Serial port output is useful when debugging UEFI runtime services in OS runtime.
>> The patches are trying to provide a handy method to enable runtime serial port
>> debug for ArmVirtQemu.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>>
>> Heyi Guo (3):
>>   MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>   ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>   ArmVirtQemu: enable runtime debug by build flag
>>
> 
> Hello Heyi,
> 
> We have had this discussion before, and IIRC, the proposed solution
> was to use status codes.
> 
> I'm not sure how that is supposed to work though - hopefully Laszlo or
> one of the MdeModulePkg maintainers can elaborate.

Here's the basic idea.

Status Code reporting and routing are defined by the PI spec for OS
runtime as well, not just boot time.

Recently we added status code *routing* to ArmVirtPkg, in commit
5c574b222ec2, via the generic runtime driver
"ReportStatusCodeRouterRuntimeDxe". We also added the library resolution

ReportStatusCodeLib -->
MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf

(for some module types). As a result, when modules of those types report
status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
driver, which "routes" the status codes.

In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
(built into BdsDxe) to register a status code *handler* callback with
ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
runtime), and only for a very specific group of status codes. As a
result, when a module of suitable type reports a status code,
ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
"handles it" (in our implementation, we simply format it to the UEFI
console), assuming the status code is the kind we are interested in.


Now we come to the current series. First, the series adds the following
DebugLib class resolution:

DebugLib -->
MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf

This will cause modules to publish their DEBUG messages as status codes
via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
too lazy to check the exact status code classes etc that
PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
"routing". As another result, until we reach a late enough stage in the
boot, those messages will not be printed by anything (because there's
not going to be any "handling" for them).

(The present series also updates the ReportStatusCodeLib resolution so
it can be used at runtime too, but that's quite auxiliary to this
discussion.)

Second, this series includes the generic Status Code *handling* driver
(also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
the particular handling that we already have in BdsDxe via the earlier
series, this generic status handler driver registers a handler callback
that simply prints status codes to the serial port (dependent on a PCD
setting), via SerialPortLib.

With the modification from the first patch, this generic status code
*handler* driver is extended to runtime serial port operation. And, the
second patch in the series provides a SerialPortLib instance for it that
can work at runtime.

All in all, when a runtime driver calls DEBUG, this happens:

  runtime driver calls DEBUG
  -> PeiDxeDebugLibReportStatusCode
    -> RuntimeDxeReportStatusCodeLib
       [status code reporting]

=> ReportStatusCodeRouterRuntimeDxe
   [status code routing]

=> StatusCodeHandlerRuntimeDxe
   [status code handling, such as SerialPortWrite():]
  -> FdtPL011SerialPortLibRuntime

This is actually a good idea, but it would be nice if:
- QEMU had two PL011 ports,
- the boot time firmware log, and the OS log, went to one port
- the runtme firmware log went to the other port.

Given that this series provides the SerialPortLib instance for
StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
for runtime firmware logging.

I don't insist that this series implement all of that, but it should
either prevent a conflict on the one PL011 between the firmware and the
OS, or else be very explicit about the possible conflict in the commit
messages.

Thanks
Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

On 2019/3/1 23:27, Laszlo Ersek wrote:
> +Peter, for the last few paragraphs
>
> On 02/28/19 13:10, Ard Biesheuvel wrote:
>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>> Serial port output is useful when debugging UEFI runtime services in OS runtime.
>>> The patches are trying to provide a handy method to enable runtime serial port
>>> debug for ArmVirtQemu.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>>
>>> Heyi Guo (3):
>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>    ArmVirtQemu: enable runtime debug by build flag
>>>
>> Hello Heyi,
>>
>> We have had this discussion before, and IIRC, the proposed solution
>> was to use status codes.
>>
>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>> one of the MdeModulePkg maintainers can elaborate.
> Here's the basic idea.
>
> Status Code reporting and routing are defined by the PI spec for OS
> runtime as well, not just boot time.
>
> Recently we added status code *routing* to ArmVirtPkg, in commit
> 5c574b222ec2, via the generic runtime driver
> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>
> ReportStatusCodeLib -->
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> (for some module types). As a result, when modules of those types report
> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
> driver, which "routes" the status codes.
>
> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
> (built into BdsDxe) to register a status code *handler* callback with
> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
> runtime), and only for a very specific group of status codes. As a
> result, when a module of suitable type reports a status code,
> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
> "handles it" (in our implementation, we simply format it to the UEFI
> console), assuming the status code is the kind we are interested in.
>
>
> Now we come to the current series. First, the series adds the following
> DebugLib class resolution:
>
> DebugLib -->
> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>
> This will cause modules to publish their DEBUG messages as status codes
> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
> too lazy to check the exact status code classes etc that
> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
> "routing". As another result, until we reach a late enough stage in the
> boot, those messages will not be printed by anything (because there's
> not going to be any "handling" for them).
>
> (The present series also updates the ReportStatusCodeLib resolution so
> it can be used at runtime too, but that's quite auxiliary to this
> discussion.)
>
> Second, this series includes the generic Status Code *handling* driver
> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
> the particular handling that we already have in BdsDxe via the earlier
> series, this generic status handler driver registers a handler callback
> that simply prints status codes to the serial port (dependent on a PCD
> setting), via SerialPortLib.
>
> With the modification from the first patch, this generic status code
> *handler* driver is extended to runtime serial port operation. And, the
> second patch in the series provides a SerialPortLib instance for it that
> can work at runtime.
>
> All in all, when a runtime driver calls DEBUG, this happens:
>
>    runtime driver calls DEBUG
>    -> PeiDxeDebugLibReportStatusCode
>      -> RuntimeDxeReportStatusCodeLib
>         [status code reporting]
>
> => ReportStatusCodeRouterRuntimeDxe
>     [status code routing]
>
> => StatusCodeHandlerRuntimeDxe
>     [status code handling, such as SerialPortWrite():]
>    -> FdtPL011SerialPortLibRuntime
>
> This is actually a good idea, but it would be nice if:
> - QEMU had two PL011 ports,
> - the boot time firmware log, and the OS log, went to one port
> - the runtme firmware log went to the other port.
This makes sense. I'll try the 2nd serial port.

Thanks,
Heyi

>
> Given that this series provides the SerialPortLib instance for
> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
> for runtime firmware logging.
>
> I don't insist that this series implement all of that, but it should
> either prevent a conflict on the one PL011 between the firmware and the
> OS, or else be very explicit about the possible conflict in the commit
> messages.
>
> Thanks
> Laszlo
>
> .
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

Hi Laszlo,

I'm thinking about a proposal as below:

1. Reuse the framework of MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
2. The boot time behavior of this module is not changed
3. Switch to status code protocol for runtime debug
4. Use an overridden SerialPortLib instance for StatusCodeHandlerRuntimeDxe; the instance must support runtime access.

Then we can have below benefits:
1. the boot time firmware log, and the OS log, went to one port; and there is no dependence for runtime drivers to output serial message at boot time.
2. By implementing different instances of SerialPortLib for StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct runtime serial message to platform specific serial port.

Please comment.

Thanks,
Heyi


On 2019/3/1 23:27, Laszlo Ersek wrote:
> +Peter, for the last few paragraphs
>
> On 02/28/19 13:10, Ard Biesheuvel wrote:
>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>> Serial port output is useful when debugging UEFI runtime services in OS runtime.
>>> The patches are trying to provide a handy method to enable runtime serial port
>>> debug for ArmVirtQemu.
>>>
>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Julien Grall <julien.grall@arm.com>
>>>
>>> Heyi Guo (3):
>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>    ArmVirtQemu: enable runtime debug by build flag
>>>
>> Hello Heyi,
>>
>> We have had this discussion before, and IIRC, the proposed solution
>> was to use status codes.
>>
>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>> one of the MdeModulePkg maintainers can elaborate.
> Here's the basic idea.
>
> Status Code reporting and routing are defined by the PI spec for OS
> runtime as well, not just boot time.
>
> Recently we added status code *routing* to ArmVirtPkg, in commit
> 5c574b222ec2, via the generic runtime driver
> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>
> ReportStatusCodeLib -->
> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>
> (for some module types). As a result, when modules of those types report
> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
> driver, which "routes" the status codes.
>
> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
> (built into BdsDxe) to register a status code *handler* callback with
> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
> runtime), and only for a very specific group of status codes. As a
> result, when a module of suitable type reports a status code,
> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
> "handles it" (in our implementation, we simply format it to the UEFI
> console), assuming the status code is the kind we are interested in.
>
>
> Now we come to the current series. First, the series adds the following
> DebugLib class resolution:
>
> DebugLib -->
> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>
> This will cause modules to publish their DEBUG messages as status codes
> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
> too lazy to check the exact status code classes etc that
> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
> "routing". As another result, until we reach a late enough stage in the
> boot, those messages will not be printed by anything (because there's
> not going to be any "handling" for them).
>
> (The present series also updates the ReportStatusCodeLib resolution so
> it can be used at runtime too, but that's quite auxiliary to this
> discussion.)
>
> Second, this series includes the generic Status Code *handling* driver
> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
> the particular handling that we already have in BdsDxe via the earlier
> series, this generic status handler driver registers a handler callback
> that simply prints status codes to the serial port (dependent on a PCD
> setting), via SerialPortLib.
>
> With the modification from the first patch, this generic status code
> *handler* driver is extended to runtime serial port operation. And, the
> second patch in the series provides a SerialPortLib instance for it that
> can work at runtime.
>
> All in all, when a runtime driver calls DEBUG, this happens:
>
>    runtime driver calls DEBUG
>    -> PeiDxeDebugLibReportStatusCode
>      -> RuntimeDxeReportStatusCodeLib
>         [status code reporting]
>
> => ReportStatusCodeRouterRuntimeDxe
>     [status code routing]
>
> => StatusCodeHandlerRuntimeDxe
>     [status code handling, such as SerialPortWrite():]
>    -> FdtPL011SerialPortLibRuntime
>
> This is actually a good idea, but it would be nice if:
> - QEMU had two PL011 ports,
> - the boot time firmware log, and the OS log, went to one port
> - the runtme firmware log went to the other port.
>
> Given that this series provides the SerialPortLib instance for
> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
> for runtime firmware logging.
>
> I don't insist that this series implement all of that, but it should
> either prevent a conflict on the one PL011 between the firmware and the
> OS, or else be very explicit about the possible conflict in the commit
> messages.
>
> Thanks
> Laszlo
>
> .
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

Hello Heyi,

On 03/12/19 07:56, Heyi Guo wrote:
> Hi Laszlo,
> 
> I'm thinking about a proposal as below:
> 
> 1. Reuse the framework of
> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
> 
> 2. The boot time behavior of this module is not changed
> 3. Switch to status code protocol for runtime debug
> 4. Use an overridden SerialPortLib instance for
> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
> 
> Then we can have below benefits:
> 1. the boot time firmware log, and the OS log, went to one port; and
> there is no dependence for runtime drivers to output serial message at
> boot time.
> 2. By implementing different instances of SerialPortLib for
> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
> runtime serial message to platform specific serial port.

This looks a bit over-engineered to me. Ultimately our goal is:
- for DEBUGs to reach "a" serial port at runtime -- namely one that
differs from the "normal" serial port.

Given that you'd have to implement a "special" SerialPortLib instance
just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:

(1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
stick universally with BaseDebugLibSerialPort, for DebugLib,

(2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
(a) runtime behavior and (b) handling of a special serial port?

In other words, push down the "runtime?" distinction from DebugLib (see
commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
would be used only with DXE_RUNTIME_DRIVERs.

The lib instance would prepare everything in advance for accessing the
"special" serial port at runtime (which could require allocating runtime
memory in advance). Once ExitBootServices() is called, a callback should
switch over the behavior of the lib instance, without allocating further
memory. (The switched-over behavior would not have to be functional
until the virtual address map is set.)

I've always seen status code reporting cumbersome in comparison to plain
DEBUG messages. My understanding is that status code reporting targets
devices that are even dumber than serial ports. But, we do target a
second serial port. So if we can keep the switchover internal to the
SerialPortLib class, at least for runtime DXE drivers, then I think we
should do that.

Thanks,
Laszlo

> On 2019/3/1 23:27, Laszlo Ersek wrote:
>> +Peter, for the last few paragraphs
>>
>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>> Serial port output is useful when debugging UEFI runtime services in
>>>> OS runtime.
>>>> The patches are trying to provide a handy method to enable runtime
>>>> serial port
>>>> debug for ArmVirtQemu.
>>>>
>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>
>>>> Heyi Guo (3):
>>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>    ArmVirtQemu: enable runtime debug by build flag
>>>>
>>> Hello Heyi,
>>>
>>> We have had this discussion before, and IIRC, the proposed solution
>>> was to use status codes.
>>>
>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>> one of the MdeModulePkg maintainers can elaborate.
>> Here's the basic idea.
>>
>> Status Code reporting and routing are defined by the PI spec for OS
>> runtime as well, not just boot time.
>>
>> Recently we added status code *routing* to ArmVirtPkg, in commit
>> 5c574b222ec2, via the generic runtime driver
>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>
>> ReportStatusCodeLib -->
>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>
>> (for some module types). As a result, when modules of those types report
>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>> driver, which "routes" the status codes.
>>
>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>> (built into BdsDxe) to register a status code *handler* callback with
>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>> runtime), and only for a very specific group of status codes. As a
>> result, when a module of suitable type reports a status code,
>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>> "handles it" (in our implementation, we simply format it to the UEFI
>> console), assuming the status code is the kind we are interested in.
>>
>>
>> Now we come to the current series. First, the series adds the following
>> DebugLib class resolution:
>>
>> DebugLib -->
>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>
>>
>> This will cause modules to publish their DEBUG messages as status codes
>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>> too lazy to check the exact status code classes etc that
>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>> "routing". As another result, until we reach a late enough stage in the
>> boot, those messages will not be printed by anything (because there's
>> not going to be any "handling" for them).
>>
>> (The present series also updates the ReportStatusCodeLib resolution so
>> it can be used at runtime too, but that's quite auxiliary to this
>> discussion.)
>>
>> Second, this series includes the generic Status Code *handling* driver
>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>> the particular handling that we already have in BdsDxe via the earlier
>> series, this generic status handler driver registers a handler callback
>> that simply prints status codes to the serial port (dependent on a PCD
>> setting), via SerialPortLib.
>>
>> With the modification from the first patch, this generic status code
>> *handler* driver is extended to runtime serial port operation. And, the
>> second patch in the series provides a SerialPortLib instance for it that
>> can work at runtime.
>>
>> All in all, when a runtime driver calls DEBUG, this happens:
>>
>>    runtime driver calls DEBUG
>>    -> PeiDxeDebugLibReportStatusCode
>>      -> RuntimeDxeReportStatusCodeLib
>>         [status code reporting]
>>
>> => ReportStatusCodeRouterRuntimeDxe
>>     [status code routing]
>>
>> => StatusCodeHandlerRuntimeDxe
>>     [status code handling, such as SerialPortWrite():]
>>    -> FdtPL011SerialPortLibRuntime
>>
>> This is actually a good idea, but it would be nice if:
>> - QEMU had two PL011 ports,
>> - the boot time firmware log, and the OS log, went to one port
>> - the runtme firmware log went to the other port.
>>
>> Given that this series provides the SerialPortLib instance for
>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>> for runtime firmware logging.
>>
>> I don't insist that this series implement all of that, but it should
>> either prevent a conflict on the one PL011 between the firmware and the
>> OS, or else be very explicit about the possible conflict in the commit
>> messages.
>>
>> Thanks
>> Laszlo
>>
>> .
>>
> 
> 

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Andrew Fish]

> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek <lersek@redhat.com> wrote:
> 
> Hello Heyi,
> 
> On 03/12/19 07:56, Heyi Guo wrote:
>> Hi Laszlo,
>> 
>> I'm thinking about a proposal as below:
>> 
>> 1. Reuse the framework of
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>> 
>> 2. The boot time behavior of this module is not changed
>> 3. Switch to status code protocol for runtime debug
>> 4. Use an overridden SerialPortLib instance for
>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>> 
>> Then we can have below benefits:
>> 1. the boot time firmware log, and the OS log, went to one port; and
>> there is no dependence for runtime drivers to output serial message at
>> boot time.
>> 2. By implementing different instances of SerialPortLib for
>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>> runtime serial message to platform specific serial port.
> 
> This looks a bit over-engineered to me. Ultimately our goal is:
> - for DEBUGs to reach "a" serial port at runtime -- namely one that
> differs from the "normal" serial port.
> 
> Given that you'd have to implement a "special" SerialPortLib instance
> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
> 
> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
> stick universally with BaseDebugLibSerialPort, for DebugLib,
> 
> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
> (a) runtime behavior and (b) handling of a special serial port?
> 
> In other words, push down the "runtime?" distinction from DebugLib (see
> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
> would be used only with DXE_RUNTIME_DRIVERs.
> 
> The lib instance would prepare everything in advance for accessing the
> "special" serial port at runtime (which could require allocating runtime
> memory in advance). Once ExitBootServices() is called, a callback should
> switch over the behavior of the lib instance, without allocating further
> memory. (The switched-over behavior would not have to be functional
> until the virtual address map is set.)
> 

Laszlo,

Runtime does not quite work like that. As soon as the Set Virtual Address map fires it is only legal to call things EFI RT from the virtual mapping. The last stage of the Set Virtual Address Map call is to reapply the fixups in the Runtime Drivers for the virtual address space. That means any absolute address reference in the code now points to the virtual address. Also the Set Virtual Address event told the Runtime Driver to convert all its pointers to point to the new virtual address space. 

The blackout and the fact you need to hide the hardware from the OS make doing things at EFI Runtime Time hard. 

I agree with you that since our logging is really just a serial stream we don't require Report Status Code. Report Status Code exists so that the old PC POST Cards could still be supported, see: https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is also a Report Status Code layer that allows redirecting the stream to multiple agents, so for example you could send data to port 80 (POST Card) and write out the same values in the serial stream. I find lookup up Report Status Code codes very tedious and not really helpful for debugging vs. DEBUG prints. 

Thanks,

Andrew Fish

> I've always seen status code reporting cumbersome in comparison to plain
> DEBUG messages. My understanding is that status code reporting targets
> devices that are even dumber than serial ports. But, we do target a
> second serial port. So if we can keep the switchover internal to the
> SerialPortLib class, at least for runtime DXE drivers, then I think we
> should do that.
> 
> Thanks,
> Laszlo
> 
>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>> +Peter, for the last few paragraphs
>>> 
>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>> OS runtime.
>>>>> The patches are trying to provide a handy method to enable runtime
>>>>> serial port
>>>>> debug for ArmVirtQemu.
>>>>> 
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>> 
>>>>> Heyi Guo (3):
>>>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>    ArmVirtQemu: enable runtime debug by build flag
>>>>> 
>>>> Hello Heyi,
>>>> 
>>>> We have had this discussion before, and IIRC, the proposed solution
>>>> was to use status codes.
>>>> 
>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>> one of the MdeModulePkg maintainers can elaborate.
>>> Here's the basic idea.
>>> 
>>> Status Code reporting and routing are defined by the PI spec for OS
>>> runtime as well, not just boot time.
>>> 
>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>> 5c574b222ec2, via the generic runtime driver
>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>> 
>>> ReportStatusCodeLib -->
>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>> 
>>> (for some module types). As a result, when modules of those types report
>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>> driver, which "routes" the status codes.
>>> 
>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>> (built into BdsDxe) to register a status code *handler* callback with
>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>> runtime), and only for a very specific group of status codes. As a
>>> result, when a module of suitable type reports a status code,
>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>> "handles it" (in our implementation, we simply format it to the UEFI
>>> console), assuming the status code is the kind we are interested in.
>>> 
>>> 
>>> Now we come to the current series. First, the series adds the following
>>> DebugLib class resolution:
>>> 
>>> DebugLib -->
>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>> 
>>> 
>>> This will cause modules to publish their DEBUG messages as status codes
>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>> too lazy to check the exact status code classes etc that
>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>> "routing". As another result, until we reach a late enough stage in the
>>> boot, those messages will not be printed by anything (because there's
>>> not going to be any "handling" for them).
>>> 
>>> (The present series also updates the ReportStatusCodeLib resolution so
>>> it can be used at runtime too, but that's quite auxiliary to this
>>> discussion.)
>>> 
>>> Second, this series includes the generic Status Code *handling* driver
>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>> the particular handling that we already have in BdsDxe via the earlier
>>> series, this generic status handler driver registers a handler callback
>>> that simply prints status codes to the serial port (dependent on a PCD
>>> setting), via SerialPortLib.
>>> 
>>> With the modification from the first patch, this generic status code
>>> *handler* driver is extended to runtime serial port operation. And, the
>>> second patch in the series provides a SerialPortLib instance for it that
>>> can work at runtime.
>>> 
>>> All in all, when a runtime driver calls DEBUG, this happens:
>>> 
>>>    runtime driver calls DEBUG
>>>    -> PeiDxeDebugLibReportStatusCode
>>>      -> RuntimeDxeReportStatusCodeLib
>>>         [status code reporting]
>>> 
>>> => ReportStatusCodeRouterRuntimeDxe
>>>     [status code routing]
>>> 
>>> => StatusCodeHandlerRuntimeDxe
>>>     [status code handling, such as SerialPortWrite():]
>>>    -> FdtPL011SerialPortLibRuntime
>>> 
>>> This is actually a good idea, but it would be nice if:
>>> - QEMU had two PL011 ports,
>>> - the boot time firmware log, and the OS log, went to one port
>>> - the runtme firmware log went to the other port.
>>> 
>>> Given that this series provides the SerialPortLib instance for
>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>> for runtime firmware logging.
>>> 
>>> I don't insist that this series implement all of that, but it should
>>> either prevent a conflict on the one PL011 between the firmware and the
>>> OS, or else be very explicit about the possible conflict in the commit
>>> messages.
>>> 
>>> Thanks
>>> Laszlo
>>> 
>>> .
>>> 
>> 
>> 
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

Hi Andrew,

On 03/12/19 18:28, Andrew Fish wrote:
> 
> 
>> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Hello Heyi,
>>
>> On 03/12/19 07:56, Heyi Guo wrote:
>>> Hi Laszlo,
>>>
>>> I'm thinking about a proposal as below:
>>>
>>> 1. Reuse the framework of
>>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>>
>>> 2. The boot time behavior of this module is not changed
>>> 3. Switch to status code protocol for runtime debug
>>> 4. Use an overridden SerialPortLib instance for
>>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>>
>>> Then we can have below benefits:
>>> 1. the boot time firmware log, and the OS log, went to one port; and
>>> there is no dependence for runtime drivers to output serial message at
>>> boot time.
>>> 2. By implementing different instances of SerialPortLib for
>>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>>> runtime serial message to platform specific serial port.
>>
>> This looks a bit over-engineered to me. Ultimately our goal is:
>> - for DEBUGs to reach "a" serial port at runtime -- namely one that
>> differs from the "normal" serial port.
>>
>> Given that you'd have to implement a "special" SerialPortLib instance
>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>
>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>
>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>> (a) runtime behavior and (b) handling of a special serial port?
>>
>> In other words, push down the "runtime?" distinction from DebugLib (see
>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>> would be used only with DXE_RUNTIME_DRIVERs.
>>
>> The lib instance would prepare everything in advance for accessing the
>> "special" serial port at runtime (which could require allocating runtime
>> memory in advance). Once ExitBootServices() is called, a callback should
>> switch over the behavior of the lib instance, without allocating further
>> memory. (The switched-over behavior would not have to be functional
>> until the virtual address map is set.)
>>
> 
> Laszlo,
> 
> Runtime does not quite work like that. As soon as the Set Virtual Address map fires it is only legal to call things EFI RT from the virtual mapping. The last stage of the Set Virtual Address Map call is to reapply the fixups in the Runtime Drivers for the virtual address space. That means any absolute address reference in the code now points to the virtual address. Also the Set Virtual Address event told the Runtime Driver to convert all its pointers to point to the new virtual address space. 

I believe I understand this, but I don't understand where my suggestion
was invalid given, your explanation.

What I meant was the following:

- the SerialPortLib instance in question configures the normal serial
  port (which is not hidden from the OS) for boot time logging

- it also sets up the other serial port (which will be hidden from the
  OS, and used for runtime firmware debug messages) with everything
  necessary. Runtime memory allocation (if dynamic memory allocation is
  needed), hardware configuration, pointers to (physical) addresses of
  registers, and even MMIO | RUNTIME entries in the GCD memory space map
  (if they don't exist yet). Any PCDs that we reasonably expect to be
  dynamic, would be cached in globals, and so on.

- Once ExitBootServices() occurs, the callback in the lib instance
  updates a boolean flag. In the SetVirtualAddressMap() handler, the
  pointers to the MMIO registers of the "hidden" serial port are
  converted to their virtual counterparts. No memory allocation or
  hardware configuration is necessary in either event handler.

- Further DEBUG messages are simply directed to the "hidden" serial
  port, via the boolean flag and the converted pointers.

Thanks,
Laszlo

> The blackout and the fact you need to hide the hardware from the OS make doing things at EFI Runtime Time hard. 
> 
> I agree with you that since our logging is really just a serial stream we don't require Report Status Code. Report Status Code exists so that the old PC POST Cards could still be supported, see: https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is also a Report Status Code layer that allows redirecting the stream to multiple agents, so for example you could send data to port 80 (POST Card) and write out the same values in the serial stream. I find lookup up Report Status Code codes very tedious and not really helpful for debugging vs. DEBUG prints. 
> 
> Thanks,
> 
> Andrew Fish
> 
>> I've always seen status code reporting cumbersome in comparison to plain
>> DEBUG messages. My understanding is that status code reporting targets
>> devices that are even dumber than serial ports. But, we do target a
>> second serial port. So if we can keep the switchover internal to the
>> SerialPortLib class, at least for runtime DXE drivers, then I think we
>> should do that.
>>
>> Thanks,
>> Laszlo
>>
>>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>>> +Peter, for the last few paragraphs
>>>>
>>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>>> OS runtime.
>>>>>> The patches are trying to provide a handy method to enable runtime
>>>>>> serial port
>>>>>> debug for ArmVirtQemu.
>>>>>>
>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>>>
>>>>>> Heyi Guo (3):
>>>>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>>    ArmVirtQemu: enable runtime debug by build flag
>>>>>>
>>>>> Hello Heyi,
>>>>>
>>>>> We have had this discussion before, and IIRC, the proposed solution
>>>>> was to use status codes.
>>>>>
>>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>>> one of the MdeModulePkg maintainers can elaborate.
>>>> Here's the basic idea.
>>>>
>>>> Status Code reporting and routing are defined by the PI spec for OS
>>>> runtime as well, not just boot time.
>>>>
>>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>>> 5c574b222ec2, via the generic runtime driver
>>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>>>
>>>> ReportStatusCodeLib -->
>>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>
>>>> (for some module types). As a result, when modules of those types report
>>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>>> driver, which "routes" the status codes.
>>>>
>>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>>> (built into BdsDxe) to register a status code *handler* callback with
>>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>>> runtime), and only for a very specific group of status codes. As a
>>>> result, when a module of suitable type reports a status code,
>>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>>> "handles it" (in our implementation, we simply format it to the UEFI
>>>> console), assuming the status code is the kind we are interested in.
>>>>
>>>>
>>>> Now we come to the current series. First, the series adds the following
>>>> DebugLib class resolution:
>>>>
>>>> DebugLib -->
>>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>>>
>>>>
>>>> This will cause modules to publish their DEBUG messages as status codes
>>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>>> too lazy to check the exact status code classes etc that
>>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>>> "routing". As another result, until we reach a late enough stage in the
>>>> boot, those messages will not be printed by anything (because there's
>>>> not going to be any "handling" for them).
>>>>
>>>> (The present series also updates the ReportStatusCodeLib resolution so
>>>> it can be used at runtime too, but that's quite auxiliary to this
>>>> discussion.)
>>>>
>>>> Second, this series includes the generic Status Code *handling* driver
>>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>>> the particular handling that we already have in BdsDxe via the earlier
>>>> series, this generic status handler driver registers a handler callback
>>>> that simply prints status codes to the serial port (dependent on a PCD
>>>> setting), via SerialPortLib.
>>>>
>>>> With the modification from the first patch, this generic status code
>>>> *handler* driver is extended to runtime serial port operation. And, the
>>>> second patch in the series provides a SerialPortLib instance for it that
>>>> can work at runtime.
>>>>
>>>> All in all, when a runtime driver calls DEBUG, this happens:
>>>>
>>>>    runtime driver calls DEBUG
>>>>    -> PeiDxeDebugLibReportStatusCode
>>>>      -> RuntimeDxeReportStatusCodeLib
>>>>         [status code reporting]
>>>>
>>>> => ReportStatusCodeRouterRuntimeDxe
>>>>     [status code routing]
>>>>
>>>> => StatusCodeHandlerRuntimeDxe
>>>>     [status code handling, such as SerialPortWrite():]
>>>>    -> FdtPL011SerialPortLibRuntime
>>>>
>>>> This is actually a good idea, but it would be nice if:
>>>> - QEMU had two PL011 ports,
>>>> - the boot time firmware log, and the OS log, went to one port
>>>> - the runtme firmware log went to the other port.
>>>>
>>>> Given that this series provides the SerialPortLib instance for
>>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>>> for runtime firmware logging.
>>>>
>>>> I don't insist that this series implement all of that, but it should
>>>> either prevent a conflict on the one PL011 between the firmware and the
>>>> OS, or else be very explicit about the possible conflict in the commit
>>>> messages.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>> .
>>>>
>>>
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> 

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Brian J. Johnson]

On 3/12/19 12:28 PM, Andrew Fish via edk2-devel wrote:
> 
> 
>> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> Hello Heyi,
>>
>> On 03/12/19 07:56, Heyi Guo wrote:
>>> Hi Laszlo,
>>>
>>> I'm thinking about a proposal as below:
>>>
>>> 1. Reuse the framework of
>>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>>
>>> 2. The boot time behavior of this module is not changed
>>> 3. Switch to status code protocol for runtime debug
>>> 4. Use an overridden SerialPortLib instance for
>>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>>
>>> Then we can have below benefits:
>>> 1. the boot time firmware log, and the OS log, went to one port; and
>>> there is no dependence for runtime drivers to output serial message at
>>> boot time.
>>> 2. By implementing different instances of SerialPortLib for
>>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>>> runtime serial message to platform specific serial port.
>>
>> This looks a bit over-engineered to me. Ultimately our goal is:
>> - for DEBUGs to reach "a" serial port at runtime -- namely one that
>> differs from the "normal" serial port.
>>
>> Given that you'd have to implement a "special" SerialPortLib instance
>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>
>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>
>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>> (a) runtime behavior and (b) handling of a special serial port?
>>
>> In other words, push down the "runtime?" distinction from DebugLib (see
>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>> would be used only with DXE_RUNTIME_DRIVERs.
>>
>> The lib instance would prepare everything in advance for accessing the
>> "special" serial port at runtime (which could require allocating runtime
>> memory in advance). Once ExitBootServices() is called, a callback should
>> switch over the behavior of the lib instance, without allocating further
>> memory. (The switched-over behavior would not have to be functional
>> until the virtual address map is set.)
>>
> 
> Laszlo,
> 
> Runtime does not quite work like that. As soon as the Set Virtual Address map fires it is only legal to call things EFI RT from the virtual mapping. The last stage of the Set Virtual Address Map call is to reapply the fixups in the Runtime Drivers for the virtual address space. That means any absolute address reference in the code now points to the virtual address. Also the Set Virtual Address event told the Runtime Driver to convert all its pointers to point to the new virtual address space.
> 
> The blackout and the fact you need to hide the hardware from the OS make doing things at EFI Runtime Time hard.
> 
> I agree with you that since our logging is really just a serial stream we don't require Report Status Code. Report Status Code exists so that the old PC POST Cards could still be supported, see: https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is also a Report Status Code layer that allows redirecting the stream to multiple agents, so for example you could send data to port 80 (POST Card) and write out the same values in the serial stream. I find lookup up Report Status Code codes very tedious and not really helpful for debugging vs. DEBUG prints.
> 
> Thanks,
> 
> Andrew Fish

Andrew, wasn't Report Status Code also intended to reduce code size? 
PeiDxeDebugLibReportStatusCode packages up DEBUG() arguments into a 
structure and passes it to the status code handler.  It avoids having to 
link a PrintLib and SerialPortLib instance into essentially every 
module.  At least that was the intent... IIRC at some point a few years 
ago folks realized that VA_LIST wasn't portable across compilers, so now 
PeiDxeDebugLibReportStatusCode converts VA_LIST into BASE_LIST, which 
requires parsing the format string much like PrintLib does.  No idea if 
that actually results in a space savings over BaseDebugLibSerialPort, 
especially for really simple SerialPortLib instances.

+1 for the difficulty of decoding Report Status Code codes... there has 
to be a better way to do that than manually parsing through 
PiStatusCode.h and friends.

Brian Johnson

> 
>> I've always seen status code reporting cumbersome in comparison to plain
>> DEBUG messages. My understanding is that status code reporting targets
>> devices that are even dumber than serial ports. But, we do target a
>> second serial port. So if we can keep the switchover internal to the
>> SerialPortLib class, at least for runtime DXE drivers, then I think we
>> should do that.
>>
>> Thanks,
>> Laszlo
>>
>>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>>> +Peter, for the last few paragraphs
>>>>
>>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>>> OS runtime.
>>>>>> The patches are trying to provide a handy method to enable runtime
>>>>>> serial port
>>>>>> debug for ArmVirtQemu.
>>>>>>
>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>>>
>>>>>> Heyi Guo (3):
>>>>>>     MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>>     ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>>     ArmVirtQemu: enable runtime debug by build flag
>>>>>>
>>>>> Hello Heyi,
>>>>>
>>>>> We have had this discussion before, and IIRC, the proposed solution
>>>>> was to use status codes.
>>>>>
>>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>>> one of the MdeModulePkg maintainers can elaborate.
>>>> Here's the basic idea.
>>>>
>>>> Status Code reporting and routing are defined by the PI spec for OS
>>>> runtime as well, not just boot time.
>>>>
>>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>>> 5c574b222ec2, via the generic runtime driver
>>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>>>
>>>> ReportStatusCodeLib -->
>>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>
>>>> (for some module types). As a result, when modules of those types report
>>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>>> driver, which "routes" the status codes.
>>>>
>>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>>> (built into BdsDxe) to register a status code *handler* callback with
>>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>>> runtime), and only for a very specific group of status codes. As a
>>>> result, when a module of suitable type reports a status code,
>>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>>> "handles it" (in our implementation, we simply format it to the UEFI
>>>> console), assuming the status code is the kind we are interested in.
>>>>
>>>>
>>>> Now we come to the current series. First, the series adds the following
>>>> DebugLib class resolution:
>>>>
>>>> DebugLib -->
>>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>>>
>>>>
>>>> This will cause modules to publish their DEBUG messages as status codes
>>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>>> too lazy to check the exact status code classes etc that
>>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>>> "routing". As another result, until we reach a late enough stage in the
>>>> boot, those messages will not be printed by anything (because there's
>>>> not going to be any "handling" for them).
>>>>
>>>> (The present series also updates the ReportStatusCodeLib resolution so
>>>> it can be used at runtime too, but that's quite auxiliary to this
>>>> discussion.)
>>>>
>>>> Second, this series includes the generic Status Code *handling* driver
>>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>>> the particular handling that we already have in BdsDxe via the earlier
>>>> series, this generic status handler driver registers a handler callback
>>>> that simply prints status codes to the serial port (dependent on a PCD
>>>> setting), via SerialPortLib.
>>>>
>>>> With the modification from the first patch, this generic status code
>>>> *handler* driver is extended to runtime serial port operation. And, the
>>>> second patch in the series provides a SerialPortLib instance for it that
>>>> can work at runtime.
>>>>
>>>> All in all, when a runtime driver calls DEBUG, this happens:
>>>>
>>>>     runtime driver calls DEBUG
>>>>     -> PeiDxeDebugLibReportStatusCode
>>>>       -> RuntimeDxeReportStatusCodeLib
>>>>          [status code reporting]
>>>>
>>>> => ReportStatusCodeRouterRuntimeDxe
>>>>      [status code routing]
>>>>
>>>> => StatusCodeHandlerRuntimeDxe
>>>>      [status code handling, such as SerialPortWrite():]
>>>>     -> FdtPL011SerialPortLibRuntime
>>>>
>>>> This is actually a good idea, but it would be nice if:
>>>> - QEMU had two PL011 ports,
>>>> - the boot time firmware log, and the OS log, went to one port
>>>> - the runtme firmware log went to the other port.
>>>>
>>>> Given that this series provides the SerialPortLib instance for
>>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>>> for runtime firmware logging.
>>>>
>>>> I don't insist that this series implement all of that, but it should
>>>> either prevent a conflict on the one PL011 between the firmware and the
>>>> OS, or else be very explicit about the possible conflict in the commit
>>>> messages.
>>>>
>>>> Thanks
>>>> Laszlo
>>>>
>>>> .
>>>>
>>>
>>>
>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Andrew Fish]

> On Mar 13, 2019, at 1:16 PM, Brian J. Johnson <brian.johnson@hpe.com> wrote:
> 
> On 3/12/19 12:28 PM, Andrew Fish via edk2-devel wrote:
>>> On Mar 12, 2019, at 10:05 AM, Laszlo Ersek <lersek@redhat.com> wrote:
>>> 
>>> Hello Heyi,
>>> 
>>> On 03/12/19 07:56, Heyi Guo wrote:
>>>> Hi Laszlo,
>>>> 
>>>> I'm thinking about a proposal as below:
>>>> 
>>>> 1. Reuse the framework of
>>>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>>> 
>>>> 2. The boot time behavior of this module is not changed
>>>> 3. Switch to status code protocol for runtime debug
>>>> 4. Use an overridden SerialPortLib instance for
>>>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>>> 
>>>> Then we can have below benefits:
>>>> 1. the boot time firmware log, and the OS log, went to one port; and
>>>> there is no dependence for runtime drivers to output serial message at
>>>> boot time.
>>>> 2. By implementing different instances of SerialPortLib for
>>>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>>>> runtime serial message to platform specific serial port.
>>> 
>>> This looks a bit over-engineered to me. Ultimately our goal is:
>>> - for DEBUGs to reach "a" serial port at runtime -- namely one that
>>> differs from the "normal" serial port.
>>> 
>>> Given that you'd have to implement a "special" SerialPortLib instance
>>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>> 
>>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>> 
>>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>>> (a) runtime behavior and (b) handling of a special serial port?
>>> 
>>> In other words, push down the "runtime?" distinction from DebugLib (see
>>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>>> would be used only with DXE_RUNTIME_DRIVERs.
>>> 
>>> The lib instance would prepare everything in advance for accessing the
>>> "special" serial port at runtime (which could require allocating runtime
>>> memory in advance). Once ExitBootServices() is called, a callback should
>>> switch over the behavior of the lib instance, without allocating further
>>> memory. (The switched-over behavior would not have to be functional
>>> until the virtual address map is set.)
>>> 
>> Laszlo,
>> Runtime does not quite work like that. As soon as the Set Virtual Address map fires it is only legal to call things EFI RT from the virtual mapping. The last stage of the Set Virtual Address Map call is to reapply the fixups in the Runtime Drivers for the virtual address space. That means any absolute address reference in the code now points to the virtual address. Also the Set Virtual Address event told the Runtime Driver to convert all its pointers to point to the new virtual address space.
>> The blackout and the fact you need to hide the hardware from the OS make doing things at EFI Runtime Time hard.
>> I agree with you that since our logging is really just a serial stream we don't require Report Status Code. Report Status Code exists so that the old PC POST Cards could still be supported, see: https://en.wikipedia.org/wiki/Power-on_self-test#Error_reporting. There is also a Report Status Code layer that allows redirecting the stream to multiple agents, so for example you could send data to port 80 (POST Card) and write out the same values in the serial stream. I find lookup up Report Status Code codes very tedious and not really helpful for debugging vs. DEBUG prints.
>> Thanks,
>> Andrew Fish
> 
> Andrew, wasn't Report Status Code also intended to reduce code size? PeiDxeDebugLibReportStatusCode packages up DEBUG() arguments into a structure and passes it to the status code handler.  It avoids having to link a PrintLib and SerialPortLib instance into essentially every module.  At least that was the intent... IIRC at some point a few years ago folks realized that VA_LIST wasn't portable across compilers, so now PeiDxeDebugLibReportStatusCode converts VA_LIST into BASE_LIST, which requires parsing the format string much like PrintLib does.  No idea if that actually results in a space savings over BaseDebugLibSerialPort, especially for really simple SerialPortLib instances.
> 

Brian,

Good point about size. We are just talking about runtime drivers, and they are likely all compressed so it would be interesting to know the impact. 

> +1 for the difficulty of decoding Report Status Code codes... there has to be a better way to do that than manually parsing through PiStatusCode.h and friends.
> 

I seem to remember there was some code that shortened it to a post code so the common codes ended up being a byte. But it seems like post processing tools would be useful. 

Thanks,

Andrew Fish

> Brian Johnson
> 
>>> I've always seen status code reporting cumbersome in comparison to plain
>>> DEBUG messages. My understanding is that status code reporting targets
>>> devices that are even dumber than serial ports. But, we do target a
>>> second serial port. So if we can keep the switchover internal to the
>>> SerialPortLib class, at least for runtime DXE drivers, then I think we
>>> should do that.
>>> 
>>> Thanks,
>>> Laszlo
>>> 
>>>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>>>> +Peter, for the last few paragraphs
>>>>> 
>>>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>>>> OS runtime.
>>>>>>> The patches are trying to provide a handy method to enable runtime
>>>>>>> serial port
>>>>>>> debug for ArmVirtQemu.
>>>>>>> 
>>>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>>>> 
>>>>>>> Heyi Guo (3):
>>>>>>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>>>    ArmVirtQemu: enable runtime debug by build flag
>>>>>>> 
>>>>>> Hello Heyi,
>>>>>> 
>>>>>> We have had this discussion before, and IIRC, the proposed solution
>>>>>> was to use status codes.
>>>>>> 
>>>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>>>> one of the MdeModulePkg maintainers can elaborate.
>>>>> Here's the basic idea.
>>>>> 
>>>>> Status Code reporting and routing are defined by the PI spec for OS
>>>>> runtime as well, not just boot time.
>>>>> 
>>>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>>>> 5c574b222ec2, via the generic runtime driver
>>>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>>>> 
>>>>> ReportStatusCodeLib -->
>>>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>>> 
>>>>> (for some module types). As a result, when modules of those types report
>>>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>>>> driver, which "routes" the status codes.
>>>>> 
>>>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>>>> (built into BdsDxe) to register a status code *handler* callback with
>>>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>>>> runtime), and only for a very specific group of status codes. As a
>>>>> result, when a module of suitable type reports a status code,
>>>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>>>> "handles it" (in our implementation, we simply format it to the UEFI
>>>>> console), assuming the status code is the kind we are interested in.
>>>>> 
>>>>> 
>>>>> Now we come to the current series. First, the series adds the following
>>>>> DebugLib class resolution:
>>>>> 
>>>>> DebugLib -->
>>>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>>>> 
>>>>> 
>>>>> This will cause modules to publish their DEBUG messages as status codes
>>>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>>>> too lazy to check the exact status code classes etc that
>>>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>>>> "routing". As another result, until we reach a late enough stage in the
>>>>> boot, those messages will not be printed by anything (because there's
>>>>> not going to be any "handling" for them).
>>>>> 
>>>>> (The present series also updates the ReportStatusCodeLib resolution so
>>>>> it can be used at runtime too, but that's quite auxiliary to this
>>>>> discussion.)
>>>>> 
>>>>> Second, this series includes the generic Status Code *handling* driver
>>>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>>>> the particular handling that we already have in BdsDxe via the earlier
>>>>> series, this generic status handler driver registers a handler callback
>>>>> that simply prints status codes to the serial port (dependent on a PCD
>>>>> setting), via SerialPortLib.
>>>>> 
>>>>> With the modification from the first patch, this generic status code
>>>>> *handler* driver is extended to runtime serial port operation. And, the
>>>>> second patch in the series provides a SerialPortLib instance for it that
>>>>> can work at runtime.
>>>>> 
>>>>> All in all, when a runtime driver calls DEBUG, this happens:
>>>>> 
>>>>>    runtime driver calls DEBUG
>>>>>    -> PeiDxeDebugLibReportStatusCode
>>>>>      -> RuntimeDxeReportStatusCodeLib
>>>>>         [status code reporting]
>>>>> 
>>>>> => ReportStatusCodeRouterRuntimeDxe
>>>>>     [status code routing]
>>>>> 
>>>>> => StatusCodeHandlerRuntimeDxe
>>>>>     [status code handling, such as SerialPortWrite():]
>>>>>    -> FdtPL011SerialPortLibRuntime
>>>>> 
>>>>> This is actually a good idea, but it would be nice if:
>>>>> - QEMU had two PL011 ports,
>>>>> - the boot time firmware log, and the OS log, went to one port
>>>>> - the runtme firmware log went to the other port.
>>>>> 
>>>>> Given that this series provides the SerialPortLib instance for
>>>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>>>> for runtime firmware logging.
>>>>> 
>>>>> I don't insist that this series implement all of that, but it should
>>>>> either prevent a conflict on the one PL011 between the firmware and the
>>>>> OS, or else be very explicit about the possible conflict in the commit
>>>>> messages.
>>>>> 
>>>>> Thanks
>>>>> Laszlo
>>>>> 
>>>>> .
>>>>> 
>>>> 
>>>> 
>>> 
>>> _______________________________________________
>>> edk2-devel mailing list
>>> edk2-devel@lists.01.org
>>> https://lists.01.org/mailman/listinfo/edk2-devel
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

On 2019/3/13 1:05, Laszlo Ersek wrote:
> Hello Heyi,
>
> On 03/12/19 07:56, Heyi Guo wrote:
>> Hi Laszlo,
>>
>> I'm thinking about a proposal as below:
>>
>> 1. Reuse the framework of
>> MdePkg/Library/DxeRuntimeDebugLibSerialPort/DxeRuntimeDebugLibSerialPort.inf
>>
>> 2. The boot time behavior of this module is not changed
>> 3. Switch to status code protocol for runtime debug
>> 4. Use an overridden SerialPortLib instance for
>> StatusCodeHandlerRuntimeDxe; the instance must support runtime access.
>>
>> Then we can have below benefits:
>> 1. the boot time firmware log, and the OS log, went to one port; and
>> there is no dependence for runtime drivers to output serial message at
>> boot time.
>> 2. By implementing different instances of SerialPortLib for
>> StatusCodeHandlerRuntimeDxe, we can have enough flexibility to direct
>> runtime serial message to platform specific serial port.
> This looks a bit over-engineered to me. Ultimately our goal is:
I need to say yes...
> - for DEBUGs to reach "a" serial port at runtime -- namely one that
> differs from the "normal" serial port.
>
> Given that you'd have to implement a "special" SerialPortLib instance
> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>
> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
> stick universally with BaseDebugLibSerialPort, for DebugLib,
>
> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
> (a) runtime behavior and (b) handling of a special serial port?
>
> In other words, push down the "runtime?" distinction from DebugLib (see
> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
> would be used only with DXE_RUNTIME_DRIVERs.
>
> The lib instance would prepare everything in advance for accessing the
> "special" serial port at runtime (which could require allocating runtime
> memory in advance). Once ExitBootServices() is called, a callback should
> switch over the behavior of the lib instance, without allocating further
> memory. (The switched-over behavior would not have to be functional
> until the virtual address map is set.)
My first idea was also simply implement a runtime serial port instance, but the problem is for the initialization.

Since serial port lib is only a library, it seems we can only initialize everything in SerialPortInitialize() or library constructor, but either of which could no work in current EDK2 code framework. Please see my explanation in earlier mail as below:

"The long story is: at first I proposed to still use BaseSerialPortDebugLib, so I added RuntimePrepare() function into the constructor directly, but the builder complained about looped constructors, for RuntimePrepare() invokes gBS and some RunTime libraries. Then I tried disabling the constructor and moved RuntimePrepare() into SerialPortInitialize() which could complete the build, but still couldn't guarantee the calling order of these constructors, for BaseSerialPortDebugLib has a constructor to call SerialPortInitialize()."

Can we use event notify or protocol notify to do the runtime intialization? Like EndOfDxe event group. Any other advice?

Thanks,
Heyi



>
> I've always seen status code reporting cumbersome in comparison to plain
> DEBUG messages. My understanding is that status code reporting targets
> devices that are even dumber than serial ports. But, we do target a
> second serial port. So if we can keep the switchover internal to the
> SerialPortLib class, at least for runtime DXE drivers, then I think we
> should do that.
>
> Thanks,
> Laszlo
>
>> On 2019/3/1 23:27, Laszlo Ersek wrote:
>>> +Peter, for the last few paragraphs
>>>
>>> On 02/28/19 13:10, Ard Biesheuvel wrote:
>>>> On Thu, 28 Feb 2019 at 09:06, Heyi Guo <guoheyi@huawei.com> wrote:
>>>>> Serial port output is useful when debugging UEFI runtime services in
>>>>> OS runtime.
>>>>> The patches are trying to provide a handy method to enable runtime
>>>>> serial port
>>>>> debug for ArmVirtQemu.
>>>>>
>>>>> Cc: Jian J Wang <jian.j.wang@intel.com>
>>>>> Cc: Hao Wu <hao.a.wu@intel.com>
>>>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>>> Cc: Julien Grall <julien.grall@arm.com>
>>>>>
>>>>> Heyi Guo (3):
>>>>>     MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>>>>     ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>>>>     ArmVirtQemu: enable runtime debug by build flag
>>>>>
>>>> Hello Heyi,
>>>>
>>>> We have had this discussion before, and IIRC, the proposed solution
>>>> was to use status codes.
>>>>
>>>> I'm not sure how that is supposed to work though - hopefully Laszlo or
>>>> one of the MdeModulePkg maintainers can elaborate.
>>> Here's the basic idea.
>>>
>>> Status Code reporting and routing are defined by the PI spec for OS
>>> runtime as well, not just boot time.
>>>
>>> Recently we added status code *routing* to ArmVirtPkg, in commit
>>> 5c574b222ec2, via the generic runtime driver
>>> "ReportStatusCodeRouterRuntimeDxe". We also added the library resolution
>>>
>>> ReportStatusCodeLib -->
>>> MdeModulePkg/Library/DxeReportStatusCodeLib/DxeReportStatusCodeLib.inf
>>>
>>> (for some module types). As a result, when modules of those types report
>>> status codes, they now reach the ReportStatusCodeRouterRuntimeDxe
>>> driver, which "routes" the status codes.
>>>
>>> In the same series, we also modified ArmVirtPkg's PlatformBootManagerLib
>>> (built into BdsDxe) to register a status code *handler* callback with
>>> ReportStatusCodeRouterRuntimeDxe -- but only for boot time (not
>>> runtime), and only for a very specific group of status codes. As a
>>> result, when a module of suitable type reports a status code,
>>> ReportStatusCodeRouterRuntimeDxe "routes it" to BdsDxe, and then BdsDxe
>>> "handles it" (in our implementation, we simply format it to the UEFI
>>> console), assuming the status code is the kind we are interested in.
>>>
>>>
>>> Now we come to the current series. First, the series adds the following
>>> DebugLib class resolution:
>>>
>>> DebugLib -->
>>> MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode/PeiDxeDebugLibReportStatusCode.inf
>>>
>>>
>>> This will cause modules to publish their DEBUG messages as status codes
>>> via ReportStatusCodeLib, rather than log them via SerialPortLib. (I'm
>>> too lazy to check the exact status code classes etc that
>>> PeiDxeDebugLibReportStatusCode embeds the DEBUG messages into.) As a
>>> result, DEBUG messages will reach ReportStatusCodeRouterRuntimeDxe for
>>> "routing". As another result, until we reach a late enough stage in the
>>> boot, those messages will not be printed by anything (because there's
>>> not going to be any "handling" for them).
>>>
>>> (The present series also updates the ReportStatusCodeLib resolution so
>>> it can be used at runtime too, but that's quite auxiliary to this
>>> discussion.)
>>>
>>> Second, this series includes the generic Status Code *handling* driver
>>> (also a runtime driver): "StatusCodeHandlerRuntimeDxe". Independently of
>>> the particular handling that we already have in BdsDxe via the earlier
>>> series, this generic status handler driver registers a handler callback
>>> that simply prints status codes to the serial port (dependent on a PCD
>>> setting), via SerialPortLib.
>>>
>>> With the modification from the first patch, this generic status code
>>> *handler* driver is extended to runtime serial port operation. And, the
>>> second patch in the series provides a SerialPortLib instance for it that
>>> can work at runtime.
>>>
>>> All in all, when a runtime driver calls DEBUG, this happens:
>>>
>>>     runtime driver calls DEBUG
>>>     -> PeiDxeDebugLibReportStatusCode
>>>       -> RuntimeDxeReportStatusCodeLib
>>>          [status code reporting]
>>>
>>> => ReportStatusCodeRouterRuntimeDxe
>>>      [status code routing]
>>>
>>> => StatusCodeHandlerRuntimeDxe
>>>      [status code handling, such as SerialPortWrite():]
>>>     -> FdtPL011SerialPortLibRuntime
>>>
>>> This is actually a good idea, but it would be nice if:
>>> - QEMU had two PL011 ports,
>>> - the boot time firmware log, and the OS log, went to one port
>>> - the runtme firmware log went to the other port.
>>>
>>> Given that this series provides the SerialPortLib instance for
>>> StatusCodeHandlerRuntimeDxe anyway, we could implement it so that it
>>> locate a "special" PL011 in QEMU's DTB -- the port that we'd only use
>>> for runtime firmware logging.
>>>
>>> I don't insist that this series implement all of that, but it should
>>> either prevent a conflict on the one PL011 between the firmware and the
>>> OS, or else be very explicit about the possible conflict in the commit
>>> messages.
>>>
>>> Thanks
>>> Laszlo
>>>
>>> .
>>>
>>
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

On 03/16/19 10:41, Heyi Guo wrote:
> On 2019/3/13 1:05, Laszlo Ersek wrote:

>> Given that you'd have to implement a "special" SerialPortLib instance
>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>
>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>
>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>> (a) runtime behavior and (b) handling of a special serial port?
>>
>> In other words, push down the "runtime?" distinction from DebugLib (see
>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>> would be used only with DXE_RUNTIME_DRIVERs.
>>
>> The lib instance would prepare everything in advance for accessing the
>> "special" serial port at runtime (which could require allocating runtime
>> memory in advance). Once ExitBootServices() is called, a callback should
>> switch over the behavior of the lib instance, without allocating further
>> memory. (The switched-over behavior would not have to be functional
>> until the virtual address map is set.)
> My first idea was also simply implement a runtime serial port instance,
> but the problem is for the initialization.
> 
> Since serial port lib is only a library, it seems we can only initialize
> everything in SerialPortInitialize() or library constructor, but either
> of which could no work in current EDK2 code framework. Please see my
> explanation in earlier mail as below:
> 
> "The long story is: at first I proposed to still use
> BaseSerialPortDebugLib, so I added RuntimePrepare() function into the
> constructor directly, but the builder complained about looped
> constructors, for RuntimePrepare() invokes gBS and some RunTime
> libraries. Then I tried disabling the constructor and moved
> RuntimePrepare() into SerialPortInitialize() which could complete the
> build, but still couldn't guarantee the calling order of these
> constructors, for BaseSerialPortDebugLib has a constructor to call
> SerialPortInitialize()."

(Sorry about the late response (I've been away for a few days), and also
for missing that construction loop that you apparently mentioned earlier.)

You mention that "RuntimePrepare() invokes gBS and some RunTime
libraries". How important is it to rely on those libraries?

For example, "gBS" is simply a convenience; you can get at the same
thing from the parameter list of the constructor function too (assuming
you use a MODULE_TYPE of (minimally) DXE_DRIVER, in the lib instance's
INF file).

If we don't have to flatten a ridiculous amount of other library code
into the RuntimePrepare() function, I think such flattening would be a
viable approach. We've run into this constructor loop several times
before, and -- if I remember correctly anyway! -- one approach has been
to declare SerialPortLib the "lowest level abstraction", and make the
affected lib instance self-contained.

> Can we use event notify or protocol notify to do the runtime
> intialization? Like EndOfDxe event group. Any other advice?

I guess this could work too. I'm not really a fan of callbacks as long
as we can manage otherwise (for example, we can't propagate errors out
of callbacks), but if the lib flattening thing gets too complex, we
could do this as well.

Regarding EndOfDxe -- I think EndOfDxe is considered a trust barrier
(namely between platform fw modules and 3rd party modules), so I
wouldn't tie our initialization to that, for clarity's sake. ReadyToBoot
seems a tiny bit less "forced" -- it is sufficiently late, you can still
allocate resources, and we can claim it is "on the path towards OS
runtime". Just be aware that ReadyToBoot can be signaled multiple times,
e.g. if some boot options fail (so uninstall the handler / close the
event when serving the first callback I guess).

Thanks
Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

On 2019/3/20 17:55, Laszlo Ersek wrote:
> On 03/16/19 10:41, Heyi Guo wrote:
>> On 2019/3/13 1:05, Laszlo Ersek wrote:
>>> Given that you'd have to implement a "special" SerialPortLib instance
>>> just for StatusCodeHandlerRuntimeDxe anyway, can we perhaps:
>>>
>>> (1) Undo -- dependent on a build flag? -- commit ebfca258f5d7; i.e.,
>>> stick universally with BaseDebugLibSerialPort, for DebugLib,
>>>
>>> (2) add a new implementation of "FdtPL011SerialPortLib.inf" with both
>>> (a) runtime behavior and (b) handling of a special serial port?
>>>
>>> In other words, push down the "runtime?" distinction from DebugLib (see
>>> commit 7f029e1b31ee) to SerialPortLib. The new SerialPortLib instance
>>> would be used only with DXE_RUNTIME_DRIVERs.
>>>
>>> The lib instance would prepare everything in advance for accessing the
>>> "special" serial port at runtime (which could require allocating runtime
>>> memory in advance). Once ExitBootServices() is called, a callback should
>>> switch over the behavior of the lib instance, without allocating further
>>> memory. (The switched-over behavior would not have to be functional
>>> until the virtual address map is set.)
>> My first idea was also simply implement a runtime serial port instance,
>> but the problem is for the initialization.
>>
>> Since serial port lib is only a library, it seems we can only initialize
>> everything in SerialPortInitialize() or library constructor, but either
>> of which could no work in current EDK2 code framework. Please see my
>> explanation in earlier mail as below:
>>
>> "The long story is: at first I proposed to still use
>> BaseSerialPortDebugLib, so I added RuntimePrepare() function into the
>> constructor directly, but the builder complained about looped
>> constructors, for RuntimePrepare() invokes gBS and some RunTime
>> libraries. Then I tried disabling the constructor and moved
>> RuntimePrepare() into SerialPortInitialize() which could complete the
>> build, but still couldn't guarantee the calling order of these
>> constructors, for BaseSerialPortDebugLib has a constructor to call
>> SerialPortInitialize()."
> (Sorry about the late response (I've been away for a few days), and also
> for missing that construction loop that you apparently mentioned earlier.)
Nothing at all, and thanks for your time in reading and replying the mails even when out of office :)
>
> You mention that "RuntimePrepare() invokes gBS and some RunTime
> libraries". How important is it to rely on those libraries?
>
> For example, "gBS" is simply a convenience; you can get at the same
> thing from the parameter list of the constructor function too (assuming
> you use a MODULE_TYPE of (minimally) DXE_DRIVER, in the lib instance's
> INF file).
>
> If we don't have to flatten a ridiculous amount of other library code
> into the RuntimePrepare() function, I think such flattening would be a
> viable approach. We've run into this constructor loop several times
> before, and -- if I remember correctly anyway! -- one approach has been
> to declare SerialPortLib the "lowest level abstraction", and make the
> affected lib instance self-contained.
In my RFC, we need to use gDS which is from DxeServicesTableLib->EfiGetSystemConfigurationTable()->UefiLib, so that we need to flatten EfiGetSystemConfigurationTable(). And gDS->SetMemorySpaceAttributes() actually relies on gEfiCpuArchProtocolGuid or it will return EFI_NOT_AVAILABLE_YET.

Thanks,
Heyi

>
>> Can we use event notify or protocol notify to do the runtime
>> intialization? Like EndOfDxe event group. Any other advice?
> I guess this could work too. I'm not really a fan of callbacks as long
> as we can manage otherwise (for example, we can't propagate errors out
> of callbacks), but if the lib flattening thing gets too complex, we
> could do this as well.
>
> Regarding EndOfDxe -- I think EndOfDxe is considered a trust barrier
> (namely between platform fw modules and 3rd party modules), so I
> wouldn't tie our initialization to that, for clarity's sake. ReadyToBoot
> seems a tiny bit less "forced" -- it is sufficiently late, you can still
> allocate resources, and we can claim it is "on the path towards OS
> runtime". Just be aware that ReadyToBoot can be signaled multiple times,
> e.g. if some boot options fail (so uninstall the handler / close the
> event when serving the first callback I guess).
>
> Thanks
> Laszlo
>
> .
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

On 03/20/19 13:16, Heyi Guo wrote:
> 
> 
> On 2019/3/20 17:55, Laszlo Ersek wrote:

>> If we don't have to flatten a ridiculous amount of other library code
>> into the RuntimePrepare() function, I think such flattening would be a
>> viable approach. We've run into this constructor loop several times
>> before, and -- if I remember correctly anyway! -- one approach has been
>> to declare SerialPortLib the "lowest level abstraction", and make the
>> affected lib instance self-contained.

> In my RFC, we need to use gDS which is from
> DxeServicesTableLib->EfiGetSystemConfigurationTable()->UefiLib, so that
> we need to flatten EfiGetSystemConfigurationTable().

I think that's acceptable.

> And
> gDS->SetMemorySpaceAttributes() actually relies on
> gEfiCpuArchProtocolGuid or it will return EFI_NOT_AVAILABLE_YET.

This is a valid dependency (even the PI spec spells it out). The way to
deal with it is to add gEfiCpuArchProtocolGuid to the DEPEX section of
the INF file -- this is possible for INF files of library instances as
well, but you have to be careful about module types. Please see the INF
spec on that.

Once you do this, modules linked against the lib instance will inherit
the lib instance's DEPEX, and "and it" together with the rest of their
DEPEX. IOW using the library will delay the containing driver module
until the CPU Arch protocol is available in the protocol database.

Then the constructor can rely on the related DXE services.

Thanks
Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

On 2019/3/21 1:47, Laszlo Ersek wrote:
> On 03/20/19 13:16, Heyi Guo wrote:
>>
>> On 2019/3/20 17:55, Laszlo Ersek wrote:
>>> If we don't have to flatten a ridiculous amount of other library code
>>> into the RuntimePrepare() function, I think such flattening would be a
>>> viable approach. We've run into this constructor loop several times
>>> before, and -- if I remember correctly anyway! -- one approach has been
>>> to declare SerialPortLib the "lowest level abstraction", and make the
>>> affected lib instance self-contained.
>> In my RFC, we need to use gDS which is from
>> DxeServicesTableLib->EfiGetSystemConfigurationTable()->UefiLib, so that
>> we need to flatten EfiGetSystemConfigurationTable().
> I think that's acceptable.
>
>> And
>> gDS->SetMemorySpaceAttributes() actually relies on
>> gEfiCpuArchProtocolGuid or it will return EFI_NOT_AVAILABLE_YET.
> This is a valid dependency (even the PI spec spells it out). The way to
> deal with it is to add gEfiCpuArchProtocolGuid to the DEPEX section of
> the INF file -- this is possible for INF files of library instances as
> well, but you have to be careful about module types. Please see the INF
> spec on that.
>
> Once you do this, modules linked against the lib instance will inherit
> the lib instance's DEPEX, and "and it" together with the rest of their
> DEPEX. IOW using the library will delay the containing driver module
> until the CPU Arch protocol is available in the protocol database.
Yes, one is VariableRuntimeDxe, whose DEPEX is TRUE right now, but I think it is acceptable.
I can take a try for the above proposal.

Thanks,
Heyi

>
> Then the constructor can rely on the related DXE services.
>
> Thanks
> Laszlo
>
> .
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

Hello Heyi,

On 02/28/19 09:05, Heyi Guo wrote:
> Serial port output is useful when debugging UEFI runtime services in OS runtime.
> The patches are trying to provide a handy method to enable runtime serial port
> debug for ArmVirtQemu.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao Wu <hao.a.wu@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Julien Grall <julien.grall@arm.com>
> 
> Heyi Guo (3):
>   MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>   ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>   ArmVirtQemu: enable runtime debug by build flag
> 
>  ArmVirtPkg/ArmVirt.dsc.inc                                                          |   6 ++
>  ArmVirtPkg/ArmVirtQemu.dsc                                                          |  13 +++
>  ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                |   5 +
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                    |  29 ++++--
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h                    |  27 +++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf                  |   3 +
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c              |  27 +++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c             | 104 ++++++++++++++++++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf           |  58 +++++++++++
>  MdeModulePkg/MdeModulePkg.dec                                                       |   6 ++
>  MdeModulePkg/MdeModulePkg.uni                                                       |   6 ++
>  MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   |   2 +-
>  MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf |   7 +-
>  13 files changed, 279 insertions(+), 14 deletions(-)
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
>  create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
> 

I'm quite swamped, so only a few general comments for now.

I'll let the MdeModulePkg maintainers comment on the first patch.

(1) The general idea to utilize

- MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode
- MdePkg/Library/DxeRuntimeDebugLibSerialPort

seems mostly OK to me.

Although, I think I would actually prefer the reverse implementation (=
don't send debug messages to the status code infrastructure; instead,
process status codes by logging them with DEBUG). I don't think there is
a status code handler already present in edk2 for that however.


(2) The use case is too generic. Until very recently, we hadn't enabled
status code routing and reporting in ArmVirtPkg at all. We only did that
recently, in commit 5c574b222ec2 ("ArmVirtPkg/ArmVirtQemu*: enable
minimal Status Code Routing in DXE", 2019-02-25), because there was no
other way to implement the boot progress reporting that I had been
after. (See commit 1797f32e0a19 ("ArmVirtPkg/PlatformBootManagerLib:
display boot option loading/starting", 2019-02-25).)

So, in the proposal, the specific use should be described. What runtime
module do you want status codes from?

And why do you prefer status codes to actual (runtime) debug messages?

... Are you perhaps using this approach only because it lets you print
DEBUGs at runtime? I.e., do you use status codes only as a "runtime
carrier" for DEBUGs?


(3) Patch #2 ("ArmVirtPkg: add runtime instance of
FdtPL011SerialPortLib") looks confusing. It introduces a file called
"FdtPL011SerialPortLibCommon.c", which doesn't seem common to multiple
INF files. It is added only to one file.

Also, the patch changes the behavior of the current lib instance with
regard to the SerialPortInitialize() function and the library
constructor. I don't see why that is necessary just for adding a runtime
instance. The runtime instance should not affect the behavior of the
existent instance.

Sorry if I misunderstood; a more detailed commit message would be nice.


(4) What's most worrying is that this change would lead to an unexpected
sharing of the PL011 device between the OS and the firmware. I don't
think that's a great idea, especially if QEMU's ACPI payload explicitly
advertises the port to the OS.


(On x86/OVMF, the above problems don't surface. There, DEBUG messages
are written to the QEMU debug IO port. The debug IO port is neither used
by the OS kernel (so no race), nor affected by page tables (the IO port
space is absolute). Hence no runtime specific processing.)

Thanks
Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Heyi Guo]

On 2019/2/28 21:39, Laszlo Ersek wrote:
> Hello Heyi,
>
> On 02/28/19 09:05, Heyi Guo wrote:
>> Serial port output is useful when debugging UEFI runtime services in OS runtime.
>> The patches are trying to provide a handy method to enable runtime serial port
>> debug for ArmVirtQemu.
>>
>> Cc: Jian J Wang <jian.j.wang@intel.com>
>> Cc: Hao Wu <hao.a.wu@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Julien Grall <julien.grall@arm.com>
>>
>> Heyi Guo (3):
>>    MdeModulePkg/StatusCode: Add PCD to enable runtime serial debug
>>    ArmVirtPkg: add runtime instance of FdtPL011SerialPortLib
>>    ArmVirtQemu: enable runtime debug by build flag
>>
>>   ArmVirtPkg/ArmVirt.dsc.inc                                                          |   6 ++
>>   ArmVirtPkg/ArmVirtQemu.dsc                                                          |  13 +++
>>   ArmVirtPkg/ArmVirtQemuFvMain.fdf.inc                                                |   5 +
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.c                    |  29 ++++--
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h                    |  27 +++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.inf                  |   3 +
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c              |  27 +++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c             | 104 ++++++++++++++++++++
>>   ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf           |  58 +++++++++++
>>   MdeModulePkg/MdeModulePkg.dec                                                       |   6 ++
>>   MdeModulePkg/MdeModulePkg.uni                                                       |   6 ++
>>   MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.c   |   2 +-
>>   MdeModulePkg/Universal/StatusCodeHandler/RuntimeDxe/StatusCodeHandlerRuntimeDxe.inf |   7 +-
>>   13 files changed, 279 insertions(+), 14 deletions(-)
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLib.h
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibCommon.c
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.c
>>   create mode 100644 ArmVirtPkg/Library/FdtPL011SerialPortLib/FdtPL011SerialPortLibRuntime.inf
>>
> I'm quite swamped, so only a few general comments for now.
>
> I'll let the MdeModulePkg maintainers comment on the first patch.
>
> (1) The general idea to utilize
>
> - MdeModulePkg/Library/PeiDxeDebugLibReportStatusCode
> - MdePkg/Library/DxeRuntimeDebugLibSerialPort
>
> seems mostly OK to me.
>
> Although, I think I would actually prefer the reverse implementation (=
> don't send debug messages to the status code infrastructure; instead,
> process status codes by logging them with DEBUG). I don't think there is
> a status code handler already present in edk2 for that however.
>
>
> (2) The use case is too generic. Until very recently, we hadn't enabled
> status code routing and reporting in ArmVirtPkg at all. We only did that
> recently, in commit 5c574b222ec2 ("ArmVirtPkg/ArmVirtQemu*: enable
> minimal Status Code Routing in DXE", 2019-02-25), because there was no
> other way to implement the boot progress reporting that I had been
> after. (See commit 1797f32e0a19 ("ArmVirtPkg/PlatformBootManagerLib:
> display boot option loading/starting", 2019-02-25).)
>
> So, in the proposal, the specific use should be described. What runtime
> module do you want status codes from?
>
> And why do you prefer status codes to actual (runtime) debug messages?
>
> ... Are you perhaps using this approach only because it lets you print
> DEBUGs at runtime? I.e., do you use status codes only as a "runtime
> carrier" for DEBUGs?
That's true:) StatusCode is only a carrier. We can see lots of X86 platforms use StatusCode driver for DEBUG serial print.

>
>
> (3) Patch #2 ("ArmVirtPkg: add runtime instance of
> FdtPL011SerialPortLib") looks confusing. It introduces a file called
> "FdtPL011SerialPortLibCommon.c", which doesn't seem common to multiple
> INF files. It is added only to one file.
Yes, the file name is not good. It only contains a null hook to build a non-runtime instance. I didn't get a proper name for that. Maybe RuntimeDummy or NoRuntime?
>
> Also, the patch changes the behavior of the current lib instance with
> regard to the SerialPortInitialize() function and the library
> constructor. I don't see why that is necessary just for adding a runtime
> instance. The runtime instance should not affect the behavior of the
> existent instance.
>
> Sorry if I misunderstood; a more detailed commit message would be nice.
Ah, this is a mistake. In current implementation we can still enable the constructor.

The long story is: at first I proposed to still use BaseSerialPortDebugLib, so I added RuntimePrepare() function into the constructor directly, but the builder complained about looped constructors, for RuntimePrepare() invokes gBS and some RunTime libraries. Then I tried disabling the constructor and moved RuntimePrepare() into SerialPortInitialize() which could complete the build, but still couldn't guarantee the calling order of these constructors, for BaseSerialPortDebugLib has a constructor to call SerialPortInitialize(). Finally I switched to the current implementation, but forgot to revert previous changes...

>
>
> (4) What's most worrying is that this change would lead to an unexpected
> sharing of the PL011 device between the OS and the firmware. I don't
> think that's a great idea, especially if QEMU's ACPI payload explicitly
> advertises the port to the OS.
That's true, so I propose to disable the feature by default. It is only used for UEFI runtime code debug. It is always painful when we don't a handy debug tool for runtime services code.
>
> (On x86/OVMF, the above problems don't surface. There, DEBUG messages
> are written to the QEMU debug IO port. The debug IO port is neither used
> by the OS kernel (so no race), nor affected by page tables (the IO port
> space is absolute). Hence no runtime specific processing.)
We have not real port IO on ARM, so I think address conversion is always needed. Also I think this can be a feasible template for physical ARM platforms, for it does not require additional hardware components.

Thanks,
Heyi

>
> Thanks
> Laszlo
>
> .
>

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

+Peter

On 03/01/19 13:24, Heyi Guo wrote:
> On 2019/2/28 21:39, Laszlo Ersek wrote:

>> (4) What's most worrying is that this change would lead to an unexpected
>> sharing of the PL011 device between the OS and the firmware. I don't
>> think that's a great idea, especially if QEMU's ACPI payload explicitly
>> advertises the port to the OS.
> That's true, so I propose to disable the feature by default. It is only
> used for UEFI runtime code debug. It is always painful when we don't a
> handy debug tool for runtime services code.

I think this is the only problem that we have at the design level.

What I'd like to understand is whether it is safe to drive the PL011
serial port from both the firmware and the kernel. Consider a situation
where one VCPU in the guest executes a runtime service call, and writes
to PL011 from firmware code. Meanwhile a different VCPU in the guest
executes some kernel code that produces a log message directly on the
serial console. (Because, I don't think that a runtime service call on
one CPU stops the world for *all* CPUs, in the Linux kernel.) For
starters, the serial output could be garbled, as a consequence, but will
the PL011 register state be messed up irrevocably as well? I don't know.

This is why I'm not a big fan of this approach. Using separate devices
for kernel and firmware would be a lot better.

I remember that Peter did some work to enable two PL011 devices on the
"virt" board. IIRC the issue was that the PL011 enumeration order /
numbering in edk2, and in the Linux (guest) kernel, was exactly the
opposite. And that caused both logs to go to different devices; you
couldn't have a single log file that started with the firmware log, and
continued (after a definite switch-over) with the kernel log.

But in this case, where the firmware could produce log messages on
serial during OS runtime, that's actually the setup I would recommend. A
clean separation between the serial devices used by the firmware and the OS.

The rest of the issues in this series should be more simple to clean up
(rework some commit messages, remove stale code etc).

Thanks
Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Ard Biesheuvel]

On Fri, 1 Mar 2019 at 15:59, Laszlo Ersek <lersek@redhat.com> wrote:
>
> +Peter
>
> On 03/01/19 13:24, Heyi Guo wrote:
> > On 2019/2/28 21:39, Laszlo Ersek wrote:
>
> >> (4) What's most worrying is that this change would lead to an unexpected
> >> sharing of the PL011 device between the OS and the firmware. I don't
> >> think that's a great idea, especially if QEMU's ACPI payload explicitly
> >> advertises the port to the OS.
> > That's true, so I propose to disable the feature by default. It is only
> > used for UEFI runtime code debug. It is always painful when we don't a
> > handy debug tool for runtime services code.
>
> I think this is the only problem that we have at the design level.
>
> What I'd like to understand is whether it is safe to drive the PL011
> serial port from both the firmware and the kernel. Consider a situation
> where one VCPU in the guest executes a runtime service call, and writes
> to PL011 from firmware code. Meanwhile a different VCPU in the guest
> executes some kernel code that produces a log message directly on the
> serial console. (Because, I don't think that a runtime service call on
> one CPU stops the world for *all* CPUs, in the Linux kernel.) For
> starters, the serial output could be garbled, as a consequence, but will
> the PL011 register state be messed up irrevocably as well? I don't know.
>

This is fundamentally broken. The OS will drive the PL011 in interrupt
mode, while the firmware will poll the FIFO registers directly.

> This is why I'm not a big fan of this approach. Using separate devices
> for kernel and firmware would be a lot better.
>

+1

> I remember that Peter did some work to enable two PL011 devices on the
> "virt" board. IIRC the issue was that the PL011 enumeration order /
> numbering in edk2, and in the Linux (guest) kernel, was exactly the
> opposite. And that caused both logs to go to different devices; you
> couldn't have a single log file that started with the firmware log, and
> continued (after a definite switch-over) with the kernel log.
>
> But in this case, where the firmware could produce log messages on
> serial during OS runtime, that's actually the setup I would recommend. A
> clean separation between the serial devices used by the firmware and the OS.
>
> The rest of the issues in this series should be more simple to clean up
> (rework some commit messages, remove stale code etc).
>
> Thanks
> Laszlo

Re: [RFC 0/3] Enable runtime serial debug for ArmVirtQemu

[Laszlo Ersek]

On 03/01/19 16:09, Peter Maydell wrote:
> On Fri, 1 Mar 2019 at 14:59, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> +Peter
>>
>> On 03/01/19 13:24, Heyi Guo wrote:
>>> On 2019/2/28 21:39, Laszlo Ersek wrote:
>>
>>>> (4) What's most worrying is that this change would lead to an unexpected
>>>> sharing of the PL011 device between the OS and the firmware. I don't
>>>> think that's a great idea, especially if QEMU's ACPI payload explicitly
>>>> advertises the port to the OS.
>>> That's true, so I propose to disable the feature by default. It is only
>>> used for UEFI runtime code debug. It is always painful when we don't a
>>> handy debug tool for runtime services code.
>>
>> I think this is the only problem that we have at the design level.
>>
>> What I'd like to understand is whether it is safe to drive the PL011
>> serial port from both the firmware and the kernel.
> 
> I would suggest that it probably is not safe. Certainly it
> does not seem likely to be robust against all possible future
> changes to the kernel's driver and/or the firmware's driver.
> 
>> This is why I'm not a big fan of this approach. Using separate devices
>> for kernel and firmware would be a lot better.
>>
>> I remember that Peter did some work to enable two PL011 devices on the
>> "virt" board. IIRC the issue was that the PL011 enumeration order /
>> numbering in edk2, and in the Linux (guest) kernel, was exactly the
>> opposite. And that caused both logs to go to different devices; you
>> couldn't have a single log file that started with the firmware log, and
>> continued (after a definite switch-over) with the kernel log.
> 
> Yes, that's basically it (I forget the exact details and it's the
> kind of thing that may well depend on exactly what version of the
> kernel or what version of the firmware you were using). I do still
> have a todo list item to add a 2nd UART (probably "if the user
> specifically asked for it with an extra -serial option"), but
> we can't just add it to the board by default.

Thanks for the info!

> Note that if you are using QEMU under emulation where it is emulating
> EL3 then you always get a second UART which is visible only to the
> secure world. This is specifically intended for firmware to use.

The ArmVirtQemu firmware doesn't handle the secure world at all (yet?).

Thanks
Laszlo