[PATCH 0/5] ArmPkg: OemMiscLib and Universal/Smbios improvements and fixes

[PATCH 0/5] ArmPkg: OemMiscLib and Universal/Smbios improvements and fixes

[Rebecca Cran]

Add the set of improvements requested during the initial series to move
closer to a full/proper implementation, fix the
chassis SKU number string, and fix a typo in a comment.

This is a breaking change wrt SbsaQemu in edk2-platforms.

Rebecca Cran (5):
  ArmPkg: Allow platforms to override PCI supported state in
    SmbiosMiscDxe
  ArmPkg: Allow platforms to supply more data for SMBIOS Type3 record
  ArmPkg: Allow platforms to report their boot status via OemMiscLib
    call
  ArmPkg: Fix calculation of offset of chassis SKU Number in
    SmbiosMiscDxe
  ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe

 ArmPkg/ArmPkg.dec                                                              |  1 +
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf                        |  1 +
 ArmPkg/Include/Library/OemMiscLib.h                                            | 70 ++++++++++++++
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                            | 99 ++++++++++++++++++++
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c          |  4 +
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c     |  2 +-
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 20 +++-
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c     |  3 +
 8 files changed, 194 insertions(+), 6 deletions(-)

-- 
2.26.2

[PATCH 1/5] ArmPkg: Allow platforms to override PCI supported state in SmbiosMiscDxe

[Rebecca Cran]

Not all platforms support PCI, so introduce a PCD to allow platforms to
specify whether they support it.

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 ArmPkg/ArmPkg.dec                                                     | 1 +
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf               | 1 +
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 4 ++++
 3 files changed, 6 insertions(+)

diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index a8a22c649ff8..51ac2191c85a 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -125,6 +125,7 @@ [PcdsFixedAtBuild.common]
   #
   # SMBIOS PCDs
   #
+  gArmTokenSpaceGuid.PcdPlatformSupportsPCI|TRUE|BOOLEAN|0x30000052
   gArmTokenSpaceGuid.PcdSystemProductName|L""|VOID*|0x30000053
   gArmTokenSpaceGuid.PcdSystemVersion|L""|VOID*|0x30000054
   gArmTokenSpaceGuid.PcdBaseBoardManufacturer|L""|VOID*|0x30000055
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
index 60d8fe31c219..ebc4c99ac436 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
@@ -71,6 +71,7 @@ [Pcd]
   gArmTokenSpaceGuid.PcdFdSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
   gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
+  gArmTokenSpaceGuid.PcdPlatformSupportsPCI
   gArmTokenSpaceGuid.PcdSystemBiosRelease
   gArmTokenSpaceGuid.PcdEmbeddedControllerFirmwareRelease
   gArmTokenSpaceGuid.PcdSystemProductName
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
index 5aea32521bd3..a06f814aeb7c 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
@@ -13,6 +13,7 @@
 #include <Library/DebugLib.h>
 #include <Library/HiiLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/PcdLib.h>
 #include <Library/PrintLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
@@ -264,6 +265,9 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor)
   UnicodeStrToAsciiStrS (Version, StrStart, VerStrLen + 1);
   StrStart += VerStrLen + 1;
   UnicodeStrToAsciiStrS (ReleaseDate, StrStart, DateStrLen + 1);
+
+  SmbiosRecord->BiosCharacteristics.PciIsSupported = FixedPcdGetBool (PcdPlatformSupportsPCI);
+
   //
   // Now we have got the full smbios record, call smbios protocol to add this record.
   //
-- 
2.26.2

[PATCH 2/5] ArmPkg: Allow platforms to supply more data for SMBIOS Type3 record

[Rebecca Cran]

Add OemMiscLib calls to allow platforms to provide the following
information about the chassis:

o Bootup state
o Power supply/supplies state
o Thermal state
o Security state
o Chassis height (in RMU)
o Number of power cords

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 ArmPkg/Include/Library/OemMiscLib.h                                            | 60 ++++++++++++++
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                            | 85 ++++++++++++++++++++
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c |  8 ++
 3 files changed, 153 insertions(+)

diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index 6dcc76a214df..25ae508ddb03 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -162,4 +162,64 @@ OemUpdateSmbiosInfo (
   IN OEM_MISC_SMBIOS_HII_STRING_FIELD Field
   );
 
+/** Fetches the chassis status when it was last booted.
+
+ @return Chassis status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisBootupState (
+  VOID
+  );
+
+/** Fetches the chassis power supply/supplies status when last booted.
+
+ @return Chassis power supply/supplies status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisPowerSupplyState (
+  VOID
+  );
+
+/** Fetches the chassis thermal status when last booted.
+
+ @return Chassis thermal status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisThermalState (
+  VOID
+  );
+
+/** Fetches the chassis security status when last booted.
+
+ @return Chassis security status.
+**/
+MISC_CHASSIS_SECURITY_STATE
+EFIAPI
+OemGetChassisSecurityStatus (
+  VOID
+  );
+
+/** Fetches the chassis height in RMUs (Rack Mount Units).
+
+  @return The height of the chassis.
+**/
+UINT8
+EFIAPI
+OemGetChassisHeight (
+  VOID
+  );
+
+/** Fetches the number of power cords.
+
+  @return The number of power cords.
+**/
+UINT8
+EFIAPI
+OemGetChassisNumPowerCords (
+  VOID
+  );
+
 #endif // OEM_MISC_LIB_H_
diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
index 21f106f1e060..e6a4793fe1c2 100644
--- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
+++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
@@ -139,3 +139,88 @@ OemUpdateSmbiosInfo (
 {
   ASSERT (FALSE);
 }
+
+/** Fetches the chassis status when it was last booted.
+
+ @return Chassis status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisBootupState (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis power supply/supplies status when last booted.
+
+ @return Chassis power supply/supplies status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisPowerSupplyState (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis thermal status when last booted.
+
+ @return Chassis thermal status.
+**/
+MISC_CHASSIS_STATE
+EFIAPI
+OemGetChassisThermalState (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return ChassisStateSafe;
+}
+
+/** Fetches the chassis security status when last booted.
+
+ @return Chassis security status.
+**/
+MISC_CHASSIS_SECURITY_STATE
+EFIAPI
+OemGetChassisSecurityStatus (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return ChassisSecurityStatusNone;
+}
+
+/** Fetches the chassis height in RMUs (Rack Mount Units).
+
+  @return The height of the chassis.
+**/
+UINT8
+EFIAPI
+OemGetChassisHeight (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return 1U;
+}
+
+/** Fetches the number of power cords.
+
+  @return The number of power cords.
+**/
+UINT8
+EFIAPI
+OemGetChassisNumPowerCords (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return 1;
+}
+
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
index fc4dba319aad..344343ed60a5 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
@@ -162,6 +162,14 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer)
   UnicodeStrToAsciiStrS (AssertTag, StrStart, AssertTagStrLen + 1);
   StrStart += AssertTagStrLen + 1;
   UnicodeStrToAsciiStrS (ChassisSkuNumber, StrStart, ChaNumStrLen + 1);
+
+  SmbiosRecord->BootupState = OemGetChassisBootupState ();
+  SmbiosRecord->PowerSupplyState = OemGetChassisPowerSupplyState ();
+  SmbiosRecord->ThermalState = OemGetChassisThermalState ();
+  SmbiosRecord->SecurityStatus = OemGetChassisSecurityStatus ();
+  SmbiosRecord->Height = OemGetChassisHeight ();
+  SmbiosRecord->NumberofPowerCords = OemGetChassisNumPowerCords ();
+
   //
   // Now we have got the full smbios record, call smbios protocol to add this record.
   //
-- 
2.26.2

[PATCH 3/5] ArmPkg: Allow platforms to report their boot status via OemMiscLib call

[Rebecca Cran]

Add a new function to OemMiscLib to allow platforms to report their boot
status into the Type32 SMBIOS table.

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 ArmPkg/Include/Library/OemMiscLib.h                                        | 10 ++++++++++
 ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                        | 14 ++++++++++++++
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c |  3 +++
 3 files changed, 27 insertions(+)

diff --git a/ArmPkg/Include/Library/OemMiscLib.h b/ArmPkg/Include/Library/OemMiscLib.h
index 25ae508ddb03..0b03fe8d4de3 100644
--- a/ArmPkg/Include/Library/OemMiscLib.h
+++ b/ArmPkg/Include/Library/OemMiscLib.h
@@ -162,6 +162,16 @@ OemUpdateSmbiosInfo (
   IN OEM_MISC_SMBIOS_HII_STRING_FIELD Field
   );
 
+/** Fetches the Type 32 boot information status.
+
+  @return Boot status.
+**/
+MISC_BOOT_INFORMATION_STATUS_DATA_TYPE
+EFIAPI
+OemGetBootStatus (
+  VOID
+  );
+
 /** Fetches the chassis status when it was last booted.
 
  @return Chassis status.
diff --git a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
index e6a4793fe1c2..74101beab1cb 100644
--- a/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
+++ b/ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c
@@ -140,6 +140,20 @@ OemUpdateSmbiosInfo (
   ASSERT (FALSE);
 }
 
+/** Fetches the Type 32 boot information status.
+
+  @return Boot status.
+**/
+MISC_BOOT_INFORMATION_STATUS_DATA_TYPE
+EFIAPI
+OemGetBootStatus (
+  VOID
+  )
+{
+  ASSERT (FALSE);
+  return BootInformationStatusNoError;
+}
+
 /** Fetches the chassis status when it was last booted.
 
  @return Chassis status.
diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c
index 733615bbcf1a..4be1e1cd29a9 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c
@@ -16,6 +16,7 @@
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MemoryAllocationLib.h>
+#include <Library/OemMiscLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 
 #include "SmbiosMisc.h"
@@ -59,6 +60,8 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscBootInformation)
 
   SmbiosRecord->Hdr.Length = sizeof (SMBIOS_TABLE_TYPE32);
 
+  SmbiosRecord->BootStatus = OemGetBootStatus ();
+
   //
   // Now we have got the full smbios record, call smbios protocol to add this record.
   //
-- 
2.26.2

[PATCH 4/5] ArmPkg: Fix calculation of offset of chassis SKU Number in SmbiosMiscDxe

[Rebecca Cran]

The calculation of the chassis SKU number field was being calculated
incorrectly, forgetting that there's one element already present in
the structure.
Fix the calculation and improve code readability by introducing a
SkuNumberField variable.

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
index 344343ed60a5..66e3e5327fc3 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c
@@ -39,6 +39,7 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer)
 {
   CHAR8                           *OptionalStrStart;
   CHAR8                           *StrStart;
+  UINT8                           *SkuNumberField;
   UINTN                           RecordLength;
   UINTN                           ManuStrLen;
   UINTN                           VerStrLen;
@@ -117,10 +118,7 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer)
   ChaNumStrLen = StrLen (ChassisSkuNumber);
 
   ContainedElementCount = InputData->ContainedElementCount;
-
-  if (ContainedElementCount > 1) {
-    ExtendLength = (ContainedElementCount - 1) * sizeof (CONTAINED_ELEMENT);
-  }
+  ExtendLength = ContainedElementCount * sizeof (CONTAINED_ELEMENT);
 
   //
   // Two zeros following the last string.
@@ -149,7 +147,11 @@ SMBIOS_MISC_TABLE_FUNCTION(MiscChassisManufacturer)
   (VOID)CopyMem (SmbiosRecord + 1, &ContainedElements, ExtendLength);
 
   //ChassisSkuNumber
-  *((UINT8 *)SmbiosRecord + sizeof (SMBIOS_TABLE_TYPE3) + ExtendLength) = 5;
+  SkuNumberField = (UINT8 *)SmbiosRecord +
+                   sizeof (SMBIOS_TABLE_TYPE3) -
+                   sizeof (CONTAINED_ELEMENT) + ExtendLength;
+
+  *SkuNumberField = 5;
 
   OptionalStrStart = (CHAR8 *)((UINT8 *)SmbiosRecord + sizeof (SMBIOS_TABLE_TYPE3) +
                                         ExtendLength + 1);
-- 
2.26.2

[PATCH 5/5] ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe

[Rebecca Cran]

'Manufacturer' was spelled wrongly in a comment in
MiscChassisManufacturerData.c.

Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
---
 ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
index 137bd941d0b1..29449b871902 100644
--- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
+++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c
@@ -23,7 +23,7 @@ SMBIOS_MISC_TABLE_DATA(SMBIOS_TABLE_TYPE3, MiscChassisManufacturer) = {
     0,                                                    // Length,
     0                                                     // Handle
   },
-  1,                                                      // Manufactrurer
+  1,                                                      // Manufacturer
   MiscChassisTypeMainServerChassis,                       // Type
   2,                                                      // Version
   3,                                                      // SerialNumber
-- 
2.26.2

Re: [PATCH 0/5] ArmPkg: OemMiscLib and Universal/Smbios improvements and fixes

[Leif Lindholm]

On Tue, Mar 30, 2021 at 20:16:14 -0600, Rebecca Cran wrote:
> Add the set of improvements requested during the initial series to move
> closer to a full/proper implementation, fix the
> chassis SKU number string, and fix a typo in a comment.
> 
> This is a breaking change wrt SbsaQemu in edk2-platforms.

I can't find a corresponding patch for that.
Are you looking for the APIs in this set to stabilise first?

/
    Leif

> Rebecca Cran (5):
>   ArmPkg: Allow platforms to override PCI supported state in
>     SmbiosMiscDxe
>   ArmPkg: Allow platforms to supply more data for SMBIOS Type3 record
>   ArmPkg: Allow platforms to report their boot status via OemMiscLib
>     call
>   ArmPkg: Fix calculation of offset of chassis SKU Number in
>     SmbiosMiscDxe
>   ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe
> 
>  ArmPkg/ArmPkg.dec                                                              |  1 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf                        |  1 +
>  ArmPkg/Include/Library/OemMiscLib.h                                            | 70 ++++++++++++++
>  ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                            | 99 ++++++++++++++++++++
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c          |  4 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c     |  2 +-
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 20 +++-
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c     |  3 +
>  8 files changed, 194 insertions(+), 6 deletions(-)
> 
> -- 
> 2.26.2
> 

Re: [PATCH 1/5] ArmPkg: Allow platforms to override PCI supported state in SmbiosMiscDxe

[Leif Lindholm]

On Tue, Mar 30, 2021 at 20:16:15 -0600, Rebecca Cran wrote:
> Not all platforms support PCI, so introduce a PCD to allow platforms to
> specify whether they support it.

Are we planning to add one?
If not, I'd rather skip this until we do.
These days, I would expect any platform providing SMBIOS tables to
have PCI.

No further comments on this set.

/
    Leif

> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> ---
>  ArmPkg/ArmPkg.dec                                                     | 1 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf               | 1 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 4 ++++
>  3 files changed, 6 insertions(+)
> 
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index a8a22c649ff8..51ac2191c85a 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -125,6 +125,7 @@ [PcdsFixedAtBuild.common]
>    #
>    # SMBIOS PCDs
>    #
> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI|TRUE|BOOLEAN|0x30000052
>    gArmTokenSpaceGuid.PcdSystemProductName|L""|VOID*|0x30000053
>    gArmTokenSpaceGuid.PcdSystemVersion|L""|VOID*|0x30000054
>    gArmTokenSpaceGuid.PcdBaseBoardManufacturer|L""|VOID*|0x30000055
> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> index 60d8fe31c219..ebc4c99ac436 100644
> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> @@ -71,6 +71,7 @@ [Pcd]
>    gArmTokenSpaceGuid.PcdFdSize
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI
>    gArmTokenSpaceGuid.PcdSystemBiosRelease
>    gArmTokenSpaceGuid.PcdEmbeddedControllerFirmwareRelease
>    gArmTokenSpaceGuid.PcdSystemProductName
> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> index 5aea32521bd3..a06f814aeb7c 100644
> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> @@ -13,6 +13,7 @@
>  #include <Library/DebugLib.h>
>  #include <Library/HiiLib.h>
>  #include <Library/MemoryAllocationLib.h>
> +#include <Library/PcdLib.h>
>  #include <Library/PrintLib.h>
>  #include <Library/UefiBootServicesTableLib.h>
>  
> @@ -264,6 +265,9 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor)
>    UnicodeStrToAsciiStrS (Version, StrStart, VerStrLen + 1);
>    StrStart += VerStrLen + 1;
>    UnicodeStrToAsciiStrS (ReleaseDate, StrStart, DateStrLen + 1);
> +
> +  SmbiosRecord->BiosCharacteristics.PciIsSupported = FixedPcdGetBool (PcdPlatformSupportsPCI);
> +
>    //
>    // Now we have got the full smbios record, call smbios protocol to add this record.
>    //
> -- 
> 2.26.2
> 

Re: [PATCH 1/5] ArmPkg: Allow platforms to override PCI supported state in SmbiosMiscDxe

[Ard Biesheuvel]

On Tue, 13 Apr 2021 at 18:51, Leif Lindholm <leif@nuviainc.com> wrote:
>
> On Tue, Mar 30, 2021 at 20:16:15 -0600, Rebecca Cran wrote:
> > Not all platforms support PCI, so introduce a PCD to allow platforms to
> > specify whether they support it.
>
> Are we planning to add one?
> If not, I'd rather skip this until we do.
> These days, I would expect any platform providing SMBIOS tables to
> have PCI.
>

Also, does it matter? SMBIOS is mostly informational, and whether a
platform 'supports' PCI does not imply that it 'implements' it. And
even if it implements PCI, it may not have any slots.

IOW, this is PC legacy that we care little about one way or the other, I think..


> No further comments on this set.
>
> /
>     Leif
>
> > Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
> > ---
> >  ArmPkg/ArmPkg.dec                                                     | 1 +
> >  ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf               | 1 +
> >  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 4 ++++
> >  3 files changed, 6 insertions(+)
> >
> > diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> > index a8a22c649ff8..51ac2191c85a 100644
> > --- a/ArmPkg/ArmPkg.dec
> > +++ b/ArmPkg/ArmPkg.dec
> > @@ -125,6 +125,7 @@ [PcdsFixedAtBuild.common]
> >    #
> >    # SMBIOS PCDs
> >    #
> > +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI|TRUE|BOOLEAN|0x30000052
> >    gArmTokenSpaceGuid.PcdSystemProductName|L""|VOID*|0x30000053
> >    gArmTokenSpaceGuid.PcdSystemVersion|L""|VOID*|0x30000054
> >    gArmTokenSpaceGuid.PcdBaseBoardManufacturer|L""|VOID*|0x30000055
> > diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> > index 60d8fe31c219..ebc4c99ac436 100644
> > --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> > +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
> > @@ -71,6 +71,7 @@ [Pcd]
> >    gArmTokenSpaceGuid.PcdFdSize
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
> > +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI
> >    gArmTokenSpaceGuid.PcdSystemBiosRelease
> >    gArmTokenSpaceGuid.PcdEmbeddedControllerFirmwareRelease
> >    gArmTokenSpaceGuid.PcdSystemProductName
> > diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> > index 5aea32521bd3..a06f814aeb7c 100644
> > --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> > +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
> > @@ -13,6 +13,7 @@
> >  #include <Library/DebugLib.h>
> >  #include <Library/HiiLib.h>
> >  #include <Library/MemoryAllocationLib.h>
> > +#include <Library/PcdLib.h>
> >  #include <Library/PrintLib.h>
> >  #include <Library/UefiBootServicesTableLib.h>
> >
> > @@ -264,6 +265,9 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor)
> >    UnicodeStrToAsciiStrS (Version, StrStart, VerStrLen + 1);
> >    StrStart += VerStrLen + 1;
> >    UnicodeStrToAsciiStrS (ReleaseDate, StrStart, DateStrLen + 1);
> > +
> > +  SmbiosRecord->BiosCharacteristics.PciIsSupported = FixedPcdGetBool (PcdPlatformSupportsPCI);
> > +
> >    //
> >    // Now we have got the full smbios record, call smbios protocol to add this record.
> >    //
> > --
> > 2.26.2
> >

Re: [PATCH 1/5] ArmPkg: Allow platforms to override PCI supported state in SmbiosMiscDxe

[Rebecca Cran]

On 4/13/21 10:51 AM, Leif Lindholm wrote:
> On Tue, Mar 30, 2021 at 20:16:15 -0600, Rebecca Cran wrote:
>> Not all platforms support PCI, so introduce a PCD to allow platforms to
>> specify whether they support it.
> 
> Are we planning to add one?

Not that I know of.

> If not, I'd rather skip this until we do.
> These days, I would expect any platform providing SMBIOS tables to
> have PCI.

I added it based on feedback on the original review (I think Samer 
requested it), but it sounds like dropping it would be fine.

-- 
Rebecca Cran

> 
> No further comments on this set.
> 
> /
>      Leif
> 
>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>> ---
>>   ArmPkg/ArmPkg.dec                                                     | 1 +
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf               | 1 +
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 4 ++++
>>   3 files changed, 6 insertions(+)
>>
>> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
>> index a8a22c649ff8..51ac2191c85a 100644
>> --- a/ArmPkg/ArmPkg.dec
>> +++ b/ArmPkg/ArmPkg.dec
>> @@ -125,6 +125,7 @@ [PcdsFixedAtBuild.common]
>>     #
>>     # SMBIOS PCDs
>>     #
>> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI|TRUE|BOOLEAN|0x30000052
>>     gArmTokenSpaceGuid.PcdSystemProductName|L""|VOID*|0x30000053
>>     gArmTokenSpaceGuid.PcdSystemVersion|L""|VOID*|0x30000054
>>     gArmTokenSpaceGuid.PcdBaseBoardManufacturer|L""|VOID*|0x30000055
>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>> index 60d8fe31c219..ebc4c99ac436 100644
>> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>> @@ -71,6 +71,7 @@ [Pcd]
>>     gArmTokenSpaceGuid.PcdFdSize
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
>> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI
>>     gArmTokenSpaceGuid.PcdSystemBiosRelease
>>     gArmTokenSpaceGuid.PcdEmbeddedControllerFirmwareRelease
>>     gArmTokenSpaceGuid.PcdSystemProductName
>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> index 5aea32521bd3..a06f814aeb7c 100644
>> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> @@ -13,6 +13,7 @@
>>   #include <Library/DebugLib.h>
>>   #include <Library/HiiLib.h>
>>   #include <Library/MemoryAllocationLib.h>
>> +#include <Library/PcdLib.h>
>>   #include <Library/PrintLib.h>
>>   #include <Library/UefiBootServicesTableLib.h>
>>   
>> @@ -264,6 +265,9 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor)
>>     UnicodeStrToAsciiStrS (Version, StrStart, VerStrLen + 1);
>>     StrStart += VerStrLen + 1;
>>     UnicodeStrToAsciiStrS (ReleaseDate, StrStart, DateStrLen + 1);
>> +
>> +  SmbiosRecord->BiosCharacteristics.PciIsSupported = FixedPcdGetBool (PcdPlatformSupportsPCI);
>> +
>>     //
>>     // Now we have got the full smbios record, call smbios protocol to add this record.
>>     //
>> -- 
>> 2.26.2
>>

Re: [PATCH 0/5] ArmPkg: OemMiscLib and Universal/Smbios improvements and fixes

[Rebecca Cran]

On 4/13/21 10:25 AM, Leif Lindholm wrote:
> On Tue, Mar 30, 2021 at 20:16:14 -0600, Rebecca Cran wrote:
>> Add the set of improvements requested during the initial series to move
>> closer to a full/proper implementation, fix the
>> chassis SKU number string, and fix a typo in a comment.
>>
>> This is a breaking change wrt SbsaQemu in edk2-platforms.
> 
> I can't find a corresponding patch for that.
> Are you looking for the APIs in this set to stabilise first?

I haven't sent it out yet. I think having this set reviewed and accepted 
first would be best, to avoid extra churn.

-- 
Rebecca Cran

> 
> /
>      Leif
> 
>> Rebecca Cran (5):
>>    ArmPkg: Allow platforms to override PCI supported state in
>>      SmbiosMiscDxe
>>    ArmPkg: Allow platforms to supply more data for SMBIOS Type3 record
>>    ArmPkg: Allow platforms to report their boot status via OemMiscLib
>>      call
>>    ArmPkg: Fix calculation of offset of chassis SKU Number in
>>      SmbiosMiscDxe
>>    ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe
>>
>>   ArmPkg/ArmPkg.dec                                                              |  1 +
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf                        |  1 +
>>   ArmPkg/Include/Library/OemMiscLib.h                                            | 70 ++++++++++++++
>>   ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                            | 99 ++++++++++++++++++++
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c          |  4 +
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c     |  2 +-
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 20 +++-
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c     |  3 +
>>   8 files changed, 194 insertions(+), 6 deletions(-)
>>
>> -- 
>> 2.26.2
>>

Re: [PATCH 1/5] ArmPkg: Allow platforms to override PCI supported state in SmbiosMiscDxe

[Rebecca Cran]

On 4/13/21 10:53 AM, Ard Biesheuvel wrote:
> On Tue, 13 Apr 2021 at 18:51, Leif Lindholm <leif@nuviainc.com> wrote:
>>
>> On Tue, Mar 30, 2021 at 20:16:15 -0600, Rebecca Cran wrote:
>>> Not all platforms support PCI, so introduce a PCD to allow platforms to
>>> specify whether they support it.
>>
>> Are we planning to add one?
>> If not, I'd rather skip this until we do.
>> These days, I would expect any platform providing SMBIOS tables to
>> have PCI.
>>
> 
> Also, does it matter? SMBIOS is mostly informational, and whether a
> platform 'supports' PCI does not imply that it 'implements' it. And
> even if it implements PCI, it may not have any slots.
> 
> IOW, this is PC legacy that we care little about one way or the other, I think..

Yeah, I think there was a comment on the original review that lots of 
platforms get these values wrong, and nothing depends on them.

-- 
Rebecca Cran

> 
> 
>> No further comments on this set.
>>
>> /
>>      Leif
>>
>>> Signed-off-by: Rebecca Cran <rebecca@nuviainc.com>
>>> ---
>>>   ArmPkg/ArmPkg.dec                                                     | 1 +
>>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf               | 1 +
>>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 4 ++++
>>>   3 files changed, 6 insertions(+)
>>>
>>> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
>>> index a8a22c649ff8..51ac2191c85a 100644
>>> --- a/ArmPkg/ArmPkg.dec
>>> +++ b/ArmPkg/ArmPkg.dec
>>> @@ -125,6 +125,7 @@ [PcdsFixedAtBuild.common]
>>>     #
>>>     # SMBIOS PCDs
>>>     #
>>> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI|TRUE|BOOLEAN|0x30000052
>>>     gArmTokenSpaceGuid.PcdSystemProductName|L""|VOID*|0x30000053
>>>     gArmTokenSpaceGuid.PcdSystemVersion|L""|VOID*|0x30000054
>>>     gArmTokenSpaceGuid.PcdBaseBoardManufacturer|L""|VOID*|0x30000055
>>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>>> index 60d8fe31c219..ebc4c99ac436 100644
>>> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>>> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf
>>> @@ -71,6 +71,7 @@ [Pcd]
>>>     gArmTokenSpaceGuid.PcdFdSize
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVendor
>>>     gEfiMdeModulePkgTokenSpaceGuid.PcdFirmwareVersionString
>>> +  gArmTokenSpaceGuid.PcdPlatformSupportsPCI
>>>     gArmTokenSpaceGuid.PcdSystemBiosRelease
>>>     gArmTokenSpaceGuid.PcdEmbeddedControllerFirmwareRelease
>>>     gArmTokenSpaceGuid.PcdSystemProductName
>>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>>> index 5aea32521bd3..a06f814aeb7c 100644
>>> --- a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>>> +++ b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>>> @@ -13,6 +13,7 @@
>>>   #include <Library/DebugLib.h>
>>>   #include <Library/HiiLib.h>
>>>   #include <Library/MemoryAllocationLib.h>
>>> +#include <Library/PcdLib.h>
>>>   #include <Library/PrintLib.h>
>>>   #include <Library/UefiBootServicesTableLib.h>
>>>
>>> @@ -264,6 +265,9 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor)
>>>     UnicodeStrToAsciiStrS (Version, StrStart, VerStrLen + 1);
>>>     StrStart += VerStrLen + 1;
>>>     UnicodeStrToAsciiStrS (ReleaseDate, StrStart, DateStrLen + 1);
>>> +
>>> +  SmbiosRecord->BiosCharacteristics.PciIsSupported = FixedPcdGetBool (PcdPlatformSupportsPCI);
>>> +
>>>     //
>>>     // Now we have got the full smbios record, call smbios protocol to add this record.
>>>     //
>>> --
>>> 2.26.2
>>>

Re: [PATCH 0/5] ArmPkg: OemMiscLib and Universal/Smbios improvements and fixes

[Leif Lindholm]

On Tue, Mar 30, 2021 at 20:16:14 -0600, Rebecca Cran wrote:
> Add the set of improvements requested during the initial series to move
> closer to a full/proper implementation, fix the
> chassis SKU number string, and fix a typo in a comment.
> 
> This is a breaking change wrt SbsaQemu in edk2-platforms.
> 
> Rebecca Cran (5):
>   ArmPkg: Allow platforms to override PCI supported state in
>     SmbiosMiscDxe
>   ArmPkg: Allow platforms to supply more data for SMBIOS Type3 record
>   ArmPkg: Allow platforms to report their boot status via OemMiscLib
>     call
>   ArmPkg: Fix calculation of offset of chassis SKU Number in
>     SmbiosMiscDxe
>   ArmPkg: Fix typo of Manufacturer in comment in SmbiosMiscDxe

As discussed on list, patch 1/5 was dropped.

I fixed up a trailing newline in
ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c from 2/5.

For the remainder of the set:
Reviewed-by: Leif Lindholm <leif@nuviainc.com>
Pushed as 8d2012035db3..5b90b8abb404.

>  ArmPkg/ArmPkg.dec                                                              |  1 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/SmbiosMiscDxe.inf                        |  1 +
>  ArmPkg/Include/Library/OemMiscLib.h                                            | 70 ++++++++++++++
>  ArmPkg/Universal/Smbios/OemMiscLibNull/OemMiscLib.c                            | 99 ++++++++++++++++++++
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c          |  4 +
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerData.c     |  2 +-
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type03/MiscChassisManufacturerFunction.c | 20 +++-
>  ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type32/MiscBootInformationFunction.c     |  3 +
>  8 files changed, 194 insertions(+), 6 deletions(-)
> 
> -- 
> 2.26.2
>