[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/Un=
iversal/SmbiosDxe/SmbiosDxe.c
index 3cdb0b1ed7..958a249cf9 100644
--- a/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
+++ b/MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c
@@ -1408,6 +1408,177 @@ SmbiosTableConstruction (
}=0D
}=0D
=0D
+/**=0D
+ Validates a SMBIOS 2.0 table entry point.=0D
+=0D
+ @param EntryPointStructure The SMBIOS_TABLE_ENTRY_POINT to validate.=
=0D
+=0D
+ @retval TRUE SMBIOS table entry point is valid.=0D
+ @retval FALSE SMBIOS table entry point is malformed.=0D
+=0D
+**/=0D
+STATIC=0D
+BOOLEAN=0D
+ValidateSmbios20Table(=0D
+ IN SMBIOS_TABLE_ENTRY_POINT *EntryPointStructure=0D
+) {=0D
+ UINT8 Checksum;=0D
+=0D
+ if (CompareMem (EntryPointStructure->AnchorString, "_SM_", 4) !=3D 0) {=
=0D
+ return FALSE;=0D
+ }=0D
+ if (EntryPointStructure->EntryPointLength < 0x1E) {=0D
+ return FALSE;=0D
+ }=0D
+ if (EntryPointStructure->MajorVersion < 2) {=0D
+ return FALSE;=0D
+ }=0D
+ if (EntryPointStructure->SmbiosBcdRevision > 0 &&=0D
+ (EntryPointStructure->SmbiosBcdRevision >> 4) < 2) {=0D
+ return FALSE;=0D
+ }=0D
+ if (EntryPointStructure->TableLength =3D=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+ if (EntryPointStructure->TableAddress =3D=3D 0 ||=0D
+ EntryPointStructure->TableAddress =3D=3D ~0) {=0D
+ return FALSE;=0D
+ }=0D
+=0D
+ Checksum =3D CalculateSum8((UINT8 *) EntryPointStructure,=0D
+ EntryPointStructure->EntryPointLength);=0D
+ if (Checksum !=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+=0D
+ Checksum =3D CalculateSum8((UINT8 *) EntryPointStructure + 0x10,=0D
+ EntryPointStructure->EntryPointLength - 0x10);=0D
+ if (Checksum !=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+ return TRUE;=0D
+}=0D
+=0D
+/**=0D
+ Validates a SMBIOS 3.0 table entry point.=0D
+=0D
+ @param Smbios30EntryPointStructure The SMBIOS_TABLE_3_0_ENTRY_POINT t=
o validate.=0D
+=0D
+ @retval TRUE SMBIOS table entry point is valid.=0D
+ @retval FALSE SMBIOS table entry point is malformed.=0D
+=0D
+**/=0D
+STATIC=0D
+BOOLEAN=0D
+ValidateSmbios30Table(=0D
+ IN SMBIOS_TABLE_3_0_ENTRY_POINT *Smbios30EntryPointStructure=0D
+) {=0D
+ UINT8 Checksum;=0D
+=0D
+ if (CompareMem (Smbios30EntryPointStructure->AnchorString, "_SM3_", 5) !=
=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+ if (Smbios30EntryPointStructure->EntryPointLength < 0x18) {=0D
+ return FALSE;=0D
+ }=0D
+ if (Smbios30EntryPointStructure->MajorVersion < 3) {=0D
+ return FALSE;=0D
+ }=0D
+ if (Smbios30EntryPointStructure->TableMaximumSize =3D=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+ if (Smbios30EntryPointStructure->TableAddress =3D=3D 0 ||=0D
+ Smbios30EntryPointStructure->TableAddress =3D=3D ~0) {=0D
+ return FALSE;=0D
+ }=0D
+=0D
+ Checksum =3D CalculateSum8((UINT8 *) Smbios30EntryPointStructure,=0D
+ Smbios30EntryPointStructure->EntryPointLength);=0D
+ if (Checksum !=3D 0) {=0D
+ return FALSE;=0D
+ }=0D
+ return TRUE;=0D
+}=0D
+=0D
+/**=0D
+ Parse an existing SMBIOS table and insert it using SmbiosAdd.=0D
+=0D
+ @param ImageHandle The EFI_HANDLE to this driver.=0D
+ @param Smbios The SMBIOS table to parse.=0D
+ @param Length The length of the SMBIOS table.=0D
+=0D
+ @retval EFI_SUCCESS SMBIOS table was parsed and installed.=0D
+ @retval EFI_OUT_OF_RESOURCES Record was not added due to lack of system=
resources.=0D
+=0D
+**/=0D
+STATIC=0D
+EFI_STATUS=0D
+ParseAndAddExistingSmbiosTable(=0D
+ IN EFI_HANDLE ImageHandle,=0D
+ IN SMBIOS_STRUCTURE_POINTER Smbios,=0D
+ IN UINTN Length=0D
+) {=0D
+ EFI_STATUS Status;=0D
+ CHAR8 *String;=0D
+ EFI_SMBIOS_HANDLE SmbiosHandle;=0D
+ SMBIOS_STRUCTURE_POINTER SmbiosEnd;=0D
+=0D
+ SmbiosEnd.Raw =3D Smbios.Raw + Length;=0D
+=0D
+ do {=0D
+ // Check for end marker=0D
+ if (Smbios.Hdr->Type =3D=3D 127) {=0D
+ break;=0D
+ }=0D
+=0D
+ // Install the table=0D
+ SmbiosHandle =3D SMBIOS_HANDLE_PI_RESERVED;=0D
+ Status =3D SmbiosAdd (=0D
+ &mPrivateData.Smbios,=0D
+ ImageHandle,=0D
+ &SmbiosHandle,=0D
+ Smbios.Hdr=0D
+ );=0D
+=0D
+ ASSERT_EFI_ERROR (Status);=0D
+ if (EFI_ERROR (Status)) {=0D
+ return Status;=0D
+ }=0D
+ //=0D
+ // Go to the next SMBIOS structure. Each SMBIOS structure may include =
2 parts:=0D
+ // 1. Formatted section; 2. Unformatted string section. So, 2 steps ar=
e needed=0D
+ // to skip one SMBIOS structure.=0D
+ //=0D
+=0D
+ //=0D
+ // Step 1: Skip over formatted section.=0D
+ //=0D
+ String =3D (CHAR8 *) (Smbios.Raw + Smbios.Hdr->Length);=0D
+=0D
+ //=0D
+ // Step 2: Skip over unformatted string section.=0D
+ //=0D
+ do {=0D
+ //=0D
+ // Each string is terminated with a NULL(00h) BYTE and the sets of s=
trings=0D
+ // is terminated with an additional NULL(00h) BYTE.=0D
+ //=0D
+ for ( ; *String !=3D 0; String++) {=0D
+ }=0D
+=0D
+ if (*(UINT8*)++String =3D=3D 0) {=0D
+ //=0D
+ // Pointer to the next SMBIOS structure.=0D
+ //=0D
+ Smbios.Raw =3D (UINT8 *)++String;=0D
+ break;=0D
+ }=0D
+ } while (TRUE);=0D
+ } while (Smbios.Raw < SmbiosEnd.Raw);=0D
+=0D
+ return EFI_SUCCESS;=0D
+}=0D
+=0D
/**=0D
=0D
Driver to produce Smbios protocol and pre-allocate 1 page for the final =
SMBIOS table.=0D
@@ -1426,7 +1597,10 @@ SmbiosDriverEntryPoint (
IN EFI_SYSTEM_TABLE *SystemTable=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
+ EFI_STATUS Status;=0D
+ SMBIOS_TABLE_ENTRY_POINT *SmbiosTable;=0D
+ SMBIOS_TABLE_3_0_ENTRY_POINT *Smbios30Table;=0D
+ SMBIOS_STRUCTURE_POINTER Smbios;=0D
=0D
mPrivateData.Signature =3D SMBIOS_INSTANCE_SIGNATURE;=0D
mPrivateData.Smbios.Add =3D SmbiosAdd;=0D
@@ -1450,6 +1624,51 @@ SmbiosDriverEntryPoint (
EFI_NATIVE_INTERFACE,=0D
&mPrivateData.Smbios=0D
);=0D
+ //=0D
+ // Scan for existing SMBIOS tables installed by bootloader=0D
+ //=0D
+ Status =3D EfiGetSystemConfigurationTable (=0D
+ &gEfiSmbios3TableGuid,=0D
+ (VOID **) &Smbios30Table=0D
+ );=0D
+ if (!EFI_ERROR (Status) && ValidateSmbios30Table(Smbios30Table)) {=0D
+ Smbios.Raw =3D AllocatePool(Smbios30Table->TableMaximumSize);=0D
+ if (!Smbios.Raw) {=0D
+ return EFI_OUT_OF_RESOURCES;=0D
+ }=0D
+ //=0D
+ // Backup old table in case it gets overwritten while parsing it=0D
+ //=0D
+ CopyMem (Smbios.Raw, (VOID *)Smbios30Table, Smbios30Table->TableMaximu=
mSize);=0D
+ Status =3D ParseAndAddExistingSmbiosTable(ImageHandle, Smbios, Smbios3=
0Table->TableMaximumSize);=0D
+ FreePool(Smbios.Raw);=0D
+ if (EFI_ERROR (Status)) {=0D
+ DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse preins=
talled tables\n"));=0D
+ Status =3D EFI_SUCCESS;=0D
+ }=0D
+ }=0D
+=0D
+ Status =3D EfiGetSystemConfigurationTable (=0D
+ &gEfiSmbiosTableGuid,=0D
+ (VOID **) &SmbiosTable=0D
+ );=0D
+ if (!EFI_ERROR (Status) && ValidateSmbios20Table(SmbiosTable)) {=0D
+ Smbios.Raw =3D AllocatePool(SmbiosTable->TableLength);=0D
+ if (!Smbios.Raw) {=0D
+ return EFI_OUT_OF_RESOURCES;=0D
+ }=0D
+ //=0D
+ // Backup old table in case it gets overwritten while parsing it=0D
+ //=0D
+ CopyMem (Smbios.Raw, (VOID *)(UINTN)SmbiosTable->TableAddress, SmbiosT=
able->TableLength);=0D
=0D
- return Status;=0D
+ Status =3D ParseAndAddExistingSmbiosTable(ImageHandle, Smbios, SmbiosT=
able->TableLength);=0D
+ FreePool(Smbios.Raw);=0D
+ if (EFI_ERROR (Status)) {=0D
+ DEBUG ((DEBUG_ERROR, "SmbiosDriverEntryPoint: Failed to parse preins=
talled tables\n"));=0D
+ Status =3D EFI_SUCCESS;=0D
+ }=0D
+ }=0D
+=0D
+ return EFI_SUCCESS;=0D
}=0D
--=20
2.26.2


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?


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?


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?


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


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


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


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


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


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?




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?