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.muja...@arm.com>
Cc: Alexei Fedorov <alexei.fedo...@arm.com>

Co-authored-by: Joe Lopez <joelo...@microsoft.com>
Signed-off-by: Kun Qin <kuqi...@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gond...@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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92200): https://edk2.groups.io/g/devel/message/92200
Mute This Topic: https://groups.io/mt/92722842/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to