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

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

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
>