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

["Kun Qin" <kuqin12@gmail.com>]

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