Ray, My primary platform for firmware development is the Skylake laptop I've submitted to the tree. I have a Tigerlake laptop too, but my porting efforts there are generally too WIP to test patches anywhere yet. Regardless, this patch series is intended to support as many platforms as possible. It's my understanding that generally, we're supporting all current client platforms, though there may be silicon-specific differences, particularly in the shim library. Best regards, Benjamin On Mon, 29 Aug 2022 at 20:26, Ni, Ray wrote: > Doran, > Which platform are you using? I thought those platforms are quite old and > no one is using them. > > > -----Original Message----- > > From: Oram, Isaac W > > Sent: Tuesday, August 30, 2022 6:27 AM > > To: Benjamin Doron ; devel@edk2.groups.io > > Cc: Desimone, Nathaniel L ; Sinha, > Ankit ; Ni, Ray > > ; Chaganty, Rangasai V > > Subject: RE: [edk2-devel][edk2-platforms][PATCH v1 1/5] > IntelSiliconPkg/Feature/PeiSmmAccessLibSmramc: Implement > > chipset support > > > > Reviewed-by: Isaac Oram > > > > I would prefer to see contents of sections indented, but it is a nit. > > It might be slightly better to have PcdsFixedAtBuild type PCD for the > register information, but this is pretty stable HW, so it > > is ok. > > > > 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 ; Ni, Ray > > ; Chaganty, Rangasai V ; > Oram, Isaac W > > Subject: [edk2-devel][edk2-platforms][PATCH v1 1/5] > IntelSiliconPkg/Feature/PeiSmmAccessLibSmramc: Implement > > chipset support > > > > SMRAM must be opened to retrieve the lockbox for S3, and SMM > communication depends on this PPI. For security > > purposes, SMRAM lock must be performed before EndOfPei (although FSP > notify performs lockdown too). > > > > It seems to me that this library is generic and applicable to all Intel > platforms in the tree using the MCH SMRAMC register. > > > > Cc: Nate DeSimone > > Cc: Ankit Sinha > > Cc: Ray Ni > > Cc: Rangasai V Chaganty > > Cc: Isaac Oram > > Signed-off-by: Benjamin Doron > > --- > > .../PeiSmmAccessLibSmramc/PeiSmmAccessLib.c | 430 ++++++++++++++++++ > > .../PeiSmmAccessLibSmramc/PeiSmmAccessLib.inf | 41 ++ > > 2 files changed, 471 insertions(+) > > create mode 100644 > > > Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.c > > create mode 100644 > > > Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.inf > > > > diff --git > a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.c > > > b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.c > > new file mode 100644 > > index 000000000000..5b472bf86abf > > --- /dev/null > > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce > > +++ ssLibSmramc/PeiSmmAccessLib.c > > @@ -0,0 +1,430 @@ > > +/** @file+ This is to publish the SMM Access 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 > > ++#include +#include > ++#define > > SMM_ACCESS_PRIVATE_DATA_SIGNATURE SIGNATURE_32 ('4', '5', 's', > 'a')++///+/// Private data+///+typedef struct {+ > > UINTN Signature;+ EFI_HANDLE Handle;+ > EFI_PEI_MM_ACCESS_PPI SmmAccess;+ //+ // Local Data for > > SMM Access interface goes here+ //+ UINTN > NumberRegions;+ EFI_SMRAM_DESCRIPTOR *SmramDesc;+} > > SMM_ACCESS_PRIVATE_DATA;++#define SMM_ACCESS_PRIVATE_DATA_FROM_THIS(a) > \+ CR (a, \+ > > SMM_ACCESS_PRIVATE_DATA, \+ SmmAccess, \+ > SMM_ACCESS_PRIVATE_DATA_SIGNATURE \+ )++//+// > > Common registers:+//+// DEVICE 0 (Memory Controller Hub)+//+#define > SA_MC_BUS 0x00+#define SA_MC_DEV > > 0x00+#define SA_MC_FUN 0x00+///+/// Description:+/// The > SMRAMC register controls how accesses to Compatible > > SMRAM spaces are treated. The Open, Close and Lock bits function only > when G_SMRAME bit is set to 1. Also, the Open > > bit must be reset before the Lock bit is set.+///+#define R_SA_SMRAMC > (0x88)+#define B_SA_SMRAMC_D_LCK_MASK > > (0x10)+#define B_SA_SMRAMC_D_CLS_MASK (0x20)+#define > B_SA_SMRAMC_D_OPEN_MASK (0x40)++/**+ This > > routine accepts a request to "open" a region of SMRAM. The+ region > could be legacy ABSEG, HSEG, or TSEG near top of > > physical memory.+ The use of "open" means that the memory is visible > from all PEIM+ and SMM agents.++ @param[in] > > PeiServices - General purpose services available to every > PEIM.+ @param[in] This - Pointer to the SMM Access > > Interface.+ @param[in] DescriptorIndex - Region of SMRAM to > Open.++ @retval EFI_SUCCESS - The region was > > successfully opened.+ @retval EFI_DEVICE_ERROR - The region > could not be opened because locked by+ > > chipset.+ @retval EFI_INVALID_PARAMETER - The descriptor index was > out of bounds.+**/+EFI_STATUS+EFIAPI+Open > > (+ IN EFI_PEI_SERVICES **PeiServices,+ IN > EFI_PEI_MM_ACCESS_PPI *This,+ IN UINTN > > DescriptorIndex+ )+{+ SMM_ACCESS_PRIVATE_DATA *SmmAccess;+ UINT8 > Index;+ UINT64 Address;+ > > UINT8 SmramControl;++ SmmAccess = > SMM_ACCESS_PRIVATE_DATA_FROM_THIS (This);+ if > > (DescriptorIndex >= SmmAccess->NumberRegions) {+ DEBUG ((DEBUG_WARN, > "SMRAM region out of range\n"));++ > > return EFI_INVALID_PARAMETER;+ } else if > (SmmAccess->SmramDesc[DescriptorIndex].RegionState & > > EFI_SMRAM_LOCKED) {+ //+ // Cannot open a "locked" region+ //+ > DEBUG ((DEBUG_WARN, "Cannot open a locked > > SMRAM region\n"));++ return EFI_DEVICE_ERROR;+ }++ ///+ /// BEGIN > CHIPSET CODE+ ///+ ///+ /// SMRAM register > > is PCI 0:0:0:88, SMRAMC (8 bit)+ ///+ Address = > PCI_SEGMENT_LIB_ADDRESS (0, SA_MC_BUS, SA_MC_DEV, SA_MC_FUN, > > R_SA_SMRAMC);+ SmramControl = PciSegmentRead8 (Address);+ ///+ /// > Is SMRAM locked?+ ///+ if ((SmramControl > > & B_SA_SMRAMC_D_LCK_MASK) != 0) {+ ///+ /// Cannot Open a locked > region+ ///+ for (Index = 0; Index < > > SmmAccess->NumberRegions; Index++) {+ > SmmAccess->SmramDesc[Index].RegionState |= > > EFI_SMRAM_LOCKED;+ }+ DEBUG ((DEBUG_WARN, "Cannot open a locked > SMRAM region\n"));+ return > > EFI_DEVICE_ERROR;+ }+ ///+ /// Open SMRAM region+ ///+ SmramControl > |= B_SA_SMRAMC_D_OPEN_MASK;+ > > SmramControl &= ~(B_SA_SMRAMC_D_CLS_MASK);++ PciSegmentWrite8 (Address, > SmramControl);+ ///+ /// END > > CHIPSET CODE+ ///++ SmmAccess->SmramDesc[DescriptorIndex].RegionState > &= (UINT64) ~(EFI_SMRAM_CLOSED | > > EFI_ALLOCATED);+ SmmAccess->SmramDesc[DescriptorIndex].RegionState |= > (UINT64) EFI_SMRAM_OPEN;+ > > SmmAccess->SmmAccess.OpenState = TRUE;+ return EFI_SUCCESS;+}++/**+ > This routine accepts a request to "close" a > > region of SMRAM. This is valid for+ compatible SMRAM region.++ > @param[in] PeiServices - General purpose services > > available to every PEIM.+ @param[in] This - Pointer to > the SMM Access Interface.+ @param[in] DescriptorIndex > > - Region of SMRAM to Close.++ @retval EFI_SUCCESS - The > region was successfully closed.+ @retval > > EFI_DEVICE_ERROR - The region could not be closed because locked > by+ chipset.+ @retval > > EFI_INVALID_PARAMETER - The descriptor index was out of > bounds.+**/+EFI_STATUS+EFIAPI+Close (+ IN > > EFI_PEI_SERVICES **PeiServices,+ IN EFI_PEI_MM_ACCESS_PPI > *This,+ IN UINTN DescriptorIndex+ )+{+ > > SMM_ACCESS_PRIVATE_DATA *SmmAccess;+ BOOLEAN > OpenState;+ UINT8 Index;+ UINT64 > > Address;+ UINT8 SmramControl;++ SmmAccess = > SMM_ACCESS_PRIVATE_DATA_FROM_THIS (This);+ if > > (DescriptorIndex >= SmmAccess->NumberRegions) {+ DEBUG ((DEBUG_WARN, > "SMRAM region out of range\n"));++ > > return EFI_INVALID_PARAMETER;+ } else if > (SmmAccess->SmramDesc[DescriptorIndex].RegionState & > > EFI_SMRAM_LOCKED) {+ //+ // Cannot close a "locked" region+ > //+ DEBUG ((DEBUG_WARN, "Cannot close a locked > > SMRAM region\n"));++ return EFI_DEVICE_ERROR;+ }++ if > (SmmAccess->SmramDesc[DescriptorIndex].RegionState & > > EFI_SMRAM_CLOSED) {+ return EFI_DEVICE_ERROR;+ }++ ///+ /// BEGIN > CHIPSET CODE+ ///+ ///+ /// SMRAM > > register is PCI 0:0:0:88, SMRAMC (8 bit)+ ///+ Address = > PCI_SEGMENT_LIB_ADDRESS (0, SA_MC_BUS, SA_MC_DEV, > > SA_MC_FUN, R_SA_SMRAMC);+ SmramControl = PciSegmentRead8 (Address);+ > ///+ /// Is SMRAM locked?+ ///+ if > > ((SmramControl & B_SA_SMRAMC_D_LCK_MASK) != 0) {+ ///+ /// Cannot > Close a locked region+ ///+ for (Index = 0; > > Index < SmmAccess->NumberRegions; Index++) {+ > SmmAccess->SmramDesc[Index].RegionState |= > > EFI_SMRAM_LOCKED;+ }+ DEBUG ((DEBUG_WARN, "Cannot close a locked > SMRAM region\n"));+ return > > EFI_DEVICE_ERROR;+ }+ ///+ /// Close SMRAM region+ ///+ > SmramControl &= ~(B_SA_SMRAMC_D_OPEN_MASK);++ > > PciSegmentWrite8 (Address, SmramControl);+ ///+ /// END CHIPSET CODE+ > ///++ SmmAccess- > > >SmramDesc[DescriptorIndex].RegionState &= (UINT64) ~EFI_SMRAM_OPEN;+ > SmmAccess- > > >SmramDesc[DescriptorIndex].RegionState |= (UINT64) (EFI_SMRAM_CLOSED | > EFI_ALLOCATED);++ //+ // Find out if any > > regions are still open+ //+ OpenState = FALSE;+ for (Index = 0; Index > < SmmAccess->NumberRegions; Index++) {+ if > > ((SmmAccess->SmramDesc[Index].RegionState & EFI_SMRAM_OPEN) == > EFI_SMRAM_OPEN) {+ OpenState = > > TRUE;+ }+ }++ SmmAccess->SmmAccess.OpenState = OpenState;+ return > EFI_SUCCESS;+}++/**+ This routine accepts a > > request to "lock" SMRAM. The+ region could be legacy AB or TSEG near > top of physical memory.+ The use of "lock" means > > that the memory can no longer be opened+ to PEIM.++ @param[in] > PeiServices - General purpose services available > > to every PEIM.+ @param[in] This - Pointer to the SMM > Access Interface.+ @param[in] DescriptorIndex - Region > > of SMRAM to Lock.++ @retval EFI_SUCCESS - The region was > successfully locked.+ @retval EFI_DEVICE_ERROR - > > The region could not be locked because at least+ > one range is still open.+ @retval > > EFI_INVALID_PARAMETER - The descriptor index was out of > bounds.+**/+EFI_STATUS+EFIAPI+Lock (+ IN > > EFI_PEI_SERVICES **PeiServices,+ IN EFI_PEI_MM_ACCESS_PPI > *This,+ IN UINTN DescriptorIndex+ )+{+ > > SMM_ACCESS_PRIVATE_DATA *SmmAccess;+ UINT64 Address;+ > UINT8 SmramControl;++ SmmAccess = > > SMM_ACCESS_PRIVATE_DATA_FROM_THIS (This);+ if (DescriptorIndex >= > SmmAccess->NumberRegions) {+ DEBUG > > ((DEBUG_WARN, "SMRAM region out of range\n"));++ return > EFI_INVALID_PARAMETER;+ } else if (SmmAccess- > > >SmmAccess.OpenState) {+ DEBUG ((DEBUG_WARN, "Cannot lock SMRAM when > SMRAM regions are still open\n"));++ > > return EFI_DEVICE_ERROR;+ }++ > SmmAccess->SmramDesc[DescriptorIndex].RegionState |= (UINT64) > > EFI_SMRAM_LOCKED;+ SmmAccess->SmmAccess.LockState = TRUE;++ ///+ /// > BEGIN CHIPSET CODE+ ///+ ///+ /// > > SMRAM register is PCI 0:0:0:88, SMRAMC (8 bit)+ ///+ Address = > PCI_SEGMENT_LIB_ADDRESS (0, SA_MC_BUS, > > SA_MC_DEV, SA_MC_FUN, R_SA_SMRAMC);+ SmramControl = PciSegmentRead8 > (Address);++ ///+ /// Lock the > > SMRAM+ ///+ SmramControl |= B_SA_SMRAMC_D_LCK_MASK;++ > PciSegmentWrite8 (Address, SmramControl);+ ///+ > > /// END CHIPSET CODE+ ///++ return EFI_SUCCESS;+}++/**+ This routine > services a user request to discover the > > SMRAM+ capabilities of this platform. This will report the possible+ > ranges that are possible for SMRAM access, based > > upon the+ memory controller capabilities.++ @param[in] PeiServices > - General purpose services available to every > > PEIM.+ @param[in] This - Pointer to the SMRAM Access > Interface.+ @param[in, out] SmramMapSize - Pointer to > > the variable containing size of the+ > buffer to contain the description information.+ @param[in, out] > > SmramMap - Buffer containing the data describing the Smram+ > region descriptors.++ @retval > > EFI_BUFFER_TOO_SMALL - The user did not provide a sufficient buffer.+ > @retval EFI_SUCCESS - The user provided a > > sufficiently-sized buffer.+**/+EFI_STATUS+EFIAPI+GetCapabilities (+ IN > EFI_PEI_SERVICES **PeiServices,+ IN > > EFI_PEI_MM_ACCESS_PPI *This,+ IN OUT UINTN > *SmramMapSize,+ IN OUT EFI_SMRAM_DESCRIPTOR > > *SmramMap+ )+{+ EFI_STATUS Status;+ > SMM_ACCESS_PRIVATE_DATA *SmmAccess;+ UINTN > > NecessaryBufferSize;++ SmmAccess = > SMM_ACCESS_PRIVATE_DATA_FROM_THIS (This);+ NecessaryBufferSize = > > SmmAccess->NumberRegions * sizeof (EFI_SMRAM_DESCRIPTOR);+ if > (*SmramMapSize < NecessaryBufferSize) {+ > > DEBUG ((DEBUG_WARN, "SMRAM Map Buffer too small\n"));++ Status = > EFI_BUFFER_TOO_SMALL;+ } else {+ > > CopyMem (SmramMap, SmmAccess->SmramDesc, NecessaryBufferSize);+ > Status = EFI_SUCCESS;+ }++ *SmramMapSize > > = NecessaryBufferSize;+ return Status;+}++/**+ This function is to > install an SMM Access PPI+ - Introduction \n+ > > An API to install an instance of EFI_PEI_MM_ACCESS_PPI. This PPI is > commonly used to control SMM mode memory access > > for S3 resume.++ @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+PeiInstallSmmAccessPpi (+ VOID+ )+{+ > EFI_STATUS Status;+ UINTN > > Index;+ EFI_PEI_PPI_DESCRIPTOR *PpiList;+ > EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock;+ > > SMM_ACCESS_PRIVATE_DATA *SmmAccessPrivate;+ VOID > *HobList;++ //+ // Initialize private data+ > > //+ SmmAccessPrivate = AllocateZeroPool (sizeof > (*SmmAccessPrivate));+ ASSERT (SmmAccessPrivate != NULL);+ if > > (SmmAccessPrivate == NULL) {+ return EFI_OUT_OF_RESOURCES;+ }+ > PpiList = AllocateZeroPool (sizeof > > (*PpiList));+ ASSERT (PpiList != NULL);+ if (PpiList == NULL) {+ > return EFI_OUT_OF_RESOURCES;+ }++ > > SmmAccessPrivate->Signature = SMM_ACCESS_PRIVATE_DATA_SIGNATURE;+ > SmmAccessPrivate->Handle = NULL;++ > > //+ // Get Hob list+ //+ HobList = GetFirstGuidHob > (&gEfiSmmSmramMemoryGuid);+ if (HobList == NULL) {+ DEBUG > > ((DEBUG_WARN, "SmramMemoryReserve HOB not found\n"));+ return > EFI_NOT_FOUND;+ }++ DescriptorBlock = > > (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *) ((UINT8 *) HobList + sizeof > (EFI_HOB_GUID_TYPE));++ //+ // Alloc space for > > SmmAccessPrivate->SmramDesc+ //+ SmmAccessPrivate->SmramDesc = > AllocateZeroPool ((DescriptorBlock- > > >NumberOfSmmReservedRegions) * sizeof (EFI_SMRAM_DESCRIPTOR));+ if > (SmmAccessPrivate->SmramDesc == NULL) > > {+ DEBUG ((DEBUG_WARN, "Alloc SmmAccessPrivate->SmramDesc > fail.\n"));+ return EFI_OUT_OF_RESOURCES;+ }++ > > DEBUG ((DEBUG_INFO, "Alloc SmmAccessPrivate->SmramDesc success.\n"));++ > //+ // use the hob to publish SMRAM > > capabilities+ //+ for (Index = 0; Index < > DescriptorBlock->NumberOfSmmReservedRegions; Index++) {+ > > SmmAccessPrivate->SmramDesc[Index].PhysicalStart = > DescriptorBlock->Descriptor[Index].PhysicalStart;+ > > SmmAccessPrivate->SmramDesc[Index].CpuStart = > DescriptorBlock->Descriptor[Index].CpuStart;+ > > SmmAccessPrivate->SmramDesc[Index].PhysicalSize = > DescriptorBlock->Descriptor[Index].PhysicalSize;+ > > SmmAccessPrivate->SmramDesc[Index].RegionState = > DescriptorBlock->Descriptor[Index].RegionState;+ }++ > > SmmAccessPrivate->NumberRegions = Index;+ > SmmAccessPrivate->SmmAccess.Open = Open;+ > > SmmAccessPrivate->SmmAccess.Close = Close;+ > SmmAccessPrivate->SmmAccess.Lock = Lock;+ > > SmmAccessPrivate->SmmAccess.GetCapabilities = GetCapabilities;+ > SmmAccessPrivate->SmmAccess.LockState = > > FALSE;+ SmmAccessPrivate->SmmAccess.OpenState = FALSE;++ //+ // > Install PPI+ //+ PpiList->Flags = > > (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST);+ > PpiList->Guid = > > &gEfiPeiMmAccessPpiGuid;+ PpiList->Ppi = > &SmmAccessPrivate->SmmAccess;++ Status = PeiServicesInstallPpi > > (PpiList);+ ASSERT_EFI_ERROR (Status);++ return EFI_SUCCESS;+}diff > --git > > > a/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.inf > > > b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAccessLibSmramc/PeiSmmAccessLib.inf > > new file mode 100644 > > index 000000000000..916346aacff3 > > --- /dev/null > > +++ b/Silicon/Intel/IntelSiliconPkg/Feature/SmmAccess/Library/PeiSmmAcce > > +++ ssLibSmramc/PeiSmmAccessLib.inf > > @@ -0,0 +1,41 @@ > > +## @file+# Library description file for the SmmAccess PPI+#+# Copyright > > +(c) 2019, Intel Corporation. All rights reserved.
+# > > +SPDX-License-Identifier: > > +BSD-2-Clause-Patent+#+##++[Defines]+INF_VERSION = 0x00010017+BASE_NAME > > += PeiSmmAccessLibSmramc+FILE_GUID = > > +3D28FD4B-F46F-4E24-88AA-9DA09C51BE87+VERSION_STRING = 1.0+MODULE_TYPE = > PEIM+LIBRARY_CLASS = > > > SmmAccessLib+++[LibraryClasses]+BaseMemoryLib+MemoryAllocationLib+DebugLib+HobLib+PciSegmentLib+PeiServicesL > > > ib+++[Packages]+MdePkg/MdePkg.dec+IntelSiliconPkg/IntelSiliconPkg.dec+++[Sources]+PeiSmmAccessLib.c+++[Ppis]+gE > > fiPeiMmAccessPpiGuid ## PRODUCES+++[Guids]+gEfiSmmSmramMemoryGuid-- > > 2.37.2 > >