Reviewed-by: Sean Rhodes <sean@starlabs.systems>
On Wed, 8 Jun 2022 at 16:05, Lean Sheng Tan <sheng....@9elements.com> wrote:
> Reviewed-by: Lean Sheng Tan <sheng....@9elements.com>
>
>
>
> On Mon, 6 Jun 2022 at 02:56, Benjamin Doron <benjamin.doro...@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.d...@intel.com>
>> Cc: Ray Ni <ray...@intel.com>
>> Cc: Maurice Ma <maurice...@intel.com>
>> Cc: Benjamin You <benjamin....@intel.com>
>> Cc: Sean Rhodes <sean@starlabs.systems>
>> Signed-off-by: Benjamin Doron <benjamin.doro...@gmail.com>
>> ---
>> UefiPayloadPkg/Include/Coreboot.h | 13 +
>> UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c | 264
>> ++++++++++++++++++++
>> UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.inf | 30 +++
>> UefiPayloadPkg/UefiPayloadPkg.dsc | 25 +-
>> 4 files changed, 327 insertions(+), 5 deletions(-)
>>
>> diff --git a/UefiPayloadPkg/Include/Coreboot.h
>> b/UefiPayloadPkg/Include/Coreboot.h
>> index a3e1109fe84e..5892fae7682b 100644
>> --- a/UefiPayloadPkg/Include/Coreboot.h
>> +++ b/UefiPayloadPkg/Include/Coreboot.h
>> @@ -199,6 +199,13 @@ struct cb_forward {
>> UINT64 forward;
>> };
>>
>> +struct cb_cbmem_ref {
>> + UINT32 tag;
>> + // Field contains size of this struct = 0x0010
>> + UINT32 size;
>> + UINT64 cbmem_addr;
>> +};
>> +
>> #define CB_TAG_FRAMEBUFFER 0x0012
>> struct cb_framebuffer {
>> UINT32 tag;
>> @@ -229,6 +236,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..863fbcfef2f0
>> --- /dev/null
>> +++ b/UefiPayloadPkg/Library/CbSerialPortLib/CbSerialPortLib.c
>> @@ -0,0 +1,264 @@
>> +/** @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 *mCbConsole = 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
>> + )
>> +{
>> + // The coreboot table contains large structures as references
>> + struct cb_cbmem_ref *cbref = FindCbTag(CB_TAG_CBMEM_CONSOLE);
>> + if (!cbref) {
>> + return RETURN_DEVICE_ERROR;
>> + }
>> +
>> + mCbConsole = (VOID *)(UINTN)cbref->cbmem_addr; // Support PEI and DXE
>> + if (mCbConsole == 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 (!mCbConsole) {
>> + return 0;
>> + }
>> +
>> + cursor = mCbConsole->cursor & CBMC_CURSOR_MASK;
>> + flags = mCbConsole->cursor & ~CBMC_CURSOR_MASK;
>> + if (cursor >= mCbConsole->size) {
>> + // Already overflowed; bail out. TODO: Is this unnecessarily
>> cautious?
>> + // - Supports old coreboot version with legacy overflow mechanism.
>> + return 0;
>> + }
>> +
>> + if (cursor + NumberOfBytes > mCbConsole->size) {
>> + // Will overflow, zero cursor and set flag.
>> + cursor = 0;
>> + flags |= CBMC_OVERFLOW;
>> + }
>> +
>> + if (NumberOfBytes > mCbConsole->size) {
>> + // This one debug message is longer than the entire buffer. Truncate
>> it.
>> + // - TODO: Is this unnecessarily cautious?
>> + NumberOfBytes = mCbConsole->size;
>> + }
>> +
>> + CopyMem (&mCbConsole->body[cursor], Buffer, NumberOfBytes);
>> + cursor += NumberOfBytes;
>> +
>> + if (cursor == mCbConsole->size) {
>> + // Next message will overflow, zero cursor.
>> + // - Set flag preemptively. This could not be determined later.
>> + cursor = 0;
>> + flags |= CBMC_OVERFLOW;
>> + }
>> +
>> + mCbConsole->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 (#90214): https://edk2.groups.io/g/devel/message/90214
>> Mute This Topic: https://groups.io/mt/91568694/6757431
>> Group Owner: devel+ow...@edk2.groups.io
>> Unsubscribe: https://edk2.groups.io/g/devel/unsub [
>> sheng....@9elements.com]
>> ------------
>>
>>
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#90382): https://edk2.groups.io/g/devel/message/90382
Mute This Topic: https://groups.io/mt/91568694/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
