[PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Patrick Rudolph]

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.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
---
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c | 223 +++++++++++++++++++-
 1 file changed, 221 insertions(+), 2 deletions(-)

diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
index 3cdb0b1ed7..958a249cf9 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -1408,6 +1408,177 @@ SmbiosTableConstruction (
   }
 }
 
+/**
+  Validates a SMBIOS 2.0 table entry point.
+
+  @param  EntryPointStructure   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
+ValidateSmbios20Table(
+  IN SMBIOS_TABLE_ENTRY_POINT      *EntryPointStructure
+) {
+  UINT8 Checksum;
+
+  if (CompareMem (EntryPointStructure->AnchorString, "_SM_", 4) != 0) {
+    return FALSE;
+  }
+  if (EntryPointStructure->EntryPointLength < 0x1E) {
+    return FALSE;
+  }
+  if (EntryPointStructure->MajorVersion < 2) {
+    return FALSE;
+  }
+  if (EntryPointStructure->SmbiosBcdRevision > 0 &&
+    (EntryPointStructure->SmbiosBcdRevision >> 4) < 2) {
+    return FALSE;
+  }
+  if (EntryPointStructure->TableLength == 0) {
+    return FALSE;
+  }
+  if (EntryPointStructure->TableAddress == 0 ||
+    EntryPointStructure->TableAddress == ~0) {
+    return FALSE;
+  }
+
+  Checksum = CalculateSum8((UINT8 *) EntryPointStructure,
+    EntryPointStructure->EntryPointLength);
+  if (Checksum != 0) {
+    return FALSE;
+  }
+
+  Checksum = CalculateSum8((UINT8 *) EntryPointStructure + 0x10,
+    EntryPointStructure->EntryPointLength - 0x10);
+  if (Checksum != 0) {
+    return FALSE;
+  }
+  return TRUE;
+}
+
+/**
+  Validates a SMBIOS 3.0 table entry point.
+
+  @param  Smbios30EntryPointStructure   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
+ValidateSmbios30Table(
+  IN SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30EntryPointStructure
+) {
+  UINT8 Checksum;
+
+  if (CompareMem (Smbios30EntryPointStructure->AnchorString, "_SM3_", 5) != 0) {
+    return FALSE;
+  }
+  if (Smbios30EntryPointStructure->EntryPointLength < 0x18) {
+    return FALSE;
+  }
+  if (Smbios30EntryPointStructure->MajorVersion < 3) {
+    return FALSE;
+  }
+  if (Smbios30EntryPointStructure->TableMaximumSize == 0) {
+    return FALSE;
+  }
+  if (Smbios30EntryPointStructure->TableAddress == 0 ||
+    Smbios30EntryPointStructure->TableAddress == ~0) {
+    return FALSE;
+  }
+
+  Checksum = CalculateSum8((UINT8 *) Smbios30EntryPointStructure,
+    Smbios30EntryPointStructure->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.
+
+**/
+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;
+
+  do {
+    // Check for end marker
+    if (Smbios.Hdr->Type == 127) {
+      break;
+    }
+
+    // Install the table
+    SmbiosHandle = SMBIOS_HANDLE_PI_RESERVED;
+    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 (*(UINT8*)++String == 0) {
+        //
+        // Pointer to the next SMBIOS structure.
+        //
+        Smbios.Raw = (UINT8 *)++String;
+        break;
+      }
+    } while (TRUE);
+  } while (Smbios.Raw < SmbiosEnd.Raw);
+
+  return EFI_SUCCESS;
+}
+
 /**
 
   Driver to produce Smbios protocol and pre-allocate 1 page for the final SMBIOS table.
@@ -1426,7 +1597,10 @@ SmbiosDriverEntryPoint (
   IN EFI_SYSTEM_TABLE     *SystemTable
   )
 {
-  EFI_STATUS            Status;
+  EFI_STATUS                    Status;
+  SMBIOS_TABLE_ENTRY_POINT      *SmbiosTable;
+  SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30Table;
+  SMBIOS_STRUCTURE_POINTER      Smbios;
 
   mPrivateData.Signature                = SMBIOS_INSTANCE_SIGNATURE;
   mPrivateData.Smbios.Add               = SmbiosAdd;
@@ -1450,6 +1624,51 @@ SmbiosDriverEntryPoint (
                   EFI_NATIVE_INTERFACE,
                   &mPrivateData.Smbios
                   );
+  //
+  // Scan for existing SMBIOS tables installed by bootloader
+  //
+  Status = EfiGetSystemConfigurationTable (
+               &gEfiSmbios3TableGuid,
+               (VOID **) &Smbios30Table
+               );
+  if (!EFI_ERROR (Status) && ValidateSmbios30Table(Smbios30Table)) {
+    Smbios.Raw = AllocatePool(Smbios30Table->TableMaximumSize);
+    if (!Smbios.Raw) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+    //
+    // Backup old table in case it gets overwritten while parsing it
+    //
+    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table->TableMaximumSize);
+    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios, Smbios30Table->TableMaximumSize);
+    FreePool(Smbios.Raw);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse preinstalled tables\n"));
+      Status = EFI_SUCCESS;
+    }
+  }
+
+  Status = EfiGetSystemConfigurationTable (
+               &gEfiSmbiosTableGuid,
+               (VOID **) &SmbiosTable
+               );
+  if (!EFI_ERROR (Status) && ValidateSmbios20Table(SmbiosTable)) {
+    Smbios.Raw = AllocatePool(SmbiosTable->TableLength);
+    if (!Smbios.Raw) {
+      return EFI_OUT_OF_RESOURCES;
+    }
+    //
+    // Backup old table in case it gets overwritten while parsing it
+    //
+    CopyMem (Smbios.Raw, (VOID *)(UINTN)SmbiosTable->TableAddress, SmbiosTable->TableLength);
 
-  return Status;
+    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios, SmbiosTable->TableLength);
+    FreePool(Smbios.Raw);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse preinstalled tables\n"));
+      Status = EFI_SUCCESS;
+    }
+  }
+
+  return EFI_SUCCESS;
 }
-- 
2.26.2

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Ni, Ray]

In general, I agree this solution that lets SMBIOS driver directly absorbs the SMBIOS table from PEI.
This can eliminate the needs of a separate driver that consumes the HOB and calls SMBIOS protocol to add the SMBIOS structures.

There are two options for the HOB design:
1. A single HOB that points to the SMBIOS table.
2. Multiple HOBs that each points to a SMBIOS structure.

In my opinion, option #2 is more flexible because it doesn't require the bootloader to consolidate all the SMBIOS structures together.
The CPU module in the bootloader can produce the type 4 and 7 structures. 
The PCI module in the bootloader can produce the type 9 structures.

But, I am not sure if option #2 is conflict with what coreboot does. Does coreboot produce the whole SMBIOS table in a single buffer?
Option #2 also doesn't care whether it's a SMBIOS 3.0 table or 2.x table. 

>+  Status = EfiGetSystemConfigurationTable (

1. Why don't you directly get the data from HOB list? This can eliminate the code in BlSupportDxe that gets data in HOB and publishes to
configuration table.
    
> +ValidateSmbios20Table(
> +ValidateSmbios30Table(

2. I will defer to experts (Dandan, Star and Zhichao) to review whether the above two functions are implemented properly.

> 
> +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;
> +
> +  do {
> +    // Check for end marker
> +    if (Smbios.Hdr->Type == 127) {

3. Please use SMBIOS_TYPE_END_OF_TABLE instead of hardcode 127.

> 
> +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> >TableMaximumSize);

4. Should we copy from Smbios30Table->TableAddress instead of Smbios30Table?

> 
> +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> Smbios30Table->TableMaximumSize);

5. Can you explain in specific why SMBIOS table should be duplicated before parsing?
 

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Patrick Rudolph]

Hi Ray,
thanks for your feedback.

Currently a single HOB containing all the SMBIOS table is exported by coreboot.
As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a solution.

I'll look into passing a HOB instead of using
EfiGetSystemConfigurationTable and see if I can get rid of the table
shadow copy.

Regards,
Patrick Rudolph

On Wed, Mar 3, 2021 at 9:13 AM Ni, Ray <ray.ni@intel.com> wrote:
>
> In general, I agree this solution that lets SMBIOS driver directly absorbs the SMBIOS table from PEI.
> This can eliminate the needs of a separate driver that consumes the HOB and calls SMBIOS protocol to add the SMBIOS structures.
>
> There are two options for the HOB design:
> 1. A single HOB that points to the SMBIOS table.
> 2. Multiple HOBs that each points to a SMBIOS structure.
>
> In my opinion, option #2 is more flexible because it doesn't require the bootloader to consolidate all the SMBIOS structures together.
> The CPU module in the bootloader can produce the type 4 and 7 structures.
> The PCI module in the bootloader can produce the type 9 structures.
>
> But, I am not sure if option #2 is conflict with what coreboot does. Does coreboot produce the whole SMBIOS table in a single buffer?
> Option #2 also doesn't care whether it's a SMBIOS 3.0 table or 2.x table.
>
> >+  Status = EfiGetSystemConfigurationTable (
>
> 1. Why don't you directly get the data from HOB list? This can eliminate the code in BlSupportDxe that gets data in HOB and publishes to
> configuration table.
>
> > +ValidateSmbios20Table(
> > +ValidateSmbios30Table(
>
> 2. I will defer to experts (Dandan, Star and Zhichao) to review whether the above two functions are implemented properly.
>
> >
> > +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;
> > +
> > +  do {
> > +    // Check for end marker
> > +    if (Smbios.Hdr->Type == 127) {
>
> 3. Please use SMBIOS_TYPE_END_OF_TABLE instead of hardcode 127.
>
> >
> > +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> > >TableMaximumSize);
>
> 4. Should we copy from Smbios30Table->TableAddress instead of Smbios30Table?
>
> >
> > +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> > Smbios30Table->TableMaximumSize);
>
> 5. Can you explain in specific why SMBIOS table should be duplicated before parsing?
>
>

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Ni, Ray]

I have 5 more comments embedded, can you read and reply?

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Wednesday, March 3, 2021 4:38 PM
> To: Ni, Ray <ray.ni@intel.com>
> Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> Benjamin <benjamin.you@intel.com>;
> philipp.deppenwiese@9elements.com; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH - resend]
> MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> 
> Hi Ray,
> thanks for your feedback.
> 
> Currently a single HOB containing all the SMBIOS table is exported by
> coreboot.
> As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> solution.
> 
> I'll look into passing a HOB instead of using
> EfiGetSystemConfigurationTable and see if I can get rid of the table
> shadow copy.
> 
> Regards,
> Patrick Rudolph
> 
> On Wed, Mar 3, 2021 at 9:13 AM Ni, Ray <ray.ni@intel.com> wrote:
> >
> > In general, I agree this solution that lets SMBIOS driver directly absorbs the
> SMBIOS table from PEI.
> > This can eliminate the needs of a separate driver that consumes the HOB
> and calls SMBIOS protocol to add the SMBIOS structures.
> >
> > There are two options for the HOB design:
> > 1. A single HOB that points to the SMBIOS table.
> > 2. Multiple HOBs that each points to a SMBIOS structure.
> >
> > In my opinion, option #2 is more flexible because it doesn't require the
> bootloader to consolidate all the SMBIOS structures together.
> > The CPU module in the bootloader can produce the type 4 and 7 structures.
> > The PCI module in the bootloader can produce the type 9 structures.
> >
> > But, I am not sure if option #2 is conflict with what coreboot does. Does
> coreboot produce the whole SMBIOS table in a single buffer?
> > Option #2 also doesn't care whether it's a SMBIOS 3.0 table or 2.x table.
> >
> > >+  Status = EfiGetSystemConfigurationTable (
> >
> > 1. Why don't you directly get the data from HOB list? This can eliminate the
> code in BlSupportDxe that gets data in HOB and publishes to
> > configuration table.
> >
> > > +ValidateSmbios20Table(
> > > +ValidateSmbios30Table(
> >
> > 2. I will defer to experts (Dandan, Star and Zhichao) to review whether the
> above two functions are implemented properly.
> >
> > >
> > > +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;
> > > +
> > > +  do {
> > > +    // Check for end marker
> > > +    if (Smbios.Hdr->Type == 127) {
> >
> > 3. Please use SMBIOS_TYPE_END_OF_TABLE instead of hardcode 127.
> >
> > >
> > > +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> > > >TableMaximumSize);
> >
> > 4. Should we copy from Smbios30Table->TableAddress instead of
> Smbios30Table?
> >
> > >
> > > +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> > > Smbios30Table->TableMaximumSize);
> >
> > 5. Can you explain in specific why SMBIOS table should be duplicated
> before parsing?
> >
> >

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Ni, Ray]

> 
> Hi Ray,
> thanks for your feedback.
> 
> Currently a single HOB containing all the SMBIOS table is exported by
> coreboot.
> As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> solution.

Hi Patrick,
I checked the code in deep.
The HOB is not created by coreboot. It's the PayloadEntry that creates the HOB.
Can we update PayloadEntry to create multiple HOBs?

Guo,
Any comments?

The reason I like this approach is it doesn't require the other bootloaders to write
a SMBIOS driver that merges all SMBIOS structures together into one table.

Thanks,
Ray

Re: [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Guo Dong]

Hi SmbiosDxe maintainers,

Do you have any comments on this patch?

Maybe it is a more clean way to check if SMBIOS table HOB exists in its entry point instead of checking GetSystemConfigurationTable.
If the HOB exists, just add the SMBIOS records. And maybe we could skip so many sanity checks.
We could define a SMBIOS table HOB as below.
///
/// SMBIOS table hob
///
typedef struct {
EFI_HOB_GUID_TYPE Header;
UINT64                         TableAddress;
} SMBIOS_TABLE_HOB;

The HOB guid could be gEfiSmbiosTableGuid or gEfiSmbios3TableGuid based on the records.
UEFI payload (or bootloader) could produce this HOB based on information from bootloader.

Thanks,
Guo

> -----Original Message-----
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> Sent: Monday, March 1, 2021 7:32 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>;
> Gao, Zhichao <zhichao.gao@intel.com>; You, Benjamin
> <benjamin.you@intel.com>; philipp.deppenwiese@9elements.com; Ma,
> Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> Subject: [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for
> existing tables
> 
> 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.
> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> ---
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c | 223
> +++++++++++++++++++-
>  1 file changed, 221 insertions(+), 2 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> index 3cdb0b1ed7..958a249cf9 100644
> --- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> +++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
> @@ -1408,6 +1408,177 @@ SmbiosTableConstruction (
>    }
> 
>  }
> 
> 
> 
> +/**
> 
> +  Validates a SMBIOS 2.0 table entry point.
> 
> +
> 
> +  @param  EntryPointStructure   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
> 
> +ValidateSmbios20Table(
> 
> +  IN SMBIOS_TABLE_ENTRY_POINT      *EntryPointStructure
> 
> +) {
> 
> +  UINT8 Checksum;
> 
> +
> 
> +  if (CompareMem (EntryPointStructure->AnchorString, "_SM_", 4) != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (EntryPointStructure->EntryPointLength < 0x1E) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (EntryPointStructure->MajorVersion < 2) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (EntryPointStructure->SmbiosBcdRevision > 0 &&
> 
> +    (EntryPointStructure->SmbiosBcdRevision >> 4) < 2) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (EntryPointStructure->TableLength == 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (EntryPointStructure->TableAddress == 0 ||
> 
> +    EntryPointStructure->TableAddress == ~0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  Checksum = CalculateSum8((UINT8 *) EntryPointStructure,
> 
> +    EntryPointStructure->EntryPointLength);
> 
> +  if (Checksum != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  Checksum = CalculateSum8((UINT8 *) EntryPointStructure + 0x10,
> 
> +    EntryPointStructure->EntryPointLength - 0x10);
> 
> +  if (Checksum != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  return TRUE;
> 
> +}
> 
> +
> 
> +/**
> 
> +  Validates a SMBIOS 3.0 table entry point.
> 
> +
> 
> +  @param  Smbios30EntryPointStructure   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
> 
> +ValidateSmbios30Table(
> 
> +  IN SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30EntryPointStructure
> 
> +) {
> 
> +  UINT8 Checksum;
> 
> +
> 
> +  if (CompareMem (Smbios30EntryPointStructure->AnchorString, "_SM3_",
> 5) != 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (Smbios30EntryPointStructure->EntryPointLength < 0x18) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (Smbios30EntryPointStructure->MajorVersion < 3) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (Smbios30EntryPointStructure->TableMaximumSize == 0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +  if (Smbios30EntryPointStructure->TableAddress == 0 ||
> 
> +    Smbios30EntryPointStructure->TableAddress == ~0) {
> 
> +    return FALSE;
> 
> +  }
> 
> +
> 
> +  Checksum = CalculateSum8((UINT8 *) Smbios30EntryPointStructure,
> 
> +    Smbios30EntryPointStructure->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.
> 
> +
> 
> +**/
> 
> +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;
> 
> +
> 
> +  do {
> 
> +    // Check for end marker
> 
> +    if (Smbios.Hdr->Type == 127) {
> 
> +      break;
> 
> +    }
> 
> +
> 
> +    // Install the table
> 
> +    SmbiosHandle = SMBIOS_HANDLE_PI_RESERVED;
> 
> +    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 (*(UINT8*)++String == 0) {
> 
> +        //
> 
> +        // Pointer to the next SMBIOS structure.
> 
> +        //
> 
> +        Smbios.Raw = (UINT8 *)++String;
> 
> +        break;
> 
> +      }
> 
> +    } while (TRUE);
> 
> +  } while (Smbios.Raw < SmbiosEnd.Raw);
> 
> +
> 
> +  return EFI_SUCCESS;
> 
> +}
> 
> +
> 
>  /**
> 
> 
> 
>    Driver to produce Smbios protocol and pre-allocate 1 page for the final
> SMBIOS table.
> 
> @@ -1426,7 +1597,10 @@ SmbiosDriverEntryPoint (
>    IN EFI_SYSTEM_TABLE     *SystemTable
> 
>    )
> 
>  {
> 
> -  EFI_STATUS            Status;
> 
> +  EFI_STATUS                    Status;
> 
> +  SMBIOS_TABLE_ENTRY_POINT      *SmbiosTable;
> 
> +  SMBIOS_TABLE_3_0_ENTRY_POINT  *Smbios30Table;
> 
> +  SMBIOS_STRUCTURE_POINTER      Smbios;
> 
> 
> 
>    mPrivateData.Signature                = SMBIOS_INSTANCE_SIGNATURE;
> 
>    mPrivateData.Smbios.Add               = SmbiosAdd;
> 
> @@ -1450,6 +1624,51 @@ SmbiosDriverEntryPoint (
>                    EFI_NATIVE_INTERFACE,
> 
>                    &mPrivateData.Smbios
> 
>                    );
> 
> +  //
> 
> +  // Scan for existing SMBIOS tables installed by bootloader
> 
> +  //
> 
> +  Status = EfiGetSystemConfigurationTable (
> 
> +               &gEfiSmbios3TableGuid,
> 
> +               (VOID **) &Smbios30Table
> 
> +               );
> 
> +  if (!EFI_ERROR (Status) && ValidateSmbios30Table(Smbios30Table)) {
> 
> +    Smbios.Raw = AllocatePool(Smbios30Table->TableMaximumSize);
> 
> +    if (!Smbios.Raw) {
> 
> +      return EFI_OUT_OF_RESOURCES;
> 
> +    }
> 
> +    //
> 
> +    // Backup old table in case it gets overwritten while parsing it
> 
> +    //
> 
> +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> >TableMaximumSize);
> 
> +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> Smbios30Table->TableMaximumSize);
> 
> +    FreePool(Smbios.Raw);
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse
> preinstalled tables\n"));
> 
> +      Status = EFI_SUCCESS;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  Status = EfiGetSystemConfigurationTable (
> 
> +               &gEfiSmbiosTableGuid,
> 
> +               (VOID **) &SmbiosTable
> 
> +               );
> 
> +  if (!EFI_ERROR (Status) && ValidateSmbios20Table(SmbiosTable)) {
> 
> +    Smbios.Raw = AllocatePool(SmbiosTable->TableLength);
> 
> +    if (!Smbios.Raw) {
> 
> +      return EFI_OUT_OF_RESOURCES;
> 
> +    }
> 
> +    //
> 
> +    // Backup old table in case it gets overwritten while parsing it
> 
> +    //
> 
> +    CopyMem (Smbios.Raw, (VOID *)(UINTN)SmbiosTable->TableAddress,
> SmbiosTable->TableLength);
> 
> 
> 
> -  return Status;
> 
> +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> SmbiosTable->TableLength);
> 
> +    FreePool(Smbios.Raw);
> 
> +    if (EFI_ERROR (Status)) {
> 
> +      DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse
> preinstalled tables\n"));
> 
> +      Status = EFI_SUCCESS;
> 
> +    }
> 
> +  }
> 
> +
> 
> +  return EFI_SUCCESS;
> 
>  }
> 
> --
> 2.26.2

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Guo Dong]

hi Ray,

Just saw the discussion on this patch.
Both coreboot and SBL would build the whole SMBIOS table and report it to payloads.

For UEFI payload, I think it is not necessary to let other driver (BlSupportDxe) to split the whole SMBIOS table into records.
I would prefer SMBIOS DXE diver could support the whole SMBIOS table from PEI/bootloader.
But it is also possible to support individual records if required by checking AnchorString to know if it is whole table.

Thanks,
Guo

> -----Original Message-----
> From: Ni, Ray <ray.ni@intel.com>
> Sent: Wednesday, March 3, 2021 3:04 AM
> To: Patrick Rudolph <patrick.rudolph@9elements.com>
> Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> Benjamin <benjamin.you@intel.com>;
> philipp.deppenwiese@9elements.com; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> Subject: RE: [edk2-devel] [PATCH - resend]
> MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> 
> >
> > Hi Ray,
> > thanks for your feedback.
> >
> > Currently a single HOB containing all the SMBIOS table is exported by
> > coreboot.
> > As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> > solution.
> 
> Hi Patrick,
> I checked the code in deep.
> The HOB is not created by coreboot. It's the PayloadEntry that creates the
> HOB.
> Can we update PayloadEntry to create multiple HOBs?
> 
> Guo,
> Any comments?
> 
> The reason I like this approach is it doesn't require the other bootloaders to
> write
> a SMBIOS driver that merges all SMBIOS structures together into one table.
> 
> Thanks,
> Ray

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Ni, Ray]

To be specific, the reasons I like to use multiple HOBs each containing a SMBIOS structure are:
1. A well modularized bootloader may have one module producing type 4 structure, another module producing type 19 structure.
2. Try to think about what the optimal design could be regarding the universal payload spec (https://universalpayload.github.io/documentation/spec/spec.html). (The spec is not widely accepted and just an RFC.)

There are two style of consumer code:
A. SmbiosDxe consumes a Guid HOB which contains a full SMBIOS table.
B. SmbiosDxe consumes multiple Guid HOBs each contains a SMBIOS structure.

There are two options of implementations:
1. Support style A for coreboot and extend to style B for more bootloaders.
2. Support style B only. PayloadEntry breaks the coreboot SMBIOS table to multiple Guid HOBs each contains a SMBIOS structure.

Either option works for me though I will be more comfortable if choosing 2. 😊

Thanks,
Ray

> -----Original Message-----
> From: Dong, Guo <guo.dong@intel.com>
> Sent: Thursday, March 4, 2021 1:54 AM
> To: Ni, Ray <ray.ni@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>
> Cc: devel@edk2.groups.io; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You, Benjamin <benjamin.you@intel.com>;
> philipp.deppenwiese@9elements.com; Ma, Maurice <maurice.ma@intel.com>
> Subject: RE: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> 
> 
> hi Ray,
> 
> Just saw the discussion on this patch.
> Both coreboot and SBL would build the whole SMBIOS table and report it to payloads.
> 
> For UEFI payload, I think it is not necessary to let other driver (BlSupportDxe) to split the whole SMBIOS table into records.
> I would prefer SMBIOS DXE diver could support the whole SMBIOS table from PEI/bootloader.
> But it is also possible to support individual records if required by checking AnchorString to know if it is whole table.
> 
> Thanks,
> Guo
> 
> > -----Original Message-----
> > From: Ni, Ray <ray.ni@intel.com>
> > Sent: Wednesday, March 3, 2021 3:04 AM
> > To: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> > Benjamin <benjamin.you@intel.com>;
> > philipp.deppenwiese@9elements.com; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> > Subject: RE: [edk2-devel] [PATCH - resend]
> > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> >
> > >
> > > Hi Ray,
> > > thanks for your feedback.
> > >
> > > Currently a single HOB containing all the SMBIOS table is exported by
> > > coreboot.
> > > As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> > > solution.
> >
> > Hi Patrick,
> > I checked the code in deep.
> > The HOB is not created by coreboot. It's the PayloadEntry that creates the
> > HOB.
> > Can we update PayloadEntry to create multiple HOBs?
> >
> > Guo,
> > Any comments?
> >
> > The reason I like this approach is it doesn't require the other bootloaders to
> > write
> > a SMBIOS driver that merges all SMBIOS structures together into one table.
> >
> > Thanks,
> > Ray

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Ni, Ray]

Patrick,
Can you please send out a new patch which modifies SmbiosDxe to consume ...?
1. A single gEfiSmbios3TableGuid HOB which contains the whole SMBIOS table (starting with SMBIOS_TABLE_3_0_ENTRY_POINT), or
2. A single gEfiSmbiosTableGuid HOB which contains the whole SMBIOS table (starting with SMBIOS_TABLE_ENTRY_POINT).

The code change that consumes multiple gEdkiiSmbiosStructureGuid HOBs which contains an individual SMBIOS structure (starting with SMBIOS_STRUCTURE) can be done later.

Thanks,
Ray

> -----Original Message-----
> From: Ni, Ray
> Sent: Thursday, March 4, 2021 9:03 AM
> To: Dong, Guo <guo.dong@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>
> Cc: devel@edk2.groups.io; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You, Benjamin <benjamin.you@intel.com>;
> philipp.deppenwiese@9elements.com; Ma, Maurice <maurice.ma@intel.com>
> Subject: RE: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> 
> To be specific, the reasons I like to use multiple HOBs each containing a SMBIOS structure are:
> 1. A well modularized bootloader may have one module producing type 4 structure, another module producing type 19 structure.
> 2. Try to think about what the optimal design could be regarding the universal payload spec
> (https://universalpayload.github.io/documentation/spec/spec.html). (The spec is not widely accepted and just an RFC.)
> 
> There are two style of consumer code:
> A. SmbiosDxe consumes a Guid HOB which contains a full SMBIOS table.
> B. SmbiosDxe consumes multiple Guid HOBs each contains a SMBIOS structure.
> 
> There are two options of implementations:
> 1. Support style A for coreboot and extend to style B for more bootloaders.
> 2. Support style B only. PayloadEntry breaks the coreboot SMBIOS table to multiple Guid HOBs each contains a SMBIOS structure.
> 
> Either option works for me though I will be more comfortable if choosing 2. 😊
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Dong, Guo <guo.dong@intel.com>
> > Sent: Thursday, March 4, 2021 1:54 AM
> > To: Ni, Ray <ray.ni@intel.com>; Patrick Rudolph <patrick.rudolph@9elements.com>
> > Cc: devel@edk2.groups.io; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You, Benjamin <benjamin.you@intel.com>;
> > philipp.deppenwiese@9elements.com; Ma, Maurice <maurice.ma@intel.com>
> > Subject: RE: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> >
> >
> > hi Ray,
> >
> > Just saw the discussion on this patch.
> > Both coreboot and SBL would build the whole SMBIOS table and report it to payloads.
> >
> > For UEFI payload, I think it is not necessary to let other driver (BlSupportDxe) to split the whole SMBIOS table into records.
> > I would prefer SMBIOS DXE diver could support the whole SMBIOS table from PEI/bootloader.
> > But it is also possible to support individual records if required by checking AnchorString to know if it is whole table.
> >
> > Thanks,
> > Guo
> >
> > > -----Original Message-----
> > > From: Ni, Ray <ray.ni@intel.com>
> > > Sent: Wednesday, March 3, 2021 3:04 AM
> > > To: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> > > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> > > Benjamin <benjamin.you@intel.com>;
> > > philipp.deppenwiese@9elements.com; Ma, Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> > > Subject: RE: [edk2-devel] [PATCH - resend]
> > > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> > >
> > > >
> > > > Hi Ray,
> > > > thanks for your feedback.
> > > >
> > > > Currently a single HOB containing all the SMBIOS table is exported by
> > > > coreboot.
> > > > As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> > > > solution.
> > >
> > > Hi Patrick,
> > > I checked the code in deep.
> > > The HOB is not created by coreboot. It's the PayloadEntry that creates the
> > > HOB.
> > > Can we update PayloadEntry to create multiple HOBs?
> > >
> > > Guo,
> > > Any comments?
> > >
> > > The reason I like this approach is it doesn't require the other bootloaders to
> > > write
> > > a SMBIOS driver that merges all SMBIOS structures together into one table.
> > >
> > > Thanks,
> > > Ray

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Zhiguang Liu]

Hi Patrick Rudolph,

I also want this feature that enables Smbios driver to parse and install existing SMBIOS table.
I notice there are some comments about your patch. If you don't have time to refine it, I can keep your signed-off-by, and continue your work based on Ray and Guo's comments.

Thanks
Zhiguang

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> Sent: Wednesday, March 3, 2021 5:15 PM
> To: Patrick Rudolph <patrick.rudolph@9elements.com>
> Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> Benjamin <benjamin.you@intel.com>;
> philipp.deppenwiese@9elements.com; Ma, Maurice
> <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> Subject: Re: [edk2-devel] [PATCH - resend]
> MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> 
> I have 5 more comments embedded, can you read and reply?
> 
> > -----Original Message-----
> > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Sent: Wednesday, March 3, 2021 4:38 PM
> > To: Ni, Ray <ray.ni@intel.com>
> > Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
> Zeng,
> > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> > Benjamin <benjamin.you@intel.com>;
> > philipp.deppenwiese@9elements.com; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH - resend]
> > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> >
> > Hi Ray,
> > thanks for your feedback.
> >
> > Currently a single HOB containing all the SMBIOS table is exported by
> > coreboot.
> > As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> > solution.
> >
> > I'll look into passing a HOB instead of using
> > EfiGetSystemConfigurationTable and see if I can get rid of the table
> > shadow copy.
> >
> > Regards,
> > Patrick Rudolph
> >
> > On Wed, Mar 3, 2021 at 9:13 AM Ni, Ray <ray.ni@intel.com> wrote:
> > >
> > > In general, I agree this solution that lets SMBIOS driver directly absorbs
> the
> > SMBIOS table from PEI.
> > > This can eliminate the needs of a separate driver that consumes the HOB
> > and calls SMBIOS protocol to add the SMBIOS structures.
> > >
> > > There are two options for the HOB design:
> > > 1. A single HOB that points to the SMBIOS table.
> > > 2. Multiple HOBs that each points to a SMBIOS structure.
> > >
> > > In my opinion, option #2 is more flexible because it doesn't require the
> > bootloader to consolidate all the SMBIOS structures together.
> > > The CPU module in the bootloader can produce the type 4 and 7
> structures.
> > > The PCI module in the bootloader can produce the type 9 structures.
> > >
> > > But, I am not sure if option #2 is conflict with what coreboot does. Does
> > coreboot produce the whole SMBIOS table in a single buffer?
> > > Option #2 also doesn't care whether it's a SMBIOS 3.0 table or 2.x table.
> > >
> > > >+  Status = EfiGetSystemConfigurationTable (
> > >
> > > 1. Why don't you directly get the data from HOB list? This can eliminate
> the
> > code in BlSupportDxe that gets data in HOB and publishes to
> > > configuration table.
> > >
> > > > +ValidateSmbios20Table(
> > > > +ValidateSmbios30Table(
> > >
> > > 2. I will defer to experts (Dandan, Star and Zhichao) to review whether
> the
> > above two functions are implemented properly.
> > >
> > > >
> > > > +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;
> > > > +
> > > > +  do {
> > > > +    // Check for end marker
> > > > +    if (Smbios.Hdr->Type == 127) {
> > >
> > > 3. Please use SMBIOS_TYPE_END_OF_TABLE instead of hardcode 127.
> > >
> > > >
> > > > +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> > > > >TableMaximumSize);
> > >
> > > 4. Should we copy from Smbios30Table->TableAddress instead of
> > Smbios30Table?
> > >
> > > >
> > > > +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> > > > Smbios30Table->TableMaximumSize);
> > >
> > > 5. Can you explain in specific why SMBIOS table should be duplicated
> > before parsing?
> > >
> > >
> 
> 
> 
> 

Re: [edk2-devel] [PATCH - resend] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables

[Patrick Rudolph]

Hi Zhiguang,
this is still on my todo-list, but I'm quite busy right now.
If you got time please take care of that patch.

Regards,
Patrick Rudolph

On Thu, Apr 1, 2021 at 6:42 AM Liu, Zhiguang <zhiguang.liu@intel.com> wrote:
>
> Hi Patrick Rudolph,
>
> I also want this feature that enables Smbios driver to parse and install existing SMBIOS table.
> I notice there are some comments about your patch. If you don't have time to refine it, I can keep your signed-off-by, and continue your work based on Ray and Guo's comments.
>
> Thanks
> Zhiguang
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ni, Ray
> > Sent: Wednesday, March 3, 2021 5:15 PM
> > To: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng,
> > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> > Benjamin <benjamin.you@intel.com>;
> > philipp.deppenwiese@9elements.com; Ma, Maurice
> > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> > Subject: Re: [edk2-devel] [PATCH - resend]
> > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> >
> > I have 5 more comments embedded, can you read and reply?
> >
> > > -----Original Message-----
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Sent: Wednesday, March 3, 2021 4:38 PM
> > > To: Ni, Ray <ray.ni@intel.com>
> > > Cc: devel@edk2.groups.io; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@intel.com>; Bi, Dandan <dandan.bi@intel.com>;
> > Zeng,
> > > Star <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; You,
> > > Benjamin <benjamin.you@intel.com>;
> > > philipp.deppenwiese@9elements.com; Ma, Maurice
> > > <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>
> > > Subject: Re: [edk2-devel] [PATCH - resend]
> > > MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
> > >
> > > Hi Ray,
> > > thanks for your feedback.
> > >
> > > Currently a single HOB containing all the SMBIOS table is exported by
> > > coreboot.
> > > As coreboot doesn't support multiple HOBs with the same ID, #2 isn't a
> > > solution.
> > >
> > > I'll look into passing a HOB instead of using
> > > EfiGetSystemConfigurationTable and see if I can get rid of the table
> > > shadow copy.
> > >
> > > Regards,
> > > Patrick Rudolph
> > >
> > > On Wed, Mar 3, 2021 at 9:13 AM Ni, Ray <ray.ni@intel.com> wrote:
> > > >
> > > > In general, I agree this solution that lets SMBIOS driver directly absorbs
> > the
> > > SMBIOS table from PEI.
> > > > This can eliminate the needs of a separate driver that consumes the HOB
> > > and calls SMBIOS protocol to add the SMBIOS structures.
> > > >
> > > > There are two options for the HOB design:
> > > > 1. A single HOB that points to the SMBIOS table.
> > > > 2. Multiple HOBs that each points to a SMBIOS structure.
> > > >
> > > > In my opinion, option #2 is more flexible because it doesn't require the
> > > bootloader to consolidate all the SMBIOS structures together.
> > > > The CPU module in the bootloader can produce the type 4 and 7
> > structures.
> > > > The PCI module in the bootloader can produce the type 9 structures.
> > > >
> > > > But, I am not sure if option #2 is conflict with what coreboot does. Does
> > > coreboot produce the whole SMBIOS table in a single buffer?
> > > > Option #2 also doesn't care whether it's a SMBIOS 3.0 table or 2.x table.
> > > >
> > > > >+  Status = EfiGetSystemConfigurationTable (
> > > >
> > > > 1. Why don't you directly get the data from HOB list? This can eliminate
> > the
> > > code in BlSupportDxe that gets data in HOB and publishes to
> > > > configuration table.
> > > >
> > > > > +ValidateSmbios20Table(
> > > > > +ValidateSmbios30Table(
> > > >
> > > > 2. I will defer to experts (Dandan, Star and Zhichao) to review whether
> > the
> > > above two functions are implemented properly.
> > > >
> > > > >
> > > > > +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;
> > > > > +
> > > > > +  do {
> > > > > +    // Check for end marker
> > > > > +    if (Smbios.Hdr->Type == 127) {
> > > >
> > > > 3. Please use SMBIOS_TYPE_END_OF_TABLE instead of hardcode 127.
> > > >
> > > > >
> > > > > +    CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table-
> > > > > >TableMaximumSize);
> > > >
> > > > 4. Should we copy from Smbios30Table->TableAddress instead of
> > > Smbios30Table?
> > > >
> > > > >
> > > > > +    Status = ParseAndAddExistingSmbiosTable(ImageHandle, Smbios,
> > > > > Smbios30Table->TableMaximumSize);
> > > >
> > > > 5. Can you explain in specific why SMBIOS table should be duplicated
> > > before parsing?
> > > >
> > > >
> >
> >
> > 
> >
>