Re: [PATCH V3 3/6] MdeModulePkg S3SaveStateDxe: Use new EfiLocateFirstAcpiTable()

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

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Zeng, Star
> Sent: Monday, September 17, 2018 5:08 PM
> 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>; Wang, Jian J <jian.j.wang@intel.com>
> Subject: [PATCH V3 3/6] MdeModulePkg S3SaveStateDxe: Use new
> EfiLocateFirstAcpiTable()
> 
> 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 updates S3SaveStateDxe to use new
> EfiLocateFirstAcpiTable() and 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>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Star Zeng <star.zeng@intel.com>
> ---
>  .../Acpi/S3SaveStateDxe/AcpiS3ContextSave.c        | 208
> +--------------------
>  .../Acpi/S3SaveStateDxe/S3SaveStateDxe.inf         |   3 +-
>  2 files changed, 5 insertions(+), 206 deletions(-)
> 
> diff --git
> a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
> b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
> index 3f99023f110f..fadafd2b608b 100644
> --- a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
> +++ b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/AcpiS3ContextSave.c
> @@ -22,8 +22,8 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF
> ANY KIND, EITHER EXPRESS OR IMPLIED.
>  #include <Library/LockBoxLib.h>
>  #include <Library/PcdLib.h>
>  #include <Library/DebugLib.h>
> +#include <Library/UefiLib.h>
>  #include <Guid/AcpiS3Context.h>
> -#include <Guid/Acpi.h>
>  #include <IndustryStandard/Acpi.h>
>  #include <Protocol/LockBox.h>
> 
> @@ -76,208 +76,6 @@ AllocateMemoryBelow4G (
>  }
> 
>  /**
> -
> -  This function scan ACPI table in RSDT.
> -
> -  @param Rsdt      ACPI RSDT
> -  @param Signature ACPI table signature
> -
> -  @return ACPI table
> -
> -**/
> -VOID *
> -ScanTableInRSDT (
> -  IN EFI_ACPI_DESCRIPTION_HEADER    *Rsdt,
> -  IN UINT32                         Signature
> -  )
> -{
> -  UINTN                              Index;
> -  UINT32                             EntryCount;
> -  UINT32                             *EntryPtr;
> -  EFI_ACPI_DESCRIPTION_HEADER        *Table;
> -
> -  if (Rsdt == NULL) {
> -    return NULL;
> -  }
> -
> -  EntryCount = (Rsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof(UINT32);
> -
> -  EntryPtr = (UINT32 *)(Rsdt + 1);
> -  for (Index = 0; Index < EntryCount; Index ++, EntryPtr ++) {
> -    Table = (EFI_ACPI_DESCRIPTION_HEADER *)((UINTN)(*EntryPtr));
> -    if (Table->Signature == Signature) {
> -      return Table;
> -    }
> -  }
> -
> -  return NULL;
> -}
> -
> -/**
> -
> -  This function scan ACPI table in XSDT.
> -
> -  @param Xsdt      ACPI XSDT
> -  @param Signature ACPI table signature
> -
> -  @return ACPI table
> -
> -**/
> -VOID *
> -ScanTableInXSDT (
> -  IN EFI_ACPI_DESCRIPTION_HEADER    *Xsdt,
> -  IN UINT32                         Signature
> -  )
> -{
> -  UINTN                          Index;
> -  UINT32                         EntryCount;
> -  UINT64                         EntryPtr;
> -  UINTN                          BasePtr;
> -  EFI_ACPI_DESCRIPTION_HEADER    *Table;
> -
> -  if (Xsdt == NULL) {
> -    return NULL;
> -  }
> -
> -  EntryCount = (Xsdt->Length - sizeof (EFI_ACPI_DESCRIPTION_HEADER)) /
> sizeof(UINT64);
> -
> -  BasePtr = (UINTN)(Xsdt + 1);
> -  for (Index = 0; Index < EntryCount; Index ++) {
> -    CopyMem (&EntryPtr, (VOID *)(BasePtr + Index * sizeof(UINT64)),
> sizeof(UINT64));
> -    Table = (EFI_ACPI_DESCRIPTION_HEADER *)((UINTN)(EntryPtr));
> -    if (Table->Signature == Signature) {
> -      return Table;
> -    }
> -  }
> -
> -  return NULL;
> -}
> -
> -/**
> -  To find Facs in FADT.
> -
> -  @param Fadt   FADT table pointer
> -
> -  @return  Facs table pointer.
> -**/
> -EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *
> -FindAcpiFacsFromFadt (
> -  IN EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE     *Fadt
> -  )
> -{
> -  EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *Facs;
> -  UINT64                                        Data64;
> -
> -  if (Fadt == NULL) {
> -    return NULL;
> -  }
> -
> -  if (Fadt->Header.Revision <
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_REVISION) {
> -    Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Fadt->FirmwareCtrl;
> -  } else {
> -    if (Fadt->FirmwareCtrl != 0) {
> -      Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Fadt->FirmwareCtrl;
> -    } else {
> -      CopyMem (&Data64, &Fadt->XFirmwareCtrl, sizeof(UINT64));
> -      Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE
> *)(UINTN)Data64;
> -    }
> -  }
> -  return Facs;
> -}
> -
> -/**
> -  To find Facs in Acpi tables.
> -
> -  To find Firmware ACPI control strutcure in Acpi Tables since the S3 waking
> vector is stored
> -  in the table.
> -
> -  @param AcpiTableGuid   The guid used to find ACPI table in UEFI
> ConfigurationTable.
> -
> -  @return  Facs table pointer.
> -**/
> -EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *
> -FindAcpiFacsTableByAcpiGuid (
> -  IN EFI_GUID  *AcpiTableGuid
> -  )
> -{
> -  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;
> -  EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *Facs;
> -  UINTN                                         Index;
> -
> -  Rsdp  = NULL;
> -  //
> -  // found ACPI table RSD_PTR from system table
> -  //
> -  for (Index = 0; Index < gST->NumberOfTableEntries; Index++) {
> -    if (CompareGuid (&(gST->ConfigurationTable[Index].VendorGuid),
> AcpiTableGuid)) {
> -      //
> -      // A match was found.
> -      //
> -      Rsdp = gST->ConfigurationTable[Index].VendorTable;
> -      break;
> -    }
> -  }
> -
> -  if (Rsdp == NULL) {
> -    return NULL;
> -  }
> -
> -  //
> -  // Search XSDT
> -  //
> -  if (Rsdp->Revision >=
> EFI_ACPI_2_0_ROOT_SYSTEM_DESCRIPTION_POINTER_REVISION) {
> -    Xsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN)
> Rsdp->XsdtAddress;
> -    Fadt = ScanTableInXSDT (Xsdt,
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE);
> -    if (Fadt != NULL) {
> -      Facs = FindAcpiFacsFromFadt (Fadt);
> -      if (Facs != NULL) {
> -        return Facs;
> -      }
> -    }
> -  }
> -
> -  //
> -  // Search RSDT
> -  //
> -  Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN) Rsdp->RsdtAddress;
> -  Fadt = ScanTableInRSDT (Rsdt,
> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE);
> -  if (Fadt != NULL) {
> -    Facs = FindAcpiFacsFromFadt (Fadt);
> -    if (Facs != NULL) {
> -      return Facs;
> -    }
> -  }
> -
> -  return NULL;
> -}
> -
> -/**
> -  To find Facs in Acpi tables.
> -
> -  To find Firmware ACPI control strutcure in Acpi Tables since the S3 waking
> vector is stored
> -  in the table.
> -
> -  @return  Facs table pointer.
> -**/
> -EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE  *
> -FindAcpiFacsTable (
> -  VOID
> -  )
> -{
> -  EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *Facs;
> -
> -  Facs = FindAcpiFacsTableByAcpiGuid (&gEfiAcpi20TableGuid);
> -  if (Facs != NULL) {
> -    return Facs;
> -  }
> -
> -  return FindAcpiFacsTableByAcpiGuid (&gEfiAcpi10TableGuid);
> -}
> -
> -/**
>    The function will check if long mode waking vector is supported.
> 
>    @param[in] Facs   Pointer to FACS table.
> @@ -460,7 +258,9 @@ AcpiS3ContextSaveOnEndOfDxe (
>    //
>    // Get ACPI Table because we will save its position to variable
>    //
> -  Facs = (EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)
> FindAcpiFacsTable ();
> +  Facs = (EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE *)
> EfiLocateFirstAcpiTable (
> +
> EFI_ACPI_4_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE
> +                                                            );
>    AcpiS3Context->AcpiFacsTable = (EFI_PHYSICAL_ADDRESS) (UINTN) Facs;
>    ASSERT (AcpiS3Context->AcpiFacsTable != 0);
> 
> diff --git
> a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> index 744cf8ab3270..e385356dee1b 100644
> --- a/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> +++ b/MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf
> @@ -49,12 +49,11 @@ [LibraryClasses]
>    PcdLib
>    HobLib
>    LockBoxLib
> +  UefiLib
> 
>  [Guids]
>    gEfiAcpiVariableGuid                       ## PRODUCES  ##
> UNDEFINED # LockBox Save Data.
>    gEfiAcpiS3ContextGuid                      ## PRODUCES  ##
> UNDEFINED # LockBox Save Data.
> -  gEfiAcpi20TableGuid                        ##
> SOMETIMES_CONSUMES  ## SystemTable
> -  gEfiAcpi10TableGuid                        ##
> SOMETIMES_CONSUMES  ## SystemTable
>    gEfiEndOfDxeEventGroupGuid                 ## CONSUMES  ##
> Event
> 
>  [Protocols]
> --
> 2.7.0.windows.1