Re: [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.

[Laszlo Ersek <lersek@redhat.com>]

Hi Marvin,

On 07/24/18 03:40, Marvin Häuser wrote:
> Update all references to the SMM PPIs from MdeModulePkg to rather use
> MdePkg's MM PPI declarations.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Marvin Haeuser <Marvin.Haeuser@outlook.com>
> ---
>  OvmfPkg/SmmAccess/SmmAccessPei.c   | 90 ++++++++++----------
>  OvmfPkg/SmmAccess/SmramInternal.c  |  8 +-
>  OvmfPkg/SmmAccess/SmmAccessPei.inf |  2 +-
>  3 files changed, 50 insertions(+), 50 deletions(-)

two comments:

(1) Please post patch sets using a cover letter message:

git config format.coverletter     true
git config format.numbered        true
git config sendemail.chainreplyto false
git config sendemail.thread       true

Otherwise the individual patches in the series will get intermixed with
other messages / threads on the mailing list (and in people's inboxes),
if the recipient uses a threaded view in their MUA (which most people
should do -- patches and discussions are very hard to follow otherwise).

(2) More specifically, I disagree with this patch. I don't see what's
wrong with calling SMM "SMM" on x86 (or with calling SMI "SMI"). I
understand that the PI spec now calls those things MM and MMI, and I
tend to agree that new code (especially ARM/AARCH64 code) should use
these more generic terms from the most recent PI spec releases. But,
this is not new code, and it is for x86; I don't see why we should
forcefully drag it forward. I don't think the edk2 headers plan to
remove the original / traditional names.

(Just the other day, I learned that EFI_FILE_HANDLE was basically a
typedef for "pointer to EFI_FILE_PROTOCOL" -- it's a non-standard type,
yet it's defined in "MdePkg/Include/Protocol/SimpleFileSystem.h", and
it's used in e.g.
- MdeModulePkg/Library/BootMaintenanceManagerUiLib
- MdeModulePkg/Library/FileExplorerLib
- MdeModulePkg/Universal/Disk/RamDiskDxe
- MdeModulePkg/Universal/EbcDxe
- MdePkg/Library/DxeServicesLib
- MdePkg/Library/UefiFileHandleLib
- NetworkPkg/TlsAuthConfigDxe
- SecurityPkg/VariableAuthenticated/SecureBootConfigDxe

Should we use EFI_FILE_HANDLE in new code we write? No, we shouldn't.
Should we embark on a journey to eradicate EFI_FILE_HANDLE? I would
disagree with that idea too.)

Obviously I'm not arguing against migrating other packages to the new
names, if their respective maintainers like the idea. I don't think it's
useful for OvmfPkg though.

Thanks
Laszlo

> diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.c b/OvmfPkg/SmmAccess/SmmAccessPei.c
> index 21119f80eefa..340122d6a598 100644
> --- a/OvmfPkg/SmmAccess/SmmAccessPei.c
> +++ b/OvmfPkg/SmmAccess/SmmAccessPei.c
> @@ -3,7 +3,7 @@
>    A PEIM with the following responsibilities:
>  
>    - verify & configure the Q35 TSEG in the entry point,
> -  - provide SMRAM access by producing PEI_SMM_ACCESS_PPI,
> +  - provide MMRAM access by producing EFI_PEI_MM_ACCESS_PPI,
>    - set aside the SMM_S3_RESUME_STATE object at the bottom of TSEG, and expose
>      it via the gEfiAcpiVariableGuid GUID HOB.
>  
> @@ -32,28 +32,28 @@
>  #include <Library/PcdLib.h>
>  #include <Library/PciLib.h>
>  #include <Library/PeiServicesLib.h>
> -#include <Ppi/SmmAccess.h>
> +#include <Ppi/MmAccess.h>
>  
>  #include <OvmfPlatforms.h>
>  
>  #include "SmramInternal.h"
>  
>  //
> -// PEI_SMM_ACCESS_PPI implementation.
> +// EFI_PEI_MM_ACCESS_PPI implementation.
>  //
>  
>  /**
> -  Opens the SMRAM area to be accessible by a PEIM driver.
> +  Opens the MMRAM area to be accessible by a PEIM driver.
>  
> -  This function "opens" SMRAM so that it is visible while not inside of SMM.
> +  This function "opens" MMRAM so that it is visible while not inside of MM.
>    The function should return EFI_UNSUPPORTED if the hardware does not support
> -  hiding of SMRAM. The function should return EFI_DEVICE_ERROR if the SMRAM
> +  hiding of MMRAM. The function should return EFI_DEVICE_ERROR if the MMRAM
>    configuration is locked.
>  
>    @param  PeiServices            General purpose services available to every
>                                   PEIM.
> -  @param  This                   The pointer to the SMM Access Interface.
> -  @param  DescriptorIndex        The region of SMRAM to Open.
> +  @param  This                   The pointer to the MM Access Interface.
> +  @param  DescriptorIndex        The region of MMRAM to Open.
>  
>    @retval EFI_SUCCESS            The region was successfully opened.
>    @retval EFI_DEVICE_ERROR       The region could not be opened because locked
> @@ -64,9 +64,9 @@
>  STATIC
>  EFI_STATUS
>  EFIAPI
> -SmmAccessPeiOpen (
> +MmAccessPeiOpen (
>    IN EFI_PEI_SERVICES                **PeiServices,
> -  IN PEI_SMM_ACCESS_PPI              *This,
> +  IN EFI_PEI_MM_ACCESS_PPI           *This,
>    IN UINTN                           DescriptorIndex
>    )
>  {
> @@ -82,16 +82,16 @@ SmmAccessPeiOpen (
>  }
>  
>  /**
> -  Inhibits access to the SMRAM.
> +  Inhibits access to the MMRAM.
>  
> -  This function "closes" SMRAM so that it is not visible while outside of SMM.
> +  This function "closes" MMRAM so that it is not visible while outside of MM.
>    The function should return EFI_UNSUPPORTED if the hardware does not support
> -  hiding of SMRAM.
> +  hiding of MMRAM.
>  
>    @param  PeiServices              General purpose services available to every
>                                     PEIM.
> -  @param  This                     The pointer to the SMM Access Interface.
> -  @param  DescriptorIndex          The region of SMRAM to Close.
> +  @param  This                     The pointer to the MM Access Interface.
> +  @param  DescriptorIndex          The region of MMRAM to Close.
>  
>    @retval EFI_SUCCESS              The region was successfully closed.
>    @retval EFI_DEVICE_ERROR         The region could not be closed because
> @@ -102,9 +102,9 @@ SmmAccessPeiOpen (
>  STATIC
>  EFI_STATUS
>  EFIAPI
> -SmmAccessPeiClose (
> +MmAccessPeiClose (
>    IN EFI_PEI_SERVICES                **PeiServices,
> -  IN PEI_SMM_ACCESS_PPI              *This,
> +  IN EFI_PEI_MM_ACCESS_PPI           *This,
>    IN UINTN                           DescriptorIndex
>    )
>  {
> @@ -120,15 +120,15 @@ SmmAccessPeiClose (
>  }
>  
>  /**
> -  Inhibits access to the SMRAM.
> +  Inhibits access to the MMRAM.
>  
> -  This function prohibits access to the SMRAM region.  This function is usually
> +  This function prohibits access to the MMRAM region.  This function is usually
>    implemented such that it is a write-once operation.
>  
>    @param  PeiServices              General purpose services available to every
>                                     PEIM.
> -  @param  This                     The pointer to the SMM Access Interface.
> -  @param  DescriptorIndex          The region of SMRAM to Close.
> +  @param  This                     The pointer to the MM Access Interface.
> +  @param  DescriptorIndex          The region of MMRAM to Close.
>  
>    @retval EFI_SUCCESS            The region was successfully locked.
>    @retval EFI_DEVICE_ERROR       The region could not be locked because at
> @@ -139,9 +139,9 @@ SmmAccessPeiClose (
>  STATIC
>  EFI_STATUS
>  EFIAPI
> -SmmAccessPeiLock (
> +MmAccessPeiLock (
>    IN EFI_PEI_SERVICES                **PeiServices,
> -  IN PEI_SMM_ACCESS_PPI              *This,
> +  IN EFI_PEI_MM_ACCESS_PPI           *This,
>    IN UINTN                           DescriptorIndex
>    )
>  {
> @@ -158,11 +158,11 @@ SmmAccessPeiLock (
>  
>  /**
>    Queries the memory controller for the possible regions that will support
> -  SMRAM.
> +  MMRAM.
>  
>    @param  PeiServices           General purpose services available to every
>                                  PEIM.
> -  @param This                   The pointer to the SmmAccessPpi Interface.
> +  @param This                   The pointer to the MmAccessPpi Interface.
>    @param SmramMapSize           The pointer to the variable containing size of
>                                  the buffer to contain the description
>                                  information.
> @@ -176,11 +176,11 @@ SmmAccessPeiLock (
>  STATIC
>  EFI_STATUS
>  EFIAPI
> -SmmAccessPeiGetCapabilities (
> +MmAccessPeiGetCapabilities (
>    IN EFI_PEI_SERVICES                **PeiServices,
> -  IN PEI_SMM_ACCESS_PPI              *This,
> +  IN EFI_PEI_MM_ACCESS_PPI           *This,
>    IN OUT UINTN                       *SmramMapSize,
> -  IN OUT EFI_SMRAM_DESCRIPTOR        *SmramMap
> +  IN OUT EFI_MMRAM_DESCRIPTOR        *SmramMap
>    )
>  {
>    return SmramAccessGetCapabilities (This->LockState, This->OpenState,
> @@ -190,18 +190,18 @@ SmmAccessPeiGetCapabilities (
>  //
>  // LockState and OpenState will be filled in by the entry point.
>  //
> -STATIC PEI_SMM_ACCESS_PPI mAccess = {
> -  &SmmAccessPeiOpen,
> -  &SmmAccessPeiClose,
> -  &SmmAccessPeiLock,
> -  &SmmAccessPeiGetCapabilities
> +STATIC EFI_PEI_MM_ACCESS_PPI mAccess = {
> +  &MmAccessPeiOpen,
> +  &MmAccessPeiClose,
> +  &MmAccessPeiLock,
> +  &MmAccessPeiGetCapabilities
>  };
>  
>  
>  STATIC EFI_PEI_PPI_DESCRIPTOR mPpiList[] = {
>    {
>      EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST,
> -    &gPeiSmmAccessPpiGuid, &mAccess
> +    &gEfiPeiMmAccessPpiGuid, &mAccess
>    }
>  };
>  
> @@ -255,7 +255,7 @@ SmmAccessPeiEntryPoint (
>    VOID                 *GuidHob;
>  
>    //
> -  // This module should only be included if SMRAM support is required.
> +  // This module should only be included if MMRAM support is required.
>    //
>    ASSERT (FeaturePcdGet (PcdSmmSmramRequire));
>  
> @@ -264,14 +264,14 @@ SmmAccessPeiEntryPoint (
>    //
>    HostBridgeDevId = PciRead16 (OVMF_HOSTBRIDGE_DID);
>    if (HostBridgeDevId != INTEL_Q35_MCH_DEVICE_ID) {
> -    DEBUG ((EFI_D_ERROR, "%a: no SMRAM with host bridge DID=0x%04x; only "
> +    DEBUG ((EFI_D_ERROR, "%a: no MMRAM with host bridge DID=0x%04x; only "
>        "DID=0x%04x (Q35) is supported\n", __FUNCTION__, HostBridgeDevId,
>        INTEL_Q35_MCH_DEVICE_ID));
>      goto WrongConfig;
>    }
>  
>    //
> -  // Confirm if QEMU supports SMRAM.
> +  // Confirm if QEMU supports MMRAM.
>    //
>    // With no support for it, the ESMRAMC (Extended System Management RAM
>    // Control) register reads as zero. If there is support, the cache-enable
> @@ -280,7 +280,7 @@ SmmAccessPeiEntryPoint (
>    EsmramcVal = PciRead8 (DRAMC_REGISTER_Q35 (MCH_ESMRAMC));
>    RegMask8 = MCH_ESMRAMC_SM_CACHE | MCH_ESMRAMC_SM_L1 | MCH_ESMRAMC_SM_L2;
>    if ((EsmramcVal & RegMask8) != RegMask8) {
> -    DEBUG ((EFI_D_ERROR, "%a: this Q35 implementation lacks SMRAM\n",
> +    DEBUG ((EFI_D_ERROR, "%a: this Q35 implementation lacks MMRAM\n",
>        __FUNCTION__));
>      goto WrongConfig;
>    }
> @@ -323,27 +323,27 @@ SmmAccessPeiEntryPoint (
>      (TopOfLowRamMb - mQ35TsegMbytes) << MCH_TSEGMB_MB_SHIFT);
>  
>    //
> -  // Set TSEG size, and disable TSEG visibility outside of SMM. Note that the
> +  // Set TSEG size, and disable TSEG visibility outside of MM. Note that the
>    // T_EN bit has inverse meaning; when T_EN is set, then TSEG visibility is
> -  // *restricted* to SMM.
> +  // *restricted* to MM.
>    //
>    EsmramcVal &= ~(UINT32)MCH_ESMRAMC_TSEG_MASK;
>    EsmramcVal |= mQ35TsegMbytes == 8 ? MCH_ESMRAMC_TSEG_8MB :
>                  mQ35TsegMbytes == 2 ? MCH_ESMRAMC_TSEG_2MB :
>                  mQ35TsegMbytes == 1 ? MCH_ESMRAMC_TSEG_1MB :
> -                MCH_ESMRAMC_TSEG_EXT;
> -  EsmramcVal |= MCH_ESMRAMC_T_EN;
> +                MCH_EMMRAMC_TSEG_EXT;
> +  EsmramcVal |= MCH_EMMRAMC_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.
> +  // (Global MMRAM Enable) too, as both D_LCK and T_EN depend on it.
>    //
>    PciAndThenOr8 (DRAMC_REGISTER_Q35 (MCH_SMRAM),
>      (UINT8)((~(UINT32)MCH_SMRAM_D_LCK) & 0xff), MCH_SMRAM_G_SMRAME);
>  
>    //
> -  // Create the GUID HOB and point it to the first SMRAM range.
> +  // Create the GUID HOB and point it to the first MMRAM range.
>    //
>    GetStates (&mAccess.LockState, &mAccess.OpenState);
>    SmramMapSize = sizeof SmramMap;
> @@ -357,7 +357,7 @@ SmmAccessPeiEntryPoint (
>      UINTN Idx;
>  
>      Count = SmramMapSize / sizeof SmramMap[0];
> -    DEBUG ((EFI_D_VERBOSE, "%a: SMRAM map follows, %d entries\n", __FUNCTION__,
> +    DEBUG ((EFI_D_VERBOSE, "%a: MMRAM map follows, %d entries\n", __FUNCTION__,
>        (INT32)Count));
>      DEBUG ((EFI_D_VERBOSE, "% 20a % 20a % 20a % 20a\n", "PhysicalStart(0x)",
>        "PhysicalSize(0x)", "CpuStart(0x)", "RegionState(0x)"));
> diff --git a/OvmfPkg/SmmAccess/SmramInternal.c b/OvmfPkg/SmmAccess/SmramInternal.c
> index 18c42d29042d..3b33d2763b4d 100644
> --- a/OvmfPkg/SmmAccess/SmramInternal.c
> +++ b/OvmfPkg/SmmAccess/SmramInternal.c
> @@ -40,10 +40,10 @@ InitQ35TsegMbytes (
>  
>  /**
>    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,
> +  OpenState fields in the EFI_PEI_MM_ACCESS_PPI / EFI_SMM_ACCESS2_PROTOCOL object,
>    from the D_LCK and T_EN bits.
>  
> -  PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL member functions can rely on
> +  EFI_PEI_MM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL member functions can rely on
>    the LockState and OpenState fields being up-to-date on entry, and they need
>    to restore the same invariant on exit, if they touch the bits in question.
>  
> @@ -68,13 +68,13 @@ GetStates (
>  }
>  
>  //
> -// The functions below follow the PEI_SMM_ACCESS_PPI and
> +// The functions below follow the EFI_PEI_MM_ACCESS_PPI and
>  // EFI_SMM_ACCESS2_PROTOCOL member declarations. The PeiServices and This
>  // pointers are removed (TSEG doesn't depend on them), and so is the
>  // DescriptorIndex parameter (TSEG doesn't support range-wise locking).
>  //
>  // The LockState and OpenState members that are common to both
> -// PEI_SMM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL are taken and updated in
> +// EFI_PEI_MM_ACCESS_PPI and EFI_SMM_ACCESS2_PROTOCOL are taken and updated in
>  // isolation from the rest of the (non-shared) members.
>  //
>  
> diff --git a/OvmfPkg/SmmAccess/SmmAccessPei.inf b/OvmfPkg/SmmAccess/SmmAccessPei.inf
> index 09f3b63446df..7360b2f5391e 100644
> --- a/OvmfPkg/SmmAccess/SmmAccessPei.inf
> +++ b/OvmfPkg/SmmAccess/SmmAccessPei.inf
> @@ -63,7 +63,7 @@ [Pcd]
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes
>  
>  [Ppis]
> -  gPeiSmmAccessPpiGuid           ## PRODUCES
> +  gEfiPeiMmAccessPpiGuid         ## PRODUCES
>  
>  [Depex]
>    gEfiPeiMemoryDiscoveredPpiGuid
>