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

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

Hi Leif,

My comments is in below

On 3/13/2023 10:03 PM, Leif Lindholm wrote:
> On Mon, Mar 13, 2023 at 13:43:21 +0700, Tinh Nguyen 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.
> Wait. Firmware version modified at boot time for extended information?
> What type of extended information?
That could be SCP version, TF-A version, etc.
>> 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.
>>
>> Reviewed-by: Nhi Pham <nhi@os.amperecomputing.com>
>> Signed-off-by: Tinh Nguyen <tinhnguyen@os.amperecomputing.com>
>> ---
>>   ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c | 36 ++++++++++++++------
>>   1 file changed, 25 insertions(+), 11 deletions(-)
>>
>> diff --git a/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c b/ArmPkg/Universal/Smbios/SmbiosMiscDxe/Type00/MiscBiosVendorFunction.c
>> index 66ead22a6e2c..31a3f6cde544 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>
>> @@ -170,6 +170,7 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>>     EFI_STRING_ID       TokenToGet;
>>     SMBIOS_TABLE_TYPE0  *SmbiosRecord;
>>     SMBIOS_TABLE_TYPE0  *InputData;
>> +  CHAR16              *DefaultVersionString;
>>   
>>     //
>>     // First check for invalid parameters.
>> @@ -187,17 +188,30 @@ SMBIOS_MISC_TABLE_FUNCTION (MiscBiosVendor) {
>>       HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Vendor, NULL);
>>     }
>>   
>> -  Version = GetBiosVersion ();
> GetBiosVersion exists as a helper function to avoid cluttering
> higher-level functions with unnecessary details. If this change is
> needed, that is where it should go.

This change is based on existing code, and goal is to get the Firmware 
version string from OemMiscLib first, rather than PcdFirmwareVersionString.

  I would rather not make any changes that aren't relevant.

> But I still don't understand the purpose of this patch, so cannot
> comment on whether I feel this is the right approach or not.
> Please elaborate.

The firmware version is currently being updated:

1. Get the string from Fixed PcdFirmwareVersionString.

2. Check to see if this string is null; if not, update the SMBIOS 
firmware version string.

3. if PcdFirmwareVersionString is null, retrieve the string from OemMiscLib.


As you can see, the implementation is intended to honor the 
PcdFirmwareVersionString.

We can't get the extend information (which can be derived from runtime) 
into the SMBIOS firmware version string if we don't set 
PcdFirmwareVersionString null

However,  in some cases we don't want to set PcdFirmwareVersionString to 
null because it might be used by other modules.


   I think it makes sense to prioritize OemMiscLib since it's more 
flexible than Pcd.
> /
>      Leif
>
>> +  DefaultVersionString = HiiGetString (
>> +                           mSmbiosMiscHiiHandle,
>> +                           STRING_TOKEN (STR_MISC_BIOS_VERSION),
>> +                           NULL
>> +                           );
>>   
>> -  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
>> -      );
>> +  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 = GetBiosVersion ();
>> +    if (StrLen (Version) > 0) {
>> +      TokenToUpdate = STRING_TOKEN (STR_MISC_BIOS_VERSION);
>> +      HiiSetString (mSmbiosMiscHiiHandle, TokenToUpdate, Version, NULL);
>> +    }
>>     }
>>   
>>     Char16String = GetBiosReleaseDate ();
>> -- 
>> 2.39.2
>>