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

["Sami Mujawar" <sami.mujawar@arm.com>]

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@arm.com>

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@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>
> ---
>   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
>