From 04c36d5a1b6cf17de62e2526c968661883567d1f Mon Sep 17 00:00:00 2001 From: Jiaxin Wu Date: Fri, 26 Apr 2024 19:17:10 +0800 Subject: OvmfPkg: Refine SmmAccess implementation This patch refines the SmmAccess implementation: 1. SmramMap will be retrieved from the gEfiSmmSmramMemoryGuid instead of original from the TSEG Memory Base register. 2. Remove the gEfiAcpiVariableGuid creation, thus the DESCRIPTOR_INDEX definition can be also cleaned. 3. The gEfiAcpiVariableGuid HOB is moved to the OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf. Cc: Ard Biesheuvel Cc: Jiewen Yao Cc: Gerd Hoffmann Cc: Ray Ni Signed-off-by: Jiaxin Wu Tested-by: Gerd Hoffmann Acked-by: Gerd Hoffmann Acked-by: Jiewen Yao --- OvmfPkg/Library/PlatformInitLib/MemDetect.c | 10 ++ .../Library/PlatformInitLib/PlatformInitLib.inf | 1 + OvmfPkg/SmmAccess/SmmAccess2Dxe.c | 4 +- OvmfPkg/SmmAccess/SmmAccess2Dxe.inf | 5 + OvmfPkg/SmmAccess/SmmAccessPei.c | 116 +++++++-------------- OvmfPkg/SmmAccess/SmmAccessPei.inf | 11 +- OvmfPkg/SmmAccess/SmramInternal.c | 72 ++++--------- OvmfPkg/SmmAccess/SmramInternal.h | 19 +--- 8 files changed, 87 insertions(+), 151 deletions(-) diff --git a/OvmfPkg/Library/PlatformInitLib/MemDetect.c b/OvmfPkg/Library/PlatformInitLib/MemDetect.c index 19abb16e79..e64c0ee324 100644 --- a/OvmfPkg/Library/PlatformInitLib/MemDetect.c +++ b/OvmfPkg/Library/PlatformInitLib/MemDetect.c @@ -42,6 +42,8 @@ Module Name: #include #include + +#include #include #define MEGABYTE_SHIFT 20 @@ -1003,6 +1005,7 @@ CreateSmmSmramMemoryHob ( UINT8 SmramRanges; EFI_PEI_HOB_POINTERS Hob; EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *SmramHobDescriptorBlock; + VOID *GuidHob; SmramRanges = 2; BufferSize = sizeof (EFI_SMRAM_HOB_DESCRIPTOR_BLOCK) + (SmramRanges - 1) * sizeof (EFI_SMRAM_DESCRIPTOR); @@ -1025,6 +1028,13 @@ CreateSmmSmramMemoryHob ( SmramHobDescriptorBlock->Descriptor[0].PhysicalSize = EFI_PAGE_SIZE; SmramHobDescriptorBlock->Descriptor[0].RegionState = EFI_SMRAM_CLOSED | EFI_CACHEABLE | EFI_ALLOCATED; + // + // 1.1 Create gEfiAcpiVariableGuid according SmramHobDescriptorBlock->Descriptor[0] since it's used in S3 resume. + // + GuidHob = BuildGuidHob (&gEfiAcpiVariableGuid, sizeof (EFI_SMRAM_DESCRIPTOR)); + ASSERT (GuidHob != NULL); + CopyMem (GuidHob, &SmramHobDescriptorBlock->Descriptor[0], sizeof (EFI_SMRAM_DESCRIPTOR)); + // // 2. Create second SMRAM descriptor, which is free and will be used by SMM foundation. // diff --git a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf index 2bb1c0296f..21e6efa5e0 100644 --- a/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf +++ b/OvmfPkg/Library/PlatformInitLib/PlatformInitLib.inf @@ -58,6 +58,7 @@ [Guids] gEfiSmmSmramMemoryGuid + gEfiAcpiVariableGuid [Pcd] gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c index 4b9e6df37f..3371592de7 100644 --- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.c +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.c @@ -6,7 +6,7 @@ driver. Copyright (C) 2013, 2015, Red Hat, Inc.
- Copyright (c) 2009 - 2010, Intel Corporation. All rights reserved.
+ Copyright (c) 2009 - 2024, Intel Corporation. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent @@ -115,8 +115,6 @@ SmmAccess2DxeGetCapabilities ( ) { return SmramAccessGetCapabilities ( - This->LockState, - This->OpenState, SmramMapSize, SmramMap ); diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf index d86381d0fb..d9f01a13c4 100644 --- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf +++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf @@ -5,6 +5,7 @@ # driver. # # Copyright (C) 2013, 2015, Red Hat, Inc. +# Copyright (c) 2024 Intel Corporation. # # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -41,6 +42,7 @@ PciLib UefiBootServicesTableLib UefiDriverEntryPoint + HobLib [Protocols] gEfiSmmAccess2ProtocolGuid ## PRODUCES @@ -48,6 +50,9 @@ [FeaturePcd] gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire +[Guids] + gEfiSmmSmramMemoryGuid # ALWAYS_CONSUMED + [Pcd] gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c index 0e57b7804c..ded22f035d 100644 --- a/OvmfPkg/SmmAccess/SmmAccessPei.c +++ b/OvmfPkg/SmmAccess/SmmAccessPei.c @@ -3,25 +3,21 @@ A PEIM with the following responsibilities: - verify & configure the Q35 TSEG in the entry point, - - provide SMRAM access by producing PEI_SMM_ACCESS_PPI, - - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose - it via the gEfiAcpiVariableGuid GUID HOB. + - provide SMRAM access by producing PEI_SMM_ACCESS_PPI This PEIM runs from RAM, so we can write to variables with static storage duration. Copyright (C) 2013, 2015, Red Hat, Inc.
- Copyright (c) 2010, Intel Corporation. All rights reserved.
+ Copyright (c) 2010 - 2024, Intel Corporation. All rights reserved.
SPDX-License-Identifier: BSD-2-Clause-Patent **/ -#include #include #include #include -#include #include #include #include @@ -64,7 +60,17 @@ SmmAccessPeiOpen ( IN UINTN DescriptorIndex ) { - if (DescriptorIndex >= DescIdxCount) { + EFI_HOB_GUID_TYPE *GuidHob; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; + + // + // Get the number of regions in the system that can be usable for SMRAM + // + GuidHob = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid); + DescriptorBlock = GET_GUID_HOB_DATA (GuidHob); + ASSERT (DescriptorBlock); + + if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) { return EFI_INVALID_PARAMETER; } @@ -102,7 +108,17 @@ SmmAccessPeiClose ( IN UINTN DescriptorIndex ) { - if (DescriptorIndex >= DescIdxCount) { + EFI_HOB_GUID_TYPE *GuidHob; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; + + // + // Get the number of regions in the system that can be usable for SMRAM + // + GuidHob = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid); + DescriptorBlock = GET_GUID_HOB_DATA (GuidHob); + ASSERT (DescriptorBlock); + + if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) { return EFI_INVALID_PARAMETER; } @@ -139,7 +155,17 @@ SmmAccessPeiLock ( IN UINTN DescriptorIndex ) { - if (DescriptorIndex >= DescIdxCount) { + EFI_HOB_GUID_TYPE *GuidHob; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; + + // + // Get the number of regions in the system that can be usable for SMRAM + // + GuidHob = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid); + DescriptorBlock = GET_GUID_HOB_DATA (GuidHob); + ASSERT (DescriptorBlock); + + if (DescriptorIndex >= DescriptorBlock->NumberOfSmmReservedRegions) { return EFI_INVALID_PARAMETER; } @@ -178,8 +204,6 @@ SmmAccessPeiGetCapabilities ( ) { return SmramAccessGetCapabilities ( - This->LockState, - This->OpenState, SmramMapSize, SmramMap ); @@ -240,14 +264,10 @@ SmmAccessPeiEntryPoint ( IN CONST EFI_PEI_SERVICES **PeiServices ) { - UINT16 HostBridgeDevId; - UINT8 EsmramcVal; - UINT8 RegMask8; - UINT32 TopOfLowRam, TopOfLowRamMb; - EFI_STATUS Status; - UINTN SmramMapSize; - EFI_SMRAM_DESCRIPTOR SmramMap[DescIdxCount]; - VOID *GuidHob; + UINT16 HostBridgeDevId; + UINT8 EsmramcVal; + UINT8 RegMask8; + UINT32 TopOfLowRam, TopOfLowRamMb; // // This module should only be included if SMRAM support is required. @@ -356,65 +376,7 @@ SmmAccessPeiEntryPoint ( MCH_SMRAM_G_SMRAME ); - // - // Create the GUID HOB and point it to the first SMRAM range. - // GetStates (&mAccess.LockState, &mAccess.OpenState); - SmramMapSize = sizeof SmramMap; - Status = SmramAccessGetCapabilities ( - mAccess.LockState, - mAccess.OpenState, - &SmramMapSize, - SmramMap - ); - ASSERT_EFI_ERROR (Status); - - DEBUG_CODE_BEGIN (); - { - UINTN Count; - UINTN Idx; - - Count = SmramMapSize / sizeof SmramMap[0]; - DEBUG (( - DEBUG_VERBOSE, - "%a: SMRAM map follows, %d entries\n", - __func__, - (INT32)Count - )); - DEBUG (( - DEBUG_VERBOSE, - "% 20a % 20a % 20a % 20a\n", - "PhysicalStart(0x)", - "PhysicalSize(0x)", - "CpuStart(0x)", - "RegionState(0x)" - )); - for (Idx = 0; Idx < Count; ++Idx) { - DEBUG (( - DEBUG_VERBOSE, - "% 20Lx % 20Lx % 20Lx % 20Lx\n", - SmramMap[Idx].PhysicalStart, - SmramMap[Idx].PhysicalSize, - SmramMap[Idx].CpuStart, - SmramMap[Idx].RegionState - )); - } - } - DEBUG_CODE_END (); - - GuidHob = BuildGuidHob ( - &gEfiAcpiVariableGuid, - sizeof SmramMap[DescIdxSmmS3ResumeState] - ); - if (GuidHob == NULL) { - return EFI_OUT_OF_RESOURCES; - } - - CopyMem ( - GuidHob, - &SmramMap[DescIdxSmmS3ResumeState], - sizeof SmramMap[DescIdxSmmS3ResumeState] - ); // // SmramAccessLock() depends on "mQ35SmramAtDefaultSmbase"; init the latter diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf index 1698c4ce6c..1e7cedf290 100644 --- a/OvmfPkg/SmmAccess/SmmAccessPei.inf +++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf @@ -2,11 +2,10 @@ # A PEIM with the following responsibilities: # # - provide SMRAM access by producing PEI_SMM_ACCESS_PPI, -# - verify & configure the Q35 TSEG in the entry point, -# - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose -# it via the gEfiAcpiVariableGuid GUIDed HOB. +# - verify & configure the Q35 TSEG in the entry point. # # Copyright (C) 2013, 2015, Red Hat, Inc. +# Copyright (c) 2024 Intel Corporation. # # SPDX-License-Identifier: BSD-2-Clause-Patent # @@ -36,9 +35,6 @@ MdePkg/MdePkg.dec OvmfPkg/OvmfPkg.dec -[Guids] - gEfiAcpiVariableGuid - [LibraryClasses] BaseLib BaseMemoryLib @@ -57,6 +53,9 @@ gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes +[Guids] + gEfiSmmSmramMemoryGuid # ALWAYS_CONSUMED + [Ppis] gPeiSmmAccessPpiGuid ## PRODUCES diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c index d391ddc9ae..ff67d302a2 100644 --- a/OvmfPkg/SmmAccess/SmramInternal.c +++ b/OvmfPkg/SmmAccess/SmramInternal.c @@ -3,12 +3,11 @@ Functions and types shared by the SMM accessor PEI and DXE modules. Copyright (C) 2015, Red Hat, Inc. + Copyright (c) 2024 Intel Corporation. SPDX-License-Identifier: BSD-2-Clause-Patent **/ - -#include #include #include #include @@ -166,68 +165,43 @@ SmramAccessLock ( EFI_STATUS SmramAccessGetCapabilities ( - IN BOOLEAN LockState, - IN BOOLEAN OpenState, IN OUT UINTN *SmramMapSize, IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap ) { - UINTN OriginalSize; - UINT32 TsegMemoryBaseMb, TsegMemoryBase; - UINT64 CommonRegionState; - UINT8 TsegSizeBits; - - OriginalSize = *SmramMapSize; - *SmramMapSize = DescIdxCount * sizeof *SmramMap; - if (OriginalSize < *SmramMapSize) { - return EFI_BUFFER_TOO_SMALL; - } + UINTN BufferSize; + EFI_HOB_GUID_TYPE *GuidHob; + EFI_SMRAM_HOB_DESCRIPTOR_BLOCK *DescriptorBlock; + UINTN Index; // - // Read the TSEG Memory Base register. + // Get Hob list // - TsegMemoryBaseMb = PciRead32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB)); - TsegMemoryBase = (TsegMemoryBaseMb >> MCH_TSEGMB_MB_SHIFT) << 20; + GuidHob = GetFirstGuidHob (&gEfiSmmSmramMemoryGuid); + DescriptorBlock = GET_GUID_HOB_DATA (GuidHob); + ASSERT (DescriptorBlock); - // - // Precompute the region state bits that will be set for all regions. - // - CommonRegionState = (OpenState ? EFI_SMRAM_OPEN : EFI_SMRAM_CLOSED) | - (LockState ? EFI_SMRAM_LOCKED : 0) | - EFI_CACHEABLE; + BufferSize = DescriptorBlock->NumberOfSmmReservedRegions * sizeof (EFI_SMRAM_DESCRIPTOR); - // - // The first region hosts an SMM_S3_RESUME_STATE object. It is located at the - // start of TSEG. We round up the size to whole pages, and we report it as - // EFI_ALLOCATED, so that the SMM_CORE stays away from it. - // - SmramMap[DescIdxSmmS3ResumeState].PhysicalStart = TsegMemoryBase; - SmramMap[DescIdxSmmS3ResumeState].CpuStart = TsegMemoryBase; - SmramMap[DescIdxSmmS3ResumeState].PhysicalSize = - EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (sizeof (SMM_S3_RESUME_STATE))); - SmramMap[DescIdxSmmS3ResumeState].RegionState = - CommonRegionState | EFI_ALLOCATED; + if (*SmramMapSize < BufferSize) { + *SmramMapSize = BufferSize; + return EFI_BUFFER_TOO_SMALL; + } // - // Get the TSEG size bits from the ESMRAMC register. + // Update SmramMapSize to real return SMRAM map size // - TsegSizeBits = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)) & - MCH_ESMRAMC_TSEG_MASK; + *SmramMapSize = BufferSize; // - // The second region is the main one, following the first. + // Use the hob to publish SMRAM capabilities // - SmramMap[DescIdxMain].PhysicalStart = - SmramMap[DescIdxSmmS3ResumeState].PhysicalStart + - SmramMap[DescIdxSmmS3ResumeState].PhysicalSize; - SmramMap[DescIdxMain].CpuStart = SmramMap[DescIdxMain].PhysicalStart; - SmramMap[DescIdxMain].PhysicalSize = - (TsegSizeBits == MCH_ESMRAMC_TSEG_8MB ? SIZE_8MB : - TsegSizeBits == MCH_ESMRAMC_TSEG_2MB ? SIZE_2MB : - TsegSizeBits == MCH_ESMRAMC_TSEG_1MB ? SIZE_1MB : - mQ35TsegMbytes * SIZE_1MB) - - SmramMap[DescIdxSmmS3ResumeState].PhysicalSize; - SmramMap[DescIdxMain].RegionState = CommonRegionState; + for (Index = 0; Index < DescriptorBlock->NumberOfSmmReservedRegions; Index++) { + SmramMap[Index].PhysicalStart = DescriptorBlock->Descriptor[Index].PhysicalStart; + SmramMap[Index].CpuStart = DescriptorBlock->Descriptor[Index].CpuStart; + SmramMap[Index].PhysicalSize = DescriptorBlock->Descriptor[Index].PhysicalSize; + SmramMap[Index].RegionState = DescriptorBlock->Descriptor[Index].RegionState; + } return EFI_SUCCESS; } diff --git a/OvmfPkg/SmmAccess/SmramInternal.h b/OvmfPkg/SmmAccess/SmramInternal.h index da5b7bbca1..aa23c6dff8 100644 --- a/OvmfPkg/SmmAccess/SmramInternal.h +++ b/OvmfPkg/SmmAccess/SmramInternal.h @@ -3,6 +3,7 @@ Functions and types shared by the SMM accessor PEI and DXE modules. Copyright (C) 2015, Red Hat, Inc. + Copyright (c) 2024 Intel Corporation. SPDX-License-Identifier: BSD-2-Clause-Patent @@ -10,20 +11,8 @@ #include -// -// We'll have two SMRAM ranges. -// -// The first is a tiny one that hosts an SMM_S3_RESUME_STATE object, to be -// filled in by the CPU SMM driver during normal boot, for the PEI instance of -// the LockBox library (which will rely on the object during S3 resume). -// -// The other SMRAM range is the main one, for the SMM core and the SMM drivers. -// -typedef enum { - DescIdxSmmS3ResumeState = 0, - DescIdxMain = 1, - DescIdxCount = 2 -} DESCRIPTOR_INDEX; +#include +#include // // The value of PcdQ35TsegMbytes is saved into this variable at module startup. @@ -97,8 +86,6 @@ SmramAccessLock ( EFI_STATUS SmramAccessGetCapabilities ( - IN BOOLEAN LockState, - IN BOOLEAN OpenState, IN OUT UINTN *SmramMapSize, IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap ); -- cgit