Re: [Patch V3 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

["Wu, Hao A" <hao.a.wu@intel.com>]

> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Friday, June 4, 2021 5:42 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>; Patrick Rudolph
> <patrick.rudolph@9elements.com>
> Subject: [Patch V3 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for
> existing tables
> 
> V1:
> The default EfiSmbiosProtocol operates on an empty SMBIOS table.
> The SMBIOS tables are provided by the bootloader on UefiPayloadPkg.
> Scan for existing tables in SmbiosDxe and load them if they seem valid.
> 
> 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.
> 
> V2:
> SmbiosDxe will get the SMBIOS from a guid Hob.
> Aslo will keep the SmbiosHandle if it is available.


Aslo -> Also


> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 320
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++--
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h   |   4 +++-
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf |   5 ++++-
>  3 files changed, 325 insertions(+), 4 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> index 3cdb0b1ed7..3579c4d890 100644
> --- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> +++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> @@ -2,7 +2,7 @@
>    This code produces the Smbios protocol. It also responsible for constructing
> 
>    SMBIOS table into system table.
> 
> 
> 
> -Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -148,6 +148,31 @@ SMBIOS_TABLE_3_0_ENTRY_POINT
> Smbios30EntryPointStructureData = {
>    //
> 
>    0
> 
>  };
> 
> +
> 
> +/**
> 
> +  Validates a SMBIOS table entry point.
> 
> +
> 
> +  @param  TableEntry       The SmBios table entry to validate.
> 
> +  @param  TableAddress     On exit, point to the smbios table addres.
> 
> +  @param  TableMaximumSize On exit, point to the maximum size of the
> table.
> 
> +
> 
> +  @retval TRUE           SMBIOS table entry point is valid.
> 
> +  @retval FALSE          SMBIOS table entry point is malformed.
> 
> +
> 
> +**/
> 
> +typedef
> 
> +BOOLEAN
> 
> +(* IS_SMBIOS_TABLE_VALID) (
> 
> +  IN  VOID               *TableEntry,
> 
> +  OUT VOID               **TableAddress,
> 
> +  OUT UINTN              *TableMaximumSize
> 
> +  );
> 
> +typedef struct {
> 
> +  EFI_GUID               *Guid;
> 
> +  IS_SMBIOS_TABLE_VALID  IsValid;
> 
> +} IS_SMBIOS_TABLE_VALID_ENTRY;
> 
> +
> 
> +


Could you help to put the above definitions into file "SmbiosDxe.h"?


> 
>  /**
> 
> 
> 
>    Get the full size of SMBIOS structure including optional strings that follow
> the formatted structure.
> 
> @@ -1408,6 +1433,296 @@ SmbiosTableConstruction (
>    }
> 
>  }
> 
> 
> 
> +/**
> 
> +  Validates a SMBIOS 2.0 table entry point.
> 
> +
> 
> +  @param  TableEntry       The SmBios table entry to validate.
> 
> +  @param  TableAddress     On exit, point to the smbios table addres.
> 
> +  @param  TableMaximumSize On exit, point to the maximum size of the
> table.
> 
> +
> 
> +  @retval TRUE           SMBIOS table entry point is valid.
> 
> +  @retval FALSE          SMBIOS table entry point is malformed.
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +BOOLEAN
> 
> +IsValidSmbios20Table (
> 
> +  IN  VOID               *TableEntry,
> 
> +  OUT VOID               **TableAddress,
> 
> +  OUT UINTN              *TableMaximumSize
> 
> +  )
> 
> +{
> 
> +  UINT8                                      Checksum;
> 
> +  SMBIOS_TABLE_ENTRY_POINT                   *SmbiosTable;
> 
> +  SmbiosTable = (SMBIOS_TABLE_ENTRY_POINT *) TableEntry;
> 
> +
> 
> +  if (CompareMem (SmbiosTable->AnchorString, "_SM_", 4) != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  if (CompareMem (SmbiosTable->IntermediateAnchorString, "_DMI_",
> 5) != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // The actual value of the EntryPointLength should be 1Fh.
> 
> +  // However, it was incorrectly stated in version 2.1 of smbios specification.
> 
> +  // Therefore, 0x1F and 0x1E are both accepted.
> 
> +  //
> 
> +  if (SmbiosTable->EntryPointLength != 0x1E && SmbiosTable-
> >EntryPointLength != sizeof (SMBIOS_TABLE_ENTRY_POINT)) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // MajorVersion should not be less than 2.
> 
> +  //
> 
> +  if (SmbiosTable->MajorVersion < 2) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // The whole struct check sum should be zero
> 
> +  //
> 
> +  Checksum = CalculateSum8 (
> 
> +               (UINT8 *) SmbiosTable,
> 
> +               SmbiosTable->EntryPointLength
> 
> +               );
> 
> +  if (Checksum != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // The Intermediate Entry Point Structure check sum should be zero.
> 
> +  //
> 
> +  Checksum = CalculateSum8 (
> 
> +               (UINT8 *) SmbiosTable + OFFSET_OF
> (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString),
> 
> +               SmbiosTable->EntryPointLength - OFFSET_OF
> (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString)
> 
> +               );
> 
> +  if (Checksum != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  *TableAddress     = (VOID *) (UINTN) SmbiosTable->TableAddress;
> 
> +  *TableMaximumSize = SmbiosTable->TableLength;
> 
> +  return TRUE;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Validates a SMBIOS 3.0 table entry point.
> 
> +
> 
> +  @param  TableEntry       The SmBios table entry to validate.
> 
> +  @param  TableAddress     On exit, point to the smbios table addres.
> 
> +  @param  TableMaximumSize On exit, point to the maximum size of the
> table.
> 
> +
> 
> +  @retval TRUE           SMBIOS table entry point is valid.
> 
> +  @retval FALSE          SMBIOS table entry point is malformed.
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +BOOLEAN
> 
> +IsValidSmbios30Table (
> 
> +  IN  VOID               *TableEntry,
> 
> +  OUT VOID               **TableAddress,
> 
> +  OUT UINTN              *TableMaximumSize
> 
> +  )
> 
> +{
> 
> +  UINT8                          Checksum;
> 
> +  SMBIOS_TABLE_3_0_ENTRY_POINT   *SmbiosTable;
> 
> +  SmbiosTable = (SMBIOS_TABLE_3_0_ENTRY_POINT *) TableEntry;
> 
> +
> 
> +  if (CompareMem (SmbiosTable->AnchorString, "_SM3_", 5) != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (SmbiosTable->EntryPointLength < sizeof
> (SMBIOS_TABLE_3_0_ENTRY_POINT)) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (SmbiosTable->MajorVersion < 3) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  //
> 
> +  // The whole struct check sum should be zero
> 
> +  //
> 
> +  Checksum = CalculateSum8 (
> 
> +               (UINT8 *) SmbiosTable,
> 
> +               SmbiosTable->EntryPointLength
> 
> +               );
> 
> +  if (Checksum != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  *TableAddress     = (VOID *) (UINTN) SmbiosTable->TableAddress;
> 
> +  *TableMaximumSize = SmbiosTable->TableMaximumSize;
> 
> +  return TRUE;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Parse an existing SMBIOS table and insert it using SmbiosAdd.
> 
> +
> 
> +  @param  ImageHandle           The EFI_HANDLE to this driver.
> 
> +  @param  Smbios                The SMBIOS table to parse.
> 
> +  @param  Length                The length of the SMBIOS table.
> 
> +
> 
> +  @retval EFI_SUCCESS           SMBIOS table was parsed and installed.
> 
> +  @retval EFI_OUT_OF_RESOURCES  Record was not added due to lack of
> system resources.
> 
> +  @retval EFI_INVALID_PARAMETER Smbios is not a correct smbios table
> 
> +
> 
> +**/
> 
> +STATIC
> 
> +EFI_STATUS
> 
> +ParseAndAddExistingSmbiosTable (
> 
> +  IN EFI_HANDLE                    ImageHandle,
> 
> +  IN SMBIOS_STRUCTURE_POINTER      Smbios,
> 
> +  IN UINTN                         Length
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS                    Status;
> 
> +  CHAR8                         *String;
> 
> +  EFI_SMBIOS_HANDLE             SmbiosHandle;
> 
> +  SMBIOS_STRUCTURE_POINTER      SmbiosEnd;
> 
> +
> 
> +  SmbiosEnd.Raw = Smbios.Raw + Length;
> 
> +
> 
> +  if (Smbios.Raw >= SmbiosEnd.Raw || Smbios.Raw == NULL) {
> 
> +    return EFI_INVALID_PARAMETER;
> 
> +  }
> 
> +
> 
> +  do {
> 
> +    //
> 
> +    // Make sure not to access memory beyond SmbiosEnd
> 
> +    //
> 
> +    if (Smbios.Raw + sizeof (SMBIOS_STRUCTURE) > SmbiosEnd.Raw ||
> 
> +      Smbios.Raw + sizeof (SMBIOS_STRUCTURE) < Smbios.Raw) {
> 
> +      return EFI_INVALID_PARAMETER;
> 
> +    }
> 
> +    //
> 
> +    // Check for end marker
> 
> +    //
> 
> +    if (Smbios.Hdr->Type == SMBIOS_TYPE_END_OF_TABLE) {
> 
> +      break;
> 
> +    }
> 
> +    //
> 
> +    // Make sure not to access memory beyond SmbiosEnd
> 
> +    // Each structure shall be terminated by a double-null (0000h).
> 
> +    //
> 
> +    if (Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) >
> SmbiosEnd.Raw ||
> 
> +      Smbios.Raw + Smbios.Hdr->Length + 2 * sizeof (UINT8) < Smbios.Raw) {
> 
> +      return EFI_INVALID_PARAMETER;
> 
> +    }
> 
> +    //
> 
> +    // Install the table
> 
> +    //
> 
> +    SmbiosHandle = Smbios.Hdr->Handle;
> 
> +    Status = SmbiosAdd (
> 
> +               &mPrivateData.Smbios,
> 
> +               ImageHandle,
> 
> +               &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 ((UINTN) String >= (UINTN) SmbiosEnd.Raw - sizeof (UINT8)) {
> 
> +          return EFI_INVALID_PARAMETER;
> 
> +        }
> 
> +      }
> 
> +
> 
> +      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;
> 
> +}
> 
> +
> 
> +
> 
> +IS_SMBIOS_TABLE_VALID_ENTRY mIsSmbiosTableValid[] = {
> 
> +  {&gPldSmbios3TableGuid, IsValidSmbios30Table },
> 
> +  {&gPldSmbiosTableGuid,  IsValidSmbios20Table }
> 
> +};


Could you help to put this global variable to the beginning of this file?

With the above 3 format comments handled:
Reviewed-by: Hao A Wu <hao.a.wu@intel.com>

Best Regards,
Hao Wu


> 
> +
> 
> +/**
> 
> +  Retrieve SMBIOS from Hob.
> 
> +  @param ImageHandle     Module's image handle
> 
> +
> 
> +  @retval EFI_SUCCESS    Smbios from Hob is installed.
> 
> +  @return EFI_NOT_FOUND  Not found Smbios from Hob.
> 
> +  @retval Other          No Smbios from Hob is installed.
> 
> +
> 
> +**/
> 
> +EFI_STATUS
> 
> +RetrieveSmbiosFromHob (
> 
> +  IN EFI_HANDLE           ImageHandle
> 
> +  )
> 
> +{
> 
> +  EFI_STATUS                    Status;
> 
> +  UINTN                         Index;
> 
> +  SMBIOS_STRUCTURE_POINTER      Smbios;
> 
> +  EFI_HOB_GUID_TYPE             *GuidHob;
> 
> +  PLD_SMBIOS_TABLE              *SmBiosTableAdress;
> 
> +  PLD_GENERIC_HEADER            *GenericHeader;
> 
> +  VOID                          *TableAddress;
> 
> +  UINTN                         TableMaximumSize;
> 
> +
> 
> +  Status = EFI_NOT_FOUND;
> 
> +
> 
> +  for (Index = 0; Index < ARRAY_SIZE (mIsSmbiosTableValid); Index++) {
> 
> +    GuidHob = GetFirstGuidHob (mIsSmbiosTableValid[Index].Guid);
> 
> +    if (GuidHob == NULL) {
> 
> +      continue;
> 
> +    }
> 
> +    GenericHeader = (PLD_GENERIC_HEADER *) GET_GUID_HOB_DATA
> (GuidHob);
> 
> +    if ((sizeof (PLD_GENERIC_HEADER) <= GET_GUID_HOB_DATA_SIZE
> (GuidHob)) && (GenericHeader->Length <= GET_GUID_HOB_DATA_SIZE
> (GuidHob))) {
> 
> +      if (GenericHeader->Revision == PLD_SMBIOS_TABLE_REVISION) {
> 
> +        //
> 
> +        // PLD_SMBIOS_TABLE structure is used when Revision equals to
> PLD_SMBIOS_TABLE_REVISION
> 
> +        //
> 
> +        SmBiosTableAdress = (PLD_SMBIOS_TABLE *) GET_GUID_HOB_DATA
> (GuidHob);
> 
> +        if (GenericHeader->Length >= PLD_SIZEOF_THROUGH_FIELD
> (PLD_SMBIOS_TABLE, SmBiosEntryPoint)) {
> 
> +          if (mIsSmbiosTableValid[Index].IsValid ((VOID *)
> (UINTN )SmBiosTableAdress->SmBiosEntryPoint, &TableAddress,
> &TableMaximumSize)) {
> 
> +            Smbios.Raw = TableAddress;
> 
> +            Status = ParseAndAddExistingSmbiosTable (ImageHandle, Smbios,
> TableMaximumSize);
> 
> +            if (EFI_ERROR (Status)) {
> 
> +              DEBUG ((DEBUG_ERROR, "RetrieveSmbiosFromHob: Failed to parse
> preinstalled tables from gPldSmbios3TableGuid Guid Hob\n"));
> 
> +              Status = EFI_UNSUPPORTED;
> 
> +            } else {
> 
> +              return EFI_SUCCESS;
> 
> +            }
> 
> +          }
> 
> +        }
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +  return Status;
> 
> +}
> 
> +
> 
>  /**
> 
> 
> 
>    Driver to produce Smbios protocol and pre-allocate 1 page for the final
> SMBIOS table.
> 
> @@ -1451,5 +1766,6 @@ SmbiosDriverEntryPoint (
>                    &mPrivateData.Smbios
> 
>                    );
> 
> 
> 
> -  return Status;
> 
> +  RetrieveSmbiosFromHob (ImageHandle);
> 
> +  return EFI_SUCCESS;
> 
>  }
> 
> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
> b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
> index f97c85ae40..a260cf695e 100644
> --- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
> +++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h
> @@ -1,7 +1,7 @@
>  /** @file
> 
>    This code supports the implementation of the Smbios protocol
> 
> 
> 
> -Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -24,6 +24,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/MemoryAllocationLib.h>
> 
>  #include <Library/UefiBootServicesTableLib.h>
> 
>  #include <Library/PcdLib.h>
> 
> +#include <Library/HobLib.h>
> 
> +#include <UniversalPayload/SmbiosTable.h>
> 
> 
> 
>  #define SMBIOS_INSTANCE_SIGNATURE SIGNATURE_32 ('S', 'B', 'i', 's')
> 
>  typedef struct {
> 
> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
> b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
> index f6c036e1dc..63f468936d 100644
> --- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
> +++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf
> @@ -1,7 +1,7 @@
>  ## @file
> 
>  # This driver initializes and installs the SMBIOS protocol, constructs SMBIOS
> table into system configuration table.
> 
>  #
> 
> -# Copyright (c) 2009 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +# Copyright (c) 2009 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #
> 
> @@ -41,6 +41,7 @@
>    UefiDriverEntryPoint
> 
>    DebugLib
> 
>    PcdLib
> 
> +  HobLib
> 
> 
> 
>  [Protocols]
> 
>    gEfiSmbiosProtocolGuid                            ## PRODUCES
> 
> @@ -48,6 +49,8 @@
>  [Guids]
> 
>    gEfiSmbiosTableGuid                               ## SOMETIMES_PRODUCES ##
> SystemTable
> 
>    gEfiSmbios3TableGuid                              ## SOMETIMES_PRODUCES ##
> SystemTable
> 
> +  gPldSmbios3TableGuid                              ## CONSUMES           ## HOB
> 
> +  gPldSmbiosTableGuid                               ## SOMETIMES_CONSUMES ## HOB
> 
> 
> 
>  [Pcd]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion   ## CONSUMES
> 
> --
> 2.30.0.windows.2