Re: [edk2-devel] [PATCH v3 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version

["Tinh Nguyen" <tinhnguyen@os.amperecomputing.com>]

Thanks Oliver

will be improved in the next version

- Tinh

On 3/21/2023 11:57 PM, Oliver Smith-Denny wrote:
> One nit below, otherwise:
>
> Reviewed-by: Oliver Smith-Denny <osd@smith-denny.com>
>
> Thanks!
>
> On 3/20/2023 8:16 PM, Tinh Nguyen via groups.io wrote:
>> The BIOS Firmware Version in the SMBIOS Type 0 can be fetched from
>> the fixed PcdFirmwareVersionString or platform specific OemMiscLib.
>> In fact, the support from OemMiscLib comes into play when the firmware
>> version may be modified at boot time for extended information.
>> Therefore, the priority of getting the version from OemMiscLib should
>> be higher.
>>
>> In case there is no modification in the OemMiscLib, we have to keep
>> HII string STR_MISC_BIOS_VERSION empty or 'Not Specified'
>> to indicate that the firmware version should be fetched from
>> the PcdFirmwareVersionString.
>>
>> Signed-off-by: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>
>> Reviewed-by: Rebecca Cran <rebecca@bsdio.com>
>> ---
>>
>> Changes since v1:
>>    + Change GetBiosVersion () to SetBiosVersion () and move the 
>> selection logic
>>    fully into SetBiosVersion ().
>> Changes since v2:
>>    + Add Reviewed-by: Rebecca Cran <rebecca@bsdio.com> and remove 
>> @retval
>>    VOID
>>
>> ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c 
>> | 57 ++++++++++++--------
>>   1 file changed, 35 insertions(+), 22 deletions(-)
>>
>> diff --git 
>> a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c 
>> b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> index 66ead22a6e2c..876a74614285 100644
>> --- 
>> a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> +++ 
>> b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> @@ -1,6 +1,6 @@
>>   /** @file
>>   -  Copyright (c) 2022, Ampere Computing LLC. All rights reserved.<BR>
>> +  Copyright (c) 2022 - 2023, 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>
>> @@ -124,22 +124,46 @@ GetBiosReleaseDate (
>>     return ReleaseDate;
>>   }
>>   -/**
>> -  Fetches the firmware ('BIOS') version from the
>> -  FirmwareVersionInfo HOB.
>> +/**  Fetches the Firmware version string for SMBIOS type 0
>> +
>> +  This function get the Firmware version string from OemMiscLib first,
>> +  if it is invalid then PcdFirmwareVersionString is used as a fallback.
>
> nit: this function comment seems a bit at odds with the name of the 
> function (i.e. the comment says it gets the FW version, but the name 
> of the function is SetBiosVersion, which I see was changed in this 
> patch). Perhaps updating the comment to indicate it retrieves the BIOS 
> version from OemMiscLib or PcdFirmwareVersionString and then sets it 
> in SMBIOS.
>
>>   -  @return The version as a UTF-16 string
>>   **/
>> -CHAR16 *
>> -GetBiosVersion (
>> +VOID
>> +SetBiosVersion (
>>     VOID
>>     )
>>   {
>> -  CHAR16  *ReleaseString;
>> +  CHAR16         *DefaultVersionString;
>> +  CHAR16         *Version;
>> +  EFI_STRING_ID  TokenToUpdate;
>>   -  ReleaseString = (CHAR16 *)FixedPcdGetPtr 
>> (PcdFirmwareVersionString);
>> +  DefaultVersionString = HiiGetString (
>> +                           mSmbiosMiscHiiHandle,
>> +                           STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> +                           NULL
>> +                           );
>>   -  return ReleaseString;
>> +  OemUpdateSmbiosInfo (
>> +    mSmbiosMiscHiiHandle,
>> +    STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> +    BiosVersionType00
>> +    );
>> +
>> +  Version = HiiGetString (
>> +              mSmbiosMiscHiiHandle,
>> +              STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> +              NULL
>> +              );
>> +
>> +  if (((StrCmp (Version, DefaultVersionString) == 0) || (StrLen 
>> (Version) == 0))) {
>> +    Version = (CHAR16 *)FixedPcdGetPtr (PcdFirmwareVersionString);
>> +    if (StrLen (Version) > 0) {
>> +      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>> +      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, 
>> NULL);
>> +    }
>> +  }
>>   }
>>     /**
>> @@ -187,18 +211,7 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>>       HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Vendor, NULL);
>>     }
>>   -  Version = GetBiosVersion ();
>> -
>> -  if (StrLen (Version) > 0) {
>> -    TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>> -    HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>> -  } else {
>> -    OemUpdateSmbiosInfo (
>> -      mSmbiosMiscHiiHandle,
>> -      STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> -      BiosVersionType00
>> -      );
>> -  }
>> +  SetBiosVersion ();
>>       Char16String = GetBiosReleaseDate ();
>>     if (StrLen (Char16String) > 0) {
>> -- 
>> 2.39.2
>>
>>
>> 
>>
>>