Hi Nhi, Please see my response inline marked [SAMI].
Regards, Sami Mujawar On 15/09/2022, 19:23, "Nhi Pham" <n...@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 <mi...@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 <n...@os.amperecomputing.com> >> Reviewed-by: Rebecca Cran <rebe...@quicinc.com> >> Reviewed-by: Sami Mujawar <sami.muja...@arm.com> >> Acked-by: Ard Biesheuvel <a...@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_llind...@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 >> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#93905): https://edk2.groups.io/g/devel/message/93905 Mute This Topic: https://groups.io/mt/93650292/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-