[PATCH v3 0/6] ArmPkg/SMBIOS fixes and improvements

[PATCH v3 0/6] ArmPkg/SMBIOS fixes and improvements

[Nhi Pham]

This patchset is to update the ArmPkg/SMBIOS for fixes and improvements.

Changes since v2:
    + Add tags from Ard and Sami
    + Fix patch 3 for compilation error with OemMiscLibNull due to lack
    of gZeroGuid in the INF file.

Changes since v1:
    + Change PartNumber to ProcessorVersion [Sami]

Minh Nguyen (5):
  ArmPkg/ProcessorSubClassDxe: Get processor version from OemMiscLib
  ArmPkg: Correct return value of "SMCCC_ARCH_SOC_ID" Function ID call
  ArmPkg/SmbiosMiscDxe: Fix typo of "AssetTagType02"
  ArmPkg/SmbiosMiscDxe: Remove redundant updates in SMBIOS Type 2
  ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

Nhi Pham (1):
  ArmPkg/SmbiosMiscDxe: Support fetching System UUID

 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf                        |  4 ++
 ArmPkg/Include/Library/OemMiscLib.h                                              | 37 +++++++++++++++-
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                              | 46 ++++++++++++++++++++
 ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c                 |  8 +++-
 ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c          |  6 +--
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c            | 32 +++++++++-----
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c    |  3 +-
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c | 13 +-----
 8 files changed, 121 insertions(+), 28 deletions(-)

-- 
2.25.1

[PATCH v3 1/6] ArmPkg/ProcessorSubClassDxe: Get processor version from OemMiscLib

[Nhi Pham]

From: Minh Nguyen <minhn@amperecomputing.com>

In some scenarios, the processor version may be updated dynamically
from pre-UEFI firmware during booting. But the processor version is
fixed with PCD (PcdProcessorVersion), so it can not be updated it
dynamically. This patch will support setting that value both
statically and dynamically.

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/OemMiscLib.h                              | 2 ++
 ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index 47cb30d84a62..330bb4b014de 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -1,5 +1,6 @@
 /** @file
 *
+*  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.
 *  Copyright (c) 2021, NUVIA Inc. All rights reserved.
 *  Copyright (c) 2015, Hisilicon Limited. All rights reserved.
 *  Copyright (c) 2015, Linaro Limited. All rights reserved.
@@ -58,6 +59,7 @@ typedef enum {
   SkuNumberType03,
   ProcessorPartNumType04,
   ProcessorSerialNumType04,
+  ProcessorVersionType04,
   SmbiosHiiStringFieldMax
 } OEM_MISC_SMBIOS_HII_STRING_FIELD;
 
diff --git a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c
index 0b9af9bd7e1c..3b12e26abf6e 100644
--- a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c
+++ b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/ProcessorSubClass.c
@@ -1,6 +1,7 @@
 /** @file
   ProcessorSubClass.c
 
+  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.
   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
   Copyright (c) 2015, Hisilicon Limited. All rights reserved.
   Copyright (c) 2015, Linaro Limited. All rights reserved.
@@ -512,7 +513,6 @@ AllocateType4AndSetProcessorInformationStrings (
   PartNumber       = STRING_TOKEN (STR_PROCESSOR_PART_NUMBER);
 
   SET_HII_STRING_IF_PCD_NOT_EMPTY (PcdProcessorManufacturer, ProcessorManu);
-  SET_HII_STRING_IF_PCD_NOT_EMPTY (PcdProcessorVersion, ProcessorVersion);
   SET_HII_STRING_IF_PCD_NOT_EMPTY (PcdProcessorAssetTag, AssetTag);
 
   if (StrLen ((CHAR16 *)FixedPcdGetPtr (PcdProcessorSerialNumber)) > 0) {
@@ -527,6 +527,12 @@ AllocateType4AndSetProcessorInformationStrings (
     OemUpdateSmbiosInfo (mHiiHandle, PartNumber, ProcessorPartNumType04);
   }
 
+  if (StrLen ((CHAR16 *)FixedPcdGetPtr (PcdProcessorVersion)) > 0) {
+    HiiSetString (mHiiHandle, ProcessorVersion, (CHAR16 *)FixedPcdGetPtr (PcdProcessorVersion), NULL);
+  } else {
+    OemUpdateSmbiosInfo (mHiiHandle, ProcessorVersion, ProcessorVersionType04);
+  }
+
   // Processor Designation
   StringBufferSize = sizeof (CHAR16) * SMBIOS_STRING_MAX_LENGTH;
   ProcessorStr     = AllocateZeroPool (StringBufferSize);
-- 
2.25.1

[PATCH v3 2/6] ArmPkg: Correct return value of "SMCCC_ARCH_SOC_ID" Function ID call

[Nhi Pham]

From: Minh Nguyen <minhn@amperecomputing.com>

According to "SMC Calling Convention" specification, section 7.4,
return value of Arm Architecture Calls is stored at first argument of
SMC aguments (ARM_SMC_ARGS). This value can be negative values indicating
error or positive values (including zero) indicating success. Positive
value would contain information of respective Function ID (Section 7.3.4
and 7.4.4).

For that reason, "SMCCC_VERSION" and "SMCCC_ARCH_FEATURES"
Function ID calls read return value from "SmcCallStatus" variable
(Args.Arg0 - first argument of SMC call). But "SMCCC_ARCH_SOC_ID"
Function ID call is reading return value from "SmcParam" variable
(Args.Arg1 - second argument of SMC call) so it leads to unexpected
results of "Jep106Code" and "SocRevision". This patch is to correct it.

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c
index e0010a40e489..b961be213358 100644
--- a/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c
+++ b/ArmPkg/Universal/Smbios/ProcessorSubClassDxe/SmbiosProcessorArmCommon.c
@@ -2,7 +2,7 @@
   Functions for processor information common to ARM and AARCH64.
 
   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
-  Copyright (c) 2021, Ampere Computing LLC. All rights reserved.<BR>
+  Copyright (c) 2021 - 2022, Ampere Computing LLC. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -131,7 +131,7 @@ SmbiosGetSmcArm64SocId (
   SmcCallStatus = ArmCallSmc1 (SMCCC_ARCH_SOC_ID, &SmcParam, NULL, NULL);
 
   if (SmcCallStatus >= 0) {
-    *Jep106Code = (INT32)SmcParam;
+    *Jep106Code = SmcCallStatus;
   } else {
     Status = EFI_UNSUPPORTED;
   }
@@ -140,7 +140,7 @@ SmbiosGetSmcArm64SocId (
   SmcCallStatus = ArmCallSmc1 (SMCCC_ARCH_SOC_ID, &SmcParam, NULL, NULL);
 
   if (SmcCallStatus >= 0) {
-    *SocRevision = (INT32)SmcParam;
+    *SocRevision = SmcCallStatus;
   } else {
     Status = EFI_UNSUPPORTED;
   }
-- 
2.25.1

[PATCH v3 3/6] ArmPkg/SmbiosMiscDxe: Support fetching System UUID

[Nhi Pham]

This adds an API to OemMiscLib for fetching the system UUID according to
the platform.

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf                     |  4 ++++
 ArmPkg/Include/Library/OemMiscLib.h                                           | 12 ++++++++++++
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                           | 18 ++++++++++++++++++
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c |  3 ++-
 4 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
index 5c4268f68b4a..8653f57720d1 100644
--- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
+++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLibNull.inf
@@ -1,6 +1,7 @@
 #/** @file
 #    OemMiscLib.inf
 #
+#    Copyright (c) 2022, Ampere Computing LLC. All rights reserved.
 #    Copyright (c) 2021, NUVIA Inc. All rights reserved.
 #    Copyright (c) 2018, Hisilicon Limited. All rights reserved.
 #    Copyright (c) 2018, Linaro Limited. All rights reserved.
@@ -29,3 +30,6 @@ [Packages]
 [LibraryClasses]
   BaseMemoryLib
   DebugLib
+
+[Guids]
+  gZeroGuid
diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index 330bb4b014de..d87737db9c2b 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -235,4 +235,16 @@ OemGetChassisNumPowerCords (
   VOID
   );
 
+/**
+  Fetches the system UUID.
+
+  @param[out] SystemUuid     The pointer to the buffer to store the System UUID.
+
+**/
+VOID
+EFIAPI
+OemGetSystemUuid (
+  OUT GUID  *SystemUuid
+  );
+
 #endif // OEM_MISC_LIB_H_
diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
index 98970407a65e..32f6d55c1a9a 100644
--- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
+++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
@@ -1,6 +1,7 @@
 /** @file
 *  OemMiscLib.c
 *
+*  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.
 *  Copyright (c) 2021, NUVIA Inc. All rights reserved.
 *  Copyright (c) 2018, Hisilicon Limited. All rights reserved.
 *  Copyright (c) 2018, Linaro Limited. All rights reserved.
@@ -10,6 +11,7 @@
 **/
 
 #include <Uefi.h>
+#include <Guid/ZeroGuid.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/HiiLib.h>
@@ -236,3 +238,19 @@ OemGetChassisNumPowerCords (
   ASSERT (FALSE);
   return 1;
 }
+
+/**
+  Fetches the system UUID.
+
+  @param[out] SystemUuid     The pointer to the buffer to store the System UUID.
+
+**/
+VOID
+EFIAPI
+OemGetSystemUuid (
+  OUT GUID  *SystemUuid
+  )
+{
+  ASSERT (FALSE);
+  CopyGuid (SystemUuid, &gZeroGuid);
+}
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c
index 5cf72644d0b2..22fb5eccaa63 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type01/MiscSystemManufacturerFunction.c
@@ -4,6 +4,7 @@
 
   Based on files under Nt32Pkg/MiscSubClassPlatformDxe/
 
+  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
   Copyright (c) 2006 - 2011, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
@@ -160,7 +161,7 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscSystemManufacturer) {
 
   SmbiosRecord->Hdr.Length = sizeof (SMBIOS_TABLE_TYPE1);
 
-  CopyGuid (&SmbiosRecord->Uuid, &InputData->Uuid);
+  OemGetSystemUuid (&SmbiosRecord->Uuid);
 
   OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
   UnicodeStrToAsciiStrS (Manufacturer, OptionalStrStart, ManuStrLen + 1);
-- 
2.25.1

[PATCH v3 4/6] ArmPkg/SmbiosMiscDxe: Fix typo of "AssetTagType02"

[Nhi Pham]

From: Minh Nguyen <minhn@amperecomputing.com>

This patch fixes typo from "AssertTagType02"
to "AssetTagType02".

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/OemMiscLib.h                                              | 2 +-
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index d87737db9c2b..1936619d9b5b 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -44,7 +44,7 @@ typedef enum {
   VersionType01,
   SkuNumberType01,
   FamilyType01,
-  AssertTagType02,
+  AssetTagType02,
   SerialNumberType02,
   BoardManufacturerType02,
   ProductNameType02,
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
index 870610b17243..f61546955f12 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
@@ -4,6 +4,7 @@
 
   Based on files under Nt32Pkg/MiscSubClassPlatformDxe/
 
+  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
   Copyright (c) 2009 - 2011, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
@@ -112,7 +113,7 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBaseBoardManufacturer) {
   OemUpdateSmbiosInfo (
     mSmbiosMiscHiiHandle,
     STRING_TOKEN (STR_MISC_BASE_BOARD_ASSET_TAG),
-    AssertTagType02
+    AssetTagType02
     );
   OemUpdateSmbiosInfo (
     mSmbiosMiscHiiHandle,
-- 
2.25.1

[PATCH v3 5/6] ArmPkg/SmbiosMiscDxe: Remove redundant updates in SMBIOS Type 2

[Nhi Pham]

From: Minh Nguyen <minhn@amperecomputing.com>

This patch removes redundant updates of "BoardManufacturerType02"
and "SerialNumberType02".

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
index f61546955f12..3441e7798860 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type02/MiscBaseBoardManufacturerFunction.c
@@ -120,16 +120,6 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBaseBoardManufacturer) {
     STRING_TOKEN (STR_MISC_BASE_BOARD_SERIAL_NUMBER),
     SerialNumberType02
     );
-  OemUpdateSmbiosInfo (
-    mSmbiosMiscHiiHandle,
-    STRING_TOKEN (STR_MISC_BASE_BOARD_MANUFACTURER),
-    BoardManufacturerType02
-    );
-  OemUpdateSmbiosInfo (
-    mSmbiosMiscHiiHandle,
-    STRING_TOKEN (STR_MISC_BASE_BOARD_SERIAL_NUMBER),
-    SerialNumberType02
-    );
   OemUpdateSmbiosInfo (
     mSmbiosMiscHiiHandle,
     STRING_TOKEN (STR_MISC_BASE_BOARD_SKU_NUMBER),
-- 
2.25.1

[PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

[Nhi Pham]

From: Minh Nguyen <minhn@amperecomputing.com>

In some scenarios, the information of Bios Version, Bios Release
and Embedded Controller Firmware Release are fetched during UEFI
booting. This patch supports updating those fields dynamically
when the PCDs are empty.

Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Acked-by: Ard Biesheuvel <ardb@kernel.org>
---
 ArmPkg/Include/Library/OemMiscLib.h                                   | 21 +++++++++++++
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                   | 28 +++++++++++++++++
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 32 +++++++++++++-------
 3 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index 1936619d9b5b..541274999e5c 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -37,6 +37,7 @@ typedef struct {
 } OEM_MISC_PROCESSOR_DATA;
 
 typedef enum {
+  BiosVersionType00,
   ProductNameType01,
   SerialNumType01,
   UuidType01,
@@ -247,4 +248,24 @@ OemGetSystemUuid (
   OUT GUID  *SystemUuid
   );
 
+/** Fetches the BIOS release.
+
+  @return The BIOS release.
+**/
+UINT16
+EFIAPI
+OemGetBiosRelease (
+  VOID
+  );
+
+/** Fetches the embedded controller firmware release.
+
+  @return The embedded controller firmware release.
+**/
+UINT16
+EFIAPI
+OemGetEmbeddedControllerFirmwareRelease (
+  VOID
+  );
+
 #endif // OEM_MISC_LIB_H_
diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
index 32f6d55c1a9a..788ccab9e8c1 100644
--- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
+++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
@@ -254,3 +254,31 @@ OemGetSystemUuid (
   ASSERT (FALSE);
   CopyGuid (SystemUuid, &gZeroGuid);
 }
+
+/** Fetches the BIOS release.
+
+  @return The BIOS release.
+**/
+UINT16
+EFIAPI
+OemGetBiosRelease (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return 0xFFFF;
+}
+
+/** Fetches the embedded controller firmware release.
+
+  @return The embedded controller firmware release.
+**/
+UINT16
+EFIAPI
+OemGetEmbeddedControllerFirmwareRelease (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return 0xFFFF;
+}
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
index b49c4b754cab..e9106a8a2fec 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
@@ -1,5 +1,6 @@
 /** @file
 
+  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
   Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
   Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
@@ -13,6 +14,7 @@
 #include <Library/DebugLib.h>
 #include <Library/HiiLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/OemMiscLib.h>
 #include <Library/PrintLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
@@ -191,11 +193,11 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
     TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
     HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
   } else {
-    Version = (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString);
-    if (StrLen (Version) > 0) {
-      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
-      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
-    }
+      OemUpdateSmbiosInfo (
+        mSmbiosMiscHiiHandle,
+        STRING_TOKEN (STR_MISC_BIOS_VERSION),
+        BiosVersionType00
+        );
   }
 
   Char16String = GetBiosReleaseDate ();
@@ -251,13 +253,21 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
     }
   }
 
-  SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
-  SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
+  if (PcdGet16 (PcdSystemBiosRelease) != 0xFFFF) {
+    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
+    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
+  } else {
+    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(OemGetBiosRelease () >> 8);
+    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(OemGetBiosRelease () & 0xFF);
+  }
 
-  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
-                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
-  SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
-                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
+  if (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) != 0xFFFF) {
+    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
+    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
+  } else {
+    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () >> 8);
+    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () & 0xFF);
+  }
 
   OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
   UnicodeStrToAsciiStrS (Vendor, OptionalStrStart, VendorStrLen + 1);
-- 
2.25.1

Re: [PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

[Leif Lindholm]

On Tue, Sep 13, 2022 at 13:19:47 +0700, Nhi Pham wrote:
> From: Minh Nguyen <minhn@amperecomputing.com>
> 
> In some scenarios, the information of Bios Version, Bios Release
> and Embedded Controller Firmware Release are fetched during UEFI
> booting. This patch supports updating those fields dynamically
> when the PCDs are empty.
> 
> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
> Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> Acked-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  ArmPkg/Include/Library/OemMiscLib.h                                   | 21 +++++++++++++
>  ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                   | 28 +++++++++++++++++
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 32 +++++++++++++-------
>  3 files changed, 70 insertions(+), 11 deletions(-)
> 
> diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
> index 1936619d9b5b..541274999e5c 100644
> --- a/ArmPkg/Include/Library/OemMiscLib.h
> +++ b/ArmPkg/Include/Library/OemMiscLib.h
> @@ -37,6 +37,7 @@ typedef struct {
>  } OEM_MISC_PROCESSOR_DATA;
>  
>  typedef enum {
> +  BiosVersionType00,
>    ProductNameType01,
>    SerialNumType01,
>    UuidType01,
> @@ -247,4 +248,24 @@ OemGetSystemUuid (
>    OUT GUID  *SystemUuid
>    );
>  
> +/** Fetches the BIOS release.
> +
> +  @return The BIOS release.
> +**/
> +UINT16
> +EFIAPI
> +OemGetBiosRelease (
> +  VOID
> +  );
> +
> +/** Fetches the embedded controller firmware release.
> +
> +  @return The embedded controller firmware release.
> +**/
> +UINT16
> +EFIAPI
> +OemGetEmbeddedControllerFirmwareRelease (
> +  VOID
> +  );
> +
>  #endif // OEM_MISC_LIB_H_
> diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
> index 32f6d55c1a9a..788ccab9e8c1 100644
> --- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
> +++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
> @@ -254,3 +254,31 @@ OemGetSystemUuid (
>    ASSERT (FALSE);
>    CopyGuid (SystemUuid, &gZeroGuid);
>  }
> +
> +/** Fetches the BIOS release.
> +
> +  @return The BIOS release.
> +**/
> +UINT16
> +EFIAPI
> +OemGetBiosRelease (
> +  VOID
> +  )
> +{
> +  ASSERT (FALSE);
> +  return 0xFFFF;

This is a change in behaviour.
The pre-existing behaviour would be preserved by returning the value
of PcdGet16 (PcdSystemBiosRelease), which defaults to 0xFFFF.

> +}
> +
> +/** Fetches the embedded controller firmware release.
> +
> +  @return The embedded controller firmware release.
> +**/
> +UINT16
> +EFIAPI
> +OemGetEmbeddedControllerFirmwareRelease (
> +  VOID
> +  )
> +{
> +  ASSERT (FALSE);
> +  return 0xFFFF;

Same as above, but PcdEmbeddedControllerFirmwareRelease.

No other comments on this set.
(Feel free to see that as Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>
for 1-5/6, but you already have the tags you need for those.)

/
    Leif

> +}
> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> index b49c4b754cab..e9106a8a2fec 100644
> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> @@ -1,5 +1,6 @@
>  /** @file
>  
> +  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
>    Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>    Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
>    Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
> @@ -13,6 +14,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/HiiLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/OemMiscLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
> @@ -191,11 +193,11 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>    } else {
> -    Version = (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString);
> -    if (StrLen (Version) > 0) {
> -      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
> -      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
> -    }
> +      OemUpdateSmbiosInfo (
> +        mSmbiosMiscHiiHandle,
> +        STRING_TOKEN (STR_MISC_BIOS_VERSION),
> +        BiosVersionType00
> +        );
>    }
>  
>    Char16String = GetBiosReleaseDate ();
> @@ -251,13 +253,21 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>      }
>    }
>  
> -  SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
> -  SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
> +  if (PcdGet16 (PcdSystemBiosRelease) != 0xFFFF) {
> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
> +  } else {
> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(OemGetBiosRelease () >> 8);
> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(OemGetBiosRelease () & 0xFF);
> +  }
>  
> -  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
> -  SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
> +  if (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) != 0xFFFF) {
> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
> +  } else {
> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () >> 8);
> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () & 0xFF);
> +  }
>  
>    OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
>    UnicodeStrToAsciiStrS (Vendor, OptionalStrStart, VendorStrLen + 1);
> -- 
> 2.25.1
> 

Re: [PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

[Nhi Pham]

Thanks Leif. I will fix as your suggestion.

-Nhi

On 9/15/2022 5:54 PM, Leif Lindholm wrote:
> On Tue, Sep 13, 2022 at 13:19:47 +0700, Nhi Pham wrote:
>> From: Minh Nguyen <minhn@amperecomputing.com>
>>
>> In some scenarios, the information of Bios Version, Bios Release
>> and Embedded Controller Firmware Release are fetched during UEFI
>> booting. This patch supports updating those fields dynamically
>> when the PCDs are empty.
>>
>> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
>> Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
>> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   ArmPkg/Include/Library/OemMiscLib.h                                   | 21 +++++++++++++
>>   ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                   | 28 +++++++++++++++++
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 32 +++++++++++++-------
>>   3 files changed, 70 insertions(+), 11 deletions(-)
>>
>> diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
>> index 1936619d9b5b..541274999e5c 100644
>> --- a/ArmPkg/Include/Library/OemMiscLib.h
>> +++ b/ArmPkg/Include/Library/OemMiscLib.h
>> @@ -37,6 +37,7 @@ typedef struct {
>>   } OEM_MISC_PROCESSOR_DATA;
>>   
>>   typedef enum {
>> +  BiosVersionType00,
>>     ProductNameType01,
>>     SerialNumType01,
>>     UuidType01,
>> @@ -247,4 +248,24 @@ OemGetSystemUuid (
>>     OUT GUID  *SystemUuid
>>     );
>>   
>> +/** Fetches the BIOS release.
>> +
>> +  @return The BIOS release.
>> +**/
>> +UINT16
>> +EFIAPI
>> +OemGetBiosRelease (
>> +  VOID
>> +  );
>> +
>> +/** Fetches the embedded controller firmware release.
>> +
>> +  @return The embedded controller firmware release.
>> +**/
>> +UINT16
>> +EFIAPI
>> +OemGetEmbeddedControllerFirmwareRelease (
>> +  VOID
>> +  );
>> +
>>   #endif // OEM_MISC_LIB_H_
>> diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>> index 32f6d55c1a9a..788ccab9e8c1 100644
>> --- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>> +++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>> @@ -254,3 +254,31 @@ OemGetSystemUuid (
>>     ASSERT (FALSE);
>>     CopyGuid (SystemUuid, &gZeroGuid);
>>   }
>> +
>> +/** Fetches the BIOS release.
>> +
>> +  @return The BIOS release.
>> +**/
>> +UINT16
>> +EFIAPI
>> +OemGetBiosRelease (
>> +  VOID
>> +  )
>> +{
>> +  ASSERT (FALSE);
>> +  return 0xFFFF;
> This is a change in behaviour.
> The pre-existing behaviour would be preserved by returning the value
> of PcdGet16 (PcdSystemBiosRelease), which defaults to 0xFFFF.
>
>> +}
>> +
>> +/** Fetches the embedded controller firmware release.
>> +
>> +  @return The embedded controller firmware release.
>> +**/
>> +UINT16
>> +EFIAPI
>> +OemGetEmbeddedControllerFirmwareRelease (
>> +  VOID
>> +  )
>> +{
>> +  ASSERT (FALSE);
>> +  return 0xFFFF;
> Same as above, but PcdEmbeddedControllerFirmwareRelease.
>
> No other comments on this set.
> (Feel free to see that as Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>
> for 1-5/6, but you already have the tags you need for those.)
>
> /
>      Leif
>
>> +}
>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> index b49c4b754cab..e9106a8a2fec 100644
>> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> @@ -1,5 +1,6 @@
>>   /** @file
>>   
>> +  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
>>     Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>>     Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
>>     Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
>> @@ -13,6 +14,7 @@
>>   #include <Library/DebugLib.h>
>>   #include <Library/HiiLib.h>
>>   #include <Library/MemoryAllocationLib.h>
>> +#include <Library/OemMiscLib.h>
>>   #include <Library/PrintLib.h>
>>   #include <Library/UefiBootServicesTableLib.h>
>>   
>> @@ -191,11 +193,11 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>>       TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>>       HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>>     } else {
>> -    Version = (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString);
>> -    if (StrLen (Version) > 0) {
>> -      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>> -      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>> -    }
>> +      OemUpdateSmbiosInfo (
>> +        mSmbiosMiscHiiHandle,
>> +        STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> +        BiosVersionType00
>> +        );
>>     }
>>   
>>     Char16String = GetBiosReleaseDate ();
>> @@ -251,13 +253,21 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>>       }
>>     }
>>   
>> -  SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
>> -  SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
>> +  if (PcdGet16 (PcdSystemBiosRelease) != 0xFFFF) {
>> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
>> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
>> +  } else {
>> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(OemGetBiosRelease () >> 8);
>> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(OemGetBiosRelease () & 0xFF);
>> +  }
>>   
>> -  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
>> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
>> -  SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
>> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
>> +  if (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) != 0xFFFF) {
>> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
>> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
>> +  } else {
>> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () >> 8);
>> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () & 0xFF);
>> +  }
>>   
>>     OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
>>     UnicodeStrToAsciiStrS (Vendor, OptionalStrStart, VendorStrLen + 1);
>> -- 
>> 2.25.1
>>

Re: [PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

[Sami Mujawar]

Hi Nhi,

Please see my response inline marked [SAMI].

Regards,

Sami Mujawar

On 15/09/2022, 19:23, "Nhi Pham" <nhi@amperemail.onmicrosoft.com> wrote:

    Thanks Leif. I will fix as your suggestion.

    -Nhi

    On 9/15/2022 5:54 PM, Leif Lindholm wrote:
    > On Tue, Sep 13, 2022 at 13:19:47 +0700, Nhi Pham wrote:
    >> From: Minh Nguyen <minhn@amperecomputing.com>
    >>
    >> In some scenarios, the information of Bios Version, Bios Release
    >> and Embedded Controller Firmware Release are fetched during UEFI
    >> booting. This patch supports updating those fields dynamically
    >> when the PCDs are empty.
    >>
    >> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
    >> Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
    >> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
    >> Acked-by: Ard Biesheuvel <ardb@kernel.org>
    >> ---
    >>   ArmPkg/Include/Library/OemMiscLib.h                                   | 21 +++++++++++++
    >>   ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                   | 28 +++++++++++++++++
    >>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 32 +++++++++++++-------
    >>   3 files changed, 70 insertions(+), 11 deletions(-)
    >>
    >> diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
    >> index 1936619d9b5b..541274999e5c 100644
    >> --- a/ArmPkg/Include/Library/OemMiscLib.h
    >> +++ b/ArmPkg/Include/Library/OemMiscLib.h
    >> @@ -37,6 +37,7 @@ typedef struct {
    >>   } OEM_MISC_PROCESSOR_DATA;
    >>   
    >>   typedef enum {
    >> +  BiosVersionType00,
    >>     ProductNameType01,
    >>     SerialNumType01,
    >>     UuidType01,
    >> @@ -247,4 +248,24 @@ OemGetSystemUuid (
    >>     OUT GUID  *SystemUuid
    >>     );
    >>   
    >> +/** Fetches the BIOS release.
    >> +
    >> +  @return The BIOS release.
    >> +**/
    >> +UINT16
    >> +EFIAPI
    >> +OemGetBiosRelease (
    >> +  VOID
    >> +  );
    >> +
    >> +/** Fetches the embedded controller firmware release.
    >> +
    >> +  @return The embedded controller firmware release.
    >> +**/
    >> +UINT16
    >> +EFIAPI
    >> +OemGetEmbeddedControllerFirmwareRelease (
    >> +  VOID
    >> +  );
    >> +
    >>   #endif // OEM_MISC_LIB_H_
    >> diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
    >> index 32f6d55c1a9a..788ccab9e8c1 100644
    >> --- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
    >> +++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
    >> @@ -254,3 +254,31 @@ OemGetSystemUuid (
    >>     ASSERT (FALSE);
    >>     CopyGuid (SystemUuid, &gZeroGuid);
    >>   }
    >> +
    >> +/** Fetches the BIOS release.
    >> +
    >> +  @return The BIOS release.
    >> +**/
    >> +UINT16
    >> +EFIAPI
    >> +OemGetBiosRelease (
    >> +  VOID
    >> +  )
    >> +{
    >> +  ASSERT (FALSE);
    >> +  return 0xFFFF;
    > This is a change in behaviour.
    > The pre-existing behaviour would be preserved by returning the value
    > of PcdGet16 (PcdSystemBiosRelease), which defaults to 0xFFFF.
    >
    >> +}
    >> +
    >> +/** Fetches the embedded controller firmware release.
    >> +
    >> +  @return The embedded controller firmware release.
    >> +**/
    >> +UINT16
    >> +EFIAPI
    >> +OemGetEmbeddedControllerFirmwareRelease (
    >> +  VOID
    >> +  )
    >> +{
    >> +  ASSERT (FALSE);
    >> +  return 0xFFFF;
    > Same as above, but PcdEmbeddedControllerFirmwareRelease.
    >
    > No other comments on this set.
    > (Feel free to see that as Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>
    > for 1-5/6, but you already have the tags you need for those.)
    >
    > /
    >      Leif
    >
    >> +}
    >> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
    >> index b49c4b754cab..e9106a8a2fec 100644
    >> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
    >> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
    >> @@ -1,5 +1,6 @@
    >>   /** @file
    >>   
    >> +  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
    >>     Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
    >>     Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
    >>     Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
    >> @@ -13,6 +14,7 @@
    >>   #include <Library/DebugLib.h>
    >>   #include <Library/HiiLib.h>
    >>   #include <Library/MemoryAllocationLib.h>
    >> +#include <Library/OemMiscLib.h>
    >>   #include <Library/PrintLib.h>
    >>   #include <Library/UefiBootServicesTableLib.h>
    >>   
    >> @@ -191,11 +193,11 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
    >>       TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
    >>       HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
    >>     } else {
    >> -    Version = (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString);
    >> -    if (StrLen (Version) > 0) {
    >> -      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
    >> -      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
    >> -    }
    >> +      OemUpdateSmbiosInfo (
    >> +        mSmbiosMiscHiiHandle,
    >> +        STRING_TOKEN (STR_MISC_BIOS_VERSION),
    >> +        BiosVersionType00
    >> +        );
    >>     }
    >>   
    >>     Char16String = GetBiosReleaseDate ();
    >> @@ -251,13 +253,21 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
    >>       }
    >>     }
    >>   
    >> -  SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
    >> -  SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
    >> +  if (PcdGet16 (PcdSystemBiosRelease) != 0xFFFF) {
    >> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
    >> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);

[SAMI] Considering that ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c will be updated to use PcdSystemBiosRelease, 
can you check whether the 'if' code block above is required, please?
[/SAMI]

    >> +  } else {
    >> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(OemGetBiosRelease () >> 8);
    >> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(OemGetBiosRelease () & 0xFF);
    >> +  }
    >>   
    >> -  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
    >> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
    >> -  SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
    >> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
    >> +  if (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) != 0xFFFF) {
    >> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
    >> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);

[SAMI] Similar comment as the previous one.

    >> +  } else {
    >> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () >> 8);
    >> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () & 0xFF);
    >> +  }
    >>   
    >>     OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
    >>     UnicodeStrToAsciiStrS (Vendor, OptionalStrStart, VendorStrLen + 1);
    >> -- 
    >> 2.25.1
    >>

Re: [PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib

[Nhi Pham]

Hi Sami,

On 9/16/2022 2:32 PM, Sami Mujawar wrote:
> Hi Nhi,
>
> Please see my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 15/09/2022, 19:23, "Nhi Pham" <nhi@amperemail.onmicrosoft.com> wrote:
>
>      Thanks Leif. I will fix as your suggestion.
>
>      -Nhi
>
>      On 9/15/2022 5:54 PM, Leif Lindholm wrote:
>      > On Tue, Sep 13, 2022 at 13:19:47 +0700, Nhi Pham wrote:
>      >> From: Minh Nguyen <minhn@amperecomputing.com>
>      >>
>      >> In some scenarios, the information of Bios Version, Bios Release
>      >> and Embedded Controller Firmware Release are fetched during UEFI
>      >> booting. This patch supports updating those fields dynamically
>      >> when the PCDs are empty.
>      >>
>      >> Signed-off-by: Nhi Pham <nhi@os.amperecomputing.com>
>      >> Reviewed-by: Rebecca Cran <rebecca@quicinc.com>
>      >> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>      >> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>      >> ---
>      >>   ArmPkg/Include/Library/OemMiscLib.h                                   | 21 +++++++++++++
>      >>   ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                   | 28 +++++++++++++++++
>      >>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 32 +++++++++++++-------
>      >>   3 files changed, 70 insertions(+), 11 deletions(-)
>      >>
>      >> diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
>      >> index 1936619d9b5b..541274999e5c 100644
>      >> --- a/ArmPkg/Include/Library/OemMiscLib.h
>      >> +++ b/ArmPkg/Include/Library/OemMiscLib.h
>      >> @@ -37,6 +37,7 @@ typedef struct {
>      >>   } OEM_MISC_PROCESSOR_DATA;
>      >>
>      >>   typedef enum {
>      >> +  BiosVersionType00,
>      >>     ProductNameType01,
>      >>     SerialNumType01,
>      >>     UuidType01,
>      >> @@ -247,4 +248,24 @@ OemGetSystemUuid (
>      >>     OUT GUID  *SystemUuid
>      >>     );
>      >>
>      >> +/** Fetches the BIOS release.
>      >> +
>      >> +  @return The BIOS release.
>      >> +**/
>      >> +UINT16
>      >> +EFIAPI
>      >> +OemGetBiosRelease (
>      >> +  VOID
>      >> +  );
>      >> +
>      >> +/** Fetches the embedded controller firmware release.
>      >> +
>      >> +  @return The embedded controller firmware release.
>      >> +**/
>      >> +UINT16
>      >> +EFIAPI
>      >> +OemGetEmbeddedControllerFirmwareRelease (
>      >> +  VOID
>      >> +  );
>      >> +
>      >>   #endif // OEM_MISC_LIB_H_
>      >> diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>      >> index 32f6d55c1a9a..788ccab9e8c1 100644
>      >> --- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>      >> +++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
>      >> @@ -254,3 +254,31 @@ OemGetSystemUuid (
>      >>     ASSERT (FALSE);
>      >>     CopyGuid (SystemUuid, &gZeroGuid);
>      >>   }
>      >> +
>      >> +/** Fetches the BIOS release.
>      >> +
>      >> +  @return The BIOS release.
>      >> +**/
>      >> +UINT16
>      >> +EFIAPI
>      >> +OemGetBiosRelease (
>      >> +  VOID
>      >> +  )
>      >> +{
>      >> +  ASSERT (FALSE);
>      >> +  return 0xFFFF;
>      > This is a change in behaviour.
>      > The pre-existing behaviour would be preserved by returning the value
>      > of PcdGet16 (PcdSystemBiosRelease), which defaults to 0xFFFF.
>      >
>      >> +}
>      >> +
>      >> +/** Fetches the embedded controller firmware release.
>      >> +
>      >> +  @return The embedded controller firmware release.
>      >> +**/
>      >> +UINT16
>      >> +EFIAPI
>      >> +OemGetEmbeddedControllerFirmwareRelease (
>      >> +  VOID
>      >> +  )
>      >> +{
>      >> +  ASSERT (FALSE);
>      >> +  return 0xFFFF;
>      > Same as above, but PcdEmbeddedControllerFirmwareRelease.
>      >
>      > No other comments on this set.
>      > (Feel free to see that as Acked-by: Leif Lindholm <quic_llindhol@quicinc.com>
>      > for 1-5/6, but you already have the tags you need for those.)
>      >
>      > /
>      >      Leif
>      >
>      >> +}
>      >> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>      >> index b49c4b754cab..e9106a8a2fec 100644
>      >> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>      >> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>      >> @@ -1,5 +1,6 @@
>      >>   /** @file
>      >>
>      >> +  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
>      >>     Copyright (c) 2021, NUVIA Inc. All rights reserved.<BR>
>      >>     Copyright (c) 2009, Intel Corporation. All rights reserved.<BR>
>      >>     Copyright (c) 2015, Hisilicon Limited. All rights reserved.<BR>
>      >> @@ -13,6 +14,7 @@
>      >>   #include <Library/DebugLib.h>
>      >>   #include <Library/HiiLib.h>
>      >>   #include <Library/MemoryAllocationLib.h>
>      >> +#include <Library/OemMiscLib.h>
>      >>   #include <Library/PrintLib.h>
>      >>   #include <Library/UefiBootServicesTableLib.h>
>      >>
>      >> @@ -191,11 +193,11 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>      >>       TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>      >>       HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>      >>     } else {
>      >> -    Version = (CHAR16 *)PcdGetPtr (PcdFirmwareVersionString);
>      >> -    if (StrLen (Version) > 0) {
>      >> -      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>      >> -      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>      >> -    }
>      >> +      OemUpdateSmbiosInfo (
>      >> +        mSmbiosMiscHiiHandle,
>      >> +        STRING_TOKEN (STR_MISC_BIOS_VERSION),
>      >> +        BiosVersionType00
>      >> +        );
>      >>     }
>      >>
>      >>     Char16String = GetBiosReleaseDate ();
>      >> @@ -251,13 +253,21 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>      >>       }
>      >>     }
>      >>
>      >> -  SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
>      >> -  SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
>      >> +  if (PcdGet16 (PcdSystemBiosRelease) != 0xFFFF) {
>      >> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) >> 8);
>      >> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(PcdGet16 (PcdSystemBiosRelease) & 0xFF);
>
> [SAMI] Considering that ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c will be updated to use PcdSystemBiosRelease,
> can you check whether the 'if' code block above is required, please?
> [/SAMI]

That's really a great point. I think the if statement above is not 
needed as the OemMiscLib can do what they want.

I will update the patch for this.

Thanks,

Nhi

>
>      >> +  } else {
>      >> +    SmbiosRecord->SystemBiosMajorRelease = (UINT8)(OemGetBiosRelease () >> 8);
>      >> +    SmbiosRecord->SystemBiosMinorRelease = (UINT8)(OemGetBiosRelease () & 0xFF);
>      >> +  }
>      >>
>      >> -  SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)
>      >> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
>      >> -  SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)
>      >> -                                                         (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
>      >> +  if (PcdGet16 (PcdEmbeddedControllerFirmwareRelease) != 0xFFFF) {
>      >> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) >> 8);
>      >> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(PcdGet16 (PcdEmbeddedControllerFirmwareRelease) & 0xFF);
>
> [SAMI] Similar comment as the previous one.
>
>      >> +  } else {
>      >> +    SmbiosRecord->EmbeddedControllerFirmwareMajorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () >> 8);
>      >> +    SmbiosRecord->EmbeddedControllerFirmwareMinorRelease = (UINT16)(OemGetEmbeddedControllerFirmwareRelease () & 0xFF);
>      >> +  }
>      >>
>      >>     OptionalStrStart = (CHAR8 *)(SmbiosRecord + 1);
>      >>     UnicodeStrToAsciiStrS (Vendor, OptionalStrStart, VendorStrLen + 1);
>      >> --
>      >> 2.25.1
>      >>
>