Laszlo, Comments below.
Mike > -----Original Message----- > From: Laszlo Ersek [mailto:ler...@redhat.com] > Sent: Monday, November 23, 2015 4:34 AM > To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2-de...@ml01.01.org > Cc: Justen, Jordan L <jordan.l.jus...@intel.com> > Subject: Re: [PATCH v4 07/41] OvmfPkg: add PEIM for providing TSEG-as-SMRAM > during PEI > > On 11/21/15 07:36, Kinney, Michael D wrote: > > Laszlo, > > > > Minor comments included below. I know from later items in this thread that > > SMM_COMMUNICATE has already been removed in > your local branch. > > > > Reviewed-by: Michael Kinney <michael.d.kin...@intel.com> > > Thank you. I've picked this up now. > > As far as I can see your comments, they are very similar to those you made > for patch #8 (sorry, I read your feedback to patch #8 first); > my followup is identical: > - I've now listed the header file in [Sources], > - I've fixed up the PRODUCES comment for the PPI GUID, > - I'd like to stick with STATIC under OvmfPkg, Ok. > - I can move the definition of the global variable to the top, remove > its initializer, and assign the members in the driver entry point > instead, if you'd like me to. You don't have to assign members in driver entry point. You can still do it in global variable. You just need function prototype before reference in global. > > Thanks! > Laszlo > > > > > Mike > > > >> -----Original Message----- > >> From: Laszlo Ersek [mailto:ler...@redhat.com] > >> Sent: Tuesday, November 3, 2015 1:01 PM > >> To: edk2-de...@ml01.01.org > >> Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Justen, Jordan L > >> <jordan.l.jus...@intel.com> > >> Subject: [PATCH v4 07/41] OvmfPkg: add PEIM for providing > >> TSEG-as-SMRAM during PEI > >> > >> "MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.inf" is the > >> library instance that implements the LockBoxLib library class with > >> SMRAM access for the PEI phase. > >> > >> Introduce a PEIM that produces the EFI_PEI_SMM_COMMUNICATION_PPI and > >> PEI_SMM_ACCESS_PPI interfaces, enabling SmmLockBoxPeiLib to work. > >> > >> Said library instance can parse and access LockBox data itself > >> (without additional LockBox drivers) if the > >> EFI_PEI_SMM_COMMUNICATION_PPI.Communicate() function returns > >> EFI_NOT_STARTED to it. However it requires that > >> EFI_PEI_SMM_COMMUNICATION_PPI exist at least. Also, > >> PEI_SMM_ACCESS_PPI must exist and work. > >> > >> The load / installation order of S3Resume2Pei and SmmAccessPei is > >> indifferent. SmmAccessPei produces the GUIDed HOB during its > >> installation (which happens during PEI), but S3Resume2Pei accesses > >> the HOB only when the DXE IPL calls its S3RestoreConfig2 PPI member, as > >> last act of PEI. > >> > >> MCH_SMRAM_D_LCK and MCH_ESMRAMC_T_EN are masked out the way they are, > >> in > >> SmmAccessPeiEntryPoint() and SmramAccessOpen() respectively, in order > >> to prevent VS20xx from warning about the (otherwise fully > >> intentional) truncation in the UINT8 casts. (Warnings reported by > >> Michael Kinney.) > >> > >> Cc: Michael Kinney <michael.d.kin...@intel.com> > >> Cc: Jordan Justen <jordan.l.jus...@intel.com> > >> Contributed-under: TianoCore Contribution Agreement 1.0 > >> Signed-off-by: Laszlo Ersek <ler...@redhat.com> > >> --- > >> > >> Notes: > >> v3: > >> - update bit-neg expressions to silence VS20xx warnings [Mike] > >> > >> OvmfPkg/OvmfPkgIa32.dsc | 6 + > >> OvmfPkg/OvmfPkgIa32X64.dsc | 6 + > >> OvmfPkg/OvmfPkgX64.dsc | 6 + > >> OvmfPkg/OvmfPkgIa32.fdf | 3 + > >> OvmfPkg/OvmfPkgIa32X64.fdf | 3 + > >> OvmfPkg/OvmfPkgX64.fdf | 3 + > >> OvmfPkg/SmmAccess/SmmAccessPei.inf | 70 +++ > >> OvmfPkg/SmmAccess/SmramInternal.h | 89 ++++ > >> OvmfPkg/SmmAccess/SmmAccessPei.c | 446 ++++++++++++++++++++ > >> OvmfPkg/SmmAccess/SmramInternal.c | 188 +++++++++ > >> 10 files changed, 820 insertions(+) > >> > >> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc index > >> c6850ff..0b729ca 100644 > >> --- a/OvmfPkg/OvmfPkgIa32.dsc > >> +++ b/OvmfPkg/OvmfPkgIa32.dsc > >> @@ -445,6 +445,12 @@ [Components] > >> <LibraryClasses> > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> } > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccessPei.inf { > >> + <LibraryClasses> > >> + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> + } > >> +!endif > >> > >> # > >> # DXE Phase modules > >> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > >> index dd65bf9..7f672d9 100644 > >> --- a/OvmfPkg/OvmfPkgIa32X64.dsc > >> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > >> @@ -451,6 +451,12 @@ [Components.IA32] > >> <LibraryClasses> > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> } > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccessPei.inf { > >> + <LibraryClasses> > >> + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> + } > >> +!endif > >> > >> [Components.X64] > >> # > >> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc index > >> 0de3c85..986c019 100644 > >> --- a/OvmfPkg/OvmfPkgX64.dsc > >> +++ b/OvmfPkg/OvmfPkgX64.dsc > >> @@ -450,6 +450,12 @@ [Components] > >> <LibraryClasses> > >> PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> } > >> +!if $(SMM_REQUIRE) == TRUE > >> + OvmfPkg/SmmAccess/SmmAccessPei.inf { > >> + <LibraryClasses> > >> + PcdLib|MdePkg/Library/PeiPcdLib/PeiPcdLib.inf > >> + } > >> +!endif > >> > >> # > >> # DXE Phase modules > >> diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf index > >> 44e4a92..650dab1 100644 > >> --- a/OvmfPkg/OvmfPkgIa32.fdf > >> +++ b/OvmfPkg/OvmfPkgIa32.fdf > >> @@ -171,6 +171,9 @@ [FV.PEIFV] > >> INF OvmfPkg/PlatformPei/PlatformPei.inf > >> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >> INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccessPei.inf > >> +!endif > >> > >> > >> ##################################################################### > >> ########### > >> > >> diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > >> index 67bfbe7..5830401 100644 > >> --- a/OvmfPkg/OvmfPkgIa32X64.fdf > >> +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > >> @@ -171,6 +171,9 @@ [FV.PEIFV] > >> INF OvmfPkg/PlatformPei/PlatformPei.inf > >> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >> INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccessPei.inf > >> +!endif > >> > >> > >> ##################################################################### > >> ########### > >> > >> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf index > >> 6624789..9dd6171 100644 > >> --- a/OvmfPkg/OvmfPkgX64.fdf > >> +++ b/OvmfPkg/OvmfPkgX64.fdf > >> @@ -171,6 +171,9 @@ [FV.PEIFV] > >> INF OvmfPkg/PlatformPei/PlatformPei.inf > >> INF MdeModulePkg/Core/DxeIplPeim/DxeIpl.inf > >> INF UefiCpuPkg/Universal/Acpi/S3Resume2Pei/S3Resume2Pei.inf > >> +!if $(SMM_REQUIRE) == TRUE > >> +INF OvmfPkg/SmmAccess/SmmAccessPei.inf > >> +!endif > >> > >> > >> ##################################################################### > >> ########### > >> > >> diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf > >> b/OvmfPkg/SmmAccess/SmmAccessPei.inf > >> new file mode 100644 > >> index 0000000..94eb6c9 > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf > >> @@ -0,0 +1,70 @@ > >> +## @file > >> +# A PEIM with the following responsibilities: > >> +# > >> +# - provide SMRAM access by producing PEI_SMM_ACCESS_PPI, # - > >> +provide a simple EFI_PEI_SMM_COMMUNICATION_PPI (always returning > >> +# EFI_NOT_STARTED), > >> +# - 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. > >> +# > >> +# Copyright (C) 2013, 2015, Red Hat, Inc. > >> +# > >> +# This program and the accompanying materials are licensed and made > >> +available # under the terms and conditions of the BSD License which > >> +accompanies this # distribution. The full text of the license may be > >> +found at # http://opensource.org/licenses/bsd-license.php > >> +# > >> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> +BASIS, WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> +# > >> +## > >> + > >> +[Defines] > >> + INF_VERSION = 0x00010005 > >> + BASE_NAME = SmmAccessPei > >> + FILE_GUID = 6C0E75B4-B0B9-44D1-8210-3377D7B4E066 > >> + MODULE_TYPE = PEIM > >> + VERSION_STRING = 1.0 > >> + ENTRY_POINT = SmmAccessPeiEntryPoint > >> + > >> +# > >> +# The following information is for reference only and not required by the > >> build tools. > >> +# > >> +# VALID_ARCHITECTURES = IA32 X64 > >> +# > >> + > >> +[Sources] > >> + SmmAccessPei.c > >> + SmramInternal.c > > > > Missing source file SmramInternal.h > > > > > >> + > >> +[Packages] > >> + MdeModulePkg/MdeModulePkg.dec > >> + MdePkg/MdePkg.dec > >> + OvmfPkg/OvmfPkg.dec > >> + > >> +[Guids] > >> + gEfiAcpiVariableGuid > >> + > >> +[LibraryClasses] > >> + BaseMemoryLib > >> + DebugLib > >> + HobLib > >> + IoLib > >> + PcdLib > >> + PciLib > >> + PeiServicesLib > >> + PeimEntryPoint > >> + > >> +[FeaturePcd] > >> + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > >> + > >> +[FixedPcd] > >> + gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes > >> + > >> +[Ppis] > >> + gEfiPeiSmmCommunicationPpiGuid # PPI ALWAYS_PRODUCED > >> + gPeiSmmAccessPpiGuid # PPI ALWAYS_PRODUCED > > > > Comment should be: ## PRODUCES > > > >> + > >> +[Depex] > >> + TRUE > >> diff --git a/OvmfPkg/SmmAccess/SmramInternal.h > >> b/OvmfPkg/SmmAccess/SmramInternal.h > >> new file mode 100644 > >> index 0000000..4e9ac05 > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmramInternal.h > >> @@ -0,0 +1,89 @@ > >> +/** @file > >> + > >> + Functions and types shared by the SMM accessor PEI and DXE modules. > >> + > >> + Copyright (C) 2015, Red Hat, Inc. > >> + > >> + This program and the accompanying materials are licensed and made > >> + available under the terms and conditions of the BSD License which > >> + accompanies this distribution. The full text of the license may be > >> + found at http://opensource.org/licenses/bsd-license.php > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include <Pi/PiMultiPhase.h> > >> + > >> +// > >> +// 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; > >> + > >> +/** > >> + Read the MCH_SMRAM and ESMRAMC registers, and update the LockState > >> +and > >> + OpenState fields in the PEI_SMM_ACCESS_PPI / > >> +EFI_SMM_ACCESS2_PROTOCOL object, > >> + from the D_LCK and T_EN bits. > >> + > >> + PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL member functions > >> + can rely on the LockState and OpenState fields being up-to-date on > >> + entry, and they need to restore the same invariant on exit, if they > >> touch the bits in question. > >> + > >> + @param[out] LockState Reflects the D_LCK bit on output; TRUE iff SMRAM > >> is > >> + locked. > >> + @param[out] OpenState Reflects the inverse of the T_EN bit on output; > >> TRUE > >> + iff SMRAM is open. > >> +**/ > >> +VOID > >> +GetStates ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> + ); > >> + > >> +// > >> +// The functions below follow the PEI_SMM_ACCESS_PPI and // > >> +EFI_SMM_ACCESS2_PROTOCOL member declarations. The PeiServices and > >> +This // pointers are removed (TSEG doesn't depend on them), and so > >> +is the // DescriptorIndex parameter (TSEG doesn't support range-wise > >> locking). > >> +// > >> +// The LockState and OpenState members that are common to both // > >> +PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL are taken and > >> +updated in // isolation from the rest of the (non-shared) members. > >> +// > >> + > >> +EFI_STATUS > >> +SmramAccessOpen ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> + ); > >> + > >> +EFI_STATUS > >> +SmramAccessClose ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> + ); > >> + > >> +EFI_STATUS > >> +SmramAccessLock ( > >> + OUT BOOLEAN *LockState, > >> + IN OUT BOOLEAN *OpenState > >> + ); > >> + > >> +EFI_STATUS > >> +SmramAccessGetCapabilities ( > >> + IN BOOLEAN LockState, > >> + IN BOOLEAN OpenState, > >> + IN OUT UINTN *SmramMapSize, > >> + IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap > >> + ); > >> diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c > >> b/OvmfPkg/SmmAccess/SmmAccessPei.c > >> new file mode 100644 > >> index 0000000..8de4e4f > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmmAccessPei.c > >> @@ -0,0 +1,446 @@ > >> +/** @file > >> + > >> + A PEIM with the following responsibilities: > >> + > >> + - verify & configure the Q35 TSEG in the entry point, > >> + - provide SMRAM access by producing PEI_SMM_ACCESS_PPI, > >> + - provide a simple EFI_PEI_SMM_COMMUNICATION_PPI (always returning > >> + EFI_NOT_STARTED), > >> + - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and > >> expose > >> + it via the gEfiAcpiVariableGuid GUID HOB. > >> + > >> + This PEIM runs from RAM, so we can write to variables with static > >> + storage duration. > >> + > >> + Copyright (C) 2013, 2015, Red Hat, Inc.<BR> Copyright (c) 2010, > >> + Intel Corporation. All rights reserved.<BR> > >> + > >> + This program and the accompanying materials are licensed and made > >> + available under the terms and conditions of the BSD License which > >> + accompanies this distribution. The full text of the license may be > >> + found at http://opensource.org/licenses/bsd-license.php > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include <Guid/AcpiS3Context.h> > >> +#include <Library/BaseMemoryLib.h> > >> +#include <Library/DebugLib.h> > >> +#include <Library/HobLib.h> > >> +#include <Library/IoLib.h> > >> +#include <Library/PcdLib.h> > >> +#include <Library/PciLib.h> > >> +#include <Library/PeiServicesLib.h> > >> +#include <Ppi/SmmAccess.h> > >> +#include <Ppi/SmmCommunication.h> > >> + > >> +#include <OvmfPlatforms.h> > >> + > >> +#include "SmramInternal.h" > >> + > >> +// > >> +// EFI_PEI_SMM_COMMUNICATION_PPI implementation. > >> +// > >> + > >> +/** > >> + Communicates with a registered handler. > >> + > >> + This function provides a service to send and receive messages from > >> + a registered UEFI service. > >> + > >> + @param[in] This The EFI_PEI_SMM_COMMUNICATION_PPI > >> instance. > >> + @param[in] CommBuffer A pointer to the buffer to convey into > >> SMRAM. > >> + @param[in] CommSize The size of the data buffer being passed > >> in.On > >> + exit, the size of data being returned. > >> Zero if > >> + the handler does not wish to reply with > >> any > >> + data. > >> + > >> + @retval EFI_SUCCESS The message was successfully posted. > >> + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL. > >> +**/ > >> +STATIC > >> +EFI_STATUS > >> +EFIAPI > >> +SmmCommunicate ( > >> + IN CONST EFI_PEI_SMM_COMMUNICATION_PPI *This, > >> + IN OUT VOID *CommBuffer, > >> + IN OUT UINTN *CommSize > >> + ) > >> +{ > >> + // > >> + // This return status is not documented, but it causes > >> + // "MdeModulePkg/Library/SmmLockBoxLib/SmmLockBoxPeiLib.c" to look > >> +for the > >> + // LockBox in SMRAM itself. It allows us to avoid implementing > >> + // EFI_PEI_SMM_COMMUNICATION_PPI with real functionality, plus we > >> +can > >> + // completely skip PEI_SMM_CONTROL_PPI (which the former would > >> +arguably rely > >> + // on). > >> + // > >> + return EFI_NOT_STARTED; > >> +} > >> + > >> +STATIC EFI_PEI_SMM_COMMUNICATION_PPI mCommunication = { > >> + &SmmCommunicate > >> +}; > >> + > >> + > >> +// > >> +// PEI_SMM_ACCESS_PPI implementation. > >> +// > >> + > >> +/** > >> + Opens the SMRAM area to be accessible by a PEIM driver. > >> + > >> + This function "opens" SMRAM so that it is visible while not inside of > >> SMM. > >> + The function should return EFI_UNSUPPORTED if the hardware does > >> + not support hiding of SMRAM. The function should return > >> + EFI_DEVICE_ERROR if the SMRAM configuration is locked. > >> + > >> + @param PeiServices General purpose services available to > >> every > >> + PEIM. > >> + @param This The pointer to the SMM Access Interface. > >> + @param DescriptorIndex The 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. > >> + > >> +**/ > >> +STATIC > > > > Recommend removing STATIC > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccessPeiOpen ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN PEI_SMM_ACCESS_PPI *This, > >> + IN UINTN DescriptorIndex > >> + ) > >> +{ > >> + if (DescriptorIndex >= DescIdxCount) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + > >> + // > >> + // According to current practice, DescriptorIndex is not > >> +considered at all, > >> + // beyond validating it. > >> + // > >> + return SmramAccessOpen (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Inhibits access to the SMRAM. > >> + > >> + This function "closes" SMRAM so that it is not visible while outside of > >> SMM. > >> + The function should return EFI_UNSUPPORTED if the hardware does > >> + not support hiding of SMRAM. > >> + > >> + @param PeiServices General purpose services available to > >> every > >> + PEIM. > >> + @param This The pointer to the SMM Access > >> Interface. > >> + @param DescriptorIndex The 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. > >> + > >> +**/ > >> +STATIC > > > > Recommend removing STATIC > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccessPeiClose ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN PEI_SMM_ACCESS_PPI *This, > >> + IN UINTN DescriptorIndex > >> + ) > >> +{ > >> + if (DescriptorIndex >= DescIdxCount) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + > >> + // > >> + // According to current practice, DescriptorIndex is not > >> +considered at all, > >> + // beyond validating it. > >> + // > >> + return SmramAccessClose (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Inhibits access to the SMRAM. > >> + > >> + This function prohibits access to the SMRAM region. This function > >> + is usually implemented such that it is a write-once operation. > >> + > >> + @param PeiServices General purpose services available to > >> every > >> + PEIM. > >> + @param This The pointer to the SMM Access > >> Interface. > >> + @param DescriptorIndex The region of SMRAM to Close. > >> + > >> + @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. > >> + > >> +**/ > >> +STATIC > > > > Recommend removing STATIC > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccessPeiLock ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN PEI_SMM_ACCESS_PPI *This, > >> + IN UINTN DescriptorIndex > >> + ) > >> +{ > >> + if (DescriptorIndex >= DescIdxCount) { > >> + return EFI_INVALID_PARAMETER; > >> + } > >> + > >> + // > >> + // According to current practice, DescriptorIndex is not > >> +considered at all, > >> + // beyond validating it. > >> + // > >> + return SmramAccessLock (&This->LockState, &This->OpenState); } > >> + > >> +/** > >> + Queries the memory controller for the possible regions that will > >> +support > >> + SMRAM. > >> + > >> + @param PeiServices General purpose services available to > >> every > >> + PEIM. > >> + @param This The pointer to the SmmAccessPpi Interface. > >> + @param SmramMapSize The pointer to the variable containing > >> size of > >> + the buffer to contain the description > >> + information. > >> + @param SmramMap The 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. > >> + > >> +**/ > >> +STATIC > > > > Recommend removing STATIC > > > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccessPeiGetCapabilities ( > >> + IN EFI_PEI_SERVICES **PeiServices, > >> + IN PEI_SMM_ACCESS_PPI *This, > >> + IN OUT UINTN *SmramMapSize, > >> + IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap > >> + ) > >> +{ > >> + return SmramAccessGetCapabilities (This->LockState, This->OpenState, > >> + SmramMapSize, SmramMap); > >> +} > >> + > >> +// > >> +// LockState and OpenState will be filled in by the entry point. > >> +// > >> +STATIC PEI_SMM_ACCESS_PPI mAccess = { > >> + &SmmAccessPeiOpen, > >> + &SmmAccessPeiClose, > >> + &SmmAccessPeiLock, > >> + &SmmAccessPeiGetCapabilities > >> +}; > > > > Recommend removing STATIC > > Global variables should be before function implementations > > > >> + > >> + > >> +STATIC EFI_PEI_PPI_DESCRIPTOR mPpiList[] = { > >> + { > >> + EFI_PEI_PPI_DESCRIPTOR_PPI, > >> + &gEfiPeiSmmCommunicationPpiGuid, &mCommunication > >> + }, > >> + { > >> + EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > >> + &gPeiSmmAccessPpiGuid, &mAccess > >> + } > >> +}; > > > > Recommend removing STATIC > > > >> + > >> + > >> +// > >> +// Utility functions. > >> +// > >> +STATIC > > > > Recommend removing STATIC > > > >> +UINT8 > >> +CmosRead8 ( > >> + IN UINT8 Index > >> + ) > >> +{ > >> + IoWrite8 (0x70, Index); > >> + return IoRead8 (0x71); > >> +} > >> + > >> +STATIC > > > > Recommend removing STATIC > > > >> +UINT32 > >> +GetSystemMemorySizeBelow4gb ( > >> + VOID > >> + ) > >> +{ > >> + UINT32 Cmos0x34; > >> + UINT32 Cmos0x35; > >> + > >> + Cmos0x34 = CmosRead8 (0x34); > >> + Cmos0x35 = CmosRead8 (0x35); > >> + > >> + return ((Cmos0x35 << 8 | Cmos0x34) << 16) + SIZE_16MB; } > >> + > >> + > >> +// > >> +// Entry point of this driver. > >> +// > >> +EFI_STATUS > >> +EFIAPI > >> +SmmAccessPeiEntryPoint ( > >> + IN EFI_PEI_FILE_HANDLE FileHandle, > >> + 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; > >> + > >> + // > >> + // This module should only be included if SMRAM support is required. > >> + // > >> + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > >> + > >> + // > >> + // Verify if we're running on a Q35 machine type. > >> + // > >> + HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID); if > >> + (HostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) { > >> + DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only " > >> + "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId, > >> + INTEL_Q35_MCH_DEVICE_ID)); > >> + goto WrongConfig; > >> + } > >> + > >> + // > >> + // Confirm if QEMU supports SMRAM. > >> + // > >> + // With no support for it, the ESMRAMC (Extended System Management > >> + RAM // Control) register reads as zero. If there is support, the > >> + cache-enable // bits are hard-coded as 1 by QEMU. > >> + // > >> + EsmramcVal = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)); > >> + RegMask8 = MCH_ESMRAMC_SM_CACHE | MCH_ESMRAMC_SM_L1 | > >> + MCH_ESMRAMC_SM_L2; if ((EsmramcVal & RegMask8) != RegMask8) { > >> + DEBUG ((EFI_D_ERROR, "%a: this Q35 implementation lacks SMRAM\n", > >> + __FUNCTION__)); > >> + goto WrongConfig; > >> + } > >> + > >> + TopOfLowRam = GetSystemMemorySizeBelow4gb (); ASSERT > >> + ((TopOfLowRam & (SIZE_1MB - 1)) == 0); TopOfLowRamMb = TopOfLowRam > >> + >> 20; > >> + > >> + // > >> + // Some of the following registers are no-ops for QEMU at the > >> + moment, but it // is recommended to set them correctly, since the > >> + ESMRAMC that we ultimately // care about is in the same set of > >> registers. > >> + // > >> + // First, we disable the integrated VGA, and set both the GTT > >> + Graphics Memory // Size and the Graphics Mode Select memory > >> pre-allocation fields to zero. > >> + // This takes just one write to the Graphics Control Register. > >> + // > >> + PciWrite16 (DRAMC_REGISTER_Q35 (MCH_GGC), MCH_GGC_IVD); > >> + > >> + // > >> + // Set Top of Low Usable DRAM. > >> + // > >> + PciWrite16 (DRAMC_REGISTER_Q35 (MCH_TOLUD), > >> + (UINT16)(TopOfLowRamMb << MCH_TOLUD_MB_SHIFT)); > >> + > >> + // > >> + // Given the zero graphics memory sizes configured above, set the > >> + // graphics-related stolen memory bases to the same as TOLUD. > >> + // > >> + PciWrite32 (DRAMC_REGISTER_Q35 (MCH_GBSM), > >> + TopOfLowRamMb << MCH_GBSM_MB_SHIFT); > >> + PciWrite32 (DRAMC_REGISTER_Q35 (MCH_BGSM), > >> + TopOfLowRamMb << MCH_BGSM_MB_SHIFT); > >> + > >> + // > >> + // Set TSEG Memory Base. > >> + // > >> + PciWrite32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB), > >> + (TopOfLowRamMb - FixedPcdGet8 (PcdQ35TsegMbytes)) << > >> + MCH_TSEGMB_MB_SHIFT); > >> + > >> + // > >> + // Set TSEG size, and disable TSEG visibility outside of SMM. Note > >> + that the // T_EN bit has inverse meaning; when T_EN is set, then > >> + TSEG visibility is // *restricted* to SMM. > >> + // > >> + EsmramcVal &= ~(UINT32)MCH_ESMRAMC_TSEG_MASK; EsmramcVal |= > >> + FixedPcdGet8 (PcdQ35TsegMbytes) == 8 ? MCH_ESMRAMC_TSEG_8MB : > >> + FixedPcdGet8 (PcdQ35TsegMbytes) == 2 ? > >> MCH_ESMRAMC_TSEG_2MB : > >> + MCH_ESMRAMC_TSEG_1MB; EsmramcVal |= > >> + MCH_ESMRAMC_T_EN; > >> + PciWrite8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), EsmramcVal); > >> + > >> + // > >> + // TSEG should be closed (see above), but unlocked, initially. Set > >> + G_SMRAME // (Global SMRAM Enable) too, as both D_LCK and T_EN depend on > >> it. > >> + // > >> + PciAndThenOr8 (DRAMC_REGISTER_Q35 (MCH_SMRAM), > >> + (UINT8)((~(UINT32)MCH_SMRAM_D_LCK) & 0xff), 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 ((EFI_D_VERBOSE, "%a: SMRAM map follows, %d entries\n", > >> __FUNCTION__, > >> + (INT32)Count)); > >> + DEBUG ((EFI_D_VERBOSE, "% 20a % 20a % 20a % 20a\n", > >> "PhysicalStart(0x)", > >> + "PhysicalSize(0x)", "CpuStart(0x)", "RegionState(0x)")); > >> + for (Idx = 0; Idx < Count; ++Idx) { > >> + DEBUG ((EFI_D_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]); > >> + > >> + // > >> + // We're done. The next step should succeed, but even if it fails, > >> + we can't // roll back the above BuildGuidHob() allocation, because > >> + PEI doesn't support // releasing memory. > >> + // > >> + return PeiServicesInstallPpi (mPpiList); > >> + > >> +WrongConfig: > >> + // > >> + // We really don't want to continue in this case. > >> + // > >> + // _ASSERT() is never compiled out, and it respects > >> PcdDebugPropertyMask (ie. > >> + // prevent further execution with CPU breakpoint vs. dead loop). > >> + // > >> + _ASSERT (FALSE); > >> + return EFI_UNSUPPORTED; > >> +} > >> diff --git a/OvmfPkg/SmmAccess/SmramInternal.c > >> b/OvmfPkg/SmmAccess/SmramInternal.c > >> new file mode 100644 > >> index 0000000..c3267ca > >> --- /dev/null > >> +++ b/OvmfPkg/SmmAccess/SmramInternal.c > >> @@ -0,0 +1,188 @@ > >> +/** @file > >> + > >> + Functions and types shared by the SMM accessor PEI and DXE modules. > >> + > >> + Copyright (C) 2015, Red Hat, Inc. > >> + > >> + This program and the accompanying materials are licensed and made > >> + available under the terms and conditions of the BSD License which > >> + accompanies this distribution. The full text of the license may be > >> + found at http://opensource.org/licenses/bsd-license.php > >> + > >> + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > >> + BASIS, WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > >> EXPRESS OR IMPLIED. > >> + > >> +**/ > >> + > >> +#include <Guid/AcpiS3Context.h> > >> +#include <IndustryStandard/Q35MchIch9.h> #include > >> +<Library/DebugLib.h> #include <Library/PciLib.h> > >> + > >> +#include "SmramInternal.h" > >> + > >> +/** > >> + Read the MCH_SMRAM and ESMRAMC registers, and update the LockState > >> +and > >> + OpenState fields in the PEI_SMM_ACCESS_PPI / > >> +EFI_SMM_ACCESS2_PROTOCOL object, > >> + from the D_LCK and T_EN bits. > >> + > >> + PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL member functions > >> + can rely on the LockState and OpenState fields being up-to-date on > >> + entry, and they need to restore the same invariant on exit, if they > >> touch the bits in question. > >> + > >> + @param[out] LockState Reflects the D_LCK bit on output; TRUE iff SMRAM > >> is > >> + locked. > >> + @param[out] OpenState Reflects the inverse of the T_EN bit on output; > >> TRUE > >> + iff SMRAM is open. > >> +**/ > >> +VOID > >> +GetStates ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> +) > >> +{ > >> + UINT8 SmramVal, EsmramcVal; > >> + > >> + SmramVal = PciRead8 (DRAMC_REGISTER_Q35 (MCH_SMRAM)); > >> + EsmramcVal = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)); > >> + > >> + *LockState = !!(SmramVal & MCH_SMRAM_D_LCK); > >> + *OpenState = !(EsmramcVal & MCH_ESMRAMC_T_EN); } > >> + > >> +// > >> +// The functions below follow the PEI_SMM_ACCESS_PPI and // > >> +EFI_SMM_ACCESS2_PROTOCOL member declarations. The PeiServices and > >> +This // pointers are removed (TSEG doesn't depend on them), and so > >> +is the // DescriptorIndex parameter (TSEG doesn't support range-wise > >> locking). > >> +// > >> +// The LockState and OpenState members that are common to both // > >> +PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL are taken and > >> +updated in // isolation from the rest of the (non-shared) members. > >> +// > >> + > >> +EFI_STATUS > >> +SmramAccessOpen ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> + ) > >> +{ > >> + // > >> + // Open TSEG by clearing T_EN. > >> + // > >> + PciAnd8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), > >> + (UINT8)((~(UINT32)MCH_ESMRAMC_T_EN) & 0xff)); > >> + > >> + GetStates (LockState, OpenState); > >> + if (!*OpenState) { > >> + return EFI_DEVICE_ERROR; > >> + } > >> + return EFI_SUCCESS; > >> +} > >> + > >> +EFI_STATUS > >> +SmramAccessClose ( > >> + OUT BOOLEAN *LockState, > >> + OUT BOOLEAN *OpenState > >> + ) > >> +{ > >> + // > >> + // Close TSEG by setting T_EN. > >> + // > >> + PciOr8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), MCH_ESMRAMC_T_EN); > >> + > >> + GetStates (LockState, OpenState); > >> + if (*OpenState) { > >> + return EFI_DEVICE_ERROR; > >> + } > >> + return EFI_SUCCESS; > >> +} > >> + > >> +EFI_STATUS > >> +SmramAccessLock ( > >> + OUT BOOLEAN *LockState, > >> + IN OUT BOOLEAN *OpenState > >> + ) > >> +{ > >> + if (*OpenState) { > >> + return EFI_DEVICE_ERROR; > >> + } > >> + > >> + // > >> + // Close & lock TSEG by setting T_EN and D_LCK. > >> + // > >> + PciOr8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC), MCH_ESMRAMC_T_EN); > >> + PciOr8 (DRAMC_REGISTER_Q35 (MCH_SMRAM), MCH_SMRAM_D_LCK); > >> + > >> + GetStates (LockState, OpenState); > >> + if (*OpenState || !*LockState) { > >> + return EFI_DEVICE_ERROR; > >> + } > >> + return EFI_SUCCESS; > >> +} > >> + > >> +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; > >> + } > >> + > >> + // > >> + // Read the TSEG Memory Base register. > >> + // > >> + TsegMemoryBaseMb = PciRead32 (DRAMC_REGISTER_Q35 (MCH_TSEGMB)); > >> + TsegMemoryBase = (TsegMemoryBaseMb >> MCH_TSEGMB_MB_SHIFT) << 20; > >> + > >> + // > >> + // 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; > >> + > >> + // > >> + // 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; > >> + > >> + // > >> + // Get the TSEG size bits from the ESMRAMC register. > >> + // > >> + TsegSizeBits = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)) & > >> + MCH_ESMRAMC_TSEG_MASK; > >> + > >> + // > >> + // The second region is the main one, following the first. > >> + // > >> + 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 : > >> + SIZE_1MB) - SmramMap[DescIdxSmmS3ResumeState].PhysicalSize; > >> + SmramMap[DescIdxMain].RegionState = CommonRegionState; > >> + > >> + return EFI_SUCCESS; > >> +} > >> -- > >> 1.8.3.1 > >> > > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
