> > S3Feature.dsc > - remove commented out code Regarding the SmmAccess library definition, which is the safer default? There is a build failure for the package if nothing is defined, but a platform should be required to define its correct library. Currently, that's the SMRAMC instance for platforms other than Tigerlake. I'm thinking that maybe this definition should be moved to S3FeaturePkg.dsc, so that this package can build, but boards will define the platform appropriate one. # NOTE: RSC will be after end-of-BS, use DebugLibSerialPort > # - No gBS in SerialPortInitialize() > # - No global assigns after ReadyToLock possible, due to LockBox copy > - These comments also don't make sense to me > Just some notes I made on the specific requirements for a debug library stack in BootScriptExecutorDxe. I elaborated in the cover letter, but they shouldn't need to go to the tree, so I'll drop them here. I intend to push a v2 soon with everything fixed. Best regards, Benjamin On Mon, 29 Aug 2022 at 20:19, Oram, Isaac W wrote: > S3Feature.dsc > - remove commented out code > > # Add library instances here that are not included in package components > and should be tested > # in the package build. > - These comments don't make a lot of sense to me in the feature include > DSC. Looks like cut and paste propagation that is not appropriate here. > Note instances in Components.IA32 and Components.X64 sections. > > # NOTE: RSC will be after end-of-BS, use DebugLibSerialPort > # - No gBS in SerialPortInitialize() > # - No global assigns after ReadyToLock possible, due to LockBox copy > - These comments also don't make sense to me > > S3Dxe.c > - infromation should be "information" in multiple instances > - GetPeiMemSize - Generally we would prefer #defines in .h or near top of > the file after includes and before function prototypes and implementations. > - Resolve ToDo > > S3Pei.c > - Resolve ToDo and TODO/TEST > > Regards, > Isaac > > -----Original Message----- > From: 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>; Chaganty, Rangasai V < > rangasai.v.chaganty@intel.com>; Oram, Isaac W ; > Gao, Liming > Subject: [edk2-devel][edk2-platforms][PATCH v1 3/5] S3FeaturePkg: > Implement working S3 resume > > Follow-up commits to MinPlatform (PeiFspWrapperHobProcessLib for > memory) and FSP-related board libraries (policy overrides) required for > successful S3 resume. > > Factored allocation logic into new module to avoid MinPlatform dependency > on S3Feature package. > > TODO: Can optimise required size. > > Cc: Nate DeSimone > Cc: Ankit Sinha > Cc: Sai Chaganty > Cc: Isaac Oram > Cc: Liming Gao > Signed-off-by: Benjamin Doron > --- > .../S3FeaturePkg/Include/PostMemory.fdf | 15 ++ > .../S3FeaturePkg/Include/PreMemory.fdf | 8 +- > .../S3FeaturePkg/Include/S3Feature.dsc | 42 ++++- > .../S3FeaturePkg/S3Dxe/S3Dxe.c | 156 ++++++++++++++++++ > .../S3FeaturePkg/S3Dxe/S3Dxe.inf | 49 ++++++ > .../S3FeaturePkg/S3Pei/S3Pei.c | 83 +++++++++- > .../S3FeaturePkg/S3Pei/S3Pei.inf | 8 +- > .../Include/AcpiS3MemoryNvData.h | 22 +++ > 8 files changed, 375 insertions(+), 8 deletions(-) create mode 100644 > Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c > create mode 100644 > Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.inf > create mode 100644 > Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h > > diff --git > a/Features/Intel/PowerManagement/S3FeaturePkg/Include/PostMemory.fdf > b/Features/Intel/PowerManagement/S3FeaturePkg/Include/PostMemory.fdf > index 9e17f853c630..5d3d96f4f317 100644 > --- a/Features/Intel/PowerManagement/S3FeaturePkg/Include/PostMemory.fdf > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/Include/PostMemory.fdf > @@ -2,7 +2,22 @@ > # FDF file for post-memory S3 advanced feature modules. # # Copyright > (c) 2019, Intel Corporation. All rights reserved.
+# Copyright (c) 2022, > Baruch Binyamin Doron.
# # SPDX-License-Identifier: BSD-2-Clause-Patent > # ##++## Dependencies+ INF > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf+ INF > MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf++## Save-state > module stack+ INF S3FeaturePkg/S3Dxe/S3Dxe.inf+ INF > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf+ # FSP may > perform CPU finalisation, requires CpuInitDxe from closed code+ # - > Presently, PiSmmCpuDxeSmm shall perform finalisation with this data+ INF > UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf++## Restore-state module stack+ > INF > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.infdiff > --git a/Features/Intel/PowerManagement/S3FeaturePkg/Include/PreMemory.fdf > b/Features/Intel/PowerManagement/S3FeaturePkg/Include/PreMemory.fdf > index fdd16a4e0356..e130fa5f098d 100644 > --- a/Features/Intel/PowerManagement/S3FeaturePkg/Include/PreMemory.fdf > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/Include/PreMemory.fdf > @@ -2,9 +2,15 @@ > # FDF file for pre-memory S3 advanced feature modules. # # Copyright (c) > 2019, Intel Corporation. All rights reserved.
+# Copyright (c) 2022, > Baruch Binyamin Doron.
# # SPDX-License-Identifier: BSD-2-Clause-Patent > # ## -INF S3FeaturePkg/S3Pei/S3Pei.inf+## Dependencies+ INF > S3FeaturePkg/S3Pei/S3Pei.inf+ INF > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf++## Restore-state > module stack+ INF > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.infdiff --git > a/Features/Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc > b/Features/Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc > index cc34e785076a..bf45b56258ff 100644 > --- a/Features/Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/Include/S3Feature.dsc > @@ -7,6 +7,7 @@ > # for the build infrastructure. # # Copyright (c) 2019 - 2021, Intel > Corporation. All rights reserved.
+# Copyright (c) 2022, Baruch Binyamin > Doron.
# # SPDX-License-Identifier: BSD-2-Clause-Patent #@@ -25,6 > +26,10 @@ > !error "DXE_ARCH must be specified to build this feature!" !endif > +[PcdsFixedAtBuild]+ # Attempts to improve performance at the cost of more > DRAM usage+ gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnS3Boot|TRUE+ > ################################################################################ > # # Library Class section - list of all Library Classes needed by this > feature.@@ -32,7 +37,15 @@ > ################################################################################ > [LibraryClasses.common.PEIM]- > SmmAccessLib|IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLib/PeiSmmAccessLib.inf+ > #SmmAccessLib|IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.inf+ > SmmControlLib|IntelSiliconPkg/Feature/SmmControl/Library/PeiSmmControlLib/PeiSmmControlLib.inf+ > #IntelCompatShimLib|$(PLATFORM_SI_PACKAGE)/Library/BaseIntelCompatShimLibKbl/BaseIntelCompatShimLibKbl.inf++[LibraryClasses.common.DXE_DRIVER, > LibraryClasses.common.DXE_SMM_DRIVER]+ > #######################################+ # Edk2 Packages+ > #######################################+ > S3BootScriptLib|MdeModulePkg/Library/PiDxeS3BootScriptLib/DxeS3BootScriptLib.inf > ################################################################################ > #@@ -65,3 +78,30 @@ > # Add components here that should be included in the package build. > S3FeaturePkg/S3Pei/S3Pei.inf+ > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationPei.inf+ > UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf++#+# Feature DXE > Components+#++# @todo: Change below line to [Components.$(DXE_ARCH)] after > https://bugzilla.tianocore.org/show_bug.cgi?id=2308+# is > completed.+[Components.X64]+ #####################################+ # S3 > Feature Package+ #####################################++ # Add library > instances here that are not included in package components and should be > tested+ # in the package build.++ # Add components here that should be > included in the package build.+ > UefiCpuPkg/PiSmmCommunication/PiSmmCommunicationSmm.inf+ > MdeModulePkg/Universal/LockBox/SmmLockBox/SmmLockBox.inf+ > S3FeaturePkg/S3Dxe/S3Dxe.inf+ > MdeModulePkg/Universal/Acpi/S3SaveStateDxe/S3SaveStateDxe.inf+ > UefiCpuPkg/CpuS3DataDxe/CpuS3DataDxe.inf+ # NOTE: RSC will be after > end-of-BS, use DebugLibSerialPort+ # - No gBS in SerialPortInitialize()+ > # - No global assigns after ReadyToLock possible, due to LockBox copy+ > MdeModulePkg/Universal/Acpi/BootScriptExecutorDxe/BootScriptExecutorDxe.infdiff > --git a/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c > b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c > new file mode 100644 > index 000000000000..b3fb63e2bc33 > --- /dev/null > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.c > @@ -0,0 +1,156 @@ > +/** @file+ Source code file for S3 DXE module++Copyright (c) 2022, > Baruch Binyamin Doron.
+SPDX-License-Identifier: > BSD-2-Clause-Patent++**/++#include +#include > +#include +#include > +#include +#include > +#include > +#include > +#include +#include > ++/**+ Get the mem size in memory type infromation > table.++ @return the mem size in memory type infromation > table.+**/+UINT64+EFIAPI+GetMemorySizeInMemoryTypeInformation (+ VOID+ > )+{+ EFI_STATUS Status;+ EFI_MEMORY_TYPE_INFORMATION > *MemoryData;+ UINT8 Index;+ UINTN > TempPageNum;++ Status = EfiGetSystemConfigurationTable > (&gEfiMemoryTypeInformationGuid, (VOID **) &MemoryData);++ if (EFI_ERROR > (Status) || MemoryData == NULL) {+ return 0;+ }++ TempPageNum = 0;+ > for (Index = 0; MemoryData[Index].Type != EfiMaxMemoryType; Index++) {+ > //+ // Accumulate default memory size requirements+ //+ > TempPageNum += MemoryData[Index].NumberOfPages;+ }++ return TempPageNum * > EFI_PAGE_SIZE;+}++/**+ Get the mem size need to be consumed and reserved > for PEI phase resume.++ @return the mem size to be reserved for PEI phase > resume.+**/+UINT64+EFIAPI+GetPeiMemSize (+ VOID+ )+{+ #define > PEI_ADDITIONAL_MEMORY_SIZE (16 * EFI_PAGE_SIZE)++ UINT64 > Size;++ Size = GetMemorySizeInMemoryTypeInformation ();++ return > PcdGet32 (PcdPeiMinMemSize) + Size + PEI_ADDITIONAL_MEMORY_SIZE;+}++/**+ > Allocate EfiACPIMemoryNVS below 4G memory address.++ This function > allocates EfiACPIMemoryNVS below 4G memory address.++ @param Size > Size of memory to allocate.++ @return Allocated address for > output.++**/+VOID *+EFIAPI+AllocateAcpiNvsMemoryBelow4G (+ IN UINTN > Size+ )+{+ UINTN Pages;+ EFI_PHYSICAL_ADDRESS > Address;+ EFI_STATUS Status;+ VOID > *Buffer;++ Pages = EFI_SIZE_TO_PAGES (Size);+ Address = 0xffffffff;++ > Status = gBS->AllocatePages (+ AllocateMaxAddress,+ > EfiACPIMemoryNVS,+ Pages,+ > &Address+ );+ ASSERT_EFI_ERROR (Status);++ Buffer = > (VOID *)(UINTN)Address;+ ZeroMem (Buffer, Size);++ return > Buffer;+}++/**+ Allocates memory to use on S3 resume.++ @param[in] > ImageHandle Not used.+ @param[in] SystemTable General > purpose services available to every DXE driver.++ @retval EFI_SUCCESS > The function completes successfully+ @retval > EFI_OUT_OF_RESOURCES Insufficient resources to create > database+**/+EFI_STATUS+EFIAPI+S3DxeEntryPoint (+ IN EFI_HANDLE > ImageHandle,+ IN EFI_SYSTEM_TABLE *SystemTable+ )+{+ UINT64 > S3PeiMemSize;+ UINT64 S3PeiMemBase;+ ACPI_S3_MEMORY > S3MemoryInfo;+ EFI_STATUS Status;++ DEBUG ((DEBUG_INFO, "%a() > Start\n", __FUNCTION__));++ S3PeiMemSize = GetPeiMemSize ();+ > S3PeiMemBase = (UINTN) AllocateAcpiNvsMemoryBelow4G (S3PeiMemSize);+ > ASSERT (S3PeiMemBase != 0);++ S3MemoryInfo.S3PeiMemBase = S3PeiMemBase;+ > S3MemoryInfo.S3PeiMemSize = S3PeiMemSize;++ DEBUG ((DEBUG_INFO, > "S3PeiMemBase: 0x%x\n", S3PeiMemBase));+ DEBUG ((DEBUG_INFO, > "S3PeiMemSize: 0x%x\n", S3PeiMemSize));++ // TODO: LockBox is potentially > superior, though requires static location+ Status = gRT->SetVariable (+ > ACPI_S3_MEMORY_NV_NAME,+ > &gEfiAcpiVariableGuid,+ EFI_VARIABLE_NON_VOLATILE | > EFI_VARIABLE_BOOTSERVICE_ACCESS,+ sizeof (S3MemoryInfo),+ > &S3MemoryInfo+ );+ ASSERT_EFI_ERROR > (Status);++ DEBUG ((DEBUG_INFO, "%a() End\n", __FUNCTION__));+ return > EFI_SUCCESS;+}diff --git > a/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.inf > b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.inf > new file mode 100644 > index 000000000000..28589c2c869b > --- /dev/null > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Dxe/S3Dxe.inf > @@ -0,0 +1,49 @@ > +### @file+# Component information file for the S3 DXE module.+#+# > Copyright (c) 2022, Baruch Binyamin Doron.
+#+# SPDX-License-Identifier: > BSD-2-Clause-Patent+#+###++[Defines]+ INF_VERSION = 0x00010017+ > BASE_NAME = S3Dxe+ FILE_GUID = > 30926F92-CC83-4381-9F70-AC96EDB5BEE0+ VERSION_STRING = 1.0+ > MODULE_TYPE = DXE_DRIVER+ ENTRY_POINT = > S3DxeEntryPoint++[LibraryClasses]+ UefiDriverEntryPoint+ > UefiBootServicesTableLib+ UefiRuntimeServicesTableLib+ BaseMemoryLib+ > DebugLib+ PcdLib+ UefiLib++[Packages]+ MdePkg/MdePkg.dec+ > MdeModulePkg/MdeModulePkg.dec+ MinPlatformPkg/MinPlatformPkg.dec+ > IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec+ > S3FeaturePkg/S3FeaturePkg.dec++[Sources]+ S3Dxe.c++[Pcd]+ > gIntelFsp2WrapperTokenSpaceGuid.PcdPeiMinMemSize++[FeaturePcd]+ > gS3FeaturePkgTokenSpaceGuid.PcdS3FeatureEnable++[Guids]+ > gEfiMemoryTypeInformationGuid ## CONSUMES+ gEfiAcpiVariableGuid > ## CONSUMES++[Depex]+ gEfiVariableArchProtocolGuid AND+ > gEfiVariableWriteArchProtocolGuiddiff --git > a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c > b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c > index b0aaa04962c8..6acb894b6fc9 100644 > --- a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.c > @@ -2,12 +2,87 @@ > Source code file for S3 PEI module Copyright (c) 2019, Intel > Corporation. All rights reserved.
+Copyright (c) 2022, Baruch Binyamin > Doron.
SPDX-License-Identifier: BSD-2-Clause-Patent **/ +#include > +#include +#include > #include #include > +#include ++// TODO: > Finalise implementation factoring+#define R_SA_PAM0 (0x80)+#define > R_SA_PAM5 (0x85)+#define R_SA_PAM6 (0x86)++/**+ This function is called > after FspSiliconInitDone installed PPI.+ For FSP API mode, this is when > FSP-M HOBs are installed into EDK2.++ @param[in] PeiServices Pointer to > PEI Services Table.+ @param[in] NotifyDesc Pointer to the descriptor > for the Notification event that+ caused this > function to execute.+ @param[in] Ppi Pointer to the PPI data > associated with this function.++ @retval EFI_STATUS Always return > EFI_SUCCESS+**/+EFI_STATUS+EFIAPI+FspSiliconInitDoneNotify (+ IN > EFI_PEI_SERVICES **PeiServices,+ IN EFI_PEI_NOTIFY_DESCRIPTOR > *NotifyDesc,+ IN VOID *Ppi+ )+{+ EFI_STATUS > Status;+ EFI_BOOT_MODE BootMode;+ UINT64 MchBaseAddress;++ > Status = PeiServicesGetBootMode (&BootMode);+ ASSERT_EFI_ERROR > (Status);++ // Enable PAM regions for AP wakeup vector (resume)+ // - CPU > is finalised by PiSmmCpuDxeSmm, not FSP. So, it's safe here?+ // > TODO/TEST: coreboot does this unconditionally, vendor FWs may not (test > resume). Should we?+ // - It is certainly interesting that only PAM0, PAM5 > and PAM6 are defined for KabylakeSiliconPkg.+ // - Also note that > 0xA0000-0xFFFFF is marked "reserved" in FSP HOB - this does not mean+ // > that the memory is unusable, perhaps this is precisely because it will > contain+ // the AP wakeup vector.+ if (BootMode == BOOT_ON_S3_RESUME) > {+ MchBaseAddress = PCI_LIB_ADDRESS (0, 0, 0, 0);+ PciWrite8 > (MchBaseAddress + R_SA_PAM0, 0x30);+ PciWrite8 (MchBaseAddress + > (R_SA_PAM0 + 1), 0x33);+ PciWrite8 (MchBaseAddress + (R_SA_PAM0 + 2), > 0x33);+ PciWrite8 (MchBaseAddress + (R_SA_PAM0 + 3), 0x33);+ > PciWrite8 (MchBaseAddress + (R_SA_PAM0 + 4), 0x33);+ PciWrite8 > (MchBaseAddress + R_SA_PAM5, 0x33);+ PciWrite8 (MchBaseAddress + > R_SA_PAM6, 0x33);+ }++ //+ // Install EFI_PEI_MM_ACCESS_PPI for S3 > resume case+ //+ Status = PeiInstallSmmAccessPpi ();+ ASSERT_EFI_ERROR > (Status);++ //+ // Install EFI_PEI_MM_CONTROL_PPI for S3 resume case+ > //+ Status = PeiInstallSmmControlPpi ();+ ASSERT_EFI_ERROR (Status);++ > return Status;+}++EFI_PEI_NOTIFY_DESCRIPTOR mFspSiliconInitDoneNotifyDesc > = {+ (EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | > EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST),+ &gFspSiliconInitDonePpiGuid,+ > FspSiliconInitDoneNotify+}; /** S3 PEI module entry point@@ -25,12 > +100,10 @@ S3PeiEntryPoint ( > IN CONST EFI_PEI_SERVICES **PeiServices ) {- EFI_STATUS > Status;+ EFI_STATUS Status; - //- // Install EFI_PEI_MM_ACCESS_PPI for > S3 resume case- //- Status = PeiInstallSmmAccessPpi ();+ Status = > PeiServicesNotifyPpi (&mFspSiliconInitDoneNotifyDesc);+ ASSERT_EFI_ERROR > (Status); return Status; }diff --git > a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.inf > b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.inf > index e485eac9521f..173919bb881e 100644 > --- a/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.inf > +++ b/Features/Intel/PowerManagement/S3FeaturePkg/S3Pei/S3Pei.inf > @@ -18,10 +18,13 @@ > [LibraryClasses] PeimEntryPoint PeiServicesLib+ DebugLib > SmmAccessLib+ SmmControlLib [Packages] MdePkg/MdePkg.dec+ > IntelFsp2WrapperPkg/IntelFsp2WrapperPkg.dec > IntelSiliconPkg/IntelSiliconPkg.dec S3FeaturePkg/S3FeaturePkg.dec @@ > -31,5 +34,8 @@ > [FeaturePcd] gS3FeaturePkgTokenSpaceGuid.PcdS3FeatureEnable +[Ppis]+ > gFspSiliconInitDonePpiGuid+ [Depex]- gEfiPeiMemoryDiscoveredPpiGuid+ > TRUEdiff --git a/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h > b/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h > new file mode 100644 > index 000000000000..0d75af8e9a03 > --- /dev/null > +++ b/Platform/Intel/MinPlatformPkg/Include/AcpiS3MemoryNvData.h > @@ -0,0 +1,22 @@ > +/** @file > + Header file for NV data structure definition. > + > +Copyright (c) 2021, Baruch Binyamin Doron > +SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef __ACPI_S3_MEMORY_NV_DATA_H__ > +#define __ACPI_S3_MEMORY_NV_DATA_H__ > + > +// > +// NV data structure > +// > +typedef struct { > + UINT64 S3PeiMemBase; > + UINT64 S3PeiMemSize; > +} ACPI_S3_MEMORY; > + > +#define ACPI_S3_MEMORY_NV_NAME L"S3MemoryInfo" > + > +#endif > -- > 2.37.2 > >