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

["Marvin Häuser" <Marvin.Haeuser@outlook.com>]

Hey Laszlo,

> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, July 24, 2018 11:20 AM
> To: Marvin Häuser <Marvin.Haeuser@outlook.com>; edk2-
> devel@lists.01.org
> Cc: michael.d.kinney@intel.com; liming.gao@intel.com;
> star.zeng@intel.com; eric.dong@intel.com; ruiyu.ni@intel.com;
> kelly.steele@intel.com; jordan.l.justen@intel.com;
> ard.biesheuvel@linaro.org
> Subject: Re: [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages.
> 
> 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).

Is V2 fine in this aspect? Outlook has never threaded multiple parts of a patch series for me but only replies to their original message, hence I did not notice something was off.

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