[edk2-devel] [PATCH v3 6/6] ArmPkg/SmbiosMiscDxe: Get SMBIOS information from OemMiscLib
Nhi Pham via groups.io
nhi=os.amperecomputing.com at groups.io
Fri Sep 16 10:33:32 UTC 2022
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 at 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 at 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 at os.amperecomputing.com>
> >> Reviewed-by: Rebecca Cran <rebecca at quicinc.com>
> >> Reviewed-by: Sami Mujawar <sami.mujawar at arm.com>
> >> Acked-by: Ard Biesheuvel <ardb at 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 at 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
> >>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#93907): https://edk2.groups.io/g/devel/message/93907
Mute This Topic: https://groups.io/mt/93650292/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list