Re: [PATCH] UefiPayloadPkg/BlSupportDxe: Use EfiSmbiosProtocol to install tables


Patrick Rudolph
 

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 <maurice.ma@intel.com> 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 <patrick.rudolph@9elements.com>
Sent: Wednesday, January 20, 2021 8:02
To: devel@edk2.groups.io
Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>;
You, Benjamin <benjamin.you@intel.com>
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 <patrick.rudolph@9elements.com>
---
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

Join devel@edk2.groups.io to automatically receive all group messages.