[edk2-devel] [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
Sami Mujawar
sami.mujawar at arm.com
Mon Aug 8 13:05:40 UTC 2022
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 at arm.com>
> Cc: Alexei Fedorov <Alexei.Fedorov at arm.com>
>
> Co-authored-by: Joe Lopez <joelopez at microsoft.com>
> Signed-off-by: Kun Qin <kuqin12 at gmail.com>
> Reviewed-by: Pierre Gondois <pierre.gondois at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list