From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from foss.arm.com (foss.arm.com [217.140.110.172])
 by mx.groups.io with SMTP id smtpd.web09.53786.1658324238226297538
 for <devel@edk2.groups.io>;
 Wed, 20 Jul 2022 06:37:18 -0700
Authentication-Results: mx.groups.io;
 dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: pierre.gondois@arm.com)
Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14])
	by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 2AA6413D5;
	Wed, 20 Jul 2022 06:37:18 -0700 (PDT)
Received: from [10.34.100.102] (pierre123.nice.arm.com [10.34.100.102])
	by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id B1BCF3F70D;
	Wed, 20 Jul 2022 06:37:16 -0700 (PDT)
Message-ID: <757b4f8a-4ad3-b140-3623-9e00dce62196@arm.com>
Date: Wed, 20 Jul 2022 15:36:54 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.1
From: "PierreGondois" <pierre.gondois@arm.com>
Subject: Re: [edk2-devel] [PATCH v1 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
To: devel@edk2.groups.io, kuqin12@gmail.com
Cc: Sami Mujawar <Sami.Mujawar@arm.com>,
 Alexei Fedorov <Alexei.Fedorov@arm.com>, Joe Lopez <joelopez@microsoft.com>
References: <20220719002254.1891-1-kuqin12@gmail.com>
 <20220719002254.1891-5-kuqin12@gmail.com>
In-Reply-To: <20220719002254.1891-5-kuqin12@gmail.com>
Content-Language: en-US
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

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