Hi Pierre,
Thank you for your feedback. I will add more document/comments to the newly define structure, as
well as the "break" as you suggested.
As per failure to locate "gEfiAcpiSdtProtocolGuid", my thought was that this protocol could be enabled
per platform through "gEfiMdeModulePkgTokenSpaceGuid.PcdInstallAcpiSdtProtocol". So I did not treat
the failure as a hard requirement (for the same reason, I did not add it to the module Depex). Please let
me know whether you think this makes sense. Otherwise, I could replace the "goto" logic with a check
for the same PCD and only conduct the routine if ACPI_SDT is expected.
Please also let me know if you have other suggestions.
Thanks,
Kun
toggle quoted message
Show quoted text
On 7/28/2022 6:07 AM, Pierre Gondois wrote: Hello Kun,
With the changes below:
Reviewed-by: Pierre Gondois <pierre.gondois@...>
Thanks!
On 7/28/22 06:31, Kun Qin via groups.io wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
This change added an extra step to allow check for installed ACPI tables.
For FADT, MADT, GTDT, DSDT, DBG2 and SPCR tables, either pre-installed or
supplied through AcpiTableInfo can be accepted.
An extra check for FADT ACPI table existence during installation step is
also added.
Cc: Sami Mujawar <Sami.Mujawar@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>
Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
Notes:
v2:
- Function description updates [Sami]
- Refactorized the table verification [Pierre]
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 182 +++++++++++---------
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
2 files changed, 103 insertions(+), 80 deletions(-)
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..4ad7c0c8dbfa 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -10,6 +10,7 @@
#include <Library/DebugLib.h>
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/AcpiSystemDescriptionTable.h>
#include <Protocol/AcpiTable.h>
// Module specific include files.
@@ -22,6 +23,29 @@
#include <Protocol/DynamicTableFactoryProtocol.h>
#include <SmbiosTableGenerator.h>
+#define ACPI_TABLE_PRESENT_INFO_LIST BIT0
+#define ACPI_TABLE_PRESENT_INSTALLED BIT1
+
+#define ACPI_TABLE_VERIFY_FADT 0
+#define ACPI_TABLE_VERIFY_COUNT 6
+
+typedef struct {
+ ESTD_ACPI_TABLE_ID EstdTableId;
+ UINT32 AcpiTableSignature;
+ CHAR8 AcpiTableName[sizeof (UINT32) + 1];
+ BOOLEAN IsMandatory;
+ UINT16 Presence;
+} ACPI_TABLE_PRESENCE_INFO; I think it needs some documentation (also for mAcpiVerifyTables).
+
+ACPI_TABLE_PRESENCE_INFO mAcpiVerifyTables[ACPI_TABLE_VERIFY_COUNT] = {
+ { EStdAcpiTableIdFadt, EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, "FADT", TRUE, 0 },
+ { EStdAcpiTableIdMadt, EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, "MADT", TRUE, 0 },
+ { EStdAcpiTableIdGtdt, EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, "GTDT", TRUE, 0 },
+ { EStdAcpiTableIdDsdt, EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, "DSDT", TRUE, 0 },
+ { EStdAcpiTableIdDbg2, EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, "DBG2", FALSE, 0 },
+ { EStdAcpiTableIdSpcr, EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, "SPCR", FALSE, 0 },
+};
+
/** This macro expands to a function that retrieves the ACPI Table
List from the Configuration Manager.
*/
@@ -395,6 +419,7 @@ BuildAndInstallAcpiTable (
@retval EFI_SUCCESS Success.
@retval EFI_NOT_FOUND If mandatory table is not found.
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
**/
STATIC
EFI_STATUS
@@ -404,75 +429,68 @@ VerifyMandatoryTablesArePresent (
IN UINT32 AcpiTableCount
)
{
- EFI_STATUS Status;
- BOOLEAN FadtFound;
- BOOLEAN MadtFound;
- BOOLEAN GtdtFound;
- BOOLEAN DsdtFound;
- BOOLEAN Dbg2Found;
- BOOLEAN SpcrFound;
+ EFI_STATUS Status;
+ UINTN Handle;
+ UINTN Index;
+ UINTN InstalledTableIndex;
+ EFI_ACPI_DESCRIPTION_HEADER *DescHeader;
+ EFI_ACPI_TABLE_VERSION Version;
+ EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
- Status = EFI_SUCCESS;
- FadtFound = FALSE;
- MadtFound = FALSE;
- GtdtFound = FALSE;
- DsdtFound = FALSE;
- Dbg2Found = FALSE;
- SpcrFound = FALSE;
ASSERT (AcpiTableInfo != NULL);
+ Status = EFI_SUCCESS;
+
+ // Check against the statically initialized ACPI tables to see if they are in ACPI info list
while (AcpiTableCount-- != 0) {
- switch (AcpiTableInfo[AcpiTableCount].AcpiTableSignature) {
- case EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE:
- FadtFound = TRUE;
- break;
- case EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE:
- MadtFound = TRUE;
- break;
- case EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE:
- GtdtFound = TRUE;
- break;
- case EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE:
- DsdtFound = TRUE;
- break;
- case EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE:
- Dbg2Found = TRUE;
- break;
- case EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE:
- SpcrFound = TRUE;
- break;
- default:
- break;
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ if (AcpiTableInfo[AcpiTableCount].AcpiTableSignature == mAcpiVerifyTables[Index].AcpiTableSignature) {
+ mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INFO_LIST; Would it be possible to add a 'break' here ?
Just a note for Sami:
These double loops seem expensive, but I cannot find anything better and/or shorter.
+ }
}
}
- // We need at least the FADT, MADT, GTDT and the DSDT tables to boot
- if (!FadtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
- Status = EFI_NOT_FOUND;
- }
+ // They also might be published already, so we can search from there
+ AcpiSdt = NULL;
+ Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
- if (!MadtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
- Status = EFI_NOT_FOUND;
+ if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
+ DEBUG ((DEBUG_WARN, "WARNING: Failed to locate ACPI SDT protocol (0x%p) - %r\n", AcpiSdt, Status));
+ goto EvaluatePresence; I think this is ok to just print and return an error, unless you think about this could happen.
}
- if (!GtdtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
- Status = EFI_NOT_FOUND;
- }
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ Handle = 0;
+ InstalledTableIndex = 0;
+ do {
+ Status = AcpiSdt->GetAcpiTable (InstalledTableIndex, (EFI_ACPI_SDT_HEADER **)&DescHeader, &Version, &Handle);
+ if (EFI_ERROR (Status)) {
+ break;
+ }
- if (!DsdtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
- Status = EFI_NOT_FOUND;
- }
+ InstalledTableIndex++;
+ } while (DescHeader->Signature != mAcpiVerifyTables[Index].AcpiTableSignature);
- if (!Dbg2Found) {
- DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
+ if (!EFI_ERROR (Status)) {
+ mAcpiVerifyTables[Index].Presence |= ACPI_TABLE_PRESENT_INSTALLED;
+ }
}
- if (!SpcrFound) {
- DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
+EvaluatePresence:
+ for (Index = 0; Index < ACPI_TABLE_VERIFY_COUNT; Index++) {
+ if (mAcpiVerifyTables[Index].Presence == 0) {
+ if (mAcpiVerifyTables[Index].IsMandatory) {
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ Status = EFI_NOT_FOUND;
+ } else {
+ DEBUG ((DEBUG_WARN, "WARNING: %a Table not found.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ }
+ } else if (mAcpiVerifyTables[Index].Presence ==
+ (ACPI_TABLE_PRESENT_INFO_LIST | ACPI_TABLE_PRESENT_INSTALLED))
+ {
+ DEBUG ((DEBUG_ERROR, "ERROR: %a Table found while already published.\n", mAcpiVerifyTables[Index].AcpiTableName));
+ Status = EFI_ALREADY_STARTED;
+ }
} Just a note for Sami:
In the loop above we return the last invalid code, this should be ok.
return Status;
@@ -489,8 +507,9 @@ VerifyMandatoryTablesArePresent (
@param [in] CfgMgrProtocol Pointer to the Configuration Manager
Protocol Interface.
- @retval EFI_SUCCESS Success.
- @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+ @retval EFI_SUCCESS Success.
+ @retval EFI_NOT_FOUND If a mandatory table or a generator is not found.
+ @retval EFI_ALREADY_STARTED If mandatory table found in AcpiTableInfo is already installed.
**/
STATIC
EFI_STATUS
@@ -562,7 +581,7 @@ ProcessAcpiTables (
if (EFI_ERROR (Status)) {
DEBUG ((
DEBUG_ERROR,
- "ERROR: Failed to find mandatory ACPI Table(s)."
+ "ERROR: Failed to verify mandatory ACPI Table(s) presence."
" Status = %r\n",
Status
));
@@ -570,29 +589,32 @@ ProcessAcpiTables (
}
// Add the FADT Table first.
- for (Idx = 0; Idx < AcpiTableCount; Idx++) {
- if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
- AcpiTableInfo[Idx].TableGeneratorId)
- {
- Status = BuildAndInstallAcpiTable (
- TableFactoryProtocol,
- CfgMgrProtocol,
- AcpiTableProtocol,
- &AcpiTableInfo[Idx]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((
- DEBUG_ERROR,
- "ERROR: Failed to find build and install ACPI FADT Table." \
- " Status = %r\n",
- Status
- ));
- return Status;
- }
+ if ((mAcpiVerifyTables[ACPI_TABLE_VERIFY_FADT].Presence & ACPI_TABLE_PRESENT_INSTALLED) == 0) {
+ // FADT is not yet installed
+ for (Idx = 0; Idx < AcpiTableCount; Idx++) {
+ if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
+ AcpiTableInfo[Idx].TableGeneratorId)
+ {
+ Status = BuildAndInstallAcpiTable (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ AcpiTableProtocol,
+ &AcpiTableInfo[Idx]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find build and install ACPI FADT Table." \
+ " Status = %r\n",
+ Status
+ ));
+ return Status;
+ }
- break;
- }
- } // for
+ break;
+ }
+ } // for
+ }
// Add remaining ACPI Tables
for (Idx = 0; Idx < AcpiTableCount; Idx++) {
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
index 028c3d413cf8..5ca98c8b4895 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -36,6 +36,7 @@ [LibraryClasses]
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+ gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
|
