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 <isaac.w.oram@intel.com>
> Sent: Tuesday, August 30, 2022 6:27 AM
> To: Benjamin Doron <benjamin.doron00@gmail.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: RE: [edk2-devel][edk2-platforms][PATCH v1 1/5] IntelSiliconPkg/Feature/PeiSmmAccessLibSmramc: Implement
> chipset support
>
> Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
>
> 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 <benjamin.doron00@gmail.com>
> Sent: Monday, August 29, 2022 1:36 PM
> To: devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Sinha, Ankit <ankit.sinha@intel.com>; Ni, Ray
> <ray.ni@intel.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>
> 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 <nathaniel.l.desimone@intel.com>
> Cc: Ankit Sinha <ankit.sinha@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Rangasai V Chaganty <rangasai.v.chaganty@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Signed-off-by: Benjamin Doron <benjamin.doron00@gmail.com>
> ---
> .../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.<BR>+ SPDX-License-Identifier: BSD-2-Clause-Patent++**/+#include <Library/BaseMemoryLib.h>+#include
> <Library/DebugLib.h>+#include <Library/MemoryAllocationLib.h>+#include <Library/PciSegmentLib.h>+#include
> <Library/PeiServicesLib.h>+#include <Library/HobLib.h>+#include <Uefi/UefiBaseType.h>+#include
> <Guid/SmramMemoryReserve.h>++#include <Ppi/MmAccess.h>+#include <IndustryStandard/Pci22.h>++#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+ - <b>Introduction</b> \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.<BR>+#
> +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