[edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
Kun Qin
kuqin12 at gmail.com
Fri Jul 29 05:00:28 UTC 2022
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
On 7/28/2022 6:07 AM, Pierre Gondois wrote:
> Hello Kun,
> With the changes below:
> Reviewed-by: Pierre Gondois <pierre.gondois at arm.com>
>
> 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 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>
>> ---
>>
>> 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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#91962): https://edk2.groups.io/g/devel/message/91962
Mute This Topic: https://groups.io/mt/92664648/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