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

["Yao, Jiewen" <jiewen.yao@intel.com>]

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