From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pg1-f181.google.com (mail-pg1-f181.google.com [209.85.215.181]) by mx.groups.io with SMTP id smtpd.web09.4649.1659070830031848896 for ; Thu, 28 Jul 2022 22:00:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=ZSfLwr0n; spf=pass (domain: gmail.com, ip: 209.85.215.181, mailfrom: kuqin12@gmail.com) Received: by mail-pg1-f181.google.com with SMTP id s206so3176384pgs.3 for ; Thu, 28 Jul 2022 22:00:29 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :from:to:cc; bh=vLmfsmHp92bgvDNSGjKb1p2NPPkln5EaU1Y848Pzwm0=; b=ZSfLwr0nGs7jutsbTE9NI6BafUIjBZ4JxeUmblOg4OI014++v3SaTvEIl4BIRw/fj1 XuJ64caKiECRupGbI3LY52waV9MDzgbCK4laFruHEZjr2Lrexith7T9GP7n0BKCLNo5w KdSkTtSkgRpoDALNJ+FF1HzgdKOQX/azVf1lZKhkKfInfBEEoZuZVWmknR2mY6jb+h/U 4XSlN6rZlS+y8l9HAk1MyBhOnlsBZAE6ot/KuMwPOKqvJF1aJQ6BeJAt2Y0NS+tUW9a1 ZQSUUVOfrwvvR173Nxw2mRup021O/Qp7BdZ3av/tSv2ficVt4xVND6dMXz+QLXUM5D/U jTQA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=vLmfsmHp92bgvDNSGjKb1p2NPPkln5EaU1Y848Pzwm0=; b=BnTPIVmgh/sX8olsI0yP35vulz2Zw8jPVF5W1VGrEgKYS49VioSIiyYCMkwYtbKQPQ ID2Y5PSwvWUgBlQYjunUs2oZkB2Ltco/qtH/R3s6+GfQH4Xc1wg9ScevaIDM15O94jYl CLG68SQ1dAcMrmBlZP48DjGBxu0E6xL9gSzR3fUmcifpXiNqj1D1iNsspAN6wWf9f9Nb ZNUM/SQ3I7DuJW+I3m6AkX2if19NgDshzaj1ozR83221DEMSQ70bITxiY8d6KjsgLvMw iHF3D6dKNCYIKdKYk1btVfTvfCFZzZ34WXZnAvuZ/aFzDMXOTQKzFNSYoipVPdglk/kq Kp2w== X-Gm-Message-State: AJIora/8sDb7EFyXdM7BVsay1CXoaugh3+i9vqUVHGQmDp0oMvAKFA3i MKlgSqlNarrLrZQUjzg4AhU= X-Google-Smtp-Source: AGRyM1uXa0EUR38FlnrYrqRvOvyFdZh1sHdRWHpuTFHaROno+jysACgRLGIpm9E1lprLTyJgv9wDZA== X-Received: by 2002:a65:6885:0:b0:412:a2ed:c3b with SMTP id e5-20020a656885000000b00412a2ed0c3bmr1602572pgt.606.1659070829236; Thu, 28 Jul 2022 22:00:29 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:c8a3:f4a8:3813:162d? ([2001:4898:80e8:8:48be:f4a8:3813:162d]) by smtp.gmail.com with ESMTPSA id w16-20020aa79a10000000b0052536c695c0sm1718797pfj.170.2022.07.28.22.00.28 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 28 Jul 2022 22:00:28 -0700 (PDT) Message-ID: <2076e6af-3464-d0ad-abc8-3408d251e09a@gmail.com> Date: Thu, 28 Jul 2022 22:00:28 -0700 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 Subject: Re: [edk2-devel] [PATCH v2 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables To: Pierre Gondois , devel@edk2.groups.io Cc: Sami Mujawar , Alexei Fedorov , Joe Lopez References: <20220728043147.395-1-kuqin12@gmail.com> <20220728043147.395-5-kuqin12@gmail.com> <1102c58a-1424-4e5f-476b-3866137382a3@arm.com> From: "Kun Qin" In-Reply-To: <1102c58a-1424-4e5f-476b-3866137382a3@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 > > 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 >> Cc: Alexei Fedorov >> >> Co-authored-by: Joe Lopez >> Signed-off-by: Kun Qin >> --- >> >> 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 >>   #include >>   #include >> +#include >>   #include >>     // Module specific include files. >> @@ -22,6 +23,29 @@ >>   #include >>   #include >>   +#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