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


Kun Qin
 

Thank you for your reviews, Sami! I will incorporate your suggestions in v2 patch and send for review after validation.

Regards,
Kun

On 7/20/2022 4:00 AM, Sami Mujawar wrote:
Hi Kun,

Thank you for this patch.

I have some minor suggestions marked inline as [SAMI], otherwise this patch looks good to me.

With that updated.

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

Regards,

Sami Mujawar

On 19/07/2022 01:22 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 and DBG2 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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 200 ++++++++++++++++----
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   1 +
  2 files changed, 167 insertions(+), 34 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..ac5fe0bed91b 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -11,6 +11,7 @@
  #include <Library/PcdLib.h>

  #include <Library/UefiBootServicesTableLib.h>

  #include <Protocol/AcpiTable.h>

+#include <Protocol/AcpiSystemDescriptionTable.h>
[SAMI] Can thie include statement above be alphabetically ordered, please?


  // Module specific include files.

  #include <AcpiTableGenerator.h>

@@ -387,6 +388,57 @@ BuildAndInstallAcpiTable (
    return Status;

  }


+/**

+  This function uses the ACPI SDT protocol to locate an ACPI table.

+  It is really only useful for finding tables that only have a single instance,

+  e.g. FADT, FACS, MADT, etc.  It is not good for locating SSDT, etc.

+

+  @param[in] Signature           - Pointer to an ASCII string containing the OEM Table ID from the ACPI table header

+  @param[in, out] Table          - Updated with a pointer to the table

+  @param[in, out] Handle         - AcpiSupport protocol table handle for the table found

+

+  @retval EFI_SUCCESS            - The function completed successfully.
[SAMI] Please add EFI_NOT_FOUND as a return type if an ACPI table with the requested signature is not found or if the ACPI SDT protocol is not installed.


+**/

+STATIC

+EFI_STATUS

+LocateAcpiTableBySignature (

+  IN      UINT32                       Signature,

+  IN OUT  EFI_ACPI_DESCRIPTION_HEADER  **Table,

+  IN OUT  UINTN                        *Handle

+  )

+{

+  EFI_STATUS              Status;

+  INTN                    Index;

+  EFI_ACPI_TABLE_VERSION  Version;

+  EFI_ACPI_SDT_PROTOCOL   *AcpiSdt;

+

+  AcpiSdt = NULL;

+  Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);

+

+  if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {

+    return EFI_NOT_FOUND;

+  }

+

+  //

+  // Locate table with matching ID

+  //

+  Version = 0;

+  Index   = 0;

+  do {

+    Status = AcpiSdt->GetAcpiTable (Index, (EFI_ACPI_SDT_HEADER **)Table, &Version, Handle);

+    if (EFI_ERROR (Status)) {

+      break;

+    }

+

+    Index++;

+  } while ((*Table)->Signature != Signature);

+

+  //

+  // If we found the table, there will be no error.

+  //

+  return Status;

+}

+

  /** The function checks if the Configuration Manager has provided the

      mandatory ACPI tables for installation.


@@ -411,6 +463,9 @@ VerifyMandatoryTablesArePresent (
    BOOLEAN     DsdtFound;

    BOOLEAN     Dbg2Found;

    BOOLEAN     SpcrFound;

+  UINTN       Handle;

+

+  EFI_ACPI_DESCRIPTION_HEADER  *DummyHeader;


    Status    = EFI_SUCCESS;

    FadtFound = FALSE;

@@ -447,32 +502,99 @@ VerifyMandatoryTablesArePresent (
    }


    // 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"));

+  // But they also might be published already, so we can search from there

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !FadtFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found.\n"));

      Status = EFI_NOT_FOUND;

+  } else if (!EFI_ERROR (Status) && FadtFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;
[SAMI] Please update the function documentation header to reflect the EFI_ALREADY_STARTED error code.

+  } else {

+    FadtFound = TRUE;

    }


-  if (!MadtFound) {

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !MadtFound) {

      DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));

      Status = EFI_NOT_FOUND;

+  } else if (!EFI_ERROR (Status) && MadtFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;

+  } else {

+    MadtFound = TRUE;

    }


-  if (!GtdtFound) {

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !GtdtFound) {

      DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));

      Status = EFI_NOT_FOUND;

+  } else if (!EFI_ERROR (Status) && GtdtFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;

+  } else {

+    GtdtFound = TRUE;

    }


-  if (!DsdtFound) {

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !DsdtFound) {

      DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));

      Status = EFI_NOT_FOUND;

+  } else if (!EFI_ERROR (Status) && DsdtFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;

+  } else {

+    DsdtFound = TRUE;

    }


-  if (!Dbg2Found) {

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+             EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !Dbg2Found) {

      DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));

+  } else if (!EFI_ERROR (Status) && Dbg2Found) {

+    DEBUG ((DEBUG_ERROR, "ERROR: DBG2 Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;

+  } else {

+    Dbg2Found = TRUE;

    }


-  if (!SpcrFound) {

+  Handle = 0;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status) && !SpcrFound) {

      DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));

+  } else if (!EFI_ERROR (Status) && SpcrFound) {

+    DEBUG ((DEBUG_ERROR, "ERROR: SPCR Table found while already published.\n"));

+    Status = EFI_ALREADY_STARTED;

+  } else {

+    SpcrFound = TRUE;

    }


    return Status;

@@ -500,11 +622,13 @@ ProcessAcpiTables (
    IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST CfgMgrProtocol

    )

  {

-  EFI_STATUS                  Status;

-  EFI_ACPI_TABLE_PROTOCOL     *AcpiTableProtocol;

-  CM_STD_OBJ_ACPI_TABLE_INFO  *AcpiTableInfo;

-  UINT32                      AcpiTableCount;

-  UINT32                      Idx;

+  EFI_STATUS                   Status;

+  EFI_ACPI_TABLE_PROTOCOL      *AcpiTableProtocol;

+  CM_STD_OBJ_ACPI_TABLE_INFO   *AcpiTableInfo;

+  UINT32                       AcpiTableCount;

+  UINT32                       Idx;

+  UINTN                        Handle;

+  EFI_ACPI_DESCRIPTION_HEADER  *DummyHeader;


    ASSERT (TableFactoryProtocol != NULL);

    ASSERT (CfgMgrProtocol != NULL);

@@ -570,29 +694,37 @@ ProcessAcpiTables (
    }
[snip]
  // Check if mandatory ACPI tables are present.
  Status = VerifyMandatoryTablesArePresent (
             AcpiTableInfo,
             AcpiTableCount
             );
  if (EFI_ERROR (Status)) {
    DEBUG ((
      DEBUG_ERROR,
      "ERROR: Failed to find mandatory ACPI Table(s)."
      " Status = %r\n",
      Status

      ));


[SAMI] Is it possible to update the error reporting to reflect the EFI_ALREADY_STARTED error type, please? Please also update the function documentation header for ProcessAcpiTables().


    return Status;

  }


[/snip]



    // 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;

+  Status = LocateAcpiTableBySignature (

+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,

+             &DummyHeader,

+             &Handle

+             );

+  if (EFI_ERROR (Status)) {

+    // 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;

        }

-

-      break;

-    }

-  } // for

+    } // 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..5ca98c8b4895 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -36,6 +36,7 @@ [LibraryClasses]

  [Protocols]

    gEfiAcpiTableProtocolGuid                     # PROTOCOL ALWAYS_CONSUMED

+  gEfiAcpiSdtProtocolGuid                       # PROTOCOL ALWAYS_CONSUMED
[SAMI] Should the DEPEX section be updated to relect the dependency on the SDT protocol?



    gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL ALWAYS_CONSUMED

    gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL ALWAYS_CONSUMED

Join devel@edk2.groups.io to automatically receive all group messages.