Adding Ray.
Hello Ray, I saw most of your comments in previous patch:
"[PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables"
have been addressed in this patch series.
Could you help to check if there are additional comments for this patch? Thanks.
Some inline comments below:
> -----Original Message-----
> From: Liu, Zhiguang <zhiguang....@intel.com>
> Sent: Monday, May 24, 2021 3:13 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.w...@intel.com>; Wu, Hao A <hao.a...@intel.com>;
> Bi, Dandan <dandan...@intel.com>; Zeng, Star <star.z...@intel.com>; Gao,
> Zhichao <zhichao....@intel.com>; Patrick Rudolph
> <patrick.rudo...@9elements.com>
> Subject: [PATCH 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.
>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Hao A Wu <hao.a...@intel.com>
> Cc: Dandan Bi <dandan...@intel.com>
> Cc: Star Zeng <star.z...@intel.com>
> Cc: Zhichao Gao <zhichao....@intel.com>
> Signed-off-by: Patrick Rudolph <patrick.rudo...@9elements.com>
> Signed-off-by: Zhiguang Liu <zhiguang....@intel.com>
> ---
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c | 299
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++--
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h | 4 +++-
> MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf | 5 ++++-
> 3 files changed, 304 insertions(+), 4 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> index 3cdb0b1ed7..d2aa15d43f 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
>
>
>
> **/
>
> @@ -1408,6 +1408,300 @@ SmbiosTableConstruction (
> }
>
> }
>
>
>
> +/**
>
> + Validates a SMBIOS 2.0 table entry point.
>
> +
>
> + @param SmbiosTable The SMBIOS_TABLE_ENTRY_POINT to validate.
>
> +
>
> + @retval TRUE SMBIOS table entry point is valid.
>
> + @retval FALSE SMBIOS table entry point is malformed.
>
> +
>
> +**/
>
> +STATIC
>
> +BOOLEAN
>
> +IsValidSmbios20Table (
>
> + IN SMBIOS_TABLE_ENTRY_POINT *SmbiosTable
>
> + )
>
> +{
>
> + UINT8 Checksum;
>
> +
>
> + if (CompareMem (SmbiosTable->AnchorString, "_SM_", 4) != 0) {
>
> + return FALSE;
>
> + }
Should a check for the 'IntermediateAnchorString' be added here as well?
>
> +
>
> + //
>
> + // 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 be 2.
>
> + //
>
> + if (SmbiosTable->MajorVersion != 2) {
>
> + return FALSE;
I might be wrong on this.
My take with the SMBIOS spec is that a 2.0 table is allowed to has this field
greater than 2.
As long as a specific version of the SMBIOS spec still support this format
(which is still true for version 3.0).
So I think I check like:
if (EntryPointStructure->MajorVersion < 2) {...}
should be used here.
>
> + }
>
> +
>
> + //
>
> + // 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)
>
> + );
>
> + return (BOOLEAN) (Checksum == 0);
>
> +}
>
> +
>
> +/**
>
> + Validates a SMBIOS 3.0 table entry point.
>
> +
>
> + @param SmbiosTable The SMBIOS_TABLE_3_0_ENTRY_POINT to validate.
>
> +
>
> + @retval TRUE SMBIOS table entry point is valid.
>
> + @retval FALSE SMBIOS table entry point is malformed.
>
> +
>
> +**/
>
> +STATIC
>
> +BOOLEAN
>
> +IsValidSmbios30Table (
>
> + IN SMBIOS_TABLE_3_0_ENTRY_POINT *SmbiosTable
>
> + )
>
> +{
>
> + UINT8 Checksum;
>
> +
>
> + 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;
>
> + }
>
> + 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;
>
> +}
>
> +
>
> +
>
> +/**
>
> + 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
>
> +EFIAPI
I think RetrieveSmbiosFromHob() is an internal function.
Please help to remove the 'EFIAPI' keyword here.
>
> +RetrieveSmbiosFromHob (
>
> + IN EFI_HANDLE ImageHandle
>
> + )
>
> +{
>
> + EFI_STATUS Status;
>
> + SMBIOS_TABLE_ENTRY_POINT *SmbiosTable;
>
> + SMBIOS_TABLE_3_0_ENTRY_POINT *Smbios30Table;
>
> + SMBIOS_STRUCTURE_POINTER Smbios;
>
> + EFI_HOB_GUID_TYPE *GuidHob;
>
> + PLD_SMBIOS_TABLE *SmBiosTableAdress;
>
> + PLD_GENERIC_HEADER *GenericHeader;
>
> +
>
> + Status = EFI_NOT_FOUND;
>
> + //
>
> + // Scan for existing SMBIOS tables from gPldSmbios3TableGuid Guid Hob
>
> + //
>
> + GuidHob = GetFirstGuidHob (&gPldSmbios3TableGuid);
>
> + if (GuidHob != NULL) {
>
> + 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)) {
>
> + Smbios30Table = (SMBIOS_TABLE_3_0_ENTRY_POINT *) (UINTN)
> + SmBiosTableAdress->SmBiosEntryPoint;
>
> + if (IsValidSmbios30Table (Smbios30Table)) {
>
> + Smbios.Raw = (UINT8 *) (UINTN) Smbios30Table->TableAddress;
>
> + Status = ParseAndAddExistingSmbiosTable (ImageHandle,
> + Smbios, Smbios30Table->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;
>
> + }
>
> + }
>
> + }
>
> + } else {
>
> + Status = EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> + }
>
> +
>
> + //
>
> + // Scan for existing SMBIOS tables from gPldSmbiosTableGuid Guid Hob,
>
> + // if gPldSmbios3TableGuid Hob doesn't exist or parsing
> + gPldSmbios3TableGuid failed
>
> + //
>
> + GuidHob = GetFirstGuidHob (&gPldSmbiosTableGuid);
>
> +
>
> + if (GuidHob != NULL) {
>
> + 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)) {
>
> + SmbiosTable = (SMBIOS_TABLE_ENTRY_POINT *) (UINTN)
> + SmBiosTableAdress->SmBiosEntryPoint;
>
> + if (IsValidSmbios20Table (SmbiosTable)) {
>
> + Smbios.Raw = (UINT8 *) (UINTN) SmbiosTable->TableAddress;
>
> + Status = ParseAndAddExistingSmbiosTable (ImageHandle,
> + Smbios, SmbiosTable->TableLength);
>
> + if (EFI_ERROR (Status)) {
>
> + DEBUG ((DEBUG_ERROR, "RetrieveSmbiosFromHob: Failed to
> + parse preinstalled tables from gPldSmbiosTableGuid Guid Hob\n"));
>
> + Status = EFI_UNSUPPORTED;
>
> + }
>
> + return EFI_SUCCESS;
>
> + }
>
> + }
>
> + } else {
>
> + Status = EFI_UNSUPPORTED;
>
> + }
>
> + }
>
> + }
Is it possible to abstract the codes that:
a) Search & parse of gPldSmbios3TableGuid
b) Search & parse of gPldSmbiosTableGuid
into a function?
I found that most of the logic is identical.
Best Regards,
Hao Wu
>
> + return Status;
>
> +}
>
> +
>
> /**
>
>
>
> Driver to produce Smbios protocol and pre-allocate 1 page for the final
> SMBIOS table.
>
> @@ -1451,5 +1745,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..3286575098 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
>
> + gPldSmbiosTableGuid
>
>
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdSmbiosVersion ## CONSUMES
>
> --
> 2.30.0.windows.2
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75595): https://edk2.groups.io/g/devel/message/75595
Mute This Topic: https://groups.io/mt/83045509/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
