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).

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 <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 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 quantities 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/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
   UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
   DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
   UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
   DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
   UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
   UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
   DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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 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                    = 1.25
+  BASE_NAME                      = Q35TsegSizeLib
+  FILE_GUID                      = 6019578F-0078-46D4-86EE-06C486A304D2
+  MODULE_TYPE                    = BASE
+  VERSION_STRING                 = 1.0
+  LIBRARY_CLASS                  = Q35TsegSizeLib|PEIM DXE_DRIVER
+
+#
+# The following information is for reference only and not required by the build
+# tools.
+#
+#  VALID_ARCHITECTURES           = 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/Library/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 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.
+**/
+
+#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 board. If
+  the function is called on another board, the function logs an informative
+  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 preferred
+  TSEG size.
+
+  The caller is responsible for calling this function only on the Q35 board. If
+  the function is called on another board, the function logs an informative
+  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 <IndustryStandard/Q35MchIch9.h>.
+**/
+UINT8
+EFIAPI
+Q35TsegSizeGetPreferredEsmramcTsegSzMask (
+  VOID
+  );
+
+/**
+  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
+  return the number of megabytes that it represents.
+
+  The caller is responsible for calling this function only on the Q35 board. If
+  the function is called on another board, the function logs an informative
+  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
+                         <IndustryStandard/Q35MchIch9.h>. If the extracted
+                         bit-field cannot be mapped to a MB count, the function
+                         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/Library/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 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 <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/PcdLib.h>
+#include <Library/PciLib.h>
+#include <Library/Q35TsegSizeLib.h>
+#include <OvmfPlatforms.h>
+
+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 enforced
+  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 = PciRead16 (OVMF_HOSTBRIDGE_DID);
+  if (HostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: %a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
+      "only DID=0x%04x (Q35) is supported\n",
+      gEfiCallerBaseName,
+      __FUNCTION__,
+      HostBridgeDevId,
+      INTEL_Q35_MCH_DEVICE_ID
+      ));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+  }
+
+  mPreferencesInitialized = TRUE;
+
+  switch (FixedPcdGet8 (PcdQ35TsegMbytes)) {
+  case 1:
+    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_1MB;
+    break;
+  case 2:
+    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_2MB;
+    break;
+  case 8:
+    mPreferredEsmramcTsegSzMask = 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 board. If
+  the function is called on another board, the function logs an informative
+  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 preferred
+  TSEG size.
+
+  The caller is responsible for calling this function only on the Q35 board. If
+  the function is called on another board, the function logs an informative
+  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 <IndustryStandard/Q35MchIch9.h>.
+**/
+UINT8
+EFIAPI
+Q35TsegSizeGetPreferredEsmramcTsegSzMask (
+  VOID
+  )
+{
+  Q35TsegSizeGetPreferences ();
+  return mPreferredEsmramcTsegSzMask;
+}
+
+
+/**
+  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
+  return the number of megabytes that it represents.
+
+  The caller is responsible for calling this function only on the Q35 board. If
+  the function is called on another board, the function logs an informative
+  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
+                         <IndustryStandard/Q35MchIch9.h>. If the extracted
+                         bit-field cannot be mapped to a MB count, the function
+                         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 = EsmramcVal & MCH_ESMRAMC_TSEG_MASK;
+  switch (TsegSizeBits) {
+  case MCH_ESMRAMC_TSEG_1MB:
+    Mbytes = 1;
+    break;
+  case MCH_ESMRAMC_TSEG_2MB:
+    Mbytes = 2;
+    break;
+  case MCH_ESMRAMC_TSEG_8MB:
+    Mbytes = 8;
+    break;
+  default:
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: %a: unknown TsegSizeBits=0x%02x\n",
+      gEfiCallerBaseName,
+      __FUNCTION__,
+      TsegSizeBits
+      ));
+    ASSERT (FALSE);
+    CpuDeadLoop ();
+
+    //
+    // Keep compilers happy.
+    //
+    Mbytes = 0;
+  }
+
+  return Mbytes;
+}
-- 
2.9.3


_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

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 <jordan.l.justen@intel.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  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 quantities 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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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 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                    = 1.25
> +  BASE_NAME                      = Q35TsegSizeLib
> +  FILE_GUID                      = 6019578F-0078-46D4-86EE-06C486A304D2
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = Q35TsegSizeLib|PEIM DXE_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +#  VALID_ARCHITECTURES           = 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/Library/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 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.
> +**/
> +
> +#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 board. If
> +  the function is called on another board, the function logs an informative
> +  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 preferred
> +  TSEG size.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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 <IndustryStandard/Q35MchIch9.h>.
> +**/
> +UINT8
> +EFIAPI
> +Q35TsegSizeGetPreferredEsmramcTsegSzMask (
> +  VOID
> +  );
> +
> +/**
> +  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
> +  return the number of megabytes that it represents.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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
> +                         <IndustryStandard/Q35MchIch9.h>. If the extracted
> +                         bit-field cannot be mapped to a MB count, the function
> +                         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/Library/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 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 <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/Q35TsegSizeLib.h>
> +#include <OvmfPlatforms.h>
> +
> +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 enforced
> +  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 = PciRead16 (OVMF_HOSTBRIDGE_DID);
> +  if (HostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
> +      "only DID=0x%04x (Q35) is supported\n",
> +      gEfiCallerBaseName,
> +      __FUNCTION__,
> +      HostBridgeDevId,
> +      INTEL_Q35_MCH_DEVICE_ID
> +      ));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +
> +  mPreferencesInitialized = TRUE;
> +
> +  switch (FixedPcdGet8 (PcdQ35TsegMbytes)) {
> +  case 1:
> +    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_1MB;
> +    break;
> +  case 2:
> +    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_2MB;
> +    break;
> +  case 8:
> +    mPreferredEsmramcTsegSzMask = 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 board. If
> +  the function is called on another board, the function logs an informative
> +  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 preferred
> +  TSEG size.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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 <IndustryStandard/Q35MchIch9.h>.
> +**/
> +UINT8
> +EFIAPI
> +Q35TsegSizeGetPreferredEsmramcTsegSzMask (
> +  VOID
> +  )
> +{
> +  Q35TsegSizeGetPreferences ();
> +  return mPreferredEsmramcTsegSzMask;
> +}
> +
> +
> +/**
> +  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
> +  return the number of megabytes that it represents.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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
> +                         <IndustryStandard/Q35MchIch9.h>. If the extracted
> +                         bit-field cannot be mapped to a MB count, the function
> +                         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 = EsmramcVal & MCH_ESMRAMC_TSEG_MASK;
> +  switch (TsegSizeBits) {
> +  case MCH_ESMRAMC_TSEG_1MB:
> +    Mbytes = 1;
> +    break;
> +  case MCH_ESMRAMC_TSEG_2MB:
> +    Mbytes = 2;
> +    break;
> +  case MCH_ESMRAMC_TSEG_8MB:
> +    Mbytes = 8;
> +    break;
> +  default:
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %a: unknown TsegSizeBits=0x%02x\n",
> +      gEfiCallerBaseName,
> +      __FUNCTION__,
> +      TsegSizeBits
> +      ));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +
> +    //
> +    // Keep compilers happy.
> +    //
> +    Mbytes = 0;
> +  }
> +
> +  return Mbytes;
> +}
> -- 
> 2.9.3
> 
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 06/19/17 19:30, Jordan Justen wrote:
> 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?

In OVMF's Ia32X64 build report file (with SMM enabled), the only PEIM
with a DEPEX on gEfiPeiMemoryDiscoveredPpiGuid is CpuMpPei.

The final DEPEX of SmmAccessPei is ((TRUE) AND (gEfiPeiPcdPpiGuid)); the
latter is inherited from the PcdLib instance that SmmAccessPei uses. (I
think.)

> Does
> gEfiPeiMemoryDiscoveredPpiGuid also install on S3 resume?

Yes. That PPI GUID is produced as a consequence of our call to
PublishSystemMemory(), which we (must) do on the S3 resume path as well
(using a pre-reserved area); see PublishPeiMemory() in "MemDetect.c".

And, we actively use CpuMpPei to set the Feature Control MSR on all
processors, on both normal boot and S3 resume. (See "FeatureControl.c".)

> 
> Laszlo, It sounded like you'd be open to using a PCD, but the PEI
> driver ordering issue prevented it. Is that right?

* If we can turn the following two variables into dynamic PCDs:

- mPreferredEsmramcTsegSzMask (UINT8)
- mExtendedTsegMbytes (UINT16)

such that PlatformPei sets them "early enough" on both normal boot and
S3 resume, incorporating the logic of the Q35TsegSizeGetPreferences()
function, then the library instance itself can drop
Q35TsegSizeGetPreferences(), and the rest of the library functions can
be rebased to these PCDs.

* The library interfaces should stay as they are; they provide precisely
what the client code needs.


Even if the PCDs were technically doable, I think I'd slightly prefer
the current approach. The PCI config space accesses at module startup
are minimal, and they affect only three modules. OTOH, introducing two
PCDs feels, in this particular case, a bit like "polluting" OvmfPkg.dec.
(Well, "PcdPreferredEsmramcTsegSzMask" would *replace*
"PcdQ35TsegMbytes", so it would mean the addition of one PCD.)

Anyway, this is just a slight preference. What I feel much more strongly
about are the library APIs.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 2017-06-19 12:39:51, Laszlo Ersek wrote:
> 
> Even if the PCDs were technically doable, I think I'd slightly prefer
> the current approach. The PCI config space accesses at module startup
> are minimal, and they affect only three modules. OTOH, introducing two
> PCDs feels, in this particular case, a bit like "polluting" OvmfPkg.dec.
> (Well, "PcdPreferredEsmramcTsegSzMask" would *replace*
> "PcdQ35TsegMbytes", so it would mean the addition of one PCD.)
> 
> Anyway, this is just a slight preference. What I feel much more strongly
> about are the library APIs.

The library is the part that seems odd to me. A library just to deal
with Q35 tseg seems a bit specific.

I looked at this series a number of times. My thought is that:

* PcdQ35TsegMbytes should become dynamic

* No PCD for tseg mask

* PlatformPei adds knowledge of qemu ext tseg to set PcdQ35TsegMbytes

* SmmAccessPei adds dep on gEfiPeiMemoryDiscoveredPpiGuid to make sure
  PcdQ35TsegMbytes is set.

* SmmAccessPei adds some duplicated code to know about qemu ext tseg
  for mask. (But, I think a reasonably small amount of dup code.)

* SmramInternal.c just reads the size from the PCD

Now, as usual, I expect you'll point out some trivially obvious issue
that I missed. :)

-Jordan
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

As far as I know, the real platform SmmAccessPeim may depend on gEfiPeiMemoryDiscoveredPpiGuid, because the SmramHob is produced in MRC wrapper.

At same time, I also saw a solution to use library to produce SmmAccessPpi. In such case, there is no dependency expression, because one module does 2 things: 1) init memory, 2) produce SmmAccessPpi.

Thank you
Yao Jiewen

From: Justen, Jordan L
Sent: Tuesday, June 20, 2017 1:30 AM
To: Laszlo Ersek <lersek@redhat.com>; Fan, Jeff <jeff.fan@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel-01 <edk2-devel@lists.01.org>
Subject: Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

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 <jordan.l.justen@intel.com<mailto:jordan.l.justen@intel.com>>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> ---
>  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 quantities 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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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/UefiBootServicesTableLib.inf
>    UefiRuntimeServicesTableLib|MdePkg/Library/UefiRuntimeServicesTableLib/UefiRuntimeServicesTableLib.inf
>    UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
>    UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
>    DevicePathLib|MdePkg/Library/UefiDevicePathLibDevicePathProtocol/UefiDevicePathLibDevicePathProtocol.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/DxeSecurityManagementLib.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 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                    = 1.25
> +  BASE_NAME                      = Q35TsegSizeLib
> +  FILE_GUID                      = 6019578F-0078-46D4-86EE-06C486A304D2
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = Q35TsegSizeLib|PEIM DXE_DRIVER
> +
> +#
> +# The following information is for reference only and not required by the build
> +# tools.
> +#
> +#  VALID_ARCHITECTURES           = 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/Library/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 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.
> +**/
> +
> +#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 board. If
> +  the function is called on another board, the function logs an informative
> +  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 preferred
> +  TSEG size.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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 <IndustryStandard/Q35MchIch9.h>.
> +**/
> +UINT8
> +EFIAPI
> +Q35TsegSizeGetPreferredEsmramcTsegSzMask (
> +  VOID
> +  );
> +
> +/**
> +  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
> +  return the number of megabytes that it represents.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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
> +                         <IndustryStandard/Q35MchIch9.h>. If the extracted
> +                         bit-field cannot be mapped to a MB count, the function
> +                         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/Library/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 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 <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/PciLib.h>
> +#include <Library/Q35TsegSizeLib.h>
> +#include <OvmfPlatforms.h>
> +
> +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 enforced
> +  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 = PciRead16 (OVMF_HOSTBRIDGE_DID);
> +  if (HostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %a: no TSEG (SMRAM) on host bridge DID=0x%04x; "
> +      "only DID=0x%04x (Q35) is supported\n",
> +      gEfiCallerBaseName,
> +      __FUNCTION__,
> +      HostBridgeDevId,
> +      INTEL_Q35_MCH_DEVICE_ID
> +      ));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +  }
> +
> +  mPreferencesInitialized = TRUE;
> +
> +  switch (FixedPcdGet8 (PcdQ35TsegMbytes)) {
> +  case 1:
> +    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_1MB;
> +    break;
> +  case 2:
> +    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_2MB;
> +    break;
> +  case 8:
> +    mPreferredEsmramcTsegSzMask = 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 board. If
> +  the function is called on another board, the function logs an informative
> +  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 preferred
> +  TSEG size.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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 <IndustryStandard/Q35MchIch9.h>.
> +**/
> +UINT8
> +EFIAPI
> +Q35TsegSizeGetPreferredEsmramcTsegSzMask (
> +  VOID
> +  )
> +{
> +  Q35TsegSizeGetPreferences ();
> +  return mPreferredEsmramcTsegSzMask;
> +}
> +
> +
> +/**
> +  Extract the TSEG_SZ bit-field from the passed in ESMRAMC register value, and
> +  return the number of megabytes that it represents.
> +
> +  The caller is responsible for calling this function only on the Q35 board. If
> +  the function is called on another board, the function logs an informative
> +  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
> +                         <IndustryStandard/Q35MchIch9.h>. If the extracted
> +                         bit-field cannot be mapped to a MB count, the function
> +                         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 = EsmramcVal & MCH_ESMRAMC_TSEG_MASK;
> +  switch (TsegSizeBits) {
> +  case MCH_ESMRAMC_TSEG_1MB:
> +    Mbytes = 1;
> +    break;
> +  case MCH_ESMRAMC_TSEG_2MB:
> +    Mbytes = 2;
> +    break;
> +  case MCH_ESMRAMC_TSEG_8MB:
> +    Mbytes = 8;
> +    break;
> +  default:
> +    DEBUG ((
> +      DEBUG_ERROR,
> +      "%a: %a: unknown TsegSizeBits=0x%02x\n",
> +      gEfiCallerBaseName,
> +      __FUNCTION__,
> +      TsegSizeBits
> +      ));
> +    ASSERT (FALSE);
> +    CpuDeadLoop ();
> +
> +    //
> +    // Keep compilers happy.
> +    //
> +    Mbytes = 0;
> +  }
> +
> +  return Mbytes;
> +}
> --
> 2.9.3
>
>
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

On 06/21/17 02:52, Yao, Jiewen wrote:
> As far as I know, the real platform SmmAccessPeim may depend on
> gEfiPeiMemoryDiscoveredPpiGuid, because the SmramHob is produced in
> MRC wrapper.

Thanks for the info, Jiewen!

- I don't know what "MRC wrapper" is.

  I found "QuarkPlatformPkg/Platform/Pei/PlatformInit/MrcWrapper.c",
  with the comment "Framework PEIM to initialize memory on a Quark
  Memory Controller".

- I don't know what "SmramHob" is.

  I see some hits in QuarkPlatformPkg and Vlv2TbltDevicePkg. The type
  seems to be "EFI_SMRAM_HOB_DESCRIPTOR_BLOCK", in
  "IntelFrameworkPkg/Include/Guid/SmramMemoryReserve.h", with the
  comment "GUID specific data structure of HOB for reserving SMRAM
  regions".

  I think the comment is inaccurate. From the data structure itself, it
  looks like the HOB is used to expose the possible SMRAM regions to
  "other" modules, and not to reserve regions from SMRAM. Anyway, I
  guess the HOB is consumed by modules such as:

  - QuarkSocPkg/QuarkNorthCluster/Smm/Dxe/SmmAccessDxe
  - QuarkSocPkg/QuarkNorthCluster/Smm/Pei/SmmAccessPei

So apparently, on Quark, the PEIM that initializes physical RAM produces
the SmramHob (which is an Intel Framework concept, apparently), and
SmmAccessPei consumes the SmramHob. In order to serialize these
operations, the two PEIMs reuse / repurpose
gEfiPeiMemoryDiscoveredPpiGuid, as a PEIM dispatch barrier. The
availability of SmramHob is parallel to the availability of
gEfiPeiMemoryDiscoveredPpiGuid.

This doesn't match OVMF for two reasons:

- In OVMF / on QEMU, we don't initialize RAM (we don't have to).

- In OVMF, we have a separate SmmAccessPei implementation, which does
  not rely on such a HOB.

Rebasing the SMM platform code in OVMF to SmramHob is out of scope, in
my opinion. The current layout of drivers works fine; we just need a new
bit pattern (11 binary) in the ESMRAMC.TSEG_SZ bitfield, and OVMF should
be able to query QEMU as to what that bit pattern translates to,
size-wise. That's all.

> At same time, I also saw a solution to use library to produce
> SmmAccessPpi. In such case, there is no dependency expression, because
> one module does 2 things: 1) init memory, 2) produce SmmAccessPpi.

I agree that this is a technical possiblity, but for OVMF it is again
out of scope. One reason is separation of concepts. Another reason is
that OVMF can be built without SMM support (that's the default
actually), and for such a build the library approach would require a
Null instance of whatever new library class we would invent. Too much
churn.

Jordan, can we please go ahead with the posted approach? If you strongly
prefer dynamic PCDs (which I *slightly* dislike), I can do that --
hacking a spurious gEfiPeiMemoryDiscoveredPpiGuid dependency into
SmmAccessPei.inf, as a serialization barrier between PCD setting and PCD
consumption --, for the sake of making progress on this series.

Thanks,
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel