From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-pj1-f42.google.com (mail-pj1-f42.google.com [209.85.216.42]) by mx.groups.io with SMTP id smtpd.web10.1188.1658368096558353521 for ; Wed, 20 Jul 2022 18:48:16 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@gmail.com header.s=20210112 header.b=bsu2tTNw; spf=pass (domain: gmail.com, ip: 209.85.216.42, mailfrom: kuqin12@gmail.com) Received: by mail-pj1-f42.google.com with SMTP id l14-20020a17090a72ce00b001f20ed3c55dso100651pjk.5 for ; Wed, 20 Jul 2022 18:48:16 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=message-id:date:mime-version:user-agent:subject:content-language:to :cc:references:from:in-reply-to:content-transfer-encoding; bh=wYR3jiS6qFuvgkqCaorAy9ifWjKUGdpmlssRKocEO4Y=; b=bsu2tTNw30g6DzxzkHJ3Fv+1bucnl6D7gLSNQIqpBk/79VMzFPcDs6eXFzt3/rhRrj NoFw8d0yGjEkdpFEah4LDYXp8g3GaLkFPpwa0fh17eATDaDt/8mqxIWMn19rzB9lrj+N R1bRGcZ+pPATmLIvYQOrouoJKolhOithFReM6mdnIfcwbTyN+yKm+pMuZn2h9BWU7FAm M4SJj8cSNeaxUR6qBb9HyS8x8vMoHBp/OgoZHJhsfSNb+Coi3qW5x7VhUTz4qXXNBTrR vt7aoZAbhCbYlX6EjYXGCXc9UI9lv0Orsg7yTVa3elSTYZYs3eFcuro/oMwy4v7PMUUE v1Qw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:message-id:date:mime-version:user-agent:subject :content-language:to:cc:references:from:in-reply-to :content-transfer-encoding; bh=wYR3jiS6qFuvgkqCaorAy9ifWjKUGdpmlssRKocEO4Y=; b=pF48O39d44LhGwdOGJKSJa2zX0U4BArl6QSvKcO51XFmeySCfyjaVLyY2ZdzpwMpJC Xb4q4bfGm1Q+P+pK56sZmcgM+63dm+oN8jzUZevmLXtsZIoUhGRK4VOJxZSr8pCnCOYW wSXWKzE/eJmZPg1t4TyNQ2EV/eLEFKTe7IMkKdXpzIKQi3obBwq+JfB0M4XZtY2rHtsp Lj7gTe3o2RQciWkl/Qq5fWGTx8C4BY0ULifE+Eg+hWSckNfjKcHuAxHKHHoqWwtg4eUw YUyzJNnCjL5xZ9RV1Q8jZBadTUYhfxI2gMxzqz2HlRVJu7BOQbqszUx1NCmeDMjRFHOn 3kBA== X-Gm-Message-State: AJIora+wOqkXtNyeKyFnwln3d3l+TNcIRCFfq1HNe3/9n6V1qHINN/bZ IKIPsOIwvb5eUT8FnMw/xfA= X-Google-Smtp-Source: AGRyM1tsOj7EGeoQ00eVudOdGEsa/gx/R6yhWebUQtsnVcsyuYtPE6wZxnGXq3BQuPdiRY3f1vL/8g== X-Received: by 2002:a17:90b:1d92:b0:1ef:e28f:ff38 with SMTP id pf18-20020a17090b1d9200b001efe28fff38mr8816828pjb.32.1658368095975; Wed, 20 Jul 2022 18:48:15 -0700 (PDT) Return-Path: Received: from ?IPV6:2001:4898:d8:33:875:145e:4dd2:e6e6? ([2001:4898:80e8:9:888e:145e:4dd2:e6e6]) by smtp.gmail.com with ESMTPSA id e30-20020aa798de000000b0052ab8a92496sm288928pfm.168.2022.07.20.18.48.15 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 20 Jul 2022 18:48:15 -0700 (PDT) Message-ID: <4c4d824c-a432-78ee-41e3-d2a9a84b5c94@gmail.com> Date: Wed, 20 Jul 2022 18:48:15 -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: [PATCH v1 4/6] DynamicTablesPkg: DynamicTableManagerDxe: Added check for installed tables To: Sami Mujawar , devel@edk2.groups.io Cc: Alexei Fedorov , Joe Lopez , nd@arm.com References: <20220719002254.1891-1-kuqin12@gmail.com> <20220719002254.1891-5-kuqin12@gmail.com> <890206a7-fec3-dc98-3ea4-8791c50689f4@arm.com> From: "Kun Qin" In-Reply-To: <890206a7-fec3-dc98-3ea4-8791c50689f4@arm.com> Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit Thank you for your reviews, Sami! I will incorporate your suggestions in v2 patch and send for review after validation. Regards, Kun On 7/20/2022 4:00 AM, Sami Mujawar wrote: > Hi Kun, > > Thank you for this patch. > > I have some minor suggestions marked inline as [SAMI], otherwise this > patch looks good to me. > > With that updated. > > Reviewed-by: Sami Mujawar > > Regards, > > Sami Mujawar > > On 19/07/2022 01:22 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 and DBG2 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 >> --- >> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c >> | 200 ++++++++++++++++---- >> DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.inf >> |   1 + >>   2 files changed, 167 insertions(+), 34 deletions(-) >> >> diff --git >> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c >> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c >> >> index ed62299f9bbd..ac5fe0bed91b 100644 >> --- >> a/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c >> +++ >> b/DynamicTablesPkg/Drivers/DynamicTableManagerDxe/DynamicTableManagerDxe.c >> @@ -11,6 +11,7 @@ >>   #include >> >>   #include >> >>   #include >> >> +#include > [SAMI] Can thie include statement above be alphabetically ordered, > please? >> >> >>   // Module specific include files. >> >>   #include >> >> @@ -387,6 +388,57 @@ BuildAndInstallAcpiTable ( >>     return Status; >> >>   } >> >> >> +/** >> >> +  This function uses the ACPI SDT protocol to locate an ACPI table. >> >> +  It is really only useful for finding tables that only have a >> single instance, >> >> +  e.g. FADT, FACS, MADT, etc.  It is not good for locating SSDT, etc. >> >> + >> >> +  @param[in] Signature           - Pointer to an ASCII string >> containing the OEM Table ID from the ACPI table header >> >> +  @param[in, out] Table          - Updated with a pointer to the table >> >> +  @param[in, out] Handle         - AcpiSupport protocol table handle >> for the table found >> >> + >> >> +  @retval EFI_SUCCESS            - The function completed successfully. > > [SAMI] Please add EFI_NOT_FOUND as a return type if an ACPI table with > the requested signature is not found or if the ACPI SDT protocol is > not installed. > >> >> +**/ >> >> +STATIC >> >> +EFI_STATUS >> >> +LocateAcpiTableBySignature ( >> >> +  IN      UINT32                       Signature, >> >> +  IN OUT  EFI_ACPI_DESCRIPTION_HEADER  **Table, >> >> +  IN OUT  UINTN                        *Handle >> >> +  ) >> >> +{ >> >> +  EFI_STATUS              Status; >> >> +  INTN                    Index; >> >> +  EFI_ACPI_TABLE_VERSION  Version; >> >> +  EFI_ACPI_SDT_PROTOCOL   *AcpiSdt; >> >> + >> >> +  AcpiSdt = NULL; >> >> +  Status  = gBS->LocateProtocol (&gEfiAcpiSdtProtocolGuid, NULL, >> (VOID **)&AcpiSdt); >> >> + >> >> +  if (EFI_ERROR (Status) || (AcpiSdt == NULL)) { >> >> +    return EFI_NOT_FOUND; >> >> +  } >> >> + >> >> +  // >> >> +  // Locate table with matching ID >> >> +  // >> >> +  Version = 0; >> >> +  Index   = 0; >> >> +  do { >> >> +    Status = AcpiSdt->GetAcpiTable (Index, (EFI_ACPI_SDT_HEADER >> **)Table, &Version, Handle); >> >> +    if (EFI_ERROR (Status)) { >> >> +      break; >> >> +    } >> >> + >> >> +    Index++; >> >> +  } while ((*Table)->Signature != Signature); >> >> + >> >> +  // >> >> +  // If we found the table, there will be no error. >> >> +  // >> >> +  return Status; >> >> +} >> >> + >> >>   /** The function checks if the Configuration Manager has provided the >> >>       mandatory ACPI tables for installation. >> >> >> @@ -411,6 +463,9 @@ VerifyMandatoryTablesArePresent ( >>     BOOLEAN     DsdtFound; >> >>     BOOLEAN     Dbg2Found; >> >>     BOOLEAN     SpcrFound; >> >> +  UINTN       Handle; >> >> + >> >> +  EFI_ACPI_DESCRIPTION_HEADER  *DummyHeader; >> >> >>     Status    = EFI_SUCCESS; >> >>     FadtFound = FALSE; >> >> @@ -447,32 +502,99 @@ VerifyMandatoryTablesArePresent ( >>     } >> >> >>     // 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")); >> >> +  // But they also might be published already, so we can search from >> there >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !FadtFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table not found.\n")); >> >>       Status = EFI_NOT_FOUND; >> >> +  } else if (!EFI_ERROR (Status) && FadtFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: FADT Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; > [SAMI] Please update the function documentation header to reflect the > EFI_ALREADY_STARTED error code. >> >> +  } else { >> >> +    FadtFound = TRUE; >> >>     } >> >> >> -  if (!MadtFound) { >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_MULTIPLE_APIC_DESCRIPTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !MadtFound) { >> >>       DEBUG ((DEBUG_ERROR, "ERROR: MADT Table not found.\n")); >> >>       Status = EFI_NOT_FOUND; >> >> +  } else if (!EFI_ERROR (Status) && MadtFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: MADT Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; >> >> +  } else { >> >> +    MadtFound = TRUE; >> >>     } >> >> >> -  if (!GtdtFound) { >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_GENERIC_TIMER_DESCRIPTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !GtdtFound) { >> >>       DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table not found.\n")); >> >>       Status = EFI_NOT_FOUND; >> >> +  } else if (!EFI_ERROR (Status) && GtdtFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: GTDT Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; >> >> +  } else { >> >> +    GtdtFound = TRUE; >> >>     } >> >> >> -  if (!DsdtFound) { >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !DsdtFound) { >> >>       DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table not found.\n")); >> >>       Status = EFI_NOT_FOUND; >> >> +  } else if (!EFI_ERROR (Status) && DsdtFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: DSDT Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; >> >> +  } else { >> >> +    DsdtFound = TRUE; >> >>     } >> >> >> -  if (!Dbg2Found) { >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> +             EFI_ACPI_6_2_DEBUG_PORT_2_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !Dbg2Found) { >> >>       DEBUG ((DEBUG_WARN, "WARNING: DBG2 Table not found.\n")); >> >> +  } else if (!EFI_ERROR (Status) && Dbg2Found) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: DBG2 Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; >> >> +  } else { >> >> +    Dbg2Found = TRUE; >> >>     } >> >> >> -  if (!SpcrFound) { >> >> +  Handle = 0; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_SERIAL_PORT_CONSOLE_REDIRECTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status) && !SpcrFound) { >> >>       DEBUG ((DEBUG_WARN, "WARNING: SPCR Table not found.\n")); >> >> +  } else if (!EFI_ERROR (Status) && SpcrFound) { >> >> +    DEBUG ((DEBUG_ERROR, "ERROR: SPCR Table found while already >> published.\n")); >> >> +    Status = EFI_ALREADY_STARTED; >> >> +  } else { >> >> +    SpcrFound = TRUE; >> >>     } >> >> >>     return Status; >> >> @@ -500,11 +622,13 @@ ProcessAcpiTables ( >>     IN CONST EDKII_CONFIGURATION_MANAGER_PROTOCOL  *CONST CfgMgrProtocol >> >>     ) >> >>   { >> >> -  EFI_STATUS                  Status; >> >> -  EFI_ACPI_TABLE_PROTOCOL     *AcpiTableProtocol; >> >> -  CM_STD_OBJ_ACPI_TABLE_INFO  *AcpiTableInfo; >> >> -  UINT32                      AcpiTableCount; >> >> -  UINT32                      Idx; >> >> +  EFI_STATUS                   Status; >> >> +  EFI_ACPI_TABLE_PROTOCOL      *AcpiTableProtocol; >> >> +  CM_STD_OBJ_ACPI_TABLE_INFO   *AcpiTableInfo; >> >> +  UINT32                       AcpiTableCount; >> >> +  UINT32                       Idx; >> >> +  UINTN                        Handle; >> >> +  EFI_ACPI_DESCRIPTION_HEADER  *DummyHeader; >> >> >>     ASSERT (TableFactoryProtocol != NULL); >> >>     ASSERT (CfgMgrProtocol != NULL); >> >> @@ -570,29 +694,37 @@ ProcessAcpiTables ( >>     } > >> [snip] > >   // Check if mandatory ACPI tables are present. >   Status = VerifyMandatoryTablesArePresent ( >              AcpiTableInfo, >              AcpiTableCount >              ); >   if (EFI_ERROR (Status)) { >     DEBUG (( >       DEBUG_ERROR, >       "ERROR: Failed to find mandatory ACPI Table(s)." >       " Status = %r\n", >       Status > >       )); > > > [SAMI] Is it possible to update the error reporting to reflect the > EFI_ALREADY_STARTED error type, please? Please also update the > function documentation header for ProcessAcpiTables(). > > >     return Status; > >   } > > > [/snip] > > >> >>     // 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; >> >> +  Status = LocateAcpiTableBySignature ( >> >> + EFI_ACPI_6_2_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE, >> >> +             &DummyHeader, >> >> +             &Handle >> >> +             ); >> >> +  if (EFI_ERROR (Status)) { >> >> +    // 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; >> >>         } >> >> - >> >> -      break; >> >> -    } >> >> -  } // for >> >> +    } // 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 > > [SAMI] Should the DEPEX section be updated to relect the dependency on > the SDT protocol? > >> >> >>     gEdkiiConfigurationManagerProtocolGuid        # PROTOCOL >> ALWAYS_CONSUMED >> >>     gEdkiiDynamicTableFactoryProtocolGuid         # PROTOCOL >> ALWAYS_CONSUMED >>