Re: [PATCH] IntelFrameworkPkg FrameworkUefiLib: Add new EfiLocateXXXAcpiTable APIs

["Yao, Jiewen" <jiewen.yao@intel.com>]

Yes.

> -----Original Message-----
> From: Zeng, Star
> Sent: Wednesday, September 19, 2018 10:11 AM
> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org
> Cc: Younas khan <pmdyounaskhan786@gmail.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Zeng,
> Star <star.zeng@intel.com>
> Subject: RE: [PATCH] IntelFrameworkPkg FrameworkUefiLib: Add new
> EfiLocateXXXAcpiTable APIs
> 
> You mean like below for the case?
> ASSERT(FALSE);
> return NULL;
> 
> Thanks,
> Star
> -----Original Message-----
> From: Yao, Jiewen
> Sent: Wednesday, September 19, 2018 10:08 AM
> To: Zeng, Star <star.zeng@intel.com>; edk2-devel@lists.01.org
> Cc: Younas khan <pmdyounaskhan786@gmail.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [PATCH] IntelFrameworkPkg FrameworkUefiLib: Add new
> EfiLocateXXXAcpiTable APIs
> 
> Hi
> I do not recommend we add more new feature to a to-be-deprecated
> package.
> 
> Can we just return UNSUPPORTED?
> 
> 
> > -----Original Message-----
> > From: Zeng, Star
> > Sent: Wednesday, September 19, 2018 9:28 AM
> > To: edk2-devel@lists.01.org
> > Cc: Zeng, Star <star.zeng@intel.com>; Younas khan
> > <pmdyounaskhan786@gmail.com>; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Gao, Liming <liming.gao@intel.com>; Yao,
> > Jiewen <jiewen.yao@intel.com>
> > Subject: [PATCH] IntelFrameworkPkg FrameworkUefiLib: Add new
> > EfiLocateXXXAcpiTable APIs
> >
> > It updates FrameworkUefiLib and is the supplement to
> > https://lists.01.org/pipermail/edk2-devel/2018-September/029757.html
> >
> > https://bugzilla.tianocore.org/show_bug.cgi?id=967
> > Request to add a library function for GetAcpiTable() in order to get
> > ACPI table using signature as input.
> >
> > After evaluation, we found there are many duplicated code to find ACPI
> > table by signature in different modules.
> >
> > This patch adds new EfiLocateXXXAcpiTable() APIs in UefiLib for the
> > request and also the following patch to remove the duplicated code.
> >
> > Cc: Younas khan <pmdyounaskhan786@gmail.com>
> > Cc: Michael D Kinney <michael.d.kinney@intel.com>
> > Cc: Liming Gao <liming.gao@intel.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Star Zeng <star.zeng@intel.com>
> > ---
> >  IntelFrameworkPkg/Library/FrameworkUefiLib/Acpi.c  | 428
> > +++++++++++++++++++++
> >  .../Library/FrameworkUefiLib/FrameworkUefiLib.inf  |   3 +
> >  2 files changed, 431 insertions(+)
> >  create mode 100644
> IntelFrameworkPkg/Library/FrameworkUefiLib/Acpi.c
> >
> > diff --git a/IntelFrameworkPkg/Library/FrameworkUefiLib/Acpi.c
> > b/IntelFrameworkPkg/Library/FrameworkUefiLib/Acpi.c
> > new file mode 100644
> > index 000000000000..e16ccc7e7b74
> > --- /dev/null
> > +++ b/IntelFrameworkPkg/Library/FrameworkUefiLib/Acpi.c
> > @@ -0,0 +1,428 @@
> > +/** @file
> > +  This module provides help function for finding ACPI table.
> > +
> > +  Copyright (c) 2018, Intel Corporation. All rights reserved.<BR>
> > + This program and the accompanying materials  are licensed and made
> > + available under the terms and conditions of the
> > BSD License
> > +  which accompanies this distribution.  The full text of the license
> > + may be
> > found at
> > +  http://opensource.org/licenses/bsd-license.php.
> > +
> > +  THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS
> IS"
> > BASIS,
> > +  WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER
> > EXPRESS OR IMPLIED.
> > +
> > +**/
> > +
> > +#include "UefiLibInternal.h"
> > +#include <IndustryStandard/Acpi.h>
> > +#include <Guid/Acpi.h>
> > +
> > +/**
> > +  This function scans ACPI table in XSDT/RSDT.
> > +
> > +  @param Sdt                    ACPI XSDT/RSDT.
> > +  @param TablePointerSize       Size of table pointer: 8(XSDT) or
> > 4(RSDT).
> > +  @param Signature              ACPI table signature.
> > +  @param PreviousTable          Pointer to previous returned table
> to
> > locate
> > +                                next table, or NULL to locate first
> > table.
> > +  @param PreviousTableLocated   Pointer to the indicator about
> > whether the
> > +                                previous returned table could be
> > located, or
> > +                                NULL if PreviousTable is NULL.
> > +
> > +  If PreviousTable is NULL and PreviousTableLocated is not NULL, then
> > ASSERT().
> > +  If PreviousTable is not NULL and PreviousTableLocated is NULL, then
> > ASSERT().
> > +
> > +  @return ACPI table or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +ScanTableInSDT (
> > +  IN  EFI_ACPI_DESCRIPTION_HEADER   *Sdt,
> > +  IN  UINTN                         TablePointerSize,
> > +  IN  UINT32                        Signature,
> > +  IN  EFI_ACPI_COMMON_HEADER        *PreviousTable,
> OPTIONAL
> > +  OUT BOOLEAN                       *PreviousTableLocated
> > OPTIONAL
> > +  )
> > +{
> > +  UINTN                             Index;
> > +  UINTN                             EntryCount;
> > +  UINT64                            EntryPtr;
> > +  UINTN                             BasePtr;
> > +  EFI_ACPI_COMMON_HEADER            *Table;
> > +
> > +  if (PreviousTableLocated != NULL) {
> > +    ASSERT (PreviousTable != NULL);
> > +    *PreviousTableLocated = FALSE;
> > +  } else {
> > +    ASSERT (PreviousTable == NULL);
> > +  }
> > +
> > +  if (Sdt == NULL) {
> > +    return NULL;
> > +  }
> > +
> > +  EntryCount = (Sdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER))
> /
> > TablePointerSize;
> > +
> > +  BasePtr = (UINTN)(Sdt + 1);
> > +  for (Index = 0; Index < EntryCount; Index ++) {
> > +    EntryPtr = 0;
> > +    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * TablePointerSize),
> > TablePointerSize);
> > +    Table = (EFI_ACPI_COMMON_HEADER *)((UINTN)(EntryPtr));
> > +    if (Table->Signature == Signature) {
> > +      if (PreviousTable != NULL) {
> > +        if (Table == PreviousTable) {
> > +          *PreviousTableLocated = TRUE;
> > +        } else if (*PreviousTableLocated) {
> > +          //
> > +          // Return next table.
> > +          //
> > +          return Table;
> > +        }
> > +      } else {
> > +        //
> > +        // Return first table.
> > +        //
> > +        return Table;
> > +      }
> > +
> > +    }
> > +  }
> > +
> > +  return NULL;
> > +}
> > +
> > +/**
> > +  To locate FACS in FADT.
> > +
> > +  @param Fadt   FADT table pointer.
> > +
> > +  @return FACS table pointer or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +LocateAcpiFacsFromFadt (
> > +  IN EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE  *Fadt
> > +  )
> > +{
> > +  EFI_ACPI_COMMON_HEADER                        *Facs;
> > +  UINT64                                        Data64;
> > +
> > +  if (Fadt == NULL) {
> > +    return NULL;
> > +  }
> > +
> > +  if (Fadt->Header.Revision <
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
> > +    Facs = (EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt->FirmwareCtrl;
> > +  } else {
> > +    CopyMem (&Data64, &Fadt->XFirmwareCtrl, sizeof(UINT64));
> > +    if (Data64 != 0) {
> > +      Facs = (EFI_ACPI_COMMON_HEADER *)(UINTN)Data64;
> > +    } else {
> > +      Facs = (EFI_ACPI_COMMON_HEADER
> *)(UINTN)Fadt->FirmwareCtrl;
> > +    }
> > +  }
> > +  return Facs;
> > +}
> > +
> > +/**
> > +  To locate DSDT in FADT.
> > +
> > +  @param Fadt   FADT table pointer.
> > +
> > +  @return DSDT table pointer or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +LocateAcpiDsdtFromFadt (
> > +  IN EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE  *Fadt
> > +  )
> > +{
> > +  EFI_ACPI_COMMON_HEADER                        *Dsdt;
> > +  UINT64                                        Data64;
> > +
> > +  if (Fadt == NULL) {
> > +    return NULL;
> > +  }
> > +
> > +  if (Fadt->Header.Revision <
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
> > +    Dsdt = (EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt->Dsdt;
> > +  } else {
> > +    CopyMem (&Data64, &Fadt->XDsdt, sizeof(UINT64));
> > +    if (Data64 != 0) {
> > +      Dsdt = (EFI_ACPI_COMMON_HEADER *)(UINTN)Data64;
> > +    } else {
> > +      Dsdt = (EFI_ACPI_COMMON_HEADER *)(UINTN)Fadt->Dsdt;
> > +    }
> > +  }
> > +  return Dsdt;
> > +}
> > +
> > +/**
> > +  To locate ACPI table in ACPI ConfigurationTable.
> > +
> > +  @param AcpiTableGuid          The GUID used to get ACPI
> > ConfigurationTable.
> > +  @param Signature              ACPI table signature.
> > +  @param PreviousTable          Pointer to previous returned table
> to
> > locate
> > +                                next table, or NULL to locate first
> > table.
> > +  @param PreviousTableLocated   Pointer to the indicator to return
> > whether the
> > +                                previous returned table could be
> > located or not,
> > +                                or NULL if PreviousTable is NULL.
> > +
> > +  If PreviousTable is NULL and PreviousTableLocated is not NULL, then
> > ASSERT().
> > +  If PreviousTable is not NULL and PreviousTableLocated is NULL, then
> > ASSERT().
> > +  If AcpiGuid is NULL, then ASSERT().
> > +
> > +  @return ACPI table or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +LocateAcpiTableInAcpiConfigurationTable (
> > +  IN  EFI_GUID                  *AcpiGuid,
> > +  IN  UINT32                    Signature,
> > +  IN  EFI_ACPI_COMMON_HEADER    *PreviousTable, OPTIONAL
> > +  OUT BOOLEAN                   *PreviousTableLocated
> OPTIONAL
> > +  )
> > +{
> > +  EFI_STATUS                                    Status;
> > +  EFI_ACPI_COMMON_HEADER                        *Table;
> > +  EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER  *Rsdp;
> > +  EFI_ACPI_DESCRIPTION_HEADER                   *Rsdt;
> > +  EFI_ACPI_DESCRIPTION_HEADER                   *Xsdt;
> > +  EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *Fadt;
> > +
> > +  if (PreviousTableLocated != NULL) {
> > +    ASSERT (PreviousTable != NULL);
> > +    *PreviousTableLocated = FALSE;
> > +  } else {
> > +    ASSERT (PreviousTable == NULL);
> > +  }
> > +
> > +  Rsdp = NULL;
> > +  //
> > +  // Get ACPI ConfigurationTable (RSD_PTR)  //  Status =
> > + EfiGetSystemConfigurationTable(AcpiGuid, (VOID **)&Rsdp);  if
> > + (EFI_ERROR (Status) || (Rsdp == NULL)) {
> > +    return NULL;
> > +  }
> > +
> > +  Table = NULL;
> > +
> > +  //
> > +  // Search XSDT
> > +  //
> > +  if (Rsdp->Revision >=
> > EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION) {
> > +    Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)
> > Rsdp->XsdtAddress;
> > +    if (Signature ==
> >
> EFI_ACPI_2_0_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE
> )
> > {
> > +      ASSERT (PreviousTable == NULL);
> > +      //
> > +      // It is to locate DSDT,
> > +      // need to locate FADT first.
> > +      //
> > +      Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
> > ScanTableInSDT (
> > +               Xsdt,
> > +               sizeof (UINT64),
> > +
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> > +               NULL,
> > +               NULL
> > +               );
> > +      Table = LocateAcpiDsdtFromFadt (Fadt);
> > +    } else if (Signature ==
> > EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> > +      ASSERT (PreviousTable == NULL);
> > +      //
> > +      // It is to locate FACS,
> > +      // need to locate FADT first.
> > +      //
> > +      Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
> > ScanTableInSDT (
> > +               Xsdt,
> > +               sizeof (UINT64),
> > +
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> > +               NULL,
> > +               NULL
> > +               );
> > +      Table = LocateAcpiFacsFromFadt (Fadt);
> > +    } else {
> > +      Table = ScanTableInSDT (
> > +                Xsdt,
> > +                sizeof (UINT64),
> > +                Signature,
> > +                PreviousTable,
> > +                PreviousTableLocated
> > +                );
> > +    }
> > +  }
> > +
> > +  if (Table != NULL) {
> > +    return Table;
> > +  } else if ((PreviousTableLocated != NULL) &&
> > +              *PreviousTableLocated) {
> > +    //
> > +    // PreviousTable could be located in XSDT,
> > +    // but next table could not be located in XSDT.
> > +    //
> > +    return NULL;
> > +  }
> > +
> > +  //
> > +  // Search RSDT
> > +  //
> > +  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)
> Rsdp->RsdtAddress;
> > + if (Signature ==
> >
> EFI_ACPI_2_0_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE
> )
> > {
> > +    ASSERT (PreviousTable == NULL);
> > +    //
> > +    // It is to locate DSDT,
> > +    // need to locate FADT first.
> > +    //
> > +    Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
> > ScanTableInSDT (
> > +             Rsdt,
> > +             sizeof (UINT32),
> > +
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> > +             NULL,
> > +             NULL
> > +             );
> > +    Table = LocateAcpiDsdtFromFadt (Fadt);  } else if (Signature ==
> > EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) {
> > +    ASSERT (PreviousTable == NULL);
> > +    //
> > +    // It is to locate FACS,
> > +    // need to locate FADT first.
> > +    //
> > +    Fadt = (EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE *)
> > ScanTableInSDT (
> > +             Rsdt,
> > +             sizeof (UINT32),
> > +
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE,
> > +             NULL,
> > +             NULL
> > +             );
> > +    Table = LocateAcpiFacsFromFadt (Fadt);  } else {
> > +    Table = ScanTableInSDT (
> > +              Rsdt,
> > +              sizeof (UINT32),
> > +              Signature,
> > +              PreviousTable,
> > +              PreviousTableLocated
> > +              );
> > +  }
> > +
> > +  return Table;
> > +}
> > +
> > +/**
> > +  This function locates next ACPI table in XSDT/RSDT based on
> > +Signature
> > and
> > +  previous returned Table.
> > +
> > +  If PreviousTable is NULL:
> > +  This function will locate the first ACPI table in XSDT/RSDT based
> > + on  Signature in gEfiAcpi20TableGuid system configuration table
> > + first, and
> > then
> > +  gEfiAcpi10TableGuid system configuration table.
> > +  This function will locate in XSDT first, and then RSDT.
> > +  For DSDT, this function will locate XDsdt in FADT first, and then
> > + Dsdt in  FADT.
> > +  For FACS, this function will locate XFirmwareCtrl in FADT first,
> > + and then  FirmwareCtrl in FADT.
> > +
> > +  If PreviousTable is not NULL:
> > +  1. If it could be located in XSDT in gEfiAcpi20TableGuid system
> > configuration
> > +     table, then this function will just locate next table in XSDT in
> > +     gEfiAcpi20TableGuid system configuration table.
> > +  2. If it could be located in RSDT in gEfiAcpi20TableGuid system
> > configuration
> > +     table, then this function will just locate next table in RSDT in
> > +     gEfiAcpi20TableGuid system configuration table.
> > +  3. If it could be located in RSDT in gEfiAcpi10TableGuid system
> > configuration
> > +     table, then this function will just locate next table in RSDT in
> > +     gEfiAcpi10TableGuid system configuration table.
> > +
> > +  It's not supported that PreviousTable is not NULL but
> > PreviousTable->Signature
> > +  is not same with Signature, NULL will be returned.
> > +
> > +  @param Signature          ACPI table signature.
> > +  @param PreviousTable      Pointer to previous returned table to
> > locate next
> > +                            table, or NULL to locate first table.
> > +
> > +  @return Next ACPI table or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +EFIAPI
> > +EfiLocateNextAcpiTable (
> > +  IN UINT32                     Signature,
> > +  IN EFI_ACPI_COMMON_HEADER     *PreviousTable OPTIONAL
> > +  )
> > +{
> > +  EFI_ACPI_COMMON_HEADER        *Table;
> > +  BOOLEAN                       TempPreviousTableLocated;
> > +  BOOLEAN                       *PreviousTableLocated;
> > +
> > +  if (PreviousTable != NULL) {
> > +    if (PreviousTable->Signature != Signature) {
> > +      //
> > +      // PreviousTable->Signature is not same with Signature.
> > +      //
> > +      return NULL;
> > +    } else if ((Signature ==
> > EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE) ||
> > +               (Signature ==
> >
> EFI_ACPI_2_0_DIFFERENTIATED_SYSTEM_DESCRIPTION_TABLE_SIGNATURE
> )
> > ||
> > +               (Signature ==
> > EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE)) {
> > +      //
> > +      // There is only one FADT/DSDT/FACS table,
> > +      // so don't try to locate next one.
> > +      //
> > +      return NULL;
> > +    }
> > +
> > +    PreviousTableLocated = &TempPreviousTableLocated;
> > +    *PreviousTableLocated = FALSE;
> > +  } else {
> > +    PreviousTableLocated = NULL;
> > +  }
> > +
> > +  Table = LocateAcpiTableInAcpiConfigurationTable (
> > +            &gEfiAcpi20TableGuid,
> > +            Signature,
> > +            PreviousTable,
> > +            PreviousTableLocated
> > +            );
> > +  if (Table != NULL) {
> > +    return Table;
> > +  } else if ((PreviousTableLocated != NULL) &&
> > +              *PreviousTableLocated) {
> > +    //
> > +    // PreviousTable could be located in gEfiAcpi20TableGuid system
> > +    // configuration table, but next table could not be located in
> > +    // gEfiAcpi20TableGuid system configuration table.
> > +    //
> > +    return NULL;
> > +  }
> > +
> > +  return LocateAcpiTableInAcpiConfigurationTable (
> > +           &gEfiAcpi10TableGuid,
> > +           Signature,
> > +           PreviousTable,
> > +           PreviousTableLocated
> > +           );
> > +}
> > +
> > +/**
> > +  This function locates first ACPI table in XSDT/RSDT based on Signature.
> > +
> > +  This function will locate the first ACPI table in XSDT/RSDT based
> > + on  Signature in gEfiAcpi20TableGuid system configuration table
> > + first, and
> > then
> > +  gEfiAcpi10TableGuid system configuration table.
> > +  This function will locate in XSDT first, and then RSDT.
> > +  For DSDT, this function will locate XDsdt in FADT first, and then
> > + Dsdt in  FADT.
> > +  For FACS, this function will locate XFirmwareCtrl in FADT first,
> > + and then  FirmwareCtrl in FADT.
> > +
> > +  @param Signature          ACPI table signature.
> > +
> > +  @return First ACPI table or NULL if not found.
> > +
> > +**/
> > +EFI_ACPI_COMMON_HEADER *
> > +EFIAPI
> > +EfiLocateFirstAcpiTable (
> > +  IN UINT32                     Signature
> > +  )
> > +{
> > +  return EfiLocateNextAcpiTable (Signature, NULL); }
> > diff --git
> > a/IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf
> > b/IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf
> > index 182d20fca051..2d333b1b4b2a 100644
> > --- a/IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf
> > +++
> b/IntelFrameworkPkg/Library/FrameworkUefiLib/FrameworkUefiLib.inf
> > @@ -37,6 +37,7 @@ [Sources]
> >    Console.c
> >    UefiLib.c
> >    UefiLibInternal.h
> > +  Acpi.c
> >
> >  [Packages]
> >    MdePkg/MdePkg.dec
> > @@ -55,6 +56,8 @@ [LibraryClasses]
> >  [Guids]
> >    gEfiEventReadyToBootGuid                      ##
> > SOMETIMES_CONSUMES  ## Event
> >    gEfiEventLegacyBootGuid                       ##
> > SOMETIMES_CONSUMES  ## Event
> > +  gEfiAcpi20TableGuid                           ##
> > SOMETIMES_CONSUMES  ## SystemTable
> > +  gEfiAcpi10TableGuid                           ##
> > SOMETIMES_CONSUMES  ## SystemTable
> >
> >  [Protocols]
> >    gEfiDriverBindingProtocolGuid                 ##
> > SOMETIMES_PRODUCES
> > --
> > 2.7.0.windows.1