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


Sami Mujawar
 

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

Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
Reviewed-by: Pierre Gondois <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 <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,58 @@
#include <Protocol/DynamicTableFactoryProtocol.h>

#include <SmbiosTableGenerator.h>


+///

+/// 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)) {

+ 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]


+ // 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

Join devel@edk2.groups.io to automatically receive all group messages.