From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1A0932095D221 for ; Mon, 19 Jun 2017 10:29:05 -0700 (PDT) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 19 Jun 2017 10:30:26 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.39,362,1493708400"; d="scan'208";a="116260281" Received: from awestwoo-mobl2.amr.corp.intel.com (HELO localhost) ([10.255.77.13]) by fmsmga005.fm.intel.com with ESMTP; 19 Jun 2017 10:30:26 -0700 MIME-Version: 1.0 To: Laszlo Ersek , Jeff Fan , Michael Kinney , Jiewen Yao , edk2-devel-01 Message-ID: <149789342556.32751.17592475673245441129@jljusten-skl> From: Jordan Justen In-Reply-To: <20170608171333.17937-2-lersek@redhat.com> References: <20170608171333.17937-1-lersek@redhat.com> <20170608171333.17937-2-lersek@redhat.com> User-Agent: alot/0.5.1 Date: Mon, 19 Jun 2017 10:30:25 -0700 Subject: Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance) X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 19 Jun 2017 17:29:05 -0000 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: quoted-printable On 2017-06-08 10:13:29, Laszlo Ersek wrote: > OvmfPkg contains three modules that work with the TSEG (SMRAM) size: > PlatformPei (PEIM), SmmAccessPei (PEIM), and SmmAccess2Dxe (DXE_DRIVER). > These modules open-code the interpretation of the ESMRAMC register's > TSEG_SZ bit-field. That is OK as long as we stick with the Q35 hardware > spec and nothing more, but it makes it difficult to benefit from an > upcoming QEMU feature, namely extended TSEG sizes. > = > Introduce the Q35TsegSizeLib class, and its sole lib instance, for > extracting / centralizing TSEG size querying and interpretation. This > library instance is self contained and does not consume dynamic PCDs (for > example, it doesn't consume PcdOvmfHostBridgePciDevId), because such PCDs > tend to be set in PlatformPei, but the dispatch order between PlatformPei > and SmmAccessPei is unspecified (both have TRUE for DEPEX). I think that on 'real' platforms there would be an enforced ordering of Platform PEI before the PEI SMM drivers. I'm not sure what the mechanism is, and whether it is applicable to OVMF. Jeff, Mike, Jiewen, Do you know? Do the chipset SMM drivers depend on gEfiPeiMemoryDiscoveredPpiGuid that is triggered by Platform PEI? Does gEfiPeiMemoryDiscoveredPpiGuid also install on S3 resume? Laszlo, It sounded like you'd be open to using a PCD, but the PEI driver ordering issue prevented it. Is that right? Thanks, -Jordan > In the next few patches we're going to rebase the three listed modules to > Q35TsegSizeLib. As introduced, the library instance only captures the > currently supported TSEG sizes. > = > Cc: Jordan Justen > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek > --- > OvmfPkg/OvmfPkg.dec | 5 + > OvmfPkg/OvmfPkgIa32.dsc | 1 + > OvmfPkg/OvmfPkgIa32X64.dsc | 1 + > OvmfPkg/OvmfPkgX64.dsc | 1 + > OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf | 47 +++++ > OvmfPkg/Include/Library/Q35TsegSizeLib.h | 74 ++++++++ > OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c | 186 ++++++++++++++++= ++++ > 7 files changed, 315 insertions(+) > = > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 5627be0bab0a..7b9220369b95 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -27,14 +27,19 @@ [LibraryClasses] > # > LoadLinuxLib|Include/Library/LoadLinuxLib.h > = > ## @libraryclass Save and restore variables using a file > # > NvVarsFileLib|Include/Library/NvVarsFileLib.h > = > + ## @libraryclass Utility library to query TSEG size-related quantiti= es on > + # Q35. > + # > + Q35TsegSizeLib|Include/Library/Q35TsegSizeLib.h > + > ## @libraryclass Access QEMU's firmware configuration interface > # > QemuFwCfgLib|Include/Library/QemuFwCfgLib.h > = > ## @libraryclass S3 support for QEMU fw_cfg > # > QemuFwCfgS3Lib|Include/Library/QemuFwCfgS3Lib.h > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index 0647f346257a..8917c2f7b085 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -129,14 +129,15 @@ [LibraryClasses] > UefiLib|MdePkg/Library/UefiLib/UefiLib.inf > UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiB= ootServicesTableLib.inf > UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib= /UefiRuntimeServicesTableLib.inf > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt= ryPoint.inf > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/Uef= iApplicationEntryPoint.inf > DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiD= evicePathLibDevicePathProtocol.inf > NvVarsFileLib|OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf > + Q35TsegSizeLib|OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf > FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf > UefiCpuLib|UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/Dx= eSecurityManagementLib.inf > NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf > IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf > UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 1182b1858a7d..56e9c5b790d7 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -134,14 +134,15 @@ [LibraryClasses] > UefiLib|MdePkg/Library/UefiLib/UefiLib.inf > UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiB= ootServicesTableLib.inf > UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib= /UefiRuntimeServicesTableLib.inf > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt= ryPoint.inf > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/Uef= iApplicationEntryPoint.inf > DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiD= evicePathLibDevicePathProtocol.inf > NvVarsFileLib|OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf > + Q35TsegSizeLib|OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf > FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf > UefiCpuLib|UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/Dx= eSecurityManagementLib.inf > NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf > IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf > UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index 636dfb1b5638..618b72bffa80 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -134,14 +134,15 @@ [LibraryClasses] > UefiLib|MdePkg/Library/UefiLib/UefiLib.inf > UefiBootServicesTableLib|MdePkg/Library/UefiBootServicesTableLib/UefiB= ootServicesTableLib.inf > UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib= /UefiRuntimeServicesTableLib.inf > UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEnt= ryPoint.inf > UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/Uef= iApplicationEntryPoint.inf > DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiD= evicePathLibDevicePathProtocol.inf > NvVarsFileLib|OvmfPkg/Library/NvVarsFileLib/NvVarsFileLib.inf > + Q35TsegSizeLib|OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf > FileHandleLib|MdePkg/Library/UefiFileHandleLib/UefiFileHandleLib.inf > UefiCpuLib|UefiCpuPkg/Library/BaseUefiCpuLib/BaseUefiCpuLib.inf > SecurityManagementLib|MdeModulePkg/Library/DxeSecurityManagementLib/Dx= eSecurityManagementLib.inf > NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf > IpIoLib|MdeModulePkg/Library/DxeIpIoLib/DxeIpIoLib.inf > UdpIoLib|MdeModulePkg/Library/DxeUdpIoLib/DxeUdpIoLib.inf > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > diff --git a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf b/OvmfPkg/= Library/Q35TsegSizeLib/Q35TsegSizeLib.inf > new file mode 100644 > index 000000000000..8f99e55b8c48 > --- /dev/null > +++ b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf > @@ -0,0 +1,47 @@ > +## @file > +# Utility library to query TSEG size-related quantities on Q35. > +# > +# Copyright (C) 2017, Red Hat, Inc. > +# > +# This program and the accompanying materials are licensed and made avai= lable > +# under the terms and conditions of the BSD License which accompanies th= is > +# 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 =3D 1.25 > + BASE_NAME =3D Q35TsegSizeLib > + FILE_GUID =3D 6019578F-0078-46D4-86EE-06C486A304D2 > + MODULE_TYPE =3D BASE > + VERSION_STRING =3D 1.0 > + LIBRARY_CLASS =3D Q35TsegSizeLib|PEIM DXE_DRIVER > + > +# > +# The following information is for reference only and not required by th= e build > +# tools. > +# > +# VALID_ARCHITECTURES =3D IA32 X64 > +# > + > +[Sources] > + Q35TsegSizeLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseLib > + DebugLib > + PcdLib > + PciLib > + > +[FeaturePcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire > + > +[FixedPcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes > diff --git a/OvmfPkg/Include/Library/Q35TsegSizeLib.h b/OvmfPkg/Include/L= ibrary/Q35TsegSizeLib.h > new file mode 100644 > index 000000000000..580ef6887931 > --- /dev/null > +++ b/OvmfPkg/Include/Library/Q35TsegSizeLib.h > @@ -0,0 +1,74 @@ > +/** @file > + Utility library to query TSEG size-related quantities on Q35. > + > + Copyright (C) 2017, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made avai= lable > + under the terms and conditions of the BSD License which accompanies th= is > + 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. > +**/ > + > +#ifndef __Q35_TSEG_SIZE_LIB__ > +#define __Q35_TSEG_SIZE_LIB__ > + > +/** > + Query the preferred size of TSEG, in megabytes. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @return The preferred size of TSEG, expressed in megabytes. > +**/ > +UINT16 > +EFIAPI > +Q35TsegSizeGetPreferredMbytes ( > + VOID > + ); > + > +/** > + Query the ESMRAMC.TSEG_SZ bit-field value that corresponds to the pref= erred > + TSEG size. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @return The ESMRAMC.TSEG_SZ bit-field value that corresponds to the > + preferred TSEG size. The return value is a subset of > + MCH_ESMRAMC_TSEG_MASK, defined in . > +**/ > +UINT8 > +EFIAPI > +Q35TsegSizeGetPreferredEsmramcTsegSzMask ( > + VOID > + ); > + > +/** > + Extract the TSEG_SZ bit-field from the passed in ESMRAMC register valu= e, and > + return the number of megabytes that it represents. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @param[in] EsmramcVal The ESMRAMC register value to extract the TSEG_= SZ > + bit-field from, using MCH_ESMRAMC_TSEG_MASK from > + . If the extract= ed > + bit-field cannot be mapped to a MB count, the f= unction > + logs an error message and does not return. > + > + @return The number of megabytes that the extracted TSEG_SZ bit-field > + represents. > +**/ > +UINT16 > +EFIAPI > +Q35TsegSizeConvertEsmramcValToMbytes ( > + IN UINT8 EsmramcVal > + ); > + > +#endif > diff --git a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c b/OvmfPkg/Li= brary/Q35TsegSizeLib/Q35TsegSizeLib.c > new file mode 100644 > index 000000000000..db57a8b308de > --- /dev/null > +++ b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c > @@ -0,0 +1,186 @@ > +/** @file > + Utility library to query TSEG size-related quantities on Q35. > + > + Copyright (C) 2017, Red Hat, Inc. > + > + This program and the accompanying materials are licensed and made avai= lable > + under the terms and conditions of the BSD License which accompanies th= is > + 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 > +#include > +#include > +#include > +#include > +#include > + > +STATIC BOOLEAN mPreferencesInitialized; > +STATIC UINT8 mPreferredEsmramcTsegSzMask; > + > +/** > + Fetch the preferences into static variables that are going to be used = by the > + public functions of this library instance. > + > + The Q35 board requirement documented on those interfaces is commonly e= nforced > + here. > +**/ > +STATIC > +VOID > +Q35TsegSizeGetPreferences ( > + VOID > + ) > +{ > + UINT16 HostBridgeDevId; > + > + if (mPreferencesInitialized) { > + return; > + } > + > + // > + // This function should only be reached if SMRAM support is required. > + // > + ASSERT (FeaturePcdGet (PcdSmmSmramRequire)); > + > + HostBridgeDevId =3D PciRead16 (OVMF_HOSTBRIDGE_DID); > + if (HostBridgeDevId !=3D INTEL_Q35_MCH_DEVICE_ID) { > + DEBUG (( > + DEBUG_ERROR, > + "%a: %a: no TSEG (SMRAM) on host bridge DID=3D0x%04x; " > + "only DID=3D0x%04x (Q35) is supported\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + HostBridgeDevId, > + INTEL_Q35_MCH_DEVICE_ID > + )); > + ASSERT (FALSE); > + CpuDeadLoop (); > + } > + > + mPreferencesInitialized =3D TRUE; > + > + switch (FixedPcdGet8 (PcdQ35TsegMbytes)) { > + case 1: > + mPreferredEsmramcTsegSzMask =3D MCH_ESMRAMC_TSEG_1MB; > + break; > + case 2: > + mPreferredEsmramcTsegSzMask =3D MCH_ESMRAMC_TSEG_2MB; > + break; > + case 8: > + mPreferredEsmramcTsegSzMask =3D MCH_ESMRAMC_TSEG_8MB; > + break; > + default: > + ASSERT (FALSE); > + } > +} > + > + > +/** > + Query the preferred size of TSEG, in megabytes. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @return The preferred size of TSEG, expressed in megabytes. > +**/ > +UINT16 > +EFIAPI > +Q35TsegSizeGetPreferredMbytes ( > + VOID > + ) > +{ > + // > + // Query the ESMRAMC.TSEG_SZ preference and convert it to megabytes. > + // > + return Q35TsegSizeConvertEsmramcValToMbytes ( > + Q35TsegSizeGetPreferredEsmramcTsegSzMask () > + ); > +} > + > + > +/** > + Query the ESMRAMC.TSEG_SZ bit-field value that corresponds to the pref= erred > + TSEG size. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @return The ESMRAMC.TSEG_SZ bit-field value that corresponds to the > + preferred TSEG size. The return value is a subset of > + MCH_ESMRAMC_TSEG_MASK, defined in . > +**/ > +UINT8 > +EFIAPI > +Q35TsegSizeGetPreferredEsmramcTsegSzMask ( > + VOID > + ) > +{ > + Q35TsegSizeGetPreferences (); > + return mPreferredEsmramcTsegSzMask; > +} > + > + > +/** > + Extract the TSEG_SZ bit-field from the passed in ESMRAMC register valu= e, and > + return the number of megabytes that it represents. > + > + The caller is responsible for calling this function only on the Q35 bo= ard. If > + the function is called on another board, the function logs an informat= ive > + error message and does not return. > + > + @param[in] EsmramcVal The ESMRAMC register value to extract the TSEG_= SZ > + bit-field from, using MCH_ESMRAMC_TSEG_MASK from > + . If the extract= ed > + bit-field cannot be mapped to a MB count, the f= unction > + logs an error message and does not return. > + > + @return The number of megabytes that the extracted TSEG_SZ bit-field > + represents. > +**/ > +UINT16 > +EFIAPI > +Q35TsegSizeConvertEsmramcValToMbytes ( > + IN UINT8 EsmramcVal > + ) > +{ > + UINT8 TsegSizeBits; > + UINT16 Mbytes; > + > + Q35TsegSizeGetPreferences (); > + > + TsegSizeBits =3D EsmramcVal & MCH_ESMRAMC_TSEG_MASK; > + switch (TsegSizeBits) { > + case MCH_ESMRAMC_TSEG_1MB: > + Mbytes =3D 1; > + break; > + case MCH_ESMRAMC_TSEG_2MB: > + Mbytes =3D 2; > + break; > + case MCH_ESMRAMC_TSEG_8MB: > + Mbytes =3D 8; > + break; > + default: > + DEBUG (( > + DEBUG_ERROR, > + "%a: %a: unknown TsegSizeBits=3D0x%02x\n", > + gEfiCallerBaseName, > + __FUNCTION__, > + TsegSizeBits > + )); > + ASSERT (FALSE); > + CpuDeadLoop (); > + > + // > + // Keep compilers happy. > + // > + Mbytes =3D 0; > + } > + > + return Mbytes; > +} > -- = > 2.9.3 > = >=20