[PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Laszlo Ersek]

This series intends to solve the following BZs:

  <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
  the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
  files

  <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
  Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
  DSC files

Public branch:
<https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.

The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
following MdePkg library class APIs:

  BaseLib:
  - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
  - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
    AsciiStrToUnicodeStr.

  PcdLib:
  - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
  - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
    PcdSetExBool.

  UefiLib:
  - GetVariable, GetEfiGlobalVariable.

The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
into ArmPkg a little bit, due to dependencies.

I couldn't build-test some changes (for example, the only compiler
toolchains I have access to at the moment are GCC48 for Ia32/X64, and
GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
flag). For all of these, I liberally sprinkled the patches with Cc's and
Notes sections, asking for help. I did make an honest effort to build
the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
could think of, perusing the various !if directives in the DSC files.

I recommend the "--word-diff" option of git-show, as a tool to assist
with the review.

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>

Thanks
Laszlo

Laszlo Ersek (19):
  MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
  OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
  OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
  OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
  OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
  OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
  OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
  OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
  OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX()
    calls
  OvmfPkg: disable deprecated interfaces
  ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
  ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX()
    calls
  ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
  ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX()
    calls
  ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
  ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX()
    calls
  ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
  ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with
    AsciiStrCatS()
  ArmVirtPkg: disable deprecated interfaces

 ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c                     | 22 +++++-----
 ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c                   | 20 ++++-----
 ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c |  5 ++-
 ArmVirtPkg/ArmVirt.dsc.inc                                              |  6 +++
 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c                | 13 ++++--
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c  |  4 +-
 ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c                            | 18 ++++----
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c  | 13 ++++--
 ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c          | 10 +++--
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c                  |  6 ++-
 MdePkg/Include/Library/DebugLib.h                                       | 27 ++++++++++++
 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                  | 12 ++++--
 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c                    | 14 +++++--
 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c               | 10 +++--
 OvmfPkg/OvmfPkgIa32.dsc                                                 |  7 ++++
 OvmfPkg/OvmfPkgIa32X64.dsc                                              |  7 ++++
 OvmfPkg/OvmfPkgX64.dsc                                                  |  7 ++++
 OvmfPkg/PlatformDxe/Platform.c                                          |  8 +++-
 OvmfPkg/PlatformPei/MemDetect.c                                         | 11 +++--
 OvmfPkg/PlatformPei/Platform.c                                          | 44 +++++++++++++-------
 OvmfPkg/PlatformPei/Xen.c                                               |  5 ++-
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c                 | 13 ++++--
 OvmfPkg/XenBusDxe/XenBusDxe.inf                                         |  1 +
 OvmfPkg/XenBusDxe/XenStore.c                                            | 22 +++++-----
 24 files changed, 213 insertions(+), 92 deletions(-)

-- 
2.9.2

[PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Laszlo Ersek]

ASSERT_EFI_ERROR() cannot be used in BASE type modules because
- the replacement text calls EFI_ERROR(),
- EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
- the inclusion of "UefiBaseType.h" is not required for BASE type modules.

While

  ASSERT (!RETURN_ERROR (StatusParameter))

would be a functional statement in BASE type modules, it would be less
convenient and less informative: ASSERT_EFI_ERROR() prints the actual
StatusParameter.

Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
original macro definition and update it as follows:
- replace EFI with RETURN,
- wrap overlong lines in the comment block and in the code,
- EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.

Cc: Liming Gao <liming.gao@intel.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
    one such BASE module.

 MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
index 81904325703f..3a910e6a208b 100644
--- a/MdePkg/Include/Library/DebugLib.h
+++ b/MdePkg/Include/Library/DebugLib.h
@@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
   #define ASSERT_EFI_ERROR(StatusParameter)
 #endif
 
+/**
+  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
+
+  If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
+  bit of PcdDebugProperyMask is set, then this macro evaluates the
+  RETURN_STATUS value specified by StatusParameter.  If StatusParameter is an
+  error code, then DebugAssert() is called passing in the source filename,
+  source line number, and StatusParameter.
+
+  @param  StatusParameter  RETURN_STATUS value to evaluate.
+
+**/
+#if !defined(MDEPKG_NDEBUG)
+  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
+    do {                                                                \
+      if (DebugAssertEnabled ()) {                                      \
+        if (RETURN_ERROR (StatusParameter)) {                           \
+          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
+            StatusParameter));                                          \
+          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
+        }                                                               \
+      }                                                                 \
+    } while (FALSE)
+#else
+  #define ASSERT_RETURN_ERROR(StatusParameter)
+#endif
+
 /**  
   Macro that calls DebugAssert() if a protocol is already installed in the 
   handle database.
-- 
2.9.2

[PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls

[Laszlo Ersek]

AsciiStrCpy() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    not used in my setup, testing would be appreciated

 OvmfPkg/XenBusDxe/XenStore.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index fb00e7393bb1..aa3ff7d3017b 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -1298,11 +1298,8 @@ XenStoreTransactionEnd (
 {
   CHAR8 AbortStr[2];
 
-  if (Abort) {
-    AsciiStrCpy (AbortStr, "F");
-  } else {
-    AsciiStrCpy (AbortStr, "T");
-  }
+  AbortStr[0] = Abort ? 'F' : 'T';
+  AbortStr[1] = '\0';
 
   return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
 }
-- 
2.9.2

[PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls

[Laszlo Ersek]

AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Replace AsciiStrCat() with AsciiSPrint(). Spell out the (already existent)
PrintLib dependency in the INF file. Add an explicit ASSERT() to document
that XenStoreJoin() assumes that the pool allocation always succeeds.

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    not used in my setup, testing would be appreciated

 OvmfPkg/XenBusDxe/XenBusDxe.inf |  1 +
 OvmfPkg/XenBusDxe/XenStore.c    | 15 +++++++++------
 2 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
index f0c5db98b1f4..5ff1cd04840c 100644
--- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
+++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
@@ -56,6 +56,7 @@ [LibraryClasses]
   DebugLib
   XenHypercallLib
   SynchronizationLib
+  PrintLib
 
 [Protocols]
   gEfiDriverBindingProtocolGuid
diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
index aa3ff7d3017b..b7ae1d04863d 100644
--- a/OvmfPkg/XenBusDxe/XenStore.c
+++ b/OvmfPkg/XenBusDxe/XenStore.c
@@ -303,14 +303,17 @@ XenStoreJoin (
   )
 {
   CHAR8 *Buf;
+  UINTN BufSize;
 
   /* +1 for '/' and +1 for '\0' */
-  Buf = AllocateZeroPool (
-          AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2);
-  AsciiStrCat (Buf, DirectoryPath);
-  if (Node[0] != '\0') {
-    AsciiStrCat (Buf, "/");
-    AsciiStrCat (Buf, Node);
+  BufSize = AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2;
+  Buf = AllocatePool (BufSize);
+  ASSERT (Buf != NULL);
+
+  if (Node[0] == '\0') {
+    AsciiSPrint (Buf, BufSize, "%a", DirectoryPath);
+  } else {
+    AsciiSPrint (Buf, BufSize, "%a/%a", DirectoryPath, Node);
   }
 
   return Buf;
-- 
2.9.2

[PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    not used in my setup, testing would be appreciated

 OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
index 7a8beb3354b9..dec6d4af50df 100644
--- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
+++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
@@ -803,6 +803,7 @@ FvbInitialize (
   BOOLEAN                             Initialize;
   EFI_HANDLE                          Handle;
   EFI_PHYSICAL_ADDRESS                Address;
+  RETURN_STATUS                       PcdStatus;
 
   DEBUG ((EFI_D_INFO, "EMU Variable FVB Started\n"));
 
@@ -862,19 +863,24 @@ FvbInitialize (
     SetMem (Ptr, EMU_FVB_SIZE, ERASED_UINT8);
     InitializeFvAndVariableStoreHeaders (Ptr);
   }
-  PcdSet64 (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
+  PcdStatus = PcdSet64S (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // Initialize the Fault Tolerant Write data area
   //
   SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
-  PcdSet32 (PcdFlashNvStorageFtwWorkingBase, (UINT32)(UINTN) SubPtr);
+  PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
+                (UINT32)(UINTN) SubPtr);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // Initialize the Fault Tolerant Write spare block
   //
   SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
-  PcdSet32 (PcdFlashNvStorageFtwSpareBase, (UINT32)(UINTN) SubPtr);
+  PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
+                (UINT32)(UINTN) SubPtr);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // Setup FVB device path
-- 
2.9.2

[PATCH 05/19] OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    the emulated variable part of the patch is un-exercised on my setup,
    testing would be appreciated

 OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
index bd1a4396f1d3..29ce21282595 100644
--- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
+++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
@@ -353,8 +353,9 @@ Returns:
 
 --*/
 {
-  EFI_HANDLE Handle;
-  EFI_STATUS Status;
+  EFI_HANDLE    Handle;
+  EFI_STATUS    Status;
+  RETURN_STATUS PcdStatus;
 
   DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
   InstallDevicePathCallback ();
@@ -394,7 +395,9 @@ Returns:
   ASSERT_EFI_ERROR (Status);
 
   PlatformInitializeConsole (gPlatformConsole);
-  PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
+  PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
+                GetFrontPageTimeoutFromQemu ());
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   PlatformRegisterOptionsAndKeys ();
 }
@@ -1281,6 +1284,7 @@ VisitingFileSystemInstance (
 {
   EFI_STATUS      Status;
   STATIC BOOLEAN  ConnectedToFileSystem = FALSE;
+  RETURN_STATUS   PcdStatus;
 
   if (ConnectedToFileSystem) {
     return EFI_ALREADY_STARTED;
@@ -1300,7 +1304,9 @@ VisitingFileSystemInstance (
       NULL,
       &mEmuVariableEventReg
       );
-  PcdSet64 (PcdEmuVariableEvent, (UINT64)(UINTN) mEmuVariableEvent);
+  PcdStatus = PcdSet64S (PcdEmuVariableEvent,
+                (UINT64)(UINTN) mEmuVariableEvent);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   return EFI_SUCCESS;
 }
-- 
2.9.2

[PATCH 06/19] OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    tested with OVMF (smbios v2) and ArmVirtQemu (smbios v3)

 OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
index 950c3f7e0a31..58180412bd1a 100644
--- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
+++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
@@ -40,6 +40,7 @@ DetectSmbiosVersion (
   UINTN                AnchorSize, TablesSize;
   QEMU_SMBIOS_ANCHOR   QemuAnchor;
   UINT16               SmbiosVersion;
+  RETURN_STATUS        PcdStatus;
 
   if (PcdGetBool (PcdQemuSmbiosValidated)) {
     //
@@ -87,7 +88,8 @@ DetectSmbiosVersion (
 
     DEBUG ((EFI_D_INFO, "%a: SMBIOS 3.x DocRev from QEMU: 0x%02x\n",
       __FUNCTION__, QemuAnchor.V3.DocRev));
-    PcdSet8 (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
+    PcdStatus = PcdSet8S (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
+    ASSERT_RETURN_ERROR (PcdStatus);
     break;
 
   default:
@@ -96,12 +98,14 @@ DetectSmbiosVersion (
 
   DEBUG ((EFI_D_INFO, "%a: SMBIOS version from QEMU: 0x%04x\n", __FUNCTION__,
     SmbiosVersion));
-  PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
+  PcdStatus = PcdSet16S (PcdSmbiosVersion, SmbiosVersion);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // SMBIOS platform drivers can now fetch and install
   // "etc/smbios/smbios-tables" from QEMU.
   //
-  PcdSetBool (PcdQemuSmbiosValidated, TRUE);
+  PcdStatus = PcdSetBoolS (PcdQemuSmbiosValidated, TRUE);
+  ASSERT_RETURN_ERROR (PcdStatus);
   return RETURN_SUCCESS;
 }
-- 
2.9.2

[PATCH 07/19] OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformDxe/Platform.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/PlatformDxe/Platform.c b/OvmfPkg/PlatformDxe/Platform.c
index 4ec327e763f6..126d8e73823b 100644
--- a/OvmfPkg/PlatformDxe/Platform.c
+++ b/OvmfPkg/PlatformDxe/Platform.c
@@ -663,6 +663,7 @@ ExecutePlatformConfig (
   EFI_STATUS      Status;
   PLATFORM_CONFIG PlatformConfig;
   UINT64          OptionalElements;
+  RETURN_STATUS   PcdStatus;
 
   Status = PlatformConfigLoad (&PlatformConfig, &OptionalElements);
   if (EFI_ERROR (Status)) {
@@ -675,10 +676,13 @@ ExecutePlatformConfig (
     //
     // Pass the preferred resolution to GraphicsConsoleDxe via dynamic PCDs.
     //
-    PcdSet32 (PcdVideoHorizontalResolution,
+    PcdStatus = PcdSet32S (PcdVideoHorizontalResolution,
       PlatformConfig.HorizontalResolution);
-    PcdSet32 (PcdVideoVerticalResolution,
+    ASSERT_RETURN_ERROR (PcdStatus);
+
+    PcdStatus = PcdSet32S (PcdVideoVerticalResolution,
       PlatformConfig.VerticalResolution);
+    ASSERT_RETURN_ERROR (PcdStatus);
   }
 
   return EFI_SUCCESS;
-- 
2.9.2

[PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Anthony PERARD <anthony.perard@citrix.com>
Cc: Gary Lin <glin@suse.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
    difference in my setup, so testing for those would be appreciated.

 OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
 OvmfPkg/PlatformPei/Platform.c  | 44 +++++++++++++-------
 OvmfPkg/PlatformPei/Xen.c       |  5 ++-
 3 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index 4863eb101067..d00a570d4381 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -107,6 +107,7 @@ GetFirstNonAddress (
   FIRMWARE_CONFIG_ITEM FwCfgItem;
   UINTN                FwCfgSize;
   UINT64               HotPlugMemoryEnd;
+  RETURN_STATUS        PcdStatus;
 
   FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
 
@@ -154,7 +155,8 @@ GetFirstNonAddress (
     if (mBootMode != BOOT_ON_S3_RESUME) {
       DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
         __FUNCTION__));
-      PcdSet64 (PcdPciMmio64Size, 0);
+      PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
+      ASSERT_RETURN_ERROR (PcdStatus);
     }
 
     //
@@ -202,8 +204,11 @@ GetFirstNonAddress (
     // the GCD memory space map through our PciHostBridgeLib instance; here we
     // only need to set the PCDs.
     //
-    PcdSet64 (PcdPciMmio64Base, Pci64Base);
-    PcdSet64 (PcdPciMmio64Size, Pci64Size);
+    PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
+    ASSERT_RETURN_ERROR (PcdStatus);
+
     DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
       __FUNCTION__, Pci64Base, Pci64Size));
   }
diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
index ca1e6dc7e320..c6e1106c9ed0 100644
--- a/OvmfPkg/PlatformPei/Platform.c
+++ b/OvmfPkg/PlatformPei/Platform.c
@@ -156,8 +156,9 @@ MemMapInitialization (
   VOID
   )
 {
-  UINT64 PciIoBase;
-  UINT64 PciIoSize;
+  UINT64        PciIoBase;
+  UINT64        PciIoSize;
+  RETURN_STATUS PcdStatus;
 
   PciIoBase = 0xC000;
   PciIoSize = 0x4000;
@@ -212,8 +213,11 @@ MemMapInitialization (
     //
     PciSize = 0xFC000000 - PciBase;
     AddIoMemoryBaseSizeHob (PciBase, PciSize);
-    PcdSet64 (PcdPciMmio32Base, PciBase);
-    PcdSet64 (PcdPciMmio32Size, PciSize);
+    PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
+    ASSERT_RETURN_ERROR (PcdStatus);
+
     AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
     AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
     if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
@@ -266,8 +270,10 @@ MemMapInitialization (
     PciIoBase,
     PciIoSize
     );
-  PcdSet64 (PcdPciIoBase, PciIoBase);
-  PcdSet64 (PcdPciIoSize, PciIoSize);
+  PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
+  ASSERT_RETURN_ERROR (PcdStatus);
 }
 
 EFI_STATUS
@@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
 
 #define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName)                   \
           do {                                                      \
-            BOOLEAN Setting;                                        \
+            BOOLEAN       Setting;                                  \
+            RETURN_STATUS PcdStatus;                                \
                                                                     \
             if (!EFI_ERROR (GetNamedFwCfgBoolean (                  \
                               "opt/ovmf/" #TokenName, &Setting))) { \
-              PcdSetBool (TokenName, Setting);                      \
+              PcdStatus = PcdSetBoolS (TokenName, Setting);         \
+              ASSERT_RETURN_ERROR (PcdStatus);                      \
             }                                                       \
           } while (0)
 
@@ -379,12 +387,13 @@ MiscInitialization (
   VOID
   )
 {
-  UINTN  PmCmd;
-  UINTN  Pmba;
-  UINT32 PmbaAndVal;
-  UINT32 PmbaOrVal;
-  UINTN  AcpiCtlReg;
-  UINT8  AcpiEnBit;
+  UINTN         PmCmd;
+  UINTN         Pmba;
+  UINT32        PmbaAndVal;
+  UINT32        PmbaOrVal;
+  UINTN         AcpiCtlReg;
+  UINT8         AcpiEnBit;
+  RETURN_STATUS PcdStatus;
 
   //
   // Disable A20 Mask
@@ -424,7 +433,8 @@ MiscInitialization (
       ASSERT (FALSE);
       return;
   }
-  PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
+  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // If the appropriate IOspace enable bit is set, assume the ACPI PMBA
@@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
   )
 {
   EFI_PHYSICAL_ADDRESS VariableStore;
+  RETURN_STATUS        PcdStatus;
 
   //
   // Allocate storage for NV variables early on so it will be
@@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
           VariableStore,
           (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
         ));
-  PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
+  PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
+  ASSERT_RETURN_ERROR (PcdStatus);
 }
 
 
diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
index 223908a4f529..ab38f97a67aa 100644
--- a/OvmfPkg/PlatformPei/Xen.c
+++ b/OvmfPkg/PlatformPei/Xen.c
@@ -210,6 +210,8 @@ InitializeXen (
   VOID
   )
 {
+  RETURN_STATUS PcdStatus;
+
   if (mXenLeaf == 0) {
     return EFI_NOT_FOUND;
   }
@@ -222,7 +224,8 @@ InitializeXen (
   //
   AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
 
-  PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
+  PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   return EFI_SUCCESS;
 }
-- 
2.9.2

[PATCH 09/19] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
index c37aed1c9e7b..ff27c1100c01 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c
@@ -965,6 +965,7 @@ FvbInitialize (
   EFI_PHYSICAL_ADDRESS                BaseAddress;
   UINTN                               Length;
   UINTN                               NumOfBlocks;
+  RETURN_STATUS                       PcdStatus;
 
   if (EFI_ERROR (QemuFlashInitialize ())) {
     //
@@ -1095,18 +1096,21 @@ FvbInitialize (
   //
   // Set several PCD values to point to flash
   //
-  PcdSet64 (
+  PcdStatus = PcdSet64S (
     PcdFlashNvStorageVariableBase64,
     (UINTN) PcdGet32 (PcdOvmfFlashNvStorageVariableBase)
     );
-  PcdSet32 (
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (
     PcdFlashNvStorageFtwWorkingBase,
     PcdGet32 (PcdOvmfFlashNvStorageFtwWorkingBase)
     );
-  PcdSet32 (
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (
     PcdFlashNvStorageFtwSpareBase,
     PcdGet32 (PcdOvmfFlashNvStorageFtwSpareBase)
     );
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   FwhInstance = (EFI_FW_VOL_INSTANCE *)
     (
@@ -1119,6 +1123,7 @@ FvbInitialize (
   //
   InstallVirtualAddressChangeHandler ();
 
-  PcdSetBool (PcdOvmfFlashVariablesEnable, TRUE);
+  PcdStatus = PcdSetBoolS (PcdOvmfFlashVariablesEnable, TRUE);
+  ASSERT_RETURN_ERROR (PcdStatus);
   return EFI_SUCCESS;
 }
-- 
2.9.2

[PATCH 10/19] OvmfPkg: disable deprecated interfaces

[Laszlo Ersek]

At this point no code in OvmfPkg (and apparently no code outside of
OvmfPkg that OVMF depends on) uses the deprecated APIs, so we can disable
them in the platform DSC files:

BaseLib:
- StrCpy
- StrnCpy
- StrCat
- StrnCat
- UnicodeStrToAsciiStr
- AsciiStrCpy
- AsciiStrnCpy
- AsciiStrCat
- AsciiStrnCat
- AsciiStrToUnicodeStr

PcdLib:
- PcdSet8
- PcdSet16
- PcdSet32
- PcdSet64
- PcdSetPtr
- PcdSetBool
- PcdSetEx8
- PcdSetEx16
- PcdSetEx32
- PcdSetEx64
- PcdSetExPtr
- PcdSetExBool

UefiLib:
- GetVariable
- GetEfiGlobalVariable

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    Tested with GCC only. My VS2015 build environment was lost and will have
    to be reinstalled from scratch at a later (unspecified) point, and I've
    never had access to Intel compilers.

 OvmfPkg/OvmfPkgIa32.dsc    | 7 +++++++
 OvmfPkg/OvmfPkgIa32X64.dsc | 7 +++++++
 OvmfPkg/OvmfPkgX64.dsc     | 7 +++++++
 3 files changed, 21 insertions(+)

diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 4f6dbc53a9fa..3f4d42d85642 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -46,6 +46,13 @@ [BuildOptions]
   MSFT:RELEASE_*_*_CC_FLAGS            = /D MDEPKG_NDEBUG
   GCC:*_*_*_CC_FLAGS                   = -mno-mmx -mno-sse
 
+  #
+  # Disable deprecated APIs.
+  #
+  MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
 
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index c21f88ec1a08..568847531a56 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -51,6 +51,13 @@ [BuildOptions]
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
 
+  #
+  # Disable deprecated APIs.
+  #
+  MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
 
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index b7a39e3c4c61..dcf64b9d6aef 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -51,6 +51,13 @@ [BuildOptions]
   INTEL:*_*_X64_GENFW_FLAGS = --keepexceptiontable
 !endif
 
+  #
+  # Disable deprecated APIs.
+  #
+  MSFT:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  INTEL:*_*_*_CC_FLAGS = /D DISABLE_NEW_DEPRECATED_INTERFACES
+  GCC:*_*_*_CC_FLAGS = -D DISABLE_NEW_DEPRECATED_INTERFACES
+
 [BuildOptions.common.EDKII.DXE_RUNTIME_DRIVER]
   GCC:*_*_*_DLINK_FLAGS = -z common-page-size=0x1000
 
-- 
2.9.2

[PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
index 648806126e78..dc1f5155f1fc 100644
--- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -41,6 +41,7 @@ ArmVirtGicArchLibConstructor (
   UINTN                 GicRevision;
   EFI_STATUS            Status;
   UINT64                DistBase, CpuBase, RedistBase;
+  RETURN_STATUS         PcdStatus;
 
   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
                   (VOID **)&FdtClient);
@@ -85,8 +86,10 @@ ArmVirtGicArchLibConstructor (
     RedistBase = SwapBytes64 (Reg[2]);
     ASSERT (RedistBase < MAX_UINTN);
 
-    PcdSet64 (PcdGicDistributorBase, DistBase);
-    PcdSet64 (PcdGicRedistributorsBase, RedistBase);
+    PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet64S (PcdGicRedistributorsBase, RedistBase);
+    ASSERT_RETURN_ERROR (PcdStatus);
 
     DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
       DistBase, RedistBase));
@@ -120,8 +123,10 @@ ArmVirtGicArchLibConstructor (
     ASSERT (DistBase < MAX_UINTN);
     ASSERT (CpuBase < MAX_UINTN);
 
-    PcdSet64 (PcdGicDistributorBase, DistBase);
-    PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase);
+    PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
+    ASSERT_RETURN_ERROR (PcdStatus);
+    PcdStatus = PcdSet64S (PcdGicInterruptInterfaceBase, CpuBase);
+    ASSERT_RETURN_ERROR (PcdStatus);
 
     DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
 
-- 
2.9.2

[PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
index 3c4e44caa2f4..82de7a51b32e 100644
--- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
@@ -34,6 +34,7 @@ ArmVirtPL031FdtClientLibConstructor (
   CONST UINT64                  *Reg;
   UINT32                        RegSize;
   UINT64                        RegBase;
+  RETURN_STATUS                 PcdStatus;
 
   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
                   (VOID **)&FdtClient);
@@ -60,7 +61,8 @@ ArmVirtPL031FdtClientLibConstructor (
   RegBase = SwapBytes64 (Reg[0]);
   ASSERT (RegBase < MAX_UINT32);
 
-  PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
+  PcdStatus = PcdSet32S (PcdPL031RtcBase, (UINT32)RegBase);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
 
-- 
2.9.2

[PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
index 7a0fc0e75e37..fcaf3c681a97 100644
--- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
+++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
@@ -70,13 +70,14 @@ ArmPlatformInitializeSystemMemory (
   VOID
   )
 {
-  VOID         *DeviceTreeBase;
-  INT32        Node, Prev;
-  UINT64       NewBase, CurBase;
-  UINT64       NewSize, CurSize;
-  CONST CHAR8  *Type;
-  INT32        Len;
-  CONST UINT64 *RegProp;
+  VOID          *DeviceTreeBase;
+  INT32         Node, Prev;
+  UINT64        NewBase, CurBase;
+  UINT64        NewSize, CurSize;
+  CONST CHAR8   *Type;
+  INT32         Len;
+  CONST UINT64  *RegProp;
+  RETURN_STATUS PcdStatus;
 
   NewBase = 0;
   NewSize = 0;
@@ -131,7 +132,8 @@ ArmPlatformInitializeSystemMemory (
   // Make sure the start of DRAM matches our expectation
   //
   ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
-  PcdSet64 (PcdSystemMemorySize, NewSize);
+  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // We need to make sure that the machine we are running on has at least
-- 
2.9.2

[PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
index 6e7461c6b2e7..c40838632352 100644
--- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
+++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
@@ -41,6 +41,7 @@ ArmVirtTimerFdtClientLibConstructor (
   CONST INTERRUPT_PROPERTY      *InterruptProp;
   UINT32                        PropSize;
   INT32                         SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
+  RETURN_STATUS                 PcdStatus;
 
   Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
                   (VOID **)&FdtClient);
@@ -78,10 +79,14 @@ ArmVirtTimerFdtClientLibConstructor (
   DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
     SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
 
-  PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
-  PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
-  PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
-  PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
+  PcdStatus = PcdSet32S (PcdArmArchTimerSecIntrNum, SecIntrNum);
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (PcdArmArchTimerIntrNum, IntrNum);
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
+  ASSERT_RETURN_ERROR (PcdStatus);
+  PcdStatus = PcdSet32S (PcdArmArchTimerHypIntrNum, HypIntrNum);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   return EFI_SUCCESS;
 }
-- 
2.9.2

[PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
index ea27cda7b77c..072f54ef7d9e 100644
--- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
+++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
@@ -98,6 +98,7 @@ FdtPciPcdProducerLibConstructor (
   INT32               Node;
   RETURN_STATUS       RetStatus;
   UINT64              IoTranslation;
+  RETURN_STATUS       PcdStatus;
 
   PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
   if (PciExpressBaseAddress != MAX_UINT64) {
@@ -126,12 +127,14 @@ FdtPciPcdProducerLibConstructor (
     if (!EFI_ERROR (Status) && RegSize == 2 * sizeof (UINT64)) {
       PciExpressBaseAddress = SwapBytes64 (*Reg);
 
-      PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
+      PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
+      ASSERT_RETURN_ERROR (PcdStatus);
 
       IoTranslation = 0;
       RetStatus = GetPciIoTranslation (FdtClient, Node, &IoTranslation);
       if (!RETURN_ERROR (RetStatus)) {
-          PcdSet64 (PcdPciIoTranslation, IoTranslation);
+          PcdStatus = PcdSet64S (PcdPciIoTranslation, IoTranslation);
+          ASSERT_RETURN_ERROR (PcdStatus);
       } else {
         //
         // Support for I/O BARs is not mandatory, and so it does not make sense
@@ -145,7 +148,8 @@ FdtPciPcdProducerLibConstructor (
     }
   }
 
-  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);
+  PcdStatus = PcdSet64S (PcdPciExpressBaseAddress, PciExpressBaseAddress);
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   return RETURN_SUCCESS;
 }
-- 
2.9.2

[PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

These are deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

Introduce a variable called PcdStatus, and use it to assert the success of
these operations (there is no reason for them to fail here).

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
index aecad570a04f..56f4c921b513 100644
--- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
+++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
@@ -578,6 +578,8 @@ PlatformBootManagerBeforeConsole (
   VOID
   )
 {
+  RETURN_STATUS PcdStatus;
+
   //
   // Signal EndOfDxe PI Event
   //
@@ -629,7 +631,9 @@ PlatformBootManagerBeforeConsole (
   //
   // Set the front page timeout from the QEMU configuration.
   //
-  PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
+  PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
+                GetFrontPageTimeoutFromQemu ());
+  ASSERT_RETURN_ERROR (PcdStatus);
 
   //
   // Register platform-specific boot options and keyboard shortcuts.
-- 
2.9.2

[PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()

[Laszlo Ersek]

AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

The "Str" variable serves no particular purpose in the MRegList() and
ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
"mThumbMregListStr" global variable (as appropriate), so that the new
AsciiStrCatS() calls are as clear as possible.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    - build-tested only
    
    - Michael (CC'd) had posted a patch earlier for this:
      <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
      but I only noticed that now that he pointed it out, in
      <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
      I'll leave it to the ArmPkg maintainers to choose one; I don't feel
      strongly either way.

 ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c   | 22 +++++++++-----------
 ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
index 29a8d4438622..8551edc379c1 100644
--- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
+++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
@@ -88,12 +88,10 @@ MRegList (
   )
 {
   UINTN     Index, Start, End;
-  CHAR8     *Str;
   BOOLEAN   First;
 
-  Str = mMregListStr;
-  *Str = '\0';
-  AsciiStrCat  (Str, "{");
+  mMregListStr[0] = '\0';
+  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "{");
   for (Index = 0, First = TRUE; Index <= 15; Index++) {
     if ((OpCode & (1 << Index)) != 0) {
       Start = End = Index;
@@ -102,25 +100,25 @@ MRegList (
       }
 
       if (!First) {
-        AsciiStrCat  (Str, ",");
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ",");
       } else {
         First = FALSE;
       }
 
       if (Start == End) {
-        AsciiStrCat  (Str, gReg[Start]);
-        AsciiStrCat  (Str, ", ");
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ", ");
       } else {
-        AsciiStrCat  (Str, gReg[Start]);
-        AsciiStrCat  (Str, "-");
-        AsciiStrCat  (Str, gReg[End]);
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "-");
+        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[End]);
       }
     }
   }
   if (First) {
-    AsciiStrCat  (Str, "ERROR");
+    AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "ERROR");
   }
-  AsciiStrCat  (Str, "}");
+  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "}");
 
   // BugBug: Make caller pass in buffer it is cleaner
   return mMregListStr;
diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
index 5bad3afcfbf6..86d5083cb189 100644
--- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
+++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
@@ -397,12 +397,10 @@ ThumbMRegList (
   )
 {
   UINTN     Index, Start, End;
-  CHAR8     *Str;
   BOOLEAN   First;
 
-  Str = mThumbMregListStr;
-  *Str = '\0';
-  AsciiStrCat  (Str, "{");
+  mThumbMregListStr[0] = '\0';
+  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "{");
 
   for (Index = 0, First = TRUE; Index <= 15; Index++) {
     if ((RegBitMask & (1 << Index)) != 0) {
@@ -412,24 +410,24 @@ ThumbMRegList (
       }
 
       if (!First) {
-        AsciiStrCat  (Str, ",");
+        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, ",");
       } else {
         First = FALSE;
       }
 
       if (Start == End) {
-        AsciiStrCat  (Str, gReg[Start]);
+        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
       } else {
-        AsciiStrCat  (Str, gReg[Start]);
-        AsciiStrCat  (Str, "-");
-        AsciiStrCat  (Str, gReg[End]);
+        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
+        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "-");
+        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
       }
     }
   }
   if (First) {
-    AsciiStrCat  (Str, "ERROR");
+    AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
   }
-  AsciiStrCat  (Str, "}");
+  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "}");
 
   // BugBug: Make caller pass in buffer it is cleaner
   return mThumbMregListStr;
-- 
2.9.2

[PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()

[Laszlo Ersek]

AsciiStrCat() is deprecated / disabled under the
DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.

The caller of CpsrString() is required to pass in "ReturnStr" with 32
CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
"Str" points to the then-terminating NUL character in "ReturnStr".

The difference (Str - ReturnStr) gives the number of non-NUL characters
we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
of remaining bytes in ReturnStr, including the ultimately terminating NUL
character.

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Leif Lindholm <leif.lindholm@linaro.org>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    - build-tested only
    
    - Michael (CC'd) had posted a patch earlier for this:
      <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
      but I only noticed that now that he pointed it out, in
      <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
      I'll leave it to the ArmPkg maintainers to choose one; I don't feel
      strongly either way.

 ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
index aece26355e2e..4b2ee9a9acf7 100644
--- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
+++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
@@ -116,7 +116,10 @@ CpsrString (
     break;
   }
 
-  AsciiStrCat (Str, ModeStr);
+  //
+  // See the interface contract in the leading comment block.
+  //
+  AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
   return;
 }
 
-- 
2.9.2

[PATCH 19/19] ArmVirtPkg: disable deprecated interfaces

[Laszlo Ersek]

At this point no code in ArmVirtPkg (and apparently no code outside of
ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
so we can disable them in the common platform DSC include file:

BaseLib:
- StrCpy
- StrnCpy
- StrCat
- StrnCat
- UnicodeStrToAsciiStr
- AsciiStrCpy
- AsciiStrnCpy
- AsciiStrCat
- AsciiStrnCat
- AsciiStrToUnicodeStr

PcdLib:
- PcdSet8
- PcdSet16
- PcdSet32
- PcdSet64
- PcdSetPtr
- PcdSetBool
- PcdSetEx8
- PcdSetEx16
- PcdSetEx32
- PcdSetEx64
- PcdSetExPtr
- PcdSetExBool

UefiLib:
- GetVariable
- GetEfiGlobalVariable

Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    not build-tested with RVCT

 ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
index c071988ad8f5..dbd6678accde 100644
--- a/ArmVirtPkg/ArmVirt.dsc.inc
+++ b/ArmVirtPkg/ArmVirt.dsc.inc
@@ -246,6 +246,12 @@ [BuildOptions]
 
   GCC:RELEASE_*_*_CC_FLAGS  = -DMDEPKG_NDEBUG
 
+  #
+  # Disable deprecated APIs.
+  #
+  RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+  GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
+
 ################################################################################
 #
 # Pcd Section - list of all EDK II PCD Entries defined by this Platform
-- 
2.9.2

Re: [PATCH 02/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls

[Gary Lin]

On Fri, Oct 21, 2016 at 11:27:20PM +0200, Laszlo Ersek wrote:
> AsciiStrCpy() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> 

Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     not used in my setup, testing would be appreciated
> 
>  OvmfPkg/XenBusDxe/XenStore.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index fb00e7393bb1..aa3ff7d3017b 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -1298,11 +1298,8 @@ XenStoreTransactionEnd (
>  {
>    CHAR8 AbortStr[2];
>  
> -  if (Abort) {
> -    AsciiStrCpy (AbortStr, "F");
> -  } else {
> -    AsciiStrCpy (AbortStr, "T");
> -  }
> +  AbortStr[0] = Abort ? 'F' : 'T';
> +  AbortStr[1] = '\0';
>  
>    return XenStoreSingle (Transaction, XS_TRANSACTION_END, AbortStr, NULL, NULL);
>  }
> -- 
> 2.9.2
> 
> 
> 

Re: [PATCH 03/19] OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls

[Gary Lin]

On Fri, Oct 21, 2016 at 11:27:21PM +0200, Laszlo Ersek wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> 
> Replace AsciiStrCat() with AsciiSPrint(). Spell out the (already existent)
> PrintLib dependency in the INF file. Add an explicit ASSERT() to document
> that XenStoreJoin() assumes that the pool allocation always succeeds.
> 

Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     not used in my setup, testing would be appreciated
> 
>  OvmfPkg/XenBusDxe/XenBusDxe.inf |  1 +
>  OvmfPkg/XenBusDxe/XenStore.c    | 15 +++++++++------
>  2 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/OvmfPkg/XenBusDxe/XenBusDxe.inf b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> index f0c5db98b1f4..5ff1cd04840c 100644
> --- a/OvmfPkg/XenBusDxe/XenBusDxe.inf
> +++ b/OvmfPkg/XenBusDxe/XenBusDxe.inf
> @@ -56,6 +56,7 @@ [LibraryClasses]
>    DebugLib
>    XenHypercallLib
>    SynchronizationLib
> +  PrintLib
>  
>  [Protocols]
>    gEfiDriverBindingProtocolGuid
> diff --git a/OvmfPkg/XenBusDxe/XenStore.c b/OvmfPkg/XenBusDxe/XenStore.c
> index aa3ff7d3017b..b7ae1d04863d 100644
> --- a/OvmfPkg/XenBusDxe/XenStore.c
> +++ b/OvmfPkg/XenBusDxe/XenStore.c
> @@ -303,14 +303,17 @@ XenStoreJoin (
>    )
>  {
>    CHAR8 *Buf;
> +  UINTN BufSize;
>  
>    /* +1 for '/' and +1 for '\0' */
> -  Buf = AllocateZeroPool (
> -          AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2);
> -  AsciiStrCat (Buf, DirectoryPath);
> -  if (Node[0] != '\0') {
> -    AsciiStrCat (Buf, "/");
> -    AsciiStrCat (Buf, Node);
> +  BufSize = AsciiStrLen (DirectoryPath) + AsciiStrLen (Node) + 2;
> +  Buf = AllocatePool (BufSize);
> +  ASSERT (Buf != NULL);
> +
> +  if (Node[0] == '\0') {
> +    AsciiSPrint (Buf, BufSize, "%a", DirectoryPath);
> +  } else {
> +    AsciiSPrint (Buf, BufSize, "%a/%a", DirectoryPath, Node);
>    }
>  
>    return Buf;
> -- 
> 2.9.2
> 
> 
> 

Re: [PATCH 04/19] OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls

[Gary Lin]

On Fri, Oct 21, 2016 at 11:27:22PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> 
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
> 

Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     not used in my setup, testing would be appreciated
> 
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> index 7a8beb3354b9..dec6d4af50df 100644
> --- a/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> +++ b/OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c
> @@ -803,6 +803,7 @@ FvbInitialize (
>    BOOLEAN                             Initialize;
>    EFI_HANDLE                          Handle;
>    EFI_PHYSICAL_ADDRESS                Address;
> +  RETURN_STATUS                       PcdStatus;
>  
>    DEBUG ((EFI_D_INFO, "EMU Variable FVB Started\n"));
>  
> @@ -862,19 +863,24 @@ FvbInitialize (
>      SetMem (Ptr, EMU_FVB_SIZE, ERASED_UINT8);
>      InitializeFvAndVariableStoreHeaders (Ptr);
>    }
> -  PcdSet64 (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
> +  PcdStatus = PcdSet64S (PcdFlashNvStorageVariableBase64, (UINT32)(UINTN) Ptr);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    //
>    // Initialize the Fault Tolerant Write data area
>    //
>    SubPtr = (VOID*) ((UINT8*) Ptr + PcdGet32 (PcdVariableStoreSize));
> -  PcdSet32 (PcdFlashNvStorageFtwWorkingBase, (UINT32)(UINTN) SubPtr);
> +  PcdStatus = PcdSet32S (PcdFlashNvStorageFtwWorkingBase,
> +                (UINT32)(UINTN) SubPtr);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    //
>    // Initialize the Fault Tolerant Write spare block
>    //
>    SubPtr = (VOID*) ((UINT8*) Ptr + EMU_FVB_BLOCK_SIZE);
> -  PcdSet32 (PcdFlashNvStorageFtwSpareBase, (UINT32)(UINTN) SubPtr);
> +  PcdStatus = PcdSet32S (PcdFlashNvStorageFtwSpareBase,
> +                (UINT32)(UINTN) SubPtr);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    //
>    // Setup FVB device path
> -- 
> 2.9.2
> 
> 
> 

Re: [PATCH 05/19] OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls

[Gary Lin]

On Fri, Oct 21, 2016 at 11:27:23PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> 
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
> 

Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     the emulated variable part of the patch is un-exercised on my setup,
>     testing would be appreciated
> 
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> index bd1a4396f1d3..29ce21282595 100644
> --- a/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> +++ b/OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> @@ -353,8 +353,9 @@ Returns:
>  
>  --*/
>  {
> -  EFI_HANDLE Handle;
> -  EFI_STATUS Status;
> +  EFI_HANDLE    Handle;
> +  EFI_STATUS    Status;
> +  RETURN_STATUS PcdStatus;
>  
>    DEBUG ((EFI_D_INFO, "PlatformBootManagerBeforeConsole\n"));
>    InstallDevicePathCallback ();
> @@ -394,7 +395,9 @@ Returns:
>    ASSERT_EFI_ERROR (Status);
>  
>    PlatformInitializeConsole (gPlatformConsole);
> -  PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
> +  PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
> +                GetFrontPageTimeoutFromQemu ());
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    PlatformRegisterOptionsAndKeys ();
>  }
> @@ -1281,6 +1284,7 @@ VisitingFileSystemInstance (
>  {
>    EFI_STATUS      Status;
>    STATIC BOOLEAN  ConnectedToFileSystem = FALSE;
> +  RETURN_STATUS   PcdStatus;
>  
>    if (ConnectedToFileSystem) {
>      return EFI_ALREADY_STARTED;
> @@ -1300,7 +1304,9 @@ VisitingFileSystemInstance (
>        NULL,
>        &mEmuVariableEventReg
>        );
> -  PcdSet64 (PcdEmuVariableEvent, (UINT64)(UINTN) mEmuVariableEvent);
> +  PcdStatus = PcdSet64S (PcdEmuVariableEvent,
> +                (UINT64)(UINTN) mEmuVariableEvent);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    return EFI_SUCCESS;
>  }
> -- 
> 2.9.2
> 
> 
> 

Re: [PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls

[Gary Lin]

On Fri, Oct 21, 2016 at 11:27:26PM +0200, Laszlo Ersek wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> 
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
> 

Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
>     difference in my setup, so testing for those would be appreciated.
> 
>  OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
>  OvmfPkg/PlatformPei/Platform.c  | 44 +++++++++++++-------
>  OvmfPkg/PlatformPei/Xen.c       |  5 ++-
>  3 files changed, 40 insertions(+), 20 deletions(-)
> 
> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
> index 4863eb101067..d00a570d4381 100644
> --- a/OvmfPkg/PlatformPei/MemDetect.c
> +++ b/OvmfPkg/PlatformPei/MemDetect.c
> @@ -107,6 +107,7 @@ GetFirstNonAddress (
>    FIRMWARE_CONFIG_ITEM FwCfgItem;
>    UINTN                FwCfgSize;
>    UINT64               HotPlugMemoryEnd;
> +  RETURN_STATUS        PcdStatus;
>  
>    FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>  
> @@ -154,7 +155,8 @@ GetFirstNonAddress (
>      if (mBootMode != BOOT_ON_S3_RESUME) {
>        DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
>          __FUNCTION__));
> -      PcdSet64 (PcdPciMmio64Size, 0);
> +      PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
> +      ASSERT_RETURN_ERROR (PcdStatus);
>      }
>  
>      //
> @@ -202,8 +204,11 @@ GetFirstNonAddress (
>      // the GCD memory space map through our PciHostBridgeLib instance; here we
>      // only need to set the PCDs.
>      //
> -    PcdSet64 (PcdPciMmio64Base, Pci64Base);
> -    PcdSet64 (PcdPciMmio64Size, Pci64Size);
> +    PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +    PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +
>      DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
>        __FUNCTION__, Pci64Base, Pci64Size));
>    }
> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
> index ca1e6dc7e320..c6e1106c9ed0 100644
> --- a/OvmfPkg/PlatformPei/Platform.c
> +++ b/OvmfPkg/PlatformPei/Platform.c
> @@ -156,8 +156,9 @@ MemMapInitialization (
>    VOID
>    )
>  {
> -  UINT64 PciIoBase;
> -  UINT64 PciIoSize;
> +  UINT64        PciIoBase;
> +  UINT64        PciIoSize;
> +  RETURN_STATUS PcdStatus;
>  
>    PciIoBase = 0xC000;
>    PciIoSize = 0x4000;
> @@ -212,8 +213,11 @@ MemMapInitialization (
>      //
>      PciSize = 0xFC000000 - PciBase;
>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
> -    PcdSet64 (PcdPciMmio32Base, PciBase);
> -    PcdSet64 (PcdPciMmio32Size, PciSize);
> +    PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +    PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +
>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
> @@ -266,8 +270,10 @@ MemMapInitialization (
>      PciIoBase,
>      PciIoSize
>      );
> -  PcdSet64 (PcdPciIoBase, PciIoBase);
> -  PcdSet64 (PcdPciIoSize, PciIoSize);
> +  PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +  PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  }
>  
>  EFI_STATUS
> @@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
>  
>  #define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName)                   \
>            do {                                                      \
> -            BOOLEAN Setting;                                        \
> +            BOOLEAN       Setting;                                  \
> +            RETURN_STATUS PcdStatus;                                \
>                                                                      \
>              if (!EFI_ERROR (GetNamedFwCfgBoolean (                  \
>                                "opt/ovmf/" #TokenName, &Setting))) { \
> -              PcdSetBool (TokenName, Setting);                      \
> +              PcdStatus = PcdSetBoolS (TokenName, Setting);         \
> +              ASSERT_RETURN_ERROR (PcdStatus);                      \
>              }                                                       \
>            } while (0)
>  
> @@ -379,12 +387,13 @@ MiscInitialization (
>    VOID
>    )
>  {
> -  UINTN  PmCmd;
> -  UINTN  Pmba;
> -  UINT32 PmbaAndVal;
> -  UINT32 PmbaOrVal;
> -  UINTN  AcpiCtlReg;
> -  UINT8  AcpiEnBit;
> +  UINTN         PmCmd;
> +  UINTN         Pmba;
> +  UINT32        PmbaAndVal;
> +  UINT32        PmbaOrVal;
> +  UINTN         AcpiCtlReg;
> +  UINT8         AcpiEnBit;
> +  RETURN_STATUS PcdStatus;
>  
>    //
>    // Disable A20 Mask
> @@ -424,7 +433,8 @@ MiscInitialization (
>        ASSERT (FALSE);
>        return;
>    }
> -  PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    //
>    // If the appropriate IOspace enable bit is set, assume the ACPI PMBA
> @@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
>    )
>  {
>    EFI_PHYSICAL_ADDRESS VariableStore;
> +  RETURN_STATUS        PcdStatus;
>  
>    //
>    // Allocate storage for NV variables early on so it will be
> @@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
>            VariableStore,
>            (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
>          ));
> -  PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
> +  PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  }
>  
>  
> diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
> index 223908a4f529..ab38f97a67aa 100644
> --- a/OvmfPkg/PlatformPei/Xen.c
> +++ b/OvmfPkg/PlatformPei/Xen.c
> @@ -210,6 +210,8 @@ InitializeXen (
>    VOID
>    )
>  {
> +  RETURN_STATUS PcdStatus;
> +
>    if (mXenLeaf == 0) {
>      return EFI_NOT_FOUND;
>    }
> @@ -222,7 +224,8 @@ InitializeXen (
>    //
>    AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
>  
> -  PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
> +  PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>  
>    return EFI_SUCCESS;
>  }
> -- 
> 2.9.2
> 
> 
> 

Re: [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> The caller of CpsrString() is required to pass in "ReturnStr" with 32
> CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
> used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
> "Str" points to the then-terminating NUL character in "ReturnStr".
>
> The difference (Str - ReturnStr) gives the number of non-NUL characters
> we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
> of remaining bytes in ReturnStr, including the ultimately terminating NUL
> character.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>
> Notes:
>     - build-tested only
>
>     - Michael (CC'd) had posted a patch earlier for this:
>       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>       but I only noticed that now that he pointed it out, in
>       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>       strongly either way.
>
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> index aece26355e2e..4b2ee9a9acf7 100644
> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
> @@ -116,7 +116,10 @@ CpsrString (
>      break;
>    }
>
> -  AsciiStrCat (Str, ModeStr);
> +  //
> +  // See the interface contract in the leading comment block.
> +  //
> +  AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
>    return;

Could you please use a symbolic constant for this '32', and replace it
in the declaration of CpsrStr[] as well?

With that

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Oh, and if the bogus 'return' happens to get lost along the way as
well, I would not mind.

Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> AsciiStrCat() is deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> The "Str" variable serves no particular purpose in the MRegList() and
> ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
> "mThumbMregListStr" global variable (as appropriate), so that the new
> AsciiStrCatS() calls are as clear as possible.
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> Notes:
>     - build-tested only
>
>     - Michael (CC'd) had posted a patch earlier for this:
>       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>       but I only noticed that now that he pointed it out, in
>       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>       strongly either way.
>

Apologies to Michael for ignoring his contribution, but I'd be happy
for Laszlo to merge this as part of the series.

>  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c   | 22 +++++++++-----------
>  ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
>  2 files changed, 19 insertions(+), 23 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> index 29a8d4438622..8551edc379c1 100644
> --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> @@ -88,12 +88,10 @@ MRegList (
>    )
>  {
>    UINTN     Index, Start, End;
> -  CHAR8     *Str;
>    BOOLEAN   First;
>
> -  Str = mMregListStr;
> -  *Str = '\0';
> -  AsciiStrCat  (Str, "{");
> +  mMregListStr[0] = '\0';
> +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "{");
>    for (Index = 0, First = TRUE; Index <= 15; Index++) {
>      if ((OpCode & (1 << Index)) != 0) {
>        Start = End = Index;
> @@ -102,25 +100,25 @@ MRegList (
>        }
>
>        if (!First) {
> -        AsciiStrCat  (Str, ",");
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ",");
>        } else {
>          First = FALSE;
>        }
>
>        if (Start == End) {
> -        AsciiStrCat  (Str, gReg[Start]);
> -        AsciiStrCat  (Str, ", ");
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ", ");
>        } else {
> -        AsciiStrCat  (Str, gReg[Start]);
> -        AsciiStrCat  (Str, "-");
> -        AsciiStrCat  (Str, gReg[End]);
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "-");
> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[End]);
>        }
>      }
>    }
>    if (First) {
> -    AsciiStrCat  (Str, "ERROR");
> +    AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "ERROR");
>    }
> -  AsciiStrCat  (Str, "}");
> +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "}");
>
>    // BugBug: Make caller pass in buffer it is cleaner
>    return mMregListStr;
> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> index 5bad3afcfbf6..86d5083cb189 100644
> --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> @@ -397,12 +397,10 @@ ThumbMRegList (
>    )
>  {
>    UINTN     Index, Start, End;
> -  CHAR8     *Str;
>    BOOLEAN   First;
>
> -  Str = mThumbMregListStr;
> -  *Str = '\0';
> -  AsciiStrCat  (Str, "{");
> +  mThumbMregListStr[0] = '\0';
> +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "{");
>
>    for (Index = 0, First = TRUE; Index <= 15; Index++) {
>      if ((RegBitMask & (1 << Index)) != 0) {
> @@ -412,24 +410,24 @@ ThumbMRegList (
>        }
>
>        if (!First) {
> -        AsciiStrCat  (Str, ",");
> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, ",");
>        } else {
>          First = FALSE;
>        }
>
>        if (Start == End) {
> -        AsciiStrCat  (Str, gReg[Start]);
> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
>        } else {
> -        AsciiStrCat  (Str, gReg[Start]);
> -        AsciiStrCat  (Str, "-");
> -        AsciiStrCat  (Str, gReg[End]);
> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "-");
> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
>        }
>      }
>    }
>    if (First) {
> -    AsciiStrCat  (Str, "ERROR");
> +    AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
>    }
> -  AsciiStrCat  (Str, "}");
> +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "}");
>
>    // BugBug: Make caller pass in buffer it is cleaner
>    return mThumbMregListStr;
> --
> 2.9.2
>
>

Re: [PATCH 16/19] ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> index aecad570a04f..56f4c921b513 100644
> --- a/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> +++ b/ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c
> @@ -578,6 +578,8 @@ PlatformBootManagerBeforeConsole (
>    VOID
>    )
>  {
> +  RETURN_STATUS PcdStatus;
> +
>    //
>    // Signal EndOfDxe PI Event
>    //
> @@ -629,7 +631,9 @@ PlatformBootManagerBeforeConsole (
>    //
>    // Set the front page timeout from the QEMU configuration.
>    //
> -  PcdSet16 (PcdPlatformBootTimeOut, GetFrontPageTimeoutFromQemu ());
> +  PcdStatus = PcdSet16S (PcdPlatformBootTimeOut,
> +                GetFrontPageTimeoutFromQemu ());
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    //
>    // Register platform-specific boot options and keyboard shortcuts.
> --
> 2.9.2
>
>

Re: [PATCH 15/19] ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> index ea27cda7b77c..072f54ef7d9e 100644
> --- a/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> +++ b/ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c
> @@ -98,6 +98,7 @@ FdtPciPcdProducerLibConstructor (
>    INT32               Node;
>    RETURN_STATUS       RetStatus;
>    UINT64              IoTranslation;
> +  RETURN_STATUS       PcdStatus;
>
>    PciExpressBaseAddress = PcdGet64 (PcdPciExpressBaseAddress);
>    if (PciExpressBaseAddress != MAX_UINT64) {
> @@ -126,12 +127,14 @@ FdtPciPcdProducerLibConstructor (
>      if (!EFI_ERROR (Status) && RegSize == 2 * sizeof (UINT64)) {
>        PciExpressBaseAddress = SwapBytes64 (*Reg);
>
> -      PcdSetBool (PcdPciDisableBusEnumeration, FALSE);
> +      PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
> +      ASSERT_RETURN_ERROR (PcdStatus);
>
>        IoTranslation = 0;
>        RetStatus = GetPciIoTranslation (FdtClient, Node, &IoTranslation);
>        if (!RETURN_ERROR (RetStatus)) {
> -          PcdSet64 (PcdPciIoTranslation, IoTranslation);
> +          PcdStatus = PcdSet64S (PcdPciIoTranslation, IoTranslation);
> +          ASSERT_RETURN_ERROR (PcdStatus);
>        } else {
>          //
>          // Support for I/O BARs is not mandatory, and so it does not make sense
> @@ -145,7 +148,8 @@ FdtPciPcdProducerLibConstructor (
>      }
>    }
>
> -  PcdSet64 (PcdPciExpressBaseAddress, PciExpressBaseAddress);
> +  PcdStatus = PcdSet64S (PcdPciExpressBaseAddress, PciExpressBaseAddress);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    return RETURN_SUCCESS;
>  }
> --
> 2.9.2
>
>

Re: [PATCH 14/19] ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> index 6e7461c6b2e7..c40838632352 100644
> --- a/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c
> @@ -41,6 +41,7 @@ ArmVirtTimerFdtClientLibConstructor (
>    CONST INTERRUPT_PROPERTY      *InterruptProp;
>    UINT32                        PropSize;
>    INT32                         SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum;
> +  RETURN_STATUS                 PcdStatus;
>
>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>                    (VOID **)&FdtClient);
> @@ -78,10 +79,14 @@ ArmVirtTimerFdtClientLibConstructor (
>    DEBUG ((EFI_D_INFO, "Found Timer interrupts %d, %d, %d, %d\n",
>      SecIntrNum, IntrNum, VirtIntrNum, HypIntrNum));
>
> -  PcdSet32 (PcdArmArchTimerSecIntrNum, SecIntrNum);
> -  PcdSet32 (PcdArmArchTimerIntrNum, IntrNum);
> -  PcdSet32 (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
> -  PcdSet32 (PcdArmArchTimerHypIntrNum, HypIntrNum);
> +  PcdStatus = PcdSet32S (PcdArmArchTimerSecIntrNum, SecIntrNum);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +  PcdStatus = PcdSet32S (PcdArmArchTimerIntrNum, IntrNum);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +  PcdStatus = PcdSet32S (PcdArmArchTimerVirtIntrNum, VirtIntrNum);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +  PcdStatus = PcdSet32S (PcdArmArchTimerHypIntrNum, HypIntrNum);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    return EFI_SUCCESS;
>  }
> --
> 2.9.2
>
>

Re: [PATCH 13/19] ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> index 7a0fc0e75e37..fcaf3c681a97 100644
> --- a/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> +++ b/ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c
> @@ -70,13 +70,14 @@ ArmPlatformInitializeSystemMemory (
>    VOID
>    )
>  {
> -  VOID         *DeviceTreeBase;
> -  INT32        Node, Prev;
> -  UINT64       NewBase, CurBase;
> -  UINT64       NewSize, CurSize;
> -  CONST CHAR8  *Type;
> -  INT32        Len;
> -  CONST UINT64 *RegProp;
> +  VOID          *DeviceTreeBase;
> +  INT32         Node, Prev;
> +  UINT64        NewBase, CurBase;
> +  UINT64        NewSize, CurSize;
> +  CONST CHAR8   *Type;
> +  INT32         Len;
> +  CONST UINT64  *RegProp;
> +  RETURN_STATUS PcdStatus;
>
>    NewBase = 0;
>    NewSize = 0;
> @@ -131,7 +132,8 @@ ArmPlatformInitializeSystemMemory (
>    // Make sure the start of DRAM matches our expectation
>    //
>    ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
> -  PcdSet64 (PcdSystemMemorySize, NewSize);
> +  PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    //
>    // We need to make sure that the machine we are running on has at least
> --
> 2.9.2
>
>

Re: [PATCH 12/19] ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> index 3c4e44caa2f4..82de7a51b32e 100644
> --- a/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c
> @@ -34,6 +34,7 @@ ArmVirtPL031FdtClientLibConstructor (
>    CONST UINT64                  *Reg;
>    UINT32                        RegSize;
>    UINT64                        RegBase;
> +  RETURN_STATUS                 PcdStatus;
>
>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>                    (VOID **)&FdtClient);
> @@ -60,7 +61,8 @@ ArmVirtPL031FdtClientLibConstructor (
>    RegBase = SwapBytes64 (Reg[0]);
>    ASSERT (RegBase < MAX_UINT32);
>
> -  PcdSet32 (PcdPL031RtcBase, (UINT32)RegBase);
> +  PcdStatus = PcdSet32S (PcdPL031RtcBase, (UINT32)RegBase);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    DEBUG ((EFI_D_INFO, "Found PL031 RTC @ 0x%Lx\n", RegBase));
>
> --
> 2.9.2
>
>

Re: [PATCH 11/19] ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> index 648806126e78..dc1f5155f1fc 100644
> --- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> +++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
> @@ -41,6 +41,7 @@ ArmVirtGicArchLibConstructor (
>    UINTN                 GicRevision;
>    EFI_STATUS            Status;
>    UINT64                DistBase, CpuBase, RedistBase;
> +  RETURN_STATUS         PcdStatus;
>
>    Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>                    (VOID **)&FdtClient);
> @@ -85,8 +86,10 @@ ArmVirtGicArchLibConstructor (
>      RedistBase = SwapBytes64 (Reg[2]);
>      ASSERT (RedistBase < MAX_UINTN);
>
> -    PcdSet64 (PcdGicDistributorBase, DistBase);
> -    PcdSet64 (PcdGicRedistributorsBase, RedistBase);
> +    PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +    PcdStatus = PcdSet64S (PcdGicRedistributorsBase, RedistBase);
> +    ASSERT_RETURN_ERROR (PcdStatus);
>
>      DEBUG ((EFI_D_INFO, "Found GIC v3 (re)distributor @ 0x%Lx (0x%Lx)\n",
>        DistBase, RedistBase));
> @@ -120,8 +123,10 @@ ArmVirtGicArchLibConstructor (
>      ASSERT (DistBase < MAX_UINTN);
>      ASSERT (CpuBase < MAX_UINTN);
>
> -    PcdSet64 (PcdGicDistributorBase, DistBase);
> -    PcdSet64 (PcdGicInterruptInterfaceBase, CpuBase);
> +    PcdStatus = PcdSet64S (PcdGicDistributorBase, DistBase);
> +    ASSERT_RETURN_ERROR (PcdStatus);
> +    PcdStatus = PcdSet64S (PcdGicInterruptInterfaceBase, CpuBase);
> +    ASSERT_RETURN_ERROR (PcdStatus);
>
>      DEBUG ((EFI_D_INFO, "Found GIC @ 0x%Lx/0x%Lx\n", DistBase, CpuBase));
>
> --
> 2.9.2
>
>

Re: [PATCH 06/19] OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> These are deprecated / disabled under the
> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>
> Introduce a variable called PcdStatus, and use it to assert the success of
> these operations (there is no reason for them to fail here).
>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

> ---
>
> Notes:
>     tested with OVMF (smbios v2) and ArmVirtQemu (smbios v3)
>
>  OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> index 950c3f7e0a31..58180412bd1a 100644
> --- a/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> +++ b/OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c
> @@ -40,6 +40,7 @@ DetectSmbiosVersion (
>    UINTN                AnchorSize, TablesSize;
>    QEMU_SMBIOS_ANCHOR   QemuAnchor;
>    UINT16               SmbiosVersion;
> +  RETURN_STATUS        PcdStatus;
>
>    if (PcdGetBool (PcdQemuSmbiosValidated)) {
>      //
> @@ -87,7 +88,8 @@ DetectSmbiosVersion (
>
>      DEBUG ((EFI_D_INFO, "%a: SMBIOS 3.x DocRev from QEMU: 0x%02x\n",
>        __FUNCTION__, QemuAnchor.V3.DocRev));
> -    PcdSet8 (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
> +    PcdStatus = PcdSet8S (PcdSmbiosDocRev, QemuAnchor.V3.DocRev);
> +    ASSERT_RETURN_ERROR (PcdStatus);
>      break;
>
>    default:
> @@ -96,12 +98,14 @@ DetectSmbiosVersion (
>
>    DEBUG ((EFI_D_INFO, "%a: SMBIOS version from QEMU: 0x%04x\n", __FUNCTION__,
>      SmbiosVersion));
> -  PcdSet16 (PcdSmbiosVersion, SmbiosVersion);
> +  PcdStatus = PcdSet16S (PcdSmbiosVersion, SmbiosVersion);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>
>    //
>    // SMBIOS platform drivers can now fetch and install
>    // "etc/smbios/smbios-tables" from QEMU.
>    //
> -  PcdSetBool (PcdQemuSmbiosValidated, TRUE);
> +  PcdStatus = PcdSetBoolS (PcdQemuSmbiosValidated, TRUE);
> +  ASSERT_RETURN_ERROR (PcdStatus);
>    return RETURN_SUCCESS;
>  }
> --
> 2.9.2
>
>

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Ard Biesheuvel]

On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> This series intends to solve the following BZs:
>
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>   the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>   files
>
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>   Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>   DSC files
>
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
>
>   BaseLib:
>   - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>   - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>     AsciiStrToUnicodeStr.
>
>   PcdLib:
>   - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>   - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>     PcdSetExBool.
>
>   UefiLib:
>   - GetVariable, GetEfiGlobalVariable.
>
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
>
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
>

I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*

Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT

Thanks,
Ard.


* In general, RVCT tends to fall over quite regularly due to its
finicky diagnostics, so I did have to apply this patch

"""
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index ca61ac5e1983..1098d9501cc7 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -891,7 +891,7 @@ NorFlashRead (
   SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);

   // Readout the data
-  AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
+  AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);

   return EFI_SUCCESS;
 }
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 812dafd065b2..0ef7b8d81bbc 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -71,3 +71,7 @@ [Depex]
   # NorFlashDxe must be loaded before VariableRuntimeDxe in case
empty flash needs populating with default values
   #
   BEFORE gVariableRuntimeDxeFileGuid
+
+[BuildOptions]
+  RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
+
"""

but these changes are entirely unrelated to the series, and I will
follow up with some patches to fix this.

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Laszlo Ersek]

On 10/24/16 10:04, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> This series intends to solve the following BZs:
>>
>>   <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>>   the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>>   files
>>
>>   <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>>   Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>>   DSC files
>>
>> Public branch:
>> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>>
>> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
>> following MdePkg library class APIs:
>>
>>   BaseLib:
>>   - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>>   - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>>     AsciiStrToUnicodeStr.
>>
>>   PcdLib:
>>   - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>>   - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>>     PcdSetExBool.
>>
>>   UefiLib:
>>   - GetVariable, GetEfiGlobalVariable.
>>
>> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
>> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
>> into ArmPkg a little bit, due to dependencies.
>>
>> I couldn't build-test some changes (for example, the only compiler
>> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
>> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
>> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
>> flag). For all of these, I liberally sprinkled the patches with Cc's and
>> Notes sections, asking for help. I did make an honest effort to build
>> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
>> could think of, perusing the various !if directives in the DSC files.
>>
> 
> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
> 
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT

Thank you!

Then,

> * In general, RVCT tends to fall over quite regularly due to its
> finicky diagnostics, so I did have to apply this patch
> 
> """
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index ca61ac5e1983..1098d9501cc7 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -891,7 +891,7 @@ NorFlashRead (
>    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> 
>    // Readout the data
> -  AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
> +  AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);
> 
>    return EFI_SUCCESS;
>  }

RVCT is correct here; standard C does not permit arithmetic on
pointers-to-void. From C99 for example,

    6.5.6 Additive operators
    ...
    Constraints

  2 For addition, either both operands shall have arithmetic type, or
    one operand shall be a pointer to an object type and the other
    shall have integer type. [...]

"Pointer to void" is not "pointer to object type".

Although in

    6.2.5 Types

we have

  27 A pointer to void shall have the same representation and alignment
     requirements as a pointer to a character type. [...]

but also:

  19 The void type comprises an empty set of values; it is an
     incomplete type that cannot be completed.

So, not an object type.

Can you submit this patch per se? It's a valid and justified change.

> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 812dafd065b2..0ef7b8d81bbc 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -71,3 +71,7 @@ [Depex]
>    # NorFlashDxe must be loaded before VariableRuntimeDxe in case
> empty flash needs populating with default values
>    #
>    BEFORE gVariableRuntimeDxeFileGuid
> +
> +[BuildOptions]
> +  RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
> +
> """

No input on this though :)

> but these changes are entirely unrelated to the series, and I will
> follow up with some patches to fix this.
> 

Yes, please.

Thanks!
Laszlo

Re: [PATCH 08/19] OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls

[Laszlo Ersek]

On 10/24/16 06:45, Gary Lin wrote:
> On Fri, Oct 21, 2016 at 11:27:26PM +0200, Laszlo Ersek wrote:
>> These are deprecated / disabled under the
>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>
>> Introduce a variable called PcdStatus, and use it to assert the success of
>> these operations (there is no reason for them to fail here).
>>
> 
> Reviewed-by: Gary Lin <glin@suse.com> and Tested-by: Gary Lin <glin@suse.com>

Thank you, Gary!
Laszlo

> 
>> Cc: Anthony PERARD <anthony.perard@citrix.com>
>> Cc: Gary Lin <glin@suse.com>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     The ReserveEmuVariableNvStore() and InitializeXen() hunks make no
>>     difference in my setup, so testing for those would be appreciated.
>>
>>  OvmfPkg/PlatformPei/MemDetect.c | 11 +++--
>>  OvmfPkg/PlatformPei/Platform.c  | 44 +++++++++++++-------
>>  OvmfPkg/PlatformPei/Xen.c       |  5 ++-
>>  3 files changed, 40 insertions(+), 20 deletions(-)
>>
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>> index 4863eb101067..d00a570d4381 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -107,6 +107,7 @@ GetFirstNonAddress (
>>    FIRMWARE_CONFIG_ITEM FwCfgItem;
>>    UINTN                FwCfgSize;
>>    UINT64               HotPlugMemoryEnd;
>> +  RETURN_STATUS        PcdStatus;
>>  
>>    FirstNonAddress = BASE_4GB + GetSystemMemorySizeAbove4gb ();
>>  
>> @@ -154,7 +155,8 @@ GetFirstNonAddress (
>>      if (mBootMode != BOOT_ON_S3_RESUME) {
>>        DEBUG ((EFI_D_INFO, "%a: disabling 64-bit PCI host aperture\n",
>>          __FUNCTION__));
>> -      PcdSet64 (PcdPciMmio64Size, 0);
>> +      PcdStatus = PcdSet64S (PcdPciMmio64Size, 0);
>> +      ASSERT_RETURN_ERROR (PcdStatus);
>>      }
>>  
>>      //
>> @@ -202,8 +204,11 @@ GetFirstNonAddress (
>>      // the GCD memory space map through our PciHostBridgeLib instance; here we
>>      // only need to set the PCDs.
>>      //
>> -    PcdSet64 (PcdPciMmio64Base, Pci64Base);
>> -    PcdSet64 (PcdPciMmio64Size, Pci64Size);
>> +    PcdStatus = PcdSet64S (PcdPciMmio64Base, Pci64Base);
>> +    ASSERT_RETURN_ERROR (PcdStatus);
>> +    PcdStatus = PcdSet64S (PcdPciMmio64Size, Pci64Size);
>> +    ASSERT_RETURN_ERROR (PcdStatus);
>> +
>>      DEBUG ((EFI_D_INFO, "%a: Pci64Base=0x%Lx Pci64Size=0x%Lx\n",
>>        __FUNCTION__, Pci64Base, Pci64Size));
>>    }
>> diff --git a/OvmfPkg/PlatformPei/Platform.c b/OvmfPkg/PlatformPei/Platform.c
>> index ca1e6dc7e320..c6e1106c9ed0 100644
>> --- a/OvmfPkg/PlatformPei/Platform.c
>> +++ b/OvmfPkg/PlatformPei/Platform.c
>> @@ -156,8 +156,9 @@ MemMapInitialization (
>>    VOID
>>    )
>>  {
>> -  UINT64 PciIoBase;
>> -  UINT64 PciIoSize;
>> +  UINT64        PciIoBase;
>> +  UINT64        PciIoSize;
>> +  RETURN_STATUS PcdStatus;
>>  
>>    PciIoBase = 0xC000;
>>    PciIoSize = 0x4000;
>> @@ -212,8 +213,11 @@ MemMapInitialization (
>>      //
>>      PciSize = 0xFC000000 - PciBase;
>>      AddIoMemoryBaseSizeHob (PciBase, PciSize);
>> -    PcdSet64 (PcdPciMmio32Base, PciBase);
>> -    PcdSet64 (PcdPciMmio32Size, PciSize);
>> +    PcdStatus = PcdSet64S (PcdPciMmio32Base, PciBase);
>> +    ASSERT_RETURN_ERROR (PcdStatus);
>> +    PcdStatus = PcdSet64S (PcdPciMmio32Size, PciSize);
>> +    ASSERT_RETURN_ERROR (PcdStatus);
>> +
>>      AddIoMemoryBaseSizeHob (0xFEC00000, SIZE_4KB);
>>      AddIoMemoryBaseSizeHob (0xFED00000, SIZE_1KB);
>>      if (mHostBridgeDevId == INTEL_Q35_MCH_DEVICE_ID) {
>> @@ -266,8 +270,10 @@ MemMapInitialization (
>>      PciIoBase,
>>      PciIoSize
>>      );
>> -  PcdSet64 (PcdPciIoBase, PciIoBase);
>> -  PcdSet64 (PcdPciIoSize, PciIoSize);
>> +  PcdStatus = PcdSet64S (PcdPciIoBase, PciIoBase);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>> +  PcdStatus = PcdSet64S (PcdPciIoSize, PciIoSize);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>>  }
>>  
>>  EFI_STATUS
>> @@ -316,11 +322,13 @@ GetNamedFwCfgBoolean (
>>  
>>  #define UPDATE_BOOLEAN_PCD_FROM_FW_CFG(TokenName)                   \
>>            do {                                                      \
>> -            BOOLEAN Setting;                                        \
>> +            BOOLEAN       Setting;                                  \
>> +            RETURN_STATUS PcdStatus;                                \
>>                                                                      \
>>              if (!EFI_ERROR (GetNamedFwCfgBoolean (                  \
>>                                "opt/ovmf/" #TokenName, &Setting))) { \
>> -              PcdSetBool (TokenName, Setting);                      \
>> +              PcdStatus = PcdSetBoolS (TokenName, Setting);         \
>> +              ASSERT_RETURN_ERROR (PcdStatus);                      \
>>              }                                                       \
>>            } while (0)
>>  
>> @@ -379,12 +387,13 @@ MiscInitialization (
>>    VOID
>>    )
>>  {
>> -  UINTN  PmCmd;
>> -  UINTN  Pmba;
>> -  UINT32 PmbaAndVal;
>> -  UINT32 PmbaOrVal;
>> -  UINTN  AcpiCtlReg;
>> -  UINT8  AcpiEnBit;
>> +  UINTN         PmCmd;
>> +  UINTN         Pmba;
>> +  UINT32        PmbaAndVal;
>> +  UINT32        PmbaOrVal;
>> +  UINTN         AcpiCtlReg;
>> +  UINT8         AcpiEnBit;
>> +  RETURN_STATUS PcdStatus;
>>  
>>    //
>>    // Disable A20 Mask
>> @@ -424,7 +433,8 @@ MiscInitialization (
>>        ASSERT (FALSE);
>>        return;
>>    }
>> -  PcdSet16 (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
>> +  PcdStatus = PcdSet16S (PcdOvmfHostBridgePciDevId, mHostBridgeDevId);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>>  
>>    //
>>    // If the appropriate IOspace enable bit is set, assume the ACPI PMBA
>> @@ -491,6 +501,7 @@ ReserveEmuVariableNvStore (
>>    )
>>  {
>>    EFI_PHYSICAL_ADDRESS VariableStore;
>> +  RETURN_STATUS        PcdStatus;
>>  
>>    //
>>    // Allocate storage for NV variables early on so it will be
>> @@ -509,7 +520,8 @@ ReserveEmuVariableNvStore (
>>            VariableStore,
>>            (2 * PcdGet32 (PcdFlashNvStorageFtwSpareSize)) / 1024
>>          ));
>> -  PcdSet64 (PcdEmuVariableNvStoreReserved, VariableStore);
>> +  PcdStatus = PcdSet64S (PcdEmuVariableNvStoreReserved, VariableStore);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>>  }
>>  
>>  
>> diff --git a/OvmfPkg/PlatformPei/Xen.c b/OvmfPkg/PlatformPei/Xen.c
>> index 223908a4f529..ab38f97a67aa 100644
>> --- a/OvmfPkg/PlatformPei/Xen.c
>> +++ b/OvmfPkg/PlatformPei/Xen.c
>> @@ -210,6 +210,8 @@ InitializeXen (
>>    VOID
>>    )
>>  {
>> +  RETURN_STATUS PcdStatus;
>> +
>>    if (mXenLeaf == 0) {
>>      return EFI_NOT_FOUND;
>>    }
>> @@ -222,7 +224,8 @@ InitializeXen (
>>    //
>>    AddReservedMemoryBaseSizeHob (0xFC000000, 0x1000000, FALSE);
>>  
>> -  PcdSetBool (PcdPciDisableBusEnumeration, TRUE);
>> +  PcdStatus = PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
>> +  ASSERT_RETURN_ERROR (PcdStatus);
>>  
>>    return EFI_SUCCESS;
>>  }
>> -- 
>> 2.9.2
>>
>>
>>

Re: [PATCH 18/19] ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with AsciiStrCatS()

[Laszlo Ersek]

On 10/24/16 09:57, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> AsciiStrCat() is deprecated / disabled under the
>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>
>> The caller of CpsrString() is required to pass in "ReturnStr" with 32
>> CHAR8 elements. (DefaultExceptionHandler() complies with this.) "Str" is
>> used to build "ReturnStr" gradually. Just before calling AsciiStrCat(),
>> "Str" points to the then-terminating NUL character in "ReturnStr".
>>
>> The difference (Str - ReturnStr) gives the number of non-NUL characters
>> we've written thus far, hence (32 - (Str - ReturnStr)) yields the number
>> of remaining bytes in ReturnStr, including the ultimately terminating NUL
>> character.
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     - build-tested only
>>
>>     - Michael (CC'd) had posted a patch earlier for this:
>>       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>>       but I only noticed that now that he pointed it out, in
>>       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>>       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>>       strongly either way.
>>
>>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> index aece26355e2e..4b2ee9a9acf7 100644
>> --- a/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> +++ b/ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c
>> @@ -116,7 +116,10 @@ CpsrString (
>>      break;
>>    }
>>
>> -  AsciiStrCat (Str, ModeStr);
>> +  //
>> +  // See the interface contract in the leading comment block.
>> +  //
>> +  AsciiStrCatS (Str, 32 - (Str - ReturnStr), ModeStr);
>>    return;
> 
> Could you please use a symbolic constant for this '32', and replace it
> in the declaration of CpsrStr[] as well?
> 
> With that
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> Oh, and if the bogus 'return' happens to get lost along the way as
> well, I would not mind.
> 

Okay, will do. :)

Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()

[Jordan Justen]

On 2016-10-24 00:58:46, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
> > AsciiStrCat() is deprecated / disabled under the
> > DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
> >
> > The "Str" variable serves no particular purpose in the MRegList() and
> > ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
> > "mThumbMregListStr" global variable (as appropriate), so that the new
> > AsciiStrCatS() calls are as clear as possible.
> >
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Leif Lindholm <leif.lindholm@linaro.org>
> > Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> 
> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> 
> > ---
> >
> > Notes:
> >     - build-tested only
> >
> >     - Michael (CC'd) had posted a patch earlier for this:
> >       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
> >       but I only noticed that now that he pointed it out, in
> >       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
> >       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
> >       strongly either way.
> >
> 
> Apologies to Michael for ignoring his contribution, but I'd be happy
> for Laszlo to merge this as part of the series.
> 
> >  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c   | 22 +++++++++-----------
> >  ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
> >  2 files changed, 19 insertions(+), 23 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > index 29a8d4438622..8551edc379c1 100644
> > --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
> > @@ -88,12 +88,10 @@ MRegList (
> >    )
> >  {
> >    UINTN     Index, Start, End;
> > -  CHAR8     *Str;
> >    BOOLEAN   First;
> >
> > -  Str = mMregListStr;
> > -  *Str = '\0';
> > -  AsciiStrCat  (Str, "{");
> > +  mMregListStr[0] = '\0';
> > +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "{");

Maybe drop the extra space after the function name throughout this
patch. (There are 2 instead of just one.)

-Jordan

> >    for (Index = 0, First = TRUE; Index <= 15; Index++) {
> >      if ((OpCode & (1 << Index)) != 0) {
> >        Start = End = Index;
> > @@ -102,25 +100,25 @@ MRegList (
> >        }
> >
> >        if (!First) {
> > -        AsciiStrCat  (Str, ",");
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ",");
> >        } else {
> >          First = FALSE;
> >        }
> >
> >        if (Start == End) {
> > -        AsciiStrCat  (Str, gReg[Start]);
> > -        AsciiStrCat  (Str, ", ");
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ", ");
> >        } else {
> > -        AsciiStrCat  (Str, gReg[Start]);
> > -        AsciiStrCat  (Str, "-");
> > -        AsciiStrCat  (Str, gReg[End]);
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "-");
> > +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[End]);
> >        }
> >      }
> >    }
> >    if (First) {
> > -    AsciiStrCat  (Str, "ERROR");
> > +    AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "ERROR");
> >    }
> > -  AsciiStrCat  (Str, "}");
> > +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "}");
> >
> >    // BugBug: Make caller pass in buffer it is cleaner
> >    return mMregListStr;
> > diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > index 5bad3afcfbf6..86d5083cb189 100644
> > --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
> > @@ -397,12 +397,10 @@ ThumbMRegList (
> >    )
> >  {
> >    UINTN     Index, Start, End;
> > -  CHAR8     *Str;
> >    BOOLEAN   First;
> >
> > -  Str = mThumbMregListStr;
> > -  *Str = '\0';
> > -  AsciiStrCat  (Str, "{");
> > +  mThumbMregListStr[0] = '\0';
> > +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "{");
> >
> >    for (Index = 0, First = TRUE; Index <= 15; Index++) {
> >      if ((RegBitMask & (1 << Index)) != 0) {
> > @@ -412,24 +410,24 @@ ThumbMRegList (
> >        }
> >
> >        if (!First) {
> > -        AsciiStrCat  (Str, ",");
> > +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, ",");
> >        } else {
> >          First = FALSE;
> >        }
> >
> >        if (Start == End) {
> > -        AsciiStrCat  (Str, gReg[Start]);
> > +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> >        } else {
> > -        AsciiStrCat  (Str, gReg[Start]);
> > -        AsciiStrCat  (Str, "-");
> > -        AsciiStrCat  (Str, gReg[End]);
> > +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
> > +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "-");
> > +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
> >        }
> >      }
> >    }
> >    if (First) {
> > -    AsciiStrCat  (Str, "ERROR");
> > +    AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
> >    }
> > -  AsciiStrCat  (Str, "}");
> > +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "}");
> >
> >    // BugBug: Make caller pass in buffer it is cleaner
> >    return mThumbMregListStr;
> > --
> > 2.9.2
> >
> >
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Jordan Justen]

Series Reviewed-by: Jordan Justen <jordan.l.justen@intel.com>

On 2016-10-21 14:27:18, Laszlo Ersek wrote:
> This series intends to solve the following BZs:
> 
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>   the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>   files
> 
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>   Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>   DSC files
> 
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
> 
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
> 
>   BaseLib:
>   - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>   - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>     AsciiStrToUnicodeStr.
> 
>   PcdLib:
>   - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>   - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>     PcdSetExBool.
> 
>   UefiLib:
>   - GetVariable, GetEfiGlobalVariable.
> 
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
> 
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
> 
> I recommend the "--word-diff" option of git-show, as a tool to assist
> with the review.
> 
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (19):
>   MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>   OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
>   OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
>   OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX()
>     calls
>   OvmfPkg: disable deprecated interfaces
>   ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
>   ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with
>     AsciiStrCatS()
>   ArmVirtPkg: disable deprecated interfaces
> 
>  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c                     | 22 +++++-----
>  ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c                   | 20 ++++-----
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c |  5 ++-
>  ArmVirtPkg/ArmVirt.dsc.inc                                              |  6 +++
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c                | 13 ++++--
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c  |  4 +-
>  ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c                            | 18 ++++----
>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c  | 13 ++++--
>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c          | 10 +++--
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c                  |  6 ++-
>  MdePkg/Include/Library/DebugLib.h                                       | 27 ++++++++++++
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                  | 12 ++++--
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c                    | 14 +++++--
>  OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c               | 10 +++--
>  OvmfPkg/OvmfPkgIa32.dsc                                                 |  7 ++++
>  OvmfPkg/OvmfPkgIa32X64.dsc                                              |  7 ++++
>  OvmfPkg/OvmfPkgX64.dsc                                                  |  7 ++++
>  OvmfPkg/PlatformDxe/Platform.c                                          |  8 +++-
>  OvmfPkg/PlatformPei/MemDetect.c                                         | 11 +++--
>  OvmfPkg/PlatformPei/Platform.c                                          | 44 +++++++++++++-------
>  OvmfPkg/PlatformPei/Xen.c                                               |  5 ++-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c                 | 13 ++++--
>  OvmfPkg/XenBusDxe/XenBusDxe.inf                                         |  1 +
>  OvmfPkg/XenBusDxe/XenStore.c                                            | 22 +++++-----
>  24 files changed, 213 insertions(+), 92 deletions(-)
> 
> -- 
> 2.9.2
> 

Re: [PATCH 17/19] ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()

[Laszlo Ersek]

On 10/24/16 20:47, Jordan Justen wrote:
> On 2016-10-24 00:58:46, Ard Biesheuvel wrote:
>> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>>> AsciiStrCat() is deprecated / disabled under the
>>> DISABLE_NEW_DEPRECATED_INTERFACES feature test macro.
>>>
>>> The "Str" variable serves no particular purpose in the MRegList() and
>>> ThumbMRegList() functions; replace it with the pointed-to "mMregListStr" /
>>> "mThumbMregListStr" global variable (as appropriate), so that the new
>>> AsciiStrCatS() calls are as clear as possible.
>>>
>>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> Cc: Leif Lindholm <leif.lindholm@linaro.org>
>>> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=164
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=165
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>>> ---
>>>
>>> Notes:
>>>     - build-tested only
>>>
>>>     - Michael (CC'd) had posted a patch earlier for this:
>>>       <https://lists.01.org/pipermail/edk2-devel/2016-August/000489.html>,
>>>       but I only noticed that now that he pointed it out, in
>>>       <https://lists.01.org/pipermail/edk2-devel/2016-October/003121.html>.
>>>       I'll leave it to the ArmPkg maintainers to choose one; I don't feel
>>>       strongly either way.
>>>
>>
>> Apologies to Michael for ignoring his contribution, but I'd be happy
>> for Laszlo to merge this as part of the series.
>>
>>>  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c   | 22 +++++++++-----------
>>>  ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c | 20 ++++++++----------
>>>  2 files changed, 19 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> index 29a8d4438622..8551edc379c1 100644
>>> --- a/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> +++ b/ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c
>>> @@ -88,12 +88,10 @@ MRegList (
>>>    )
>>>  {
>>>    UINTN     Index, Start, End;
>>> -  CHAR8     *Str;
>>>    BOOLEAN   First;
>>>
>>> -  Str = mMregListStr;
>>> -  *Str = '\0';
>>> -  AsciiStrCat  (Str, "{");
>>> +  mMregListStr[0] = '\0';
>>> +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "{");
> 
> Maybe drop the extra space after the function name throughout this
> patch. (There are 2 instead of just one.)

Yep, I saw that; I couldn't decide whether to remove them or to leave
them undisturbed.

I will remove them.

Thanks!
Laszlo

> 
> -Jordan
> 
>>>    for (Index = 0, First = TRUE; Index <= 15; Index++) {
>>>      if ((OpCode & (1 << Index)) != 0) {
>>>        Start = End = Index;
>>> @@ -102,25 +100,25 @@ MRegList (
>>>        }
>>>
>>>        if (!First) {
>>> -        AsciiStrCat  (Str, ",");
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ",");
>>>        } else {
>>>          First = FALSE;
>>>        }
>>>
>>>        if (Start == End) {
>>> -        AsciiStrCat  (Str, gReg[Start]);
>>> -        AsciiStrCat  (Str, ", ");
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, ", ");
>>>        } else {
>>> -        AsciiStrCat  (Str, gReg[Start]);
>>> -        AsciiStrCat  (Str, "-");
>>> -        AsciiStrCat  (Str, gReg[End]);
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[Start]);
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "-");
>>> +        AsciiStrCatS  (mMregListStr, sizeof mMregListStr, gReg[End]);
>>>        }
>>>      }
>>>    }
>>>    if (First) {
>>> -    AsciiStrCat  (Str, "ERROR");
>>> +    AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "ERROR");
>>>    }
>>> -  AsciiStrCat  (Str, "}");
>>> +  AsciiStrCatS  (mMregListStr, sizeof mMregListStr, "}");
>>>
>>>    // BugBug: Make caller pass in buffer it is cleaner
>>>    return mMregListStr;
>>> diff --git a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> index 5bad3afcfbf6..86d5083cb189 100644
>>> --- a/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> +++ b/ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c
>>> @@ -397,12 +397,10 @@ ThumbMRegList (
>>>    )
>>>  {
>>>    UINTN     Index, Start, End;
>>> -  CHAR8     *Str;
>>>    BOOLEAN   First;
>>>
>>> -  Str = mThumbMregListStr;
>>> -  *Str = '\0';
>>> -  AsciiStrCat  (Str, "{");
>>> +  mThumbMregListStr[0] = '\0';
>>> +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "{");
>>>
>>>    for (Index = 0, First = TRUE; Index <= 15; Index++) {
>>>      if ((RegBitMask & (1 << Index)) != 0) {
>>> @@ -412,24 +410,24 @@ ThumbMRegList (
>>>        }
>>>
>>>        if (!First) {
>>> -        AsciiStrCat  (Str, ",");
>>> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, ",");
>>>        } else {
>>>          First = FALSE;
>>>        }
>>>
>>>        if (Start == End) {
>>> -        AsciiStrCat  (Str, gReg[Start]);
>>> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
>>>        } else {
>>> -        AsciiStrCat  (Str, gReg[Start]);
>>> -        AsciiStrCat  (Str, "-");
>>> -        AsciiStrCat  (Str, gReg[End]);
>>> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[Start]);
>>> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "-");
>>> +        AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, gReg[End]);
>>>        }
>>>      }
>>>    }
>>>    if (First) {
>>> -    AsciiStrCat  (Str, "ERROR");
>>> +    AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "ERROR");
>>>    }
>>> -  AsciiStrCat  (Str, "}");
>>> +  AsciiStrCatS  (mThumbMregListStr, sizeof mThumbMregListStr, "}");
>>>
>>>    // BugBug: Make caller pass in buffer it is cleaner
>>>    return mThumbMregListStr;
>>> --
>>> 2.9.2
>>>
>>>
>> _______________________________________________
>> edk2-devel mailing list
>> edk2-devel@lists.01.org
>> https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Laszlo Ersek]

Mike, Liming,

On 10/21/16 23:27, Laszlo Ersek wrote:
> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> - the replacement text calls EFI_ERROR(),
> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
> 
> While
> 
>   ASSERT (!RETURN_ERROR (StatusParameter))
> 
> would be a functional statement in BASE type modules, it would be less
> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
> StatusParameter.
> 
> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
> original macro definition and update it as follows:
> - replace EFI with RETURN,
> - wrap overlong lines in the comment block and in the code,
> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
> 
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>     one such BASE module.
> 
>  MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> index 81904325703f..3a910e6a208b 100644
> --- a/MdePkg/Include/Library/DebugLib.h
> +++ b/MdePkg/Include/Library/DebugLib.h
> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>    #define ASSERT_EFI_ERROR(StatusParameter)
>  #endif
>  
> +/**
> +  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> +
> +  If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> +  bit of PcdDebugProperyMask is set, then this macro evaluates the
> +  RETURN_STATUS value specified by StatusParameter.  If StatusParameter is an
> +  error code, then DebugAssert() is called passing in the source filename,
> +  source line number, and StatusParameter.
> +
> +  @param  StatusParameter  RETURN_STATUS value to evaluate.
> +
> +**/
> +#if !defined(MDEPKG_NDEBUG)
> +  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
> +    do {                                                                \
> +      if (DebugAssertEnabled ()) {                                      \
> +        if (RETURN_ERROR (StatusParameter)) {                           \
> +          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> +            StatusParameter));                                          \
> +          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
> +        }                                                               \
> +      }                                                                 \
> +    } while (FALSE)
> +#else
> +  #define ASSERT_RETURN_ERROR(StatusParameter)
> +#endif
> +
>  /**  
>    Macro that calls DebugAssert() if a protocol is already installed in the 
>    handle database.
> 

can I please get a maintainer review for this patch? The rest of the
series is ready to go, but it depends on this patch.

Thanks!
Laszlo

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Kinney, Michael D]

Hi Laszlo,

Sorry for the delay.  I was traveling last week.

I did see this and I have been thinking about it.
I think it does make sense to add this new macro 
for libraries of type BASE.  I am surprised we did
not run into an issue before that would have required
the introduction of this macro earlier.  Unless the
workaround has been to add #include of 
<Uefi/UefiBaseTypes.h>, which makes me think we should
review BASE libraries to make sure that extra include
is not present.

The EFI_* error codes are mapped to RETURN_* error
codes.  So the only feedback I was considering was 
to implement ASSERT_EFI_ERROR() using 
ASSERT_RETURN_ERROR(), but that might not always be
the right mapping because the RETURN_* codes are
a subset of EFI_* error codes.

Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>

Best regards,

Mike


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 24, 2016 2:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
> <liming.gao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> 
> Mike, Liming,
> 
> On 10/21/16 23:27, Laszlo Ersek wrote:
> > ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> > - the replacement text calls EFI_ERROR(),
> > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> > - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
> >
> > While
> >
> >   ASSERT (!RETURN_ERROR (StatusParameter))
> >
> > would be a functional statement in BASE type modules, it would be less
> > convenient and less informative: ASSERT_EFI_ERROR() prints the actual
> > StatusParameter.
> >
> > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
> > original macro definition and update it as follows:
> > - replace EFI with RETURN,
> > - wrap overlong lines in the comment block and in the code,
> > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> >     OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
> >     one such BASE module.
> >
> >  MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
> > index 81904325703f..3a910e6a208b 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
> >    #define ASSERT_EFI_ERROR(StatusParameter)
> >  #endif
> >
> > +/**
> > +  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> > +
> > +  If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> > +  bit of PcdDebugProperyMask is set, then this macro evaluates the
> > +  RETURN_STATUS value specified by StatusParameter.  If StatusParameter is an
> > +  error code, then DebugAssert() is called passing in the source filename,
> > +  source line number, and StatusParameter.
> > +
> > +  @param  StatusParameter  RETURN_STATUS value to evaluate.
> > +
> > +**/
> > +#if !defined(MDEPKG_NDEBUG)
> > +  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
> > +    do {                                                                \
> > +      if (DebugAssertEnabled ()) {                                      \
> > +        if (RETURN_ERROR (StatusParameter)) {                           \
> > +          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> > +            StatusParameter));                                          \
> > +          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
> > +        }                                                               \
> > +      }                                                                 \
> > +    } while (FALSE)
> > +#else
> > +  #define ASSERT_RETURN_ERROR(StatusParameter)
> > +#endif
> > +
> >  /**
> >    Macro that calls DebugAssert() if a protocol is already installed in the
> >    handle database.
> >
> 
> can I please get a maintainer review for this patch? The rest of the
> series is ready to go, but it depends on this patch.
> 
> Thanks!
> Laszlo

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Laszlo Ersek]

On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
> 
> Sorry for the delay.  I was traveling last week.
> 
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro 
> for libraries of type BASE.  I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier.  Unless the
> workaround has been to add #include of 
> <Uefi/UefiBaseTypes.h>, which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.

I spent a few minutes on the following shell script, to identify such
libraries:

{

  # Locate the INF files that have a LIBRARY_CLASS define with a client
  # module type list that explicitly includes BASE
  git grep -l -E '\<LIBRARY_CLASS *= *[A-Za-z0-9_]+ *\|.*\<BASE\>' -- \
      '*.inf'

  # Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
  # define without a client type list.
  git grep -l -E '\<MODULE_TYPE *= *BASE\>' -- '*inf' \
  | xargs -r -- grep -l -E '\<LIBRARY_CLASS\>[^|]+$' --

} \
| {

  # Cut off the last pathname component, in order to get the pathname of
  # the directory containing the INF file
  rev | cut -f 2- -d / | rev

  } \
| {

  # If a directory has several matching INF files, list the directory
  # only once.
  sort -u

  } \
| {

    # Check if any file in these directories includes
    # "Uefi/UefiBaseType.h".
    xargs -r -- grep -r -l Uefi/UefiBaseType.h --

  }

It prints the following files:

CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c

We should likely investigate them.

I'll handle the OvmfPkg one.

> 
> The EFI_* error codes are mapped to RETURN_* error
> codes.  So the only feedback I was considering was 
> to implement ASSERT_EFI_ERROR() using 
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
> 
> Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>

Thank you!
Laszlo

> Best regards,
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming
>> <liming.gao@intel.com>
>> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>>   ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>>     one such BASE module.
>>>
>>>  MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>>    #define ASSERT_EFI_ERROR(StatusParameter)
>>>  #endif
>>>
>>> +/**
>>> +  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> +  If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> +  bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> +  RETURN_STATUS value specified by StatusParameter.  If StatusParameter is an
>>> +  error code, then DebugAssert() is called passing in the source filename,
>>> +  source line number, and StatusParameter.
>>> +
>>> +  @param  StatusParameter  RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> +  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
>>> +    do {                                                                \
>>> +      if (DebugAssertEnabled ()) {                                      \
>>> +        if (RETURN_ERROR (StatusParameter)) {                           \
>>> +          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> +            StatusParameter));                                          \
>>> +          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
>>> +        }                                                               \
>>> +      }                                                                 \
>>> +    } while (FALSE)
>>> +#else
>>> +  #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>>  /**
>>>    Macro that calls DebugAssert() if a protocol is already installed in the
>>>    handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
> 

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Zeng, Star]

In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR().

Thanks,
Star
-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
Sent: Tuesday, October 25, 2016 7:05 AM
To: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

Hi Laszlo,

Sorry for the delay.  I was traveling last week.

I did see this and I have been thinking about it.
I think it does make sense to add this new macro for libraries of type BASE.  I am surprised we did not run into an issue before that would have required the introduction of this macro earlier.  Unless the workaround has been to add #include of <Uefi/UefiBaseTypes.h>, which makes me think we should review BASE libraries to make sure that extra include is not present.

The EFI_* error codes are mapped to RETURN_* error codes.  So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes.

Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>

Best regards,

Mike


> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Monday, October 24, 2016 2:00 PM
> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
> <liming.gao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add 
> ASSERT_RETURN_ERROR()
> 
> Mike, Liming,
> 
> On 10/21/16 23:27, Laszlo Ersek wrote:
> > ASSERT_EFI_ERROR() cannot be used in BASE type modules because
> > - the replacement text calls EFI_ERROR(),
> > - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
> > - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
> >
> > While
> >
> >   ASSERT (!RETURN_ERROR (StatusParameter))
> >
> > would be a functional statement in BASE type modules, it would be 
> > less convenient and less informative: ASSERT_EFI_ERROR() prints the 
> > actual StatusParameter.
> >
> > Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). 
> > Copy the original macro definition and update it as follows:
> > - replace EFI with RETURN,
> > - wrap overlong lines in the comment block and in the code,
> > - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
> >
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> > ---
> >
> > Notes:
> >     OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
> >     one such BASE module.
> >
> >  MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/MdePkg/Include/Library/DebugLib.h 
> > b/MdePkg/Include/Library/DebugLib.h
> > index 81904325703f..3a910e6a208b 100644
> > --- a/MdePkg/Include/Library/DebugLib.h
> > +++ b/MdePkg/Include/Library/DebugLib.h
> > @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
> >    #define ASSERT_EFI_ERROR(StatusParameter)  #endif
> >
> > +/**
> > +  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
> > +
> > +  If MDEPKG_NDEBUG is not defined and the 
> > + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
> > +  bit of PcdDebugProperyMask is set, then this macro evaluates the  
> > + RETURN_STATUS value specified by StatusParameter.  If 
> > + StatusParameter is an  error code, then DebugAssert() is called 
> > + passing in the source filename,  source line number, and StatusParameter.
> > +
> > +  @param  StatusParameter  RETURN_STATUS value to evaluate.
> > +
> > +**/
> > +#if !defined(MDEPKG_NDEBUG)
> > +  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
> > +    do {                                                                \
> > +      if (DebugAssertEnabled ()) {                                      \
> > +        if (RETURN_ERROR (StatusParameter)) {                           \
> > +          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
> > +            StatusParameter));                                          \
> > +          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
> > +        }                                                               \
> > +      }                                                                 \
> > +    } while (FALSE)
> > +#else
> > +  #define ASSERT_RETURN_ERROR(StatusParameter)
> > +#endif
> > +
> >  /**
> >    Macro that calls DebugAssert() if a protocol is already installed in the
> >    handle database.
> >
> 
> can I please get a maintainer review for this patch? The rest of the 
> series is ready to go, but it depends on this patch.
> 
> Thanks!
> Laszlo

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Re: [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces

[Laszlo Ersek]

Ard,

On 10/21/16 23:27, Laszlo Ersek wrote:
> At this point no code in ArmVirtPkg (and apparently no code outside of
> ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
> so we can disable them in the common platform DSC include file:
> 
> BaseLib:
> - StrCpy
> - StrnCpy
> - StrCat
> - StrnCat
> - UnicodeStrToAsciiStr
> - AsciiStrCpy
> - AsciiStrnCpy
> - AsciiStrCat
> - AsciiStrnCat
> - AsciiStrToUnicodeStr
> 
> PcdLib:
> - PcdSet8
> - PcdSet16
> - PcdSet32
> - PcdSet64
> - PcdSetPtr
> - PcdSetBool
> - PcdSetEx8
> - PcdSetEx16
> - PcdSetEx32
> - PcdSetEx64
> - PcdSetExPtr
> - PcdSetExBool
> 
> UefiLib:
> - GetVariable
> - GetEfiGlobalVariable
> 
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     not build-tested with RVCT
> 
>  ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
> index c071988ad8f5..dbd6678accde 100644
> --- a/ArmVirtPkg/ArmVirt.dsc.inc
> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
> @@ -246,6 +246,12 @@ [BuildOptions]
>  
>    GCC:RELEASE_*_*_CC_FLAGS  = -DMDEPKG_NDEBUG
>  
> +  #
> +  # Disable deprecated APIs.
> +  #
> +  RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
> +  GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
> +
>  ################################################################################
>  #
>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
> 

Can you please give an R-b for this too?

Thanks
Laszlo

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Laszlo Ersek]

On 10/24/16 10:04, Ard Biesheuvel wrote:
> On 21 October 2016 at 22:27, Laszlo Ersek <lersek@redhat.com> wrote:
>> This series intends to solve the following BZs:
>>
>>   <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>>   the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>>   files
>>
>>   <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>>   Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>>   DSC files
>>
>> Public branch:
>> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
>>
>> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
>> following MdePkg library class APIs:
>>
>>   BaseLib:
>>   - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>>   - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>>     AsciiStrToUnicodeStr.
>>
>>   PcdLib:
>>   - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>>   - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>>     PcdSetExBool.
>>
>>   UefiLib:
>>   - GetVariable, GetEfiGlobalVariable.
>>
>> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
>> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
>> into ArmPkg a little bit, due to dependencies.
>>
>> I couldn't build-test some changes (for example, the only compiler
>> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
>> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
>> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
>> flag). For all of these, I liberally sprinkled the patches with Cc's and
>> Notes sections, asking for help. I did make an honest effort to build
>> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
>> could think of, perusing the various !if directives in the DSC files.
>>
> 
> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
> 
> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT

Okay, I'll add this tag to the ArmVirtPkg patches.

Thanks,
Laszlo


> 
> Thanks,
> Ard.
> 
> 
> * In general, RVCT tends to fall over quite regularly due to its
> finicky diagnostics, so I did have to apply this patch
> 
> """
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index ca61ac5e1983..1098d9501cc7 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -891,7 +891,7 @@ NorFlashRead (
>    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
> 
>    // Readout the data
> -  AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
> +  AlignedCopyMem (Buffer, (VOID *)(StartAddress + Offset), BufferSizeInBytes);
> 
>    return EFI_SUCCESS;
>  }
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 812dafd065b2..0ef7b8d81bbc 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -71,3 +71,7 @@ [Depex]
>    # NorFlashDxe must be loaded before VariableRuntimeDxe in case
> empty flash needs populating with default values
>    #
>    BEFORE gVariableRuntimeDxeFileGuid
> +
> +[BuildOptions]
> +  RVCT:*_*_*_CC_FLAGS = --diag_suppress=6314
> +
> """
> 
> but these changes are entirely unrelated to the series, and I will
> follow up with some patches to fix this.
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 19/19] ArmVirtPkg: disable deprecated interfaces

[Ard Biesheuvel]

On 25 October 2016 at 09:25, Laszlo Ersek <lersek@redhat.com> wrote:
> Ard,
>
> On 10/21/16 23:27, Laszlo Ersek wrote:
>> At this point no code in ArmVirtPkg (and apparently no code outside of
>> ArmVirtPkg that the ArmVirt binaries depend on) uses the deprecated APIs,
>> so we can disable them in the common platform DSC include file:
>>
>> BaseLib:
>> - StrCpy
>> - StrnCpy
>> - StrCat
>> - StrnCat
>> - UnicodeStrToAsciiStr
>> - AsciiStrCpy
>> - AsciiStrnCpy
>> - AsciiStrCat
>> - AsciiStrnCat
>> - AsciiStrToUnicodeStr
>>
>> PcdLib:
>> - PcdSet8
>> - PcdSet16
>> - PcdSet32
>> - PcdSet64
>> - PcdSetPtr
>> - PcdSetBool
>> - PcdSetEx8
>> - PcdSetEx16
>> - PcdSetEx32
>> - PcdSetEx64
>> - PcdSetExPtr
>> - PcdSetExBool
>>
>> UefiLib:
>> - GetVariable
>> - GetEfiGlobalVariable
>>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     not build-tested with RVCT
>>
>>  ArmVirtPkg/ArmVirt.dsc.inc | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/ArmVirtPkg/ArmVirt.dsc.inc b/ArmVirtPkg/ArmVirt.dsc.inc
>> index c071988ad8f5..dbd6678accde 100644
>> --- a/ArmVirtPkg/ArmVirt.dsc.inc
>> +++ b/ArmVirtPkg/ArmVirt.dsc.inc
>> @@ -246,6 +246,12 @@ [BuildOptions]
>>
>>    GCC:RELEASE_*_*_CC_FLAGS  = -DMDEPKG_NDEBUG
>>
>> +  #
>> +  # Disable deprecated APIs.
>> +  #
>> +  RVCT:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
>> +  GCC:*_*_*_CC_FLAGS = -DDISABLE_NEW_DEPRECATED_INTERFACES
>> +
>>  ################################################################################
>>  #
>>  # Pcd Section - list of all EDK II PCD Entries defined by this Platform
>>
>
> Can you please give an R-b for this too?
>

Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

However, I am still a bit uneasy regarding the lack of explicit
confirmation that it is the responsibility of the person who moves
functions under a '#ifndef DISABLE_NEW_DEPRECATED_INTERFACES' to
ensure that all references have been removed from the *entire*
codebase, not just the intel bits.

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Laszlo Ersek]

On 10/25/16 10:22, Zeng, Star wrote:
> In fact, EFI_* codes are a subset of RETURN_* codes, so it seems work to implement ASSERT_EFI_ERROR() using new ASSERT_RETURN_ERROR().

Let me commit the patch as-is for now, with Mike's review, and let's
continue the discussion on the above-suggested code sharing. If everyone
agrees, I can submit a separate patch that reuses ASSERT_RETURN_ERROR in
ASSERT_EFI_ERROR.

One thing I'm not so sure about (regarding the code sharing) is that
each of these macros prints its own name, as a literal string. If we
simply redefine ASSERT_EFI_ERROR with ASSERT_RETURN_ERROR, the error
message won't match the source code any longer for the former.

We can get around this by introducing a common base macro that also
takes the name to print as a parameter. But, I think that justifies a
separate patch even more.

Thanks!
Laszlo

> Thanks,
> Star
> -----Original Message-----
> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Kinney, Michael D
> Sent: Tuesday, October 25, 2016 7:05 AM
> To: Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> 
> Hi Laszlo,
> 
> Sorry for the delay.  I was traveling last week.
> 
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro for libraries of type BASE.  I am surprised we did not run into an issue before that would have required the introduction of this macro earlier.  Unless the workaround has been to add #include of <Uefi/UefiBaseTypes.h>, which makes me think we should review BASE libraries to make sure that extra include is not present.
> 
> The EFI_* error codes are mapped to RETURN_* error codes.  So the only feedback I was considering was to implement ASSERT_EFI_ERROR() using ASSERT_RETURN_ERROR(), but that might not always be the right mapping because the RETURN_* codes are a subset of EFI_* error codes.
> 
> Reviewed-by: Michael Kinney <michael.d.kinney@intel.com>
> 
> Best regards,
> 
> Mike
> 
> 
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming 
>> <liming.gao@intel.com>
>> Cc: edk2-devel-01 <edk2-devel@ml01.01.org>
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add 
>> ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>>   ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be 
>>> less convenient and less informative: ASSERT_EFI_ERROR() prints the 
>>> actual StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). 
>>> Copy the original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao <liming.gao@intel.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>>> ---
>>>
>>> Notes:
>>>     OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>>     one such BASE module.
>>>
>>>  MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>>  1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h 
>>> b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>>    #define ASSERT_EFI_ERROR(StatusParameter)  #endif
>>>
>>> +/**
>>> +  Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> +  If MDEPKG_NDEBUG is not defined and the 
>>> + DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> +  bit of PcdDebugProperyMask is set, then this macro evaluates the  
>>> + RETURN_STATUS value specified by StatusParameter.  If 
>>> + StatusParameter is an  error code, then DebugAssert() is called 
>>> + passing in the source filename,  source line number, and StatusParameter.
>>> +
>>> +  @param  StatusParameter  RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> +  #define ASSERT_RETURN_ERROR(StatusParameter)                          \
>>> +    do {                                                                \
>>> +      if (DebugAssertEnabled ()) {                                      \
>>> +        if (RETURN_ERROR (StatusParameter)) {                           \
>>> +          DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> +            StatusParameter));                                          \
>>> +          _ASSERT (!RETURN_ERROR (StatusParameter));                    \
>>> +        }                                                               \
>>> +      }                                                                 \
>>> +    } while (FALSE)
>>> +#else
>>> +  #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>>  /**
>>>    Macro that calls DebugAssert() if a protocol is already installed in the
>>>    handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the 
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
> 
> _______________________________________________
> edk2-devel mailing list
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel
> 

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Laszlo Ersek]

On 10/25/16 10:26, Laszlo Ersek wrote:
> On 10/24/16 10:04, Ard Biesheuvel wrote:

>> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>>
>> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
> 
> Okay, I'll add this tag to the ArmVirtPkg patches.

Actually, I'm adding it to the following non-ArmVirtPkg patches as well:

- MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
- OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
- OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls

because, according to the ArmVirtQemu build report file, these modules
are pulled into ArmVirtQemu.

Thanks
Laszlo

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Ard Biesheuvel]

On 25 October 2016 at 09:45, Laszlo Ersek <lersek@redhat.com> wrote:
> On 10/25/16 10:26, Laszlo Ersek wrote:
>> On 10/24/16 10:04, Ard Biesheuvel wrote:
>
>>> I tried ArmVirtQemu.dsc in DEBUG mode with RVCTLINUX, and it built fine*
>>>
>>> Tested-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> # RVCT
>>
>> Okay, I'll add this tag to the ArmVirtPkg patches.
>
> Actually, I'm adding it to the following non-ArmVirtPkg patches as well:
>
> - MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
> - OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
> - OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
>
> because, according to the ArmVirtQemu build report file, these modules
> are pulled into ArmVirtQemu.
>

That's fine

Re: [PATCH 00/19] OvmfPkg, ArmVirtPkg: leave deprecated interfaces behind

[Laszlo Ersek]

On 10/21/16 23:27, Laszlo Ersek wrote:
> This series intends to solve the following BZs:
> 
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=166> -- OvmfPkg: Add
>   the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package DSC
>   files
> 
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=165> -- ArmVirtPkg:
>   Add the build option "/D DISABLE_NEW_DEPRECATED_INTERFACES" in package
>   DSC files
> 
> Public branch:
> <https://github.com/lersek/edk2/commits/no_deprec_ovmf_bz166_armvirt_bz165>.
> 
> The DISABLE_NEW_DEPRECATED_INTERFACES feature test macro disables the
> following MdePkg library class APIs:
> 
>   BaseLib:
>   - StrCpy, StrnCpy, StrCat, StrnCat, UnicodeStrToAsciiStr,
>   - AsciiStrCpy, AsciiStrnCpy, AsciiStrCat, AsciiStrnCat,
>     AsciiStrToUnicodeStr.
> 
>   PcdLib:
>   - PcdSet8, PcdSet16, PcdSet32, PcdSet64, PcdSetPtr, PcdSetBool,
>   - PcdSetEx8, PcdSetEx16, PcdSetEx32, PcdSetEx64, PcdSetExPtr,
>     PcdSetExBool.
> 
>   UefiLib:
>   - GetVariable, GetEfiGlobalVariable.
> 
> The series gradually weans the OvmfPkg and ArmVirtPkg modules off these.
> For the 32-bit ARM builds of ArmVirtPkg platforms, I had to dip my toes
> into ArmPkg a little bit, due to dependencies.
> 
> I couldn't build-test some changes (for example, the only compiler
> toolchains I have access to at the moment are GCC48 for Ia32/X64, and
> GCC5 for ARM/AARCH64). Some changes I could build, but not functionally
> test (Xen en bloc, 32-bit ARM, RAM-emulated variables in OVMF, -bios
> flag). For all of these, I liberally sprinkled the patches with Cc's and
> Notes sections, asking for help. I did make an honest effort to build
> the ArmVirt and OVMF platforms in as many configurations (-D ...) as I
> could think of, perusing the various !if directives in the DSC files.
> 
> I recommend the "--word-diff" option of git-show, as a tool to assist
> with the review.
> 
> Cc: Anthony PERARD <anthony.perard@citrix.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gary Lin <glin@suse.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Leif Lindholm <leif.lindholm@linaro.org>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (19):
>   MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>   OvmfPkg/XenBusDxe: eliminate AsciiStrCpy() calls
>   OvmfPkg/XenBusDxe: eliminate AsciiStrCat() calls
>   OvmfPkg/EmuVariableFvbRuntimeDxe: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/SmbiosVersionLib: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformDxe: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/PlatformPei: eliminate unchecked PcdSetXX() calls
>   OvmfPkg/QemuFlashFvbServicesRuntimeDxe: eliminate unchecked PcdSetXX()
>     calls
>   OvmfPkg: disable deprecated interfaces
>   ArmVirtPkg/ArmVirtGicArchLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/ArmVirtPL031FdtClientLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmVirtPkg/ArmVirtPlatformLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/ArmVirtTimerFdtClientLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmVirtPkg/FdtPciPcdProducerLib: eliminate unchecked PcdSetXX() calls
>   ArmVirtPkg/PlatformBootManagerLib: eliminate unchecked PcdSetXX()
>     calls
>   ArmPkg/ArmDisassemblerLib: replace AsciiStrCat() with AsciiStrCatS()
>   ArmPkg/DefaultExceptionHandlerLib: replace AsciiStrCat() with
>     AsciiStrCatS()
>   ArmVirtPkg: disable deprecated interfaces
> 
>  ArmPkg/Library/ArmDisassemblerLib/ArmDisassembler.c                     | 22 +++++-----
>  ArmPkg/Library/ArmDisassemblerLib/ThumbDisassembler.c                   | 20 ++++-----
>  ArmPkg/Library/DefaultExceptionHandlerLib/Arm/DefaultExceptionHandler.c |  5 ++-
>  ArmVirtPkg/ArmVirt.dsc.inc                                              |  6 +++
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c                | 13 ++++--
>  ArmVirtPkg/Library/ArmVirtPL031FdtClientLib/ArmVirtPL031FdtClientLib.c  |  4 +-
>  ArmVirtPkg/Library/ArmVirtPlatformLib/Virt.c                            | 18 ++++----
>  ArmVirtPkg/Library/ArmVirtTimerFdtClientLib/ArmVirtTimerFdtClientLib.c  | 13 ++++--
>  ArmVirtPkg/Library/FdtPciPcdProducerLib/FdtPciPcdProducerLib.c          | 10 +++--
>  ArmVirtPkg/Library/PlatformBootManagerLib/PlatformBm.c                  |  6 ++-
>  MdePkg/Include/Library/DebugLib.h                                       | 27 ++++++++++++
>  OvmfPkg/EmuVariableFvbRuntimeDxe/Fvb.c                                  | 12 ++++--
>  OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c                    | 14 +++++--
>  OvmfPkg/Library/SmbiosVersionLib/DetectSmbiosVersionLib.c               | 10 +++--
>  OvmfPkg/OvmfPkgIa32.dsc                                                 |  7 ++++
>  OvmfPkg/OvmfPkgIa32X64.dsc                                              |  7 ++++
>  OvmfPkg/OvmfPkgX64.dsc                                                  |  7 ++++
>  OvmfPkg/PlatformDxe/Platform.c                                          |  8 +++-
>  OvmfPkg/PlatformPei/MemDetect.c                                         | 11 +++--
>  OvmfPkg/PlatformPei/Platform.c                                          | 44 +++++++++++++-------
>  OvmfPkg/PlatformPei/Xen.c                                               |  5 ++-
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/FwBlockService.c                 | 13 ++++--
>  OvmfPkg/XenBusDxe/XenBusDxe.inf                                         |  1 +
>  OvmfPkg/XenBusDxe/XenStore.c                                            | 22 +++++-----
>  24 files changed, 213 insertions(+), 92 deletions(-)
> 

Thanks for the feedback everyone, pushed the series as
2a3263303b51..17dc8ebc0d76. Beyond adding the tags, I implemented the
following last-minute changes, as requested:

- drop extraneous space in "AsciiStrCatS  ()" [Jordan]
- introduce CPSR_STRING_SIZE, and replace the open-coded occurrences of
the integer constant 32 with it [Ard]
- remove superfluous return statement [Ard]

Thanks!
Laszlo

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Gao, Liming]

Laszlo:
  Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. MdePkg BaseLib should be BASE type. It doesn't depend on UEFI. I will clean up it. MdeModulePkg FrameBufferBltLib is designed for UEFI GOP BLT operation. This library instance type should be UEFI_DRIVER.

  I will provide the patch to clean up them.

Thanks
Liming
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, October 25, 2016 3:54 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro
> for libraries of type BASE. I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier. Unless the
> workaround has been to add #include of
> , which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.

I spent a few minutes on the following shell script, to identify such
libraries:

{

# Locate the INF files that have a LIBRARY_CLASS define with a client
# module type list that explicitly includes BASE
git grep -l -E '\' -- \
'*.inf'

# Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
# define without a client type list.
git grep -l -E '\' -- '*inf' \
| xargs -r -- grep -l -E '\[^|]+$' --

} \
| {

# Cut off the last pathname component, in order to get the pathname of
# the directory containing the INF file
rev | cut -f 2- -d / | rev

} \
| {

# If a directory has several matching INF files, list the directory
# only once.
sort -u

} \
| {

# Check if any file in these directories includes
# "Uefi/UefiBaseType.h".
xargs -r -- grep -r -l Uefi/UefiBaseType.h --

}

It prints the following files:

CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c

We should likely investigate them.

I'll handle the OvmfPkg one.

>
> The EFI_* error codes are mapped to RETURN_* error
> codes. So the only feedback I was considering was
> to implement ASSERT_EFI_ERROR() using
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney

Thank you!
Laszlo

> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D ; Gao, Liming
>>
>> Cc: edk2-devel-01
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao
>>> Cc: Michael D Kinney
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
>>> + error code, then DebugAssert() is called passing in the source filename,
>>> + source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>

Re: [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

[Kinney, Michael D]

Hi Laszlo,

I investigated the QuarkSocPkg ones.

The extra #include of BaseType.h should be removed from:
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c

However, it should not be removed from the other one:
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c

The ResetSystemLib uses EFI_TIME that is defined in BaseType.h.

I will send patch for QNCSmmLib.

Mike

From: Gao, Liming
Sent: Tuesday, October 25, 2016 3:33 AM
To: Laszlo Ersek <lersek@redhat.com>; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
Subject: RE: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

Laszlo:
  Thanks for your report them. I just investigate MdePkg and MdeModulePkg ones. MdePkg BaseLib should be BASE type. It doesn't depend on UEFI. I will clean up it. MdeModulePkg FrameBufferBltLib is designed for UEFI GOP BLT operation. This library instance type should be UEFI_DRIVER.

  I will provide the patch to clean up them.

Thanks
Liming
From: Laszlo Ersek [mailto:lersek@redhat.com]
Sent: Tuesday, October 25, 2016 3:54 PM
To: Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>>; Gao, Liming <liming.gao@intel.com<mailto:liming.gao@intel.com>>
Cc: edk2-devel-01 <edk2-devel@ml01.01.org<mailto:edk2-devel@ml01.01.org>>; Justen, Jordan L <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>; Ard Biesheuvel <ard.biesheuvel@linaro.org<mailto:ard.biesheuvel@linaro.org>>
Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()

On 10/25/16 01:05, Kinney, Michael D wrote:
> Hi Laszlo,
>
> Sorry for the delay. I was traveling last week.
>
> I did see this and I have been thinking about it.
> I think it does make sense to add this new macro
> for libraries of type BASE. I am surprised we did
> not run into an issue before that would have required
> the introduction of this macro earlier. Unless the
> workaround has been to add #include of
> , which makes me think we should
> review BASE libraries to make sure that extra include
> is not present.

I spent a few minutes on the following shell script, to identify such
libraries:

{

# Locate the INF files that have a LIBRARY_CLASS define with a client
# module type list that explicitly includes BASE
git grep -l -E '\' -- \
'*.inf'

# Locate the INF files that have MODULE_TYPE=BASE, and a LIBRARY_CLASS
# define without a client type list.
git grep -l -E '\' -- '*inf' \
| xargs -r -- grep -l -E '\[^|]+$' --

} \
| {

# Cut off the last pathname component, in order to get the pathname of
# the directory containing the INF file
rev | cut -f 2- -d / | rev

} \
| {

# If a directory has several matching INF files, list the directory
# only once.
sort -u

} \
| {

# Check if any file in these directories includes
# "Uefi/UefiBaseType.h".
xargs -r -- grep -r -l Uefi/UefiBaseType.h --

}

It prints the following files:

CorebootModulePkg/Library/CbParseLib/CbParseLib.c
CorebootPayloadPkg/Library/PlatformHookLib/PlatformHookLib.c
MdeModulePkg/Library/FrameBufferBltLib/FrameBufferBltLib.c
MdePkg/Library/BaseLib/FilePaths.c
OvmfPkg/Library/XenConsoleSerialPortLib/XenConsoleSerialPortLib.c
QuarkSocPkg/QuarkNorthCluster/Library/QNCSmmLib/QNCSmmLib.c
QuarkSocPkg/QuarkNorthCluster/Library/ResetSystemLib/ResetSystemLib.c

We should likely investigate them.

I'll handle the OvmfPkg one.

>
> The EFI_* error codes are mapped to RETURN_* error
> codes. So the only feedback I was considering was
> to implement ASSERT_EFI_ERROR() using
> ASSERT_RETURN_ERROR(), but that might not always be
> the right mapping because the RETURN_* codes are
> a subset of EFI_* error codes.
>
> Reviewed-by: Michael Kinney

Thank you!
Laszlo

> Best regards,
>
> Mike
>
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Monday, October 24, 2016 2:00 PM
>> To: Kinney, Michael D ; Gao, Liming
>>
>> Cc: edk2-devel-01
>> Subject: Re: [edk2] [PATCH 01/19] MdePkg/DebugLib.h: add ASSERT_RETURN_ERROR()
>>
>> Mike, Liming,
>>
>> On 10/21/16 23:27, Laszlo Ersek wrote:
>>> ASSERT_EFI_ERROR() cannot be used in BASE type modules because
>>> - the replacement text calls EFI_ERROR(),
>>> - EFI_ERROR() is defined in "MdePkg/Include/Uefi/UefiBaseType.h",
>>> - the inclusion of "UefiBaseType.h" is not required for BASE type modules.
>>>
>>> While
>>>
>>> ASSERT (!RETURN_ERROR (StatusParameter))
>>>
>>> would be a functional statement in BASE type modules, it would be less
>>> convenient and less informative: ASSERT_EFI_ERROR() prints the actual
>>> StatusParameter.
>>>
>>> Hence add ASSERT_RETURN_ERROR(), paralleling ASSERT_EFI_ERROR(). Copy the
>>> original macro definition and update it as follows:
>>> - replace EFI with RETURN,
>>> - wrap overlong lines in the comment block and in the code,
>>> - EFI_D_ERROR is deprecated, so employ DEBUG_ERROR instead.
>>>
>>> Cc: Liming Gao
>>> Cc: Michael D Kinney
>>> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=166
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Laszlo Ersek
>>> ---
>>>
>>> Notes:
>>> OvmfPkg/SmbiosVersionLib, modified in one of the upcoming patches, is
>>> one such BASE module.
>>>
>>> MdePkg/Include/Library/DebugLib.h | 27 ++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/MdePkg/Include/Library/DebugLib.h b/MdePkg/Include/Library/DebugLib.h
>>> index 81904325703f..3a910e6a208b 100644
>>> --- a/MdePkg/Include/Library/DebugLib.h
>>> +++ b/MdePkg/Include/Library/DebugLib.h
>>> @@ -348,6 +348,33 @@ DebugPrintLevelEnabled (
>>> #define ASSERT_EFI_ERROR(StatusParameter)
>>> #endif
>>>
>>> +/**
>>> + Macro that calls DebugAssert() if a RETURN_STATUS evaluates to an error code.
>>> +
>>> + If MDEPKG_NDEBUG is not defined and the DEBUG_PROPERTY_DEBUG_ASSERT_ENABLED
>>> + bit of PcdDebugProperyMask is set, then this macro evaluates the
>>> + RETURN_STATUS value specified by StatusParameter. If StatusParameter is an
>>> + error code, then DebugAssert() is called passing in the source filename,
>>> + source line number, and StatusParameter.
>>> +
>>> + @param StatusParameter RETURN_STATUS value to evaluate.
>>> +
>>> +**/
>>> +#if !defined(MDEPKG_NDEBUG)
>>> + #define ASSERT_RETURN_ERROR(StatusParameter) \
>>> + do { \
>>> + if (DebugAssertEnabled ()) { \
>>> + if (RETURN_ERROR (StatusParameter)) { \
>>> + DEBUG ((DEBUG_ERROR, "\nASSERT_RETURN_ERROR (Status = %r)\n", \
>>> + StatusParameter)); \
>>> + _ASSERT (!RETURN_ERROR (StatusParameter)); \
>>> + } \
>>> + } \
>>> + } while (FALSE)
>>> +#else
>>> + #define ASSERT_RETURN_ERROR(StatusParameter)
>>> +#endif
>>> +
>>> /**
>>> Macro that calls DebugAssert() if a protocol is already installed in the
>>> handle database.
>>>
>>
>> can I please get a maintainer review for this patch? The rest of the
>> series is ready to go, but it depends on this patch.
>>
>> Thanks!
>> Laszlo
>