From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oo1-f43.google.com (mail-oo1-f43.google.com [209.85.161.43]) by mx.groups.io with SMTP id smtpd.web11.4228.1611216689939168382 for ; Thu, 21 Jan 2021 00:11:30 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@9elements.com header.s=google header.b=Hweayz6x; spf=pass (domain: 9elements.com, ip: 209.85.161.43, mailfrom: patrick.rudolph@9elements.com) Received: by mail-oo1-f43.google.com with SMTP id x203so287371ooa.9 for ; Thu, 21 Jan 2021 00:11:29 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=9elements.com; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=AS1FAjGSh+kkIUznHQin5Iip6OK00b21XLBMielDIZQ=; b=Hweayz6xFqmAUhzomkP/20AFW/7jZjX0xs9l5dROj2KZ7M1kU0kcBf0s1FSXqhFf8Z GGVdjzwOjbT8mAtrEUv8jacGklpfdJigYQNeBvTlpHJ+CRtMH2tCoK+DMxB68t7yFr5Q ssd02Ucti309fAS4ef+/zzaV3JyEYRHYCGJJpjF0+GZn3BVARe7RUv63UUR3bSos+3pU N3S7t29W5gOhAt2vHyN8dR5DCeGFMVRDv/DH+Mk3NuCuLXWWfdO9h1gVd1sICfNhBevC P56r7vMmesSKG1PlECewFxvuklHOpjluNtbJwEpyCGndkrwmia4tpD/LhGjBXKRYfg0Z 9vBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=AS1FAjGSh+kkIUznHQin5Iip6OK00b21XLBMielDIZQ=; b=ZgeXBZTUeaCqrDRYtRDUnJOrEB8OEBWFYuuRUfD+Chzosre1qJVhhFSbyl82djGdFc aO2oBUHHLq5Ti2Mn+Ew6pkraVBVlaAlsJToo2i4PRZF14ClADR3iApdZyyEHOg2CS8Q/ J0YTzJGwQExsM9dJY0txwN4T5/fWbKvFvs9KMAelf3zhzONO8y3MFXUJk7RQyBHH0aho 5dg0rUT0wAsYFkJkjF1LtuOnz9PEI6+pcemr+ub47MnuH8ZwYzWKSjdL5T31nC/sHM8X ILDyicrhDr681LQx4w1nOJ3V/DZjR/G6mC/bOrqNXEkzeYzKqxAe2J5fppQMqghnP5EO K0yA== X-Gm-Message-State: AOAM530KEobG9rKKW1UNVY8EIrMtV4dK9smeMnmnomMkXym836uFQZJv cBszSBD6k3l+ncZUp8ZtuInn5CKMatMafX/jDgCv4w== X-Google-Smtp-Source: ABdhPJyKj74J1ddH1hiillRmmKBD7LlWxLbsqjVfuO+6Ap4fYyNkTDGwujut8JG6PGSfera/Wq1mHvbDwzYH/yE5F68= X-Received: by 2002:a4a:81d2:: with SMTP id s18mr8654209oog.76.1611216689307; Thu, 21 Jan 2021 00:11:29 -0800 (PST) MIME-Version: 1.0 References: <20210120160157.3343911-1-patrick.rudolph@9elements.com> In-Reply-To: From: "Patrick Rudolph" Date: Thu, 21 Jan 2021 09:11:18 +0100 Message-ID: Subject: Re: [PATCH] UefiPayloadPkg/BlSupportDxe: Use EfiSmbiosProtocol to install tables To: "Ma, Maurice" Cc: "devel@edk2.groups.io" , "Dong, Guo" , "You, Benjamin" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 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 th= e 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 ca= n still be dispatched and the boot flow can continue. > > Some other comments: > - Please add function and parameter description for BlDxeInstallSMBIOSta= bles(). > - To follow the naming convention in EDK2, maybe use BlDxeInstallSmbi= osTables instead of BlDxeInstallSMBIOStables(). > > Thanks > Maurice > > -----Original Message----- > > From: Patrick Rudolph > > Sent: Wednesday, January 20, 2021 8:02 > > To: devel@edk2.groups.io > > Cc: Ma, Maurice ; Dong, Guo ; > > You, Benjamin > > 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, inst= ead only > > "0 MB RAM" was displayed. > > > > Tests showed that the OS can still see the SMBIOS tables. > > > > Signed-off-by: Patrick Rudolph > > --- > > 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 U= INT64 > > 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 =3D gBS->LocateProtocol (&gEfiSmbiosProt= ocolGuid, > > NULL, (VOID **)&SmbiosProto);+ if (EFI_ERROR (Status)) {+ DEBUG > > ((DEBUG_ERROR, "%a: Failed to locate gEfiSmbiosProtocolGuid\n",+ > > __FUNCTION__));+ return Status;+ }++ Smbios30Table =3D > > (SMBIOS_TABLE_3_0_ENTRY_POINT *)(UINTN)(SmbiosTableBase);+ > > SmbiosTable =3D (SMBIOS_TABLE_ENTRY_POINT *)(UINTN)(SmbiosTableBase);++ > > if (CompareMem (Smbios30Table->AnchorString, "_SM3_", 5) =3D=3D 0) {+ > > Smbios.Hdr =3D (SMBIOS_STRUCTURE *) (UINTN) Smbios30Table- > > >TableAddress;+ SmbiosEnd.Raw =3D (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->AnchorStrin= g, > > "_SM_", 4) =3D=3D 0) {+ Smbios.Hdr =3D (SMBIOS_STRUCTURE *) (UINT= N) > > SmbiosTable->TableAddress;+ SmbiosEnd.Raw =3D (UINT8 *) ((UINTN) > > SmbiosTable->TableAddress + SmbiosTable->TableLength);++ if (SmbiosT= able- > > >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 =3D=3D 127) {= + > > break;+ }++ // Install the table+ SmbiosHandle =3D > > SMBIOS_HANDLE_PI_RESERVED;+ Status =3D SmbiosProto->Add (+ > > SmbiosProto,+ gImageHandle,+ = &SmbiosHandle,+ > > Smbios.Hdr+ );+ ASSERT_EFI_ERROR (Status);+= if (EFI_ERROR > > (Status)) {+ return Status;+ }+ //+ // Go to the next SMB= IOS structure. > > Each SMBIOS structure may include 2 parts:+ // 1. Formatted section;= 2. > > Unformatted string section. So, 2 steps are needed+ // to skip one S= MBIOS > > structure.+ //++ //+ // Step 1: Skip over formatted section.+ = //+ String =3D > > (CHAR8 *) (Smbios.Raw + Smbios.Hdr->Length);++ //+ // Step 2: Ski= p over > > unformatted string section.+ //+ do {+ //+ // Each stri= ng is terminated > > with a NULL(00h) BYTE and the sets of strings+ // is terminated wi= th an > > additional NULL(00h) BYTE.+ //+ for ( ; *String !=3D 0; Strin= g++) {+ }++ if > > (*(UINT8*)++String =3D=3D 0) {+ //+ // Pointer to the nex= t SMBIOS > > structure.+ //+ Smbios.Raw =3D (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 = !=3D 0 && > > SystemTableInfo->SmbiosTableSize !=3D 0) {- DEBUG ((DEBUG_ERROR, "In= stall > > Smbios Table at 0x%lx, length 0x%x\n", SystemTableInfo->SmbiosTableBase= , > > SystemTableInfo->SmbiosTableSize));- Status =3D gBS->InstallConfigur= ationTable > > (&gEfiSmbiosTableGuid, (VOID *)(UINTN)SystemTableInfo->SmbiosTableBase)= ;- > > ASSERT_EFI_ERROR (Status);+ DEBUG ((DEBUG_ERROR, "Install Smbios Tab= le > > at 0x%lx, length 0x%x\n",+ SystemTableInfo->SmbiosTableBase, > > SystemTableInfo->SmbiosTableSize));++ if > > (BlDxeInstallSMBIOStables(SystemTableInfo->SmbiosTableBase, > > SystemTableInfo->SmbiosTableSize) !=3D EFI_SUCCESS) {+ Status =3D = gBS- > > >InstallConfigurationTable (&gEfiSmbiosTableGuid, (VOID > > *)(UINTN)SystemTableInfo->SmbiosTableBase);+ ASSERT_EFI_ERROR > > (Status);+ } } //diff --git a/UefiPayloadPkg/BlSupportDxe/BlSup= portDxe.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 +#include + #include > > #include > > #include @@ -26,5 +28,6 @@ SPDX-License- > > Identifier: BSD-2-Clause-Patent #include #in= clude > > +#include #endifd= iff - > > -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 >