Thank you for your reviews, Sami! I will incorporate your suggestions in v2 patch and send for review after validation.
Regards,
Kun
toggle quoted message
Show quoted text
On 7/20/2022 4:00 AM, Sami Mujawar wrote: Hi Kun,
Thank you for this patch.
I have some minor suggestions marked inline as [SAMI], otherwise this patch looks good to me.
With that updated.
Reviewed-by: Sami Mujawar <sami.mujawar@...>
Regards,
Sami Mujawar
On 19/07/2022 01:22 am, Kun Qin wrote:
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3997
This change added an extra step to allow check for installed ACPI tables.
For FADT, MADT, GTDT, DSDT and DBG2 tables, either pre-installed or
supplied through AcpiTableInfo can be accepted.
An extra check for FADT ACPI table existence during installation step is
also added.
Cc: Sami Mujawar <Sami.Mujawar@...>
Cc: Alexei Fedorov <Alexei.Fedorov@...>
Co-authored-by: Joe Lopez <joelopez@...>
Signed-off-by: Kun Qin <kuqin12@...>
---
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c | 200 ++++++++++++++++----
DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf | 1 +
2 files changed, 167 insertions(+), 34 deletions(-)
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
index ed62299f9bbd..ac5fe0bed91b 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c
@@ -11,6 +11,7 @@
#include <Library/PcdLib.h>
#include <Library/UefiBootServicesTableLib.h>
#include <Protocol/AcpiTable.h>
+#include <Protocol/AcpiSystemDescriptionTable.h> [SAMI] Can thie include statement above be alphabetically ordered, please?
// Module specific include files.
#include <AcpiTableGenerator.h>
@@ -387,6 +388,57 @@ BuildAndInstallAcpiTable (
return Status;
}
+/**
+ This function uses the ACPI SDT protocol to locate an ACPI table.
+ It is really only useful for finding tables that only have a single instance,
+ e.g. FADT, FACS, MADT, etc. It is not good for locating SSDT, etc.
+
+ @param[in] Signature - Pointer to an ASCII string containing the OEM Table ID from the ACPI table header
+ @param[in, out] Table - Updated with a pointer to the table
+ @param[in, out] Handle - AcpiSupport protocol table handle for the table found
+
+ @retval EFI_SUCCESS - The function completed successfully.
[SAMI] Please add EFI_NOT_FOUND as a return type if an ACPI table with the requested signature is not found or if the ACPI SDT protocol is not installed.
+**/
+STATIC
+EFI_STATUS
+LocateAcpiTableBySignature (
+ IN UINT32 Signature,
+ IN OUT EFI_ACPI_DESCRIPTION_HEADER **Table,
+ IN OUT UINTN *Handle
+ )
+{
+ EFI_STATUS Status;
+ INTN Index;
+ EFI_ACPI_TABLE_VERSION Version;
+ EFI_ACPI_SDT_PROTOCOL *AcpiSdt;
+
+ AcpiSdt = NULL;
+ Status = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, (VOID **)&AcpiSdt);
+
+ if (EFI_ERROR (Status) || (AcpiSdt == NULL)) {
+ return EFI_NOT_FOUND;
+ }
+
+ //
+ // Locate table with matching ID
+ //
+ Version = 0;
+ Index = 0;
+ do {
+ Status = AcpiSdt->GetAcpiTable (Index, (EFI_ACPI_SDT_HEADER **)Table, &Version, Handle);
+ if (EFI_ERROR (Status)) {
+ break;
+ }
+
+ Index++;
+ } while ((*Table)->Signature != Signature);
+
+ //
+ // If we found the table, there will be no error.
+ //
+ return Status;
+}
+
/** The function checks if the Configuration Manager has provided the
mandatory ACPI tables for installation.
@@ -411,6 +463,9 @@ VerifyMandatoryTablesArePresent (
BOOLEAN DsdtFound;
BOOLEAN Dbg2Found;
BOOLEAN SpcrFound;
+ UINTN Handle;
+
+ EFI_ACPI_DESCRIPTION_HEADER *DummyHeader;
Status = EFI_SUCCESS;
FadtFound = FALSE;
@@ -447,32 +502,99 @@ VerifyMandatoryTablesArePresent (
}
// We need at least the FADT, MADT, GTDT and the DSDT tables to boot
- if (!FadtFound) {
- DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found\n"));
+ // But they also might be published already, so we can search from there
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !FadtFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found.\n"));
Status = EFI_NOT_FOUND;
+ } else if (!EFI_ERROR (Status) && FadtFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: FADT Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
[SAMI] Please update the function documentation header to reflect the EFI_ALREADY_STARTED error code.
+ } else {
+ FadtFound = TRUE;
}
- if (!MadtFound) {
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !MadtFound) {
DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n"));
Status = EFI_NOT_FOUND;
+ } else if (!EFI_ERROR (Status) && MadtFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: MADT Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
+ } else {
+ MadtFound = TRUE;
}
- if (!GtdtFound) {
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !GtdtFound) {
DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n"));
Status = EFI_NOT_FOUND;
+ } else if (!EFI_ERROR (Status) && GtdtFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
+ } else {
+ GtdtFound = TRUE;
}
- if (!DsdtFound) {
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !DsdtFound) {
DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n"));
Status = EFI_NOT_FOUND;
+ } else if (!EFI_ERROR (Status) && DsdtFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
+ } else {
+ DsdtFound = TRUE;
}
- if (!Dbg2Found) {
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !Dbg2Found) {
DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n"));
+ } else if (!EFI_ERROR (Status) && Dbg2Found) {
+ DEBUG ((DEBUG_ERROR, "ERROR: DBG2 Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
+ } else {
+ Dbg2Found = TRUE;
}
- if (!SpcrFound) {
+ Handle = 0;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status) && !SpcrFound) {
DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n"));
+ } else if (!EFI_ERROR (Status) && SpcrFound) {
+ DEBUG ((DEBUG_ERROR, "ERROR: SPCR Table found while already published.\n"));
+ Status = EFI_ALREADY_STARTED;
+ } else {
+ SpcrFound = TRUE;
}
return Status;
@@ -500,11 +622,13 @@ ProcessAcpiTables (
IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL *CONST CfgMgrProtocol
)
{
- EFI_STATUS Status;
- EFI_ACPI_TABLE_PROTOCOL *AcpiTableProtocol;
- CM_STD_OBJ_ACPI_TABLE_INFO *AcpiTableInfo;
- UINT32 AcpiTableCount;
- UINT32 Idx;
+ EFI_STATUS Status;
+ EFI_ACPI_TABLE_PROTOCOL *AcpiTableProtocol;
+ CM_STD_OBJ_ACPI_TABLE_INFO *AcpiTableInfo;
+ UINT32 AcpiTableCount;
+ UINT32 Idx;
+ UINTN Handle;
+ EFI_ACPI_DESCRIPTION_HEADER *DummyHeader;
ASSERT (TableFactoryProtocol != NULL);
ASSERT (CfgMgrProtocol != NULL);
@@ -570,29 +694,37 @@ ProcessAcpiTables (
}
[snip] // Check if mandatory ACPI tables are present.
Status = VerifyMandatoryTablesArePresent (
AcpiTableInfo,
AcpiTableCount
);
if (EFI_ERROR (Status)) {
DEBUG ((
DEBUG_ERROR,
"ERROR: Failed to find mandatory ACPI Table(s)."
" Status = %r\n",
Status
));
[SAMI] Is it possible to update the error reporting to reflect the EFI_ALREADY_STARTED error type, please? Please also update the function documentation header for ProcessAcpiTables().
return Status;
}
[/snip]
// Add the FADT Table first.
- for (Idx = 0; Idx < AcpiTableCount; Idx++) {
- if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
- AcpiTableInfo[Idx].TableGeneratorId)
- {
- Status = BuildAndInstallAcpiTable (
- TableFactoryProtocol,
- CfgMgrProtocol,
- AcpiTableProtocol,
- &AcpiTableInfo[Idx]
- );
- if (EFI_ERROR (Status)) {
- DEBUG ((
- DEBUG_ERROR,
- "ERROR: Failed to find build and install ACPI FADT Table." \
- " Status = %r\n",
- Status
- ));
- return Status;
+ Status = LocateAcpiTableBySignature (
+ EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
+ &DummyHeader,
+ &Handle
+ );
+ if (EFI_ERROR (Status)) {
+ // FADT is not yet installed
+ for (Idx = 0; Idx < AcpiTableCount; Idx++) {
+ if (CREATE_STD_ACPI_TABLE_GEN_ID (EStdAcpiTableIdFadt) ==
+ AcpiTableInfo[Idx].TableGeneratorId)
+ {
+ Status = BuildAndInstallAcpiTable (
+ TableFactoryProtocol,
+ CfgMgrProtocol,
+ AcpiTableProtocol,
+ &AcpiTableInfo[Idx]
+ );
+ if (EFI_ERROR (Status)) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "ERROR: Failed to find build and install ACPI FADT Table." \
+ " Status = %r\n",
+ Status
+ ));
+ return Status;
+ }
+
+ break;
}
-
- break;
- }
- } // for
+ } // for
+ }
// Add remaining ACPI Tables
for (Idx = 0; Idx < AcpiTableCount; Idx++) {
diff --git a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
index 028c3d413cf8..5ca98c8b4895 100644
---
a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
+++ b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf
@@ -36,6 +36,7 @@ [LibraryClasses]
[Protocols]
gEfiAcpiTableProtocolGuid # PROTOCOL ALWAYS_CONSUMED
+ gEfiAcpiSdtProtocolGuid # PROTOCOL ALWAYS_CONSUMED
[SAMI] Should the DEPEX section be updated to relect the dependency on the SDT protocol?
gEdkiiConfigurationManagerProtocolGuid # PROTOCOL ALWAYS_CONSUMED
gEdkiiDynamicTableFactoryProtocolGuid # PROTOCOL ALWAYS_CONSUMED
|
