Sorry for missing one comment. I think we can add below information for GUID information in SmbiosDxe.inf:
gPldSmbios3TableGuid ## CONSUMES ## HOB gPldSmbiosTableGuid ## SOMETIMES_CONSUMES ## HOB Best Regards, Hao Wu > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Wu, Hao > A > Sent: Wednesday, May 26, 2021 2:15 PM > To: Liu, Zhiguang <zhiguang....@intel.com>; Ni, Ray <ray...@intel.com>; > devel@edk2.groups.io > Cc: Wang, Jian J <jian.j.w...@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: Re: [edk2-devel] [PATCH 5/9] > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables > > 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 (#75597): https://edk2.groups.io/g/devel/message/75597 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] -=-=-=-=-=-=-=-=-=-=-=-