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

["Sami Mujawar" <sami.mujawar@arm.com>]

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
    >>