I prefer S1. I believe that the EfiLocateNextAcpiTable() can also be used in S1.
> -----Original Message----- > From: Zeng, Star > Sent: Monday, September 3, 2018 2:12 PM > To: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > Kinney, > Michael D <michael.d.kin...@intel.com> > Cc: edk2-devel@lists.01.org; Younas khan <pmdyounaskhan...@gmail.com>; > Gao, Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com> > Subject: RE: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > EfiFindAcpiTableBySignature() API > > Thanks. > Ok, please help and we can have good and flexible interface(s) for both > producer > and consumer. > > First, there are two cases we need to consider. > 1. Single table, like FADT, FACS, DSDT, etc. > 2. Multiple tables, like SSDT, etc. > > Then, we have two solutions. > S1: > One interface for single table case, consumer only needs input Signature, has > no > need input previous returned Table. Like GetFirstGuidHob? > One interface for multiple table case, consumer needs input Signature and > previous returned Table. Like GetNextGuidHob? Could we add it later based on > real request? > S2: > One interface for both single table and multiple table cases, consumer needs > input Signature and previous returned Table. Does consumer need input the > table header stored in configuration table by itself(getting configuration > table)? > > > If we like S2, the interface should be like below? > > /** > This function locates next ACPI table based on Signature and previous > returned > Table. > If the input previous returned Table is NULL, this function will locate > next table > in gEfiAcpi20TableGuid system configuration table first, and then > gEfiAcpi10TableGuid > system configuration table. > If the input previous returned Table is not NULL and could be located in > gEfiAcpi20TableGuid system configuration table, this function will just > locate > next > table in gEfiAcpi20TableGuid system configuration table, otherwise > gEfiAcpi10TableGuid > system configuration table. > > @param Signature ACPI table signature. > @param Table Pointer to previous returned 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 *Table > ) > > > > Thanks, > Star > -----Original Message----- > From: Ni, Ruiyu > Sent: Monday, September 3, 2018 1:09 PM > To: Zeng, Star <star.z...@intel.com>; Yao, Jiewen <jiewen....@intel.com>; > Kinney, Michael D <michael.d.kin...@intel.com> > Cc: edk2-devel@lists.01.org; Younas khan <pmdyounaskhan...@gmail.com>; > Gao, Liming <liming....@intel.com> > Subject: RE: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > EfiFindAcpiTableBySignature() API > > That's fine to be in UefiLib. It's already a combo library. > > But I do recommend we think about how to handle multiple tables with same > signature. > When we are adding new APIs, we not only need to evaluate the existing real > case, but also we need to generalize the real cases and try to think about the > more flexible interface. > > > Thanks/Ray > > > -----Original Message----- > > From: Zeng, Star > > Sent: Monday, September 3, 2018 11:26 AM > > To: Yao, Jiewen <jiewen....@intel.com>; Ni, Ruiyu > > <ruiyu...@intel.com>; Kinney, Michael D <michael.d.kin...@intel.com> > > Cc: edk2-devel@lists.01.org; Younas khan <pmdyounaskhan...@gmail.com>; > > Gao, Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com> > > Subject: RE: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > > EfiFindAcpiTableBySignature() API > > > > Good idea. > > > > I did consider DSDT and multiple SSDTs cases. But I did not find any > > real case for them. > > So I made the code simply for current cases, and the code can be > > easily enhanced later for DSDT, a new API can be added later for multiple > SSDTs. > > > > About adding the new API in UefiLib VS new library class, I did also > > consider it and even created code for new library class > > (g...@github.com:lzeng14/edk2.git branch > > FindAcpiTableBySignature_UefiAcpiTableLib). > > But I remember I did discuss it with Mike and Jiewen, we recommended > > UefiLib as it will do not require any platform change. > > > > > > > > Thanks, > > Star > > -----Original Message----- > > From: Yao, Jiewen > > Sent: Saturday, September 1, 2018 7:04 AM > > To: Ni, Ruiyu <ruiyu...@intel.com> > > Cc: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org; Kinney, > > Michael D <michael.d.kin...@intel.com>; Younas khan > > <pmdyounaskhan...@gmail.com>; Gao, Liming <liming....@intel.com> > > Subject: RE: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > > EfiFindAcpiTableBySignature() API > > > > Good idea on LocateNextAcpiTable(). > > > > > > > -----Original Message----- > > > From: Ni, Ruiyu > > > Sent: Saturday, September 1, 2018 12:29 AM > > > To: Yao, Jiewen <jiewen....@intel.com> > > > Cc: Zeng, Star <star.z...@intel.com>; edk2-devel@lists.01.org; > > > Kinney, Michael D <michael.d.kin...@intel.com>; Younas khan > > > <pmdyounaskhan...@gmail.com>; Gao, Liming <liming....@intel.com> > > > Subject: Re: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > > > EfiFindAcpiTableBySignature() API > > > > > > I think LocateNextAcpiTable() is more proper to handle the multiple > > > tables with same signature. It will carry three parameters, one is > > > the table header stored in configuration table, one is the > > > signature, another is > > the previous located table. > > > Can we return a common table header other than void*? > > > > > > Is there better place other than UefiLib? > > > Do we need to add a new library class like AcpiLib? > > > > > > 发自我的 iPhone > > > > > > > 在 2018年8月31日,下午8:00,Yao, Jiewen <jiewen....@intel.com> > > 写 > > > 道: > > > > > > > > Good enhancement. > > > > > > > > I have 2 additional thought: > > > > > > > > 1) How to handle DSDT? > > > > We have special code to handle FACS, but no DSDT. > > > > > > > > 2) How to handle SSDT or other multiple ACPI tables? > > > > We may have multiple SSDT. Usually, it is identified as OEMID. > > > > Do we want to provide similar function for them? > > > > > > > > Anyway, just *additional* thought. :-) Current implementation is > > > > good enough for check in. > > > > > > > > Reviewed-by: jiewen....@intel.com > > > > > > > > Thank you > > > > Yao Jiewen > > > > > > > > > > > >> -----Original Message----- > > > >> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On > > > >> Behalf Of > > > Star > > > >> Zeng > > > >> Sent: Friday, August 31, 2018 7:29 PM > > > >> To: edk2-devel@lists.01.org > > > >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Younas khan > > > >> <pmdyounaskhan...@gmail.com>; Yao, Jiewen > > <jiewen....@intel.com>; > > > Gao, > > > >> Liming <liming....@intel.com>; Zeng, Star <star.z...@intel.com> > > > >> Subject: [edk2] [PATCH 1/6] MdePkg UefiLib: Add new > > > >> EfiFindAcpiTableBySignature() API > > > >> > > > >> 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 EfiFindAcpiTableBySignature() API in UefiLib > > > >> for the request and also the following patch to remove the > > > >> duplicated code. > > > >> > > > >> Cc: Younas khan <pmdyounaskhan...@gmail.com> > > > >> Cc: Michael D Kinney <michael.d.kin...@intel.com> > > > >> Cc: Liming Gao <liming....@intel.com> > > > >> Cc: Jiewen Yao <jiewen....@intel.com> > > > >> Contributed-under: TianoCore Contribution Agreement 1.1 > > > >> Signed-off-by: Star Zeng <star.z...@intel.com> > > > >> --- > > > >> MdePkg/Include/Library/UefiLib.h | 17 +++ > > > >> MdePkg/Library/UefiLib/Acpi.c | 226 > > > >> +++++++++++++++++++++++++++++++++++++ > > > >> MdePkg/Library/UefiLib/UefiLib.inf | 3 + > > > >> 3 files changed, 246 insertions(+) create mode 100644 > > > >> MdePkg/Library/UefiLib/Acpi.c > > > >> > > > >> diff --git a/MdePkg/Include/Library/UefiLib.h > > > >> b/MdePkg/Include/Library/UefiLib.h > > > >> index f80067f11103..8dd25f324fd2 100644 > > > >> --- a/MdePkg/Include/Library/UefiLib.h > > > >> +++ b/MdePkg/Include/Library/UefiLib.h > > > >> @@ -1595,4 +1595,21 @@ EfiOpenFileByDevicePath ( > > > >> IN UINT64 OpenMode, > > > >> IN UINT64 Attributes > > > >> ); > > > >> + > > > >> +/** > > > >> + This function finds ACPI table by signature. > > > >> + It will find the table in gEfiAcpi20TableGuid system > > > >> +configuration table > > > first, > > > >> + and then gEfiAcpi10TableGuid system configuration table. > > > >> + > > > >> + @param Signature ACPI table signature. > > > >> + > > > >> + @return ACPI table or NULL if not found. > > > >> + > > > >> +**/ > > > >> +VOID * > > > >> +EFIAPI > > > >> +EfiFindAcpiTableBySignature ( > > > >> + IN UINT32 Signature > > > >> + ); > > > >> + > > > >> #endif > > > >> diff --git a/MdePkg/Library/UefiLib/Acpi.c > > > >> b/MdePkg/Library/UefiLib/Acpi.c new file mode 100644 index > > > >> 000000000000..5cb93966b59f > > > >> --- /dev/null > > > >> +++ b/MdePkg/Library/UefiLib/Acpi.c > > > >> @@ -0,0 +1,226 @@ > > > >> +/** @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 RSDT. > > > >> + > > > >> + @param Rsdt ACPI RSDT > > > >> + @param Signature ACPI table signature > > > >> + > > > >> + @return ACPI table or NULL if not found. > > > >> + > > > >> +**/ > > > >> +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 scans ACPI table in XSDT. > > > >> + > > > >> + @param Xsdt ACPI XSDT > > > >> + @param Signature ACPI table signature > > > >> + > > > >> + @return ACPI table or NULL if not found. > > > >> + > > > >> +**/ > > > >> +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 or NULL if not found. > > > >> + > > > >> +**/ > > > >> +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 { > > > >> + CopyMem (&Data64, &Fadt->XFirmwareCtrl, sizeof(UINT64)); > > > >> + if (Data64 != 0) { > > > >> + Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE > > > >> *)(UINTN)Data64; > > > >> + } else { > > > >> + Facs = (EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE > > > >> *)(UINTN)Fadt->FirmwareCtrl; > > > >> + } > > > >> + } > > > >> + return Facs; > > > >> +} > > > >> + > > > >> +/** > > > >> + To find ACPI table in ACPI ConfigurationTable. > > > >> + > > > >> + @param AcpiTableGuid The guid used to find ACPI > > ConfigurationTable. > > > >> + @param Signature ACPI table signature. > > > >> + > > > >> + @return ACPI table or NULL if not found. > > > >> + > > > >> +**/ > > > >> +VOID * > > > >> +FindAcpiTableInAcpiConfigurationTable ( > > > >> + IN EFI_GUID *AcpiGuid, > > > >> + IN UINT32 Signature > > > >> + > > > >> + ) > > > >> +{ > > > >> + EFI_STATUS Status; > > > >> + VOID *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; > > > >> + > > > >> + Rsdp = NULL; > > > >> + // > > > >> + // Find 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_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) { > > > >> + // > > > >> + // It is to find FACS ACPI table, > > > >> + // need find FADT first. > > > >> + // > > > >> + Fadt = ScanTableInXSDT (Xsdt, > > > >> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE); > > > >> + Table = FindAcpiFacsFromFadt (Fadt); > > > >> + } else { > > > >> + Table = ScanTableInXSDT (Xsdt, Signature); > > > >> + } > > > >> + } > > > >> + > > > >> + if (Table != NULL) { > > > >> + return Table; > > > >> + } > > > >> + > > > >> + // > > > >> + // Search RSDT > > > >> + // > > > >> + Rsdt = (EFI_ACPI_DESCRIPTION_HEADER *)(UINTN) Rsdp- > > >RsdtAddress; > > > >> + if (Signature == > > > >> EFI_ACPI_2_0_FIRMWARE_ACPI_CONTROL_STRUCTURE_SIGNATURE) { > > > >> + // > > > >> + // It is to find FACS ACPI table, > > > >> + // need find FADT first. > > > >> + // > > > >> + Fadt = ScanTableInRSDT (Rsdt, > > > >> EFI_ACPI_2_0_FIXED_ACPI_DESCRIPTION_TABLE_SIGNATURE); > > > >> + Table = FindAcpiFacsFromFadt (Fadt); } else { > > > >> + Table = ScanTableInRSDT (Rsdt, Signature); } > > > >> + > > > >> + return Table; > > > >> +} > > > >> + > > > >> +/** > > > >> + This function finds ACPI table by signature. > > > >> + It will find the table in gEfiAcpi20TableGuid system > > > >> +configuration table > > > first, > > > >> + and then gEfiAcpi10TableGuid system configuration table. > > > >> + > > > >> + @param Signature ACPI table signature. > > > >> + > > > >> + @return ACPI table or NULL if not found. > > > >> + > > > >> +**/ > > > >> +VOID * > > > >> +EFIAPI > > > >> +EfiFindAcpiTableBySignature ( > > > >> + IN UINT32 Signature > > > >> + ) > > > >> +{ > > > >> + VOID *Table; > > > >> + > > > >> + Table = FindAcpiTableInAcpiConfigurationTable > > > >> + (&gEfiAcpi20TableGuid, > > > >> Signature); > > > >> + if (Table != NULL) { > > > >> + return Table; > > > >> + } > > > >> + > > > >> + return FindAcpiTableInAcpiConfigurationTable > > > >> + (&gEfiAcpi10TableGuid, > > > >> Signature); > > > >> +} > > > >> + > > > >> diff --git a/MdePkg/Library/UefiLib/UefiLib.inf > > > >> b/MdePkg/Library/UefiLib/UefiLib.inf > > > >> index a6c739ef3d6d..aea20fe67153 100644 > > > >> --- a/MdePkg/Library/UefiLib/UefiLib.inf > > > >> +++ b/MdePkg/Library/UefiLib/UefiLib.inf > > > >> @@ -41,6 +41,7 @@ [Sources] > > > >> Console.c > > > >> UefiLib.c > > > >> UefiLibInternal.h > > > >> + Acpi.c > > > >> > > > >> > > > >> [Packages] > > > >> @@ -62,6 +63,8 @@ [Guids] > > > >> gEfiEventReadyToBootGuid ## > > > >> SOMETIMES_CONSUMES ## Event > > > >> gEfiEventLegacyBootGuid ## > > > >> SOMETIMES_CONSUMES ## Event > > > >> gEfiGlobalVariableGuid ## > > > >> SOMETIMES_CONSUMES ## Variable > > > >> + gEfiAcpi20TableGuid ## > > > >> SOMETIMES_CONSUMES ## SystemTable > > > >> + gEfiAcpi10TableGuid ## > > > >> SOMETIMES_CONSUMES ## SystemTable > > > >> > > > >> [Protocols] > > > >> gEfiDriverBindingProtocolGuid ## > > > >> SOMETIMES_PRODUCES > > > >> -- > > > >> 2.7.0.windows.1 > > > >> > > > >> _______________________________________________ > > > >> edk2-devel mailing list > > > >> edk2-devel@lists.01.org > > > >> https://lists.01.org/mailman/listinfo/edk2-devel > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel