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

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
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/DynamicTableMa=
nagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableMa=
nagerDxe.c
index ed62299f9bbd..ac5fe0bed91b 100644
--- a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDx=
e.c
@@ -11,6 +11,7 @@
#include <Library/PcdLib.h>=0D
#include <Library/UefiBootServicesTableLib.h>=0D
#include <Protocol/AcpiTable.h>=0D
+#include <Protocol/AcpiSystemDescriptionTable.h>=0D
=0D
// Module specific include files.=0D
#include <AcpiTableGenerator.h>=0D
@@ -387,6 +388,57 @@ BuildAndInstallAcpiTable (
return Status;=0D
}=0D
=0D
+/**=0D
+ This function uses the ACPI SDT protocol to locate an ACPI table.=0D
+ It is really only useful for finding tables that only have a single inst=
ance,=0D
+ e.g. FADT, FACS, MADT, etc. It is not good for locating SSDT, etc.=0D
+=0D
+ @param[in] Signature - Pointer to an ASCII string containing t=
he OEM Table ID from the ACPI table header=0D
+ @param[in, out] Table - Updated with a pointer to the table=0D
+ @param[in, out] Handle - AcpiSupport protocol table handle for t=
he table found=0D
+=0D
+ @retval EFI_SUCCESS - The function completed successfully.=0D
+**/=0D
+STATIC=0D
+EFI_STATUS=0D
+LocateAcpiTableBySignature (=0D
+ IN UINT32 Signature,=0D
+ IN OUT EFI_ACPI_DESCRIPTION_HEADER **Table,=0D
+ IN OUT UINTN *Handle=0D
+ )=0D
+{=0D
+ EFI_STATUS Status;=0D
+ INTN Index;=0D
+ EFI_ACPI_TABLE_VERSION Version;=0D
+ EFI_ACPI_SDT_PROTOCOL *AcpiSdt;=0D
+=0D
+ AcpiSdt =3D NULL;=0D
+ Status =3D gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID *=
*)&AcpiSdt);=0D
+=0D
+ if (EFI_ERROR (Status) || (AcpiSdt =3D=3D NULL)) {=0D
+ return EFI_NOT_FOUND;=0D
+ }=0D
+=0D
+ //=0D
+ // Locate table with matching ID=0D
+ //=0D
+ Version =3D 0;=0D
+ Index =3D 0;=0D
+ do {=0D
+ Status =3D AcpiSdt->GetAcpiTable (Index, (EFI_ACPI_SDT_HEADER **)Table=
, &Version, Handle);=0D
+ if (EFI_ERROR (Status)) {=0D
+ break;=0D
+ }=0D
+=0D
+ Index++;=0D
+ } while ((*Table)->Signature !=3D Signature);=0D
+=0D
+ //=0D
+ // If we found the table, there will be no error.=0D
+ //=0D
+ return Status;=0D
+}=0D
+=0D
/** The function checks if the Configuration Manager has provided the=0D
mandatory ACPI tables for installation.=0D
=0D
@@ -411,6 +463,9 @@ VerifyMandatoryTablesArePresent (
BOOLEAN DsdtFound;=0D
BOOLEAN Dbg2Found;=0D
BOOLEAN SpcrFound;=0D
+ UINTN Handle;=0D
+=0D
+ EFI_ACPI_DESCRIPTION_HEADER *DummyHeader;=0D
=0D
Status =3D EFI_SUCCESS;=0D
FadtFound =3D FALSE;=0D
@@ -447,32 +502,99 @@ VerifyMandatoryTablesArePresent (
}=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
+ // But they also might be published already, so we can search from there=
=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !FadtFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found.\n"));=0D
Status =3D EFI_NOT_FOUND;=0D
+ } else if (!EFI_ERROR (Status) && FadtFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: FADT Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ FadtFound =3D TRUE;=0D
}=0D
=0D
- if (!MadtFound) {=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !MadtFound) {=0D
DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));=0D
Status =3D EFI_NOT_FOUND;=0D
+ } else if (!EFI_ERROR (Status) && MadtFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: MADT Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ MadtFound =3D TRUE;=0D
}=0D
=0D
- if (!GtdtFound) {=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !GtdtFound) {=0D
DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));=0D
Status =3D EFI_NOT_FOUND;=0D
+ } else if (!EFI_ERROR (Status) && GtdtFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ GtdtFound =3D TRUE;=0D
}=0D
=0D
- if (!DsdtFound) {=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATUR=
E,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !DsdtFound) {=0D
DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));=0D
Status =3D EFI_NOT_FOUND;=0D
+ } else if (!EFI_ERROR (Status) && DsdtFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ DsdtFound =3D TRUE;=0D
}=0D
=0D
- if (!Dbg2Found) {=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !Dbg2Found) {=0D
DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));=0D
+ } else if (!EFI_ERROR (Status) && Dbg2Found) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: DBG2 Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ Dbg2Found =3D TRUE;=0D
}=0D
=0D
- if (!SpcrFound) {=0D
+ Handle =3D 0;=0D
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,=
=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status) && !SpcrFound) {=0D
DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));=0D
+ } else if (!EFI_ERROR (Status) && SpcrFound) {=0D
+ DEBUG ((DEBUG_ERROR, "ERROR: SPCR Table found while already published.=
\n"));=0D
+ Status =3D EFI_ALREADY_STARTED;=0D
+ } else {=0D
+ SpcrFound =3D TRUE;=0D
}=0D
=0D
return Status;=0D
@@ -500,11 +622,13 @@ ProcessAcpiTables (
IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol=0D
)=0D
{=0D
- EFI_STATUS Status;=0D
- EFI_ACPI_TABLE_PROTOCOL *AcpiTableProtocol;=0D
- CM_STD_OBJ_ACPI_TABLE_INFO *AcpiTableInfo;=0D
- UINT32 AcpiTableCount;=0D
- UINT32 Idx;=0D
+ EFI_STATUS Status;=0D
+ EFI_ACPI_TABLE_PROTOCOL *AcpiTableProtocol;=0D
+ CM_STD_OBJ_ACPI_TABLE_INFO *AcpiTableInfo;=0D
+ UINT32 AcpiTableCount;=0D
+ UINT32 Idx;=0D
+ UINTN Handle;=0D
+ EFI_ACPI_DESCRIPTION_HEADER *DummyHeader;=0D
=0D
ASSERT (TableFactoryProtocol !=3D NULL);=0D
ASSERT (CfgMgrProtocol !=3D NULL);=0D
@@ -570,29 +694,37 @@ 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
+ Status =3D LocateAcpiTableBySignature (=0D
+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,=0D
+ &DummyHeader,=0D
+ &Handle=0D
+ );=0D
+ if (EFI_ERROR (Status)) {=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
-=0D
- break;=0D
- }=0D
- } // for=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.36.0.windows.1


Sami Mujawar
 

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

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

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
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


PierreGondois
 

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@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>
Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
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


Kun Qin
 

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

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

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
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