* [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
@ 2023-03-13 6:43 Tinh Nguyen
2023-03-13 15:03 ` Leif Lindholm
2023-03-16 8:30 ` Tinh Nguyen
0 siblings, 2 replies; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-13 6:43 UTC (permalink / raw)
To: devel; +Cc: patches, quic_llindhol, ardb+tianocore, Tinh Nguyen, Nhi Pham
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.
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 ();
+ 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
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-13 6:43 [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version Tinh Nguyen
@ 2023-03-13 15:03 ` Leif Lindholm
2023-03-13 16:52 ` Tinh Nguyen
2023-03-16 8:30 ` Tinh Nguyen
1 sibling, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2023-03-13 15:03 UTC (permalink / raw)
To: Tinh Nguyen; +Cc: devel, patches, ardb+tianocore, Nhi Pham
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?
> 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.
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.
/
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-13 15:03 ` Leif Lindholm
@ 2023-03-13 16:52 ` Tinh Nguyen
2023-03-14 12:48 ` [edk2-devel] " Leif Lindholm
0 siblings, 1 reply; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-13 16:52 UTC (permalink / raw)
To: Leif Lindholm, Tinh Nguyen; +Cc: devel, patches, ardb+tianocore, Nhi Pham
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
>>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-13 16:52 ` Tinh Nguyen
@ 2023-03-14 12:48 ` Leif Lindholm
2023-03-14 17:59 ` Rebecca Cran
0 siblings, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2023-03-14 12:48 UTC (permalink / raw)
To: devel, tinhnguyen; +Cc: patches, ardb+tianocore, Nhi Pham, Rebecca Cran
Hi Tinh, +Rebecca
On Mon, Mar 13, 2023 at 23:52:41 +0700, Tinh Nguyen via groups.io wrote:
> 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.
Ah, ok - so the edk2 image never knows a proper version name, so you
want to keep the Pcd as a fallback only, to provide the edk2 build
version if no other info can be retrieved?
That's valid, just not a case I would have expected.
> > > 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.
I see now why you want to re-jig the current logic, and that includes
the selection logic. But this function is already too long. It needs
to be simplified, not made more complex.
> > 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.
That's the function - I was asking for the purpose, which you
described higher up in your reply.
> 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.
Hmm. On one level I think you're highlighting a desire for separation
of "firmware version" from "edk2 version", which isn't something the
codebase generally does today.
But we don't need to solve that completely to make this change.
> I think it makes sense to prioritize OemMiscLib since it's more flexible
> than Pcd.
No objection to that.
But can we do it like this?:
Change GetBiosVersion to SetBiosVersion and in MiscBiosVendor, only call
SetBiosVersion ();
and move the selection logic fully into SetBiosVersion?
Rebecca, thoughts?
Arguably, once an OemMiscLib dependency was added, the Pcd values became
less useful in the core code.
/
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
> > >
>
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-14 12:48 ` [edk2-devel] " Leif Lindholm
@ 2023-03-14 17:59 ` Rebecca Cran
2023-03-17 14:52 ` Tinh Nguyen
0 siblings, 1 reply; 10+ messages in thread
From: Rebecca Cran @ 2023-03-14 17:59 UTC (permalink / raw)
To: Leif Lindholm, devel, tinhnguyen; +Cc: patches, ardb+tianocore, Nhi Pham
On 3/14/23 6:48 AM, Leif Lindholm wrote:
> No objection to that.
> But can we do it like this?:
>
> Change GetBiosVersion to SetBiosVersion and in MiscBiosVendor, only call
>
> SetBiosVersion ();
>
> and move the selection logic fully into SetBiosVersion?
>
> Rebecca, thoughts?
> Arguably, once an OemMiscLib dependency was added, the Pcd values became
> less useful in the core code.
That sounds very reasonable.
--
Rebecca Cran
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-13 6:43 [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version Tinh Nguyen
2023-03-13 15:03 ` Leif Lindholm
@ 2023-03-16 8:30 ` Tinh Nguyen
2023-03-16 13:00 ` [edk2-devel] " Rebecca Cran
1 sibling, 1 reply; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-16 8:30 UTC (permalink / raw)
To: Tinh Nguyen, devel
Cc: patches, quic_llindhol, ardb+tianocore, Nhi Pham, rebecca
+ Rebecca
Could you kindly help me in reviewing this patch?
Thanks,
- Tinh
On 13/03/2023 13:43, 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.
>
> 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 ();
> + 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 ();
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-16 8:30 ` Tinh Nguyen
@ 2023-03-16 13:00 ` Rebecca Cran
2023-03-17 14:50 ` Tinh Nguyen
2023-03-17 14:50 ` Tinh Nguyen
0 siblings, 2 replies; 10+ messages in thread
From: Rebecca Cran @ 2023-03-16 13:00 UTC (permalink / raw)
To: devel, tinhnguyen; +Cc: patches, quic_llindhol, ardb+tianocore, Nhi Pham
Given Leif's comments, I was expecting a new version with some changes.
--
Rebecca Cran
On 3/16/23 2:30 AM, Tinh Nguyen via groups.io wrote:
> + Rebecca
>
> Could you kindly help me in reviewing this patch?
>
> Thanks,
>
> - Tinh
>
> On 13/03/2023 13:43, 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.
>>
>> 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 ();
>> + 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 ();
>
>
>
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-16 13:00 ` [edk2-devel] " Rebecca Cran
@ 2023-03-17 14:50 ` Tinh Nguyen
2023-03-17 14:50 ` Tinh Nguyen
1 sibling, 0 replies; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-17 14:50 UTC (permalink / raw)
To: Rebecca Cran, devel, tinhnguyen
I sorry for not seeing your and Leif's emails, so I added you again.
- Tinh
On 3/16/2023 8:00 PM, Rebecca Cran wrote:
> Given Leif's comments, I was expecting a new version with some changes.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-16 13:00 ` [edk2-devel] " Rebecca Cran
2023-03-17 14:50 ` Tinh Nguyen
@ 2023-03-17 14:50 ` Tinh Nguyen
1 sibling, 0 replies; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-17 14:50 UTC (permalink / raw)
To: Rebecca Cran, devel
I sorry for not seeing your and Leif's emails, so I added you again.
- Tinh
On 3/16/2023 8:00 PM, Rebecca Cran wrote:
> Given Leif's comments, I was expecting a new version with some changes.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [edk2-devel] [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version
2023-03-14 17:59 ` Rebecca Cran
@ 2023-03-17 14:52 ` Tinh Nguyen
0 siblings, 0 replies; 10+ messages in thread
From: Tinh Nguyen @ 2023-03-17 14:52 UTC (permalink / raw)
To: Rebecca Cran, Leif Lindholm, devel; +Cc: patches, ardb+tianocore, Nhi Pham
Thank you for your feedback; I will submit v2 as soon as possible.
Regards,
Tinh
On 3/15/2023 12:59 AM, Rebecca Cran wrote:
> On 3/14/23 6:48 AM, Leif Lindholm wrote:
>> No objection to that.
>> But can we do it like this?:
>>
>> Change GetBiosVersion to SetBiosVersion and in MiscBiosVendor, only call
>>
>> SetBiosVersion ();
>>
>> and move the selection logic fully into SetBiosVersion?
>>
>> Rebecca, thoughts?
>> Arguably, once an OemMiscLib dependency was added, the Pcd values became
>> less useful in the core code.
>
> That sounds very reasonable.
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-03-17 14:52 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-03-13 6:43 [PATCH 1/1] ArmPkg/SmbiosMiscDxe: Adjust the priority of getting firmware version Tinh Nguyen
2023-03-13 15:03 ` Leif Lindholm
2023-03-13 16:52 ` Tinh Nguyen
2023-03-14 12:48 ` [edk2-devel] " Leif Lindholm
2023-03-14 17:59 ` Rebecca Cran
2023-03-17 14:52 ` Tinh Nguyen
2023-03-16 8:30 ` Tinh Nguyen
2023-03-16 13:00 ` [edk2-devel] " Rebecca Cran
2023-03-17 14:50 ` Tinh Nguyen
2023-03-17 14:50 ` Tinh Nguyen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox