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

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

Hello Kun,

On 7/19/22 02:22, 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 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>
>   
>   // Module specific include files.
>   #include <AcpiTableGenerator.h>
> @@ -387,6 +388,57 @@ BuildAndInstallAcpiTable (
>     return Status;
>   }
>   

[snip]

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

Since there are many tables, maybe it would be good to factorize the code and
create a static array containing all the tables that are mandatory, and containing:
1. the ESTD_ACPI_TABLE_ID of the table
2. the table signature (*_DESCRIPTION_TABLE_SIGNATURE)
3. the table name (for printing)
4. whether the table was found (dynamically populated)
(maybe other fields would be required)

Like this we could have 2 loops in VerifyMandatoryTablesArePresent(),
one going through AcpiTableInfo[AcpiTableCount].AcpiTableSignature,
and a second one going through the already installed tables (AcpiSdt->GetAcpiTable (Index, ...))

This should also allow to simplify the code for installing the FADT aswell
and code of LocateAcpiTableBySignature() would be included in VerifyMandatoryTablesArePresent().

Also, I think you will have to rebase your patches on the latest master and
do the same thing as
6cda306da1dd DynamicTablesPkg: AcpiSsdtPcieLibArm: Correct translation value
does.

Regards,
Pierre