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

["Zeng, Star" <star.zeng@intel.com>]

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