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

["PierreGondois" <pierre.gondois@arm.com>]

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@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@arm.com>
>>> Cc: Alexei Fedorov <Alexei.Fedorov@arm.com>
>>>
>>> Co-authored-by: Joe Lopez <joelopez@microsoft.com>
>>> Signed-off-by: Kun Qin <kuqin12@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