Re: [edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

["PierreGondois" <pierre.gondois@arm.com>]

Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>

Thanks!

On 7/28/22 06:31, Kun Qin via groups.io 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>
> ---
> 
> Notes:
>      v2:
>      - Function description updates [Sami]
>      - Refactorized the table verification [Pierre]
> 
>   DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c   | 182 +++++++++++---------
>   DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   1 +
>   2 files changed, 103 insertions(+), 80 deletions(-)
> 
> diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
> index ed62299f9bbd..4ad7c0c8dbfa 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,29 @@
>   #include <Protocol/DynamicTableFactoryProtocol.h>
>   #include <SmbiosTableGenerator.h>
>   
> +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
> +#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
> +
> +#define ACPI_TABLE_VERIFY_FADT   0
> +#define ACPI_TABLE_VERIFY_COUNT  6
> +
> +typedef struct {
> +  ESTD_ACPI_TABLE_ID    EstdTableId;
> +  UINT32                AcpiTableSignature;
> +  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
> +  BOOLEAN               IsMandatory;
> +  UINT16                Presence;
> +} ACPI_TABLE_PRESENCE_INFO;

I think it needs some documentation (also for mAcpiVerifyTables).

> +
> +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 +419,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 +429,68 @@ 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:
> -        break;
> +    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
> +      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
> +        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;

Would it be possible to add a 'break' here ?

Just a note for Sami:
These double loops seem expensive, but I cannot find anything better and/or shorter.

> +      }
>       }
>     }
>   
> -  // 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
> +  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_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
> +    goto EvaluatePresence;

I think this is ok to just print and return an error, unless you think about this could happen.


>     }
>   
> -  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"));
> +EvaluatePresence:
> +  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;
> +    }
>     }
Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.


>   
>     return Status;
> @@ -489,8 +507,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 +581,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 +589,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..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
>   
>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL ALWAYS_CONSUMED
>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL ALWAYS_CONSUMED