From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id ECF0B21B02845 for ; Tue, 24 Jul 2018 02:20:05 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 65F7481A4EA9; Tue, 24 Jul 2018 09:20:04 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-190.rdu2.redhat.com [10.10.120.190]) by smtp.corp.redhat.com (Postfix) with ESMTP id 803C0111AF29; Tue, 24 Jul 2018 09:20:02 +0000 (UTC) To: =?UTF-8?Q?Marvin_H=c3=a4user?= , "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" References: From: Laszlo Ersek Message-ID: <4aaed815-a16d-a6b9-ddd5-f941ea48b22c@redhat.com> Date: Tue, 24 Jul 2018 11:20:01 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.3 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 24 Jul 2018 09:20:04 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.8]); Tue, 24 Jul 2018 09:20:04 +0000 (UTC) for IP:'10.11.54.3' DOMAIN:'int-mx03.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 7/8] OvmfPkg/SmmAccessPei: Update MM PPI usages. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.27 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 24 Jul 2018 09:20:06 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit 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 > --- > 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 > #include > #include > -#include > +#include > > #include > > #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 >