Right, but Kabylake has a different implementation that retrieves it from HW registers - PchAcpiBaseGet(). This is probably optional, there is a PCD, but it's in a different package scope. I don't know how to handle the Packages in the INF to keep this silicon package agnostic. For that matter, it might not really be a CFL *plus* shim, because Tigerlake, etc are different package DECs too. Best regards, Benjamin On Mon, 29 Aug 2022 at 19:17, Oram, Isaac W wrote: > I think that the shim lib might be overkill. PmcGetAcpiBase just resolves > to PcdGet16 (PcdAcpiBaseAddress); > I think that you should be able to use that PCD for any Intel > chipset/silicon for the foreseeable future. > > I would prefer to see contents of sections in INF files indented, but it > is a nit. > > Regards, > Isaac > > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Benjamin > Doron > Sent: Monday, August 29, 2022 1:36 PM > To: devel@edk2.groups.io > Cc: Desimone, Nathaniel L ; Sinha, Ankit < > ankit.sinha@intel.com>; Ni, Ray ; Chaganty, Rangasai V < > rangasai.v.chaganty@intel.com>; Oram, Isaac W > Subject: [edk2-devel][edk2-platforms][PATCH v1 2/5] Silicon/Intel: Port > SmmControl protocol to PPI for S3 > > S3 resume may require communication with SMM, for which we need the > SmmControl PPI. Therefore, port the DXE drivers to a library, like there is > for SMM Access. > > As the registers are common across Intel platforms in the tree, while a > helper function definition is not, implement a new library as a > compatibility shim. > > Tested, working on Kabylake. Further testing required after the refactor > for compatibility. > > Cc: Nate DeSimone > Cc: Ankit Sinha > Cc: Ray Ni > Cc: Rangasai V Chaganty > Cc: Isaac Oram > Signed-off-by: Benjamin Doron > --- > .../BaseIntelCompatShimLibCfl.c | 24 ++ > .../BaseIntelCompatShimLibCfl.inf | 24 ++ > .../PeiSmmControlLib/PeiSmmControlLib.c | 309 ++++++++++++++++++ > .../PeiSmmControlLib/PeiSmmControlLib.inf | 36 ++ > .../Include/Library/IntelCompatShimLib.h | 20 ++ > .../Include/Library/SmmControlLib.h | 26 ++ > .../Intel/IntelSiliconPkg/IntelSiliconPkg.dec | 4 + > .../BaseIntelCompatShimLibKbl.c | 29 ++ > .../BaseIntelCompatShimLibKbl.inf | 24 ++ > 9 files changed, 496 insertions(+) > create mode 100644 > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.c > create mode 100644 > Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.inf > create mode 100644 > Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.c > create mode 100644 > Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.inf > create mode 100644 > Silicon/Intel/IntelSiliconPkg/Include/Library/IntelCompatShimLib.h > create mode 100644 > Silicon/Intel/IntelSiliconPkg/Include/Library/SmmControlLib.h > create mode 100644 > Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.c > create mode 100644 > Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.inf > > diff --git > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.c > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.c > new file mode 100644 > index 000000000000..5353126267e6 > --- /dev/null > +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibC > +++ fl/BaseIntelCompatShimLibCfl.c > @@ -0,0 +1,24 @@ > +/** @file > + A Coffeelake+ compatibility shim > + > + Copyright (c) 2022, Baruch Binyamin Doron
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > + > +/** > + Get PCH ACPI base address. > + > + @retval Address Address of PWRM base address. > +**/ > +UINT16 > +EFIAPI > +CompatShimGetAcpiBase ( > + VOID > + ) > +{ > + return PmcGetAcpiBase (); > +} > diff --git > a/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.inf > b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibCfl/BaseIntelCompatShimLibCfl.inf > new file mode 100644 > index 000000000000..48b071ed05ae > --- /dev/null > +++ b/Silicon/Intel/CoffeelakeSiliconPkg/Library/BaseIntelCompatShimLibC > +++ fl/BaseIntelCompatShimLibCfl.inf > @@ -0,0 +1,24 @@ > +## @file > +# Library description file for the Coffeelake+ compatibility shim # # > +Copyright (c) 2022, Baruch Binyamin Doron
# > +SPDX-License-Identifier: BSD-2-Clause-Patent # ## > + > +[Defines] > +INF_VERSION = 0x00010017 > +BASE_NAME = BaseIntelCompatShimLibCfl > +FILE_GUID = 3D0BB32E-D328-4615-ADFC-782CECC68D53 > +VERSION_STRING = 1.0 > +MODULE_TYPE = BASE > +LIBRARY_CLASS = IntelCompatShimLib > + > +[LibraryClasses] > +PmcLib > + > +[Packages] > +CoffeelakeSiliconPkg/SiPkg.dec > + > +[Sources] > +BaseIntelCompatShimLibCfl.c > diff --git > a/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.c > b/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.c > new file mode 100644 > index 000000000000..80c2c1be90b1 > --- /dev/null > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmCon > +++ trolLib/PeiSmmControlLib.c > @@ -0,0 +1,309 @@ > +/** @file+ This is to publish the SMM Control Ppi instance.++ Copyright > (c) 2019 - 2020, Intel Corporation. All rights reserved.
+ > SPDX-License-Identifier: BSD-2-Clause-Patent++**/+#include > +#include +#include > +#include +#include > +#include > ++#include +#include > ++#define SMM_CONTROL_PRIVATE_DATA_SIGNATURE > SIGNATURE_32 ('i', '4', 's', 'c')++typedef struct {+ UINTN > Signature;+ EFI_HANDLE Handle;+ > EFI_PEI_MM_CONTROL_PPI SmmControl;+} > SMM_CONTROL_PRIVATE_DATA;++#define SMM_CONTROL_PRIVATE_DATA_FROM_THIS(a) > \+ CR (a, \+ SMM_CONTROL_PRIVATE_DATA, \+ > SmmControl, \+ SMM_CONTROL_DEV_SIGNATURE \+ )++//+// Common > registers:+//+//+// APM Registers+//+#define R_PCH_APM_CNT > 0xB2+//+// ACPI and legacy I/O register offsets from > ACPIBASE+//+#define R_PCH_ACPI_PM1_STS 0x00+#define > B_PCH_ACPI_PM1_STS_PRBTNOR BIT11++#define R_PCH_SMI_EN > 0x30++#define R_PCH_SMI_STS > 0x34+#define B_PCH_SMI_STS_APM BIT5+#define > B_PCH_SMI_EN_APMC BIT5+#define B_PCH_SMI_EN_EOS > BIT1+#define B_PCH_SMI_EN_GBL_SMI > BIT0++/**+ Trigger the software SMI++ @param[in] Data > The value to be set on the software SMI data port++ @retval EFI_SUCCESS > Function completes successfully+**/+EFI_STATUS+EFIAPI+SmmTrigger > (+ UINT8 Data+ )+{+ UINT16 ABase;+ UINT32 OutputData;+ UINT32 > OutputPort;++ ABase = CompatShimGetAcpiBase ();++ ///+ /// Enable the > APMC SMI+ ///+ OutputPort = ABase + R_PCH_SMI_EN;+ OutputData = > IoRead32 ((UINTN) OutputPort);+ OutputData |= (B_PCH_SMI_EN_APMC | > B_PCH_SMI_EN_GBL_SMI);+ DEBUG (+ (DEBUG_EVENT,+ "The SMI Control > Port at address %x will be written to %x.\n",+ OutputPort,+ > OutputData)+ );+ IoWrite32 (+ (UINTN) OutputPort,+ (UINT32) > (OutputData)+ );++ OutputPort = R_PCH_APM_CNT;+ OutputData = > Data;++ ///+ /// Generate the APMC SMI+ ///+ IoWrite8 (+ (UINTN) > OutputPort,+ (UINT8) (OutputData)+ );++ return EFI_SUCCESS;+}++/**+ > Clear the SMI status+++ @retval EFI_SUCCESS The function > completes successfully+ @retval EFI_DEVICE_ERROR Something error > occurred+**/+EFI_STATUS+EFIAPI+SmmClear (+ VOID+ )+{+ UINT16 ABase;+ > UINT32 OutputData;+ UINT32 OutputPort;++ ABase = CompatShimGetAcpiBase > ();++ ///+ /// Clear the Power Button Override Status Bit, it gates EOS > from being set.+ ///+ OutputPort = ABase + R_PCH_ACPI_PM1_STS;+ > OutputData = B_PCH_ACPI_PM1_STS_PRBTNOR;+ DEBUG (+ (DEBUG_EVENT,+ > "The PM1 Status Port at address %x will be written to %x.\n",+ > OutputPort,+ OutputData)+ );+ IoWrite16 (+ (UINTN) > OutputPort,+ (UINT16) (OutputData)+ );++ ///+ /// Clear the APM SMI > Status Bit+ ///+ OutputPort = ABase + R_PCH_SMI_STS;+ OutputData = > B_PCH_SMI_STS_APM;+ DEBUG (+ (DEBUG_EVENT,+ "The SMI Status Port at > address %x will be written to %x.\n",+ OutputPort,+ OutputData)+ > );+ IoWrite32 (+ (UINTN) OutputPort,+ (UINT32) (OutputData)+ > );++ ///+ /// Set the EOS Bit+ ///+ OutputPort = ABase + > R_PCH_SMI_EN;+ OutputData = IoRead32 ((UINTN) OutputPort);+ OutputData > |= B_PCH_SMI_EN_EOS;+ DEBUG (+ (DEBUG_EVENT,+ "The SMI Control Port > at address %x will be written to %x.\n",+ OutputPort,+ > OutputData)+ );+ IoWrite32 (+ (UINTN) OutputPort,+ (UINT32) > (OutputData)+ );++ ///+ /// There is no need to read EOS back and > check if it is set.+ /// This can lead to a reading of zero if an SMI > occurs right after the SMI_EN port read+ /// but before the data is > returned to the CPU.+ /// SMM Dispatcher should make sure that EOS is set > after all SMI sources are processed.+ ///+ return EFI_SUCCESS;+}++/**+ > This routine generates an SMI++ @param[in] This The > EFI SMM Control protocol instance+ @param[in, out] ArgumentBuffer > The buffer of argument+ @param[in, out] ArgumentBufferSize The size of > the argument buffer+ @param[in] Periodic Periodic or > not+ @param[in] ActivationInterval Interval of periodic SMI++ > @retval EFI Status Describing the result of the > operation+ @retval EFI_INVALID_PARAMETER Some parameter value > passed is not supported+**/+EFI_STATUS+EFIAPI+Activate (+ IN > EFI_PEI_SERVICES **PeiServices,+ IN EFI_PEI_MM_CONTROL_PPI * > This,+ IN OUT INT8 *ArgumentBuffer OPTIONAL,+ IN OUT > UINTN *ArgumentBufferSize OPTIONAL,+ IN BOOLEAN > Periodic OPTIONAL,+ IN UINTN ActivationInterval > OPTIONAL+ )+{+ EFI_STATUS Status;+ UINT8 Data;++ if (Periodic) > {+ DEBUG ((DEBUG_WARN, "Invalid parameter\n"));+ return > EFI_INVALID_PARAMETER;+ }++ // NOTE: Copied from Quark. Matches the usage > in PiSmmCommunicationPei+ if (ArgumentBuffer == NULL) {+ Data = 0xFF;+ > } else {+ if (ArgumentBufferSize == NULL || *ArgumentBufferSize != 1) > {+ return EFI_INVALID_PARAMETER;+ }++ Data = *ArgumentBuffer;+ > }+ ///+ /// Clear any pending the APM SMI+ ///+ Status = SmmClear ();+ > if (EFI_ERROR (Status)) {+ return Status;+ }++ return SmmTrigger > (Data);+}++/**+ This routine clears an SMI++ @param[in] This > The EFI SMM Control protocol instance+ @param[in] Periodic > Periodic or not++ @retval EFI Status Describing the result > of the operation+ @retval EFI_INVALID_PARAMETER Some parameter value > passed is not supported+**/+EFI_STATUS+EFIAPI+Deactivate (+ IN > EFI_PEI_SERVICES **PeiServices,+ IN EFI_PEI_MM_CONTROL_PPI * > This,+ IN BOOLEAN Periodic OPTIONAL+ )+{+ if (Periodic) > {+ return EFI_INVALID_PARAMETER;+ }++ return SmmClear ();+}++/**+ > This function is to install an SMM Control PPI+ - Introduction \n+ > An API to install an instance of EFI_PEI_MM_CONTROL_PPI. This PPI > provides a standard+ way for other modules to trigger software SMIs.++ > @retval EFI_SUCCESS - Ppi successfully started and installed.+ > @retval EFI_NOT_FOUND - Ppi can't be found.+ @retval > EFI_OUT_OF_RESOURCES - Ppi does not have enough resources to initialize > the driver.+**/+EFI_STATUS+EFIAPI+PeiInstallSmmControlPpi (+ VOID+ )+{+ > EFI_STATUS Status;+ EFI_PEI_PPI_DESCRIPTOR > *PpiList;+ SMM_CONTROL_PRIVATE_DATA *SmmControlPrivate;++ //+ // > Initialize private data+ //+ SmmControlPrivate = AllocateZeroPool > (sizeof (*SmmControlPrivate));+ ASSERT (SmmControlPrivate != NULL);+ if > (SmmControlPrivate == NULL) {+ return EFI_OUT_OF_RESOURCES;+ }+ > PpiList = AllocateZeroPool (sizeof (*PpiList));+ ASSERT (PpiList > != NULL);+ if (PpiList == NULL) {+ return EFI_OUT_OF_RESOURCES;+ }++ > SmmControlPrivate->Signature = SMM_CONTROL_PRIVATE_DATA_SIGNATURE;+ > SmmControlPrivate->Handle = NULL;++ > SmmControlPrivate->SmmControl.Trigger = Activate;+ > SmmControlPrivate->SmmControl.Clear = Deactivate;++ //+ // Install > PPI+ //+ PpiList->Flags = (EFI_PEI_PPI_DESCRIPTOR_PPI | > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST);+ PpiList->Guid = > &gEfiPeiMmControlPpiGuid;+ PpiList->Ppi = > &SmmControlPrivate->SmmControl;++ Status = PeiServicesInstallPpi > (PpiList);+ ASSERT_EFI_ERROR (Status);++ // Unlike driver, do not disable > SMIs as S3 resume continues+ return EFI_SUCCESS;+}diff --git > a/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.inf > b/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.inf > new file mode 100644 > index 000000000000..92f2879d82ab > --- /dev/null > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmCon > +++ trolLib/PeiSmmControlLib.inf > @@ -0,0 +1,36 @@ > +## @file+# Library description file for the SmmControl PPI+#+# > +Copyright (c) 2019, Intel Corporation. All rights reserved.
+# > +SPDX-License-Identifier: > +BSD-2-Clause-Patent+#+##++[Defines]+INF_VERSION = 0x00010017+BASE_NAME > += PeiSmmControlLib+FILE_GUID = > +F45D521A-C0DF-4283-A3CA-65AD01B479E7+VERSION_STRING = 1.0+MODULE_TYPE = > +PEIM+LIBRARY_CLASS = > +SmmControlLib+++[LibraryClasses]+IntelCompatShimLib+IoLib+DebugLib+Memo > +ryAllocationLib+PeiServicesLib+++[Packages]+MdePkg/MdePkg.dec+IntelSili > +conPkg/IntelSiliconPkg.dec+++[Sources]+PeiSmmControlLib.c+++[Ppis]+gEfi > +PeiMmControlPpiGuid ## PRODUCESdiff --git > +a/Silicon/Intel/IntelSiliconPkg/Include/Library/IntelCompatShimLib.h > +b/Silicon/Intel/IntelSiliconPkg/Include/Library/IntelCompatShimLib.h > new file mode 100644 > index 000000000000..f8621d92e41a > --- /dev/null > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/IntelCompatShimLib.h > @@ -0,0 +1,20 @@ > +/** @file > + Library description file for compatibility shim > + > + Copyright (c) 2022, Baruch Binyamin Doron
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > + > +/** > + Get PCH ACPI base address. > + > + @retval Address Address of PWRM base address. > +**/ > +UINT16 > +EFIAPI > +CompatShimGetAcpiBase ( > + VOID > + ); > diff --git a/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmControlLib.h > b/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmControlLib.h > new file mode 100644 > index 000000000000..b532dd13f373 > --- /dev/null > +++ b/Silicon/Intel/IntelSiliconPkg/Include/Library/SmmControlLib.h > @@ -0,0 +1,26 @@ > +/** @file+ This is to publish the SMM Control Ppi instance.++ Copyright > (c) 2019 - 2020, Intel Corporation. All rights reserved.
+ > SPDX-License-Identifier: BSD-2-Clause-Patent++**/+#ifndef > _SMM_CONTROL_LIB_H_+#define _SMM_CONTROL_LIB_H_++/**+ This function is to > install an SMM Control PPI+ - Introduction \n+ An API to install > an instance of EFI_PEI_MM_CONTROL_PPI. This PPI provides a standard+ way > for other modules to trigger software SMIs.++ @retval EFI_SUCCESS > - Ppi successfully started and installed.+ @retval EFI_NOT_FOUND > - Ppi can't be found.+ @retval EFI_OUT_OF_RESOURCES - Ppi does not > have enough resources to initialize the > driver.+**/+EFI_STATUS+EFIAPI+PeiInstallSmmControlPpi (+ VOID+ > );+#endifdiff --git a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec > b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec > index c36d130a0197..fc27b394d267 100644 > --- a/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec > +++ b/Silicon/Intel/IntelSiliconPkg/IntelSiliconPkg.dec > @@ -35,6 +35,10 @@ > # SmmAccessLib|Include/Library/SmmAccessLib.h + ## @libraryclass > Provides services to trigger SMI+ #+ > SmmControlLib|Include/Library/SmmControlLib.h+ ## @libraryclass Provides > services to access config block # > ConfigBlockLib|Include/Library/ConfigBlockLib.hdiff --git > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.c > b/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.c > new file mode 100644 > index 000000000000..573f67555aa3 > --- /dev/null > +++ b/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl > +++ /BaseIntelCompatShimLibKbl.c > @@ -0,0 +1,29 @@ > +/** @file > + A Kabylake compatibility shim > + > + Copyright (c) 2022, Baruch Binyamin Doron
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > + > +/** > + Get PCH ACPI base address. > + > + @retval Address Address of PWRM base address. > +**/ > +UINT16 > +EFIAPI > +CompatShimGetAcpiBase ( > + VOID > + ) > +{ > + UINT16 Address; > + > + Address = 0; > + PchAcpiBaseGet (&Address); > + > + return Address; > +} > diff --git > a/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.inf > b/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.inf > new file mode 100644 > index 000000000000..af3e5a4726e6 > --- /dev/null > +++ b/Silicon/Intel/KabylakeSiliconPkg/Library/BaseIntelCompatShimLibKbl > +++ /BaseIntelCompatShimLibKbl.inf > @@ -0,0 +1,24 @@ > +## @file > +# Library description file for the Kabylake compatibility shim # # > +Copyright (c) 2022, Baruch Binyamin Doron
# > +SPDX-License-Identifier: BSD-2-Clause-Patent # ## > + > +[Defines] > +INF_VERSION = 0x00010017 > +BASE_NAME = BaseIntelCompatShimLibKbl > +FILE_GUID = B4A2193E-CF3E-46E6-8617-49E48143B5AB > +VERSION_STRING = 1.0 > +MODULE_TYPE = BASE > +LIBRARY_CLASS = IntelCompatShimLib > + > +[LibraryClasses] > +PchCycleDecodingLib > + > +[Packages] > +KabylakeSiliconPkg/SiPkg.dec > + > +[Sources] > +BaseIntelCompatShimLibKbl.c > -- > 2.37.2 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#92921): https://edk2.groups.io/g/devel/message/92921 > Mute This Topic: https://groups.io/mt/93335519/1492418 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram@intel.com] > -=-=-=-=-=-= > > >