public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
* [PATCH 0/9] Create multiple Hobs for Universal Payload
@ 2021-05-24  7:12 Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 1/9] MdePkg: Add Universal Payload general defination header file Zhiguang Liu
                   ` (9 more replies)
  0 siblings, 10 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel

This patch set is based on Universal Payload on
https://universalpayload.github.io/documentation/payload-interfaces/index.html

This patch set introduce one general header, three different hob types
and how Universal Payload consume these hobs.


Zhiguang Liu (9):
  MdePkg: Add Universal Payload general defination header file
  MdePkg: Add new structure for the PCI Root Bridge Info Hob
  UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob
  MdePkg: Add new structure for the Universal Payload SMBios Table Info
    Hob
  MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob
  MdePkg: Add new structure for the Universal Payload ACPI Table Info
    Hob
  MdeModulePkg/ACPI: Install ACPI table from HOB.
  UefiPayloadPkg: Creat gPldAcpiTableGuid Hob

 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h           |   4 +++-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf      |   4 +++-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c   | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c                   | 299 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h                   |   4 +++-
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf                 |   5 ++++-
 MdePkg/Include/UniversalPayload/AcpiTable.h                    |  28 ++++++++++++++++++++++++++++
 MdePkg/Include/UniversalPayload/PciRootBridges.h               |  89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 MdePkg/Include/UniversalPayload/SmbiosTable.h                  |  28 ++++++++++++++++++++++++++++
 MdePkg/Include/UniversalPayload/UniversalPayload.h             |  33 +++++++++++++++++++++++++++++++++
 MdePkg/MdePkg.dec                                              |  15 +++++++++++++++
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c                     |  28 +---------------------------
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h                     |   5 +----
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf                   |   4 +---
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h        |  40 ++++++++++++++++++++++++++++++++++++++--
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c     |  47 ++++++++++++++++++++++++++++++++++++++++++++---
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf   |   8 +++++++-
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |  73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c             |  23 ++++++++++++++++++++++-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h             |   5 +++--
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf           |   4 +++-
 UefiPayloadPkg/UefiPayloadPkg.dsc                              |   2 +-
 22 files changed, 832 insertions(+), 60 deletions(-)
 create mode 100644 MdePkg/Include/UniversalPayload/AcpiTable.h
 create mode 100644 MdePkg/Include/UniversalPayload/PciRootBridges.h
 create mode 100644 MdePkg/Include/UniversalPayload/SmbiosTable.h
 create mode 100644 MdePkg/Include/UniversalPayload/UniversalPayload.h

-- 
2.30.0.windows.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/9] MdePkg: Add Universal Payload general defination header file
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 2/9] MdePkg: Add new structure for the PCI Root Bridge Info Hob Zhiguang Liu
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Liming Gao

Add Universal Payload general defination header file according to
Universal Payload’s documentation

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/UniversalPayload/UniversalPayload.h | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/MdePkg/Include/UniversalPayload/UniversalPayload.h b/MdePkg/Include/UniversalPayload/UniversalPayload.h
new file mode 100644
index 0000000000..627b9e880e
--- /dev/null
+++ b/MdePkg/Include/UniversalPayload/UniversalPayload.h
@@ -0,0 +1,33 @@
+/** @file
+  Universal Payload general definations.
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __UNIVERSAL_PAYLOAD_H__
+#define __UNIVERSAL_PAYLOAD_H__
+
+#pragma pack(1)
+
+typedef struct {
+  UINT8                Revision;
+  UINT8                Reserved;
+  UINT16               Length;
+} PLD_GENERIC_HEADER;
+
+#pragma pack()
+
+/**
+  Returns the size of a structure of known type, up through and including a specified field.
+
+  @param   TYPE     The name of the data structure that contains the field specified by Field.
+  @param   Field    The name of the field in the data structure.
+
+  @return  size, in bytes.
+
+**/
+#define PLD_SIZEOF_THROUGH_FIELD(TYPE, Field) (OFFSET_OF(TYPE, Field) + sizeof (((TYPE *) 0)->Field))
+
+#endif // __UNIVERSAL_PAYLOAD_H__
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/9] MdePkg: Add new structure for the PCI Root Bridge Info Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 1/9] MdePkg: Add Universal Payload general defination header file Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 3/9] UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob Zhiguang Liu
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Liming Gao

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/UniversalPayload/PciRootBridges.h | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 MdePkg/MdePkg.dec                                |  6 ++++++
 2 files changed, 95 insertions(+)

diff --git a/MdePkg/Include/UniversalPayload/PciRootBridges.h b/MdePkg/Include/UniversalPayload/PciRootBridges.h
new file mode 100644
index 0000000000..72e8331ede
--- /dev/null
+++ b/MdePkg/Include/UniversalPayload/PciRootBridges.h
@@ -0,0 +1,89 @@
+/** @file
+  This file defines the structure for the PCI Root Bridges.
+
+  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PLD_PCI_ROOT_BRIDGES_H__
+#define __PLD_PCI_ROOT_BRIDGES_H__
+
+#include <UniversalPayload/UniversalPayload.h>
+
+#pragma pack(1)
+
+//
+// (Base > Limit) indicates an aperture is not available.
+//
+typedef struct {
+  //
+  // Base and Limit are the device address instead of host address when
+  // Translation is not zero
+  //
+  UINT64 Base;
+  UINT64 Limit;
+  //
+  // According to UEFI 2.7, Device Address = Host Address + Translation,
+  // so Translation = Device Address - Host Address.
+  // On platforms where Translation is not zero, the subtraction is probably to
+  // be performed with UINT64 wrap-around semantics, for we may translate an
+  // above-4G host address into a below-4G device address for legacy PCIe device
+  // compatibility.
+  //
+  // NOTE: The alignment of Translation is required to be larger than any BAR
+  // alignment in the same root bridge, so that the same alignment can be
+  // applied to both device address and host address, which simplifies the
+  // situation and makes the current resource allocation code in generic PCI
+  // host bridge driver still work.
+  //
+  UINT64 Translation;
+} PLD_PCI_ROOT_BRIDGE_APERTURE;
+
+///
+/// Payload PCI Root Bridge Information HOB
+///
+typedef struct {
+  UINT32                       Segment;               ///< Segment number.
+  UINT64                       Supports;              ///< Supported attributes.
+                                                      ///< Refer to EFI_PCI_ATTRIBUTE_xxx used by GetAttributes()
+                                                      ///< and SetAttributes() in EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.
+  UINT64                       Attributes;            ///< Initial attributes.
+                                                      ///< Refer to EFI_PCI_ATTRIBUTE_xxx used by GetAttributes()
+                                                      ///< and SetAttributes() in EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL.
+  BOOLEAN                      DmaAbove4G;            ///< DMA above 4GB memory.
+                                                      ///< Set to TRUE when root bridge supports DMA above 4GB memory.
+  BOOLEAN                      NoExtendedConfigSpace; ///< When FALSE, the root bridge supports
+                                                      ///< Extended (4096-byte) Configuration Space.
+                                                      ///< When TRUE, the root bridge supports
+                                                      ///< 256-byte Configuration Space only.
+  UINT64                       AllocationAttributes;  ///< Allocation attributes.
+                                                      ///< Refer to EFI_PCI_HOST_BRIDGE_COMBINE_MEM_PMEM and
+                                                      ///< EFI_PCI_HOST_BRIDGE_MEM64_DECODE used by GetAllocAttributes()
+                                                      ///< in EFI_PCI_HOST_BRIDGE_RESOURCE_ALLOCATION_PROTOCOL.
+  PLD_PCI_ROOT_BRIDGE_APERTURE Bus;                   ///< Bus aperture which can be used by the root bridge.
+  PLD_PCI_ROOT_BRIDGE_APERTURE Io;                    ///< IO aperture which can be used by the root bridge.
+  PLD_PCI_ROOT_BRIDGE_APERTURE Mem;                   ///< MMIO aperture below 4GB which can be used by the root bridge.
+  PLD_PCI_ROOT_BRIDGE_APERTURE MemAbove4G;            ///< MMIO aperture above 4GB which can be used by the root bridge.
+  PLD_PCI_ROOT_BRIDGE_APERTURE PMem;                  ///< Prefetchable MMIO aperture below 4GB which can be used by the root bridge.
+  PLD_PCI_ROOT_BRIDGE_APERTURE PMemAbove4G;           ///< Prefetchable MMIO aperture above 4GB which can be used by the root bridge.
+  UINT32                       HID;                   ///< PnP hardware ID of the root bridge. This value must match the corresponding
+                                                      ///< _HID in the ACPI name space.
+  UINT32                       UID;                   ///< Unique ID that is required by ACPI if two devices have the same _HID.
+                                                      ///< This value must also match the corresponding _UID/_HID pair in the ACPI name space.
+} PLD_PCI_ROOT_BRIDGE;
+
+typedef struct {
+  PLD_GENERIC_HEADER   PldHeader;
+  BOOLEAN              ResourceAssigned;
+  UINT8                Count;
+  PLD_PCI_ROOT_BRIDGE  RootBridge[0];
+} PLD_PCI_ROOT_BRIDGES;
+
+#pragma pack()
+
+#define PLD_PCI_ROOT_BRIDGES_REVISION 1
+
+extern GUID gPldPciRootBridgeInfoGuid;
+
+#endif // __PLD_PCI_ROOT_BRIDGES_H__
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index b49f88d8e1..3cf509728b 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -818,6 +818,12 @@
   #
   gTianoCustomDecompressGuid     = { 0xA31280AD, 0x481E, 0x41B6, { 0x95, 0xE8, 0x12, 0x7F, 0x4C, 0x98, 0x47, 0x79 }}
 
+  #
+  # GUID defined in UniversalPayload
+  #
+  ## Include/UniversalPayload/PciRootBridges.h
+  gPldPciRootBridgeInfoGuid = { 0xec4ebacb, 0x2638, 0x416e, { 0xbe, 0x80, 0xe5, 0xfa, 0x4b, 0x51, 0x19, 0x01 }}
+
 [Guids.IA32, Guids.X64]
   ## Include/Guid/Cper.h
   gEfiIa32X64ErrorTypeCacheCheckGuid = { 0xA55701F5, 0xE3EF, 0x43de, { 0xAC, 0x72, 0x24, 0x9B, 0x57, 0x3F, 0xAD, 0x2C }}
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/9] UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 1/9] MdePkg: Add Universal Payload general defination header file Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 2/9] MdePkg: Add new structure for the PCI Root Bridge Info Hob Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 4/9] MdePkg: Add new structure for the Universal Payload SMBios Table Info Hob Zhiguang Liu
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You

UefiPayload parse gPldPciRootBridgeInfoGuid Guid Hob to retrieve PCI root bridges
information. gPldPciRootBridgeInfoGuid Guid Hob should be created by Bootloader.

Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h        | 40 ++++++++++++++++++++++++++++++++++++++--
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c     | 47 ++++++++++++++++++++++++++++++++++++++++++++---
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf   |  8 +++++++-
 UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c | 73 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 UefiPayloadPkg/UefiPayloadPkg.dsc                              |  2 +-
 5 files changed, 162 insertions(+), 8 deletions(-)

diff --git a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h
index c2961b3bee..b22703e79e 100644
--- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h
+++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h
@@ -2,7 +2,7 @@
   Header file of PciHostBridgeLib.
 
   Copyright (C) 2016, Red Hat, Inc.
-  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -11,14 +11,38 @@
 #ifndef _PCI_HOST_BRIDGE_H
 #define _PCI_HOST_BRIDGE_H
 
+#include <UniversalPayload/PciRootBridges.h>
+
 typedef struct {
   ACPI_HID_DEVICE_PATH     AcpiDevicePath;
   EFI_DEVICE_PATH_PROTOCOL EndDevicePath;
 } CB_PCI_ROOT_BRIDGE_DEVICE_PATH;
 
+/**
+  Scan for all root bridges in platform.
+
+  @param[out] NumberOfRootBridges  Number of root bridges detected
+
+  @retval     Pointer to the allocated PCI_ROOT_BRIDGE structure array.
+**/
 PCI_ROOT_BRIDGE *
 ScanForRootBridges (
-  UINTN      *NumberOfRootBridges
+  OUT UINTN      *NumberOfRootBridges
+);
+
+/**
+  Scan for all root bridges from PldPciRootBridgeInfoHob
+
+  @param[in]  PciRootBridgeInfo    Pointer of PLD PCI Root Bridge Info Hob
+  @param[out] NumberOfRootBridges  Number of root bridges detected
+
+  @retval     Pointer to the allocated PCI_ROOT_BRIDGE structure array.
+
+**/
+PCI_ROOT_BRIDGE *
+RetrieveRootBridgeInfoFromHob (
+  IN  PLD_PCI_ROOT_BRIDGES  *PciRootBridgeInfo,
+  OUT UINTN                 *NumberOfRootBridges
 );
 
 /**
@@ -77,4 +101,16 @@ InitRootBridge (
   OUT PCI_ROOT_BRIDGE          *RootBus
 );
 
+/**
+  Initialize DevicePath for a PCI_ROOT_BRIDGE.
+  @param[in] HID               HID for device path
+  @param[in] UID               UID for device path
+
+  @retval A pointer to the new created device patch.
+**/
+EFI_DEVICE_PATH_PROTOCOL *
+CreateRootBridgeDevicePath (
+  IN     UINT32                   HID,
+  IN     UINT32                   UID
+);
 #endif
diff --git a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
index 512c3127cc..6aa43e8c7e 100644
--- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
+++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c
@@ -2,7 +2,7 @@
   Library instance of PciHostBridgeLib library class for coreboot.
 
   Copyright (C) 2016, Red Hat, Inc.
-  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -19,6 +19,7 @@
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PciHostBridgeLib.h>
 #include <Library/PciLib.h>
+#include <Library/HobLib.h>
 
 #include "PciHostBridge.h"
 
@@ -48,7 +49,6 @@ CB_PCI_ROOT_BRIDGE_DEVICE_PATH mRootBridgeDevicePathTemplate = {
   }
 };
 
-
 /**
   Initialize a PCI_ROOT_BRIDGE structure.
 
@@ -145,6 +145,27 @@ InitRootBridge (
   return EFI_SUCCESS;
 }
 
+/**
+  Initialize DevicePath for a PCI_ROOT_BRIDGE.
+  @param[in] HID               HID for device path
+  @param[in] UID               UID for device path
+
+  @retval A pointer to the new created device patch.
+**/
+EFI_DEVICE_PATH_PROTOCOL *
+CreateRootBridgeDevicePath (
+  IN     UINT32                   HID,
+  IN     UINT32                   UID
+)
+{
+  CB_PCI_ROOT_BRIDGE_DEVICE_PATH *DevicePath;
+  DevicePath = AllocateCopyPool (sizeof (mRootBridgeDevicePathTemplate),
+                                 &mRootBridgeDevicePathTemplate);
+  ASSERT (DevicePath != NULL);
+  DevicePath->AcpiDevicePath.HID = HID;
+  DevicePath->AcpiDevicePath.UID = UID;
+  return (EFI_DEVICE_PATH_PROTOCOL *)DevicePath;
+}
 
 /**
   Return all the root bridge instances in an array.
@@ -161,10 +182,30 @@ PciHostBridgeGetRootBridges (
   UINTN *Count
 )
 {
+  PLD_PCI_ROOT_BRIDGES  *PciRootBridgeInfo;
+  EFI_HOB_GUID_TYPE     *GuidHob;
+  PLD_GENERIC_HEADER    *GenericHeader;
+  //
+  // Find PLD PCI Root Bridge Info hob
+  //
+  GuidHob = GetFirstGuidHob (&gPldPciRootBridgeInfoGuid);
+  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_PCI_ROOT_BRIDGES_REVISION) && (GenericHeader->Length >= sizeof (PLD_PCI_ROOT_BRIDGES))) {
+        //
+        // PLD_PCI_ROOT_BRIDGES structure is used when Revision equals to PLD_PCI_ROOT_BRIDGES_REVISION
+        //
+        PciRootBridgeInfo = (PLD_PCI_ROOT_BRIDGES *) GET_GUID_HOB_DATA (GuidHob);
+        if (PciRootBridgeInfo->Count <= (GET_GUID_HOB_DATA_SIZE (GuidHob) - sizeof(PLD_PCI_ROOT_BRIDGES)) / sizeof(PLD_PCI_ROOT_BRIDGE)) {
+          return RetrieveRootBridgeInfoFromHob (PciRootBridgeInfo, Count);
+        }
+      }
+    }
+  }
   return ScanForRootBridges (Count);
 }
 
-
 /**
   Free the root bridge instances array returned from
   PciHostBridgeGetRootBridges().
diff --git a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
index 7896df2416..1c6a47828a 100644
--- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
+++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf
@@ -2,7 +2,7 @@
 #  Library instance of PciHostBridgeLib library class for coreboot.
 #
 #  Copyright (C) 2016, Red Hat, Inc.
-#  Copyright (c) 2016 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -39,3 +39,9 @@
   DevicePathLib
   MemoryAllocationLib
   PciLib
+
+[Guids]
+  gPldPciRootBridgeInfoGuid
+
+[Pcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
diff --git a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
index fffbf04cad..d6d59b6659 100644
--- a/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
+++ b/UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c
@@ -1,7 +1,7 @@
 /** @file
   Scan the entire PCI bus for root bridges to support coreboot UEFI payload.
 
-  Copyright (c) 2016, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -582,3 +582,74 @@ ScanForRootBridges (
 
   return RootBridges;
 }
+
+/**
+  Scan for all root bridges from PldPciRootBridgeInfoHob
+
+  @param[in]  PciRootBridgeInfo    Pointer of PLD PCI Root Bridge Info Hob
+  @param[out] NumberOfRootBridges  Number of root bridges detected
+
+  @retval     Pointer to the allocated PCI_ROOT_BRIDGE structure array.
+
+**/
+PCI_ROOT_BRIDGE *
+RetrieveRootBridgeInfoFromHob (
+  IN  PLD_PCI_ROOT_BRIDGES  *PciRootBridgeInfo,
+  OUT UINTN                 *NumberOfRootBridges
+)
+{
+  PCI_ROOT_BRIDGE                *PciRootBridges;
+  UINTN                          Size;
+  UINT8                          Index;
+
+  ASSERT (PciRootBridgeInfo != NULL);
+  ASSERT (NumberOfRootBridges != NULL);
+  if (PciRootBridgeInfo == NULL) {
+    return NULL;
+  }
+  if (PciRootBridgeInfo->Count == 0) {
+    return NULL;
+  }
+  Size = PciRootBridgeInfo->Count * sizeof (PCI_ROOT_BRIDGE);
+  PciRootBridges = (PCI_ROOT_BRIDGE *) AllocatePool (Size);
+  ASSERT (PciRootBridges != NULL);
+  if (PciRootBridges == NULL) {
+    return NULL;
+  }
+  ZeroMem (PciRootBridges, PciRootBridgeInfo->Count * sizeof (PCI_ROOT_BRIDGE));
+
+  //
+  // Create all root bridges with PciRootBridgeInfoHob
+  //
+  for (Index = 0; Index < PciRootBridgeInfo->Count; Index++) {
+    PciRootBridges[Index].Segment               = PciRootBridgeInfo->RootBridge[Index].Segment;
+    PciRootBridges[Index].Supports              = PciRootBridgeInfo->RootBridge[Index].Supports;
+    PciRootBridges[Index].Attributes            = PciRootBridgeInfo->RootBridge[Index].Attributes;
+    PciRootBridges[Index].DmaAbove4G            = PciRootBridgeInfo->RootBridge[Index].DmaAbove4G;
+    PciRootBridges[Index].NoExtendedConfigSpace = PciRootBridgeInfo->RootBridge[Index].NoExtendedConfigSpace;
+    PciRootBridges[Index].ResourceAssigned      = PciRootBridgeInfo->ResourceAssigned;
+    PciRootBridges[Index].AllocationAttributes  = PciRootBridgeInfo->RootBridge[Index].AllocationAttributes;
+    PciRootBridges[Index].DevicePath            = CreateRootBridgeDevicePath(PciRootBridgeInfo->RootBridge[Index].HID, PciRootBridgeInfo->RootBridge[Index].UID);
+    CopyMem(&PciRootBridges[Index].Bus,         &PciRootBridgeInfo->RootBridge[Index].Bus,         sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+    CopyMem(&PciRootBridges[Index].Io,          &PciRootBridgeInfo->RootBridge[Index].Io,          sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+    CopyMem(&PciRootBridges[Index].Mem,         &PciRootBridgeInfo->RootBridge[Index].Mem,         sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+    CopyMem(&PciRootBridges[Index].MemAbove4G,  &PciRootBridgeInfo->RootBridge[Index].MemAbove4G,  sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+    CopyMem(&PciRootBridges[Index].PMem,        &PciRootBridgeInfo->RootBridge[Index].PMem,        sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+    CopyMem(&PciRootBridges[Index].PMemAbove4G, &PciRootBridgeInfo->RootBridge[Index].PMemAbove4G, sizeof(PLD_PCI_ROOT_BRIDGE_APERTURE));
+  }
+
+  *NumberOfRootBridges = PciRootBridgeInfo->Count;
+
+  //
+  // Now, this library only supports RootBridge that ResourceAssigned is True
+  //
+  if (PciRootBridgeInfo->ResourceAssigned) {
+    PcdSetBoolS (PcdPciDisableBusEnumeration, TRUE);
+  } else {
+    DEBUG ((DEBUG_ERROR, "There is root bridge whose ResourceAssigned is FALSE\n"));
+    PcdSetBoolS (PcdPciDisableBusEnumeration, FALSE);
+    return NULL;
+  }
+
+  return PciRootBridges;
+}
diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 37ad5a0ae7..e9211adf86 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -323,7 +323,6 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialFifoControl|$(SERIAL_FIFO_CONTROL)
   gEfiMdeModulePkgTokenSpaceGuid.PcdSerialExtendedTxFifoSize|$(SERIAL_EXTENDED_TX_FIFO_SIZE)
 
-  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|$(UART_DEFAULT_BAUD_RATE)
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits|$(UART_DEFAULT_DATA_BITS)
   gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity|$(UART_DEFAULT_PARITY)
@@ -363,6 +362,7 @@
   gEfiMdeModulePkgTokenSpaceGuid.PcdConOutColumn|100
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress|0
   gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseSize|0
+  gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration|TRUE
 
 ################################################################################
 #
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/9] MdePkg: Add new structure for the Universal Payload SMBios Table Info Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (2 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 3/9] UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Liming Gao

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>

Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/UniversalPayload/SmbiosTable.h | 28 ++++++++++++++++++++++++++++
 MdePkg/MdePkg.dec                             |  6 ++++++
 2 files changed, 34 insertions(+)

diff --git a/MdePkg/Include/UniversalPayload/SmbiosTable.h b/MdePkg/Include/UniversalPayload/SmbiosTable.h
new file mode 100644
index 0000000000..3aaa926396
--- /dev/null
+++ b/MdePkg/Include/UniversalPayload/SmbiosTable.h
@@ -0,0 +1,28 @@
+/** @file
+ Define the structure for the Payload SmBios.
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _PLD_SMBIOS_TABL_H_
+#define _PLD_SMBIOS_TABL_H_
+
+#include <Uefi.h>
+#include <UniversalPayload/UniversalPayload.h>
+
+#pragma pack (1)
+
+typedef struct {
+  PLD_GENERIC_HEADER   PldHeader;
+  EFI_PHYSICAL_ADDRESS SmBiosEntryPoint;
+} PLD_SMBIOS_TABLE;
+
+#pragma pack()
+
+#define PLD_SMBIOS_TABLE_REVISION 1
+
+extern GUID gPldSmbios3TableGuid;
+extern GUID gPldSmbiosTableGuid;
+#endif //_PLD_SMBIOS_TABL_H_
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 3cf509728b..7a62829059 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -824,6 +824,12 @@
   ## Include/UniversalPayload/PciRootBridges.h
   gPldPciRootBridgeInfoGuid = { 0xec4ebacb, 0x2638, 0x416e, { 0xbe, 0x80, 0xe5, 0xfa, 0x4b, 0x51, 0x19, 0x01 }}
 
+  ## Include/UniversalPayload/SmbiosTable.h
+  gPldSmbios3TableGuid = { 0x92b7896c, 0x3362, 0x46ce, { 0x99, 0xb3, 0x4f, 0x5e, 0x3c, 0x34, 0xeb, 0x42 } }
+
+  ## Include/UniversalPayload/SmbiosTable.h
+  gPldSmbiosTableGuid = { 0x590a0d26, 0x06e5, 0x4d20, { 0x8a, 0x82, 0x59, 0xea, 0x1b, 0x34, 0x98, 0x2d } }
+
 [Guids.IA32, Guids.X64]
   ## Include/Guid/Cper.h
   gEfiIa32X64ErrorTypeCacheCheckGuid = { 0xA55701F5, 0xE3EF, 0x43de, { 0xAC, 0x72, 0x24, 0x9B, 0x57, 0x3F, 0xAD, 0x2C }}
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (3 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 4/9] MdePkg: Add new structure for the Universal Payload SMBios Table Info Hob Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-26  6:15   ` Wu, Hao A
  2021-05-26 13:04   ` Patrick Rudolph
  2021-05-24  7:12 ` [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob Zhiguang Liu
                   ` (4 subsequent siblings)
  9 siblings, 2 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel
  Cc: Jian J Wang, Hao A Wu, Dandan Bi, Star Zeng, Zhichao Gao,
	Patrick Rudolph

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.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Star Zeng <star.zeng@intel.com>
Cc: Zhichao Gao <zhichao.gao@intel.com>
Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 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;
+  }
+
+  //
+  // 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;
+  }
+
+  //
+  // 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
+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;
+      }
+    }
+  }
+  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


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (4 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-06-02  3:39   ` Guo Dong
  2021-05-24  7:12 ` [PATCH 7/9] MdePkg: Add new structure for the Universal Payload ACPI Table Info Hob Zhiguang Liu
                   ` (3 subsequent siblings)
  9 siblings, 1 reply; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You

>From SysTableInfo Hob, get Smbios table address, and creat gPldSmbiosTableGuid Hob
to store it. Remove diretly adding smbios table to ConfigurationTable.
Dxe module SmbiosDxe will parse it and install smbios table from it.

Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c           | 11 +----------
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf         |  3 +--
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c   | 12 +++++++++++-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h   |  3 ++-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf |  3 ++-
 5 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index a746d0581e..56b85b8e6d 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -2,7 +2,7 @@
   This driver will report some MMIO/IO resources to dxe core, extract smbios and acpi
   tables from bootloader.
 
-  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -129,15 +129,6 @@ BlDxeEntryPoint (
     ASSERT_EFI_ERROR (Status);
   }
 
-  //
-  // Install Smbios Table
-  //
-  if (SystemTableInfo->SmbiosTableBase != 0 && SystemTableInfo->SmbiosTableSize != 0) {
-    DEBUG ((DEBUG_ERROR, "Install Smbios Table at 0x%lx, length 0x%x\n", SystemTableInfo->SmbiosTableBase, SystemTableInfo->SmbiosTableSize));
-    Status = gBS->InstallConfigurationTable (&gEfiSmbiosTableGuid, (VOID *)(UINTN)SystemTableInfo->SmbiosTableBase);
-    ASSERT_EFI_ERROR (Status);
-  }
-
   //
   // Find the frame buffer information and update PCDs
   //
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
index cebc811355..30f41f8c39 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
@@ -3,7 +3,7 @@
 #
 # Report some MMIO/IO resources to dxe core, extract smbios and acpi tables
 #
-#  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -43,7 +43,6 @@
 
 [Guids]
   gEfiAcpiTableGuid
-  gEfiSmbiosTableGuid
   gUefiSystemTableInfoGuid
   gUefiAcpiBoardInfoGuid
   gEfiGraphicsInfoHobGuid
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 805f5448d9..7b71d37f94 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -1,6 +1,6 @@
 /** @file
 
-  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -234,6 +234,7 @@ BuildHobFromBl (
   EFI_PEI_GRAPHICS_INFO_HOB        *NewGfxInfo;
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB GfxDeviceInfo;
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
+  PLD_SMBIOS_TABLE                 *SmBiosTableHob;
 
   //
   // Parse memory info and build memory HOBs
@@ -276,6 +277,15 @@ BuildHobFromBl (
     DEBUG ((DEBUG_INFO, "Detected Acpi Table at 0x%lx, length 0x%x\n", SysTableInfo.AcpiTableBase, SysTableInfo.AcpiTableSize));
     DEBUG ((DEBUG_INFO, "Detected Smbios Table at 0x%lx, length 0x%x\n", SysTableInfo.SmbiosTableBase, SysTableInfo.SmbiosTableSize));
   }
+  //
+  // Creat SmBios table Hob
+  //
+  SmBiosTableHob = BuildGuidHob (&gPldSmbiosTableGuid, sizeof (PLD_SMBIOS_TABLE));
+  ASSERT (SmBiosTableHob != NULL);
+  SmBiosTableHob->PldHeader.Revision = PLD_SMBIOS_TABLE_REVISION;
+  SmBiosTableHob->PldHeader.Length = sizeof (PLD_SMBIOS_TABLE);
+  SmBiosTableHob->SmBiosEntryPoint = SysTableInfo.SmbiosTableBase;
+  DEBUG ((DEBUG_INFO, "Create smbios table gPldSmbiosTableGuid guid hob\n"));
 
   //
   // Create guid hob for acpi board information
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
index 2c84d6ed53..e7d0d15118 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
@@ -1,6 +1,6 @@
 /** @file
 *
-* Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
+* Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
 *
 *  SPDX-License-Identifier: BSD-2-Clause-Patent
 *
@@ -31,6 +31,7 @@
 #include <Guid/MemoryMapInfoGuid.h>
 #include <Guid/AcpiBoardInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>
+#include <UniversalPayload/SmbiosTable.h>
 
 
 #define LEGACY_8259_MASK_REGISTER_MASTER  0x21
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
index cc59f1903b..444f39acf3 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
@@ -1,7 +1,7 @@
 ## @file
 #  This is the first module for UEFI payload.
 #
-#  Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
 #  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
@@ -64,6 +64,7 @@
   gEfiGraphicsInfoHobGuid
   gEfiGraphicsDeviceInfoHobGuid
   gUefiAcpiBoardInfoGuid
+  gPldSmbiosTableGuid
 
 [FeaturePcd.IA32]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ## CONSUMES
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 7/9] MdePkg: Add new structure for the Universal Payload ACPI Table Info Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (5 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-24  7:12 ` [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Michael D Kinney, Liming Gao

Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdePkg/Include/UniversalPayload/AcpiTable.h | 28 ++++++++++++++++++++++++++++
 MdePkg/MdePkg.dec                           |  3 +++
 2 files changed, 31 insertions(+)

diff --git a/MdePkg/Include/UniversalPayload/AcpiTable.h b/MdePkg/Include/UniversalPayload/AcpiTable.h
new file mode 100644
index 0000000000..a85c555d9c
--- /dev/null
+++ b/MdePkg/Include/UniversalPayload/AcpiTable.h
@@ -0,0 +1,28 @@
+/** @file
+ Define the structure for the Payload APCI table.
+
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef _PLD_ACPI_TABLE_H_
+#define _PLD_ACPI_TABLE_H_
+
+#include <Uefi.h>
+#include <UniversalPayload/UniversalPayload.h>
+
+#pragma pack(1)
+
+typedef struct {
+  PLD_GENERIC_HEADER   PldHeader;
+  EFI_PHYSICAL_ADDRESS Rsdp;
+} PLD_ACPI_TABLE;
+
+#pragma pack()
+
+#define PLD_ACPI_TABLE_REVISION 1
+
+extern GUID gPldAcpiTableGuid;
+
+#endif //_PLD_ACPI_TABLE_H_
diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec
index 7a62829059..42a7fa2155 100644
--- a/MdePkg/MdePkg.dec
+++ b/MdePkg/MdePkg.dec
@@ -830,6 +830,9 @@
   ## Include/UniversalPayload/SmbiosTable.h
   gPldSmbiosTableGuid = { 0x590a0d26, 0x06e5, 0x4d20, { 0x8a, 0x82, 0x59, 0xea, 0x1b, 0x34, 0x98, 0x2d } }
 
+  ## Include/UniversalPayload/AcpiTable.h
+  gPldAcpiTableGuid = { 0x9f9a9506, 0x5597, 0x4515, { 0xba, 0xb6, 0x8b, 0xcd, 0xe7, 0x84, 0xba, 0x87 } }
+
 [Guids.IA32, Guids.X64]
   ## Include/Guid/Cper.h
   gEfiIa32X64ErrorTypeCacheCheckGuid = { 0xA55701F5, 0xE3EF, 0x43de, { 0xAC, 0x72, 0x24, 0x9B, 0x57, 0x3F, 0xAD, 0x2C }}
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB.
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (6 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 7/9] MdePkg: Add new structure for the Universal Payload ACPI Table Info Hob Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-27  0:42   ` Wu, Hao A
  2021-05-24  7:12 ` [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob Zhiguang Liu
  2021-05-28  3:00 ` 回复: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload gaoliming
  9 siblings, 1 reply; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Jian J Wang, Hao A Wu, Dandan Bi, Liming Gao, Ray Ni

If HOB contains APCI table information, entry point of AcpiTableDxe.inf
should parse the APCI table from HOB, and install these tables.
We assume the whole ACPI table (starting with EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
is contained by a single gEfiAcpiTableGuid HOB.

Cc: Jian J Wang <jian.j.wang@intel.com>
Cc: Hao A Wu <hao.a.wu@intel.com>
Cc: Dandan Bi <dandan.bi@intel.com>
Cc: Liming Gao <gaoliming@byosoft.com.cn>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h         |   4 +++-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf    |   4 +++-
 MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 144 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--------
 3 files changed, 142 insertions(+), 10 deletions(-)

diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
index 9d7cf7ccfc..7fd393aab3 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
@@ -1,7 +1,7 @@
 /** @file
   ACPI Table Protocol Driver
 
-  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -24,6 +24,8 @@
 #include <Library/MemoryAllocationLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <Library/PcdLib.h>
+#include <Library/HobLib.h>
+#include <UniversalPayload/AcpiTable.h>
 
 //
 // Statements that include other files
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
index d341df439e..df80c4db35 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
@@ -4,7 +4,7 @@
 #  This driver initializes ACPI tables (Rsdp, Rsdt and Xsdt) and produces UEFI/PI
 #  services to install/uninstall/manage ACPI tables.
 #
-#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+#  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
 #  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -51,10 +51,12 @@
   DebugLib
   BaseLib
   PcdLib
+  HobLib
 
 [Guids]
   gEfiAcpi10TableGuid                           ## PRODUCES ## SystemTable
   gEfiAcpiTableGuid                             ## PRODUCES ## SystemTable
+  gPldAcpiTableGuid
 
 [FeaturePcd]
   gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol  ## CONSUMES
diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
index 5a2afdff27..24962843a1 100644
--- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
+++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
@@ -1,7 +1,7 @@
 /** @file
   ACPI Table Protocol Implementation
 
-  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
+  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
   Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -30,6 +30,7 @@ STATIC EFI_ALLOCATE_TYPE      mAcpiTableAllocType;
   @param  Table                     Table to add.
   @param  Checksum                  Does the table require checksumming.
   @param  Version                   The version of the list to add the table to.
+  @param  IsFromHob                 True, if add Apci Table from Hob List.
   @param  Handle                    Pointer for returning the handle.
 
   @return EFI_SUCCESS               The function completed successfully.
@@ -44,6 +45,7 @@ AddTableToList (
   IN VOID                                 *Table,
   IN BOOLEAN                              Checksum,
   IN EFI_ACPI_TABLE_VERSION               Version,
+  IN BOOLEAN                              IsFromHob,
   OUT UINTN                               *Handle
   );
 
@@ -238,6 +240,7 @@ InstallAcpiTable (
              AcpiTableBufferConst,
              TRUE,
              Version,
+             FALSE,
              TableKey
              );
   if (!EFI_ERROR (Status)) {
@@ -472,6 +475,7 @@ FreeTableMemory (
   @param  Table                     Table to add.
   @param  Checksum                  Does the table require checksumming.
   @param  Version                   The version of the list to add the table to.
+  @param  IsFromHob                 True, if add Apci Table from Hob List.
   @param  Handle                    Pointer for returning the handle.
 
   @return EFI_SUCCESS               The function completed successfully.
@@ -487,6 +491,7 @@ AddTableToList (
   IN VOID                                 *Table,
   IN BOOLEAN                              Checksum,
   IN EFI_ACPI_TABLE_VERSION               Version,
+  IN BOOLEAN                              IsFromHob,
   OUT UINTN                               *Handle
   )
 {
@@ -552,13 +557,16 @@ AddTableToList (
     // could be updated by OS present agent. For example, BufferPtrAddress in
     // SMM communication ACPI table.
     //
-    ASSERT ((EFI_PAGE_SIZE % 64) == 0);
-    Status = gBS->AllocatePages (
-                    AllocateMaxAddress,
-                    EfiACPIMemoryNVS,
-                    EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
-                    &AllocPhysAddress
-                    );
+    if (IsFromHob){
+      AllocPhysAddress = (UINTN)Table;
+    } else {
+      Status = gBS->AllocatePages (
+                      AllocateMaxAddress,
+                      EfiACPIMemoryNVS,
+                      EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
+                      &AllocPhysAddress
+                      );
+    }
   } else if (mAcpiTableAllocType == AllocateAnyPages) {
     //
     // If there is no allocation limit, there is also no need to use page
@@ -1689,6 +1697,124 @@ ChecksumCommonTables (
   return EFI_SUCCESS;
 }
 
+/**
+  This function will find gPldAcpiTableGuid Guid Hob, and install Acpi table from it.
+
+  @param  AcpiTableInstance  Protocol instance private data.
+
+  @return EFI_SUCCESS        The function completed successfully.
+  @return EFI_NOT_FOUND      The function doesn't find the gEfiAcpiTableGuid Guid Hob.
+  @return EFI_ABORTED        The function could not complete successfully.
+
+**/
+EFI_STATUS
+InstallAcpiTableFromHob (
+  EFI_ACPI_TABLE_INSTANCE                   *AcpiTableInstance
+  )
+{
+  EFI_HOB_GUID_TYPE                             *GuidHob;
+  EFI_ACPI_TABLE_VERSION                        Version;
+  EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
+  EFI_ACPI_DESCRIPTION_HEADER                   *Rsdt;
+  EFI_ACPI_DESCRIPTION_HEADER                   *ChildTable;
+  UINT64                                        ChildTableAddress;
+  UINTN                                         Count;
+  UINTN                                         Index;
+  UINTN                                         TableKey;
+  EFI_STATUS                                    Status;
+  UINTN                                         EntrySize;
+  PLD_ACPI_TABLE                                *AcpiTableAdress;
+  VOID                                          *TableToInstall;
+  PLD_GENERIC_HEADER                            *GenericHeader;
+
+  TableKey = 0;
+  Version = PcdGet32 (PcdAcpiExposedTableVersions);
+
+  //
+  // HOB only contains the ACPI table in 2.0+ format.
+  //
+  GuidHob = GetFirstGuidHob (&gPldAcpiTableGuid);
+  if (GuidHob == NULL) {
+    return EFI_NOT_FOUND;
+  }
+
+  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))) {
+    return EFI_NOT_FOUND;
+  }
+  if (GenericHeader->Revision == PLD_ACPI_TABLE_REVISION) {
+    //
+    // PLD_ACPI_TABLE structure is used when Revision equals to PLD_ACPI_TABLE_REVISION
+    //
+    AcpiTableAdress = (PLD_ACPI_TABLE *) GET_GUID_HOB_DATA (GuidHob);
+    if (GenericHeader->Length < PLD_SIZEOF_THROUGH_FIELD (PLD_ACPI_TABLE, Rsdp)) {
+      //
+      // Retrun if can't find the ACPI Info Hob with enough length
+      //
+      return EFI_NOT_FOUND;
+    }
+    Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) (UINTN) (AcpiTableAdress->Rsdp);
+
+    //
+    // An ACPI-compatible OS must use the XSDT if present.
+    // It shouldn't happen that XsdtAddress points beyond 4G range in 32-bit environment.
+    //
+    ASSERT ((UINTN) Rsdp->XsdtAddress == Rsdp->XsdtAddress);
+
+    EntrySize = sizeof (UINT64);
+    Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress;
+    if (Rsdt == NULL) {
+      //
+      // XsdtAddress is zero, then we use Rsdt which has 32 bit entry
+      //
+      Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress;
+      EntrySize = sizeof (UINT32);
+    }
+    Count = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) / EntrySize;
+
+    for (Index = 0; Index < Count; Index++){
+      ChildTableAddress = 0;
+      CopyMem (&ChildTableAddress, (UINT8 *) (Rsdt + 1) + EntrySize * Index, EntrySize);
+      //
+      // If the address is of UINT64 while this module runs at 32 bits,
+      // make sure the upper bits are all-zeros.
+      //
+      ASSERT (ChildTableAddress == (UINTN) ChildTableAddress);
+
+      ChildTable = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) ChildTableAddress;
+      Status = AddTableToList (AcpiTableInstance, ChildTable, TRUE, Version, TRUE, &TableKey);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI table at 0x%p\n", ChildTable));
+        ASSERT_EFI_ERROR (Status);
+      }
+      if (ChildTable->Signature == EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE){
+        //
+        // Add the FACS and DSDT tables if it is not NULL.
+        //
+        if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->FirmwareCtrl != 0) {
+          TableToInstall = (VOID *) (UINTN) ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->FirmwareCtrl;
+          Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE, Version, TRUE, &TableKey);
+          if (EFI_ERROR (Status)) {
+            DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI table FACS\n"));
+            ASSERT_EFI_ERROR (Status);
+          }
+        }
+
+        if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->Dsdt != 0) {
+          TableToInstall = (VOID *) (UINTN) ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->Dsdt;
+          Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE, Version, TRUE, &TableKey);
+          if (EFI_ERROR (Status)) {
+            DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI table DSDT\n"));
+            ASSERT_EFI_ERROR (Status);
+          }
+        }
+      }
+    }
+  }
+  Status = PublishTables (AcpiTableInstance, Version);
+  ASSERT_EFI_ERROR (Status);
+  return Status;
+}
 
 /**
   Constructor for the ACPI table protocol.  Initializes instance
@@ -1918,6 +2044,8 @@ AcpiTableAcpiTableConstructor (
 
   ChecksumCommonTables (AcpiTableInstance);
 
+  InstallAcpiTableFromHob (AcpiTableInstance);
+
   //
   // Completed successfully
   //
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (7 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
@ 2021-05-24  7:12 ` Zhiguang Liu
  2021-05-26 13:50   ` [edk2-devel] " Patrick Rudolph
  2021-05-28  3:00 ` 回复: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload gaoliming
  9 siblings, 1 reply; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-24  7:12 UTC (permalink / raw)
  To: devel; +Cc: Maurice Ma, Guo Dong, Benjamin You, Ray Ni

>From SysTableInfo Hob, get ACPI table address, and creat gPldAcpiTableGuid Hob
to store it. Remove diretly adding ACPI table to ConfigurationTable.
Dxe ACPI driver will parse it and install ACPI table from Guid Hob.

Cc: Maurice Ma <maurice.ma@intel.com>
Cc: Guo Dong <guo.dong@intel.com>
Cc: Benjamin You <benjamin.you@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
---
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c           | 17 -----------------
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h           |  5 +----
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf         |  1 -
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c   | 11 +++++++++++
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h   |  2 +-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf |  1 +
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index 56b85b8e6d..ffd3427fb3 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -99,7 +99,6 @@ BlDxeEntryPoint (
 {
   EFI_STATUS Status;
   EFI_HOB_GUID_TYPE          *GuidHob;
-  SYSTEM_TABLE_INFO          *SystemTableInfo;
   EFI_PEI_GRAPHICS_INFO_HOB  *GfxInfo;
   ACPI_BOARD_INFO            *AcpiBoardInfo;
 
@@ -113,22 +112,6 @@ BlDxeEntryPoint (
   Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
   ASSERT_EFI_ERROR (Status);
 
-  //
-  // Find the system table information guid hob
-  //
-  GuidHob = GetFirstGuidHob (&gUefiSystemTableInfoGuid);
-  ASSERT (GuidHob != NULL);
-  SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
-
-  //
-  // Install Acpi Table
-  //
-  if (SystemTableInfo->AcpiTableBase != 0 && SystemTableInfo->AcpiTableSize != 0) {
-    DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n", SystemTableInfo->AcpiTableBase, SystemTableInfo->AcpiTableSize));
-    Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID *)(UINTN)SystemTableInfo->AcpiTableBase);
-    ASSERT_EFI_ERROR (Status);
-  }
-
   //
   // Find the frame buffer information and update PCDs
   //
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
index 512105fafd..3332a30eae 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
@@ -1,7 +1,7 @@
 /** @file
   The header file of bootloader support DXE.
 
-Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent
 
 **/
@@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/IoLib.h>
 #include <Library/HobLib.h>
 
-#include <Guid/Acpi.h>
 #include <Guid/SmBios.h>
 #include <Guid/SystemTableInfoGuid.h>
 #include <Guid/AcpiBoardInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>
 
-#include <IndustryStandard/Acpi.h>
-
 #endif
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
index 30f41f8c39..1ccb250991 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
@@ -42,7 +42,6 @@
   HobLib
 
 [Guids]
-  gEfiAcpiTableGuid
   gUefiSystemTableInfoGuid
   gUefiAcpiBoardInfoGuid
   gEfiGraphicsInfoHobGuid
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 7b71d37f94..14b7a732da 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -235,6 +235,7 @@ BuildHobFromBl (
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB GfxDeviceInfo;
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
   PLD_SMBIOS_TABLE                 *SmBiosTableHob;
+  PLD_ACPI_TABLE                   *AcpiTableHob;
 
   //
   // Parse memory info and build memory HOBs
@@ -287,6 +288,16 @@ BuildHobFromBl (
   SmBiosTableHob->SmBiosEntryPoint = SysTableInfo.SmbiosTableBase;
   DEBUG ((DEBUG_INFO, "Create smbios table gPldSmbiosTableGuid guid hob\n"));
 
+  // 
+  // Creat ACPI table Hob
+  //
+  AcpiTableHob = BuildGuidHob (&gPldAcpiTableGuid, sizeof (PLD_ACPI_TABLE));
+  ASSERT (AcpiTableHob != NULL);
+  AcpiTableHob->PldHeader.Revision = PLD_ACPI_TABLE_REVISION;
+  AcpiTableHob->PldHeader.Length = sizeof (PLD_ACPI_TABLE);
+  AcpiTableHob->Rsdp = SysTableInfo.AcpiTableBase;
+  DEBUG ((DEBUG_INFO, "Create smbios table gPldAcpiTableGuid guid hob\n"));
+
   //
   // Create guid hob for acpi board information
   //
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
index e7d0d15118..a4c9da128e 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
@@ -32,7 +32,7 @@
 #include <Guid/AcpiBoardInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>
 #include <UniversalPayload/SmbiosTable.h>
-
+#include <UniversalPayload/AcpiTable.h>
 
 #define LEGACY_8259_MASK_REGISTER_MASTER  0x21
 #define LEGACY_8259_MASK_REGISTER_SLAVE   0xA1
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
index 444f39acf3..01388b8831 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
@@ -65,6 +65,7 @@
   gEfiGraphicsDeviceInfoHobGuid
   gUefiAcpiBoardInfoGuid
   gPldSmbiosTableGuid
+  gPldAcpiTableGuid
 
 [FeaturePcd.IA32]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ## CONSUMES
-- 
2.30.0.windows.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  2021-05-24  7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
@ 2021-05-26  6:15   ` Wu, Hao A
  2021-05-26  6:32     ` [edk2-devel] " Wu, Hao A
  2021-05-26 13:04   ` Patrick Rudolph
  1 sibling, 1 reply; 19+ messages in thread
From: Wu, Hao A @ 2021-05-26  6:15 UTC (permalink / raw)
  To: Liu, Zhiguang, Ni, Ray, devel@edk2.groups.io
  Cc: Wang, Jian J, Bi, Dandan, Zeng, Star, Gao, Zhichao,
	Patrick Rudolph

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.liu@intel.com>
> Sent: Monday, May 24, 2021 3:13 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao,
> Zhichao <zhichao.gao@intel.com>; Patrick Rudolph
> <patrick.rudolph@9elements.com>
> Subject: [PATCH 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.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [edk2-devel] [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  2021-05-26  6:15   ` Wu, Hao A
@ 2021-05-26  6:32     ` Wu, Hao A
  2021-05-26  8:56       ` Zhiguang Liu
  0 siblings, 1 reply; 19+ messages in thread
From: Wu, Hao A @ 2021-05-26  6:32 UTC (permalink / raw)
  To: devel@edk2.groups.io, Wu, Hao A, Liu, Zhiguang, Ni, Ray
  Cc: Wang, Jian J, Bi, Dandan, Zeng, Star, Gao, Zhichao,
	Patrick Rudolph

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.liu@intel.com>; Ni, Ray <ray.ni@intel.com>;
> devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Patrick Rudolph
> <patrick.rudolph@9elements.com>
> Subject: 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.liu@intel.com>
> > Sent: Monday, May 24, 2021 3:13 PM
> > To: devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng, Star
> > <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Patrick
> > Rudolph <patrick.rudolph@9elements.com>
> > Subject: [PATCH 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.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Dandan Bi <dandan.bi@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > ---
> >  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 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
> 
> 
> 
> 
> 


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [edk2-devel] [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  2021-05-26  6:32     ` [edk2-devel] " Wu, Hao A
@ 2021-05-26  8:56       ` Zhiguang Liu
  0 siblings, 0 replies; 19+ messages in thread
From: Zhiguang Liu @ 2021-05-26  8:56 UTC (permalink / raw)
  To: Wu, Hao A, devel@edk2.groups.io, Ni, Ray
  Cc: Wang, Jian J, Bi, Dandan, Zeng, Star, Gao, Zhichao,
	Patrick Rudolph

Hi Hao,

Thanks for the comments. I will send another version soon.

Thanks
Zhiguang

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Wednesday, May 26, 2021 2:33 PM
> To: devel@edk2.groups.io; Wu, Hao A <hao.a.wu@intel.com>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Ni, Ray <ray.ni@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Bi, Dandan
> <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Patrick Rudolph
> <patrick.rudolph@9elements.com>
> Subject: RE: [edk2-devel] [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe:
> Scan for existing tables
> 
> 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.liu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; devel@edk2.groups.io
> > Cc: Wang, Jian J <jian.j.wang@intel.com>; Bi, Dandan
> > <dandan.bi@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Zhichao
> > <zhichao.gao@intel.com>; Patrick Rudolph
> > <patrick.rudolph@9elements.com>
> > Subject: 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.liu@intel.com>
> > > Sent: Monday, May 24, 2021 3:13 PM
> > > To: devel@edk2.groups.io
> > > Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A
> > > <hao.a.wu@intel.com>; Bi, Dandan <dandan.bi@intel.com>; Zeng, Star
> > > <star.zeng@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Patrick
> > > Rudolph <patrick.rudolph@9elements.com>
> > > Subject: [PATCH 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.wang@intel.com>
> > > Cc: Hao A Wu <hao.a.wu@intel.com>
> > > Cc: Dandan Bi <dandan.bi@intel.com>
> > > Cc: Star Zeng <star.zeng@intel.com>
> > > Cc: Zhichao Gao <zhichao.gao@intel.com>
> > > Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> > > ---
> > >  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 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
> >
> >
> >
> > 
> >


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [edk2-devel] [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
  2021-05-24  7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
  2021-05-26  6:15   ` Wu, Hao A
@ 2021-05-26 13:04   ` Patrick Rudolph
  1 sibling, 0 replies; 19+ messages in thread
From: Patrick Rudolph @ 2021-05-26 13:04 UTC (permalink / raw)
  To: devel, Liu, Zhiguang
  Cc: Jian J Wang, Hao A Wu, Dandan Bi, Star Zeng, Zhichao Gao

[-- Attachment #1: Type: text/plain, Size: 15478 bytes --]

On Mon, May 24, 2021 at 9:13 AM Zhiguang Liu <zhiguang.liu@intel.com> wrote:

> 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.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Star Zeng <star.zeng@intel.com>
> Cc: Zhichao Gao <zhichao.gao@intel.com>
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c   | 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;
> +  }
> +
> +  //
> +  // 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;
> +  }
> +
>

This is not correct, it should be

if (SmbiosTable->EntryPointLength != 0x1E && SmbiosTable->EntryPointLength
!= sizeof (SMBIOS_TABLE_ENTRY_POINT)) {

otherwise the table wouldn't be recognized as valid.

+  //
> +  // MajorVersion should be 2.
> +  //
> +  if (SmbiosTable->MajorVersion != 2) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // The whole struct check sum should be zero
> +  //
> +  Checksum = CalculateSum8 (
> +               (UINT8 *) SmbiosTable,
> +               SmbiosTable->EntryPointLength
> +               );
> +  if (Checksum != 0) {
> +    return FALSE;
> +  }
> +
> +  //
> +  // The Intermediate Entry Point Structure check sum should be zero.
> +  //
> +  Checksum = CalculateSum8 (
> +               (UINT8 *) SmbiosTable + OFFSET_OF
> (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString),
> +               SmbiosTable->EntryPointLength - OFFSET_OF
> (SMBIOS_TABLE_ENTRY_POINT, IntermediateAnchorString)
> +               );
> +  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
> +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;
> +      }
> +    }
> +  }
> +  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 (#75489): https://edk2.groups.io/g/devel/message/75489
> Mute This Topic: https://groups.io/mt/83045509/2917327
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [
> patrick.rudolph@9elements.com]
> ------------
>
>
>

[-- Attachment #2: Type: text/html, Size: 18833 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [edk2-devel] [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob
  2021-05-24  7:12 ` [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob Zhiguang Liu
@ 2021-05-26 13:50   ` Patrick Rudolph
  2021-06-02  3:47     ` Guo Dong
  0 siblings, 1 reply; 19+ messages in thread
From: Patrick Rudolph @ 2021-05-26 13:50 UTC (permalink / raw)
  To: devel, Liu, Zhiguang; +Cc: Maurice Ma, Guo Dong, Benjamin You, Ray Ni

[-- Attachment #1: Type: text/plain, Size: 6641 bytes --]

On Mon, May 24, 2021 at 9:13 AM Zhiguang Liu <zhiguang.liu@intel.com> wrote:

> From SysTableInfo Hob, get ACPI table address, and creat gPldAcpiTableGuid
> Hob
> to store it. Remove diretly adding ACPI table to ConfigurationTable.
> Dxe ACPI driver will parse it and install ACPI table from Guid Hob.
>
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c           | 17
> -----------------
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h           |  5 +----
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf         |  1 -
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c   | 11 +++++++++++
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h   |  2 +-
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf |  1 +
>  6 files changed, 14 insertions(+), 23 deletions(-)
>
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> index 56b85b8e6d..ffd3427fb3 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> @@ -99,7 +99,6 @@ BlDxeEntryPoint (
>  {
>    EFI_STATUS Status;
>    EFI_HOB_GUID_TYPE          *GuidHob;
> -  SYSTEM_TABLE_INFO          *SystemTableInfo;
>    EFI_PEI_GRAPHICS_INFO_HOB  *GfxInfo;
>    ACPI_BOARD_INFO            *AcpiBoardInfo;
>
> @@ -113,22 +112,6 @@ BlDxeEntryPoint (
>    Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo,
> 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
>    ASSERT_EFI_ERROR (Status);
>
> -  //
> -  // Find the system table information guid hob
> -  //
> -  GuidHob = GetFirstGuidHob (&gUefiSystemTableInfoGuid);
> -  ASSERT (GuidHob != NULL);
> -  SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
> -
> -  //
> -  // Install Acpi Table
> -  //
> -  if (SystemTableInfo->AcpiTableBase != 0 &&
> SystemTableInfo->AcpiTableSize != 0) {
> -    DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n",
> SystemTableInfo->AcpiTableBase, SystemTableInfo->AcpiTableSize));
> -    Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID
> *)(UINTN)SystemTableInfo->AcpiTableBase);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> -
>

Note that AcpiTableDxe.inf is currently not part of the FV on
UefipayloadPkg, so there won't be any tables installed after all.

   //
>    // Find the frame buffer information and update PCDs
>    //
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> index 512105fafd..3332a30eae 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
> @@ -1,7 +1,7 @@
>  /** @file
>    The header file of bootloader support DXE.
>
> -Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
>  SPDX-License-Identifier: BSD-2-Clause-Patent
>
>  **/
> @@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>  #include <Library/IoLib.h>
>  #include <Library/HobLib.h>
>
> -#include <Guid/Acpi.h>
>  #include <Guid/SmBios.h>
>  #include <Guid/SystemTableInfoGuid.h>
>  #include <Guid/AcpiBoardInfoGuid.h>
>  #include <Guid/GraphicsInfoHob.h>
>
> -#include <IndustryStandard/Acpi.h>
> -
>  #endif
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> index 30f41f8c39..1ccb250991 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> @@ -42,7 +42,6 @@
>    HobLib
>
>  [Guids]
> -  gEfiAcpiTableGuid
>    gUefiSystemTableInfoGuid
>    gUefiAcpiBoardInfoGuid
>    gEfiGraphicsInfoHobGuid
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> index 7b71d37f94..14b7a732da 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> @@ -235,6 +235,7 @@ BuildHobFromBl (
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB GfxDeviceInfo;
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
>    PLD_SMBIOS_TABLE                 *SmBiosTableHob;
> +  PLD_ACPI_TABLE                   *AcpiTableHob;
>
>    //
>    // Parse memory info and build memory HOBs
> @@ -287,6 +288,16 @@ BuildHobFromBl (
>    SmBiosTableHob->SmBiosEntryPoint = SysTableInfo.SmbiosTableBase;
>    DEBUG ((DEBUG_INFO, "Create smbios table gPldSmbiosTableGuid guid
> hob\n"));
>
> +  //
> +  // Creat ACPI table Hob
> +  //
> +  AcpiTableHob = BuildGuidHob (&gPldAcpiTableGuid, sizeof
> (PLD_ACPI_TABLE));
> +  ASSERT (AcpiTableHob != NULL);
> +  AcpiTableHob->PldHeader.Revision = PLD_ACPI_TABLE_REVISION;
> +  AcpiTableHob->PldHeader.Length = sizeof (PLD_ACPI_TABLE);
> +  AcpiTableHob->Rsdp = SysTableInfo.AcpiTableBase;
> +  DEBUG ((DEBUG_INFO, "Create smbios table gPldAcpiTableGuid guid
> hob\n"));
> +
>

 DEBUG ((DEBUG_INFO, "Create acpi table ...

   //
>    // Create guid hob for acpi board information
>    //
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> index e7d0d15118..a4c9da128e 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> @@ -32,7 +32,7 @@
>  #include <Guid/AcpiBoardInfoGuid.h>
>  #include <Guid/GraphicsInfoHob.h>
>  #include <UniversalPayload/SmbiosTable.h>
> -
> +#include <UniversalPayload/AcpiTable.h>
>
>  #define LEGACY_8259_MASK_REGISTER_MASTER  0x21
>  #define LEGACY_8259_MASK_REGISTER_SLAVE   0xA1
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> index 444f39acf3..01388b8831 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> @@ -65,6 +65,7 @@
>    gEfiGraphicsDeviceInfoHobGuid
>    gUefiAcpiBoardInfoGuid
>    gPldSmbiosTableGuid
> +  gPldAcpiTableGuid
>
>  [FeaturePcd.IA32]
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ##
> CONSUMES
> --
> 2.30.0.windows.2
>
>
>
> ------------
> Groups.io Links: You receive all messages sent to this group.
> View/Reply Online (#75493): https://edk2.groups.io/g/devel/message/75493
> Mute This Topic: https://groups.io/mt/83045527/2917327
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [
> patrick.rudolph@9elements.com]
> ------------
>
>
>

[-- Attachment #2: Type: text/html, Size: 8821 bytes --]

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB.
  2021-05-24  7:12 ` [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
@ 2021-05-27  0:42   ` Wu, Hao A
  0 siblings, 0 replies; 19+ messages in thread
From: Wu, Hao A @ 2021-05-27  0:42 UTC (permalink / raw)
  To: Liu, Zhiguang, devel@edk2.groups.io
  Cc: Wang, Jian J, Bi, Dandan, Liming Gao, Ni, Ray

Some inline comments below:


> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Monday, May 24, 2021 3:13 PM
> To: devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>;
> Bi, Dandan <dandan.bi@intel.com>; Liming Gao
> <gaoliming@byosoft.com.cn>; Ni, Ray <ray.ni@intel.com>
> Subject: [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB.
> 
> If HOB contains APCI table information, entry point of AcpiTableDxe.inf
> should parse the APCI table from HOB, and install these tables.
> We assume the whole ACPI table (starting with
> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER)
> is contained by a single gEfiAcpiTableGuid HOB.
> 
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Ray Ni <ray.ni@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h         |   4 +++-
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf    |   4 +++-
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c | 144
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++--------
>  3 files changed, 142 insertions(+), 10 deletions(-)
> 
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> index 9d7cf7ccfc..7fd393aab3 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h
> @@ -1,7 +1,7 @@
>  /** @file
> 
>    ACPI Table Protocol Driver
> 
> 
> 
> -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -24,6 +24,8 @@
>  #include <Library/MemoryAllocationLib.h>
> 
>  #include <Library/UefiBootServicesTableLib.h>
> 
>  #include <Library/PcdLib.h>
> 
> +#include <Library/HobLib.h>
> 
> +#include <UniversalPayload/AcpiTable.h>
> 
> 
> 
>  //
> 
>  // Statements that include other files
> 
> diff --git a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> index d341df439e..df80c4db35 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf
> @@ -4,7 +4,7 @@
>  #  This driver initializes ACPI tables (Rsdp, Rsdt and Xsdt) and produces
> UEFI/PI
> 
>  #  services to install/uninstall/manage ACPI tables.
> 
>  #
> 
> -#  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +#  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  #  Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #
> 
> @@ -51,10 +51,12 @@
>    DebugLib
> 
>    BaseLib
> 
>    PcdLib
> 
> +  HobLib
> 
> 
> 
>  [Guids]
> 
>    gEfiAcpi10TableGuid                           ## PRODUCES ## SystemTable
> 
>    gEfiAcpiTableGuid                             ## PRODUCES ## SystemTable
> 
> +  gPldAcpiTableGuid


Please help to add "## SOMETIMES_CONSUMES ## HOB" as simple description of the GUID usage.


> 
> 
> 
>  [FeaturePcd]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol  ##
> CONSUMES
> 
> diff --git
> a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> index 5a2afdff27..24962843a1 100644
> --- a/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> +++ b/MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c
> @@ -1,7 +1,7 @@
>  /** @file
> 
>    ACPI Table Protocol Implementation
> 
> 
> 
> -  Copyright (c) 2006 - 2018, Intel Corporation. All rights reserved.<BR>
> 
> +  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>    Copyright (c) 2016, Linaro Ltd. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
> @@ -30,6 +30,7 @@ STATIC EFI_ALLOCATE_TYPE      mAcpiTableAllocType;
>    @param  Table                     Table to add.
> 
>    @param  Checksum                  Does the table require checksumming.
> 
>    @param  Version                   The version of the list to add the table to.
> 
> +  @param  IsFromHob                 True, if add Apci Table from Hob List.
> 
>    @param  Handle                    Pointer for returning the handle.
> 
> 
> 
>    @return EFI_SUCCESS               The function completed successfully.
> 
> @@ -44,6 +45,7 @@ AddTableToList (
>    IN VOID                                 *Table,
> 
>    IN BOOLEAN                              Checksum,
> 
>    IN EFI_ACPI_TABLE_VERSION               Version,
> 
> +  IN BOOLEAN                              IsFromHob,
> 
>    OUT UINTN                               *Handle
> 
>    );
> 
> 
> 
> @@ -238,6 +240,7 @@ InstallAcpiTable (
>               AcpiTableBufferConst,
> 
>               TRUE,
> 
>               Version,
> 
> +             FALSE,
> 
>               TableKey
> 
>               );
> 
>    if (!EFI_ERROR (Status)) {
> 
> @@ -472,6 +475,7 @@ FreeTableMemory (
>    @param  Table                     Table to add.
> 
>    @param  Checksum                  Does the table require checksumming.
> 
>    @param  Version                   The version of the list to add the table to.
> 
> +  @param  IsFromHob                 True, if add Apci Table from Hob List.
> 
>    @param  Handle                    Pointer for returning the handle.
> 
> 
> 
>    @return EFI_SUCCESS               The function completed successfully.
> 
> @@ -487,6 +491,7 @@ AddTableToList (
>    IN VOID                                 *Table,
> 
>    IN BOOLEAN                              Checksum,
> 
>    IN EFI_ACPI_TABLE_VERSION               Version,
> 
> +  IN BOOLEAN                              IsFromHob,
> 
>    OUT UINTN                               *Handle
> 
>    )
> 
>  {
> 
> @@ -552,13 +557,16 @@ AddTableToList (
>      // could be updated by OS present agent. For example, BufferPtrAddress
> in
> 
>      // SMM communication ACPI table.
> 
>      //
> 
> -    ASSERT ((EFI_PAGE_SIZE % 64) == 0);


Is there any special consideration for removing the above check when the table is not from HOB?
Also, will the tables from HOB violate the rule mentioned in the below comment?
    // Allocate memory for the FACS.  This structure must be aligned
    // on a 64 byte boundary and must be ACPI NVS memory.


> 
> -    Status = gBS->AllocatePages (
> 
> -                    AllocateMaxAddress,
> 
> -                    EfiACPIMemoryNVS,
> 
> -                    EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
> 
> -                    &AllocPhysAddress
> 
> -                    );
> 
> +    if (IsFromHob){
> 
> +      AllocPhysAddress = (UINTN)Table;
> 
> +    } else {
> 
> +      Status = gBS->AllocatePages (
> 
> +                      AllocateMaxAddress,
> 
> +                      EfiACPIMemoryNVS,
> 
> +                      EFI_SIZE_TO_PAGES (CurrentTableList->TableSize),
> 
> +                      &AllocPhysAddress
> 
> +                      );
> 
> +    }
> 
>    } else if (mAcpiTableAllocType == AllocateAnyPages) {
> 
>      //
> 
>      // If there is no allocation limit, there is also no need to use page
> 
> @@ -1689,6 +1697,124 @@ ChecksumCommonTables (
>    return EFI_SUCCESS;
> 
>  }
> 
> 
> 
> +/**
> 
> +  This function will find gPldAcpiTableGuid Guid Hob, and install Acpi table
> from it.
> 
> +
> 
> +  @param  AcpiTableInstance  Protocol instance private data.
> 
> +
> 
> +  @return EFI_SUCCESS        The function completed successfully.
> 
> +  @return EFI_NOT_FOUND      The function doesn't find the
> gEfiAcpiTableGuid Guid Hob.
> 
> +  @return EFI_ABORTED        The function could not complete successfully.
> 
> +
> 
> +**/
> 
> +EFI_STATUS
> 
> +InstallAcpiTableFromHob (
> 
> +  EFI_ACPI_TABLE_INSTANCE                   *AcpiTableInstance
> 
> +  )
> 
> +{
> 
> +  EFI_HOB_GUID_TYPE                             *GuidHob;
> 
> +  EFI_ACPI_TABLE_VERSION                        Version;
> 
> +  EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
> 
> +  EFI_ACPI_DESCRIPTION_HEADER                   *Rsdt;
> 
> +  EFI_ACPI_DESCRIPTION_HEADER                   *ChildTable;
> 
> +  UINT64                                        ChildTableAddress;
> 
> +  UINTN                                         Count;
> 
> +  UINTN                                         Index;
> 
> +  UINTN                                         TableKey;
> 
> +  EFI_STATUS                                    Status;
> 
> +  UINTN                                         EntrySize;
> 
> +  PLD_ACPI_TABLE                                *AcpiTableAdress;
> 
> +  VOID                                          *TableToInstall;
> 
> +  PLD_GENERIC_HEADER                            *GenericHeader;
> 
> +
> 
> +  TableKey = 0;
> 
> +  Version = PcdGet32 (PcdAcpiExposedTableVersions);
> 
> +
> 
> +  //
> 
> +  // HOB only contains the ACPI table in 2.0+ format.
> 
> +  //
> 
> +  GuidHob = GetFirstGuidHob (&gPldAcpiTableGuid);
> 
> +  if (GuidHob == NULL) {
> 
> +    return EFI_NOT_FOUND;
> 
> +  }
> 
> +
> 
> +  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))) {
> 
> +    return EFI_NOT_FOUND;
> 
> +  }
> 
> +  if (GenericHeader->Revision == PLD_ACPI_TABLE_REVISION) {
> 
> +    //
> 
> +    // PLD_ACPI_TABLE structure is used when Revision equals to
> PLD_ACPI_TABLE_REVISION
> 
> +    //
> 
> +    AcpiTableAdress = (PLD_ACPI_TABLE *) GET_GUID_HOB_DATA
> (GuidHob);
> 
> +    if (GenericHeader->Length < PLD_SIZEOF_THROUGH_FIELD
> (PLD_ACPI_TABLE, Rsdp)) {
> 
> +      //
> 
> +      // Retrun if can't find the ACPI Info Hob with enough length
> 
> +      //
> 
> +      return EFI_NOT_FOUND;
> 
> +    }
> 
> +    Rsdp = (EFI_ACPI_3_0_ROOT_SYSTEM_DESCRIPTION_POINTER *) (UINTN)
> (AcpiTableAdress->Rsdp);
> 
> +
> 
> +    //
> 
> +    // An ACPI-compatible OS must use the XSDT if present.
> 
> +    // It shouldn't happen that XsdtAddress points beyond 4G range in 32-bit
> environment.
> 
> +    //
> 
> +    ASSERT ((UINTN) Rsdp->XsdtAddress == Rsdp->XsdtAddress);
> 
> +
> 
> +    EntrySize = sizeof (UINT64);
> 
> +    Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->XsdtAddress;
> 
> +    if (Rsdt == NULL) {
> 
> +      //
> 
> +      // XsdtAddress is zero, then we use Rsdt which has 32 bit entry
> 
> +      //
> 
> +      Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN) Rsdp->RsdtAddress;
> 
> +      EntrySize = sizeof (UINT32);
> 
> +    }
> 
> +    Count = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> EntrySize;


Do we need a check on the validity of 'Rsdt->Length'?
If 'Dsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)' underflows,
I think it is possible to consume invalid content.


> 
> +
> 
> +    for (Index = 0; Index < Count; Index++){
> 
> +      ChildTableAddress = 0;
> 
> +      CopyMem (&ChildTableAddress, (UINT8 *) (Rsdt + 1) + EntrySize * Index,
> EntrySize);
> 
> +      //
> 
> +      // If the address is of UINT64 while this module runs at 32 bits,
> 
> +      // make sure the upper bits are all-zeros.
> 
> +      //
> 
> +      ASSERT (ChildTableAddress == (UINTN) ChildTableAddress);


Sorry for a question, is the condition in the above ASSERT a case that will never happen or
the above ASSERT is used for table content check?

If it is a check, please help to add error handling logic.


> 
> +
> 
> +      ChildTable = (EFI_ACPI_DESCRIPTION_HEADER *) (UINTN)
> ChildTableAddress;
> 
> +      Status = AddTableToList (AcpiTableInstance, ChildTable, TRUE, Version,
> TRUE, &TableKey);
> 
> +      if (EFI_ERROR (Status)) {
> 
> +        DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table at 0x%p\n", ChildTable));
> 
> +        ASSERT_EFI_ERROR (Status);


For a single ChildTable, if the installation fails here, is it still needed to install the FACS and DSDT within the ChildTable?


> 
> +      }
> 
> +      if (ChildTable->Signature ==
> EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE){
> 
> +        //
> 
> +        // Add the FACS and DSDT tables if it is not NULL.
> 
> +        //
> 
> +        if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >FirmwareCtrl != 0) {
> 
> +          TableToInstall = (VOID *) (UINTN)
> ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >FirmwareCtrl;
> 
> +          Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE,
> Version, TRUE, &TableKey);
> 
> +          if (EFI_ERROR (Status)) {
> 
> +            DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table FACS\n"));
> 
> +            ASSERT_EFI_ERROR (Status);


If the installation of FACS fails, is it still needed to install the DSDT below?


> 
> +          }
> 
> +        }
> 
> +
> 
> +        if (((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)-
> >Dsdt != 0) {
> 
> +          TableToInstall = (VOID *) (UINTN)
> ((EFI_ACPI_3_0_FIXED_ACPI_DESCRIPTION_TABLE *) ChildTable)->Dsdt;
> 
> +          Status = AddTableToList (AcpiTableInstance, TableToInstall, TRUE,
> Version, TRUE, &TableKey);
> 
> +          if (EFI_ERROR (Status)) {
> 
> +            DEBUG ((DEBUG_ERROR, "InstallAcpiTableFromHob: Fail to add ACPI
> table DSDT\n"));
> 
> +            ASSERT_EFI_ERROR (Status);
> 
> +          }
> 
> +        }
> 
> +      }
> 
> +    }
> 
> +  }
> 
> +  Status = PublishTables (AcpiTableInstance, Version);


If there are errors occurred during the parse of the tables, do we still need to publish them anyway?

Best Regards,
Hao Wu


> 
> +  ASSERT_EFI_ERROR (Status);
> 
> +  return Status;
> 
> +}
> 
> 
> 
>  /**
> 
>    Constructor for the ACPI table protocol.  Initializes instance
> 
> @@ -1918,6 +2044,8 @@ AcpiTableAcpiTableConstructor (
> 
> 
>    ChecksumCommonTables (AcpiTableInstance);
> 
> 
> 
> +  InstallAcpiTableFromHob (AcpiTableInstance);
> 
> +
> 
>    //
> 
>    // Completed successfully
> 
>    //
> 
> --
> 2.30.0.windows.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* 回复: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload
  2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
                   ` (8 preceding siblings ...)
  2021-05-24  7:12 ` [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob Zhiguang Liu
@ 2021-05-28  3:00 ` gaoliming
  9 siblings, 0 replies; 19+ messages in thread
From: gaoliming @ 2021-05-28  3:00 UTC (permalink / raw)
  To: devel, zhiguang.liu
  Cc: 'Ni, Ray', 'Kinney, Michael D',
	'Laszlo Ersek'

Zhiguang:
  MdePkg definitions are from Industry Standards. 

  Universalpayload definitions are from its documentation 0.75 version. I don't think it belongs to the formal industry standards. Can you provide more information to show Universalpayload is the public industry standard?

  https://universalpayload.github.io/documentation/payload-interfaces/index.html

Thanks
Liming
> -----邮件原件-----
> 发件人: devel@edk2.groups.io <devel@edk2.groups.io> 代表 Zhiguang Liu
> 发送时间: 2021年5月24日 15:12
> 收件人: devel@edk2.groups.io
> 主题: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload
> 
> This patch set is based on Universal Payload on
> https://universalpayload.github.io/documentation/payload-interfaces/index.
> html
> 
> This patch set introduce one general header, three different hob types
> and how Universal Payload consume these hobs.
> 
> 
> Zhiguang Liu (9):
>   MdePkg: Add Universal Payload general defination header file
>   MdePkg: Add new structure for the PCI Root Bridge Info Hob
>   UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob
>   MdePkg: Add new structure for the Universal Payload SMBios Table Info
>     Hob
>   MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables
>   UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob
>   MdePkg: Add new structure for the Universal Payload ACPI Table Info
>     Hob
>   MdeModulePkg/ACPI: Install ACPI table from HOB.
>   UefiPayloadPkg: Creat gPldAcpiTableGuid Hob
> 
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTable.h           |
> 4 +++-
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableDxe.inf      |   4
> +++-
>  MdeModulePkg/Universal/Acpi/AcpiTableDxe/AcpiTableProtocol.c   | 144
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++--------
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.c                   |
> 299
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++++++++++++++++++++++++--
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.h                   |
> 4 +++-
>  MdeModulePkg/Universal/SmbiosDxe/SmbiosDxe.inf                 |
> 5 ++++-
>  MdePkg/Include/UniversalPayload/AcpiTable.h                    |
> 28 ++++++++++++++++++++++++++++
>  MdePkg/Include/UniversalPayload/PciRootBridges.h               |
> 89
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> +++++++++++++++++++++++++++
>  MdePkg/Include/UniversalPayload/SmbiosTable.h                  |
> 28 ++++++++++++++++++++++++++++
>  MdePkg/Include/UniversalPayload/UniversalPayload.h             |
> 33 +++++++++++++++++++++++++++++++++
>  MdePkg/MdePkg.dec
> |  15 +++++++++++++++
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c                     |
> 28 +---------------------------
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h                     |
> 5 +----
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf                   |
> 4 +---
>  UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridge.h        |  40
> ++++++++++++++++++++++++++++++++++++++--
>  UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.c     |  47
> ++++++++++++++++++++++++++++++++++++++++++++---
>  UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeLib.inf   |   8
> +++++++-
>  UefiPayloadPkg/Library/PciHostBridgeLib/PciHostBridgeSupport.c |  73
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> ++++++++++-
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c             |  23
> ++++++++++++++++++++++-
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h             |   5
> +++--
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf           |   4
> +++-
>  UefiPayloadPkg/UefiPayloadPkg.dsc                              |
> 2 +-
>  22 files changed, 832 insertions(+), 60 deletions(-)
>  create mode 100644 MdePkg/Include/UniversalPayload/AcpiTable.h
>  create mode 100644 MdePkg/Include/UniversalPayload/PciRootBridges.h
>  create mode 100644 MdePkg/Include/UniversalPayload/SmbiosTable.h
>  create mode 100644 MdePkg/Include/UniversalPayload/UniversalPayload.h
> 
> --
> 2.30.0.windows.2
> 
> 
> 
> 
> 




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob
  2021-05-24  7:12 ` [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob Zhiguang Liu
@ 2021-06-02  3:39   ` Guo Dong
  0 siblings, 0 replies; 19+ messages in thread
From: Guo Dong @ 2021-06-02  3:39 UTC (permalink / raw)
  To: Liu, Zhiguang, devel@edk2.groups.io; +Cc: Ma, Maurice, You, Benjamin


Reviewed-by: Guo Dong <guo.dong@intel.com>

> -----Original Message-----
> From: Liu, Zhiguang <zhiguang.liu@intel.com>
> Sent: Monday, May 24, 2021 12:13 AM
> To: devel@edk2.groups.io
> Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo
> <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>
> Subject: [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob
> 
> From SysTableInfo Hob, get Smbios table address, and creat
> gPldSmbiosTableGuid Hob
> to store it. Remove diretly adding smbios table to ConfigurationTable.
> Dxe module SmbiosDxe will parse it and install smbios table from it.
> 
> Cc: Maurice Ma <maurice.ma@intel.com>
> Cc: Guo Dong <guo.dong@intel.com>
> Cc: Benjamin You <benjamin.you@intel.com>
> Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com>
> ---
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c           | 11 +----------
>  UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf         |  3 +--
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c   | 12 +++++++++++-
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h   |  3 ++-
>  UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf |  3 ++-
>  5 files changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> index a746d0581e..56b85b8e6d 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
> @@ -2,7 +2,7 @@
>    This driver will report some MMIO/IO resources to dxe core, extract smbios
> and acpi
> 
>    tables from bootloader.
> 
> 
> 
> -  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
> 
> +  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -129,15 +129,6 @@ BlDxeEntryPoint (
>      ASSERT_EFI_ERROR (Status);
> 
>    }
> 
> 
> 
> -  //
> 
> -  // Install Smbios Table
> 
> -  //
> 
> -  if (SystemTableInfo->SmbiosTableBase != 0 && SystemTableInfo-
> >SmbiosTableSize != 0) {
> 
> -    DEBUG ((DEBUG_ERROR, "Install Smbios Table at 0x%lx, length 0x%x\n",
> SystemTableInfo->SmbiosTableBase, SystemTableInfo->SmbiosTableSize));
> 
> -    Status = gBS->InstallConfigurationTable (&gEfiSmbiosTableGuid, (VOID
> *)(UINTN)SystemTableInfo->SmbiosTableBase);
> 
> -    ASSERT_EFI_ERROR (Status);
> 
> -  }
> 
> -
> 
>    //
> 
>    // Find the frame buffer information and update PCDs
> 
>    //
> 
> diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> index cebc811355..30f41f8c39 100644
> --- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> +++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
> @@ -3,7 +3,7 @@
>  #
> 
>  # Report some MMIO/IO resources to dxe core, extract smbios and acpi
> tables
> 
>  #
> 
> -#  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
> 
> +#  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  #
> 
> @@ -43,7 +43,6 @@
> 
> 
>  [Guids]
> 
>    gEfiAcpiTableGuid
> 
> -  gEfiSmbiosTableGuid
> 
>    gUefiSystemTableInfoGuid
> 
>    gUefiAcpiBoardInfoGuid
> 
>    gEfiGraphicsInfoHobGuid
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> index 805f5448d9..7b71d37f94 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
> @@ -1,6 +1,6 @@
>  /** @file
> 
> 
> 
> -  Copyright (c) 2014 - 2020, Intel Corporation. All rights reserved.<BR>
> 
> +  Copyright (c) 2014 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>    SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> 
> 
>  **/
> 
> @@ -234,6 +234,7 @@ BuildHobFromBl (
>    EFI_PEI_GRAPHICS_INFO_HOB        *NewGfxInfo;
> 
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB GfxDeviceInfo;
> 
>    EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
> 
> +  PLD_SMBIOS_TABLE                 *SmBiosTableHob;
> 
> 
> 
>    //
> 
>    // Parse memory info and build memory HOBs
> 
> @@ -276,6 +277,15 @@ BuildHobFromBl (
>      DEBUG ((DEBUG_INFO, "Detected Acpi Table at 0x%lx, length 0x%x\n",
> SysTableInfo.AcpiTableBase, SysTableInfo.AcpiTableSize));
> 
>      DEBUG ((DEBUG_INFO, "Detected Smbios Table at 0x%lx, length 0x%x\n",
> SysTableInfo.SmbiosTableBase, SysTableInfo.SmbiosTableSize));
> 
>    }
> 
> +  //
> 
> +  // Creat SmBios table Hob
> 
> +  //
> 
> +  SmBiosTableHob = BuildGuidHob (&gPldSmbiosTableGuid, sizeof
> (PLD_SMBIOS_TABLE));
> 
> +  ASSERT (SmBiosTableHob != NULL);
> 
> +  SmBiosTableHob->PldHeader.Revision = PLD_SMBIOS_TABLE_REVISION;
> 
> +  SmBiosTableHob->PldHeader.Length = sizeof (PLD_SMBIOS_TABLE);
> 
> +  SmBiosTableHob->SmBiosEntryPoint = SysTableInfo.SmbiosTableBase;
> 
> +  DEBUG ((DEBUG_INFO, "Create smbios table gPldSmbiosTableGuid guid
> hob\n"));
> 
> 
> 
>    //
> 
>    // Create guid hob for acpi board information
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> index 2c84d6ed53..e7d0d15118 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
> @@ -1,6 +1,6 @@
>  /** @file
> 
>  *
> 
> -* Copyright (c) 2020, Intel Corporation. All rights reserved.<BR>
> 
> +* Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> 
>  *
> 
>  *  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
>  *
> 
> @@ -31,6 +31,7 @@
>  #include <Guid/MemoryMapInfoGuid.h>
> 
>  #include <Guid/AcpiBoardInfoGuid.h>
> 
>  #include <Guid/GraphicsInfoHob.h>
> 
> +#include <UniversalPayload/SmbiosTable.h>
> 
> 
> 
> 
> 
>  #define LEGACY_8259_MASK_REGISTER_MASTER  0x21
> 
> diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> index cc59f1903b..444f39acf3 100644
> --- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> +++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
> @@ -1,7 +1,7 @@
>  ## @file
> 
>  #  This is the first module for UEFI payload.
> 
>  #
> 
> -#  Copyright (c) 2006 - 2020, Intel Corporation. All rights reserved.<BR>
> 
> +#  Copyright (c) 2006 - 2021, Intel Corporation. All rights reserved.<BR>
> 
>  #  Copyright (c) 2017, AMD Incorporated. All rights reserved.<BR>
> 
>  #
> 
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> 
> @@ -64,6 +64,7 @@
>    gEfiGraphicsInfoHobGuid
> 
>    gEfiGraphicsDeviceInfoHobGuid
> 
>    gUefiAcpiBoardInfoGuid
> 
> +  gPldSmbiosTableGuid
> 
> 
> 
>  [FeaturePcd.IA32]
> 
>    gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ##
> CONSUMES
> 
> --
> 2.30.0.windows.2


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [edk2-devel] [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob
  2021-05-26 13:50   ` [edk2-devel] " Patrick Rudolph
@ 2021-06-02  3:47     ` Guo Dong
  0 siblings, 0 replies; 19+ messages in thread
From: Guo Dong @ 2021-06-02  3:47 UTC (permalink / raw)
  To: Patrick Rudolph, devel@edk2.groups.io, Liu, Zhiguang
  Cc: Ma, Maurice, You, Benjamin, Ni, Ray

[-- Attachment #1: Type: text/plain, Size: 7262 bytes --]


Agree with Patrick.
In this patch, it removed gBS->InstallConfigurationTable(), it should also add AcpiTableDxe.inf to avoid regression.

Thanks,
Guo

From: Patrick Rudolph <patrick.rudolph@9elements.com>
Sent: Wednesday, May 26, 2021 6:50 AM
To: devel@edk2.groups.io; Liu, Zhiguang <zhiguang.liu@intel.com>
Cc: Ma, Maurice <maurice.ma@intel.com>; Dong, Guo <guo.dong@intel.com>; You, Benjamin <benjamin.you@intel.com>; Ni, Ray <ray.ni@intel.com>
Subject: Re: [edk2-devel] [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob


On Mon, May 24, 2021 at 9:13 AM Zhiguang Liu <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>> wrote:
From SysTableInfo Hob, get ACPI table address, and creat gPldAcpiTableGuid Hob
to store it. Remove diretly adding ACPI table to ConfigurationTable.
Dxe ACPI driver will parse it and install ACPI table from Guid Hob.

Cc: Maurice Ma <maurice.ma@intel.com<mailto:maurice.ma@intel.com>>
Cc: Guo Dong <guo.dong@intel.com<mailto:guo.dong@intel.com>>
Cc: Benjamin You <benjamin.you@intel.com<mailto:benjamin.you@intel.com>>
Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
Signed-off-by: Zhiguang Liu <zhiguang.liu@intel.com<mailto:zhiguang.liu@intel.com>>
---
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c           | 17 -----------------
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h           |  5 +----
 UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf         |  1 -
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c   | 11 +++++++++++
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h   |  2 +-
 UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf |  1 +
 6 files changed, 14 insertions(+), 23 deletions(-)

diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
index 56b85b8e6d..ffd3427fb3 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.c
@@ -99,7 +99,6 @@ BlDxeEntryPoint (
 {
   EFI_STATUS Status;
   EFI_HOB_GUID_TYPE          *GuidHob;
-  SYSTEM_TABLE_INFO          *SystemTableInfo;
   EFI_PEI_GRAPHICS_INFO_HOB  *GfxInfo;
   ACPI_BOARD_INFO            *AcpiBoardInfo;

@@ -113,22 +112,6 @@ BlDxeEntryPoint (
   Status = ReserveResourceInGcd (TRUE, EfiGcdMemoryTypeMemoryMappedIo, 0xFED00000, SIZE_1KB, 0, ImageHandle); // HPET
   ASSERT_EFI_ERROR (Status);

-  //
-  // Find the system table information guid hob
-  //
-  GuidHob = GetFirstGuidHob (&gUefiSystemTableInfoGuid);
-  ASSERT (GuidHob != NULL);
-  SystemTableInfo = (SYSTEM_TABLE_INFO *)GET_GUID_HOB_DATA (GuidHob);
-
-  //
-  // Install Acpi Table
-  //
-  if (SystemTableInfo->AcpiTableBase != 0 && SystemTableInfo->AcpiTableSize != 0) {
-    DEBUG ((DEBUG_ERROR, "Install Acpi Table at 0x%lx, length 0x%x\n", SystemTableInfo->AcpiTableBase, SystemTableInfo->AcpiTableSize));
-    Status = gBS->InstallConfigurationTable (&gEfiAcpiTableGuid, (VOID *)(UINTN)SystemTableInfo->AcpiTableBase);
-    ASSERT_EFI_ERROR (Status);
-  }
-

Note that AcpiTableDxe.inf is currently not part of the FV on UefipayloadPkg, so there won't be any tables installed after all.

   //
   // Find the frame buffer information and update PCDs
   //
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
index 512105fafd..3332a30eae 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.h
@@ -1,7 +1,7 @@
 /** @file
   The header file of bootloader support DXE.

-Copyright (c) 2014, Intel Corporation. All rights reserved.<BR>
+Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
 SPDX-License-Identifier: BSD-2-Clause-Patent

 **/
@@ -19,12 +19,9 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/IoLib.h>
 #include <Library/HobLib.h>

-#include <Guid/Acpi.h>
 #include <Guid/SmBios.h>
 #include <Guid/SystemTableInfoGuid.h>
 #include <Guid/AcpiBoardInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>

-#include <IndustryStandard/Acpi.h>
-
 #endif
diff --git a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
index 30f41f8c39..1ccb250991 100644
--- a/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
+++ b/UefiPayloadPkg/BlSupportDxe/BlSupportDxe.inf
@@ -42,7 +42,6 @@
   HobLib

 [Guids]
-  gEfiAcpiTableGuid
   gUefiSystemTableInfoGuid
   gUefiAcpiBoardInfoGuid
   gEfiGraphicsInfoHobGuid
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
index 7b71d37f94..14b7a732da 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.c
@@ -235,6 +235,7 @@ BuildHobFromBl (
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB GfxDeviceInfo;
   EFI_PEI_GRAPHICS_DEVICE_INFO_HOB *NewGfxDeviceInfo;
   PLD_SMBIOS_TABLE                 *SmBiosTableHob;
+  PLD_ACPI_TABLE                   *AcpiTableHob;

   //
   // Parse memory info and build memory HOBs
@@ -287,6 +288,16 @@ BuildHobFromBl (
   SmBiosTableHob->SmBiosEntryPoint = SysTableInfo.SmbiosTableBase;
   DEBUG ((DEBUG_INFO, "Create smbios table gPldSmbiosTableGuid guid hob\n"));

+  //
+  // Creat ACPI table Hob
+  //
+  AcpiTableHob = BuildGuidHob (&gPldAcpiTableGuid, sizeof (PLD_ACPI_TABLE));
+  ASSERT (AcpiTableHob != NULL);
+  AcpiTableHob->PldHeader.Revision = PLD_ACPI_TABLE_REVISION;
+  AcpiTableHob->PldHeader.Length = sizeof (PLD_ACPI_TABLE);
+  AcpiTableHob->Rsdp = SysTableInfo.AcpiTableBase;
+  DEBUG ((DEBUG_INFO, "Create smbios table gPldAcpiTableGuid guid hob\n"));
+

 DEBUG ((DEBUG_INFO, "Create acpi table ...

   //
   // Create guid hob for acpi board information
   //
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
index e7d0d15118..a4c9da128e 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.h
@@ -32,7 +32,7 @@
 #include <Guid/AcpiBoardInfoGuid.h>
 #include <Guid/GraphicsInfoHob.h>
 #include <UniversalPayload/SmbiosTable.h>
-
+#include <UniversalPayload/AcpiTable.h>

 #define LEGACY_8259_MASK_REGISTER_MASTER  0x21
 #define LEGACY_8259_MASK_REGISTER_SLAVE   0xA1
diff --git a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
index 444f39acf3..01388b8831 100644
--- a/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
+++ b/UefiPayloadPkg/UefiPayloadEntry/UefiPayloadEntry.inf
@@ -65,6 +65,7 @@
   gEfiGraphicsDeviceInfoHobGuid
   gUefiAcpiBoardInfoGuid
   gPldSmbiosTableGuid
+  gPldAcpiTableGuid

 [FeaturePcd.IA32]
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode      ## CONSUMES
--
2.30.0.windows.2



------------
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#75493): https://edk2.groups.io/g/devel/message/75493
Mute This Topic: https://groups.io/mt/83045527/2917327
Group Owner: devel+owner@edk2.groups.io<mailto:devel%2Bowner@edk2.groups.io>
Unsubscribe: https://edk2.groups.io/g/devel/unsub [patrick.rudolph@9elements.com<mailto:patrick.rudolph@9elements.com>]
------------


[-- Attachment #2: Type: text/html, Size: 12797 bytes --]

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-06-02  3:47 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-05-24  7:12 [PATCH 0/9] Create multiple Hobs for Universal Payload Zhiguang Liu
2021-05-24  7:12 ` [PATCH 1/9] MdePkg: Add Universal Payload general defination header file Zhiguang Liu
2021-05-24  7:12 ` [PATCH 2/9] MdePkg: Add new structure for the PCI Root Bridge Info Hob Zhiguang Liu
2021-05-24  7:12 ` [PATCH 3/9] UefiPayloadPkg: UefiPayload retrieve PCI root bridge from Guid Hob Zhiguang Liu
2021-05-24  7:12 ` [PATCH 4/9] MdePkg: Add new structure for the Universal Payload SMBios Table Info Hob Zhiguang Liu
2021-05-24  7:12 ` [PATCH 5/9] MdeModulePkg/Universal/SmbiosDxe: Scan for existing tables Zhiguang Liu
2021-05-26  6:15   ` Wu, Hao A
2021-05-26  6:32     ` [edk2-devel] " Wu, Hao A
2021-05-26  8:56       ` Zhiguang Liu
2021-05-26 13:04   ` Patrick Rudolph
2021-05-24  7:12 ` [PATCH 6/9] UefiPayloadPkg: Creat gPldSmbiosTableGuid Hob Zhiguang Liu
2021-06-02  3:39   ` Guo Dong
2021-05-24  7:12 ` [PATCH 7/9] MdePkg: Add new structure for the Universal Payload ACPI Table Info Hob Zhiguang Liu
2021-05-24  7:12 ` [PATCH 8/9] MdeModulePkg/ACPI: Install ACPI table from HOB Zhiguang Liu
2021-05-27  0:42   ` Wu, Hao A
2021-05-24  7:12 ` [PATCH 9/9] UefiPayloadPkg: Creat gPldAcpiTableGuid Hob Zhiguang Liu
2021-05-26 13:50   ` [edk2-devel] " Patrick Rudolph
2021-06-02  3:47     ` Guo Dong
2021-05-28  3:00 ` 回复: [edk2-devel] [PATCH 0/9] Create multiple Hobs for Universal Payload gaoliming

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox