[edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables
PierreGondois
pierre.gondois at arm.com
Fri Jul 29 15:11:05 UTC 2022
On 7/29/22 07:00, Kun Qin wrote:
> 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.
>
Ok yes, this is a good idea,
Regards,
Pierre
> 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 (#91973): https://edk2.groups.io/g/devel/message/91973
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