public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
@ 2022-05-22 18:47 Benjamin Doron
  2022-05-26  9:16 ` Sean Rhodes
  2022-05-27  5:59 ` Guo Dong
  0 siblings, 2 replies; 6+ messages in thread
From: Benjamin Doron @ 2022-05-22 18:47 UTC (permalink / raw)
  To: devel; +Cc: Guo Dong, Ray Ni, Maurice Ma, Benjamin You, Sean Rhodes

Writes TianoCore debug logs into the CBMEM console ringbuffer, from
where the user can retrieve them with the `cbmem` userspace utility.

The intention is to aid in debugging non-fatal issues even in release
builds, or simply make TianoCore's logs available to those interested.
Consequently, MDEPKG_NDEBUG must be masked. As an in-memory debug
logging library, ASSERTs must be non-fatal to be seen, so they neither
dead-loop nor create a breakpoint. It is assumed that ASSERT() neither
enforces fatal conditions nor security integrity, as release builds do
not call DebugAssert() from the ASSERT macro.

More detailed debug logs are produced with the DEBUG_CODE macro, but
this guards other debug-related code throughout the codebase. To avoid
changing behaviour on release builds, this is only set for debug builds.

Tested on QEMU, dumping the appropriate memory region in the UEFI shell
shows the TianoCore log. An improved revision of the debug library used
in several coreboot-related EDK2 forks, including MrChromebox's.
Previous revisions also tested on an Acer Aspire VN7-572G laptop.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 UefiPayloadPkg/Include/Coreboot.h                          |  12 +
 UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c   | 258 ++++++++++++++++++++
 UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf |  30 +++
 UefiPayloadPkg/UefiPayloadPkg.dsc                          |  25 +-
 4 files changed, 320 insertions(+), 5 deletions(-)

diff --git a/UefiPayloadPkg/Include/Coreboot.h b/UefiPayloadPkg/Include/Coreboot.h
index a3e1109fe84e..4c9cf965519b 100644
--- a/UefiPayloadPkg/Include/Coreboot.h
+++ b/UefiPayloadPkg/Include/Coreboot.h
@@ -199,6 +199,12 @@ struct cb_forward {
   UINT64    forward;
 };
 
+struct cb_cbmem_ref {
+  UINT32 tag;
+  UINT32 size;
+  UINT64 cbmem_addr;
+};
+
 #define CB_TAG_FRAMEBUFFER  0x0012
 struct cb_framebuffer {
   UINT32    tag;
@@ -229,6 +235,12 @@ struct cb_vdat {
 
 #define CB_TAG_TIMESTAMPS     0x0016
 #define CB_TAG_CBMEM_CONSOLE  0x0017
+struct cbmem_console {
+  UINT32 size;
+  UINT32 cursor;
+  UINT8  body[0];
+} __attribute__((packed));
+
 #define CB_TAG_MRC_CACHE      0x0018
 struct cb_cbmem_tab {
   UINT32    tag;
diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
new file mode 100644
index 000000000000..f7c06b0f41e9
--- /dev/null
+++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
@@ -0,0 +1,258 @@
+/** @file
+  CBMEM console SerialPortLib instance
+
+  Copyright (c) 2022, Baruch Binyamin Doron
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#include <Base.h>
+#include <Coreboot.h>
+
+#include <Library/BaseMemoryLib.h>
+#include <Library/BlParseLib.h>
+#include <Library/SerialPortLib.h>
+
+// Upper nibble contains flags
+#define CBMC_CURSOR_MASK ((1 << 28) - 1)
+#define CBMC_OVERFLOW (1 << 31)
+
+STATIC struct cbmem_console  *gCbConsole = NULL;
+
+/**
+  Find coreboot record with given Tag.
+  NOTE: This coreboot-specific function definition is absent
+         from the common BlParseLib header.
+
+  @param  Tag                The tag id to be found
+
+  @retval NULL              The Tag is not found.
+  @retval Others            The pointer to the record found.
+
+**/
+VOID *
+FindCbTag (
+  IN  UINT32         Tag
+  );
+
+/**
+  Initialize the serial device hardware.
+
+  If no initialization is required, then return RETURN_SUCCESS.
+  If the serial device was successfully initialized, then return RETURN_SUCCESS.
+  If the serial device could not be initialized, then return RETURN_DEVICE_ERROR.
+
+  @retval RETURN_SUCCESS        The serial device was initialized.
+  @retval RETURN_DEVICE_ERROR   The serial device could not be initialized.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortInitialize (
+  VOID
+  )
+{
+  struct cb_cbmem_ref *cbref = FindCbTag(CB_TAG_CBMEM_CONSOLE);
+  if (!cbref) {
+    return RETURN_DEVICE_ERROR;
+  }
+
+  gCbConsole = (VOID *)(UINTN)cbref->cbmem_addr;  // Support PEI and DXE
+  if (gCbConsole == NULL) {
+    return RETURN_DEVICE_ERROR;
+  }
+
+  return RETURN_SUCCESS;
+}
+
+/**
+  Write data from buffer to serial device.
+
+  Writes NumberOfBytes data bytes from Buffer to the serial device.
+  The number of bytes actually written to the serial device is returned.
+  If the return value is less than NumberOfBytes, then the write operation failed.
+  If Buffer is NULL, then ASSERT().
+  If NumberOfBytes is zero, then return 0.
+
+  @param  Buffer           Pointer to the data buffer to be written.
+  @param  NumberOfBytes    Number of bytes to written to the serial device.
+
+  @retval 0                NumberOfBytes is 0.
+  @retval >0               The number of bytes written to the serial device.
+                           If this value is less than NumberOfBytes, then the write operation failed.
+
+**/
+UINTN
+EFIAPI
+SerialPortWrite (
+  IN UINT8     *Buffer,
+  IN UINTN     NumberOfBytes
+  )
+{
+  UINT32            cursor;
+  UINT32            flags;
+
+  if (Buffer == NULL || NumberOfBytes == 0) {
+    return 0;
+  }
+
+  if (!gCbConsole) {
+    return 0;
+  }
+
+  cursor = gCbConsole->cursor & CBMC_CURSOR_MASK;
+  flags = gCbConsole->cursor & ~CBMC_CURSOR_MASK;
+  if (cursor >= gCbConsole->size) {
+    // Already overflowed; bail out. TODO: Is this unnecessarily cautious?
+    // - Supports old coreboot version with legacy overflow mechanism.
+    return 0;
+  }
+
+  if (cursor + NumberOfBytes > gCbConsole->size) {
+    // Will overflow, zero cursor and set flag.
+    cursor = 0;
+    flags |= CBMC_OVERFLOW;
+  }
+
+  // Longest debug messages supported by a DebugLib seems to be CHAR16[0x100] == 512.
+  // So, no chance that one message could overflow even the smallest buffer.
+  CopyMem (&gCbConsole->body[cursor], Buffer, NumberOfBytes);
+  cursor += NumberOfBytes;
+
+  if (cursor == gCbConsole->size) {
+    // Next message will overflow, zero cursor.
+    // - But not overflowed yet. Do not set flag.
+    cursor = 0;
+  }
+
+  gCbConsole->cursor = flags | cursor;
+
+  return NumberOfBytes;
+}
+
+/**
+  Read data from serial device and save the datas in buffer.
+
+  Reads NumberOfBytes data bytes from a serial device into the buffer
+  specified by Buffer. The number of bytes actually read is returned.
+  If Buffer is NULL, then ASSERT().
+  If NumberOfBytes is zero, then return 0.
+
+  @param  Buffer           Pointer to the data buffer to store the data read from the serial device.
+  @param  NumberOfBytes    Number of bytes which will be read.
+
+  @retval 0                Read data failed, no data is to be read.
+  @retval >0               Actual number of bytes read from serial device.
+
+**/
+UINTN
+EFIAPI
+SerialPortRead (
+  OUT UINT8     *Buffer,
+  IN  UINTN     NumberOfBytes
+)
+{
+  return 0;
+}
+
+/**
+  Polls a serial device to see if there is any data waiting to be read.
+
+  @retval TRUE             Data is waiting to be read from the serial device.
+  @retval FALSE            There is no data waiting to be read from the serial device.
+
+**/
+BOOLEAN
+EFIAPI
+SerialPortPoll (
+  VOID
+  )
+{
+  return FALSE;
+}
+
+/**
+  Sets the control bits on a serial device.
+
+  @param Control                Sets the bits of Control that are settable.
+
+  @retval RETURN_SUCCESS        The new control bits were set on the serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetControl (
+  IN UINT32 Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Retrieve the status of the control bits on a serial device.
+
+  @param Control                A pointer to return the current control signals from the serial device.
+
+  @retval RETURN_SUCCESS        The control bits were read from the serial device.
+  @retval RETURN_UNSUPPORTED    The serial device does not support this operation.
+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortGetControl (
+  OUT UINT32 *Control
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
+/**
+  Sets the baud rate, receive FIFO depth, transmit/receive time out, parity,
+  data bits, and stop bits on a serial device.
+
+  @param BaudRate           The requested baud rate. A BaudRate value of 0 will use the
+                            device's default interface speed.
+                            On output, the value actually set.
+  @param ReceiveFifoDepth   The requested depth of the FIFO on the receive side of the
+                            serial interface. A ReceiveFifoDepth value of 0 will use
+                            the device's default FIFO depth.
+                            On output, the value actually set.
+  @param Timeout            The requested time out for a single character in microseconds.
+                            This timeout applies to both the transmit and receive side of the
+                            interface. A Timeout value of 0 will use the device's default time
+                            out value.
+                            On output, the value actually set.
+  @param Parity             The type of parity to use on this serial device. A Parity value of
+                            DefaultParity will use the device's default parity value.
+                            On output, the value actually set.
+  @param DataBits           The number of data bits to use on the serial device. A DataBits
+                            value of 0 will use the device's default data bit setting.
+                            On output, the value actually set.
+  @param StopBits           The number of stop bits to use on this serial device. A StopBits
+                            value of DefaultStopBits will use the device's default number of
+                            stop bits.
+                            On output, the value actually set.
+
+  @retval RETURN_SUCCESS            The new attributes were set on the serial device.
+  @retval RETURN_UNSUPPORTED        The serial device does not support this operation.
+  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an unsupported value.
+  @retval RETURN_DEVICE_ERROR       The serial device is not functioning correctly.
+
+**/
+RETURN_STATUS
+EFIAPI
+SerialPortSetAttributes (
+  IN OUT UINT64             *BaudRate,
+  IN OUT UINT32             *ReceiveFifoDepth,
+  IN OUT UINT32             *Timeout,
+  IN OUT EFI_PARITY_TYPE    *Parity,
+  IN OUT UINT8              *DataBits,
+  IN OUT EFI_STOP_BITS_TYPE *StopBits
+  )
+{
+  return RETURN_UNSUPPORTED;
+}
+
diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
new file mode 100644
index 000000000000..3df6a0c736a5
--- /dev/null
+++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
@@ -0,0 +1,30 @@
+## @file
+#  Component description file for CbSerialPortLib library.
+#
+#  Copyright (c) 2022, Baruch Binyamin Doron
+#  SPDX-License-Identifier: BSD-2-Clause-Patent
+#
+##
+
+[Defines]
+  INF_VERSION                    = 0x00010005
+  BASE_NAME                      = CbSerialPortLib
+  FILE_GUID                      = 0DB3EF12-1426-4086-B012-113184C4CE11
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  # Recall that debug logging can be unsafe to core. Route over RSC.
+  LIBRARY_CLASS                  = SerialPortLib
+  CONSTRUCTOR                    = SerialPortInitialize
+
+[Packages]
+  MdePkg/MdePkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  UefiPayloadPkg/UefiPayloadPkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  BlParseLib
+
+[Sources]
+  CbSerialPortLib.c
+
diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 4d9bbc80c866..0e4248767756 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -37,6 +37,7 @@
   DEFINE ABOVE_4G_MEMORY              = TRUE
   DEFINE BOOT_MANAGER_ESCAPE          = FALSE
   DEFINE SD_MMC_TIMEOUT               = 1000000
+  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE
   #
   # SBL:      UEFI payload for Slim Bootloader
   # COREBOOT: UEFI payload for coreboot
@@ -121,10 +122,11 @@
 
 [BuildOptions]
   *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES
-  GCC:*_UNIXGCC_*_CC_FLAGS       = -DMDEPKG_NDEBUG
+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
   GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
   INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG
   MSFT:RELEASE_*_*_CC_FLAGS      = /D MDEPKG_NDEBUG
+!endif
 
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_*_DLINK_FLAGS      = -z common-page-size=0x1000
@@ -231,8 +233,13 @@
   TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
 !endif
   ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
+!if $(USE_CBMEM_FOR_CONSOLE) == TRUE
+  SerialPortLib|UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
+  PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
+!else
   SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
   PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
+!endif
   PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
   IoApicLib|PcAtChipsetPkg/Library/BaseIoApicLib/BaseIoApicLib.inf
 
@@ -422,10 +429,18 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }
   gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x7
   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8000004F
-!if $(SOURCE_DEBUG_ENABLE)
-  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
+  !if $(SOURCE_DEBUG_ENABLE)
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
+  !else
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
+  !endif
 !else
-  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
+  !if $(TARGET) == DEBUG
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x07
+  !else
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03
+  !endif
 !endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|$(MAX_SIZE_NON_POPULATE_CAPSULE)
   #
@@ -471,7 +486,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize
-!if $(TARGET) == DEBUG
+!if ($(TARGET) == DEBUG || $(USE_CBMEM_FOR_CONSOLE) == TRUE)
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE
 !else
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
  2022-05-22 18:47 [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console Benjamin Doron
@ 2022-05-26  9:16 ` Sean Rhodes
  2022-05-26 17:08   ` [edk2-devel] " Benjamin Doron
  2022-05-27  5:59 ` Guo Dong
  1 sibling, 1 reply; 6+ messages in thread
From: Sean Rhodes @ 2022-05-26  9:16 UTC (permalink / raw)
  To: Benjamin Doron; +Cc: devel, Guo Dong, Ray Ni, Maurice Ma, Benjamin You

[-- Attachment #1: Type: text/plain, Size: 16140 bytes --]

Reviewed-by: Sean Rhodes <sean@starlabs.systems>


On Sun, 22 May 2022 at 19:47, Benjamin Doron <benjamin.doron00@gmail.com>
wrote:

> Writes TianoCore debug logs into the CBMEM console ringbuffer, from
> where the user can retrieve them with the `cbmem` userspace utility.
>
> The intention is to aid in debugging non-fatal issues even in release
> builds, or simply make TianoCore's logs available to those interested.
> Consequently, MDEPKG_NDEBUG must be masked. As an in-memory debug
> logging library, ASSERTs must be non-fatal to be seen, so they neither
> dead-loop nor create a breakpoint. It is assumed that ASSERT() neither
> enforces fatal conditions nor security integrity, as release builds do
> not call DebugAssert() from the ASSERT macro.
>
> More detailed debug logs are produced with the DEBUG_CODE macro, but
> this guards other debug-related code throughout the codebase. To avoid
> changing behaviour on release builds, this is only set for debug builds.
>
> Tested on QEMU, dumping the appropriate memory region in the UEFI shell
> shows the TianoCore log. An improved revision of the debug library used
> in several coreboot-related EDK2 forks, including MrChromebox's.
> Previous revisions also tested on an Acer Aspire VN7-572G laptop.
>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Sean Rhodes <sean@starlabs.systems>
> Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
> ---
>  UefiPayloadPkg/Include/Coreboot.h                          |  12 +
>  UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c   | 258
> ++++++++++++++++++++
>  UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf |  30 +++
>  UefiPayloadPkg/UefiPayloadPkg.dsc                          |  25 +-
>  4 files changed, 320 insertions(+), 5 deletions(-)
>
> diff --git a/UefiPayloadPkg/Include/Coreboot.h
> b/UefiPayloadPkg/Include/Coreboot.h
> index a3e1109fe84e..4c9cf965519b 100644
> --- a/UefiPayloadPkg/Include/Coreboot.h
> +++ b/UefiPayloadPkg/Include/Coreboot.h
> @@ -199,6 +199,12 @@ struct cb_forward {
>    UINT64    forward;
>  };
>
> +struct cb_cbmem_ref {
> +  UINT32 tag;
> +  UINT32 size;
> +  UINT64 cbmem_addr;
> +};
> +
>  #define CB_TAG_FRAMEBUFFER  0x0012
>  struct cb_framebuffer {
>    UINT32    tag;
> @@ -229,6 +235,12 @@ struct cb_vdat {
>
>  #define CB_TAG_TIMESTAMPS     0x0016
>  #define CB_TAG_CBMEM_CONSOLE  0x0017
> +struct cbmem_console {
> +  UINT32 size;
> +  UINT32 cursor;
> +  UINT8  body[0];
> +} __attribute__((packed));
> +
>  #define CB_TAG_MRC_CACHE      0x0018
>  struct cb_cbmem_tab {
>    UINT32    tag;
> diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
> b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
> new file mode 100644
> index 000000000000..f7c06b0f41e9
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
> @@ -0,0 +1,258 @@
> +/** @file
> +  CBMEM console SerialPortLib instance
> +
> +  Copyright (c) 2022, Baruch Binyamin Doron
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Base.h>
> +#include <Coreboot.h>
> +
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/BlParseLib.h>
> +#include <Library/SerialPortLib.h>
> +
> +// Upper nibble contains flags
> +#define CBMC_CURSOR_MASK ((1 << 28) - 1)
> +#define CBMC_OVERFLOW (1 << 31)
> +
> +STATIC struct cbmem_console  *gCbConsole = NULL;
> +
> +/**
> +  Find coreboot record with given Tag.
> +  NOTE: This coreboot-specific function definition is absent
> +         from the common BlParseLib header.
> +
> +  @param  Tag                The tag id to be found
> +
> +  @retval NULL              The Tag is not found.
> +  @retval Others            The pointer to the record found.
> +
> +**/
> +VOID *
> +FindCbTag (
> +  IN  UINT32         Tag
> +  );
> +
> +/**
> +  Initialize the serial device hardware.
> +
> +  If no initialization is required, then return RETURN_SUCCESS.
> +  If the serial device was successfully initialized, then return
> RETURN_SUCCESS.
> +  If the serial device could not be initialized, then return
> RETURN_DEVICE_ERROR.
> +
> +  @retval RETURN_SUCCESS        The serial device was initialized.
> +  @retval RETURN_DEVICE_ERROR   The serial device could not be
> initialized.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SerialPortInitialize (
> +  VOID
> +  )
> +{
> +  struct cb_cbmem_ref *cbref = FindCbTag(CB_TAG_CBMEM_CONSOLE);
> +  if (!cbref) {
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  gCbConsole = (VOID *)(UINTN)cbref->cbmem_addr;  // Support PEI and DXE
> +  if (gCbConsole == NULL) {
> +    return RETURN_DEVICE_ERROR;
> +  }
> +
> +  return RETURN_SUCCESS;
> +}
> +
> +/**
> +  Write data from buffer to serial device.
> +
> +  Writes NumberOfBytes data bytes from Buffer to the serial device.
> +  The number of bytes actually written to the serial device is returned.
> +  If the return value is less than NumberOfBytes, then the write
> operation failed.
> +  If Buffer is NULL, then ASSERT().
> +  If NumberOfBytes is zero, then return 0.
> +
> +  @param  Buffer           Pointer to the data buffer to be written.
> +  @param  NumberOfBytes    Number of bytes to written to the serial
> device.
> +
> +  @retval 0                NumberOfBytes is 0.
> +  @retval >0               The number of bytes written to the serial
> device.
> +                           If this value is less than NumberOfBytes, then
> the write operation failed.
> +
> +**/
> +UINTN
> +EFIAPI
> +SerialPortWrite (
> +  IN UINT8     *Buffer,
> +  IN UINTN     NumberOfBytes
> +  )
> +{
> +  UINT32            cursor;
> +  UINT32            flags;
> +
> +  if (Buffer == NULL || NumberOfBytes == 0) {
> +    return 0;
> +  }
> +
> +  if (!gCbConsole) {
> +    return 0;
> +  }
> +
> +  cursor = gCbConsole->cursor & CBMC_CURSOR_MASK;
> +  flags = gCbConsole->cursor & ~CBMC_CURSOR_MASK;
> +  if (cursor >= gCbConsole->size) {
> +    // Already overflowed; bail out. TODO: Is this unnecessarily cautious?
> +    // - Supports old coreboot version with legacy overflow mechanism.
> +    return 0;
> +  }
> +
> +  if (cursor + NumberOfBytes > gCbConsole->size) {
> +    // Will overflow, zero cursor and set flag.
> +    cursor = 0;
> +    flags |= CBMC_OVERFLOW;
> +  }
> +
> +  // Longest debug messages supported by a DebugLib seems to be
> CHAR16[0x100] == 512.
> +  // So, no chance that one message could overflow even the smallest
> buffer.
> +  CopyMem (&gCbConsole->body[cursor], Buffer, NumberOfBytes);
> +  cursor += NumberOfBytes;
> +
> +  if (cursor == gCbConsole->size) {
> +    // Next message will overflow, zero cursor.
> +    // - But not overflowed yet. Do not set flag.
> +    cursor = 0;
> +  }
> +
> +  gCbConsole->cursor = flags | cursor;
> +
> +  return NumberOfBytes;
> +}
> +
> +/**
> +  Read data from serial device and save the datas in buffer.
> +
> +  Reads NumberOfBytes data bytes from a serial device into the buffer
> +  specified by Buffer. The number of bytes actually read is returned.
> +  If Buffer is NULL, then ASSERT().
> +  If NumberOfBytes is zero, then return 0.
> +
> +  @param  Buffer           Pointer to the data buffer to store the data
> read from the serial device.
> +  @param  NumberOfBytes    Number of bytes which will be read.
> +
> +  @retval 0                Read data failed, no data is to be read.
> +  @retval >0               Actual number of bytes read from serial device.
> +
> +**/
> +UINTN
> +EFIAPI
> +SerialPortRead (
> +  OUT UINT8     *Buffer,
> +  IN  UINTN     NumberOfBytes
> +)
> +{
> +  return 0;
> +}
> +
> +/**
> +  Polls a serial device to see if there is any data waiting to be read.
> +
> +  @retval TRUE             Data is waiting to be read from the serial
> device.
> +  @retval FALSE            There is no data waiting to be read from the
> serial device.
> +
> +**/
> +BOOLEAN
> +EFIAPI
> +SerialPortPoll (
> +  VOID
> +  )
> +{
> +  return FALSE;
> +}
> +
> +/**
> +  Sets the control bits on a serial device.
> +
> +  @param Control                Sets the bits of Control that are
> settable.
> +
> +  @retval RETURN_SUCCESS        The new control bits were set on the
> serial device.
> +  @retval RETURN_UNSUPPORTED    The serial device does not support this
> operation.
> +  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
> correctly.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SerialPortSetControl (
> +  IN UINT32 Control
> +  )
> +{
> +  return RETURN_UNSUPPORTED;
> +}
> +
> +/**
> +  Retrieve the status of the control bits on a serial device.
> +
> +  @param Control                A pointer to return the current control
> signals from the serial device.
> +
> +  @retval RETURN_SUCCESS        The control bits were read from the
> serial device.
> +  @retval RETURN_UNSUPPORTED    The serial device does not support this
> operation.
> +  @retval RETURN_DEVICE_ERROR   The serial device is not functioning
> correctly.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SerialPortGetControl (
> +  OUT UINT32 *Control
> +  )
> +{
> +  return RETURN_UNSUPPORTED;
> +}
> +
> +/**
> +  Sets the baud rate, receive FIFO depth, transmit/receive time out,
> parity,
> +  data bits, and stop bits on a serial device.
> +
> +  @param BaudRate           The requested baud rate. A BaudRate value of
> 0 will use the
> +                            device's default interface speed.
> +                            On output, the value actually set.
> +  @param ReceiveFifoDepth   The requested depth of the FIFO on the
> receive side of the
> +                            serial interface. A ReceiveFifoDepth value of
> 0 will use
> +                            the device's default FIFO depth.
> +                            On output, the value actually set.
> +  @param Timeout            The requested time out for a single character
> in microseconds.
> +                            This timeout applies to both the transmit and
> receive side of the
> +                            interface. A Timeout value of 0 will use the
> device's default time
> +                            out value.
> +                            On output, the value actually set.
> +  @param Parity             The type of parity to use on this serial
> device. A Parity value of
> +                            DefaultParity will use the device's default
> parity value.
> +                            On output, the value actually set.
> +  @param DataBits           The number of data bits to use on the serial
> device. A DataBits
> +                            value of 0 will use the device's default data
> bit setting.
> +                            On output, the value actually set.
> +  @param StopBits           The number of stop bits to use on this serial
> device. A StopBits
> +                            value of DefaultStopBits will use the
> device's default number of
> +                            stop bits.
> +                            On output, the value actually set.
> +
> +  @retval RETURN_SUCCESS            The new attributes were set on the
> serial device.
> +  @retval RETURN_UNSUPPORTED        The serial device does not support
> this operation.
> +  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an
> unsupported value.
> +  @retval RETURN_DEVICE_ERROR       The serial device is not functioning
> correctly.
> +
> +**/
> +RETURN_STATUS
> +EFIAPI
> +SerialPortSetAttributes (
> +  IN OUT UINT64             *BaudRate,
> +  IN OUT UINT32             *ReceiveFifoDepth,
> +  IN OUT UINT32             *Timeout,
> +  IN OUT EFI_PARITY_TYPE    *Parity,
> +  IN OUT UINT8              *DataBits,
> +  IN OUT EFI_STOP_BITS_TYPE *StopBits
> +  )
> +{
> +  return RETURN_UNSUPPORTED;
> +}
> +
> diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
> b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
> new file mode 100644
> index 000000000000..3df6a0c736a5
> --- /dev/null
> +++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
> @@ -0,0 +1,30 @@
> +## @file
> +#  Component description file for CbSerialPortLib library.
> +#
> +#  Copyright (c) 2022, Baruch Binyamin Doron
> +#  SPDX-License-Identifier: BSD-2-Clause-Patent
> +#
> +##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = CbSerialPortLib
> +  FILE_GUID                      = 0DB3EF12-1426-4086-B012-113184C4CE11
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  # Recall that debug logging can be unsafe to core. Route over RSC.
> +  LIBRARY_CLASS                  = SerialPortLib
> +  CONSTRUCTOR                    = SerialPortInitialize
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MdeModulePkg/MdeModulePkg.dec
> +  UefiPayloadPkg/UefiPayloadPkg.dec
> +
> +[LibraryClasses]
> +  BaseMemoryLib
> +  BlParseLib
> +
> +[Sources]
> +  CbSerialPortLib.c
> +
> diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc
> b/UefiPayloadPkg/UefiPayloadPkg.dsc
> index 4d9bbc80c866..0e4248767756 100644
> --- a/UefiPayloadPkg/UefiPayloadPkg.dsc
> +++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
> @@ -37,6 +37,7 @@
>    DEFINE ABOVE_4G_MEMORY              = TRUE
>    DEFINE BOOT_MANAGER_ESCAPE          = FALSE
>    DEFINE SD_MMC_TIMEOUT               = 1000000
> +  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE
>    #
>    # SBL:      UEFI payload for Slim Bootloader
>    # COREBOOT: UEFI payload for coreboot
> @@ -121,10 +122,11 @@
>
>  [BuildOptions]
>    *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES
> -  GCC:*_UNIXGCC_*_CC_FLAGS       = -DMDEPKG_NDEBUG
> +!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
>    GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
>    INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG
>    MSFT:RELEASE_*_*_CC_FLAGS      = /D MDEPKG_NDEBUG
> +!endif
>
>  [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
>    GCC:*_*_*_DLINK_FLAGS      = -z common-page-size=0x1000
> @@ -231,8 +233,13 @@
>    TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf
>  !endif
>    ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf
> +!if $(USE_CBMEM_FOR_CONSOLE) == TRUE
> +  SerialPortLib|UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
> +
> PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf
> +!else
>
>  SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf
>
>  PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf
> +!endif
>
>  PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf
>    IoApicLib|PcAtChipsetPkg/Library/BaseIoApicLib/BaseIoApicLib.inf
>
> @@ -422,10 +429,18 @@
>    gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa,
> 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66,
> 0x23, 0x31 }
>    gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x7
>    gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8000004F
> -!if $(SOURCE_DEBUG_ENABLE)
> -  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
> +!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
> +  !if $(SOURCE_DEBUG_ENABLE)
> +    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
> +  !else
> +    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
> +  !endif
>  !else
> -  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
> +  !if $(TARGET) == DEBUG
> +    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x07
> +  !else
> +    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03
> +  !endif
>  !endif
>
>  gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|$(MAX_SIZE_NON_POPULATE_CAPSULE)
>    #
> @@ -471,7 +486,7 @@
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize
> -!if $(TARGET) == DEBUG
> +!if ($(TARGET) == DEBUG || $(USE_CBMEM_FOR_CONSOLE) == TRUE)
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE
>  !else
>    gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
> --
> 2.36.1
>
>

[-- Attachment #2: Type: text/html, Size: 19229 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
  2022-05-26  9:16 ` Sean Rhodes
@ 2022-05-26 17:08   ` Benjamin Doron
  0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Doron @ 2022-05-26 17:08 UTC (permalink / raw)
  To: Sean Rhodes, devel

[-- Attachment #1: Type: text/plain, Size: 995 bytes --]

There actually is a possibility that even one message would overflow the buffer. Unlike the other debug library stack I wrote (which I might work on upstreaming during GSoC), where buffers are specified in kilobyte multiples, the user specifies CBMEM console size as a number of bytes. Some of this code, such as the comment "So, no chance that one message could overflow even the smallest buffer", is based on my work there. If the user sets the size to less than 512 (the size of the largest message that DebugLib instances might route here), the buffer will overflow. Is another sanity check needed, or is this possibility so much of an edge-case that it's unnecessary?

I know that UefiPayloadPkg gained SMM support recently, but we're actually safe from confused-deputy attacks because SMM uses the null instance of ReportStatusCodeLib, so no messages are routed here. (Aside: While SMM debug is desirable and could possibly be implemented safely, I don't consider it production-ready.)

[-- Attachment #2: Type: text/html, Size: 1003 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
  2022-05-22 18:47 [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console Benjamin Doron
  2022-05-26  9:16 ` Sean Rhodes
@ 2022-05-27  5:59 ` Guo Dong
  2022-05-27 16:33   ` Benjamin Doron
  1 sibling, 1 reply; 6+ messages in thread
From: Guo Dong @ 2022-05-27  5:59 UTC (permalink / raw)
  To: devel@edk2.groups.io, benjamin.doron00@gmail.com
  Cc: Ni, Ray, Maurice Ma, You, Benjamin, Rhodes, Sean


It would be great if you could add some comments to the newly added cbmem. It looks cb_cbmem_ref size was not used in this patch. Is it expected?
+struct cb_cbmem_ref {
+  UINT32 tag;
+  UINT32 size;
+  UINT64 cbmem_addr;
+};
+
+struct cbmem_console {
+  UINT32 size;
+  UINT32 cursor;
+  UINT8  body[0];
+} __attribute__((packed));
+

Rename gCbConsole to mCbConsole since this global variable is only used in this module.
+STATIC struct cbmem_console  *gCbConsole = NULL;

Thanks,
Guo
-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Benjamin Doron
Sent: Sunday, May 22, 2022 11:47 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Maurice Ma <maurice.ma@intel.com>; You, Benjamin <benjamin.you@intel.com>; Rhodes, Sean <sean@starlabs.systems>
Subject: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console

Writes TianoCore debug logs into the CBMEM console ringbuffer, from where the user can retrieve them with the `cbmem` userspace utility.

The intention is to aid in debugging non-fatal issues even in release builds, or simply make TianoCore's logs available to those interested.
Consequently, MDEPKG_NDEBUG must be masked. As an in-memory debug logging library, ASSERTs must be non-fatal to be seen, so they neither dead-loop nor create a breakpoint. It is assumed that ASSERT() neither enforces fatal conditions nor security integrity, as release builds do not call DebugAssert() from the ASSERT macro.

More detailed debug logs are produced with the DEBUG_CODE macro, but this guards other debug-related code throughout the codebase. To avoid changing behaviour on release builds, this is only set for debug builds.

Tested on QEMU, dumping the appropriate memory region in the UEFI shell shows the TianoCore log. An improved revision of the debug library used in several coreboot-related EDK2 forks, including MrChromebox's.
Previous revisions also tested on an Acer Aspire VN7-572G laptop.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 UefiPayloadPkg/Include/Coreboot.h                          |  12 +
 UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c   | 258 ++++++++++++++++++++
 UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf |  30 +++
 UefiPayloadPkg/UefiPayloadPkg.dsc                          |  25 +-
 4 files changed, 320 insertions(+), 5 deletions(-)

diff --git a/UefiPayloadPkg/Include/Coreboot.h b/UefiPayloadPkg/Include/Coreboot.h
index a3e1109fe84e..4c9cf965519b 100644
--- a/UefiPayloadPkg/Include/Coreboot.h
+++ b/UefiPayloadPkg/Include/Coreboot.h
@@ -199,6 +199,12 @@ struct cb_forward {
   UINT64    forward; }; +struct cb_cbmem_ref {+  UINT32 tag;+  UINT32 size;+  UINT64 cbmem_addr;+};+ #define CB_TAG_FRAMEBUFFER  0x0012 struct cb_framebuffer {   UINT32    tag;@@ -229,6 +235,12 @@ struct cb_vdat {
  #define CB_TAG_TIMESTAMPS     0x0016 #define CB_TAG_CBMEM_CONSOLE  0x0017+struct cbmem_console {+  UINT32 size;+  UINT32 cursor;+  UINT8  body[0];+} __attribute__((packed));+ #define CB_TAG_MRC_CACHE      0x0018 struct cb_cbmem_tab {   UINT32    tag;diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
new file mode 100644
index 000000000000..f7c06b0f41e9
--- /dev/null
+++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
@@ -0,0 +1,258 @@
+/** @file+  CBMEM console SerialPortLib instance++  Copyright (c) 2022, Baruch Binyamin Doron+  SPDX-License-Identifier: BSD-2-Clause-Patent++**/++#include <Base.h>+#include <Coreboot.h>++#include <Library/BaseMemoryLib.h>+#include <Library/BlParseLib.h>+#include <Library/SerialPortLib.h>++// Upper nibble contains flags+#define CBMC_CURSOR_MASK ((1 << 28) - 1)+#define CBMC_OVERFLOW (1 << 31)++STATIC struct cbmem_console  *gCbConsole = NULL;++/**+  Find coreboot record with given Tag.+  NOTE: This coreboot-specific function definition is absent+         from the common BlParseLib header.++  @param  Tag                The tag id to be found++  @retval NULL              The Tag is not found.+  @retval Others            The pointer to the record found.++**/+VOID *+FindCbTag (+  IN  UINT32         Tag+  );++/**+  Initialize the serial device hardware.++  If no initialization is required, then return RETURN_SUCCESS.+  If the serial device was successfully initialized, then return RETURN_SUCCESS.+  If the serial device could not be initialized, then return RETURN_DEVICE_ERROR.++  @retval RETURN_SUCCESS        The serial device was initialized.+  @retval RETURN_DEVICE_ERROR   The serial device could not be initialized.++**/+RETURN_STATUS+EFIAPI+SerialPortInitialize (+  VOID+  )+{+  struct cb_cbmem_ref *cbref = FindCbTag(CB_TAG_CBMEM_CONSOLE);+  if (!cbref) {+    return RETURN_DEVICE_ERROR;+  }++  gCbConsole = (VOID *)(UINTN)cbref->cbmem_addr;  // Support PEI and DXE+  if (gCbConsole == NULL) {+    return RETURN_DEVICE_ERROR;+  }++  return RETURN_SUCCESS;+}++/**+  Write data from buffer to serial device.++  Writes NumberOfBytes data bytes from Buffer to the serial device.+  The number of bytes actually written to the serial device is returned.+  If the return value is less than NumberOfBytes, then the write operation failed.+  If Buffer is NULL, then ASSERT().+  If NumberOfBytes is zero, then return 0.++  @param  Buffer           Pointer to the data buffer to be written.+  @param  NumberOfBytes    Number of bytes to written to the serial device.++  @retval 0                NumberOfBytes is 0.+  @retval >0               The number of bytes written to the serial device.+                           If this value is less than NumberOfBytes, then the write operation failed.++**/+UINTN+EFIAPI+SerialPortWrite (+  IN UINT8     *Buffer,+  IN UINTN     NumberOfBytes+  )+{+  UINT32            cursor;+  UINT32            flags;++  if (Buffer == NULL || NumberOfBytes == 0) {+    return 0;+  }++  if (!gCbConsole) {+    return 0;+  }++  cursor = gCbConsole->cursor & CBMC_CURSOR_MASK;+  flags = gCbConsole->cursor & ~CBMC_CURSOR_MASK;+  if (cursor >= gCbConsole->size) {+    // Already overflowed; bail out. TODO: Is this unnecessarily cautious?+    // - Supports old coreboot version with legacy overflow mechanism.+    return 0;+  }++  if (cursor + NumberOfBytes > gCbConsole->size) {+    // Will overflow, zero cursor and set flag.+    cursor = 0;+    flags |= CBMC_OVERFLOW;+  }++  // Longest debug messages supported by a DebugLib seems to be CHAR16[0x100] == 512.+  // So, no chance that one message could overflow even the smallest buffer.+  CopyMem (&gCbConsole->body[cursor], Buffer, NumberOfBytes);+  cursor += NumberOfBytes;++  if (cursor == gCbConsole->size) {+    // Next message will overflow, zero cursor.+    // - But not overflowed yet. Do not set flag.+    cursor = 0;+  }++  gCbConsole->cursor = flags | cursor;++  return NumberOfBytes;+}++/**+  Read data from serial device and save the datas in buffer.++  Reads NumberOfBytes data bytes from a serial device into the buffer+  specified by Buffer. The number of bytes actually read is returned.+  If Buffer is NULL, then ASSERT().+  If NumberOfBytes is zero, then return 0.++  @param  Buffer           Pointer to the data buffer to store the data read from the serial device.+  @param  NumberOfBytes    Number of bytes which will be read.++  @retval 0                Read data failed, no data is to be read.+  @retval >0               Actual number of bytes read from serial device.++**/+UINTN+EFIAPI+SerialPortRead (+  OUT UINT8     *Buffer,+  IN  UINTN     NumberOfBytes+)+{+  return 0;+}++/**+  Polls a serial device to see if there is any data waiting to be read.++  @retval TRUE             Data is waiting to be read from the serial device.+  @retval FALSE            There is no data waiting to be read from the serial device.++**/+BOOLEAN+EFIAPI+SerialPortPoll (+  VOID+  )+{+  return FALSE;+}++/**+  Sets the control bits on a serial device.++  @param Control                Sets the bits of Control that are settable.++  @retval RETURN_SUCCESS        The new control bits were set on the serial device.+  @retval RETURN_UNSUPPORTED    The serial device does not support this operation.+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning correctly.++**/+RETURN_STATUS+EFIAPI+SerialPortSetControl (+  IN UINT32 Control+  )+{+  return RETURN_UNSUPPORTED;+}++/**+  Retrieve the status of the control bits on a serial device.++  @param Control                A pointer to return the current control signals from the serial device.++  @retval RETURN_SUCCESS        The control bits were read from the serial device.+  @retval RETURN_UNSUPPORTED    The serial device does not support this operation.+  @retval RETURN_DEVICE_ERROR   The serial device is not functioning correctly.++**/+RETURN_STATUS+EFIAPI+SerialPortGetControl (+  OUT UINT32 *Control+  )+{+  return RETURN_UNSUPPORTED;+}++/**+  Sets the baud rate, receive FIFO depth, transmit/receive time out, parity,+  data bits, and stop bits on a serial device.++  @param BaudRate           The requested baud rate. A BaudRate value of 0 will use the+                            device's default interface speed.+                            On output, the value actually set.+  @param ReceiveFifoDepth   The requested depth of the FIFO on the receive side of the+                            serial interface. A ReceiveFifoDepth value of 0 will use+                            the device's default FIFO depth.+                            On output, the value actually set.+  @param Timeout            The requested time out for a single character in microseconds.+                            This timeout applies to both the transmit and receive side of the+                            interface. A Timeout value of 0 will use the device's default time+                            out value.+                            On output, the value actually set.+  @param Parity             The type of parity to use on this serial device. A Parity value of+                            DefaultParity will use the device's default parity value.+                            On output, the value actually set.+  @param DataBits           The number of data bits to use on the serial device. A DataBits+                            value of 0 will use the device's default data bit setting.+                            On output, the value actually set.+  @param StopBits           The number of stop bits to use on this serial device. A StopBits+                            value of DefaultStopBits will use the device's default number of+                            stop bits.+                            On output, the value actually set.++  @retval RETURN_SUCCESS            The new attributes were set on the serial device.+  @retval RETURN_UNSUPPORTED        The serial device does not support this operation.+  @retval RETURN_INVALID_PARAMETER  One or more of the attributes has an unsupported value.+  @retval RETURN_DEVICE_ERROR       The serial device is not functioning correctly.++**/+RETURN_STATUS+EFIAPI+SerialPortSetAttributes (+  IN OUT UINT64             *BaudRate,+  IN OUT UINT32             *ReceiveFifoDepth,+  IN OUT UINT32             *Timeout,+  IN OUT EFI_PARITY_TYPE    *Parity,+  IN OUT UINT8              *DataBits,+  IN OUT EFI_STOP_BITS_TYPE *StopBits+  )+{+  return RETURN_UNSUPPORTED;+}+diff --git a/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
new file mode 100644
index 000000000000..3df6a0c736a5
--- /dev/null
+++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf
@@ -0,0 +1,30 @@
+## @file+#  Component description file for CbSerialPortLib library.+#+#  Copyright (c) 2022, Baruch Binyamin Doron+#  SPDX-License-Identifier: BSD-2-Clause-Patent+#+##++[Defines]+  INF_VERSION                    = 0x00010005+  BASE_NAME                      = CbSerialPortLib+  FILE_GUID                      = 0DB3EF12-1426-4086-B012-113184C4CE11+  MODULE_TYPE                    = BASE+  VERSION_STRING                 = 1.0+  # Recall that debug logging can be unsafe to core. Route over RSC.+  LIBRARY_CLASS                  = SerialPortLib+  CONSTRUCTOR                    = SerialPortInitialize++[Packages]+  MdePkg/MdePkg.dec+  MdeModulePkg/MdeModulePkg.dec+  UefiPayloadPkg/UefiPayloadPkg.dec++[LibraryClasses]+  BaseMemoryLib+  BlParseLib++[Sources]+  CbSerialPortLib.c+diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 4d9bbc80c866..0e4248767756 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -37,6 +37,7 @@
   DEFINE ABOVE_4G_MEMORY              = TRUE   DEFINE BOOT_MANAGER_ESCAPE          = FALSE   DEFINE SD_MMC_TIMEOUT               = 1000000+  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE   #   # SBL:      UEFI payload for Slim Bootloader   # COREBOOT: UEFI payload for coreboot@@ -121,10 +122,11 @@
  [BuildOptions]   *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES-  GCC:*_UNIXGCC_*_CC_FLAGS       = -DMDEPKG_NDEBUG+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE   GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG   INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG   MSFT:RELEASE_*_*_CC_FLAGS      = /D MDEPKG_NDEBUG+!endif  [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]   GCC:*_*_*_DLINK_FLAGS      = -z common-page-size=0x1000@@ -231,8 +233,13 @@
   TimerLib|UefiPayloadPkg/Library/AcpiTimerLib/AcpiTimerLib.inf !endif   ResetSystemLib|UefiPayloadPkg/Library/ResetSystemLib/ResetSystemLib.inf+!if $(USE_CBMEM_FOR_CONSOLE) == TRUE+  SerialPortLib|UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf+  PlatformHookLib|MdeModulePkg/Library/BasePlatformHookLibNull/BasePlatformHookLibNull.inf+!else   SerialPortLib|MdeModulePkg/Library/BaseSerialPortLib16550/BaseSerialPortLib16550.inf   PlatformHookLib|UefiPayloadPkg/Library/PlatformHookLib/PlatformHookLib.inf+!endif   PlatformBootManagerLib|UefiPayloadPkg/Library/PlatformBootManagerLib/PlatformBootManagerLib.inf   IoApicLib|PcAtChipsetPkg/Library/BaseIoApicLib/BaseIoApicLib.inf @@ -422,10 +429,18 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdBootManagerMenuFile|{ 0x21, 0xaa, 0x2c, 0x46, 0x14, 0x76, 0x03, 0x45, 0x83, 0x6e, 0x8a, 0xb6, 0xf4, 0x66, 0x23, 0x31 }   gEfiMdePkgTokenSpaceGuid.PcdReportStatusCodePropertyMask|0x7   gEfiMdePkgTokenSpaceGuid.PcdDebugPrintErrorLevel|0x8000004F-!if $(SOURCE_DEBUG_ENABLE)-  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE+  !if $(SOURCE_DEBUG_ENABLE)+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17+  !else+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F+  !endif !else-  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F+  !if $(TARGET) == DEBUG+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x07+  !else+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03+  !endif !endif   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|$(MAX_SIZE_NON_POPULATE_CAPSULE)   #@@ -471,7 +486,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize-!if $(TARGET) == DEBUG+!if ($(TARGET) == DEBUG || $(USE_CBMEM_FOR_CONSOLE) == TRUE)   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE !else   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE-- 
2.36.1



-=-=-=-=-=-=
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89944): https://edk2.groups.io/g/devel/message/89944
Mute This Topic: https://groups.io/mt/91273919/1781375
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [guo.dong@intel.com] -=-=-=-=-=-=



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
  2022-05-27  5:59 ` Guo Dong
@ 2022-05-27 16:33   ` Benjamin Doron
  2022-05-27 17:08     ` Guo Dong
  0 siblings, 1 reply; 6+ messages in thread
From: Benjamin Doron @ 2022-05-27 16:33 UTC (permalink / raw)
  To: Guo Dong, devel

[-- Attachment #1: Type: text/plain, Size: 748 bytes --]

Hi Guo,
Dumping some of the coreboot table shows that some smaller entries/data (such as build information) are inserted inline while larger entries use the cb_cbmem_ref structure. This may answer why some users of FindCbTag() assign the returned pointer directly to their requested structure, while this library must first dereference the console's address.

For reasons unclear to me, cb_cbmem_ref->size == 0x0010, the size of the cb_cbmem_ref structure. Perhaps the structure of cb_cbmem_ref has been changed in the past and the size field informs coreboot payloads? Either way, it doesn't look like a useful field at the moment.

I will add comments and submit a v3. Is there anything else that needs some comments?

Regards,
Benjamin

[-- Attachment #2: Type: text/html, Size: 783 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console
  2022-05-27 16:33   ` Benjamin Doron
@ 2022-05-27 17:08     ` Guo Dong
  0 siblings, 0 replies; 6+ messages in thread
From: Guo Dong @ 2022-05-27 17:08 UTC (permalink / raw)
  To: Benjamin Doron, devel@edk2.groups.io

[-- Attachment #1: Type: text/plain, Size: 1073 bytes --]


Thanks Benjamin for the explaining. I have no other comments.

Thanks,
Guo

From: Benjamin Doron <benjamin.doron00@gmail.com>
Sent: Friday, May 27, 2022 9:33 AM
To: Dong, Guo <guo.dong@intel.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console

Hi Guo,
Dumping some of the coreboot table shows that some smaller entries/data (such as build information) are inserted inline while larger entries use the cb_cbmem_ref structure. This may answer why some users of FindCbTag() assign the returned pointer directly to their requested structure, while this library must first dereference the console's address.

For reasons unclear to me, cb_cbmem_ref->size == 0x0010, the size of the cb_cbmem_ref structure. Perhaps the structure of cb_cbmem_ref has been changed in the past and the size field informs coreboot payloads? Either way, it doesn't look like a useful field at the moment.

I will add comments and submit a v3. Is there anything else that needs some comments?

Regards,
Benjamin

[-- Attachment #2: Type: text/html, Size: 3152 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-05-27 17:08 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-05-22 18:47 [PATCH v2] UefiPayloadPkg: Add support for logging to CBMEM console Benjamin Doron
2022-05-26  9:16 ` Sean Rhodes
2022-05-26 17:08   ` [edk2-devel] " Benjamin Doron
2022-05-27  5:59 ` Guo Dong
2022-05-27 16:33   ` Benjamin Doron
2022-05-27 17:08     ` Guo Dong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox