[PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

In <https://bugzilla.redhat.com/show_bug.cgi?id=1447027> we found that
the SMM_REQUIRE build of OVMF cannot boot with as many VCPUs as we'd
like, due to SMRAM exhaustion (even with the largest TSEG, 8MB).

Related thread on edk2-devel:
<https://lists.01.org/pipermail/edk2-devel/2017-May/010371.html>.

The QEMU patch at
<http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com>
adds a new device property / PCI config register to the Q35 board's DRAM
controller. The property allows the user (and new Q35 machine types by
default) to specify an arbitrary TSEG size (expressed as a number of
megabytes). The register enables the firmware to query the size /
availability of the feature. The extended size, when available, can be
selected by writing the "11" bitmask to the ESMRAMC.TSEG_SZ register
bit-field. This bitmask (similarly to the invented register's location
in PCI config space) is defined as reserved in the original Q35/MCH
spec.

This series adds support for the feature to OVMF. When an extended TSEG
is offered, OVMF will choose it.

The first four patches extract Q35TsegSizeLib, centralizing the
interpretation of ESMRAMC.TSEG_SZ between PlatformPei, SmmAccessPei and
SmmAccess2Dxe. This subset of patches incurs no observable change in
behavior.

The last patch implements the feature in Q35TsegSizeLib.

Repo:   https://github.com/lersek/edk2.git
Branch: extended_tseg_bz1447027

Cc: Jordan Justen <jordan.l.justen@intel.com>

Thanks
Laszlo

Laszlo Ersek (5):
  OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)
  OvmfPkg/PlatformPei: rebase to Q35TsegSizeLib
  OvmfPkg/SmmAccess: rebase code unique to SmmAccessPei to
    Q35TsegSizeLib
  OvmfPkg/SmmAccess: rebase shared PEIM/DXE code to Q35TsegSizeLib
  OvmfPkg/Q35TsegSizeLib: recognize an extended TSEG when QEMU offers it

 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   4 +
 OvmfPkg/Include/Library/Q35TsegSizeLib.h          |  74 +++++++
 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c   | 230 ++++++++++++++++++++
 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf |  47 ++++
 OvmfPkg/OvmfPkg.dec                               |  13 +-
 OvmfPkg/OvmfPkgIa32.dsc                           |   1 +
 OvmfPkg/OvmfPkgIa32X64.dsc                        |   1 +
 OvmfPkg/OvmfPkgX64.dsc                            |   1 +
 OvmfPkg/PlatformPei/MemDetect.c                   |   7 +-
 OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
 OvmfPkg/SmmAccess/SmmAccess2Dxe.inf               |   1 +
 OvmfPkg/SmmAccess/SmmAccessPei.c                  |   7 +-
 OvmfPkg/SmmAccess/SmmAccessPei.inf                |   4 +-
 OvmfPkg/SmmAccess/SmramInternal.c                 |  13 +-
 14 files changed, 384 insertions(+), 21 deletions(-)
 create mode 100644 OvmfPkg/Include/Library/Q35TsegSizeLib.h
 create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
 create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf

-- 
2.9.3

[PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

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

[PATCH 2/5] OvmfPkg/PlatformPei: rebase to Q35TsegSizeLib

[Laszlo Ersek]

Replace

  FixedPcdGet8 (PcdQ35TsegMbytes)

macro invocations with

  Q35TsegSizeGetPreferredMbytes ()

function invocations. This causes no change in observable behavior.

Remember that Q35TsegSizeGetPreferredMbytes() -- indirectly -- logs an
error and calls CpuDeadLoop() if it is executed on a non-Q35 board.  This
is safe and intentional, because all the converted call sites are reached
only when the PcdSmmSmramRequire Feature PCD is set, for which Q35 has
always been a hard requirement.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/PlatformPei/PlatformPei.inf | 2 +-
 OvmfPkg/PlatformPei/MemDetect.c     | 7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
index a1e12c1fc7e2..6125b52b8466 100644
--- a/OvmfPkg/PlatformPei/PlatformPei.inf
+++ b/OvmfPkg/PlatformPei/PlatformPei.inf
@@ -57,14 +57,15 @@ [LibraryClasses]
   PeiServicesLib
   PeiServicesTablePointerLib
   PeimEntryPoint
   QemuFwCfgLib
   QemuFwCfgS3Lib
   MtrrLib
   PcdLib
+  Q35TsegSizeLib
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfPeiMemFvSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
@@ -78,15 +79,14 @@ [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdPciIoBase
   gUefiOvmfPkgTokenSpaceGuid.PcdPciIoSize
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Base
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio32Size
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Base
   gUefiOvmfPkgTokenSpaceGuid.PcdPciMmio64Size
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDecompressionScratchEnd
-  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
   gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
   gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvStoreReserved
   gEfiMdeModulePkgTokenSpaceGuid.PcdPciDisableBusEnumeration
   gEfiMdeModulePkgTokenSpaceGuid.PcdDxeIplSwitchToLongMode
   gEfiMdeModulePkgTokenSpaceGuid.PcdUse1GPageTable
diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
index af96a04d194a..795a3f47d12a 100644
--- a/OvmfPkg/PlatformPei/MemDetect.c
+++ b/OvmfPkg/PlatformPei/MemDetect.c
@@ -29,14 +29,15 @@ Module Name:
 #include <Library/HobLib.h>
 #include <Library/IoLib.h>
 #include <Library/PcdLib.h>
 #include <Library/PeimEntryPoint.h>
 #include <Library/ResourcePublicationLib.h>
 #include <Library/MtrrLib.h>
 #include <Library/QemuFwCfgLib.h>
+#include <Library/Q35TsegSizeLib.h>
 
 #include "Platform.h"
 #include "Cmos.h"
 
 UINT8 mPhysMemAddressWidth;
 
 STATIC UINT32 mS3AcpiReservedMemoryBase;
@@ -344,15 +345,15 @@ PublishPeiMemory (
   UINT32                      PeiMemoryCap;
 
   LowerMemorySize = GetSystemMemorySizeBelow4gb ();
   if (FeaturePcdGet (PcdSmmSmramRequire)) {
     //
     // TSEG is chipped from the end of low RAM
     //
-    LowerMemorySize -= FixedPcdGet8 (PcdQ35TsegMbytes) * SIZE_1MB;
+    LowerMemorySize -= Q35TsegSizeGetPreferredMbytes () * SIZE_1MB;
   }
 
   //
   // If S3 is supported, then the S3 permanent PEI memory is placed next,
   // downwards. Its size is primarily dictated by CpuMpPei. The formula below
   // is an approximation.
   //
@@ -452,15 +453,15 @@ QemuInitializeRam (
     // Create memory HOBs
     //
     AddMemoryRangeHob (0, BASE_512KB + BASE_128KB);
 
     if (FeaturePcdGet (PcdSmmSmramRequire)) {
       UINT32 TsegSize;
 
-      TsegSize = FixedPcdGet8 (PcdQ35TsegMbytes) * SIZE_1MB;
+      TsegSize = Q35TsegSizeGetPreferredMbytes () * SIZE_1MB;
       AddMemoryRangeHob (BASE_1MB, LowerMemorySize - TsegSize);
       AddReservedMemoryBaseSizeHob (LowerMemorySize - TsegSize, TsegSize,
         TRUE);
     } else {
       AddMemoryRangeHob (BASE_1MB, LowerMemorySize);
     }
 
@@ -601,15 +602,15 @@ InitializeRamRegions (
     if (FeaturePcdGet (PcdSmmSmramRequire)) {
       UINT32 TsegSize;
 
       //
       // Make sure the TSEG area that we reported as a reserved memory resource
       // cannot be used for reserved memory allocations.
       //
-      TsegSize = FixedPcdGet8 (PcdQ35TsegMbytes) * SIZE_1MB;
+      TsegSize = Q35TsegSizeGetPreferredMbytes () * SIZE_1MB;
       BuildMemoryAllocationHob (
         GetSystemMemorySizeBelow4gb() - TsegSize,
         TsegSize,
         EfiReservedMemoryType
         );
     }
   }
-- 
2.9.3

[PATCH 3/5] OvmfPkg/SmmAccess: rebase code unique to SmmAccessPei to Q35TsegSizeLib

[Laszlo Ersek]

SmmAccessPei and SmmAccess2Dxe share the internals between their
PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL implementations,
respectively, in "SmramInternal.c".

In this patch, convert the code that is unique to SmmAccessPei -- namely
SmmAccessPeiEntryPoint() -- to Q35TsegSizeLib client code, as follows:

- Replace FixedPcdGet8 (PcdQ35TsegMbytes) macro invocations with
  Q35TsegSizeGetPreferredMbytes () function calls.

- Replace any mapping, from FixedPcdGet8 (PcdQ35TsegMbytes) to
  MCH_ESMRAMC_TSEG_xMB bitmask macros, with
  Q35TsegSizeGetPreferredEsmramcTsegSzMask() function calls.

This causes no change in observable behavior.

After this patch, no module INF file except
"OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf" refers to
PcdQ35TsegMbytes.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/SmmAccess/SmmAccessPei.inf | 4 +---
 OvmfPkg/SmmAccess/SmmAccessPei.c   | 7 +++----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf
index 3908b085da3a..c07c603cb663 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.inf
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf
@@ -51,19 +51,17 @@ [LibraryClasses]
   DebugLib
   HobLib
   IoLib
   PcdLib
   PciLib
   PeiServicesLib
   PeimEntryPoint
+  Q35TsegSizeLib
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
-[FixedPcd]
-  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
-
 [Ppis]
   gPeiSmmAccessPpiGuid           ## PRODUCES
 
 [Depex]
   TRUE
diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
index a4ce610a4650..104671a15c64 100644
--- a/OvmfPkg/SmmAccess/SmmAccessPei.c
+++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
@@ -28,14 +28,15 @@
 #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 <Library/Q35TsegSizeLib.h>
 #include <Ppi/SmmAccess.h>
 
 #include <OvmfPlatforms.h>
 
 #include "SmramInternal.h"
 
 //
@@ -315,25 +316,23 @@ SmmAccessPeiEntryPoint (
   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);
+    (TopOfLowRamMb - Q35TsegSizeGetPreferredMbytes ()) << 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 |= Q35TsegSizeGetPreferredEsmramcTsegSzMask ();
   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.
   //
-- 
2.9.3

[PATCH 4/5] OvmfPkg/SmmAccess: rebase shared PEIM/DXE code to Q35TsegSizeLib

[Laszlo Ersek]

SmmAccessPei and SmmAccess2Dxe share the internals between their
PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL implementations,
respectively, in "SmramInternal.c".

In this patch, convert "SmramInternal.c" to Q35TsegSizeLib client code.
Replace any mapping, from MCH_ESMRAMC_TSEG_xMB bitmask macros to byte
counts, with

  (Q35TsegSizeConvertEsmramcValToMbytes (EsmramcVal) * SIZE_1MB)

expressions.

This causes no change in observable behavior.

After this patch, the conversion to Q35TsegSizeLib is complete.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/SmmAccess/SmmAccess2Dxe.inf |  1 +
 OvmfPkg/SmmAccess/SmramInternal.c   | 13 ++++++-------
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
index 31e4dfa02991..f591b837bb62 100644
--- a/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
+++ b/OvmfPkg/SmmAccess/SmmAccess2Dxe.inf
@@ -41,14 +41,15 @@ [Packages]
   MdePkg/MdePkg.dec
   OvmfPkg/OvmfPkg.dec
 
 [LibraryClasses]
   DebugLib
   PcdLib
   PciLib
+  Q35TsegSizeLib
   UefiBootServicesTableLib
   UefiDriverEntryPoint
 
 [Protocols]
   gEfiSmmAccess2ProtocolGuid   ## PRODUCES
 
 [FeaturePcd]
diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c
index c3267ca94031..e7a7acbf1f8b 100644
--- a/OvmfPkg/SmmAccess/SmramInternal.c
+++ b/OvmfPkg/SmmAccess/SmramInternal.c
@@ -14,14 +14,15 @@
 
 **/
 
 #include <Guid/AcpiS3Context.h>
 #include <IndustryStandard/Q35MchIch9.h>
 #include <Library/DebugLib.h>
 #include <Library/PciLib.h>
+#include <Library/Q35TsegSizeLib.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.
@@ -128,15 +129,15 @@ SmramAccessGetCapabilities (
   IN OUT UINTN                *SmramMapSize,
   IN OUT EFI_SMRAM_DESCRIPTOR *SmramMap
   )
 {
   UINTN  OriginalSize;
   UINT32 TsegMemoryBaseMb, TsegMemoryBase;
   UINT64 CommonRegionState;
-  UINT8  TsegSizeBits;
+  UINT8  EsmramcVal;
 
   OriginalSize  = *SmramMapSize;
   *SmramMapSize = DescIdxCount * sizeof *SmramMap;
   if (OriginalSize < *SmramMapSize) {
     return EFI_BUFFER_TOO_SMALL;
   }
 
@@ -162,27 +163,25 @@ SmramAccessGetCapabilities (
   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.
+  // Read the ESMRAMC register so we can extract the TSEG size bits.
   //
-  TsegSizeBits = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC)) &
-                 MCH_ESMRAMC_TSEG_MASK;
+  EsmramcVal = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC));
 
   //
   // 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;
+    (Q35TsegSizeConvertEsmramcValToMbytes (EsmramcVal) * SIZE_1MB) -
+    SmramMap[DescIdxSmmS3ResumeState].PhysicalSize;
   SmramMap[DescIdxMain].RegionState = CommonRegionState;
 
   return EFI_SUCCESS;
 }
-- 
2.9.3

[PATCH 5/5] OvmfPkg/Q35TsegSizeLib: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

This patch interfaces with the QEMU feature introduced in QEMU patch

  q35/mch: implement extended TSEG sizes

Excerpt:

> The q35 machine type currently lets the guest firmware select a 1MB, 2MB
> or 8MB TSEG (basically, SMRAM) size. In edk2/OVMF, we use 8MB, but even
> that is not enough when a lot of VCPUs (more than approx. 224) are
> configured -- SMRAM footprint scales largely proportionally with VCPU
> count.
>
> Introduce a new property for "mch" called "extended-tseg-mbytes", which
> expresses (in megabytes) the user's choice of TSEG (SMRAM) size.
>
> Invent a new, QEMU-specific register in the config space of the DRAM
> Controller, at offset 0x50, in order to allow guest firmware to query
> the TSEG (SMRAM) size.
>
> According to Intel Document Number 316966-002, Table 5-1 "DRAM
> Controller Register Address Map (D0:F0)":
>
> [...]
>
> Offsets 0x50 and 0x51 are not listed in Table 5-1. They are also not
> part of the standard PCI config space header. And they precede the
> capability list as well, which starts at 0xe0 for this device.
>
> When the guest writes value 0xffff to this register, the value that can
> be read back is that of "mch.extended-tseg-mbytes" -- unless it remains
> 0xffff. The guest is required to write 0xffff first (as opposed to a
> read-only register) because PCI config space is generally not cleared on
> QEMU reset, and after S3 resume or reboot, new guest firmware running on
> old QEMU could read a guest OS-injected value from this register.
>
> After reading the available "extended" TSEG size, the guest firmware may
> actually request that TSEG size by writing pattern 11b to the ESMRAMC
> register's TSEG_SZ bit-field. (The Intel spec referenced above defines
> only patterns 00b (1MB), 01b (2MB) and 10b (8MB); 11b is reserved.)
>
> On the QEMU command line, the value can be set with
>
>   -global mch.extended-tseg-mbytes=N
>
> The default value for 2.10+ q35 machine types is 16. [...] Users are
> responsible for choosing sensible TSEG sizes.
>
> On 2.9 and earlier q35 machine types, the default value is 0. This lets
> the 11b bit pattern in ESMRAMC.TSEG_SZ, and the register at offset 0x50,
> keep their original behavior.

Relegate PcdQ35TsegMbytes to a fallback role, renaming it to
PcdQ35TsegDefaultMbytes.

Cc: Jordan Justen <jordan.l.justen@intel.com>
Ref: https://bugzilla.redhat.com/show_bug.cgi?id=1447027
Ref: https://lists.01.org/pipermail/edk2-devel/2017-May/010456.html
Ref: http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 OvmfPkg/OvmfPkg.dec                               |  8 ++--
 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf |  2 +-
 OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |  4 ++
 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c   | 46 +++++++++++++++++++-
 4 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 7b9220369b95..2cae6a189d62 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -92,19 +92,21 @@ [PcdsFixedAtBuild]
   #  MaxTarget and MaxLun, independently, should the host report higher values,
   #  so that scanning the number of devices given by their product is still
   #  acceptably fast.
   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxTargetLimit|31|UINT16|6
   gUefiOvmfPkgTokenSpaceGuid.PcdVirtioScsiMaxLunLimit|7|UINT32|7
 
   ## The following setting controls how many megabytes we configure as TSEG on
-  #  Q35, for SMRAM purposes. Permitted values are: 1, 2, 8. Other values cause
-  #  undefined behavior.
+  #  Q35, for SMRAM purposes, by default. Permitted values are: 1, 2, 8. Other
+  #  values cause undefined behavior.
   #
   #  This PCD is only consulted if PcdSmmSmramRequire is TRUE (see below).
-  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8|UINT8|0x20
+  #  Furthermore, if QEMU offers an extended TSEG (as a Q35 extension), then
+  #  this PCD is ignored.
+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegDefaultMbytes|8|UINT8|0x20
 
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogBase|0x0|UINT32|0x8
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageEventLogSize|0x0|UINT32|0x9
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareFdSize|0x0|UINT32|0xa
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFirmwareBlockSize|0|UINT32|0xb
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageVariableBase|0x0|UINT32|0xc
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashNvStorageFtwSpareBase|0x0|UINT32|0xd
diff --git a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
index 8f99e55b8c48..b7467d1a716c 100644
--- a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
+++ b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
@@ -40,8 +40,8 @@ [LibraryClasses]
   PcdLib
   PciLib
 
 [FeaturePcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdSmmSmramRequire
 
 [FixedPcd]
-  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegDefaultMbytes
diff --git a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
index f480455ae432..68485bec71f7 100644
--- a/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
+++ b/OvmfPkg/Include/IndustryStandard/Q35MchIch9.h
@@ -29,14 +29,17 @@
 #define INTEL_Q35_MCH_DEVICE_ID 0x29C0
 
 //
 // B/D/F/Type: 0/0/0/PCI
 //
 #define DRAMC_REGISTER_Q35(Offset) PCI_LIB_ADDRESS (0, 0, 0, (Offset))
 
+#define MCH_EXT_TSEG_MB       0x50
+#define MCH_EXT_TSEG_MB_QUERY   0xFFFF
+
 #define MCH_GGC               0x52
 #define MCH_GGC_IVD             BIT1
 
 #define MCH_PCIEXBAR_LOW      0x60
 #define MCH_PCIEXBAR_LOWMASK    0x0FFFFFFF
 #define MCH_PCIEXBAR_BUS_FF     0
 #define MCH_PCIEXBAR_EN         BIT0
@@ -50,14 +53,15 @@
 
 #define MCH_ESMRAMC           0x9E
 #define MCH_ESMRAMC_H_SMRAME    BIT7
 #define MCH_ESMRAMC_E_SMERR     BIT6
 #define MCH_ESMRAMC_SM_CACHE    BIT5
 #define MCH_ESMRAMC_SM_L1       BIT4
 #define MCH_ESMRAMC_SM_L2       BIT3
+#define MCH_ESMRAMC_TSEG_EXT    (BIT2 | BIT1)
 #define MCH_ESMRAMC_TSEG_8MB    BIT2
 #define MCH_ESMRAMC_TSEG_2MB    BIT1
 #define MCH_ESMRAMC_TSEG_1MB    0
 #define MCH_ESMRAMC_TSEG_MASK   (BIT2 | BIT1)
 #define MCH_ESMRAMC_T_EN        BIT0
 
 #define MCH_GBSM              0xA4
diff --git a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
index db57a8b308de..01054a093f51 100644
--- a/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
+++ b/OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
@@ -17,14 +17,15 @@
 #include <Library/PcdLib.h>
 #include <Library/PciLib.h>
 #include <Library/Q35TsegSizeLib.h>
 #include <OvmfPlatforms.h>
 
 STATIC BOOLEAN mPreferencesInitialized;
 STATIC UINT8   mPreferredEsmramcTsegSzMask;
+STATIC UINT16  mExtendedTsegMbytes;
 
 /**
   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.
@@ -59,15 +60,49 @@ Q35TsegSizeGetPreferences (
       ));
     ASSERT (FALSE);
     CpuDeadLoop ();
   }
 
   mPreferencesInitialized = TRUE;
 
-  switch (FixedPcdGet8 (PcdQ35TsegMbytes)) {
+  //
+  // Check if QEMU offers an extended TSEG.
+  //
+  // This can be seen from writing MCH_EXT_TSEG_MB_QUERY to the MCH_EXT_TSEG_MB
+  // register, and reading back the register.
+  //
+  // On a QEMU machine type that does not offer an extended TSEG, the initial
+  // write overwrites whatever value a malicious guest OS may have placed in
+  // the (unimplemented) register, before entering S3 or rebooting.
+  // Subsequently, the read returns MCH_EXT_TSEG_MB_QUERY unchanged.
+  //
+  // On a QEMU machine type that offers an extended TSEG, the initial write
+  // triggers an update to the register. Subsequently, the value read back
+  // (which is guaranteed to differ from MCH_EXT_TSEG_MB_QUERY) tells us the
+  // number of megabytes.
+  //
+  PciWrite16 (DRAMC_REGISTER_Q35 (MCH_EXT_TSEG_MB), MCH_EXT_TSEG_MB_QUERY);
+  mExtendedTsegMbytes = PciRead16 (DRAMC_REGISTER_Q35 (MCH_EXT_TSEG_MB));
+  if (mExtendedTsegMbytes != MCH_EXT_TSEG_MB_QUERY) {
+    DEBUG ((
+      DEBUG_INFO,
+      "%a: %a: QEMU offers an extended TSEG (%d MB)\n",
+      gEfiCallerBaseName,
+      __FUNCTION__,
+      mExtendedTsegMbytes
+      ));
+
+    mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_EXT;
+    return;
+  }
+
+  //
+  // Fall back to the default TSEG size otherwise.
+  //
+  switch (FixedPcdGet8 (PcdQ35TsegDefaultMbytes)) {
   case 1:
     mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_1MB;
     break;
   case 2:
     mPreferredEsmramcTsegSzMask = MCH_ESMRAMC_TSEG_2MB;
     break;
   case 8:
@@ -161,14 +196,23 @@ Q35TsegSizeConvertEsmramcValToMbytes (
     break;
   case MCH_ESMRAMC_TSEG_2MB:
     Mbytes = 2;
     break;
   case MCH_ESMRAMC_TSEG_8MB:
     Mbytes = 8;
     break;
+  case MCH_ESMRAMC_TSEG_EXT:
+    if (mExtendedTsegMbytes != MCH_EXT_TSEG_MB_QUERY) {
+      Mbytes = mExtendedTsegMbytes;
+      break;
+    }
+    //
+    // Fall through otherwise -- QEMU didn't offer an extended TSEG, so this
+    // should never happen.
+    //
   default:
     DEBUG ((
       DEBUG_ERROR,
       "%a: %a: unknown TsegSizeBits=0x%02x\n",
       gEfiCallerBaseName,
       __FUNCTION__,
       TsegSizeBits
-- 
2.9.3

Re: [PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

Jordan,

On 06/08/17 19:13, Laszlo Ersek wrote:
> In <https://bugzilla.redhat.com/show_bug.cgi?id=1447027> we found that
> the SMM_REQUIRE build of OVMF cannot boot with as many VCPUs as we'd
> like, due to SMRAM exhaustion (even with the largest TSEG, 8MB).
> 
> Related thread on edk2-devel:
> <https://lists.01.org/pipermail/edk2-devel/2017-May/010371.html>.
> 
> The QEMU patch at
> <http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com>
> adds a new device property / PCI config register to the Q35 board's DRAM
> controller. The property allows the user (and new Q35 machine types by
> default) to specify an arbitrary TSEG size (expressed as a number of
> megabytes). The register enables the firmware to query the size /
> availability of the feature. The extended size, when available, can be
> selected by writing the "11" bitmask to the ESMRAMC.TSEG_SZ register
> bit-field. This bitmask (similarly to the invented register's location
> in PCI config space) is defined as reserved in the original Q35/MCH
> spec.
> 
> This series adds support for the feature to OVMF. When an extended TSEG
> is offered, OVMF will choose it.
> 
> The first four patches extract Q35TsegSizeLib, centralizing the
> interpretation of ESMRAMC.TSEG_SZ between PlatformPei, SmmAccessPei and
> SmmAccess2Dxe. This subset of patches incurs no observable change in
> behavior.
> 
> The last patch implements the feature in Q35TsegSizeLib.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: extended_tseg_bz1447027
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>

Can you please review this set?

Thanks,
Laszlo


> Laszlo Ersek (5):
>   OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)
>   OvmfPkg/PlatformPei: rebase to Q35TsegSizeLib
>   OvmfPkg/SmmAccess: rebase code unique to SmmAccessPei to
>     Q35TsegSizeLib
>   OvmfPkg/SmmAccess: rebase shared PEIM/DXE code to Q35TsegSizeLib
>   OvmfPkg/Q35TsegSizeLib: recognize an extended TSEG when QEMU offers it
> 
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   4 +
>  OvmfPkg/Include/Library/Q35TsegSizeLib.h          |  74 +++++++
>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c   | 230 ++++++++++++++++++++
>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf |  47 ++++
>  OvmfPkg/OvmfPkg.dec                               |  13 +-
>  OvmfPkg/OvmfPkgIa32.dsc                           |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                        |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                            |   1 +
>  OvmfPkg/PlatformPei/MemDetect.c                   |   7 +-
>  OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf               |   1 +
>  OvmfPkg/SmmAccess/SmmAccessPei.c                  |   7 +-
>  OvmfPkg/SmmAccess/SmmAccessPei.inf                |   4 +-
>  OvmfPkg/SmmAccess/SmramInternal.c                 |  13 +-
>  14 files changed, 384 insertions(+), 21 deletions(-)
>  create mode 100644 OvmfPkg/Include/Library/Q35TsegSizeLib.h
>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
> 

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Jordan Justen]

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

Re: [PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Jordan Justen]

On 2017-06-08 10:13:28, Laszlo Ersek wrote:
> In <https://bugzilla.redhat.com/show_bug.cgi?id=1447027> we found that
> the SMM_REQUIRE build of OVMF cannot boot with as many VCPUs as we'd
> like, due to SMRAM exhaustion (even with the largest TSEG, 8MB).
> 
> Related thread on edk2-devel:
> <https://lists.01.org/pipermail/edk2-devel/2017-May/010371.html>.
> 
> The QEMU patch at
> <http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com>

This patch hasn't been merged to qemu yet, right? Would you propose
that we wait until this is merged to qemu master, or present in a qemu
release before merging the OVMF support? At what point does qemu
consider it an unchangable supported feature?

-Jordan

> adds a new device property / PCI config register to the Q35 board's DRAM
> controller. The property allows the user (and new Q35 machine types by
> default) to specify an arbitrary TSEG size (expressed as a number of
> megabytes). The register enables the firmware to query the size /
> availability of the feature. The extended size, when available, can be
> selected by writing the "11" bitmask to the ESMRAMC.TSEG_SZ register
> bit-field. This bitmask (similarly to the invented register's location
> in PCI config space) is defined as reserved in the original Q35/MCH
> spec.
> 
> This series adds support for the feature to OVMF. When an extended TSEG
> is offered, OVMF will choose it.
> 
> The first four patches extract Q35TsegSizeLib, centralizing the
> interpretation of ESMRAMC.TSEG_SZ between PlatformPei, SmmAccessPei and
> SmmAccess2Dxe. This subset of patches incurs no observable change in
> behavior.
> 
> The last patch implements the feature in Q35TsegSizeLib.
> 
> Repo:   https://github.com/lersek/edk2.git
> Branch: extended_tseg_bz1447027
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> 
> Thanks
> Laszlo
> 
> Laszlo Ersek (5):
>   OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)
>   OvmfPkg/PlatformPei: rebase to Q35TsegSizeLib
>   OvmfPkg/SmmAccess: rebase code unique to SmmAccessPei to
>     Q35TsegSizeLib
>   OvmfPkg/SmmAccess: rebase shared PEIM/DXE code to Q35TsegSizeLib
>   OvmfPkg/Q35TsegSizeLib: recognize an extended TSEG when QEMU offers it
> 
>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   4 +
>  OvmfPkg/Include/Library/Q35TsegSizeLib.h          |  74 +++++++
>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c   | 230 ++++++++++++++++++++
>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf |  47 ++++
>  OvmfPkg/OvmfPkg.dec                               |  13 +-
>  OvmfPkg/OvmfPkgIa32.dsc                           |   1 +
>  OvmfPkg/OvmfPkgIa32X64.dsc                        |   1 +
>  OvmfPkg/OvmfPkgX64.dsc                            |   1 +
>  OvmfPkg/PlatformPei/MemDetect.c                   |   7 +-
>  OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf               |   1 +
>  OvmfPkg/SmmAccess/SmmAccessPei.c                  |   7 +-
>  OvmfPkg/SmmAccess/SmmAccessPei.inf                |   4 +-
>  OvmfPkg/SmmAccess/SmramInternal.c                 |  13 +-
>  14 files changed, 384 insertions(+), 21 deletions(-)
>  create mode 100644 OvmfPkg/Include/Library/Q35TsegSizeLib.h
>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
> 
> -- 
> 2.9.3
> 

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

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

Re: [PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

On 06/19/17 20:09, Jordan Justen wrote:
> On 2017-06-08 10:13:28, Laszlo Ersek wrote:
>> In <https://bugzilla.redhat.com/show_bug.cgi?id=1447027> we found that
>> the SMM_REQUIRE build of OVMF cannot boot with as many VCPUs as we'd
>> like, due to SMRAM exhaustion (even with the largest TSEG, 8MB).
>>
>> Related thread on edk2-devel:
>> <https://lists.01.org/pipermail/edk2-devel/2017-May/010371.html>.
>>
>> The QEMU patch at
>> <http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com>
> 
> This patch hasn't been merged to qemu yet, right? Would you propose
> that we wait until this is merged to qemu master, or present in a qemu
> release before merging the OVMF support? At what point does qemu
> consider it an unchangable supported feature?

The QEMU patch is now part of a pending PULL request:

http://lists.nongnu.org/archive/html/qemu-devel/2017-06/msg03901.html

Given that the QEMU patch (intentionally) preserves compatibility /
behavior for earlier machine types, we can safely commit the OVMF series
once upstream QEMU merges the patch. Using pc-q35-2.9 or earlier machine
types, the feature won't be (or won't appear) available; with
pc-q35-2.10 or later, the feature will be turned on.

I agree we should wait until Michael's qemu PULL req is actually merged.
There's no reason (specific to QEMU) to wait with the OVMF patches
longer than that. (SeaBIOS follows the same method.)

Thanks
Laszlo


>> adds a new device property / PCI config register to the Q35 board's DRAM
>> controller. The property allows the user (and new Q35 machine types by
>> default) to specify an arbitrary TSEG size (expressed as a number of
>> megabytes). The register enables the firmware to query the size /
>> availability of the feature. The extended size, when available, can be
>> selected by writing the "11" bitmask to the ESMRAMC.TSEG_SZ register
>> bit-field. This bitmask (similarly to the invented register's location
>> in PCI config space) is defined as reserved in the original Q35/MCH
>> spec.
>>
>> This series adds support for the feature to OVMF. When an extended TSEG
>> is offered, OVMF will choose it.
>>
>> The first four patches extract Q35TsegSizeLib, centralizing the
>> interpretation of ESMRAMC.TSEG_SZ between PlatformPei, SmmAccessPei and
>> SmmAccess2Dxe. This subset of patches incurs no observable change in
>> behavior.
>>
>> The last patch implements the feature in Q35TsegSizeLib.
>>
>> Repo:   https://github.com/lersek/edk2.git
>> Branch: extended_tseg_bz1447027
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>
>> Thanks
>> Laszlo
>>
>> Laszlo Ersek (5):
>>   OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)
>>   OvmfPkg/PlatformPei: rebase to Q35TsegSizeLib
>>   OvmfPkg/SmmAccess: rebase code unique to SmmAccessPei to
>>     Q35TsegSizeLib
>>   OvmfPkg/SmmAccess: rebase shared PEIM/DXE code to Q35TsegSizeLib
>>   OvmfPkg/Q35TsegSizeLib: recognize an extended TSEG when QEMU offers it
>>
>>  OvmfPkg/Include/IndustryStandard/Q35MchIch9.h     |   4 +
>>  OvmfPkg/Include/Library/Q35TsegSizeLib.h          |  74 +++++++
>>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c   | 230 ++++++++++++++++++++
>>  OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf |  47 ++++
>>  OvmfPkg/OvmfPkg.dec                               |  13 +-
>>  OvmfPkg/OvmfPkgIa32.dsc                           |   1 +
>>  OvmfPkg/OvmfPkgIa32X64.dsc                        |   1 +
>>  OvmfPkg/OvmfPkgX64.dsc                            |   1 +
>>  OvmfPkg/PlatformPei/MemDetect.c                   |   7 +-
>>  OvmfPkg/PlatformPei/PlatformPei.inf               |   2 +-
>>  OvmfPkg/SmmAccess/SmmAccess2Dxe.inf               |   1 +
>>  OvmfPkg/SmmAccess/SmmAccessPei.c                  |   7 +-
>>  OvmfPkg/SmmAccess/SmmAccessPei.inf                |   4 +-
>>  OvmfPkg/SmmAccess/SmramInternal.c                 |  13 +-
>>  14 files changed, 384 insertions(+), 21 deletions(-)
>>  create mode 100644 OvmfPkg/Include/Library/Q35TsegSizeLib.h
>>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.c
>>  create mode 100644 OvmfPkg/Library/Q35TsegSizeLib/Q35TsegSizeLib.inf
>>
>> -- 
>> 2.9.3
>>

Re: [PATCH 0/5] OvmfPkg: recognize an extended TSEG when QEMU offers it

[Laszlo Ersek]

On 06/20/17 00:39, Laszlo Ersek wrote:
> On 06/19/17 20:09, Jordan Justen wrote:
>> On 2017-06-08 10:13:28, Laszlo Ersek wrote:
>>> In <https://bugzilla.redhat.com/show_bug.cgi?id=1447027> we found that
>>> the SMM_REQUIRE build of OVMF cannot boot with as many VCPUs as we'd
>>> like, due to SMRAM exhaustion (even with the largest TSEG, 8MB).
>>>
>>> Related thread on edk2-devel:
>>> <https://lists.01.org/pipermail/edk2-devel/2017-May/010371.html>.
>>>
>>> The QEMU patch at
>>> <http://mid.mail-archive.com/20170608161013.17920-1-lersek@redhat.com>
>>
>> This patch hasn't been merged to qemu yet, right?

Merged now: QEMU commit 2f295167e0c4.

Thanks
Laszlo

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Yao, Jiewen]

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

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

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

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Jordan Justen]

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

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

On 06/26/17 19:59, Jordan Justen wrote:
> 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.

Maybe so, but that's only because edk2 requires all libraries (even
one-off utility ones) that are used by at least two modules to have
full-blown library classes. (NULL class library resolution is different,
but those in turn require constructors.) This makes the overhead for
small utility libraries worse.

Specifically, the first four patches in the series can be considered a
refactoring / improvement per se, because they centralize the
TSEG_SZ<->megabytes mapping to one module.

Extracting the mapping is also useful because the last patch can then
add the feature to all affected modules at once. If we proceed
piecemeal, then there's going to be a stage where the PEI phase modules
know how to handle extended TSEG, but the DXE phase doesn't. Not tragic,
but not optimal for bisection.

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

Arbitrary and quite ugly, but I guess I can live with it.

> * 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

This last bullet is a functional change for SmramAccessGetCapabilities()
that I wanted to avoid -- I sought to preserve the logic of working off
of the currently set (and possibly locked) contents of ESMRAMC.TSEG_SZ.

I guess the idea is still doable if SmramInternal.c introduces a global
variable for caching the PCD at module startup -- setting the variable
from the PCD would be the responsibility of each separate module, both
PEIM and DXE driver --, and then SmramAccessGetCapabilities() could use
the global variable if it read "11" binary from the TSEG_SZ bit-field
register.

This would be in effect the same as the current patch series, except the
bit-field<->MB mappings would remain scattered over the tree.

Please confirm if you are OK with the above and I'll spin a v2.

Thanks
Laszlo

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

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Jordan Justen]

On 2017-06-26 12:12:45, Laszlo Ersek wrote:
> On 06/26/17 19:59, Jordan Justen wrote:
> > 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.
> 
> Arbitrary and quite ugly, but I guess I can live with it.

I'm assuming you mean that the gEfiPeiMemoryDiscoveredPpiGuid
dependency is arbitrary and quite ugly.

I guess it doesn't seem arbitrary and ugly to me, since on most
firmware you'd need to detect memory before considering anything SMM
related. We should probably still minimize memory usage until Platform
PEI 'installs' it, even though no configuration is required.

> > * 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
> 
> This last bullet is a functional change for SmramAccessGetCapabilities()
> that I wanted to avoid -- I sought to preserve the logic of working off
> of the currently set (and possibly locked) contents of ESMRAMC.TSEG_SZ.
> 
> I guess the idea is still doable if SmramInternal.c introduces a global
> variable for caching the PCD at module startup -- setting the variable
> from the PCD would be the responsibility of each separate module, both
> PEIM and DXE driver --, and then SmramAccessGetCapabilities() could use
> the global variable if it read "11" binary from the TSEG_SZ bit-field
> register.

Why can't SmramInternal.c read the size from the PCD directly?

-Jordan

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

On 06/27/17 00:36, Jordan Justen wrote:
> On 2017-06-26 12:12:45, Laszlo Ersek wrote:
>> On 06/26/17 19:59, Jordan Justen wrote:
>>> 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.
>>
>> Arbitrary and quite ugly, but I guess I can live with it.
> 
> I'm assuming you mean that the gEfiPeiMemoryDiscoveredPpiGuid
> dependency is arbitrary and quite ugly.
> 
> I guess it doesn't seem arbitrary and ugly to me, since on most
> firmware you'd need to detect memory before considering anything SMM
> related.

OK.

> We should probably still minimize memory usage until Platform
> PEI 'installs' it, even though no configuration is required.
> 
>>> * 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
>>
>> This last bullet is a functional change for SmramAccessGetCapabilities()
>> that I wanted to avoid -- I sought to preserve the logic of working off
>> of the currently set (and possibly locked) contents of ESMRAMC.TSEG_SZ.
>>
>> I guess the idea is still doable if SmramInternal.c introduces a global
>> variable for caching the PCD at module startup -- setting the variable
>> from the PCD would be the responsibility of each separate module, both
>> PEIM and DXE driver --, and then SmramAccessGetCapabilities() could use
>> the global variable if it read "11" binary from the TSEG_SZ bit-field
>> register.
> 
> Why can't SmramInternal.c read the size from the PCD directly?

If I remember correctly, I wanted to base SmramAccessGetCapabilities()
directly on hardware configuration. At that point (regardless of whether
the function would be called via PEI_SMM_ACCESS_PPI or
EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for
the calculation would already be locked, by the SmmAccessPei entry point
function.

I don't have a clear threat model in my mind, but I feel unsafe basing
SmramAccessGetCapabilities() on a dynamic PcdGet() call. Dynamic PCDs
can be changed well into BDS (for example by a UEFI app or driver
launched / loaded from the UEFI shell prompt), without breaking any
rules. I don't think EFI_SMM_ACCESS2_PROTOCOL.GetCapabilities() should
return a different output based on that. I just feel unsafe about adding
a functional change like this to the series.

Most drivers that I can recall from under MdeModulePkg that consume
dynamic PCDs tend to save the values to global variables in their entry
points.

Thanks
Laszlo

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Jordan Justen]

On 2017-06-26 16:04:41, Laszlo Ersek wrote:
> On 06/27/17 00:36, Jordan Justen wrote:
> > 
> > Why can't SmramInternal.c read the size from the PCD directly?
> 
> If I remember correctly, I wanted to base SmramAccessGetCapabilities()
> directly on hardware configuration. At that point (regardless of whether
> the function would be called via PEI_SMM_ACCESS_PPI or
> EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for
> the calculation would already be locked, by the SmmAccessPei entry point
> function.

I guess it does look slightly more appropriate to read to reg, even if
the end result will always match the PCD. The 'internal' file is used
by PEI as well, so if you want to add a function or variable in the
'internal' give the mask, it sounds good.

-Jordan

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Laszlo Ersek]

On 06/29/17 21:14, Jordan Justen wrote:
> On 2017-06-26 16:04:41, Laszlo Ersek wrote:
>> On 06/27/17 00:36, Jordan Justen wrote:
>>>
>>> Why can't SmramInternal.c read the size from the PCD directly?
>>
>> If I remember correctly, I wanted to base SmramAccessGetCapabilities()
>> directly on hardware configuration. At that point (regardless of whether
>> the function would be called via PEI_SMM_ACCESS_PPI or
>> EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for
>> the calculation would already be locked, by the SmmAccessPei entry point
>> function.
> 
> I guess it does look slightly more appropriate to read to reg, even if
> the end result will always match the PCD. The 'internal' file is used
> by PEI as well, so if you want to add a function or variable in the
> 'internal' give the mask, it sounds good.

I would like to introduce a global variable in SmramInternal.c, to be
set by both the PEIM's and the DXE driver's entry point functions to the
dynamic PCD value (i.e., at module startup).

- This global variable would be consulted in
SmramAccessGetCapabilities() (in SmramInternal.c, called from both the
corresponding PPI and protocol member functions) if, and only if, the
value read from ESMRAMC.TSEG_SZ were 11 binary.

- If the bit pattern read from this bit-field register were different,
then constant values would be used (exactly like now).

- And the dynamic PCD would never be consulted in
SmramAccessGetCapabilities().

OK?

Thanks,
Laszlo

Re: [PATCH 1/5] OvmfPkg: introduce Q35TsegSizeLib (class header and sole lib instance)

[Jordan Justen]

On 2017-07-01 13:42:06, Laszlo Ersek wrote:
> On 06/29/17 21:14, Jordan Justen wrote:
> > On 2017-06-26 16:04:41, Laszlo Ersek wrote:
> >> On 06/27/17 00:36, Jordan Justen wrote:
> >>>
> >>> Why can't SmramInternal.c read the size from the PCD directly?
> >>
> >> If I remember correctly, I wanted to base SmramAccessGetCapabilities()
> >> directly on hardware configuration. At that point (regardless of whether
> >> the function would be called via PEI_SMM_ACCESS_PPI or
> >> EFI_SMM_ACCESS2_PROTOCOL), the ESMRAMC.TSEG_SZ bits used as source for
> >> the calculation would already be locked, by the SmmAccessPei entry point
> >> function.
> > 
> > I guess it does look slightly more appropriate to read to reg, even if
> > the end result will always match the PCD. The 'internal' file is used
> > by PEI as well, so if you want to add a function or variable in the
> > 'internal' give the mask, it sounds good.
> 
> I would like to introduce a global variable in SmramInternal.c, to be
> set by both the PEIM's and the DXE driver's entry point functions to the
> dynamic PCD value (i.e., at module startup).

Sounds good. Maybe they can both call a function in SmramInternal.c to
init it. Or, have a function in SmramInternal.c return the mask.

-Jordan

> - This global variable would be consulted in
> SmramAccessGetCapabilities() (in SmramInternal.c, called from both the
> corresponding PPI and protocol member functions) if, and only if, the
> value read from ESMRAMC.TSEG_SZ were 11 binary.
> 
> - If the bit pattern read from this bit-field register were different,
> then constant values would be used (exactly like now).
> 
> - And the dynamic PCD would never be consulted in
> SmramAccessGetCapabilities().
> 
> OK?
> 
> Thanks,
> Laszlo