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@arm.com>
Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
Co-authored-by: Joe Lopez <joelopez@microsoft.com>
Signed-off-by: Kun Qin <kuqin12@gmail.com>
Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
---
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) {
+ // 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
--
2.37.1.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91999): https://edk2.groups.io/g/devel/message/91999
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-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@arm.com>
> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>
> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> ---
>
> 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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92200): https://edk2.groups.io/g/devel/message/92200
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <Sami.Mujawar@arm.com>
>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>
>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>> ---
>>
>> 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)) {
[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]
>>
>> + 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
>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92205): https://edk2.groups.io/g/devel/message/92205
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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 <Sami.Mujawar@arm.com>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>
>>> 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)) {
>
> [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]
>
>>>
>>> + 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]
>>
>>>
>>> + // 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
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92225): https://edk2.groups.io/g/devel/message/92225
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Kun,
Please find my response inline marked [SAMI].
Regards,
Sami Mujawar
On 09/08/2022, 00:44, "Kun Qin" <kuqin12@gmail.com> 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 <Sami.Mujawar@arm.com>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
>>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
>>> ---
>>>
>>> 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)) {
>
> [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
>>>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92242): https://edk2.groups.io/g/devel/message/92242
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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" <kuqin12@gmail.com> 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 <Sami.Mujawar@arm.com>
> >>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
> >>>
> >>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
> >>> Signed-off-by: Kun Qin <kuqin12@gmail.com>
> >>> Reviewed-by: Pierre Gondois <pierre.gondois@arm.com>
> >>> ---
> >>>
> >>> 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)) {
> >
> > [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
> >>>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92319): https://edk2.groups.io/g/devel/message/92319
Mute This Topic: https://groups.io/mt/92722842/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.