* [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: [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
* 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
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