[PATCH] UefiPayloadPkg: Use clearer debug flags with more flexibility

[PATCH] UefiPayloadPkg: Use clearer debug flags with more flexibility

[Benjamin Doron]

The default behaviour is that RELEASE builds have all DebugLib macros
removed by MDEPKG_NDEBUG. Currently, UefiPayloadPkg has been using
USE_CBMEM_FOR_CONSOLE to disable this behaviour, but it's unclear that
this option changes DEBUG build behaviour too.

Therefore, add a new build flag, RELEASE_LOGGING, that can be used
to set the logging behaviour. The USE_CBMEM_FOR_CONSOLE flag only
selects the library and options specific to it. For instance, logs can
only be retrieved when the boot completes, so do not produce a CPU
breakpoint or deadloop in this case.

There are also cases where the platform builder may want behaviour or
information from the DEBUG_CODE macros. Therefore, separate this into
another build flag.

Bitwise ORs enable this flexibility.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: James Lu <james.lu@intel.com>
Cc: Gua Guo <gua.guo@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 44 +++++++++++++++++++------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 998d22290922..360ea71b149d 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -26,7 +26,6 @@
   FLASH_DEFINITION                    = UefiPayloadPkg/UefiPayloadPkg.fdf
   PCD_DYNAMIC_AS_DYNAMICEX            = TRUE
 
-  DEFINE SOURCE_DEBUG_ENABLE          = FALSE
   DEFINE PS2_KEYBOARD_ENABLE          = FALSE
   DEFINE RAM_DISK_ENABLE              = FALSE
   DEFINE SIO_BUS_ENABLE               = FALSE
@@ -40,7 +39,6 @@
   DEFINE PS2_MOUSE_ENABLE             = TRUE
   DEFINE CRYPTO_PROTOCOL_SUPPORT      = FALSE
   DEFINE SD_MMC_TIMEOUT               = 1000000
-  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE
   DEFINE BOOTSPLASH_IMAGE             = FALSE
   DEFINE NVME_ENABLE                  = TRUE
 
@@ -101,6 +99,15 @@
   #                                       [Vendor]   [Device]  [----ClockRate---]  [------------Offset-----------] [Bar] [Stride] [RxFifo] [TxFifo]   [Rsvd]   [Vendor]
   DEFINE PCI_SERIAL_PARAMETERS        = {0xff,0xff, 0x00,0x00, 0x0,0x20,0x1c,0x00, 0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0, 0x00,    0x01, 0x0,0x0, 0x0,0x0, 0x0,0x0, 0xff,0xff}
 
+  #
+  # Debug options
+  #
+  DEFINE RELEASE_LOGGING              = FALSE
+  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE
+  DEFINE ENABLE_DEBUG_CODE            = FALSE
+
+  DEFINE SOURCE_DEBUG_ENABLE          = FALSE
+
   #
   # Shell options: [BUILD_SHELL, MIN_BIN, NONE, UEFI_BIN]
   #
@@ -135,7 +142,7 @@
 
 [BuildOptions]
   *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES
-!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
+!if $(RELEASE_LOGGING) == FALSE
   GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG
   INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG
   MSFT:RELEASE_*_*_CC_FLAGS      = /D MDEPKG_NDEBUG
@@ -436,6 +443,21 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE
 
+  #
+  # Build the PcdDebugPropertyMask from build flags
+  #
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03
+!if $(ENABLE_DEBUG_CODE) == TRUE
+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x04)
+!endif
+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE
+  !if $(SOURCE_DEBUG_ENABLE) == TRUE
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x10)
+  !else
+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x28)
+  !endif
+!endif
+
   gUefiPayloadPkgTokenSpaceGuid.PcdPcdDriverFile|{ 0x57, 0x72, 0xcf, 0x80, 0xab, 0x87, 0xf9, 0x47, 0xa3, 0xfe, 0xD5, 0x0B, 0x76, 0xd8, 0x95, 0x41 }
 
 !if $(SOURCE_DEBUG_ENABLE)
@@ -484,20 +506,8 @@
   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 $(USE_CBMEM_FOR_CONSOLE) == FALSE
-  !if $(SOURCE_DEBUG_ENABLE)
-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17
-  !else
-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F
-  !endif
-!else
-  !if $(TARGET) == DEBUG
-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x07
-  !else
-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03
-  !endif
-!endif
   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|$(MAX_SIZE_NON_POPULATE_CAPSULE)
+
   #
   # The following parameters are set by Library/PlatformHookLib
   #
@@ -541,7 +551,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode|1
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0
   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize|0x0
-!if ($(TARGET) == DEBUG || $(USE_CBMEM_FOR_CONSOLE) == TRUE)
+!if ($(TARGET) == DEBUG || $(RELEASE_LOGGING) == TRUE)
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE
 !else
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE
-- 
2.39.2

Re: [PATCH] UefiPayloadPkg: Use clearer debug flags with more flexibility

[Guo, Gua]

Could you also create Edk2 PR ?

-----Original Message-----
From: Benjamin Doron <benjamin.doron00@gmail.com> 
Sent: Friday, May 5, 2023 6:36 AM
To: devel@edk2.groups.io
Cc: Dong, Guo <guo.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Rhodes, Sean <sean@starlabs.systems>; Lu, James <james.lu@intel.com>; Guo, Gua <gua.guo@intel.com>
Subject: [PATCH] UefiPayloadPkg: Use clearer debug flags with more flexibility

The default behaviour is that RELEASE builds have all DebugLib macros removed by MDEPKG_NDEBUG. Currently, UefiPayloadPkg has been using USE_CBMEM_FOR_CONSOLE to disable this behaviour, but it's unclear that this option changes DEBUG build behaviour too.

Therefore, add a new build flag, RELEASE_LOGGING, that can be used to set the logging behaviour. The USE_CBMEM_FOR_CONSOLE flag only selects the library and options specific to it. For instance, logs can only be retrieved when the boot completes, so do not produce a CPU breakpoint or deadloop in this case.

There are also cases where the platform builder may want behaviour or information from the DEBUG_CODE macros. Therefore, separate this into another build flag.

Bitwise ORs enable this flexibility.

Cc: Guo Dong <guo.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Sean Rhodes <sean@starlabs.systems>
Cc: James Lu <james.lu@intel.com>
Cc: Gua Guo <gua.guo@intel.com>
Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
---
 UefiPayloadPkg/UefiPayloadPkg.dsc | 44 +++++++++++++++++++------------
 1 file changed, 27 insertions(+), 17 deletions(-)

diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 998d22290922..360ea71b149d 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -26,7 +26,6 @@
   FLASH_DEFINITION                    = UefiPayloadPkg/UefiPayloadPkg.fdf   PCD_DYNAMIC_AS_DYNAMICEX            = TRUE -  DEFINE SOURCE_DEBUG_ENABLE          = FALSE   DEFINE PS2_KEYBOARD_ENABLE          = FALSE   DEFINE RAM_DISK_ENABLE              = FALSE   DEFINE SIO_BUS_ENABLE               = FALSE@@ -40,7 +39,6 @@
   DEFINE PS2_MOUSE_ENABLE             = TRUE   DEFINE CRYPTO_PROTOCOL_SUPPORT      = FALSE   DEFINE SD_MMC_TIMEOUT               = 1000000-  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE   DEFINE BOOTSPLASH_IMAGE             = FALSE   DEFINE NVME_ENABLE                  = TRUE @@ -101,6 +99,15 @@
   #                                       [Vendor]   [Device]  [----ClockRate---]  [------------Offset-----------] [Bar] [Stride] [RxFifo] [TxFifo]   [Rsvd]   [Vendor]   DEFINE PCI_SERIAL_PARAMETERS        = {0xff,0xff, 0x00,0x00, 0x0,0x20,0x1c,0x00, 0x0,0x0,0x0,0x0,0x0,0x0,0x0,0x0, 0x00,    0x01, 0x0,0x0, 0x0,0x0, 0x0,0x0, 0xff,0xff} +  #+  # Debug options+  #+  DEFINE RELEASE_LOGGING              = FALSE+  DEFINE USE_CBMEM_FOR_CONSOLE        = FALSE+  DEFINE ENABLE_DEBUG_CODE            = FALSE++  DEFINE SOURCE_DEBUG_ENABLE          = FALSE+   #   # Shell options: [BUILD_SHELL, MIN_BIN, NONE, UEFI_BIN]   #@@ -135,7 +142,7 @@
  [BuildOptions]   *_*_*_CC_FLAGS                 = -D DISABLE_NEW_DEPRECATED_INTERFACES-!if $(USE_CBMEM_FOR_CONSOLE) == FALSE+!if $(RELEASE_LOGGING) == FALSE   GCC:RELEASE_*_*_CC_FLAGS       = -DMDEPKG_NDEBUG   INTEL:RELEASE_*_*_CC_FLAGS     = /D MDEPKG_NDEBUG   MSFT:RELEASE_*_*_CC_FLAGS      = /D MDEPKG_NDEBUG@@ -436,6 +443,21 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseMemory|FALSE   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable|TRUE +  #+  # Build the PcdDebugPropertyMask from build flags+  #+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03+!if $(ENABLE_DEBUG_CODE) == TRUE+  gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x04)+!endif+!if $(USE_CBMEM_FOR_CONSOLE) == FALSE+  !if $(SOURCE_DEBUG_ENABLE) == TRUE+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x10)+  !else+    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|(gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask | 0x28)+  !endif+!endif+   gUefiPayloadPkgTokenSpaceGuid.PcdPcdDriverFile|{ 0x57, 0x72, 0xcf, 0x80, 0xab, 0x87, 0xf9, 0x47, 0xa3, 0xfe, 0xD5, 0x0B, 0x76, 0xd8, 0x95, 0x41 }  !if $(SOURCE_DEBUG_ENABLE)@@ -484,20 +506,8 @@
   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 $(USE_CBMEM_FOR_CONSOLE) == FALSE-  !if $(SOURCE_DEBUG_ENABLE)-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x17-  !else-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x2F-  !endif-!else-  !if $(TARGET) == DEBUG-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x07-  !else-    gEfiMdePkgTokenSpaceGuid.PcdDebugPropertyMask|0x03-  !endif-!endif   gEfiMdeModulePkgTokenSpaceGuid.PcdMaxSizeNonPopulateCapsule|$(MAX_SIZE_NON_POPULATE_CAPSULE)+   #   # The following parameters are set by Library/PlatformHookLib   #@@ -541,7 +551,7 @@
   gUefiCpuPkgTokenSpaceGuid.PcdCpuApLoopMode|1   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0   gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize|0x0-!if ($(TARGET) == DEBUG || $(USE_CBMEM_FOR_CONSOLE) == TRUE)+!if ($(TARGET) == DEBUG || $(RELEASE_LOGGING) == TRUE)   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|TRUE !else   gEfiMdeModulePkgTokenSpaceGuid.PcdStatusCodeUseSerial|FALSE-- 
2.39.2

Re: [edk2-devel] [PATCH] UefiPayloadPkg: Use clearer debug flags with more flexibility

[Benjamin Doron]

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

Sure. Done, see https://github.com/tianocore/edk2/pull/4337.

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