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


Kun Qin
 

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D3997

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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
v2:
- Function description updates [Sami]
- Refactorized the table verification [Pierre]

DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c =
| 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf=
| 1 +
2 files changed, 103 insertions(+), 80 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableMa=
nagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableMa=
nagerDxe.c
index ed62299f9bbd..4ad7c0c8dbfa 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.c
@@ -10,6 +10,7 @@
#include <Library/DebugLib.h>=0D
#include <Library/PcdLib.h>=0D
#include <Library/UefiBootServicesTableLib.h>=0D
+#include <Protocol/AcpiSystemDescriptionTable.h>=0D
#include <Protocol/AcpiTable.h>=0D
=0D
// Module specific include files.=0D
@@ -22,6 +23,29 @@
#include <Protocol/DynamicTableFactoryProtocol.h>=0D
#include <SmbiosTableGenerator.h>=0D
=0D
+#define ACPI_TABLE_PRESENT_INFO_LIST BIT0=0D
+#define ACPI_TABLE_PRESENT_INSTALLED BIT1=0D
+=0D
+#define ACPI_TABLE_VERIFY_FADT 0=0D
+#define ACPI_TABLE_VERIFY_COUNT 6=0D
+=0D
+typedef struct {=0D
+ ESTD_ACPI_TABLE_ID EstdTableId;=0D
+ UINT32 AcpiTableSignature;=0D
+ CHAR8 AcpiTableName[sizeof (UINT32) + 1];=0D
+ BOOLEAN IsMandatory;=0D
+ UINT16 Presence;=0D
+} ACPI_TABLE_PRESENCE_INFO;=0D
+=0D
+ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] =3D {=
=0D
+ { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATU=
RE, "FADT", TRUE, 0 },=0D
+ { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGN=
ATURE, "MADT", TRUE, 0 },=0D
+ { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGN=
ATURE, "GTDT", TRUE, 0 },=0D
+ { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TA=
BLE_SIGNATURE, "DSDT", TRUE, 0 },=0D
+ { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, =
"DBG2", FALSE, 0 },=0D
+ { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABL=
E_SIGNATURE, "SPCR", FALSE, 0 },=0D
+};=0D
+=0D
/** This macro expands to a function that retrieves the ACPI Table=0D
List from the Configuration Manager.=0D
*/=0D
@@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
=0D
@retval EFI_SUCCESS Success.=0D
@retval EFI_NOT_FOUND If mandatory table is not found.=0D
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo =
is already installed.=0D
**/=0D
STATIC=0D
EFI_STATUS=0D
@@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
IN UINT32 AcpiTableCount=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
- BOOLEAN FadtFound;=0D
- BOOLEAN MadtFound;=0D
- BOOLEAN GtdtFound;=0D
- BOOLEAN DsdtFound;=0D
- BOOLEAN Dbg2Found;=0D
- BOOLEAN SpcrFound;=0D
+ EFI_STATUS Status;=0D
+ UINTN Handle;=0D
+ UINTN Index;=0D
+ UINTN InstalledTableIndex;=0D
+ EFI_ACPI_DESCRIPTION_HEADER *DescHeader;=0D
+ EFI_ACPI_TABLE_VERSION Version;=0D
+ EFI_ACPI_SDT_PROTOCOL *AcpiSdt;=0D
=0D
- Status =3D EFI_SUCCESS;=0D
- FadtFound =3D FALSE;=0D
- MadtFound =3D FALSE;=0D
- GtdtFound =3D FALSE;=0D
- DsdtFound =3D FALSE;=0D
- Dbg2Found =3D FALSE;=0D
- SpcrFound =3D FALSE;=0D
ASSERT (AcpiTableInfo !=3D NULL);=0D
=0D
+ Status =3D EFI_SUCCESS;=0D
+=0D
+ // Check against the statically initialized ACPI tables to see if they a=
re in ACPI info list=0D
while (AcpiTableCount-- !=3D 0) {=0D
- switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {=0D
- case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:=0D
- FadtFound =3D TRUE;=0D
- break;=0D
- case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:=0D
- MadtFound =3D TRUE;=0D
- break;=0D
- case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:=0D
- GtdtFound =3D TRUE;=0D
- break;=0D
- case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:=
=0D
- DsdtFound =3D TRUE;=0D
- break;=0D
- case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:=0D
- Dbg2Found =3D TRUE;=0D
- break;=0D
- case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:=0D
- SpcrFound =3D TRUE;=0D
- break;=0D
- default:=0D
- break;=0D
+ for (Index =3D 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {=0D
+ if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature =3D=3D mAcpiVer=
ifyTables[Index].AcpiTableSignature) {=0D
+ mAcpiVerifyTables[Index].Presence |=3D ACPI_TABLE_PRESENT_INFO_LIS=
T;=0D
+ }=0D
}=0D
}=0D
=0D
- // We need at least the FADT, MADT, GTDT and the DSDT tables to boot=0D
- if (!FadtFound) {=0D
- DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));=0D
- Status =3D EFI_NOT_FOUND;=0D
- }=0D
+ // They also might be published already, so we can search from there=0D
+ AcpiSdt =3D NULL;=0D
+ Status =3D gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID *=
*)&AcpiSdt);=0D
=0D
- if (!MadtFound) {=0D
- DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));=0D
- Status =3D EFI_NOT_FOUND;=0D
+ if (EFI_ERROR (Status) || (AcpiSdt =3D=3D NULL)) {=0D
+ DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p=
) - %r\n", AcpiSdt, Status));=0D
+ goto EvaluatePresence;=0D
}=0D
=0D
- if (!GtdtFound) {=0D
- DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));=0D
- Status =3D EFI_NOT_FOUND;=0D
- }=0D
+ for (Index =3D 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {=0D
+ Handle =3D 0;=0D
+ InstalledTableIndex =3D 0;=0D
+ do {=0D
+ Status =3D AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT=
_HEADER **)&DescHeader, &Version, &Handle);=0D
+ if (EFI_ERROR (Status)) {=0D
+ break;=0D
+ }=0D
=0D
- if (!DsdtFound) {=0D
- DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));=0D
- Status =3D EFI_NOT_FOUND;=0D
- }=0D
+ InstalledTableIndex++;=0D
+ } while (DescHeader->Signature !=3D mAcpiVerifyTables[Index].AcpiTable=
Signature);=0D
=0D
- if (!Dbg2Found) {=0D
- DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));=0D
+ if (!EFI_ERROR (Status)) {=0D
+ mAcpiVerifyTables[Index].Presence |=3D ACPI_TABLE_PRESENT_INSTALLED;=
=0D
+ }=0D
}=0D
=0D
- if (!SpcrFound) {=0D
- DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));=0D
+EvaluatePresence:=0D
+ for (Index =3D 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {=0D
+ if (mAcpiVerifyTables[Index].Presence =3D=3D 0) {=0D
+ if (mAcpiVerifyTables[Index].IsMandatory) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTa=
bles[Index].AcpiTableName));=0D
+ Status =3D EFI_NOT_FOUND;=0D
+ } else {=0D
+ DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyT=
ables[Index].AcpiTableName));=0D
+ }=0D
+ } else if (mAcpiVerifyTables[Index].Presence =3D=3D=0D
+ (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLE=
D))=0D
+ {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.=
\n", mAcpiVerifyTables[Index].AcpiTableName));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ }=0D
}=0D
=0D
return Status;=0D
@@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
@param [in] CfgMgrProtocol Pointer to the Configuration Manager=0D
Protocol Interface.=0D
=0D
- @retval EFI_SUCCESS Success.=0D
- @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.=
=0D
+ @retval EFI_SUCCESS Success.=0D
+ @retval EFI_NOT_FOUND If a mandatory table or a generator is not=
found.=0D
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo =
is already installed.=0D
**/=0D
STATIC=0D
EFI_STATUS=0D
@@ -562,7 +581,7 @@ ProcessAcpiTables (
if (EFI_ERROR (Status)) {=0D
DEBUG ((=0D
DEBUG_ERROR,=0D
- "ERROR: Failed to find mandatory ACPI Table(s)."=0D
+ "ERROR: Failed to verify mandatory ACPI Table(s) presence."=0D
" Status =3D %r\n",=0D
Status=0D
));=0D
@@ -570,29 +589,32 @@ ProcessAcpiTables (
}=0D
=0D
// Add the FADT Table first.=0D
- for (Idx =3D 0; Idx < AcpiTableCount; Idx++) {=0D
- if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) =3D=3D=0D
- AcpiTableInfo[Idx].TableGeneratorId)=0D
- {=0D
- Status =3D BuildAndInstallAcpiTable (=0D
- TableFactoryProtocol,=0D
- CfgMgrProtocol,=0D
- AcpiTableProtocol,=0D
- &AcpiTableInfo[Idx]=0D
- );=0D
- if (EFI_ERROR (Status)) {=0D
- DEBUG ((=0D
- DEBUG_ERROR,=0D
- "ERROR: Failed to find build and install ACPI FADT Table." \=0D
- " Status =3D %r\n",=0D
- Status=0D
- ));=0D
- return Status;=0D
- }=0D
+ if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRE=
SENT_INSTALLED) =3D=3D 0) {=0D
+ // FADT is not yet installed=0D
+ for (Idx =3D 0; Idx < AcpiTableCount; Idx++) {=0D
+ if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) =3D=3D=0D
+ AcpiTableInfo[Idx].TableGeneratorId)=0D
+ {=0D
+ Status =3D BuildAndInstallAcpiTable (=0D
+ TableFactoryProtocol,=0D
+ CfgMgrProtocol,=0D
+ AcpiTableProtocol,=0D
+ &AcpiTableInfo[Idx]=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=0D
+ DEBUG ((=0D
+ DEBUG_ERROR,=0D
+ "ERROR: Failed to find build and install ACPI FADT Table." \=0D
+ " Status =3D %r\n",=0D
+ Status=0D
+ ));=0D
+ return Status;=0D
+ }=0D
=0D
- break;=0D
- }=0D
- } // for=0D
+ break;=0D
+ }=0D
+ } // for=0D
+ }=0D
=0D
// Add remaining ACPI Tables=0D
for (Idx =3D 0; Idx < AcpiTableCount; Idx++) {=0D
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableMa=
nagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTable=
ManagerDxe.inf
index 028c3d413cf8..5ca98c8b4895 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.inf
@@ -36,6 +36,7 @@ [LibraryClasses]
=0D
[Protocols]=0D
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED=
=0D
+ gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED=
=0D
=0D
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED=
=0D
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED=
=0D
--=20
2.37.1.windows.1


PierreGondois
 

Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@...>

Thanks!

On 7/28/22 06:31, 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, 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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>
Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
Notes:
v2:
- Function description updates [Sami]
- Refactorized the table verification [Pierre]
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
2 files changed, 103 insertions(+), 80 deletions(-)
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..4ad7c0c8dbfa 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,29 @@
#include <Protocol/DynamicTableFactoryProtocol.h>
#include <SmbiosTableGenerator.h>
+#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED BIT1
+
+#define ACPI_TABLE_VERIFY_FADT 0
+#define ACPI_TABLE_VERIFY_COUNT 6
+
+typedef struct {
+ ESTD_ACPI_TABLE_ID EstdTableId;
+ UINT32 AcpiTableSignature;
+ CHAR8 AcpiTableName[sizeof (UINT32) + 1];
+ BOOLEAN IsMandatory;
+ UINT16 Presence;
+} ACPI_TABLE_PRESENCE_INFO;
I think it needs some documentation (also for mAcpiVerifyTables).

+
+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 +419,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 +429,68 @@ 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:
- break;
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
+ mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
Would it be possible to add a 'break' here ?

Just a note for Sami:
These double loops seem expensive, but I cannot find anything better and/or shorter.

+ }
}
}
- // 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
+ 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_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
+ goto EvaluatePresence;
I think this is ok to just print and return an error, unless you think about this could happen.


}
- 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"));
+EvaluatePresence:
+ 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;
+ }
}
Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.


return Status;
@@ -489,8 +507,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 +581,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 +589,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) {
+ // 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..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
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED


Kun Qin
 

Hi Pierre,

Thank you for your feedback. I will add more document/comments to the newly define structure, as
well as the "break" as you suggested.

As per failure to locate "gEfiAcpiSdtProtocolGuid", my thought was that this protocol could be enabled
per platform through "gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol". So I did not treat
the failure as a hard requirement (for the same reason, I did not add it to the module Depex). Please let
me know whether you think this makes sense. Otherwise, I could replace the "goto" logic with a check
for the same PCD and only conduct the routine if ACPI_SDT is expected.

Please also let me know if you have other suggestions.

Thanks,
Kun

On 7/28/2022 6:07 AM, Pierre Gondois wrote:
Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@...>

Thanks!

On 7/28/22 06:31, 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, 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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
     v2:
     - Function description updates [Sami]
     - Refactorized the table verification [Pierre]

DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf |   1 +
  2 files changed, 103 insertions(+), 80 deletions(-)

diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..4ad7c0c8dbfa 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,29 @@
  #include <Protocol/DynamicTableFactoryProtocol.h>
  #include <SmbiosTableGenerator.h>
  +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
+
+#define ACPI_TABLE_VERIFY_FADT   0
+#define ACPI_TABLE_VERIFY_COUNT  6
+
+typedef struct {
+  ESTD_ACPI_TABLE_ID    EstdTableId;
+  UINT32                AcpiTableSignature;
+  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
+  BOOLEAN               IsMandatory;
+  UINT16                Presence;
+} ACPI_TABLE_PRESENCE_INFO;
I think it needs some documentation (also for mAcpiVerifyTables).

+
+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 +419,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 +429,68 @@ 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:
-        break;
+    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
+        mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST;
Would it be possible to add a 'break' here ?

Just a note for Sami:
These double loops seem expensive, but I cannot find anything better and/or shorter.

+      }
      }
    }
  -  // 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
+  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_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
+    goto EvaluatePresence;
I think this is ok to just print and return an error, unless you think about this could happen.


    }
  -  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"));
+EvaluatePresence:
+  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;
+    }
    }
Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.


      return Status;
@@ -489,8 +507,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 +581,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 +589,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) {
+    // 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..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
      gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL ALWAYS_CONSUMED
    gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL ALWAYS_CONSUMED


PierreGondois
 

On 7/29/22 07:00, Kun Qin wrote:
Hi Pierre,
Thank you for your feedback. I will add more document/comments to the
newly define structure, as
well as the "break" as you suggested.
As per failure to locate "gEfiAcpiSdtProtocolGuid", my thought was that
this protocol could be enabled
per platform through
"gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol". So I did not
treat
the failure as a hard requirement (for the same reason, I did not add it
to the module Depex). Please let
me know whether you think this makes sense. Otherwise, I could replace
the "goto" logic with a check
for the same PCD and only conduct the routine if ACPI_SDT is expected.
Ok yes, this is a good idea,
Regards,
Pierre

Please also let me know if you have other suggestions.
Thanks,
Kun
On 7/28/2022 6:07 AM, Pierre Gondois wrote:
Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@...>

Thanks!

On 7/28/22 06:31, 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, 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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---

Notes:
     v2:
     - Function description updates [Sami]
     - Refactorized the table verification [Pierre]

DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
| 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
|   1 +
  2 files changed, 103 insertions(+), 80 deletions(-)

diff --git
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c

index ed62299f9bbd..4ad7c0c8dbfa 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,29 @@
  #include <Protocol/DynamicTableFactoryProtocol.h>
  #include <SmbiosTableGenerator.h>
  +#define ACPI_TABLE_PRESENT_INFO_LIST  BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED  BIT1
+
+#define ACPI_TABLE_VERIFY_FADT   0
+#define ACPI_TABLE_VERIFY_COUNT  6
+
+typedef struct {
+  ESTD_ACPI_TABLE_ID    EstdTableId;
+  UINT32                AcpiTableSignature;
+  CHAR8                 AcpiTableName[sizeof (UINT32) + 1];
+  BOOLEAN               IsMandatory;
+  UINT16                Presence;
+} ACPI_TABLE_PRESENCE_INFO;
I think it needs some documentation (also for mAcpiVerifyTables).

+
+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 +419,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 +429,68 @@ 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:
-        break;
+    for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+      if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature ==
mAcpiVerifyTables[Index].AcpiTableSignature) {
+        mAcpiVerifyTables[Index].Presence |=
ACPI_TABLE_PRESENT_INFO_LIST;
Would it be possible to add a 'break' here ?

Just a note for Sami:
These double loops seem expensive, but I cannot find anything better
and/or shorter.

+      }
      }
    }
  -  // 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
+  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_WARN, "WARNING: Failed to locate ACPI SDT protocol
(0x%p) - %r\n", AcpiSdt, Status));
+    goto EvaluatePresence;
I think this is ok to just print and return an error, unless you think
about this could happen.


    }
  -  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"));
+EvaluatePresence:
+  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;
+    }
    }
Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.


      return Status;
@@ -489,8 +507,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 +581,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 +589,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) {
+    // 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..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
      gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL
ALWAYS_CONSUMED
    gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL
ALWAYS_CONSUMED