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" <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]
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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
