From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f181.google.com (mail-pg1-f181.google.com [209.85.215.181]) by mx.groups.io with SMTP id smtpd.web11.965.1660170663834080255 for ; Wed, 10 Aug 2022 15:31:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=OkHm7A71; spf=pass (domain: gmail.com, ip: 209.85.215.181, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f181.google.com with SMTP id q16so15575900pgq.6 for ; Wed, 10 Aug 2022 15:31:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=QQvNo1TiU65huFw3qQjgAVQPGyy9js1RdWfRelZKP4Y=; b=OkHm7A715yUm51ZSsWYQ7dH7b/dF8Mh9xDCYaOsoE5IaHQE6jD5hps4XzJlx63mlep 9q5k/eLwFetwrOmo7HN9uN/eqNHcGlnK1ugyETZyrJNMNZEecW7lwSyLbWvh2JJYx74O ZYbcIXw9CkYwBO12NM2IfJRiCuFxaqhF3KYXMp0jjilLxWyIChuqS4WoOvwymocZtJtX YLtGDK1U//p4bzUl7wVNuynsxQOTJU1lBGL/3pf0a5W0/xlfsMjS0qyZEI0IVSUUpTvw JkYuhSaHuzWcaskn+x+g/JZrfN4KsNvOgMxSrBO69NUQl9SbbSc2t4xdYDhClwOYQKh3 CNIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=QQvNo1TiU65huFw3qQjgAVQPGyy9js1RdWfRelZKP4Y=; b=dLhb9zWStPsQh9L/6R5amgsN/p15JPa+lZbAAG1LSkjvJAsLSEcWJYbQA26+cL5t5C kAkw412aH911sj/KNoDNquZpNfFzu1oW93Sh+ZQwkD9YRMacYsiAwnthx2YW43rtUR10 XfugCdczkvxBUjkvf8mxBuOhmU2hzWCfVtVfpf/G+NdmcvLhhJhTs7c9abxWrErwM7Bz pPO+GHLcvA5Wgy8Hby+4SmHE2cZ6pc2rnYS4biI8kTVQv8HlucGjgQL4ENHcc7FgG86z Wrx663UTz402gf8e/ObZxAps8OKAYTItMVWyAZcUFBAUd7lzoORO9jVzdR68PL/nAJZD ScCQ== X-Gm-Message-State: ACgBeo2YYlPmIryiPdVJyaYErxjqYDiaaS7+bIgat/f5vangMCJNcJq6 RxlkWFa+SMk6EqfVrG//020= X-Google-Smtp-Source: AA6agR7smqjiWtKejVgEW6n/xmwPpDK7iLORZH3xa93uMdS0mYbiznHW0R3A21EQf+S8iXF4/9Kdkw== X-Received: by 2002:a65:6d14:0:b0:41d:5f95:179d with SMTP id bf20-20020a656d14000000b0041d5f95179dmr14785763pgb.580.1660170663131; Wed, 10 Aug 2022 15:31:03 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:352e:f1a8:aef2:7dc4? ([2001:4898:80e8:38:b518:f1a8:aef2:7dc4]) by smtp.gmail.com with ESMTPSA id p16-20020a170902e75000b0016dc2153f54sm13576931plf.299.2022.08.10.15.31.02 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 10 Aug 2022 15:31:02 -0700 (PDT) Message-ID: Date: Wed, 10 Aug 2022 15:31:02 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables To: Sami Mujawar , "devel@edk2.groups.io" Cc: Alexei Fedorov , Joe Lopez , Pierre Gondois , nd References: <20220731053727.536-1-kuqin12@gmail.com> <20220731053727.536-5-kuqin12@gmail.com> <7FC748E2-5436-450E-BC33-F9437168A88A@arm.com> From: "Kun Qin" In-Reply-To: <7FC748E2-5436-450E-BC33-F9437168A88A@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Hi Sami, Thanks for your Acks below. The updated patch is sent here: https://edk2.groups.io/g/devel/message/92317 Please let me know if there is any further feedback/issues. Regards, Kun On 8/9/2022 2:01 AM, Sami Mujawar wrote: > Hi Kun, > > Please find my response inline marked [SAMI]. > > Regards, > > Sami Mujawar > > On 09/08/2022, 00:44, "Kun Qin" wrote: > > Hi Sami, > > Thank you for taking time testing this change! > > I have a question about one comment you have for this specific patch > inline (marked with [KQ]). > Could you please provide more details? > > I also responded to your other comments, please let me know if the > proposed change makes > sense to you. Looking forward to your reply. > > Thanks, > Kun > > On 8/8/2022 8:39 AM, Sami Mujawar wrote: > > Hi Kun, > > > > Please find my response inline marked [SAMI]. > > > > Regards, > > > > Sami Mujawar > > > > On 08/08/2022 02:05 pm, Sami Mujawar wrote: > >> 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 > >>> Cc: Alexei Fedorov > >>> > >>> Co-authored-by: Joe Lopez > >>> Signed-off-by: Kun Qin > >>> Reviewed-by: Pierre Gondois > >>> --- > >>> > >>> 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 > >>> > >>> #include > >>> > >>> #include > >>> > >>> +#include > >>> > >>> #include > >>> > >>> > >>> // Module specific include files. > >>> > >>> @@ -22,6 +23,58 @@ > >>> #include > >>> > >>> #include > >>> > >>> > >>> +/// > >>> > >>> +/// 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)) { > > > > [SAMI] When running Kvmtool guest firmware I break from here with > > EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE > > in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any > > preinstalled tables (i.e. all tables are generated using > > DynamicTablesFramework). > > > > This means platforms that use only Dynamic Tables Framework would all > > need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to > > rework this logic so that the existing platform code does not need > > updating, please? > > > > [/SAMI] > > [KQ] > > There was a mistake that the Status should be set to EFI_SUCCESS before > entering the presence > checking step. Otherwise it will remain the same value from GetAcpiTable > if all tables are present. > > To your original observation, it is correct that this GetAcpiTable call > will fail with EFI_NOT_FOUND, > but it should only break from the internal `do-while` loop, and continue > with the rest of table > look-ups. Then during table inspection step, either installed table or > from info_list will count as a > passed check. The return Status should be reset before inspecting the > full look-up results. Thanks > for catching this! Will add the `Status = EFI_SUCCESS` statement in the > next round. > > [/KQ] > > [SAMI] Ack. I look forward to an updated patch with this fixed. > > > > >>> > >>> + 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] > > [KQ] > > You are correct that there will be only above 2 conditions for mandatory > tables. > But the check is to make sure the FADT will only be installed on > condition #1 above, > which means it will only be installed if it has not already been > installed. Please let > me know if I missed anything here. > > [/KQ] > [SAMI] Ack. Please keep this check. If the table is already installed this check prevents searching in the AcpiTableInfo list and therefore optimises. > >> > >>> > >>> + // 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 > >>> >