[edk2-devel] [PATCH v3 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables

Kun Qin kuqin12 at gmail.com
Wed Aug 10 22:31:02 UTC 2022


Hi Sami,

Thanks for your Acks below. The updated patch is sent here:
https://edk2.groups.io/g/devel/message/92317

Please let me know if there is any further feedback/issues.

Regards,
Kun

On 8/9/2022 2:01 AM, Sami Mujawar wrote:
> Hi Kun,
>
> Please find my response inline marked [SAMI].
>
> Regards,
>
> Sami Mujawar
>
> On 09/08/2022, 00:44, "Kun Qin" <kuqin12 at gmail.com> wrote:
>
>      Hi Sami,
>
>      Thank you for taking time testing this change!
>
>      I have a question about one comment you have for this specific patch
>      inline (marked with [KQ]).
>      Could you please provide more details?
>
>      I also responded to your other comments, please let me know if the
>      proposed change makes
>      sense to you. Looking forward to your reply.
>
>      Thanks,
>      Kun
>
>      On 8/8/2022 8:39 AM, Sami Mujawar wrote:
>      > Hi Kun,
>      >
>      > Please find my response inline marked [SAMI].
>      >
>      > Regards,
>      >
>      > Sami Mujawar
>      >
>      > On 08/08/2022 02:05 pm, Sami Mujawar wrote:
>      >> 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)) {
>      >
>      > [SAMI] When running Kvmtool guest firmware I break from here with
>      > EFI_NOT_FOUND. The problem is PcdInstallAcpiSdtProtocol is set to TRUE
>      > in ArmVirt.dsc.inc and the Kvmtool guest firmware does not have any
>      > preinstalled tables (i.e. all tables are generated using
>      > DynamicTablesFramework).
>      >
>      > This means platforms that use only Dynamic Tables Framework would all
>      > need to define PcdInstallAcpiSdtProtocol to FALSE. Is it possible to
>      > rework this logic so that the existing platform code does not need
>      > updating, please?
>      >
>      > [/SAMI]
>
>      [KQ]
>
>      There was a mistake that the Status should be set to EFI_SUCCESS before
>      entering the presence
>      checking step. Otherwise it will remain the same value from GetAcpiTable
>      if all tables are present.
>
>      To your original observation, it is correct that this GetAcpiTable call
>      will fail with EFI_NOT_FOUND,
>      but it should only break from the internal `do-while` loop, and continue
>      with the rest of table
>      look-ups. Then during table inspection step, either installed table or
>      from info_list will count as a
>      passed check. The return Status should be reset before inspecting the
>      full look-up results. Thanks
>      for catching this! Will add the `Status = EFI_SUCCESS` statement in the
>      next round.
>
>      [/KQ]
>
> [SAMI] Ack. I look forward to an updated patch with this fixed.
>
>      >
>      >>>
>      >>> +          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]
>
>      [KQ]
>
>      You are correct that there will be only above 2 conditions for mandatory
>      tables.
>      But the check is to make sure the FADT will only be installed on
>      condition #1 above,
>      which means it will only be installed if it has not already been
>      installed. Please let
>      me know if I missed anything here.
>
>      [/KQ]
> [SAMI] Ack. Please keep this check. If the table is already installed this check prevents searching in the AcpiTableInfo list and therefore optimises.
>      >>
>      >>>
>      >>> +    // 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 (#92319): https://edk2.groups.io/g/devel/message/92319
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