Hi Maurice,
I'll adapt the function names to match EDK2 naming conventions.
The SmbiosDxe is already part of UefiPayloadPkg, so it's not optional
(right now). I don't see how registering gEfiSmbiosProtocolGuid could
fail.
If you think Depex must be true, there should be
a) a comment stating the reasons why Depex must not be changed
b) I'll have to move the SMBIOS code out of BlSupportDxe into
BlSupportSmbiosDxe and add the Depex section there.
A failed dispatch of BlSupportSmbiosDxe would then be non critical.
Do you think this would be a better solution?
I don't want to use RegisterProtocolNotify() as it's discouraged, isn't it?
Kind Regards,
Patrick Rudolph
On Thu, Jan 21, 2021 at 2:14 AM Ma, Maurice <[email protected]> wrote:
>
> Hi, Patrick
>
> In this patch I noticed that we changed the BlSupportDxe dependency from True
> to gEfiSmbiosProtocolGuid.
> Since BlSupportDxe is considered as critical for UEFI payload, and on the
> other side SMBIOS driver could be optional in some case, do you think it is
> better to handle it through RegisterProtocolNotify() ? In this way, if
> gEfiSmbiosProtocolGuid is not installed for any reason, BlSupportDxe can
> still be dispatched and the boot flow can continue.
>
> Some other comments:
> - Please add function and parameter description for
> BlDxeInstallSMBIOStables().
> - To follow the naming convention in EDK2, maybe use
> BlDxeInstallSmbiosTables instead of BlDxeInstallSMBIOStables().
>
> Thanks
> Maurice
> > -----Original Message-----
> > From: Patrick Rudolph <[email protected]>
> > Sent: Wednesday, January 20, 2021 8:02
> > To: [email protected]
> > Cc: Ma, Maurice <[email protected]>; Dong, Guo <[email protected]>;
> > You, Benjamin <[email protected]>
> > Subject: [PATCH] UefiPayloadPkg/BlSupportDxe: Use EfiSmbiosProtocol to
> > install tables
> >
> > The default EfiSmbiosProtocol operates on an empty SMBIOS table.
> > As the SMBIOS tables are provided by the bootloader, install the SMBIOS
> > tables
> > using the EfiSmbiosProtocol.
> >
> > This fixes the settings menu not showing any hardware information, instead
> > only
> > "0 MB RAM" was displayed.
> >
> > Tests showed that the OS can still see the SMBIOS tables.
> >
> > Signed-off-by: Patrick Rudolph <[email protected]>
> > ---
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c | 111
> > +++++++++++++++++++-
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h | 3 +
> > UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf | 5 +-
> > 3 files changed, 115 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> > b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> > index a746d0581e..db478c1abc 100644
> > --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> > +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> > @@ -79,6 +79,107 @@ ReserveResourceInGcd (
> > return Status; } +EFI_STATUS+EFIAPI+BlDxeInstallSMBIOStables(+ IN UINT64
> > SmbiosTableBase,+ IN UINT32 SmbiosTableSize+)+{+ EFI_STATUS
> > Status;+ SMBIOS_TABLE_ENTRY_POINT *SmbiosTable;+
> > SMBIOS_TABLE_3_0_ENTRY_POINT *Smbios30Table;+
> > SMBIOS_STRUCTURE_POINTER Smbios;+ SMBIOS_STRUCTURE_POINTER
> > SmbiosEnd;+ CHAR8 *String;+ EFI_SMBIOS_HANDLE
> > SmbiosHandle;+ EFI_SMBIOS_PROTOCOL *SmbiosProto;++ //+ //
> > Locate
> > Smbios protocol.+ //+ Status = gBS->LocateProtocol
> > (&gEfiSmbiosProtocolGuid,
> > NULL, (VOID **)&SmbiosProto);+ if (EFI_ERROR (Status)) {+ DEBUG
> > ((DEBUG_ERROR, "%a: Failed to locate gEfiSmbiosProtocolGuid\n",+
> > __FUNCTION__));+ return Status;+ }++ Smbios30Table =
> > (SMBIOS_TABLE_3_0_ENTRY_POINT *)(UINTN)(SmbiosTableBase);+
> > SmbiosTable = (SMBIOS_TABLE_ENTRY_POINT *)(UINTN)(SmbiosTableBase);++
> > if (CompareMem (Smbios30Table->AnchorString, "_SM3_", 5) == 0) {+
> > Smbios.Hdr = (SMBIOS_STRUCTURE *) (UINTN) Smbios30Table-
> > >TableAddress;+ SmbiosEnd.Raw = (UINT8 *) (UINTN) (Smbios30Table-
> > >TableAddress + Smbios30Table->TableMaximumSize);+ if (Smbios30Table-
> > >TableMaximumSize > SmbiosTableSize) {+ DEBUG((DEBUG_INFO, "%a:
> > SMBIOS table size greater than reported by bootloader\n",+
> > __FUNCTION__));+ }+ } else if (CompareMem (SmbiosTable->AnchorString,
> > "_SM_", 4) == 0) {+ Smbios.Hdr = (SMBIOS_STRUCTURE *) (UINTN)
> > SmbiosTable->TableAddress;+ SmbiosEnd.Raw = (UINT8 *) ((UINTN)
> > SmbiosTable->TableAddress + SmbiosTable->TableLength);++ if (SmbiosTable-
> > >TableLength > SmbiosTableSize) {+ DEBUG((DEBUG_INFO, "%a: SMBIOS
> > table size greater than reported by bootloader\n",+
> > __FUNCTION__));+ }+ } else {+ DEBUG ((DEBUG_ERROR, "%a: No valid
> > SMBIOS table found\n", __FUNCTION__ ));+ return EFI_NOT_FOUND;+ }++
> > do {+ // Check for end marker+ if (Smbios.Hdr->Type == 127) {+
> > break;+ }++ // Install the table+ SmbiosHandle =
> > SMBIOS_HANDLE_PI_RESERVED;+ Status = SmbiosProto->Add (+
> > SmbiosProto,+ gImageHandle,+
> > &SmbiosHandle,+
> > Smbios.Hdr+ );+ ASSERT_EFI_ERROR (Status);+
> > if (EFI_ERROR
> > (Status)) {+ return Status;+ }+ //+ // Go to the next SMBIOS
> > structure.
> > Each SMBIOS structure may include 2 parts:+ // 1. Formatted section; 2.
> > Unformatted string section. So, 2 steps are needed+ // to skip one SMBIOS
> > structure.+ //++ //+ // Step 1: Skip over formatted section.+
> > //+ String =
> > (CHAR8 *) (Smbios.Raw + Smbios.Hdr->Length);++ //+ // Step 2: Skip
> > over
> > unformatted string section.+ //+ do {+ //+ // Each string
> > is terminated
> > with a NULL(00h) BYTE and the sets of strings+ // is terminated with an
> > additional NULL(00h) BYTE.+ //+ for ( ; *String != 0; String++)
> > {+ }++ if
> > (*(UINT8*)++String == 0) {+ //+ // Pointer to the next SMBIOS
> > structure.+ //+ Smbios.Raw = (UINT8 *)++String;+
> > break;+ }+ }
> > while (TRUE);+ } while (Smbios.Raw < SmbiosEnd.Raw);++ return
> > EFI_SUCCESS;+} /** Main entry for the bootloader support DXE module.@@ -
> > 133,9 +234,13 @@ BlDxeEntryPoint (
> > // Install Smbios Table // if (SystemTableInfo->SmbiosTableBase != 0
> > &&
> > SystemTableInfo->SmbiosTableSize != 0) {- DEBUG ((DEBUG_ERROR, "Install
> > Smbios Table at 0x%lx, length 0x%x\n", SystemTableInfo->SmbiosTableBase,
> > SystemTableInfo->SmbiosTableSize));- Status =
> > gBS->InstallConfigurationTable
> > (&gEfiSmbiosTableGuid, (VOID *)(UINTN)SystemTableInfo->SmbiosTableBase);-
> > ASSERT_EFI_ERROR (Status);+ DEBUG ((DEBUG_ERROR, "Install Smbios Table
> > at 0x%lx, length 0x%x\n",+ SystemTableInfo->SmbiosTableBase,
> > SystemTableInfo->SmbiosTableSize));++ if
> > (BlDxeInstallSMBIOStables(SystemTableInfo->SmbiosTableBase,
> > SystemTableInfo->SmbiosTableSize) != EFI_SUCCESS) {+ Status = gBS-
> > >InstallConfigurationTable (&gEfiSmbiosTableGuid, (VOID
> > *)(UINTN)SystemTableInfo->SmbiosTableBase);+ ASSERT_EFI_ERROR
> > (Status);+ } } //diff --git
> > a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> > b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> > index 512105fafd..a5216cd2e9 100644
> > --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> > +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> > @@ -10,6 +10,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <PiDxe.h> +#include <Protocol/Smbios.h>+ #include
> > <Library/UefiDriverEntryPoint.h> #include
> > <Library/UefiBootServicesTableLib.h>
> > #include <Library/DxeServicesTableLib.h>@@ -26,5 +28,6 @@ SPDX-License-
> > Identifier: BSD-2-Clause-Patent #include <Guid/GraphicsInfoHob.h> #include
> > <IndustryStandard/Acpi.h>+#include <IndustryStandard/SmBios.h> #endifdiff -
> > -git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> > b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> > index cebc811355..d26a75248b 100644
> > --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> > +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> > @@ -56,5 +56,8 @@
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> > gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize +[Protocols]+
> > gEfiSmbiosProtocolGuid+ [Depex]- TRUE+ gEfiSmbiosProtocolGuid--
> > 2.26.2
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#70611): https://edk2.groups.io/g/devel/message/70611
Mute This Topic: https://groups.io/mt/79981721/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
