[PATCH v3 0/6] Enhance DynamicTablesPkg modules

[PATCH v3 0/6] Enhance DynamicTablesPkg modules

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998

This patch series is a follow-up of previous submission:
https://edk2.groups.io/g/devel/message/91926

The main changes between v2 and v3 patches are:
  - Added reviewed-by collected from previous iteration
  - Added descriptions for newly introduced structures
  - Updated functions names

Current DynamicTablesPkg provide great support for creating dynamic ACPI
tables during boot time.

However, there are some modules needs minor tweaks to expand support and
compatibility for OS requirements and platform needs.

This patch series proposes a few fixes to resolve minor issues discovered
in DynamicPlatRepoLib, AcpiSsdtPcieLibArm and DynamicTableManagerDxe.

Patch v3 branch: https://github.com/kuqin12/edk2/tree/dynamic_update_v3

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Cc: Pierre Gondois <pierre.gondois@arm.com>

Kun Qin (6):
  DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf
  DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing
  DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers
  DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed
    tables
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM
    space
  DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI
    config

 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c   | 214 ++++++++++++--------
 DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c   | 176 ++++++++++++++++
 DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c    |  80 +++++++-
 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   4 +
 DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf  |   1 +
 5 files changed, 391 insertions(+), 84 deletions(-)

-- 
2.37.1.windows.1

[PATCH v3 1/6] DynamicTablesPkg: DynamicPlatRepoLib: Added MemoryAllocationLib to inf

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996

The DynamicPlatRepoLib has multiple reference to MemoryAllocationLib,
such as DynamicPlatRepo.c and TokenMapper.c. Not including it in the
library inf file could lead to potential build break.

This change added the MemoryAllocationLib into this inf file.

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Added Reviewed-by tag [Sami]
    - Added Reviewed-by tag [Pierre]
    
    v3:
    - No change.

 DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
index 9a3cc87fd91d..8423352550c2 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/DynamicPlatRepoLib.inf
@@ -31,3 +31,4 @@ [Packages]
 [LibraryClasses]
   AcpiHelperLib
   BaseLib
+  MemoryAllocationLib
-- 
2.37.1.windows.1

[PATCH v3 2/6] DynamicTablesPkg: DynamicPlatRepoLib: Fix incorrect dereferencing

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996

The content of token should be derived from the data section of the
`CmObject` instead of the object itself.

This change fixed the issue by dereferencing the token value from the
data buffer of input CmObject.

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Added Reviewed-by tag [Sami]
    - Added Reviewed-by tag [Pierre]
    
    v3:
    - No change.

 DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
index 80d0aa17bc1a..84e4bb7e3bc8 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
@@ -60,7 +60,7 @@ TokenFixerItsGroup (
   )
 {
   ASSERT (CmObject != NULL);
-  ((CM_ARM_ITS_GROUP_NODE *)CmObject)->Token = Token;
+  ((CM_ARM_ITS_GROUP_NODE *)CmObject->Data)->Token = Token;
   return EFI_SUCCESS;
 }
 
-- 
2.37.1.windows.1

[PATCH v3 3/6] DynamicTablesPkg: DynamicPlatRepoLib: Adding more token fixers

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3996

This change added more token fixers for other node types, including
NamedComponentNode, RootComplexNode, and SmmuV3Node.

The corresponding entries for tokenFixer functions table is also updated.

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Added Reviewed-by tag [Sami]
    - Added Reviewed-by tag [Pierre]
    
    v3:
    - No change.

 DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c | 78 +++++++++++++++++++-
 1 file changed, 75 insertions(+), 3 deletions(-)

diff --git a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
index 84e4bb7e3bc8..345acab53f74 100644
--- a/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
+++ b/DynamicTablesPkg/Library/Common/DynamicPlatRepoLib/CmObjectTokenFixer.c
@@ -64,6 +64,78 @@ TokenFixerItsGroup (
   return EFI_SUCCESS;
 }
 
+/** EArmObjNamedComponent token fixer.
+
+  CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+  @param [in]  CmObject    Pointer to the Configuration Manager Object.
+  @param [in]  Token       Token to be updated in the CmObject.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_UNSUPPORTED       Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerNamedComponentNode (
+  IN  CM_OBJ_DESCRIPTOR  *CmObject,
+  IN  CM_OBJECT_TOKEN    Token
+  )
+{
+  ASSERT (CmObject != NULL);
+  ((CM_ARM_NAMED_COMPONENT_NODE *)CmObject->Data)->Token = Token;
+  return EFI_SUCCESS;
+}
+
+/** EArmObjRootComplex token fixer.
+
+  CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+  @param [in]  CmObject    Pointer to the Configuration Manager Object.
+  @param [in]  Token       Token to be updated in the CmObject.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_UNSUPPORTED       Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerRootComplexNode (
+  IN  CM_OBJ_DESCRIPTOR  *CmObject,
+  IN  CM_OBJECT_TOKEN    Token
+  )
+{
+  ASSERT (CmObject != NULL);
+  ((CM_ARM_ROOT_COMPLEX_NODE *)CmObject->Data)->Token = Token;
+  return EFI_SUCCESS;
+}
+
+/** EArmObjSmmuV3 token fixer.
+
+  CmObjectToken fixer function that updates the Tokens in the CmObjects.
+
+  @param [in]  CmObject    Pointer to the Configuration Manager Object.
+  @param [in]  Token       Token to be updated in the CmObject.
+
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_INVALID_PARAMETER A parameter is invalid.
+  @retval EFI_UNSUPPORTED       Not supported.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+TokenFixerSmmuV3Node (
+  IN  CM_OBJ_DESCRIPTOR  *CmObject,
+  IN  CM_OBJECT_TOKEN    Token
+  )
+{
+  ASSERT (CmObject != NULL);
+  ((CM_ARM_SMMUV3_NODE *)CmObject->Data)->Token = Token;
+  return EFI_SUCCESS;
+}
+
 /** TokenFixer functions table.
 
   A CmObj having a CM_OBJECT_TOKEN field might need to have its
@@ -90,10 +162,10 @@ CM_OBJECT_TOKEN_FIXER  TokenFixer[EArmObjMax] = {
   NULL,                             ///< 16 - Hypervisor Vendor Id
   NULL,                             ///< 17 - Fixed feature flags for FADT
   TokenFixerItsGroup,               ///< 18 - ITS Group
-  TokenFixerNotImplemented,         ///< 19 - Named Component
-  TokenFixerNotImplemented,         ///< 20 - Root Complex
+  TokenFixerNamedComponentNode,     ///< 19 - Named Component
+  TokenFixerRootComplexNode,        ///< 20 - Root Complex
   TokenFixerNotImplemented,         ///< 21 - SMMUv1 or SMMUv2
-  TokenFixerNotImplemented,         ///< 22 - SMMUv3
+  TokenFixerSmmuV3Node,             ///< 22 - SMMUv3
   TokenFixerNotImplemented,         ///< 23 - PMCG
   NULL,                             ///< 24 - GIC ITS Identifier Array
   NULL,                             ///< 25 - ID Mapping Array
-- 
2.37.1.windows.1

[PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997

This change added an extra step to allow check for installed ACPI tables.

For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either pre-installed or
supplied through AcpiTableInfo can be accepted.

An extra check for FADT ACPI table existence during installation step is
also added.

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Function description updates [Sami]
    - Refactorized the table verification [Pierre]
    
    v3:
    - Added descriptions for new structures [Pierre]
    - Added check for SDT protocol PCD before using it [Pierre]

 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c   | 214 ++++++++++++--------
 DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   4 +
 2 files changed, 138 insertions(+), 80 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..7f3deef08a66 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -10,6 +10,7 @@
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 #include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/AcpiSystemDescriptionTable.h>
 #include <Protocol/AcpiTable.h>
 
 // Module specific include files.
@@ -22,6 +23,58 @@
 #include <Protocol/DynamicTableFactoryProtocol.h>
 #include <SmbiosTableGenerator.h>
 
+///
+/// Bit definitions for acceptable ACPI table presence formats.
+/// Currently only ACPI tables present in the ACPI info list and
+/// already installed will count towards "Table Present" during
+/// verification routine.
+///
+#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
+
+///
+/// Order of ACPI table being verified during presence inspection.
+///
+#define ACPI_TABLE_VERIFY_FADT   0
+#define ACPI_TABLE_VERIFY_MADT   1
+#define ACPI_TABLE_VERIFY_GTDT   2
+#define ACPI_TABLE_VERIFY_DSDT   3
+#define ACPI_TABLE_VERIFY_DBG2   4
+#define ACPI_TABLE_VERIFY_SPCR   5
+#define ACPI_TABLE_VERIFY_COUNT  6
+
+///
+/// Private data structure to verify the presence of mandatory
+/// or optional ACPI tables.
+///
+typedef struct {
+  /// ESTD ID for the ACPI table of interest.
+  ESTD_ACPI_TABLE_ID    EstdTableId;
+  /// Standard UINT32 ACPI signature.
+  UINT32                AcpiTableSignature;
+  /// 4 character ACPI table name (the 5th char8 is for null terminator).
+  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
+  /// Indicator on whether the ACPI table is required.
+  BOOLEAN               IsMandatory;
+  /// Formats of verified presences, as defined by ACPI_TABLE_PRESENT_*
+  /// This field should be initialized to 0 and will be populated during
+  /// verification routine.
+  UINT16                Presence;
+} ACPI_TABLE_PRESENCE_INFO;
+
+///
+/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
+/// This list also include optional ACPI tables: DBG2, SPCR.
+///
+ACPI_TABLE_PRESENCE_INFO  mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
+  { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,            "FADT", TRUE,  0 },
+  { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,         "MADT", TRUE,  0 },
+  { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,         "GTDT", TRUE,  0 },
+  { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, "DSDT", TRUE,  0 },
+  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,                      "DBG2", FALSE, 0 },
+  { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,   "SPCR", FALSE, 0 },
+};
+
 /** This macro expands to a function that retrieves the ACPI Table
     List from the Configuration Manager.
 */
@@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
 
   @retval EFI_SUCCESS           Success.
   @retval EFI_NOT_FOUND         If mandatory table is not found.
+  @retval EFI_ALREADY_STARTED   If mandatory table found in AcpiTableInfo is already installed.
 **/
 STATIC
 EFI_STATUS
@@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
   IN       UINT32                              AcpiTableCount
   )
 {
-  EFI_STATUS  Status;
-  BOOLEAN     FadtFound;
-  BOOLEAN     MadtFound;
-  BOOLEAN     GtdtFound;
-  BOOLEAN     DsdtFound;
-  BOOLEAN     Dbg2Found;
-  BOOLEAN     SpcrFound;
+  EFI_STATUS                   Status;
+  UINTN                        Handle;
+  UINTN                        Index;
+  UINTN                        InstalledTableIndex;
+  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
+  EFI_ACPI_TABLE_VERSION       Version;
+  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
 
-  Status    = EFI_SUCCESS;
-  FadtFound = FALSE;
-  MadtFound = FALSE;
-  GtdtFound = FALSE;
-  DsdtFound = FALSE;
-  Dbg2Found = FALSE;
-  SpcrFound = FALSE;
   ASSERT (AcpiTableInfo != NULL);
 
+  Status = EFI_SUCCESS;
+
+  // Check against the statically initialized ACPI tables to see if they are in ACPI info list
   while (AcpiTableCount-- != 0) {
-    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
-      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
-        FadtFound = TRUE;
-        break;
-      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
-        MadtFound = TRUE;
-        break;
-      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
-        GtdtFound = TRUE;
-        break;
-      case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
-        DsdtFound = TRUE;
-        break;
-      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
-        Dbg2Found = TRUE;
-        break;
-      case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
-        SpcrFound = TRUE;
-        break;
-      default:
+    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
+        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
+        // Found this table, skip the rest.
         break;
+      }
     }
   }
 
-  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
-  if (!FadtFound) {
-    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
-    Status = EFI_NOT_FOUND;
-  }
+  // They also might be published already, so we can search from there
+  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
+    AcpiSdt = NULL;
+    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
 
-  if (!MadtFound) {
-    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
-    Status = EFI_NOT_FOUND;
-  }
+    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
+      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
+      return Status;
+    }
 
-  if (!GtdtFound) {
-    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
-    Status = EFI_NOT_FOUND;
-  }
+    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+      Handle              = 0;
+      InstalledTableIndex = 0;
+      do {
+        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
+        if (EFI_ERROR (Status)) {
+          break;
+        }
 
-  if (!DsdtFound) {
-    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
-    Status = EFI_NOT_FOUND;
-  }
+        InstalledTableIndex++;
+      } while (DescHeader->Signature != mAcpiVerifyTables[Index].AcpiTableSignature);
 
-  if (!Dbg2Found) {
-    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
+      if (!EFI_ERROR (Status)) {
+        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INSTALLED;
+      }
+    }
   }
 
-  if (!SpcrFound) {
-    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
+  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+    if (mAcpiVerifyTables[Index].Presence == 0) {
+      if (mAcpiVerifyTables[Index].IsMandatory) {
+        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+        Status = EFI_NOT_FOUND;
+      } else {
+        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+      }
+    } else if (mAcpiVerifyTables[Index].Presence ==
+               (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLED))
+    {
+      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.\n", mAcpiVerifyTables[Index].AcpiTableName));
+      Status = EFI_ALREADY_STARTED;
+    }
   }
 
   return Status;
@@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
   @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
                                     Protocol Interface.
 
-  @retval EFI_SUCCESS   Success.
-  @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+  @retval EFI_SUCCESS           Success.
+  @retval EFI_NOT_FOUND         If a mandatory table or a generator is not found.
+  @retval EFI_ALREADY_STARTED   If mandatory table found in AcpiTableInfo is already installed.
 **/
 STATIC
 EFI_STATUS
@@ -562,7 +613,7 @@ ProcessAcpiTables (
   if (EFI_ERROR (Status)) {
     DEBUG ((
       DEBUG_ERROR,
-      "ERROR: Failed to find mandatory ACPI Table(s)."
+      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
       " Status = %r\n",
       Status
       ));
@@ -570,29 +621,32 @@ ProcessAcpiTables (
   }
 
   // Add the FADT Table first.
-  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
-    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
-        AcpiTableInfo[Idx].TableGeneratorId)
-    {
-      Status = BuildAndInstallAcpiTable (
-                 TableFactoryProtocol,
-                 CfgMgrProtocol,
-                 AcpiTableProtocol,
-                 &AcpiTableInfo[Idx]
-                 );
-      if (EFI_ERROR (Status)) {
-        DEBUG ((
-          DEBUG_ERROR,
-          "ERROR: Failed to find build and install ACPI FADT Table." \
-          " Status = %r\n",
-          Status
-          ));
-        return Status;
-      }
+  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRESENT_INSTALLED) == 0) {
+    // FADT is not yet installed
+    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
+      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
+          AcpiTableInfo[Idx].TableGeneratorId)
+      {
+        Status = BuildAndInstallAcpiTable (
+                   TableFactoryProtocol,
+                   CfgMgrProtocol,
+                   AcpiTableProtocol,
+                   &AcpiTableInfo[Idx]
+                   );
+        if (EFI_ERROR (Status)) {
+          DEBUG ((
+            DEBUG_ERROR,
+            "ERROR: Failed to find build and install ACPI FADT Table." \
+            " Status = %r\n",
+            Status
+            ));
+          return Status;
+        }
 
-      break;
-    }
-  } // for
+        break;
+      }
+    } // for
+  }
 
   // Add remaining ACPI Tables
   for (Idx = 0; Idx < AcpiTableCount; Idx++) {
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
index 028c3d413cf8..ad8b3d037c16 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -34,8 +34,12 @@ [LibraryClasses]
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
+[FeaturePcd]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol  ## CONSUMES
+
 [Protocols]
   gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
+  gEfiAcpiSdtProtocolGuid                       # PROTOCOL ALWAYS_CONSUMED
 
   gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL ALWAYS_CONSUMED
   gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL ALWAYS_CONSUMED
-- 
2.37.1.windows.1

Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Sami Mujawar]

Hi Kun,

Thank you for this patch.

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, Kun Qin wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>
> This change added an extra step to allow check for installed ACPI tables.
>
> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either pre-installed or
> supplied through AcpiTableInfo can be accepted.
>
> An extra check for FADT ACPI table existence during installation step is
> also added.
>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>
> Notes:
>      v2:
>      - Function description updates [Sami]
>      - Refactorized the table verification [Pierre]
>      
>      v3:
>      - Added descriptions for new structures [Pierre]
>      - Added check for SDT protocol PCD before using it [Pierre]
>
>   DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c   | 214 ++++++++++++--------
>   DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   4 +
>   2 files changed, 138 insertions(+), 80 deletions(-)
>
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> index ed62299f9bbd..7f3deef08a66 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> @@ -10,6 +10,7 @@
>   #include <Library/DebugLib.h>
>
>   #include <Library/PcdLib.h>
>
>   #include <Library/UefiBootServicesTableLib.h>
>
> +#include <Protocol/AcpiSystemDescriptionTable.h>
>
>   #include <Protocol/AcpiTable.h>
>
>   
>
>   // Module specific include files.
>
> @@ -22,6 +23,58 @@
>   #include <Protocol/DynamicTableFactoryProtocol.h>
>
>   #include <SmbiosTableGenerator.h>
>
>   
>
> +///
>
> +/// Bit definitions for acceptable ACPI table presence formats.
>
> +/// Currently only ACPI tables present in the ACPI info list and
>
> +/// already installed will count towards "Table Present" during
>
> +/// verification routine.
>
> +///
>
> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
>
> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
>
> +
>
> +///
>
> +/// Order of ACPI table being verified during presence inspection.
>
> +///
>
> +#define ACPI_TABLE_VERIFY_FADT   0
>
> +#define ACPI_TABLE_VERIFY_MADT   1
>
> +#define ACPI_TABLE_VERIFY_GTDT   2
>
> +#define ACPI_TABLE_VERIFY_DSDT   3
>
> +#define ACPI_TABLE_VERIFY_DBG2   4
>
> +#define ACPI_TABLE_VERIFY_SPCR   5
>
> +#define ACPI_TABLE_VERIFY_COUNT  6
>
> +
>
> +///
>
> +/// Private data structure to verify the presence of mandatory
>
> +/// or optional ACPI tables.
>
> +///
>
> +typedef struct {
>
> +  /// ESTD ID for the ACPI table of interest.
>
> +  ESTD_ACPI_TABLE_ID    EstdTableId;
>
> +  /// Standard UINT32 ACPI signature.
>
> +  UINT32                AcpiTableSignature;
>
> +  /// 4 character ACPI table name (the 5th char8 is for null terminator).
>
> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
>
> +  /// Indicator on whether the ACPI table is required.
>
> +  BOOLEAN               IsMandatory;
>
> +  /// Formats of verified presences, as defined by ACPI_TABLE_PRESENT_*
>
> +  /// This field should be initialized to 0 and will be populated during
>
> +  /// verification routine.
>
> +  UINT16                Presence;
>
> +} ACPI_TABLE_PRESENCE_INFO;
>
> +
>
> +///
>
> +/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
>
> +/// This list also include optional ACPI tables: DBG2, SPCR.
>
> +///
>
> +ACPI_TABLE_PRESENCE_INFO  mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
>
> +  { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,            "FADT", TRUE,  0 },
>
> +  { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,         "MADT", TRUE,  0 },
>
> +  { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,         "GTDT", TRUE,  0 },
>
> +  { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, "DSDT", TRUE,  0 },
>
> +  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,                      "DBG2", FALSE, 0 },
>
> +  { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,   "SPCR", FALSE, 0 },
>
> +};
>
> +
>
>   /** This macro expands to a function that retrieves the ACPI Table
>
>       List from the Configuration Manager.
>
>   */
>
> @@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
>   
>
>     @retval EFI_SUCCESS           Success.
>
>     @retval EFI_NOT_FOUND         If mandatory table is not found.
>
> +  @retval EFI_ALREADY_STARTED   If mandatory table found in AcpiTableInfo is already installed.
>
>   **/
>
>   STATIC
>
>   EFI_STATUS
>
> @@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
>     IN       UINT32                              AcpiTableCount
>
>     )
>
>   {
>
> -  EFI_STATUS  Status;
>
> -  BOOLEAN     FadtFound;
>
> -  BOOLEAN     MadtFound;
>
> -  BOOLEAN     GtdtFound;
>
> -  BOOLEAN     DsdtFound;
>
> -  BOOLEAN     Dbg2Found;
>
> -  BOOLEAN     SpcrFound;
>
> +  EFI_STATUS                   Status;
>
> +  UINTN                        Handle;
>
> +  UINTN                        Index;
>
> +  UINTN                        InstalledTableIndex;
>
> +  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
>
> +  EFI_ACPI_TABLE_VERSION       Version;
>
> +  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
>
>   
>
> -  Status    = EFI_SUCCESS;
>
> -  FadtFound = FALSE;
>
> -  MadtFound = FALSE;
>
> -  GtdtFound = FALSE;
>
> -  DsdtFound = FALSE;
>
> -  Dbg2Found = FALSE;
>
> -  SpcrFound = FALSE;
>
>     ASSERT (AcpiTableInfo != NULL);
>
>   
>
> +  Status = EFI_SUCCESS;
>
> +
>
> +  // Check against the statically initialized ACPI tables to see if they are in ACPI info list
>
>     while (AcpiTableCount-- != 0) {
>
> -    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>
> -      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>
> -        FadtFound = TRUE;
>
> -        break;
>
> -      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>
> -        MadtFound = TRUE;
>
> -        break;
>
> -      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>
> -        GtdtFound = TRUE;
>
> -        break;
>
> -      case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>
> -        DsdtFound = TRUE;
>
> -        break;
>
> -      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>
> -        Dbg2Found = TRUE;
>
> -        break;
>
> -      case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>
> -        SpcrFound = TRUE;
>
> -        break;
>
> -      default:
>
> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>
> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
>
> +        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
>
> +        // Found this table, skip the rest.
>
>           break;
>
> +      }
>
>       }
>
>     }
>
>   
>
> -  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
>
> -  if (!FadtFound) {
>
> -    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>
> -    Status = EFI_NOT_FOUND;
>
> -  }
>
> +  // They also might be published already, so we can search from there
>
> +  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
>
> +    AcpiSdt = NULL;
>
> +    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
>
>   
>
> -  if (!MadtFound) {
>
> -    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>
> -    Status = EFI_NOT_FOUND;
>
> -  }
>
> +    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>
> +      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
>
> +      return Status;
>
> +    }
>
>   
>
> -  if (!GtdtFound) {
>
> -    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>
> -    Status = EFI_NOT_FOUND;
>
> -  }
>
> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>
> +      Handle              = 0;
>
> +      InstalledTableIndex = 0;
>
> +      do {
>
> +        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>
> +        if (EFI_ERROR (Status)) {
>
> +          break;
>
> +        }
>
>   
>
> -  if (!DsdtFound) {
>
> -    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>
> -    Status = EFI_NOT_FOUND;
>
> -  }
>
> +        InstalledTableIndex++;
>
> +      } while (DescHeader->Signature != mAcpiVerifyTables[Index].AcpiTableSignature);
>
>   
>
> -  if (!Dbg2Found) {
>
> -    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>
> +      if (!EFI_ERROR (Status)) {
>
> +        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INSTALLED;
>
> +      }
>
> +    }
>
>     }
>
>   
>
> -  if (!SpcrFound) {
>
> -    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>
> +  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>
> +    if (mAcpiVerifyTables[Index].Presence == 0) {
>
> +      if (mAcpiVerifyTables[Index].IsMandatory) {
>
> +        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
>
> +        Status = EFI_NOT_FOUND;
>
> +      } else {
>
> +        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
>
> +      }
>
> +    } else if (mAcpiVerifyTables[Index].Presence ==
>
> +               (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLED))
>
> +    {
>
> +      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>
> +      Status = EFI_ALREADY_STARTED;
>
> +    }
>
>     }
>
>   
>
>     return Status;
>
> @@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
>     @param [in]  CfgMgrProtocol       Pointer to the Configuration Manager
>
>                                       Protocol Interface.
>
>   
>
> -  @retval EFI_SUCCESS   Success.
>
> -  @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
>
> +  @retval EFI_SUCCESS           Success.
>
> +  @retval EFI_NOT_FOUND         If a mandatory table or a generator is not found.
>
> +  @retval EFI_ALREADY_STARTED   If mandatory table found in AcpiTableInfo is already installed.
>
>   **/
>
>   STATIC
>
>   EFI_STATUS
>
> @@ -562,7 +613,7 @@ ProcessAcpiTables (
>     if (EFI_ERROR (Status)) {
>
>       DEBUG ((
>
>         DEBUG_ERROR,
>
> -      "ERROR: Failed to find mandatory ACPI Table(s)."
>
> +      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>
>         " Status = %r\n",
>
>         Status
>
>         ));
>
> @@ -570,29 +621,32 @@ ProcessAcpiTables (
>     }
>
>   
>
>     // Add the FADT Table first.
>
> -  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>
> -    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>
> -        AcpiTableInfo[Idx].TableGeneratorId)
>
> -    {
>
> -      Status = BuildAndInstallAcpiTable (
>
> -                 TableFactoryProtocol,
>
> -                 CfgMgrProtocol,
>
> -                 AcpiTableProtocol,
>
> -                 &AcpiTableInfo[Idx]
>
> -                 );
>
> -      if (EFI_ERROR (Status)) {
>
> -        DEBUG ((
>
> -          DEBUG_ERROR,
>
> -          "ERROR: Failed to find build and install ACPI FADT Table." \
>
> -          " Status = %r\n",
>
> -          Status
>
> -          ));
>
> -        return Status;
>
> -      }
>
> +  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRESENT_INSTALLED) == 0) {

[SAMI] If I understand correctly, the mAcpiVerifyTables[x].Presence 
filed cannot be ACPI_TABLE_PRESENT_INFO_LIST and 
ACPI_TABLE_PRESENT_INSTALLED at the same time. Otherwise we would have 
returned EFI_ALREADY_STARTED from VerifyMandatoryTablesArePresent().

Since FADT is mandatory, the only valid conditions are:

1. ACPI_TABLE_PRESENT_INFO_LIST and !ACPI_TABLE_PRESENT_INSTALLED

2. !ACPI_TABLE_PRESENT_INFO_LIST & ACPI_TABLE_PRESENT_INSTALLED

Therefore, I think the above check is not required. What do you think?

[/SAMI]

>
> +    // FADT is not yet installed
>
> +    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>
> +      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>
> +          AcpiTableInfo[Idx].TableGeneratorId)
>
> +      {
>
> +        Status = BuildAndInstallAcpiTable (
>
> +                   TableFactoryProtocol,
>
> +                   CfgMgrProtocol,
>
> +                   AcpiTableProtocol,
>
> +                   &AcpiTableInfo[Idx]
>
> +                   );
>
> +        if (EFI_ERROR (Status)) {
>
> +          DEBUG ((
>
> +            DEBUG_ERROR,
>
> +            "ERROR: Failed to find build and install ACPI FADT Table." \
>
> +            " Status = %r\n",
>
> +            Status
>
> +            ));
>
> +          return Status;
>
> +        }
>
>   
>
> -      break;
>
> -    }
>
> -  } // for
>
> +        break;
>
> +      }
>
> +    } // for
>
> +  }
>
>   
>
>     // Add remaining ACPI Tables
>
>     for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> index 028c3d413cf8..ad8b3d037c16 100644
> --- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> +++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
> @@ -34,8 +34,12 @@ [LibraryClasses]
>     UefiBootServicesTableLib
>
>     UefiDriverEntryPoint
>
>   
>
> +[FeaturePcd]
>
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol  ## CONSUMES
>
> +
>
>   [Protocols]
>
>     gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED
>
> +  gEfiAcpiSdtProtocolGuid                       # PROTOCOL ALWAYS_CONSUMED
>
>   
>
>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL ALWAYS_CONSUMED
>
>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL ALWAYS_CONSUMED
>

Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Sami Mujawar]

Hi Kun,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 08/08/2022 02:05 pm, Sami Mujawar wrote:
> Hi Kun,
>
> Thank you for this patch.
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 31/07/2022 06:37 am, Kun Qin wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>>
>> This change added an extra step to allow check for installed ACPI 
>> tables.
>>
>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either 
>> pre-installed or
>> supplied through AcpiTableInfo can be accepted.
>>
>> An extra check for FADT ACPI table existence during installation step is
>> also added.
>>
>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>
>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - Function description updates [Sami]
>>      - Refactorized the table verification [Pierre]
>>           v3:
>>      - Added descriptions for new structures [Pierre]
>>      - Added check for SDT protocol PCD before using it [Pierre]
>>
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>> | 214 ++++++++++++--------
>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>> |   4 +
>>   2 files changed, 138 insertions(+), 80 deletions(-)
>>
>> diff --git 
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>>
>> index ed62299f9bbd..7f3deef08a66 100644
>> --- 
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> +++ 
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>> @@ -10,6 +10,7 @@
>>   #include <Library/DebugLib.h>
>>
>>   #include <Library/PcdLib.h>
>>
>>   #include <Library/UefiBootServicesTableLib.h>
>>
>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>>
>>   #include <Protocol/AcpiTable.h>
>>
>>
>>   // Module specific include files.
>>
>> @@ -22,6 +23,58 @@
>>   #include <Protocol/DynamicTableFactoryProtocol.h>
>>
>>   #include <SmbiosTableGenerator.h>
>>
>>
>> +///
>>
>> +/// Bit definitions for acceptable ACPI table presence formats.
>>
>> +/// Currently only ACPI tables present in the ACPI info list and
>>
>> +/// already installed will count towards "Table Present" during
>>
>> +/// verification routine.
>>
>> +///
>>
>> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
>>
>> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
>>
>> +
>>
>> +///
>>
>> +/// Order of ACPI table being verified during presence inspection.
>>
>> +///
>>
>> +#define ACPI_TABLE_VERIFY_FADT   0
>>
>> +#define ACPI_TABLE_VERIFY_MADT   1
>>
>> +#define ACPI_TABLE_VERIFY_GTDT   2
>>
>> +#define ACPI_TABLE_VERIFY_DSDT   3
>>
>> +#define ACPI_TABLE_VERIFY_DBG2   4
>>
>> +#define ACPI_TABLE_VERIFY_SPCR   5
>>
>> +#define ACPI_TABLE_VERIFY_COUNT  6
>>
>> +
>>
>> +///
>>
>> +/// Private data structure to verify the presence of mandatory
>>
>> +/// or optional ACPI tables.
>>
>> +///
>>
>> +typedef struct {
>>
>> +  /// ESTD ID for the ACPI table of interest.
>>
>> +  ESTD_ACPI_TABLE_ID    EstdTableId;
>>
>> +  /// Standard UINT32 ACPI signature.
>>
>> +  UINT32                AcpiTableSignature;
>>
>> +  /// 4 character ACPI table name (the 5th char8 is for null 
>> terminator).
>>
>> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
>>
>> +  /// Indicator on whether the ACPI table is required.
>>
>> +  BOOLEAN               IsMandatory;
>>
>> +  /// Formats of verified presences, as defined by ACPI_TABLE_PRESENT_*
>>
>> +  /// This field should be initialized to 0 and will be populated 
>> during
>>
>> +  /// verification routine.
>>
>> +  UINT16                Presence;
>>
>> +} ACPI_TABLE_PRESENCE_INFO;
>>
>> +
>>
>> +///
>>
>> +/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
>>
>> +/// This list also include optional ACPI tables: DBG2, SPCR.
>>
>> +///
>>
>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
>>
>> +  { EStdAcpiTableIdFadt, 
>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE,  0 },
>>
>> +  { EStdAcpiTableIdMadt, 
>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", 
>> TRUE,  0 },
>>
>> +  { EStdAcpiTableIdGtdt, 
>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", 
>> TRUE,  0 },
>>
>> +  { EStdAcpiTableIdDsdt, 
>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, 
>> "DSDT", TRUE,  0 },
>>
>> +  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, 
>> "DBG2", FALSE, 0 },
>>
>> +  { EStdAcpiTableIdSpcr, 
>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR", 
>> FALSE, 0 },
>>
>> +};
>>
>> +
>>
>>   /** This macro expands to a function that retrieves the ACPI Table
>>
>>       List from the Configuration Manager.
>>
>>   */
>>
>> @@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
>>
>>     @retval EFI_SUCCESS           Success.
>>
>>     @retval EFI_NOT_FOUND         If mandatory table is not found.
>>
>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
>> AcpiTableInfo is already installed.
>>
>>   **/
>>
>>   STATIC
>>
>>   EFI_STATUS
>>
>> @@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
>>     IN       UINT32                              AcpiTableCount
>>
>>     )
>>
>>   {
>>
>> -  EFI_STATUS  Status;
>>
>> -  BOOLEAN     FadtFound;
>>
>> -  BOOLEAN     MadtFound;
>>
>> -  BOOLEAN     GtdtFound;
>>
>> -  BOOLEAN     DsdtFound;
>>
>> -  BOOLEAN     Dbg2Found;
>>
>> -  BOOLEAN     SpcrFound;
>>
>> +  EFI_STATUS                   Status;
>>
>> +  UINTN                        Handle;
>>
>> +  UINTN                        Index;
>>
>> +  UINTN                        InstalledTableIndex;
>>
>> +  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
>>
>> +  EFI_ACPI_TABLE_VERSION       Version;
>>
>> +  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
>>
>>
>> -  Status    = EFI_SUCCESS;
>>
>> -  FadtFound = FALSE;
>>
>> -  MadtFound = FALSE;
>>
>> -  GtdtFound = FALSE;
>>
>> -  DsdtFound = FALSE;
>>
>> -  Dbg2Found = FALSE;
>>
>> -  SpcrFound = FALSE;
>>
>>     ASSERT (AcpiTableInfo != NULL);
>>
>>
>> +  Status = EFI_SUCCESS;
>>
>> +
>>
>> +  // Check against the statically initialized ACPI tables to see if 
>> they are in ACPI info list
>>
>>     while (AcpiTableCount-- != 0) {
>>
>> -    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>>
>> -      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>>
>> -        FadtFound = TRUE;
>>
>> -        break;
>>
>> -      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>>
>> -        MadtFound = TRUE;
>>
>> -        break;
>>
>> -      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>>
>> -        GtdtFound = TRUE;
>>
>> -        break;
>>
>> -      case 
>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>>
>> -        DsdtFound = TRUE;
>>
>> -        break;
>>
>> -      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>>
>> -        Dbg2Found = TRUE;
>>
>> -        break;
>>
>> -      case 
>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>>
>> -        SpcrFound = TRUE;
>>
>> -        break;
>>
>> -      default:
>>
>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>
>> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == 
>> mAcpiVerifyTables[Index].AcpiTableSignature) {
>>
>> +        mAcpiVerifyTables[Index].Presence |= 
>> ACPI_TABLE_PRESENT_INFO_LIST;
>>
>> +        // Found this table, skip the rest.
>>
>>           break;
>>
>> +      }
>>
>>       }
>>
>>     }
>>
>>
>> -  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
>>
>> -  if (!FadtFound) {
>>
>> -    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>>
>> -    Status = EFI_NOT_FOUND;
>>
>> -  }
>>
>> +  // They also might be published already, so we can search from there
>>
>> +  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
>>
>> +    AcpiSdt = NULL;
>>
>> +    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, 
>> (VOID **)&AcpiSdt);
>>
>>
>> -  if (!MadtFound) {
>>
>> -    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>>
>> -    Status = EFI_NOT_FOUND;
>>
>> -  }
>>
>> +    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>>
>> +      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT 
>> protocol (0x%p) - %r\n", AcpiSdt, Status));
>>
>> +      return Status;
>>
>> +    }
>>
>>
>> -  if (!GtdtFound) {
>>
>> -    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>>
>> -    Status = EFI_NOT_FOUND;
>>
>> -  }
>>
>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>
>> +      Handle              = 0;
>>
>> +      InstalledTableIndex = 0;
>>
>> +      do {
>>
>> +        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, 
>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>>
>> +        if (EFI_ERROR (Status)) {

[SAMI] When running Kvmtool guest firmware I break from here with 
EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE 
in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any 
preinstalled tables (i.e. all tables are generated using 
DynamicTablesFramework).

This means platforms that use only Dynamic Tables Framework would all 
need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to 
rework this logic so that the existing platform code does not need 
updating, please?

[/SAMI]

>>
>> +          break;
>>
>> +        }
>>
>>
>> -  if (!DsdtFound) {
>>
>> -    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>>
>> -    Status = EFI_NOT_FOUND;
>>
>> -  }
>>
>> +        InstalledTableIndex++;
>>
>> +      } while (DescHeader->Signature != 
>> mAcpiVerifyTables[Index].AcpiTableSignature);
>>
>>
>> -  if (!Dbg2Found) {
>>
>> -    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>>
>> +      if (!EFI_ERROR (Status)) {
>>
>> +        mAcpiVerifyTables[Index].Presence |= 
>> ACPI_TABLE_PRESENT_INSTALLED;
>>
>> +      }
>>
>> +    }
>>
>>     }
>>
>>
>> -  if (!SpcrFound) {
>>
>> -    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>>
>> +  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>
>> +    if (mAcpiVerifyTables[Index].Presence == 0) {
>>
>> +      if (mAcpiVerifyTables[Index].IsMandatory) {
>>
>> +        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", 
>> mAcpiVerifyTables[Index].AcpiTableName));
>>
>> +        Status = EFI_NOT_FOUND;
>>
>> +      } else {
>>
>> +        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", 
>> mAcpiVerifyTables[Index].AcpiTableName));
>>
>> +      }
>>
>> +    } else if (mAcpiVerifyTables[Index].Presence ==
>>
>> +               (ACPI_TABLE_PRESENT_INFO_LIST | 
>> ACPI_TABLE_PRESENT_INSTALLED))
>>
>> +    {
>>
>> +      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already 
>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>>
>> +      Status = EFI_ALREADY_STARTED;
>>
>> +    }
>>
>>     }
>>
>>
>>     return Status;
>>
>> @@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
>>     @param [in]  CfgMgrProtocol       Pointer to the Configuration 
>> Manager
>>
>>                                       Protocol Interface.
>>
>>
>> -  @retval EFI_SUCCESS   Success.
>>
>> -  @retval EFI_NOT_FOUND If a mandatory table or a generator is not 
>> found.
>>
>> +  @retval EFI_SUCCESS           Success.
>>
>> +  @retval EFI_NOT_FOUND         If a mandatory table or a generator 
>> is not found.
>>
>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
>> AcpiTableInfo is already installed.
>>
>>   **/
>>
>>   STATIC
>>
>>   EFI_STATUS
>>
>> @@ -562,7 +613,7 @@ ProcessAcpiTables (
>>     if (EFI_ERROR (Status)) {
>>
>>       DEBUG ((
>>
>>         DEBUG_ERROR,
>>
>> -      "ERROR: Failed to find mandatory ACPI Table(s)."
>>
>> +      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>>
>>         " Status = %r\n",
>>
>>         Status
>>
>>         ));
>>
>> @@ -570,29 +621,32 @@ ProcessAcpiTables (
>>     }
>>
>>
>>     // Add the FADT Table first.
>>
>> -  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>
>> -    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>
>> -        AcpiTableInfo[Idx].TableGeneratorId)
>>
>> -    {
>>
>> -      Status = BuildAndInstallAcpiTable (
>>
>> -                 TableFactoryProtocol,
>>
>> -                 CfgMgrProtocol,
>>
>> -                 AcpiTableProtocol,
>>
>> -                 &AcpiTableInfo[Idx]
>>
>> -                 );
>>
>> -      if (EFI_ERROR (Status)) {
>>
>> -        DEBUG ((
>>
>> -          DEBUG_ERROR,
>>
>> -          "ERROR: Failed to find build and install ACPI FADT Table." \
>>
>> -          " Status = %r\n",
>>
>> -          Status
>>
>> -          ));
>>
>> -        return Status;
>>
>> -      }
>>
>> +  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & 
>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
>
> [SAMI] If I understand correctly, the mAcpiVerifyTables[x].Presence 
> filed cannot be ACPI_TABLE_PRESENT_INFO_LIST and 
> ACPI_TABLE_PRESENT_INSTALLED at the same time. Otherwise we would have 
> returned EFI_ALREADY_STARTED from VerifyMandatoryTablesArePresent().
>
> Since FADT is mandatory, the only valid conditions are:
>
> 1. ACPI_TABLE_PRESENT_INFO_LIST and !ACPI_TABLE_PRESENT_INSTALLED
>
> 2. !ACPI_TABLE_PRESENT_INFO_LIST & ACPI_TABLE_PRESENT_INSTALLED
>
> Therefore, I think the above check is not required. What do you think?
>
> [/SAMI]
>
>>
>> +    // FADT is not yet installed
>>
>> +    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>
>> +      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>
>> +          AcpiTableInfo[Idx].TableGeneratorId)
>>
>> +      {
>>
>> +        Status = BuildAndInstallAcpiTable (
>>
>> +                   TableFactoryProtocol,
>>
>> +                   CfgMgrProtocol,
>>
>> +                   AcpiTableProtocol,
>>
>> +                   &AcpiTableInfo[Idx]
>>
>> +                   );
>>
>> +        if (EFI_ERROR (Status)) {
>>
>> +          DEBUG ((
>>
>> +            DEBUG_ERROR,
>>
>> +            "ERROR: Failed to find build and install ACPI FADT 
>> Table." \
>>
>> +            " Status = %r\n",
>>
>> +            Status
>>
>> +            ));
>>
>> +          return Status;
>>
>> +        }
>>
>>
>> -      break;
>>
>> -    }
>>
>> -  } // for
>>
>> +        break;
>>
>> +      }
>>
>> +    } // for
>>
>> +  }
>>
>>
>>     // Add remaining ACPI Tables
>>
>>     for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>
>> diff --git 
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>
>> index 028c3d413cf8..ad8b3d037c16 100644
>> --- 
>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> +++ 
>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>> @@ -34,8 +34,12 @@ [LibraryClasses]
>>     UefiBootServicesTableLib
>>
>>     UefiDriverEntryPoint
>>
>>
>> +[FeaturePcd]
>>
>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol  ## CONSUMES
>>
>> +
>>
>>   [Protocols]
>>
>>     gEfiAcpiTableProtocolGuid                     # PROTOCOL 
>> ALWAYS_CONSUMED
>>
>> +  gEfiAcpiSdtProtocolGuid                       # PROTOCOL 
>> ALWAYS_CONSUMED
>>
>>
>>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL 
>> ALWAYS_CONSUMED
>>
>>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL 
>> ALWAYS_CONSUMED
>>

Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Kun Qin]

Hi Sami,

Thank you for taking time testing this change!

I have a question about one comment you have for this specific patch 
inline (marked with [KQ]).
Could you please provide more details?

I also responded to your other comments, please let me know if the 
proposed change makes
sense to you. Looking forward to your reply.

Thanks,
Kun

On 8/8/2022 8:39 AM, Sami Mujawar wrote:
> Hi Kun,
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 08/08/2022 02:05 pm, Sami Mujawar wrote:
>> Hi Kun,
>>
>> Thank you for this patch.
>>
>> Please find my response inline marked [SAMI].
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 31/07/2022 06:37 am, Kun Qin wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>>>
>>> This change added an extra step to allow check for installed ACPI 
>>> tables.
>>>
>>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either 
>>> pre-installed or
>>> supplied through AcpiTableInfo can be accepted.
>>>
>>> An extra check for FADT ACPI table existence during installation 
>>> step is
>>> also added.
>>>
>>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>
>>> Notes:
>>>      v2:
>>>      - Function description updates [Sami]
>>>      - Refactorized the table verification [Pierre]
>>>           v3:
>>>      - Added descriptions for new structures [Pierre]
>>>      - Added check for SDT protocol PCD before using it [Pierre]
>>>
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>>> | 214 ++++++++++++--------
>>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>> |   4 +
>>>   2 files changed, 138 insertions(+), 80 deletions(-)
>>>
>>> diff --git 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
>>>
>>> index ed62299f9bbd..7f3deef08a66 100644
>>> --- 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> +++ 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>>> @@ -10,6 +10,7 @@
>>>   #include <Library/DebugLib.h>
>>>
>>>   #include <Library/PcdLib.h>
>>>
>>>   #include <Library/UefiBootServicesTableLib.h>
>>>
>>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>>>
>>>   #include <Protocol/AcpiTable.h>
>>>
>>>
>>>   // Module specific include files.
>>>
>>> @@ -22,6 +23,58 @@
>>>   #include <Protocol/DynamicTableFactoryProtocol.h>
>>>
>>>   #include <SmbiosTableGenerator.h>
>>>
>>>
>>> +///
>>>
>>> +/// Bit definitions for acceptable ACPI table presence formats.
>>>
>>> +/// Currently only ACPI tables present in the ACPI info list and
>>>
>>> +/// already installed will count towards "Table Present" during
>>>
>>> +/// verification routine.
>>>
>>> +///
>>>
>>> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
>>>
>>> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
>>>
>>> +
>>>
>>> +///
>>>
>>> +/// Order of ACPI table being verified during presence inspection.
>>>
>>> +///
>>>
>>> +#define ACPI_TABLE_VERIFY_FADT   0
>>>
>>> +#define ACPI_TABLE_VERIFY_MADT   1
>>>
>>> +#define ACPI_TABLE_VERIFY_GTDT   2
>>>
>>> +#define ACPI_TABLE_VERIFY_DSDT   3
>>>
>>> +#define ACPI_TABLE_VERIFY_DBG2   4
>>>
>>> +#define ACPI_TABLE_VERIFY_SPCR   5
>>>
>>> +#define ACPI_TABLE_VERIFY_COUNT  6
>>>
>>> +
>>>
>>> +///
>>>
>>> +/// Private data structure to verify the presence of mandatory
>>>
>>> +/// or optional ACPI tables.
>>>
>>> +///
>>>
>>> +typedef struct {
>>>
>>> +  /// ESTD ID for the ACPI table of interest.
>>>
>>> +  ESTD_ACPI_TABLE_ID    EstdTableId;
>>>
>>> +  /// Standard UINT32 ACPI signature.
>>>
>>> +  UINT32                AcpiTableSignature;
>>>
>>> +  /// 4 character ACPI table name (the 5th char8 is for null 
>>> terminator).
>>>
>>> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
>>>
>>> +  /// Indicator on whether the ACPI table is required.
>>>
>>> +  BOOLEAN               IsMandatory;
>>>
>>> +  /// Formats of verified presences, as defined by 
>>> ACPI_TABLE_PRESENT_*
>>>
>>> +  /// This field should be initialized to 0 and will be populated 
>>> during
>>>
>>> +  /// verification routine.
>>>
>>> +  UINT16                Presence;
>>>
>>> +} ACPI_TABLE_PRESENCE_INFO;
>>>
>>> +
>>>
>>> +///
>>>
>>> +/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
>>>
>>> +/// This list also include optional ACPI tables: DBG2, SPCR.
>>>
>>> +///
>>>
>>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] 
>>> = {
>>>
>>> +  { EStdAcpiTableIdFadt, 
>>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE,  
>>> 0 },
>>>
>>> +  { EStdAcpiTableIdMadt, 
>>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", 
>>> TRUE,  0 },
>>>
>>> +  { EStdAcpiTableIdGtdt, 
>>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", 
>>> TRUE,  0 },
>>>
>>> +  { EStdAcpiTableIdDsdt, 
>>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, 
>>> "DSDT", TRUE,  0 },
>>>
>>> +  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, 
>>> "DBG2", FALSE, 0 },
>>>
>>> +  { EStdAcpiTableIdSpcr, 
>>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, 
>>> "SPCR", FALSE, 0 },
>>>
>>> +};
>>>
>>> +
>>>
>>>   /** This macro expands to a function that retrieves the ACPI Table
>>>
>>>       List from the Configuration Manager.
>>>
>>>   */
>>>
>>> @@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
>>>
>>>     @retval EFI_SUCCESS           Success.
>>>
>>>     @retval EFI_NOT_FOUND         If mandatory table is not found.
>>>
>>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
>>> AcpiTableInfo is already installed.
>>>
>>>   **/
>>>
>>>   STATIC
>>>
>>>   EFI_STATUS
>>>
>>> @@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
>>>     IN       UINT32 AcpiTableCount
>>>
>>>     )
>>>
>>>   {
>>>
>>> -  EFI_STATUS  Status;
>>>
>>> -  BOOLEAN     FadtFound;
>>>
>>> -  BOOLEAN     MadtFound;
>>>
>>> -  BOOLEAN     GtdtFound;
>>>
>>> -  BOOLEAN     DsdtFound;
>>>
>>> -  BOOLEAN     Dbg2Found;
>>>
>>> -  BOOLEAN     SpcrFound;
>>>
>>> +  EFI_STATUS                   Status;
>>>
>>> +  UINTN                        Handle;
>>>
>>> +  UINTN                        Index;
>>>
>>> +  UINTN                        InstalledTableIndex;
>>>
>>> +  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
>>>
>>> +  EFI_ACPI_TABLE_VERSION       Version;
>>>
>>> +  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
>>>
>>>
>>> -  Status    = EFI_SUCCESS;
>>>
>>> -  FadtFound = FALSE;
>>>
>>> -  MadtFound = FALSE;
>>>
>>> -  GtdtFound = FALSE;
>>>
>>> -  DsdtFound = FALSE;
>>>
>>> -  Dbg2Found = FALSE;
>>>
>>> -  SpcrFound = FALSE;
>>>
>>>     ASSERT (AcpiTableInfo != NULL);
>>>
>>>
>>> +  Status = EFI_SUCCESS;
>>>
>>> +
>>>
>>> +  // Check against the statically initialized ACPI tables to see if 
>>> they are in ACPI info list
>>>
>>>     while (AcpiTableCount-- != 0) {
>>>
>>> -    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>>>
>>> -      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>>>
>>> -        FadtFound = TRUE;
>>>
>>> -        break;
>>>
>>> -      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>>>
>>> -        MadtFound = TRUE;
>>>
>>> -        break;
>>>
>>> -      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>>>
>>> -        GtdtFound = TRUE;
>>>
>>> -        break;
>>>
>>> -      case 
>>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>>>
>>> -        DsdtFound = TRUE;
>>>
>>> -        break;
>>>
>>> -      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>>>
>>> -        Dbg2Found = TRUE;
>>>
>>> -        break;
>>>
>>> -      case 
>>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>>>
>>> -        SpcrFound = TRUE;
>>>
>>> -        break;
>>>
>>> -      default:
>>>
>>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>>
>>> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == 
>>> mAcpiVerifyTables[Index].AcpiTableSignature) {
>>>
>>> +        mAcpiVerifyTables[Index].Presence |= 
>>> ACPI_TABLE_PRESENT_INFO_LIST;
>>>
>>> +        // Found this table, skip the rest.
>>>
>>>           break;
>>>
>>> +      }
>>>
>>>       }
>>>
>>>     }
>>>
>>>
>>> -  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
>>>
>>> -  if (!FadtFound) {
>>>
>>> -    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>>>
>>> -    Status = EFI_NOT_FOUND;
>>>
>>> -  }
>>>
>>> +  // They also might be published already, so we can search from there
>>>
>>> +  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
>>>
>>> +    AcpiSdt = NULL;
>>>
>>> +    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, 
>>> (VOID **)&AcpiSdt);
>>>
>>>
>>> -  if (!MadtFound) {
>>>
>>> -    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>>>
>>> -    Status = EFI_NOT_FOUND;
>>>
>>> -  }
>>>
>>> +    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>>>
>>> +      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT 
>>> protocol (0x%p) - %r\n", AcpiSdt, Status));
>>>
>>> +      return Status;
>>>
>>> +    }
>>>
>>>
>>> -  if (!GtdtFound) {
>>>
>>> -    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>>>
>>> -    Status = EFI_NOT_FOUND;
>>>
>>> -  }
>>>
>>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>>
>>> +      Handle              = 0;
>>>
>>> +      InstalledTableIndex = 0;
>>>
>>> +      do {
>>>
>>> +        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, 
>>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>>>
>>> +        if (EFI_ERROR (Status)) {
>
> [SAMI] When running Kvmtool guest firmware I break from here with 
> EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE 
> in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any 
> preinstalled tables (i.e. all tables are generated using 
> DynamicTablesFramework).
>
> This means platforms that use only Dynamic Tables Framework would all 
> need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to 
> rework this logic so that the existing platform code does not need 
> updating, please?
>
> [/SAMI]

[KQ]

There was a mistake that the Status should be set to EFI_SUCCESS before 
entering the presence
checking step. Otherwise it will remain the same value from GetAcpiTable 
if all tables are present.

To your original observation, it is correct that this GetAcpiTable call 
will fail with EFI_NOT_FOUND,
but it should only break from the internal `do-while` loop, and continue 
with the rest of table
look-ups. Then during table inspection step, either installed table or 
from info_list will count as a
passed check. The return Status should be reset before inspecting the 
full look-up results. Thanks
for catching this! Will add the `Status = EFI_SUCCESS` statement in the 
next round.

[/KQ]

>
>>>
>>> +          break;
>>>
>>> +        }
>>>
>>>
>>> -  if (!DsdtFound) {
>>>
>>> -    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>>>
>>> -    Status = EFI_NOT_FOUND;
>>>
>>> -  }
>>>
>>> +        InstalledTableIndex++;
>>>
>>> +      } while (DescHeader->Signature != 
>>> mAcpiVerifyTables[Index].AcpiTableSignature);
>>>
>>>
>>> -  if (!Dbg2Found) {
>>>
>>> -    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>>>
>>> +      if (!EFI_ERROR (Status)) {
>>>
>>> +        mAcpiVerifyTables[Index].Presence |= 
>>> ACPI_TABLE_PRESENT_INSTALLED;
>>>
>>> +      }
>>>
>>> +    }
>>>
>>>     }
>>>
>>>
>>> -  if (!SpcrFound) {
>>>
>>> -    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>>>
>>> +  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>>>
>>> +    if (mAcpiVerifyTables[Index].Presence == 0) {
>>>
>>> +      if (mAcpiVerifyTables[Index].IsMandatory) {
>>>
>>> +        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", 
>>> mAcpiVerifyTables[Index].AcpiTableName));
>>>
>>> +        Status = EFI_NOT_FOUND;
>>>
>>> +      } else {
>>>
>>> +        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", 
>>> mAcpiVerifyTables[Index].AcpiTableName));
>>>
>>> +      }
>>>
>>> +    } else if (mAcpiVerifyTables[Index].Presence ==
>>>
>>> +               (ACPI_TABLE_PRESENT_INFO_LIST | 
>>> ACPI_TABLE_PRESENT_INSTALLED))
>>>
>>> +    {
>>>
>>> +      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already 
>>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>>>
>>> +      Status = EFI_ALREADY_STARTED;
>>>
>>> +    }
>>>
>>>     }
>>>
>>>
>>>     return Status;
>>>
>>> @@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
>>>     @param [in]  CfgMgrProtocol       Pointer to the Configuration 
>>> Manager
>>>
>>>                                       Protocol Interface.
>>>
>>>
>>> -  @retval EFI_SUCCESS   Success.
>>>
>>> -  @retval EFI_NOT_FOUND If a mandatory table or a generator is not 
>>> found.
>>>
>>> +  @retval EFI_SUCCESS           Success.
>>>
>>> +  @retval EFI_NOT_FOUND         If a mandatory table or a generator 
>>> is not found.
>>>
>>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
>>> AcpiTableInfo is already installed.
>>>
>>>   **/
>>>
>>>   STATIC
>>>
>>>   EFI_STATUS
>>>
>>> @@ -562,7 +613,7 @@ ProcessAcpiTables (
>>>     if (EFI_ERROR (Status)) {
>>>
>>>       DEBUG ((
>>>
>>>         DEBUG_ERROR,
>>>
>>> -      "ERROR: Failed to find mandatory ACPI Table(s)."
>>>
>>> +      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>>>
>>>         " Status = %r\n",
>>>
>>>         Status
>>>
>>>         ));
>>>
>>> @@ -570,29 +621,32 @@ ProcessAcpiTables (
>>>     }
>>>
>>>
>>>     // Add the FADT Table first.
>>>
>>> -  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>>
>>> -    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>>
>>> -        AcpiTableInfo[Idx].TableGeneratorId)
>>>
>>> -    {
>>>
>>> -      Status = BuildAndInstallAcpiTable (
>>>
>>> -                 TableFactoryProtocol,
>>>
>>> -                 CfgMgrProtocol,
>>>
>>> -                 AcpiTableProtocol,
>>>
>>> -                 &AcpiTableInfo[Idx]
>>>
>>> -                 );
>>>
>>> -      if (EFI_ERROR (Status)) {
>>>
>>> -        DEBUG ((
>>>
>>> -          DEBUG_ERROR,
>>>
>>> -          "ERROR: Failed to find build and install ACPI FADT Table." \
>>>
>>> -          " Status = %r\n",
>>>
>>> -          Status
>>>
>>> -          ));
>>>
>>> -        return Status;
>>>
>>> -      }
>>>
>>> +  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & 
>>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
>>
>> [SAMI] If I understand correctly, the mAcpiVerifyTables[x].Presence 
>> filed cannot be ACPI_TABLE_PRESENT_INFO_LIST and 
>> ACPI_TABLE_PRESENT_INSTALLED at the same time. Otherwise we would 
>> have returned EFI_ALREADY_STARTED from 
>> VerifyMandatoryTablesArePresent().
>>
>> Since FADT is mandatory, the only valid conditions are:
>>
>> 1. ACPI_TABLE_PRESENT_INFO_LIST and !ACPI_TABLE_PRESENT_INSTALLED
>>
>> 2. !ACPI_TABLE_PRESENT_INFO_LIST & ACPI_TABLE_PRESENT_INSTALLED
>>
>> Therefore, I think the above check is not required. What do you think?
>>
>> [/SAMI]

[KQ]

You are correct that there will be only above 2 conditions for mandatory 
tables.
But the check is to make sure the FADT will only be installed on 
condition #1 above,
which means it will only be installed if it has not already been 
installed. Please let
me know if I missed anything here.

[/KQ]

>>
>>>
>>> +    // FADT is not yet installed
>>>
>>> +    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>>
>>> +      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>>>
>>> +          AcpiTableInfo[Idx].TableGeneratorId)
>>>
>>> +      {
>>>
>>> +        Status = BuildAndInstallAcpiTable (
>>>
>>> +                   TableFactoryProtocol,
>>>
>>> +                   CfgMgrProtocol,
>>>
>>> +                   AcpiTableProtocol,
>>>
>>> +                   &AcpiTableInfo[Idx]
>>>
>>> +                   );
>>>
>>> +        if (EFI_ERROR (Status)) {
>>>
>>> +          DEBUG ((
>>>
>>> +            DEBUG_ERROR,
>>>
>>> +            "ERROR: Failed to find build and install ACPI FADT 
>>> Table." \
>>>
>>> +            " Status = %r\n",
>>>
>>> +            Status
>>>
>>> +            ));
>>>
>>> +          return Status;
>>>
>>> +        }
>>>
>>>
>>> -      break;
>>>
>>> -    }
>>>
>>> -  } // for
>>>
>>> +        break;
>>>
>>> +      }
>>>
>>> +    } // for
>>>
>>> +  }
>>>
>>>
>>>     // Add remaining ACPI Tables
>>>
>>>     for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>>>
>>> diff --git 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
>>>
>>> index 028c3d413cf8..ad8b3d037c16 100644
>>> --- 
>>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> +++ 
>>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>>> @@ -34,8 +34,12 @@ [LibraryClasses]
>>>     UefiBootServicesTableLib
>>>
>>>     UefiDriverEntryPoint
>>>
>>>
>>> +[FeaturePcd]
>>>
>>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol ## CONSUMES
>>>
>>> +
>>>
>>>   [Protocols]
>>>
>>>     gEfiAcpiTableProtocolGuid                     # PROTOCOL 
>>> ALWAYS_CONSUMED
>>>
>>> +  gEfiAcpiSdtProtocolGuid                       # PROTOCOL 
>>> ALWAYS_CONSUMED
>>>
>>>
>>>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL 
>>> ALWAYS_CONSUMED
>>>
>>>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL 
>>> ALWAYS_CONSUMED
>>>

Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Sami Mujawar]

Hi Kun,

Please find my response inline marked [SAMI].

Regards,

Sami Mujawar

On 09/08/2022, 00:44, "Kun Qin" <kuqin12@gmail.com> wrote:

    Hi Sami,

    Thank you for taking time testing this change!

    I have a question about one comment you have for this specific patch 
    inline (marked with [KQ]).
    Could you please provide more details?

    I also responded to your other comments, please let me know if the 
    proposed change makes
    sense to you. Looking forward to your reply.

    Thanks,
    Kun

    On 8/8/2022 8:39 AM, Sami Mujawar wrote:
    > Hi Kun,
    >
    > Please find my response inline marked [SAMI].
    >
    > Regards,
    >
    > Sami Mujawar
    >
    > On 08/08/2022 02:05 pm, Sami Mujawar wrote:
    >> Hi Kun,
    >>
    >> Thank you for this patch.
    >>
    >> Please find my response inline marked [SAMI].
    >>
    >> Regards,
    >>
    >> Sami Mujawar
    >>
    >> On 31/07/2022 06:37 am, Kun Qin wrote:
    >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
    >>>
    >>> This change added an extra step to allow check for installed ACPI 
    >>> tables.
    >>>
    >>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either 
    >>> pre-installed or
    >>> supplied through AcpiTableInfo can be accepted.
    >>>
    >>> An extra check for FADT ACPI table existence during installation 
    >>> step is
    >>> also added.
    >>>
    >>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
    >>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
    >>>
    >>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
    >>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
    >>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
    >>> ---
    >>>
    >>> Notes:
    >>>      v2:
    >>>      - Function description updates [Sami]
    >>>      - Refactorized the table verification [Pierre]
    >>>           v3:
    >>>      - Added descriptions for new structures [Pierre]
    >>>      - Added check for SDT protocol PCD before using it [Pierre]
    >>>
    >>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
    >>> | 214 ++++++++++++--------
    >>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
    >>> |   4 +
    >>>   2 files changed, 138 insertions(+), 80 deletions(-)
    >>>
    >>> diff --git 
    >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
    >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c 
    >>>
    >>> index ed62299f9bbd..7f3deef08a66 100644
    >>> --- 
    >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
    >>> +++ 
    >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
    >>> @@ -10,6 +10,7 @@
    >>>   #include <Library/DebugLib.h>
    >>>
    >>>   #include <Library/PcdLib.h>
    >>>
    >>>   #include <Library/UefiBootServicesTableLib.h>
    >>>
    >>> +#include <Protocol/AcpiSystemDescriptionTable.h>
    >>>
    >>>   #include <Protocol/AcpiTable.h>
    >>>
    >>>
    >>>   // Module specific include files.
    >>>
    >>> @@ -22,6 +23,58 @@
    >>>   #include <Protocol/DynamicTableFactoryProtocol.h>
    >>>
    >>>   #include <SmbiosTableGenerator.h>
    >>>
    >>>
    >>> +///
    >>>
    >>> +/// Bit definitions for acceptable ACPI table presence formats.
    >>>
    >>> +/// Currently only ACPI tables present in the ACPI info list and
    >>>
    >>> +/// already installed will count towards "Table Present" during
    >>>
    >>> +/// verification routine.
    >>>
    >>> +///
    >>>
    >>> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
    >>>
    >>> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
    >>>
    >>> +
    >>>
    >>> +///
    >>>
    >>> +/// Order of ACPI table being verified during presence inspection.
    >>>
    >>> +///
    >>>
    >>> +#define ACPI_TABLE_VERIFY_FADT   0
    >>>
    >>> +#define ACPI_TABLE_VERIFY_MADT   1
    >>>
    >>> +#define ACPI_TABLE_VERIFY_GTDT   2
    >>>
    >>> +#define ACPI_TABLE_VERIFY_DSDT   3
    >>>
    >>> +#define ACPI_TABLE_VERIFY_DBG2   4
    >>>
    >>> +#define ACPI_TABLE_VERIFY_SPCR   5
    >>>
    >>> +#define ACPI_TABLE_VERIFY_COUNT  6
    >>>
    >>> +
    >>>
    >>> +///
    >>>
    >>> +/// Private data structure to verify the presence of mandatory
    >>>
    >>> +/// or optional ACPI tables.
    >>>
    >>> +///
    >>>
    >>> +typedef struct {
    >>>
    >>> +  /// ESTD ID for the ACPI table of interest.
    >>>
    >>> +  ESTD_ACPI_TABLE_ID    EstdTableId;
    >>>
    >>> +  /// Standard UINT32 ACPI signature.
    >>>
    >>> +  UINT32                AcpiTableSignature;
    >>>
    >>> +  /// 4 character ACPI table name (the 5th char8 is for null 
    >>> terminator).
    >>>
    >>> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
    >>>
    >>> +  /// Indicator on whether the ACPI table is required.
    >>>
    >>> +  BOOLEAN               IsMandatory;
    >>>
    >>> +  /// Formats of verified presences, as defined by 
    >>> ACPI_TABLE_PRESENT_*
    >>>
    >>> +  /// This field should be initialized to 0 and will be populated 
    >>> during
    >>>
    >>> +  /// verification routine.
    >>>
    >>> +  UINT16                Presence;
    >>>
    >>> +} ACPI_TABLE_PRESENCE_INFO;
    >>>
    >>> +
    >>>
    >>> +///
    >>>
    >>> +/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
    >>>
    >>> +/// This list also include optional ACPI tables: DBG2, SPCR.
    >>>
    >>> +///
    >>>
    >>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] 
    >>> = {
    >>>
    >>> +  { EStdAcpiTableIdFadt, 
    >>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE,  
    >>> 0 },
    >>>
    >>> +  { EStdAcpiTableIdMadt, 
    >>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", 
    >>> TRUE,  0 },
    >>>
    >>> +  { EStdAcpiTableIdGtdt, 
    >>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", 
    >>> TRUE,  0 },
    >>>
    >>> +  { EStdAcpiTableIdDsdt, 
    >>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, 
    >>> "DSDT", TRUE,  0 },
    >>>
    >>> +  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, 
    >>> "DBG2", FALSE, 0 },
    >>>
    >>> +  { EStdAcpiTableIdSpcr, 
    >>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, 
    >>> "SPCR", FALSE, 0 },
    >>>
    >>> +};
    >>>
    >>> +
    >>>
    >>>   /** This macro expands to a function that retrieves the ACPI Table
    >>>
    >>>       List from the Configuration Manager.
    >>>
    >>>   */
    >>>
    >>> @@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
    >>>
    >>>     @retval EFI_SUCCESS           Success.
    >>>
    >>>     @retval EFI_NOT_FOUND         If mandatory table is not found.
    >>>
    >>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
    >>> AcpiTableInfo is already installed.
    >>>
    >>>   **/
    >>>
    >>>   STATIC
    >>>
    >>>   EFI_STATUS
    >>>
    >>> @@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
    >>>     IN       UINT32 AcpiTableCount
    >>>
    >>>     )
    >>>
    >>>   {
    >>>
    >>> -  EFI_STATUS  Status;
    >>>
    >>> -  BOOLEAN     FadtFound;
    >>>
    >>> -  BOOLEAN     MadtFound;
    >>>
    >>> -  BOOLEAN     GtdtFound;
    >>>
    >>> -  BOOLEAN     DsdtFound;
    >>>
    >>> -  BOOLEAN     Dbg2Found;
    >>>
    >>> -  BOOLEAN     SpcrFound;
    >>>
    >>> +  EFI_STATUS                   Status;
    >>>
    >>> +  UINTN                        Handle;
    >>>
    >>> +  UINTN                        Index;
    >>>
    >>> +  UINTN                        InstalledTableIndex;
    >>>
    >>> +  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
    >>>
    >>> +  EFI_ACPI_TABLE_VERSION       Version;
    >>>
    >>> +  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
    >>>
    >>>
    >>> -  Status    = EFI_SUCCESS;
    >>>
    >>> -  FadtFound = FALSE;
    >>>
    >>> -  MadtFound = FALSE;
    >>>
    >>> -  GtdtFound = FALSE;
    >>>
    >>> -  DsdtFound = FALSE;
    >>>
    >>> -  Dbg2Found = FALSE;
    >>>
    >>> -  SpcrFound = FALSE;
    >>>
    >>>     ASSERT (AcpiTableInfo != NULL);
    >>>
    >>>
    >>> +  Status = EFI_SUCCESS;
    >>>
    >>> +
    >>>
    >>> +  // Check against the statically initialized ACPI tables to see if 
    >>> they are in ACPI info list
    >>>
    >>>     while (AcpiTableCount-- != 0) {
    >>>
    >>> -    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
    >>>
    >>> -      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
    >>>
    >>> -        FadtFound = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
    >>>
    >>> -        MadtFound = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
    >>>
    >>> -        GtdtFound = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      case 
    >>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
    >>>
    >>> -        DsdtFound = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
    >>>
    >>> -        Dbg2Found = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      case 
    >>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
    >>>
    >>> -        SpcrFound = TRUE;
    >>>
    >>> -        break;
    >>>
    >>> -      default:
    >>>
    >>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
    >>>
    >>> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == 
    >>> mAcpiVerifyTables[Index].AcpiTableSignature) {
    >>>
    >>> +        mAcpiVerifyTables[Index].Presence |= 
    >>> ACPI_TABLE_PRESENT_INFO_LIST;
    >>>
    >>> +        // Found this table, skip the rest.
    >>>
    >>>           break;
    >>>
    >>> +      }
    >>>
    >>>       }
    >>>
    >>>     }
    >>>
    >>>
    >>> -  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
    >>>
    >>> -  if (!FadtFound) {
    >>>
    >>> -    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
    >>>
    >>> -    Status = EFI_NOT_FOUND;
    >>>
    >>> -  }
    >>>
    >>> +  // They also might be published already, so we can search from there
    >>>
    >>> +  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
    >>>
    >>> +    AcpiSdt = NULL;
    >>>
    >>> +    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, 
    >>> (VOID **)&AcpiSdt);
    >>>
    >>>
    >>> -  if (!MadtFound) {
    >>>
    >>> -    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
    >>>
    >>> -    Status = EFI_NOT_FOUND;
    >>>
    >>> -  }
    >>>
    >>> +    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
    >>>
    >>> +      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT 
    >>> protocol (0x%p) - %r\n", AcpiSdt, Status));
    >>>
    >>> +      return Status;
    >>>
    >>> +    }
    >>>
    >>>
    >>> -  if (!GtdtFound) {
    >>>
    >>> -    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
    >>>
    >>> -    Status = EFI_NOT_FOUND;
    >>>
    >>> -  }
    >>>
    >>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
    >>>
    >>> +      Handle              = 0;
    >>>
    >>> +      InstalledTableIndex = 0;
    >>>
    >>> +      do {
    >>>
    >>> +        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, 
    >>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
    >>>
    >>> +        if (EFI_ERROR (Status)) {
    >
    > [SAMI] When running Kvmtool guest firmware I break from here with 
    > EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE 
    > in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any 
    > preinstalled tables (i.e. all tables are generated using 
    > DynamicTablesFramework).
    >
    > This means platforms that use only Dynamic Tables Framework would all 
    > need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to 
    > rework this logic so that the existing platform code does not need 
    > updating, please?
    >
    > [/SAMI]

    [KQ]

    There was a mistake that the Status should be set to EFI_SUCCESS before 
    entering the presence
    checking step. Otherwise it will remain the same value from GetAcpiTable 
    if all tables are present.

    To your original observation, it is correct that this GetAcpiTable call 
    will fail with EFI_NOT_FOUND,
    but it should only break from the internal `do-while` loop, and continue 
    with the rest of table
    look-ups. Then during table inspection step, either installed table or 
    from info_list will count as a
    passed check. The return Status should be reset before inspecting the 
    full look-up results. Thanks
    for catching this! Will add the `Status = EFI_SUCCESS` statement in the 
    next round.

    [/KQ]

[SAMI] Ack. I look forward to an updated patch with this fixed.

    >
    >>>
    >>> +          break;
    >>>
    >>> +        }
    >>>
    >>>
    >>> -  if (!DsdtFound) {
    >>>
    >>> -    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
    >>>
    >>> -    Status = EFI_NOT_FOUND;
    >>>
    >>> -  }
    >>>
    >>> +        InstalledTableIndex++;
    >>>
    >>> +      } while (DescHeader->Signature != 
    >>> mAcpiVerifyTables[Index].AcpiTableSignature);
    >>>
    >>>
    >>> -  if (!Dbg2Found) {
    >>>
    >>> -    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
    >>>
    >>> +      if (!EFI_ERROR (Status)) {
    >>>
    >>> +        mAcpiVerifyTables[Index].Presence |= 
    >>> ACPI_TABLE_PRESENT_INSTALLED;
    >>>
    >>> +      }
    >>>
    >>> +    }
    >>>
    >>>     }
    >>>
    >>>
    >>> -  if (!SpcrFound) {
    >>>
    >>> -    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
    >>>
    >>> +  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
    >>>
    >>> +    if (mAcpiVerifyTables[Index].Presence == 0) {
    >>>
    >>> +      if (mAcpiVerifyTables[Index].IsMandatory) {
    >>>
    >>> +        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", 
    >>> mAcpiVerifyTables[Index].AcpiTableName));
    >>>
    >>> +        Status = EFI_NOT_FOUND;
    >>>
    >>> +      } else {
    >>>
    >>> +        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", 
    >>> mAcpiVerifyTables[Index].AcpiTableName));
    >>>
    >>> +      }
    >>>
    >>> +    } else if (mAcpiVerifyTables[Index].Presence ==
    >>>
    >>> +               (ACPI_TABLE_PRESENT_INFO_LIST | 
    >>> ACPI_TABLE_PRESENT_INSTALLED))
    >>>
    >>> +    {
    >>>
    >>> +      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already 
    >>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
    >>>
    >>> +      Status = EFI_ALREADY_STARTED;
    >>>
    >>> +    }
    >>>
    >>>     }
    >>>
    >>>
    >>>     return Status;
    >>>
    >>> @@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
    >>>     @param [in]  CfgMgrProtocol       Pointer to the Configuration 
    >>> Manager
    >>>
    >>>                                       Protocol Interface.
    >>>
    >>>
    >>> -  @retval EFI_SUCCESS   Success.
    >>>
    >>> -  @retval EFI_NOT_FOUND If a mandatory table or a generator is not 
    >>> found.
    >>>
    >>> +  @retval EFI_SUCCESS           Success.
    >>>
    >>> +  @retval EFI_NOT_FOUND         If a mandatory table or a generator 
    >>> is not found.
    >>>
    >>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in 
    >>> AcpiTableInfo is already installed.
    >>>
    >>>   **/
    >>>
    >>>   STATIC
    >>>
    >>>   EFI_STATUS
    >>>
    >>> @@ -562,7 +613,7 @@ ProcessAcpiTables (
    >>>     if (EFI_ERROR (Status)) {
    >>>
    >>>       DEBUG ((
    >>>
    >>>         DEBUG_ERROR,
    >>>
    >>> -      "ERROR: Failed to find mandatory ACPI Table(s)."
    >>>
    >>> +      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
    >>>
    >>>         " Status = %r\n",
    >>>
    >>>         Status
    >>>
    >>>         ));
    >>>
    >>> @@ -570,29 +621,32 @@ ProcessAcpiTables (
    >>>     }
    >>>
    >>>
    >>>     // Add the FADT Table first.
    >>>
    >>> -  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
    >>>
    >>> -    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
    >>>
    >>> -        AcpiTableInfo[Idx].TableGeneratorId)
    >>>
    >>> -    {
    >>>
    >>> -      Status = BuildAndInstallAcpiTable (
    >>>
    >>> -                 TableFactoryProtocol,
    >>>
    >>> -                 CfgMgrProtocol,
    >>>
    >>> -                 AcpiTableProtocol,
    >>>
    >>> -                 &AcpiTableInfo[Idx]
    >>>
    >>> -                 );
    >>>
    >>> -      if (EFI_ERROR (Status)) {
    >>>
    >>> -        DEBUG ((
    >>>
    >>> -          DEBUG_ERROR,
    >>>
    >>> -          "ERROR: Failed to find build and install ACPI FADT Table." \
    >>>
    >>> -          " Status = %r\n",
    >>>
    >>> -          Status
    >>>
    >>> -          ));
    >>>
    >>> -        return Status;
    >>>
    >>> -      }
    >>>
    >>> +  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & 
    >>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
    >>
    >> [SAMI] If I understand correctly, the mAcpiVerifyTables[x].Presence 
    >> filed cannot be ACPI_TABLE_PRESENT_INFO_LIST and 
    >> ACPI_TABLE_PRESENT_INSTALLED at the same time. Otherwise we would 
    >> have returned EFI_ALREADY_STARTED from 
    >> VerifyMandatoryTablesArePresent().
    >>
    >> Since FADT is mandatory, the only valid conditions are:
    >>
    >> 1. ACPI_TABLE_PRESENT_INFO_LIST and !ACPI_TABLE_PRESENT_INSTALLED
    >>
    >> 2. !ACPI_TABLE_PRESENT_INFO_LIST & ACPI_TABLE_PRESENT_INSTALLED
    >>
    >> Therefore, I think the above check is not required. What do you think?
    >>
    >> [/SAMI]

    [KQ]

    You are correct that there will be only above 2 conditions for mandatory 
    tables.
    But the check is to make sure the FADT will only be installed on 
    condition #1 above,
    which means it will only be installed if it has not already been 
    installed. Please let
    me know if I missed anything here.

    [/KQ]
[SAMI] Ack. Please keep this check. If the table is already installed this check prevents searching in the AcpiTableInfo list and therefore optimises.
    >>
    >>>
    >>> +    // FADT is not yet installed
    >>>
    >>> +    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
    >>>
    >>> +      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
    >>>
    >>> +          AcpiTableInfo[Idx].TableGeneratorId)
    >>>
    >>> +      {
    >>>
    >>> +        Status = BuildAndInstallAcpiTable (
    >>>
    >>> +                   TableFactoryProtocol,
    >>>
    >>> +                   CfgMgrProtocol,
    >>>
    >>> +                   AcpiTableProtocol,
    >>>
    >>> +                   &AcpiTableInfo[Idx]
    >>>
    >>> +                   );
    >>>
    >>> +        if (EFI_ERROR (Status)) {
    >>>
    >>> +          DEBUG ((
    >>>
    >>> +            DEBUG_ERROR,
    >>>
    >>> +            "ERROR: Failed to find build and install ACPI FADT 
    >>> Table." \
    >>>
    >>> +            " Status = %r\n",
    >>>
    >>> +            Status
    >>>
    >>> +            ));
    >>>
    >>> +          return Status;
    >>>
    >>> +        }
    >>>
    >>>
    >>> -      break;
    >>>
    >>> -    }
    >>>
    >>> -  } // for
    >>>
    >>> +        break;
    >>>
    >>> +      }
    >>>
    >>> +    } // for
    >>>
    >>> +  }
    >>>
    >>>
    >>>     // Add remaining ACPI Tables
    >>>
    >>>     for (Idx = 0; Idx < AcpiTableCount; Idx++) {
    >>>
    >>> diff --git 
    >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
    >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf 
    >>>
    >>> index 028c3d413cf8..ad8b3d037c16 100644
    >>> --- 
    >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
    >>> +++ 
    >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
    >>> @@ -34,8 +34,12 @@ [LibraryClasses]
    >>>     UefiBootServicesTableLib
    >>>
    >>>     UefiDriverEntryPoint
    >>>
    >>>
    >>> +[FeaturePcd]
    >>>
    >>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol ## CONSUMES
    >>>
    >>> +
    >>>
    >>>   [Protocols]
    >>>
    >>>     gEfiAcpiTableProtocolGuid                     # PROTOCOL 
    >>> ALWAYS_CONSUMED
    >>>
    >>> +  gEfiAcpiSdtProtocolGuid                       # PROTOCOL 
    >>> ALWAYS_CONSUMED
    >>>
    >>>
    >>>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL 
    >>> ALWAYS_CONSUMED
    >>>
    >>>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL 
    >>> ALWAYS_CONSUMED
    >>>

Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

[Kun Qin]

Hi Sami,

Thanks for your Acks below. The updated patch is sent here:
https://edk2.groups.io/g/devel/message/92317

Please let me know if there is any further feedback/issues.

Regards,
Kun

On 8/9/2022 2:01 AM, Sami Mujawar wrote:
> Hi Kun,
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 09/08/2022, 00:44, "Kun Qin" <kuqin12@gmail.com> wrote:
>
>      Hi Sami,
>
>      Thank you for taking time testing this change!
>
>      I have a question about one comment you have for this specific patch
>      inline (marked with [KQ]).
>      Could you please provide more details?
>
>      I also responded to your other comments, please let me know if the
>      proposed change makes
>      sense to you. Looking forward to your reply.
>
>      Thanks,
>      Kun
>
>      On 8/8/2022 8:39 AM, Sami Mujawar wrote:
>      > Hi Kun,
>      >
>      > Please find my response inline marked [SAMI].
>      >
>      > Regards,
>      >
>      > Sami Mujawar
>      >
>      > On 08/08/2022 02:05 pm, Sami Mujawar wrote:
>      >> Hi Kun,
>      >>
>      >> Thank you for this patch.
>      >>
>      >> Please find my response inline marked [SAMI].
>      >>
>      >> Regards,
>      >>
>      >> Sami Mujawar
>      >>
>      >> On 31/07/2022 06:37 am, Kun Qin wrote:
>      >>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
>      >>>
>      >>> This change added an extra step to allow check for installed ACPI
>      >>> tables.
>      >>>
>      >>> For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either
>      >>> pre-installed or
>      >>> supplied through AcpiTableInfo can be accepted.
>      >>>
>      >>> An extra check for FADT ACPI table existence during installation
>      >>> step is
>      >>> also added.
>      >>>
>      >>> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
>      >>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>      >>>
>      >>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>      >>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>      >>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>      >>> ---
>      >>>
>      >>> Notes:
>      >>>      v2:
>      >>>      - Function description updates [Sami]
>      >>>      - Refactorized the table verification [Pierre]
>      >>>           v3:
>      >>>      - Added descriptions for new structures [Pierre]
>      >>>      - Added check for SDT protocol PCD before using it [Pierre]
>      >>>
>      >>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>      >>> | 214 ++++++++++++--------
>      >>> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>      >>> |   4 +
>      >>>   2 files changed, 138 insertions(+), 80 deletions(-)
>      >>>
>      >>> diff --git
>      >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>      >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>      >>>
>      >>> index ed62299f9bbd..7f3deef08a66 100644
>      >>> ---
>      >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>      >>> +++
>      >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
>      >>> @@ -10,6 +10,7 @@
>      >>>   #include <Library/DebugLib.h>
>      >>>
>      >>>   #include <Library/PcdLib.h>
>      >>>
>      >>>   #include <Library/UefiBootServicesTableLib.h>
>      >>>
>      >>> +#include <Protocol/AcpiSystemDescriptionTable.h>
>      >>>
>      >>>   #include <Protocol/AcpiTable.h>
>      >>>
>      >>>
>      >>>   // Module specific include files.
>      >>>
>      >>> @@ -22,6 +23,58 @@
>      >>>   #include <Protocol/DynamicTableFactoryProtocol.h>
>      >>>
>      >>>   #include <SmbiosTableGenerator.h>
>      >>>
>      >>>
>      >>> +///
>      >>>
>      >>> +/// Bit definitions for acceptable ACPI table presence formats.
>      >>>
>      >>> +/// Currently only ACPI tables present in the ACPI info list and
>      >>>
>      >>> +/// already installed will count towards "Table Present" during
>      >>>
>      >>> +/// verification routine.
>      >>>
>      >>> +///
>      >>>
>      >>> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
>      >>>
>      >>> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
>      >>>
>      >>> +
>      >>>
>      >>> +///
>      >>>
>      >>> +/// Order of ACPI table being verified during presence inspection.
>      >>>
>      >>> +///
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_FADT   0
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_MADT   1
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_GTDT   2
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_DSDT   3
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_DBG2   4
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_SPCR   5
>      >>>
>      >>> +#define ACPI_TABLE_VERIFY_COUNT  6
>      >>>
>      >>> +
>      >>>
>      >>> +///
>      >>>
>      >>> +/// Private data structure to verify the presence of mandatory
>      >>>
>      >>> +/// or optional ACPI tables.
>      >>>
>      >>> +///
>      >>>
>      >>> +typedef struct {
>      >>>
>      >>> +  /// ESTD ID for the ACPI table of interest.
>      >>>
>      >>> +  ESTD_ACPI_TABLE_ID    EstdTableId;
>      >>>
>      >>> +  /// Standard UINT32 ACPI signature.
>      >>>
>      >>> +  UINT32                AcpiTableSignature;
>      >>>
>      >>> +  /// 4 character ACPI table name (the 5th char8 is for null
>      >>> terminator).
>      >>>
>      >>> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
>      >>>
>      >>> +  /// Indicator on whether the ACPI table is required.
>      >>>
>      >>> +  BOOLEAN               IsMandatory;
>      >>>
>      >>> +  /// Formats of verified presences, as defined by
>      >>> ACPI_TABLE_PRESENT_*
>      >>>
>      >>> +  /// This field should be initialized to 0 and will be populated
>      >>> during
>      >>>
>      >>> +  /// verification routine.
>      >>>
>      >>> +  UINT16                Presence;
>      >>>
>      >>> +} ACPI_TABLE_PRESENCE_INFO;
>      >>>
>      >>> +
>      >>>
>      >>> +///
>      >>>
>      >>> +/// We require the FADT, MADT, GTDT and the DSDT tables to boot.
>      >>>
>      >>> +/// This list also include optional ACPI tables: DBG2, SPCR.
>      >>>
>      >>> +///
>      >>>
>      >>> +ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT]
>      >>> = {
>      >>>
>      >>> +  { EStdAcpiTableIdFadt,
>      >>> EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE,
>      >>> 0 },
>      >>>
>      >>> +  { EStdAcpiTableIdMadt,
>      >>> EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT",
>      >>> TRUE,  0 },
>      >>>
>      >>> +  { EStdAcpiTableIdGtdt,
>      >>> EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT",
>      >>> TRUE,  0 },
>      >>>
>      >>> +  { EStdAcpiTableIdDsdt,
>      >>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
>      >>> "DSDT", TRUE,  0 },
>      >>>
>      >>> +  { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,
>      >>> "DBG2", FALSE, 0 },
>      >>>
>      >>> +  { EStdAcpiTableIdSpcr,
>      >>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,
>      >>> "SPCR", FALSE, 0 },
>      >>>
>      >>> +};
>      >>>
>      >>> +
>      >>>
>      >>>   /** This macro expands to a function that retrieves the ACPI Table
>      >>>
>      >>>       List from the Configuration Manager.
>      >>>
>      >>>   */
>      >>>
>      >>> @@ -395,6 +448,7 @@ BuildAndInstallAcpiTable (
>      >>>
>      >>>     @retval EFI_SUCCESS           Success.
>      >>>
>      >>>     @retval EFI_NOT_FOUND         If mandatory table is not found.
>      >>>
>      >>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in
>      >>> AcpiTableInfo is already installed.
>      >>>
>      >>>   **/
>      >>>
>      >>>   STATIC
>      >>>
>      >>>   EFI_STATUS
>      >>>
>      >>> @@ -404,75 +458,71 @@ VerifyMandatoryTablesArePresent (
>      >>>     IN       UINT32 AcpiTableCount
>      >>>
>      >>>     )
>      >>>
>      >>>   {
>      >>>
>      >>> -  EFI_STATUS  Status;
>      >>>
>      >>> -  BOOLEAN     FadtFound;
>      >>>
>      >>> -  BOOLEAN     MadtFound;
>      >>>
>      >>> -  BOOLEAN     GtdtFound;
>      >>>
>      >>> -  BOOLEAN     DsdtFound;
>      >>>
>      >>> -  BOOLEAN     Dbg2Found;
>      >>>
>      >>> -  BOOLEAN     SpcrFound;
>      >>>
>      >>> +  EFI_STATUS                   Status;
>      >>>
>      >>> +  UINTN                        Handle;
>      >>>
>      >>> +  UINTN                        Index;
>      >>>
>      >>> +  UINTN                        InstalledTableIndex;
>      >>>
>      >>> +  EFI_ACPI_DESCRIPTION_HEADER  *DescHeader;
>      >>>
>      >>> +  EFI_ACPI_TABLE_VERSION       Version;
>      >>>
>      >>> +  EFI_ACPI_SDT_PROTOCOL        *AcpiSdt;
>      >>>
>      >>>
>      >>> -  Status    = EFI_SUCCESS;
>      >>>
>      >>> -  FadtFound = FALSE;
>      >>>
>      >>> -  MadtFound = FALSE;
>      >>>
>      >>> -  GtdtFound = FALSE;
>      >>>
>      >>> -  DsdtFound = FALSE;
>      >>>
>      >>> -  Dbg2Found = FALSE;
>      >>>
>      >>> -  SpcrFound = FALSE;
>      >>>
>      >>>     ASSERT (AcpiTableInfo != NULL);
>      >>>
>      >>>
>      >>> +  Status = EFI_SUCCESS;
>      >>>
>      >>> +
>      >>>
>      >>> +  // Check against the statically initialized ACPI tables to see if
>      >>> they are in ACPI info list
>      >>>
>      >>>     while (AcpiTableCount-- != 0) {
>      >>>
>      >>> -    switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
>      >>>
>      >>> -      case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
>      >>>
>      >>> -        FadtFound = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
>      >>>
>      >>> -        MadtFound = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
>      >>>
>      >>> -        GtdtFound = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      case
>      >>> EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
>      >>>
>      >>> -        DsdtFound = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
>      >>>
>      >>> -        Dbg2Found = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      case
>      >>> EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
>      >>>
>      >>> -        SpcrFound = TRUE;
>      >>>
>      >>> -        break;
>      >>>
>      >>> -      default:
>      >>>
>      >>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>      >>>
>      >>> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature ==
>      >>> mAcpiVerifyTables[Index].AcpiTableSignature) {
>      >>>
>      >>> +        mAcpiVerifyTables[Index].Presence |=
>      >>> ACPI_TABLE_PRESENT_INFO_LIST;
>      >>>
>      >>> +        // Found this table, skip the rest.
>      >>>
>      >>>           break;
>      >>>
>      >>> +      }
>      >>>
>      >>>       }
>      >>>
>      >>>     }
>      >>>
>      >>>
>      >>> -  // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
>      >>>
>      >>> -  if (!FadtFound) {
>      >>>
>      >>> -    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
>      >>>
>      >>> -    Status = EFI_NOT_FOUND;
>      >>>
>      >>> -  }
>      >>>
>      >>> +  // They also might be published already, so we can search from there
>      >>>
>      >>> +  if (FeaturePcdGet (PcdInstallAcpiSdtProtocol)) {
>      >>>
>      >>> +    AcpiSdt = NULL;
>      >>>
>      >>> +    Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL,
>      >>> (VOID **)&AcpiSdt);
>      >>>
>      >>>
>      >>> -  if (!MadtFound) {
>      >>>
>      >>> -    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
>      >>>
>      >>> -    Status = EFI_NOT_FOUND;
>      >>>
>      >>> -  }
>      >>>
>      >>> +    if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
>      >>>
>      >>> +      DEBUG ((DEBUG_ERROR, "ERROR: Failed to locate ACPI SDT
>      >>> protocol (0x%p) - %r\n", AcpiSdt, Status));
>      >>>
>      >>> +      return Status;
>      >>>
>      >>> +    }
>      >>>
>      >>>
>      >>> -  if (!GtdtFound) {
>      >>>
>      >>> -    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
>      >>>
>      >>> -    Status = EFI_NOT_FOUND;
>      >>>
>      >>> -  }
>      >>>
>      >>> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>      >>>
>      >>> +      Handle              = 0;
>      >>>
>      >>> +      InstalledTableIndex = 0;
>      >>>
>      >>> +      do {
>      >>>
>      >>> +        Status = AcpiSdt->GetAcpiTable (InstalledTableIndex,
>      >>> (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
>      >>>
>      >>> +        if (EFI_ERROR (Status)) {
>      >
>      > [SAMI] When running Kvmtool guest firmware I break from here with
>      > EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE
>      > in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any
>      > preinstalled tables (i.e. all tables are generated using
>      > DynamicTablesFramework).
>      >
>      > This means platforms that use only Dynamic Tables Framework would all
>      > need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to
>      > rework this logic so that the existing platform code does not need
>      > updating, please?
>      >
>      > [/SAMI]
>
>      [KQ]
>
>      There was a mistake that the Status should be set to EFI_SUCCESS before
>      entering the presence
>      checking step. Otherwise it will remain the same value from GetAcpiTable
>      if all tables are present.
>
>      To your original observation, it is correct that this GetAcpiTable call
>      will fail with EFI_NOT_FOUND,
>      but it should only break from the internal `do-while` loop, and continue
>      with the rest of table
>      look-ups. Then during table inspection step, either installed table or
>      from info_list will count as a
>      passed check. The return Status should be reset before inspecting the
>      full look-up results. Thanks
>      for catching this! Will add the `Status = EFI_SUCCESS` statement in the
>      next round.
>
>      [/KQ]
>
> [SAMI] Ack. I look forward to an updated patch with this fixed.
>
>      >
>      >>>
>      >>> +          break;
>      >>>
>      >>> +        }
>      >>>
>      >>>
>      >>> -  if (!DsdtFound) {
>      >>>
>      >>> -    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
>      >>>
>      >>> -    Status = EFI_NOT_FOUND;
>      >>>
>      >>> -  }
>      >>>
>      >>> +        InstalledTableIndex++;
>      >>>
>      >>> +      } while (DescHeader->Signature !=
>      >>> mAcpiVerifyTables[Index].AcpiTableSignature);
>      >>>
>      >>>
>      >>> -  if (!Dbg2Found) {
>      >>>
>      >>> -    DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
>      >>>
>      >>> +      if (!EFI_ERROR (Status)) {
>      >>>
>      >>> +        mAcpiVerifyTables[Index].Presence |=
>      >>> ACPI_TABLE_PRESENT_INSTALLED;
>      >>>
>      >>> +      }
>      >>>
>      >>> +    }
>      >>>
>      >>>     }
>      >>>
>      >>>
>      >>> -  if (!SpcrFound) {
>      >>>
>      >>> -    DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
>      >>>
>      >>> +  for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
>      >>>
>      >>> +    if (mAcpiVerifyTables[Index].Presence == 0) {
>      >>>
>      >>> +      if (mAcpiVerifyTables[Index].IsMandatory) {
>      >>>
>      >>> +        DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n",
>      >>> mAcpiVerifyTables[Index].AcpiTableName));
>      >>>
>      >>> +        Status = EFI_NOT_FOUND;
>      >>>
>      >>> +      } else {
>      >>>
>      >>> +        DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n",
>      >>> mAcpiVerifyTables[Index].AcpiTableName));
>      >>>
>      >>> +      }
>      >>>
>      >>> +    } else if (mAcpiVerifyTables[Index].Presence ==
>      >>>
>      >>> +               (ACPI_TABLE_PRESENT_INFO_LIST |
>      >>> ACPI_TABLE_PRESENT_INSTALLED))
>      >>>
>      >>> +    {
>      >>>
>      >>> +      DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already
>      >>> published.\n", mAcpiVerifyTables[Index].AcpiTableName));
>      >>>
>      >>> +      Status = EFI_ALREADY_STARTED;
>      >>>
>      >>> +    }
>      >>>
>      >>>     }
>      >>>
>      >>>
>      >>>     return Status;
>      >>>
>      >>> @@ -489,8 +539,9 @@ VerifyMandatoryTablesArePresent (
>      >>>     @param [in]  CfgMgrProtocol       Pointer to the Configuration
>      >>> Manager
>      >>>
>      >>>                                       Protocol Interface.
>      >>>
>      >>>
>      >>> -  @retval EFI_SUCCESS   Success.
>      >>>
>      >>> -  @retval EFI_NOT_FOUND If a mandatory table or a generator is not
>      >>> found.
>      >>>
>      >>> +  @retval EFI_SUCCESS           Success.
>      >>>
>      >>> +  @retval EFI_NOT_FOUND         If a mandatory table or a generator
>      >>> is not found.
>      >>>
>      >>> +  @retval EFI_ALREADY_STARTED   If mandatory table found in
>      >>> AcpiTableInfo is already installed.
>      >>>
>      >>>   **/
>      >>>
>      >>>   STATIC
>      >>>
>      >>>   EFI_STATUS
>      >>>
>      >>> @@ -562,7 +613,7 @@ ProcessAcpiTables (
>      >>>     if (EFI_ERROR (Status)) {
>      >>>
>      >>>       DEBUG ((
>      >>>
>      >>>         DEBUG_ERROR,
>      >>>
>      >>> -      "ERROR: Failed to find mandatory ACPI Table(s)."
>      >>>
>      >>> +      "ERROR: Failed to verify mandatory ACPI Table(s) presence."
>      >>>
>      >>>         " Status = %r\n",
>      >>>
>      >>>         Status
>      >>>
>      >>>         ));
>      >>>
>      >>> @@ -570,29 +621,32 @@ ProcessAcpiTables (
>      >>>     }
>      >>>
>      >>>
>      >>>     // Add the FADT Table first.
>      >>>
>      >>> -  for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>      >>>
>      >>> -    if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>      >>>
>      >>> -        AcpiTableInfo[Idx].TableGeneratorId)
>      >>>
>      >>> -    {
>      >>>
>      >>> -      Status = BuildAndInstallAcpiTable (
>      >>>
>      >>> -                 TableFactoryProtocol,
>      >>>
>      >>> -                 CfgMgrProtocol,
>      >>>
>      >>> -                 AcpiTableProtocol,
>      >>>
>      >>> -                 &AcpiTableInfo[Idx]
>      >>>
>      >>> -                 );
>      >>>
>      >>> -      if (EFI_ERROR (Status)) {
>      >>>
>      >>> -        DEBUG ((
>      >>>
>      >>> -          DEBUG_ERROR,
>      >>>
>      >>> -          "ERROR: Failed to find build and install ACPI FADT Table." \
>      >>>
>      >>> -          " Status = %r\n",
>      >>>
>      >>> -          Status
>      >>>
>      >>> -          ));
>      >>>
>      >>> -        return Status;
>      >>>
>      >>> -      }
>      >>>
>      >>> +  if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence &
>      >>> ACPI_TABLE_PRESENT_INSTALLED) == 0) {
>      >>
>      >> [SAMI] If I understand correctly, the mAcpiVerifyTables[x].Presence
>      >> filed cannot be ACPI_TABLE_PRESENT_INFO_LIST and
>      >> ACPI_TABLE_PRESENT_INSTALLED at the same time. Otherwise we would
>      >> have returned EFI_ALREADY_STARTED from
>      >> VerifyMandatoryTablesArePresent().
>      >>
>      >> Since FADT is mandatory, the only valid conditions are:
>      >>
>      >> 1. ACPI_TABLE_PRESENT_INFO_LIST and !ACPI_TABLE_PRESENT_INSTALLED
>      >>
>      >> 2. !ACPI_TABLE_PRESENT_INFO_LIST & ACPI_TABLE_PRESENT_INSTALLED
>      >>
>      >> Therefore, I think the above check is not required. What do you think?
>      >>
>      >> [/SAMI]
>
>      [KQ]
>
>      You are correct that there will be only above 2 conditions for mandatory
>      tables.
>      But the check is to make sure the FADT will only be installed on
>      condition #1 above,
>      which means it will only be installed if it has not already been
>      installed. Please let
>      me know if I missed anything here.
>
>      [/KQ]
> [SAMI] Ack. Please keep this check. If the table is already installed this check prevents searching in the AcpiTableInfo list and therefore optimises.
>      >>
>      >>>
>      >>> +    // FADT is not yet installed
>      >>>
>      >>> +    for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>      >>>
>      >>> +      if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
>      >>>
>      >>> +          AcpiTableInfo[Idx].TableGeneratorId)
>      >>>
>      >>> +      {
>      >>>
>      >>> +        Status = BuildAndInstallAcpiTable (
>      >>>
>      >>> +                   TableFactoryProtocol,
>      >>>
>      >>> +                   CfgMgrProtocol,
>      >>>
>      >>> +                   AcpiTableProtocol,
>      >>>
>      >>> +                   &AcpiTableInfo[Idx]
>      >>>
>      >>> +                   );
>      >>>
>      >>> +        if (EFI_ERROR (Status)) {
>      >>>
>      >>> +          DEBUG ((
>      >>>
>      >>> +            DEBUG_ERROR,
>      >>>
>      >>> +            "ERROR: Failed to find build and install ACPI FADT
>      >>> Table." \
>      >>>
>      >>> +            " Status = %r\n",
>      >>>
>      >>> +            Status
>      >>>
>      >>> +            ));
>      >>>
>      >>> +          return Status;
>      >>>
>      >>> +        }
>      >>>
>      >>>
>      >>> -      break;
>      >>>
>      >>> -    }
>      >>>
>      >>> -  } // for
>      >>>
>      >>> +        break;
>      >>>
>      >>> +      }
>      >>>
>      >>> +    } // for
>      >>>
>      >>> +  }
>      >>>
>      >>>
>      >>>     // Add remaining ACPI Tables
>      >>>
>      >>>     for (Idx = 0; Idx < AcpiTableCount; Idx++) {
>      >>>
>      >>> diff --git
>      >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>      >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>      >>>
>      >>> index 028c3d413cf8..ad8b3d037c16 100644
>      >>> ---
>      >>> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>      >>> +++
>      >>> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
>      >>> @@ -34,8 +34,12 @@ [LibraryClasses]
>      >>>     UefiBootServicesTableLib
>      >>>
>      >>>     UefiDriverEntryPoint
>      >>>
>      >>>
>      >>> +[FeaturePcd]
>      >>>
>      >>> +  gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol ## CONSUMES
>      >>>
>      >>> +
>      >>>
>      >>>   [Protocols]
>      >>>
>      >>>     gEfiAcpiTableProtocolGuid                     # PROTOCOL
>      >>> ALWAYS_CONSUMED
>      >>>
>      >>> +  gEfiAcpiSdtProtocolGuid                       # PROTOCOL
>      >>> ALWAYS_CONSUMED
>      >>>
>      >>>
>      >>>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL
>      >>> ALWAYS_CONSUMED
>      >>>
>      >>>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL
>      >>> ALWAYS_CONSUMED
>      >>>
>

[PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998

Certain OSes will complain if the ECAM config space is not reserved in
the ACPI namespace.

This change adds a function to reserve PNP motherboard resources for a
given PCI node.

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Only create RES0 after config space checking [Pierre]
    
    v3:
    - Updated function names and descriptions [Pierre]
    - Moved translation calculation to CONFIG case [Pierre]

 DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
 1 file changed, 171 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index ceffe2838c03..658a089c8f1f 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -616,6 +616,169 @@ GeneratePciCrs (
   return Status;
 }
 
+/** Generate a RES0 device node to reserve PNP motherboard resources
+  for a given PCI node.
+
+  @param [in]   PciNode       Parent PCI node handle of the generated
+                              resource object.
+  @param [out]  CrsNode       CRS node of the AML tree to populate.
+
+  @retval EFI_SUCCESS             The function completed successfully.
+  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
+  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+GenerateMotherboardDevice (
+  IN  AML_OBJECT_NODE_HANDLE  PciNode,
+  OUT AML_OBJECT_NODE_HANDLE  *CrsNode
+  )
+{
+  EFI_STATUS              Status;
+  UINT32                  EisaId;
+  AML_OBJECT_NODE_HANDLE  ResNode;
+
+  if (CrsNode == NULL) {
+    ASSERT (0);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  // ASL: Device (RES0) {}
+  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // ASL: Name (_HID, EISAID ("PNP0C02"))
+  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  // ASL: Name (_CRS, ResourceTemplate () {})
+  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  return Status;
+}
+
+/** Reserves ECAM space for PCI config space
+
+  @param [in]       Generator       The SSDT Pci generator.
+  @param [in]       CfgMgrProtocol  Pointer to the Configuration Manager
+                                    Protocol interface.
+  @param [in]       PciInfo         Pci device information.
+  @param [in, out]  PciNode         RootNode of the AML tree to populate.
+
+  @retval EFI_SUCCESS             The function completed successfully.
+  @retval EFI_INVALID_PARAMETER   Invalid parameter.
+  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+ReserveEcamSpace (
+  IN            ACPI_PCI_GENERATOR                            *Generator,
+  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
+  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO                  *PciInfo,
+  IN  OUT       AML_OBJECT_NODE_HANDLE                        PciNode
+  )
+{
+  EFI_STATUS                   Status;
+  AML_OBJECT_NODE_HANDLE       CrsNode;
+  BOOLEAN                      Translation;
+  UINT32                       Index;
+  CM_ARM_OBJ_REF               *RefInfo;
+  UINT32                       RefCount;
+  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
+  BOOLEAN                      IsPosDecode;
+
+  // Get the array of CM_ARM_OBJ_REF referencing the
+  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
+  Status = GetEArmObjCmRef (
+             CfgMgrProtocol,
+             PciInfo->AddressMapToken,
+             &RefInfo,
+             &RefCount
+             );
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
+  for (Index = 0; Index < RefCount; Index++) {
+    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
+    Status = GetEArmObjPciAddressMapInfo (
+               CfgMgrProtocol,
+               RefInfo[Index].ReferenceToken,
+               &AddrMapInfo,
+               NULL
+               );
+    if (EFI_ERROR (Status)) {
+      ASSERT (0);
+      return Status;
+    }
+
+    switch (AddrMapInfo->SpaceCode) {
+      case PCI_SS_CONFIG:
+        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
+        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
+          IsPosDecode = TRUE;
+        } else {
+          IsPosDecode = FALSE;
+        }
+
+        Status = GenerateMotherboardDevice (PciNode, &CrsNode);
+        if (EFI_ERROR (Status)) {
+          ASSERT (0);
+          break;
+        }
+
+        Status = AmlCodeGenRdQWordMemory (
+                   FALSE,
+                   IsPosDecode,
+                   TRUE,
+                   TRUE,
+                   FALSE, // non-cacheable
+                   TRUE,
+                   0,
+                   AddrMapInfo->PciAddress,
+                   AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
+                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
+                   AddrMapInfo->AddressSize,
+                   0,
+                   NULL,
+                   0,
+                   TRUE,
+                   CrsNode,
+                   NULL
+                   );
+        break;
+      default:
+        break;
+    } // switch
+
+    if (EFI_ERROR (Status)) {
+      ASSERT (0);
+      return Status;
+    }
+  }
+
+  return Status;
+}
+
 /** Generate a Pci device.
 
   @param [in]       Generator       The SSDT Pci generator.
@@ -702,9 +865,17 @@ GeneratePciDevice (
     return Status;
   }
 
+  // Add the PNP Motherboard Resources Device to reserve ECAM space
+  Status = ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, PciNode);
+  if (EFI_ERROR (Status)) {
+    ASSERT (0);
+    return Status;
+  }
+
   // Add the template _OSC method.
   Status = AddOscMethod (PciInfo, PciNode);
   ASSERT_EFI_ERROR (Status);
+
   return Status;
 }
 
-- 
2.37.1.windows.1

Re: [edk2-devel] [PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[Sami Mujawar]

Hi Kun,

Thank you for this patch.

These changes look good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, Kun Qin via groups.io wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>
> Certain OSes will complain if the ECAM config space is not reserved in
> the ACPI namespace.
>
> This change adds a function to reserve PNP motherboard resources for a
> given PCI node.
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>
> Notes:
>      v2:
>      - Only create RES0 after config space checking [Pierre]
>      
>      v3:
>      - Updated function names and descriptions [Pierre]
>      - Moved translation calculation to CONFIG case [Pierre]
>
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
>   1 file changed, 171 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index ceffe2838c03..658a089c8f1f 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -616,6 +616,169 @@ GeneratePciCrs (
>     return Status;
>
>   }
>
>   
>
> +/** Generate a RES0 device node to reserve PNP motherboard resources
>
> +  for a given PCI node.
>
> +
>
> +  @param [in]   PciNode       Parent PCI node handle of the generated
>
> +                              resource object.
>
> +  @param [out]  CrsNode       CRS node of the AML tree to populate.
>
> +
>
> +  @retval EFI_SUCCESS             The function completed successfully.
>
> +  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
>
> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +GenerateMotherboardDevice (
>
> +  IN  AML_OBJECT_NODE_HANDLE  PciNode,
>
> +  OUT AML_OBJECT_NODE_HANDLE  *CrsNode
>
> +  )
>
> +{
>
> +  EFI_STATUS              Status;
>
> +  UINT32                  EisaId;
>
> +  AML_OBJECT_NODE_HANDLE  ResNode;
>
> +
>
> +  if (CrsNode == NULL) {
>
> +    ASSERT (0);
>
> +    return EFI_INVALID_PARAMETER;
>
> +  }
>
> +
>
> +  // ASL: Device (RES0) {}
>
> +  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
> +  // ASL: Name (_HID, EISAID ("PNP0C02"))
>
> +  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
> +  // ASL: Name (_CRS, ResourceTemplate () {})
>
> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
> +  return Status;
>
> +}
>
> +
>
> +/** Reserves ECAM space for PCI config space
>
> +
>
> +  @param [in]       Generator       The SSDT Pci generator.
>
> +  @param [in]       CfgMgrProtocol  Pointer to the Configuration Manager
>
> +                                    Protocol interface.
>
> +  @param [in]       PciInfo         Pci device information.
>
> +  @param [in, out]  PciNode         RootNode of the AML tree to populate.
>
> +
>
> +  @retval EFI_SUCCESS             The function completed successfully.
>
> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
>
> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>
> +**/
>
> +STATIC
>
> +EFI_STATUS
>
> +EFIAPI
>
> +ReserveEcamSpace (
>
> +  IN            ACPI_PCI_GENERATOR                            *Generator,
>
> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST  CfgMgrProtocol,
>
> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO                  *PciInfo,
>
> +  IN  OUT       AML_OBJECT_NODE_HANDLE                        PciNode
>
> +  )
>
> +{
>
> +  EFI_STATUS                   Status;
>
> +  AML_OBJECT_NODE_HANDLE       CrsNode;
>
> +  BOOLEAN                      Translation;
>
> +  UINT32                       Index;
>
> +  CM_ARM_OBJ_REF               *RefInfo;
>
> +  UINT32                       RefCount;
>
> +  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
>
> +  BOOLEAN                      IsPosDecode;
>
> +
>
> +  // Get the array of CM_ARM_OBJ_REF referencing the
>
> +  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>
> +  Status = GetEArmObjCmRef (
>
> +             CfgMgrProtocol,
>
> +             PciInfo->AddressMapToken,
>
> +             &RefInfo,
>
> +             &RefCount
>
> +             );
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
> +  for (Index = 0; Index < RefCount; Index++) {
>
> +    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>
> +    Status = GetEArmObjPciAddressMapInfo (
>
> +               CfgMgrProtocol,
>
> +               RefInfo[Index].ReferenceToken,
>
> +               &AddrMapInfo,
>
> +               NULL
>
> +               );
>
> +    if (EFI_ERROR (Status)) {
>
> +      ASSERT (0);
>
> +      return Status;
>
> +    }
>
> +
>
> +    switch (AddrMapInfo->SpaceCode) {
>
> +      case PCI_SS_CONFIG:
>
> +        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
>
> +        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>
> +          IsPosDecode = TRUE;
>
> +        } else {
>
> +          IsPosDecode = FALSE;
>
> +        }
>
> +
>
> +        Status = GenerateMotherboardDevice (PciNode, &CrsNode);
>
> +        if (EFI_ERROR (Status)) {
>
> +          ASSERT (0);
>
> +          break;
>
> +        }
>
> +
>
> +        Status = AmlCodeGenRdQWordMemory (
>
> +                   FALSE,
>
> +                   IsPosDecode,
>
> +                   TRUE,
>
> +                   TRUE,
>
> +                   FALSE, // non-cacheable
>
> +                   TRUE,
>
> +                   0,
>
> +                   AddrMapInfo->PciAddress,
>
> +                   AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
>
> +                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
>
> +                   AddrMapInfo->AddressSize,
>
> +                   0,
>
> +                   NULL,
>
> +                   0,
>
> +                   TRUE,
>
> +                   CrsNode,
>
> +                   NULL
>
> +                   );
>
> +        break;
>
> +      default:
>
> +        break;
>
> +    } // switch
>
> +
>
> +    if (EFI_ERROR (Status)) {
>
> +      ASSERT (0);
>
> +      return Status;
>
> +    }
>
> +  }
>
> +
>
> +  return Status;
>
> +}
>
> +
>
>   /** Generate a Pci device.
>
>   
>
>     @param [in]       Generator       The SSDT Pci generator.
>
> @@ -702,9 +865,17 @@ GeneratePciDevice (
>       return Status;
>
>     }
>
>   
>
> +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>
> +  Status = ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, PciNode);
>
> +  if (EFI_ERROR (Status)) {
>
> +    ASSERT (0);
>
> +    return Status;
>
> +  }
>
> +
>
>     // Add the template _OSC method.
>
>     Status = AddOscMethod (PciInfo, PciNode);
>
>     ASSERT_EFI_ERROR (Status);
>
> +
>
>     return Status;
>
>   }
>
>   
>

Re: [edk2-devel] [PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[Sami Mujawar]

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

Hi Kun,

I have just tried testing your patch with Kvmtool guest firmware and 
think this patch may need some modifications.

Also, the patch 4/6 may need some adjustment, which I will reply back on 
that patch separately.

Regards,

Sami Mujawar

On 08/08/2022 02:22 pm, Sami Mujawar wrote:
> Hi Kun,
>
> Thank you for this patch.
>
> These changes look good to me.
>
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>
> Regards,
>
> Sami Mujawar
>
> On 31/07/2022 06:37 am, Kun Qin via groups.io wrote:
>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>>
>> Certain OSes will complain if the ECAM config space is not reserved in
>> the ACPI namespace.
>>
>> This change adds a function to reserve PNP motherboard resources for a
>> given PCI node.
>>
>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>
>> Notes:
>>      v2:
>>      - Only create RES0 after config space checking [Pierre]
>>           v3:
>>      - Updated function names and descriptions [Pierre]
>>      - Moved translation calculation to CONFIG case [Pierre]
>>
>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> | 171 ++++++++++++++++++++
>>   1 file changed, 171 insertions(+)
>>
>> diff --git 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>
>> index ceffe2838c03..658a089c8f1f 100644
>> --- 
>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> +++ 
>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>> @@ -616,6 +616,169 @@ GeneratePciCrs (
>>     return Status;
>>
>>   }
>>
>>
>> +/** Generate a RES0 device node to reserve PNP motherboard resources
>>
>> +  for a given PCI node.
>>
>> +
>>
>> +  @param [in]   PciNode       Parent PCI node handle of the generated
>>
>> +                              resource object.
>>
>> +  @param [out]  CrsNode       CRS node of the AML tree to populate.
>>
>> +
>>
>> +  @retval EFI_SUCCESS             The function completed successfully.
>>
>> +  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
>>
>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>
>> +**/
>>
>> +STATIC
>>
>> +EFI_STATUS
>>
>> +EFIAPI
>>
>> +GenerateMotherboardDevice (
>>
>> +  IN  AML_OBJECT_NODE_HANDLE  PciNode,
>>
>> +  OUT AML_OBJECT_NODE_HANDLE  *CrsNode
>>
>> +  )
>>
>> +{
>>
>> +  EFI_STATUS              Status;
>>
>> +  UINT32                  EisaId;
>>
>> +  AML_OBJECT_NODE_HANDLE  ResNode;
>>
>> +
>>
>> +  if (CrsNode == NULL) {
>>
>> +    ASSERT (0);
>>
>> +    return EFI_INVALID_PARAMETER;
>>
>> +  }
>>
>> +
>>
>> +  // ASL: Device (RES0) {}
>>
>> +  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>> +  // ASL: Name (_HID, EISAID ("PNP0C02"))
>>
>> +  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP 
>> Motherboard Resources */
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>> +  // ASL: Name (_CRS, ResourceTemplate () {})
>>
>> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>> +  return Status;
>>
>> +}
>>
>> +
>>
>> +/** Reserves ECAM space for PCI config space
>>
>> +
>>
>> +  @param [in]       Generator       The SSDT Pci generator.
>>
>> +  @param [in]       CfgMgrProtocol  Pointer to the Configuration 
>> Manager
>>
>> +                                    Protocol interface.
>>
>> +  @param [in]       PciInfo         Pci device information.
>>
>> +  @param [in, out]  PciNode         RootNode of the AML tree to 
>> populate.
>>
>> +
>>
>> +  @retval EFI_SUCCESS             The function completed successfully.
>>
>> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
>>
>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>
>> +**/
>>
>> +STATIC
>>
>> +EFI_STATUS
>>
>> +EFIAPI
>>
>> +ReserveEcamSpace (
>>
>> +  IN            ACPI_PCI_GENERATOR *Generator,
>>
>> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST 
>> CfgMgrProtocol,
>>
>> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
>>
>> +  IN  OUT       AML_OBJECT_NODE_HANDLE PciNode
>>
>> +  )
>>
>> +{
>>
>> +  EFI_STATUS                   Status;
>>
>> +  AML_OBJECT_NODE_HANDLE       CrsNode;
>>
>> +  BOOLEAN                      Translation;
>>
>> +  UINT32                       Index;
>>
>> +  CM_ARM_OBJ_REF               *RefInfo;
>>
>> +  UINT32                       RefCount;
>>
>> +  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
>>
>> +  BOOLEAN                      IsPosDecode;
>>
>> +
>>
>> +  // Get the array of CM_ARM_OBJ_REF referencing the
>>
>> +  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>>
>> +  Status = GetEArmObjCmRef (
>>
>> +             CfgMgrProtocol,
>>
>> +             PciInfo->AddressMapToken,
>>
>> +             &RefInfo,
>>
>> +             &RefCount
>>
>> +             );
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>> +  for (Index = 0; Index < RefCount; Index++) {
>>
>> +    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>>
>> +    Status = GetEArmObjPciAddressMapInfo (
>>
>> +               CfgMgrProtocol,
>>
>> +               RefInfo[Index].ReferenceToken,
>>
>> +               &AddrMapInfo,
>>
>> +               NULL
>>
>> +               );
>>
>> +    if (EFI_ERROR (Status)) {
>>
>> +      ASSERT (0);
>>
>> +      return Status;
>>
>> +    }
>>
[SAMI] Sorry for missing this earlier in the review. However, the ECAM 
memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I think 
that needs to be used here.

The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length 
of the configuration space and would probably need to be updated.

Which platform are you testing these changes on? I would like to 
understand more about your use case. Is it possible to share some more 
details, please?

[/SAMI]

>> +
>>
>> +    switch (AddrMapInfo->SpaceCode) {
>>
>> +      case PCI_SS_CONFIG:
>>
>> +        Translation = (AddrMapInfo->CpuAddress != 
>> AddrMapInfo->PciAddress);
>>
>> +        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>>
>> +          IsPosDecode = TRUE;
>>
>> +        } else {
>>
>> +          IsPosDecode = FALSE;
>>
>> +        }
>>
>> +
>>
>> +        Status = GenerateMotherboardDevice (PciNode, &CrsNode);
>>
>> +        if (EFI_ERROR (Status)) {
>>
>> +          ASSERT (0);
>>
>> +          break;
>>
>> +        }
>>
>> +
>>
>> +        Status = AmlCodeGenRdQWordMemory (
>>
>> +                   FALSE,
>>
>> +                   IsPosDecode,
>>
>> +                   TRUE,
>>
>> +                   TRUE,
>>
>> +                   FALSE, // non-cacheable
>>
>> +                   TRUE,
>>
>> +                   0,
>>
>> +                   AddrMapInfo->PciAddress,
>>
>> +                   AddrMapInfo->PciAddress + 
>> AddrMapInfo->AddressSize - 1,
>>
>> +                   Translation ? AddrMapInfo->CpuAddress - 
>> AddrMapInfo->PciAddress : 0,
>>
>> +                   AddrMapInfo->AddressSize,
>>
>> +                   0,
>>
>> +                   NULL,
>>
>> +                   0,
>>
>> +                   TRUE,
>>
>> +                   CrsNode,
>>
>> +                   NULL
>>
>> +                   );
>>
>> +        break;
>>
>> +      default:
>>
>> +        break;
>>
>> +    } // switch
>>
>> +
>>
>> +    if (EFI_ERROR (Status)) {
>>
>> +      ASSERT (0);
>>
>> +      return Status;
>>
>> +    }
>>
>> +  }
>>
>> +
>>
>> +  return Status;
>>
>> +}
>>
>> +
>>
>>   /** Generate a Pci device.
>>
>>
>>     @param [in]       Generator       The SSDT Pci generator.
>>
>> @@ -702,9 +865,17 @@ GeneratePciDevice (
>>       return Status;
>>
>>     }
>>
>>
>> +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>>
>> +  Status = ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, 
>> PciNode);
>>
>> +  if (EFI_ERROR (Status)) {
>>
>> +    ASSERT (0);
>>
>> +    return Status;
>>
>> +  }
>>
>> +
>>
>>     // Add the template _OSC method.
>>
>>     Status = AddOscMethod (PciInfo, PciNode);
>>
>>     ASSERT_EFI_ERROR (Status);
>>
>> +
>>
>>     return Status;
>>
>>   }
>>
>>

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

Re: [edk2-devel] [PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[PierreGondois]

On 8/8/22 17:39, Sami Mujawar wrote:
> Hi Kun,
> 
> I have just tried testing your patch with Kvmtool guest firmware and think this patch may need some modifications.
> 
> Also, the patch 4/6 may need some adjustment, which I will reply back on that patch separately.
> 
> Regards,
> 
> Sami Mujawar
> 
> On 08/08/2022 02:22 pm, Sami Mujawar wrote:
>> Hi Kun,
>>
>> Thank you for this patch.
>>
>> These changes look good to me.
>>
>> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 31/07/2022 06:37 am, Kun Qin via groups.io wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>>>
>>> Certain OSes will complain if the ECAM config space is not reserved in
>>> the ACPI namespace.
>>>
>>> This change adds a function to reserve PNP motherboard resources for a
>>> given PCI node.
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>
>>> Notes:
>>>      v2:
>>>      - Only create RES0 after config space checking [Pierre]
>>>           v3:
>>>      - Updated function names and descriptions [Pierre]
>>>      - Moved translation calculation to CONFIG case [Pierre]
>>>
>>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 171 ++++++++++++++++++++
>>>   1 file changed, 171 insertions(+)
>>>
>>> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>> index ceffe2838c03..658a089c8f1f 100644
>>> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>> @@ -616,6 +616,169 @@ GeneratePciCrs (
>>>     return Status;
>>>
>>>   }
>>>
>>>
>>> +/** Generate a RES0 device node to reserve PNP motherboard resources
>>>
>>> +  for a given PCI node.
>>>
>>> +
>>>
>>> +  @param [in]   PciNode       Parent PCI node handle of the generated
>>>
>>> +                              resource object.
>>>
>>> +  @param [out]  CrsNode       CRS node of the AML tree to populate.
>>>
>>> +
>>>
>>> +  @retval EFI_SUCCESS             The function completed successfully.
>>>
>>> +  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
>>>
>>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +EFIAPI
>>>
>>> +GenerateMotherboardDevice (
>>>
>>> +  IN  AML_OBJECT_NODE_HANDLE  PciNode,
>>>
>>> +  OUT AML_OBJECT_NODE_HANDLE  *CrsNode
>>>
>>> +  )
>>>
>>> +{
>>>
>>> +  EFI_STATUS              Status;
>>>
>>> +  UINT32                  EisaId;
>>>
>>> +  AML_OBJECT_NODE_HANDLE  ResNode;
>>>
>>> +
>>>
>>> +  if (CrsNode == NULL) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return EFI_INVALID_PARAMETER;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  // ASL: Device (RES0) {}
>>>
>>> +  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  // ASL: Name (_HID, EISAID ("PNP0C02"))
>>>
>>> +  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP Motherboard Resources */
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  // ASL: Name (_CRS, ResourceTemplate () {})
>>>
>>> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  return Status;
>>>
>>> +}
>>>
>>> +
>>>
>>> +/** Reserves ECAM space for PCI config space
>>>
>>> +
>>>
>>> +  @param [in]       Generator       The SSDT Pci generator.
>>>
>>> +  @param [in]       CfgMgrProtocol  Pointer to the Configuration Manager
>>>
>>> +                                    Protocol interface.
>>>
>>> +  @param [in]       PciInfo         Pci device information.
>>>
>>> +  @param [in, out]  PciNode         RootNode of the AML tree to populate.
>>>
>>> +
>>>
>>> +  @retval EFI_SUCCESS             The function completed successfully.
>>>
>>> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
>>>
>>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>>
>>> +**/
>>>
>>> +STATIC
>>>
>>> +EFI_STATUS
>>>
>>> +EFIAPI
>>>
>>> +ReserveEcamSpace (
>>>
>>> +  IN            ACPI_PCI_GENERATOR *Generator,
>>>
>>> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST CfgMgrProtocol,
>>>
>>> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
>>>
>>> +  IN  OUT       AML_OBJECT_NODE_HANDLE PciNode
>>>
>>> +  )
>>>
>>> +{
>>>
>>> +  EFI_STATUS                   Status;
>>>
>>> +  AML_OBJECT_NODE_HANDLE       CrsNode;
>>>
>>> +  BOOLEAN                      Translation;
>>>
>>> +  UINT32                       Index;
>>>
>>> +  CM_ARM_OBJ_REF               *RefInfo;
>>>
>>> +  UINT32                       RefCount;
>>>
>>> +  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
>>>
>>> +  BOOLEAN                      IsPosDecode;
>>>
>>> +
>>>
>>> +  // Get the array of CM_ARM_OBJ_REF referencing the
>>>
>>> +  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>>>
>>> +  Status = GetEArmObjCmRef (
>>>
>>> +             CfgMgrProtocol,
>>>
>>> +             PciInfo->AddressMapToken,
>>>
>>> +             &RefInfo,
>>>
>>> +             &RefCount
>>>
>>> +             );
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  for (Index = 0; Index < RefCount; Index++) {
>>>
>>> +    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>>>
>>> +    Status = GetEArmObjPciAddressMapInfo (
>>>
>>> +               CfgMgrProtocol,
>>>
>>> +               RefInfo[Index].ReferenceToken,
>>>
>>> +               &AddrMapInfo,
>>>
>>> +               NULL
>>>
>>> +               );
>>>
>>> +    if (EFI_ERROR (Status)) {
>>>
>>> +      ASSERT (0);
>>>
>>> +      return Status;
>>>
>>> +    }
>>>
> [SAMI] Sorry for missing this earlier in the review. However, the ECAM memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I think that needs to be used here.
> 
> The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the length of the configuration space and would probably need to be updated.
> 
> Which platform are you testing these changes on? I would like to understand more about your use case. Is it possible to share some more details, please?
> 
> [/SAMI]

[Pierre]

Yes indeed, CM_ARM_PCI_CONFIG_SPACE_INFO should contain the configuration
address space, it should not be described in CM_ARM_PCI_ADDRESS_MAP_INFO.
So there should be no need to search through the CM_ARM_PCI_ADDRESS_MAP_INFO
objects. Sorry for missing this earlier.

The length of the address space could be computed as:
length = (end_bus - start_bus + 1) × 32 devices × 8 functions × 4 KB

[/Pierre]

> 
>>> +
>>>
>>> +    switch (AddrMapInfo->SpaceCode) {
>>>
>>> +      case PCI_SS_CONFIG:
>>>
>>> +        Translation = (AddrMapInfo->CpuAddress != AddrMapInfo->PciAddress);
>>>
>>> +        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>>>
>>> +          IsPosDecode = TRUE;
>>>
>>> +        } else {
>>>
>>> +          IsPosDecode = FALSE;
>>>
>>> +        }
>>>
>>> +
>>>
>>> +        Status = GenerateMotherboardDevice (PciNode, &CrsNode);
>>>
>>> +        if (EFI_ERROR (Status)) {
>>>
>>> +          ASSERT (0);
>>>
>>> +          break;
>>>
>>> +        }
>>>
>>> +
>>>
>>> +        Status = AmlCodeGenRdQWordMemory (
>>>
>>> +                   FALSE,
>>>
>>> +                   IsPosDecode,
>>>
>>> +                   TRUE,
>>>
>>> +                   TRUE,
>>>
>>> +                   FALSE, // non-cacheable
>>>
>>> +                   TRUE,
>>>
>>> +                   0,
>>>
>>> +                   AddrMapInfo->PciAddress,
>>>
>>> +                   AddrMapInfo->PciAddress + AddrMapInfo->AddressSize - 1,
>>>
>>> +                   Translation ? AddrMapInfo->CpuAddress - AddrMapInfo->PciAddress : 0,
>>>
>>> +                   AddrMapInfo->AddressSize,
>>>
>>> +                   0,
>>>
>>> +                   NULL,
>>>
>>> +                   0,
>>>
>>> +                   TRUE,
>>>
>>> +                   CrsNode,
>>>
>>> +                   NULL
>>>
>>> +                   );
>>>
>>> +        break;
>>>
>>> +      default:
>>>
>>> +        break;
>>>
>>> +    } // switch
>>>
>>> +
>>>
>>> +    if (EFI_ERROR (Status)) {
>>>
>>> +      ASSERT (0);
>>>
>>> +      return Status;
>>>
>>> +    }
>>>
>>> +  }
>>>
>>> +
>>>
>>> +  return Status;
>>>
>>> +}
>>>
>>> +
>>>
>>>   /** Generate a Pci device.
>>>
>>>
>>>     @param [in]       Generator       The SSDT Pci generator.
>>>
>>> @@ -702,9 +865,17 @@ GeneratePciDevice (
>>>       return Status;
>>>
>>>     }
>>>
>>>
>>> +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>>>
>>> +  Status = ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, PciNode);
>>>
>>> +  if (EFI_ERROR (Status)) {
>>>
>>> +    ASSERT (0);
>>>
>>> +    return Status;
>>>
>>> +  }
>>>
>>> +
>>>
>>>     // Add the template _OSC method.
>>>
>>>     Status = AddOscMethod (PciInfo, PciNode);
>>>
>>>     ASSERT_EFI_ERROR (Status);
>>>
>>> +
>>>
>>>     return Status;
>>>
>>>   }
>>>
>>>

Re: [edk2-devel] [PATCH v3 5/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added function to reserve ECAM space

[Kun Qin]

Hi Pierre/Sami,

Thanks for your feedback. We modified the routine to be based on
`CM_ARM_PCI_CONFIG_SPACE_INFO` and sanity checked the table
outputs from UEFI shell and verified the system bootabilty on VDK
based ARM platform.

The new patch can be found here:
https://edk2.groups.io/g/devel/message/92317

Looking forward to your further feedback.

Thanks,
Kun

On 8/10/2022 1:51 AM, Pierre Gondois wrote:
>
>
> On 8/8/22 17:39, Sami Mujawar wrote:
>> Hi Kun,
>>
>> I have just tried testing your patch with Kvmtool guest firmware and 
>> think this patch may need some modifications.
>>
>> Also, the patch 4/6 may need some adjustment, which I will reply back 
>> on that patch separately.
>>
>> Regards,
>>
>> Sami Mujawar
>>
>> On 08/08/2022 02:22 pm, Sami Mujawar wrote:
>>> Hi Kun,
>>>
>>> Thank you for this patch.
>>>
>>> These changes look good to me.
>>>
>>> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
>>>
>>> Regards,
>>>
>>> Sami Mujawar
>>>
>>> On 31/07/2022 06:37 am, Kun Qin via groups.io wrote:
>>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>>>>
>>>> Certain OSes will complain if the ECAM config space is not reserved in
>>>> the ACPI namespace.
>>>>
>>>> This change adds a function to reserve PNP motherboard resources for a
>>>> given PCI node.
>>>>
>>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>>> ---
>>>>
>>>> Notes:
>>>>      v2:
>>>>      - Only create RES0 after config space checking [Pierre]
>>>>           v3:
>>>>      - Updated function names and descriptions [Pierre]
>>>>      - Moved translation calculation to CONFIG case [Pierre]
>>>>
>>>> DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>> | 171 ++++++++++++++++++++
>>>>   1 file changed, 171 insertions(+)
>>>>
>>>> diff --git 
>>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c 
>>>>
>>>> index ceffe2838c03..658a089c8f1f 100644
>>>> --- 
>>>> a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>>> +++ 
>>>> b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
>>>> @@ -616,6 +616,169 @@ GeneratePciCrs (
>>>>     return Status;
>>>>
>>>>   }
>>>>
>>>>
>>>> +/** Generate a RES0 device node to reserve PNP motherboard resources
>>>>
>>>> +  for a given PCI node.
>>>>
>>>> +
>>>>
>>>> +  @param [in]   PciNode       Parent PCI node handle of the generated
>>>>
>>>> +                              resource object.
>>>>
>>>> +  @param [out]  CrsNode       CRS node of the AML tree to populate.
>>>>
>>>> +
>>>>
>>>> +  @retval EFI_SUCCESS             The function completed 
>>>> successfully.
>>>>
>>>> +  @retval EFI_INVALID_PARAMETER   Invalid input parameter.
>>>>
>>>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>>>
>>>> +**/
>>>>
>>>> +STATIC
>>>>
>>>> +EFI_STATUS
>>>>
>>>> +EFIAPI
>>>>
>>>> +GenerateMotherboardDevice (
>>>>
>>>> +  IN  AML_OBJECT_NODE_HANDLE  PciNode,
>>>>
>>>> +  OUT AML_OBJECT_NODE_HANDLE  *CrsNode
>>>>
>>>> +  )
>>>>
>>>> +{
>>>>
>>>> +  EFI_STATUS              Status;
>>>>
>>>> +  UINT32                  EisaId;
>>>>
>>>> +  AML_OBJECT_NODE_HANDLE  ResNode;
>>>>
>>>> +
>>>>
>>>> +  if (CrsNode == NULL) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return EFI_INVALID_PARAMETER;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  // ASL: Device (RES0) {}
>>>>
>>>> +  Status = AmlCodeGenDevice ("RES0", PciNode, &ResNode);
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  // ASL: Name (_HID, EISAID ("PNP0C02"))
>>>>
>>>> +  Status = AmlGetEisaIdFromString ("PNP0C02", &EisaId); /* PNP 
>>>> Motherboard Resources */
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  Status = AmlCodeGenNameInteger ("_HID", EisaId, ResNode, NULL);
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  // ASL: Name (_CRS, ResourceTemplate () {})
>>>>
>>>> +  Status = AmlCodeGenNameResourceTemplate ("_CRS", ResNode, CrsNode);
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  return Status;
>>>>
>>>> +}
>>>>
>>>> +
>>>>
>>>> +/** Reserves ECAM space for PCI config space
>>>>
>>>> +
>>>>
>>>> +  @param [in]       Generator       The SSDT Pci generator.
>>>>
>>>> +  @param [in]       CfgMgrProtocol  Pointer to the Configuration 
>>>> Manager
>>>>
>>>> +                                    Protocol interface.
>>>>
>>>> +  @param [in]       PciInfo         Pci device information.
>>>>
>>>> +  @param [in, out]  PciNode         RootNode of the AML tree to 
>>>> populate.
>>>>
>>>> +
>>>>
>>>> +  @retval EFI_SUCCESS             The function completed 
>>>> successfully.
>>>>
>>>> +  @retval EFI_INVALID_PARAMETER   Invalid parameter.
>>>>
>>>> +  @retval EFI_OUT_OF_RESOURCES    Could not allocate memory.
>>>>
>>>> +**/
>>>>
>>>> +STATIC
>>>>
>>>> +EFI_STATUS
>>>>
>>>> +EFIAPI
>>>>
>>>> +ReserveEcamSpace (
>>>>
>>>> +  IN            ACPI_PCI_GENERATOR *Generator,
>>>>
>>>> +  IN      CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST 
>>>> CfgMgrProtocol,
>>>>
>>>> +  IN      CONST CM_ARM_PCI_CONFIG_SPACE_INFO *PciInfo,
>>>>
>>>> +  IN  OUT       AML_OBJECT_NODE_HANDLE PciNode
>>>>
>>>> +  )
>>>>
>>>> +{
>>>>
>>>> +  EFI_STATUS                   Status;
>>>>
>>>> +  AML_OBJECT_NODE_HANDLE       CrsNode;
>>>>
>>>> +  BOOLEAN                      Translation;
>>>>
>>>> +  UINT32                       Index;
>>>>
>>>> +  CM_ARM_OBJ_REF               *RefInfo;
>>>>
>>>> +  UINT32                       RefCount;
>>>>
>>>> +  CM_ARM_PCI_ADDRESS_MAP_INFO  *AddrMapInfo;
>>>>
>>>> +  BOOLEAN                      IsPosDecode;
>>>>
>>>> +
>>>>
>>>> +  // Get the array of CM_ARM_OBJ_REF referencing the
>>>>
>>>> +  // CM_ARM_PCI_ADDRESS_MAP_INFO objects.
>>>>
>>>> +  Status = GetEArmObjCmRef (
>>>>
>>>> +             CfgMgrProtocol,
>>>>
>>>> +             PciInfo->AddressMapToken,
>>>>
>>>> +             &RefInfo,
>>>>
>>>> +             &RefCount
>>>>
>>>> +             );
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  for (Index = 0; Index < RefCount; Index++) {
>>>>
>>>> +    // Get CM_ARM_PCI_ADDRESS_MAP_INFO structures one by one.
>>>>
>>>> +    Status = GetEArmObjPciAddressMapInfo (
>>>>
>>>> +               CfgMgrProtocol,
>>>>
>>>> +               RefInfo[Index].ReferenceToken,
>>>>
>>>> +               &AddrMapInfo,
>>>>
>>>> +               NULL
>>>>
>>>> +               );
>>>>
>>>> +    if (EFI_ERROR (Status)) {
>>>>
>>>> +      ASSERT (0);
>>>>
>>>> +      return Status;
>>>>
>>>> +    }
>>>>
>> [SAMI] Sorry for missing this earlier in the review. However, the 
>> ECAM memory space is described byCM_ARM_PCI_CONFIG_SPACE_INFO. So, I 
>> think that needs to be used here.
>>
>> The CM_ARM_PCI_CONFIG_SPACE_INFO structure does not include the 
>> length of the configuration space and would probably need to be updated.
>>
>> Which platform are you testing these changes on? I would like to 
>> understand more about your use case. Is it possible to share some 
>> more details, please?
>>
>> [/SAMI]
>
> [Pierre]
>
> Yes indeed, CM_ARM_PCI_CONFIG_SPACE_INFO should contain the configuration
> address space, it should not be described in CM_ARM_PCI_ADDRESS_MAP_INFO.
> So there should be no need to search through the 
> CM_ARM_PCI_ADDRESS_MAP_INFO
> objects. Sorry for missing this earlier.
>
> The length of the address space could be computed as:
> length = (end_bus - start_bus + 1) × 32 devices × 8 functions × 4 KB
>
> [/Pierre]
>
>>
>>>> +
>>>>
>>>> +    switch (AddrMapInfo->SpaceCode) {
>>>>
>>>> +      case PCI_SS_CONFIG:
>>>>
>>>> +        Translation = (AddrMapInfo->CpuAddress != 
>>>> AddrMapInfo->PciAddress);
>>>>
>>>> +        if (AddrMapInfo->CpuAddress >= AddrMapInfo->PciAddress) {
>>>>
>>>> +          IsPosDecode = TRUE;
>>>>
>>>> +        } else {
>>>>
>>>> +          IsPosDecode = FALSE;
>>>>
>>>> +        }
>>>>
>>>> +
>>>>
>>>> +        Status = GenerateMotherboardDevice (PciNode, &CrsNode);
>>>>
>>>> +        if (EFI_ERROR (Status)) {
>>>>
>>>> +          ASSERT (0);
>>>>
>>>> +          break;
>>>>
>>>> +        }
>>>>
>>>> +
>>>>
>>>> +        Status = AmlCodeGenRdQWordMemory (
>>>>
>>>> +                   FALSE,
>>>>
>>>> +                   IsPosDecode,
>>>>
>>>> +                   TRUE,
>>>>
>>>> +                   TRUE,
>>>>
>>>> +                   FALSE, // non-cacheable
>>>>
>>>> +                   TRUE,
>>>>
>>>> +                   0,
>>>>
>>>> +                   AddrMapInfo->PciAddress,
>>>>
>>>> +                   AddrMapInfo->PciAddress + 
>>>> AddrMapInfo->AddressSize - 1,
>>>>
>>>> +                   Translation ? AddrMapInfo->CpuAddress - 
>>>> AddrMapInfo->PciAddress : 0,
>>>>
>>>> +                   AddrMapInfo->AddressSize,
>>>>
>>>> +                   0,
>>>>
>>>> +                   NULL,
>>>>
>>>> +                   0,
>>>>
>>>> +                   TRUE,
>>>>
>>>> +                   CrsNode,
>>>>
>>>> +                   NULL
>>>>
>>>> +                   );
>>>>
>>>> +        break;
>>>>
>>>> +      default:
>>>>
>>>> +        break;
>>>>
>>>> +    } // switch
>>>>
>>>> +
>>>>
>>>> +    if (EFI_ERROR (Status)) {
>>>>
>>>> +      ASSERT (0);
>>>>
>>>> +      return Status;
>>>>
>>>> +    }
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>> +  return Status;
>>>>
>>>> +}
>>>>
>>>> +
>>>>
>>>>   /** Generate a Pci device.
>>>>
>>>>
>>>>     @param [in]       Generator       The SSDT Pci generator.
>>>>
>>>> @@ -702,9 +865,17 @@ GeneratePciDevice (
>>>>       return Status;
>>>>
>>>>     }
>>>>
>>>>
>>>> +  // Add the PNP Motherboard Resources Device to reserve ECAM space
>>>>
>>>> +  Status = ReserveEcamSpace (Generator, CfgMgrProtocol, PciInfo, 
>>>> PciNode);
>>>>
>>>> +  if (EFI_ERROR (Status)) {
>>>>
>>>> +    ASSERT (0);
>>>>
>>>> +    return Status;
>>>>
>>>> +  }
>>>>
>>>> +
>>>>
>>>>     // Add the template _OSC method.
>>>>
>>>>     Status = AddOscMethod (PciInfo, PciNode);
>>>>
>>>>     ASSERT_EFI_ERROR (Status);
>>>>
>>>> +
>>>>
>>>>     return Status;
>>>>
>>>>   }
>>>>
>>>>

[PATCH v3 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config

[Kun Qin]

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998

This change added a switch case handling for PCI_SS_CONFIG during SSDT
generation. This will allow PCI config case return EFI_SUCCESS instead of
EFI_INVALID_PARAMETER.

Cc: Sami Mujawar <Sami.Mujawar@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>

Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---

Notes:
    v2:
    - Added Reviewed-by tag [Pierre]
    
    v3:
    - No change

 DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
index 658a089c8f1f..740271b504ca 100644
--- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
+++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
@@ -603,6 +603,11 @@ GeneratePciCrs (
                    );
         break;
 
+      case PCI_SS_CONFIG:
+        // Do nothing
+        Status = EFI_SUCCESS;
+        break;
+
       default:
         Status = EFI_INVALID_PARAMETER;
     } // switch
-- 
2.37.1.windows.1

Re: [PATCH v3 6/6] DynamicTablesPkg: AcpiSsdtPcieLibArm: Added case handling for PCI config

[Sami Mujawar]

Hi Kun,

Thank you for this patch.

This change looks good to me.

Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>

Regards,

Sami Mujawar

On 31/07/2022 06:37 am, Kun Qin wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3998
>
> This change added a switch case handling for PCI_SS_CONFIG during SSDT
> generation. This will allow PCI config case return EFI_SUCCESS instead of
> EFI_INVALID_PARAMETER.
>
> Cc: Sami Mujawar <Sami.Mujawar@arm.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>
> Notes:
>      v2:
>      - Added Reviewed-by tag [Pierre]
>      
>      v3:
>      - No change
>
>   DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c | 5 +++++
>   1 file changed, 5 insertions(+)
>
> diff --git a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> index 658a089c8f1f..740271b504ca 100644
> --- a/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> +++ b/DynamicTablesPkg/Library/Acpi/Arm/AcpiSsdtPcieLibArm/SsdtPcieGenerator.c
> @@ -603,6 +603,11 @@ GeneratePciCrs (
>                      );
>
>           break;
>
>   
>
> +      case PCI_SS_CONFIG:
>
> +        // Do nothing
>
> +        Status = EFI_SUCCESS;
>
> +        break;
>
> +
>
>         default:
>
>           Status = EFI_INVALID_PARAMETER;
>
>       } // switch
>