Thanks for finding these. I missed the return data type completely and agree that @retval should be used. Will update the patch in v5.
Regards, Kun From: Laszlo Ersek<mailto:ler...@redhat.com> Sent: Wednesday, March 3, 2021 07:34 To: Kun Qin<mailto:ku...@outlook.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Michael D Kinney<mailto:michael.d.kin...@intel.com>; Liming Gao<mailto:gaolim...@byosoft.com.cn>; Zhiguang Liu<mailto:zhiguang....@intel.com>; Hao A Wu<mailto:hao.a...@intel.com>; Jiewen Yao<mailto:jiewen....@intel.com> Subject: Re: [PATCH v4 1/7] MdePkg: MmUnblockMemoryLib: Added definition and null instance On 03/02/21 21:04, Kun Qin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3168 > > This interface provides an abstration layer to allow MM modules to access > requested areas that are outside of MMRAM. On MM model that blocks all > non-MMRAM accesses, areas requested through this API will be mapped or > unblocked for accessibility inside MM environment. > > For MM modules that need to access regions outside of MMRAMs, the agents > that set up these regions are responsible for invoking this API in order > for these memory areas to be accessible from inside MM. > > Example usages: > 1. To enable runtime cache feature for variable service, Variable MM > module will need to access the allocated runtime buffer. Thus the agent > sets up these buffers, VariableSmmRuntimeDxe, will need to invoke this > API to make these regions accessible by Variable MM. > 2. For TPM ACPI table to communicate to physical presence handler, the > corresponding NVS region has to be accessible from inside MM. Once the > NVS region are assigned, it needs to be unblocked thourgh this API. > > Cc: Michael D Kinney <michael.d.kin...@intel.com> > Cc: Liming Gao <gaolim...@byosoft.com.cn> > Cc: Zhiguang Liu <zhiguang....@intel.com> > Cc: Hao A Wu <hao.a...@intel.com> > Cc: Jiewen Yao <jiewen....@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > > Signed-off-by: Kun Qin <ku...@outlook.com> > --- > > Notes: > v4: > - Added more commit message [Laszlo] The commit message looks great, thanks for that. However, I notice the only "RETURN_xxx" macro in the patch is the "RETURN_UNSUPPORTED" return code, in the Null library instance. My point was that the *interface* should be downgraded to RETURN_xxx (that is, the lib class header -- the EFI_STATUS return type should be RETURN_STATUS; the documented @retval codes should all be RETURN_xxx, and so on). An example here is the PcdLib class. It uses the RETURN_STATUS return type, and documents the RETURN_INVALID_PARAMETER retval in some spots. And then edk2 provides several lib instances: - MdePkg/Library/BasePcdLibNull/BasePcdLibNull.inf - MdePkg/Library/PeiPcdLib/PeiPcdLib.inf - MdePkg/Library/DxePcdLib/DxePcdLib.inf The first of these is "BASE" (with no particular module type restriction), the 2nd is "PEIM" (with PEIM PEI_CORE SEC restrictions), and the 3rd is "DXE_DRIVER" (restricted to all the remaining module types). Every instance uses RETURN_xxx just fine. ... In case I'm wrong, please correct me, of course. Yet another comment: I'm just noticing that "@return" is used with actual constants. That's wrong, "@retval" needs to be used for constants; "@return" is only suitable if you provide a natural language description without a constant. Example: @retval TRUE It is currently raining. @retval FALSE It is currently not raining. vs. @return Whether it's currently raining or not. Thanks! Laszlo > - Added UNI file [Hao] > > v3: > - Move interface to MdePkg [Hao, Liming, Jiewen] > - Remove Dxe prefix [Jiewen] > > v2: > - Resend with practical usage. No change [Hao] > > MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.c | 44 > ++++++++++++++++++++ > MdePkg/Include/Library/MmUnblockMemoryLib.h | 44 > ++++++++++++++++++++ > MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.inf | 34 > +++++++++++++++ > MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.uni | 21 ++++++++++ > MdePkg/MdePkg.dec | 5 +++ > MdePkg/MdePkg.dsc | 1 + > 6 files changed, 149 insertions(+) > > diff --git a/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.c > b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.c > new file mode 100644 > index 000000000000..abdce41f10d1 > --- /dev/null > +++ b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.c > @@ -0,0 +1,44 @@ > +/** @file > + Null instance of MM Unblock Page Library. > + > + This library provides an interface to request non-MMRAM pages to be > mapped/unblocked > + from inside MM environment. > + > + For MM modules that need to access regions outside of MMRAMs, the agents > that set up > + these regions are responsible for invoking this API in order for these > memory areas > + to be accessed from inside MM. > + > + Copyright (c) Microsoft Corporation. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include <Uefi.h> > + > +/** > + This API provides a way to unblock certain data pages to be accessible > inside MM environment. > + > + @param UnblockAddress The address of buffer caller requests to > unblock, the address > + has to be page aligned. > + @param NumberOfPages The number of pages requested to be > unblocked from MM > + environment. > + > + @return EFI_SUCCESS The request goes through successfully. > + @return EFI_NOT_AVAILABLE_YET The requested functionality is not > produced yet. > + @return EFI_UNSUPPORTED The requested functionality is not > supported on current platform. > + @return EFI_SECURITY_VIOLATION The requested address failed to pass > security check for > + unblocking. > + @return EFI_INVALID_PARAMETER Input address either NULL pointer or not > page aligned. > + @return EFI_ACCESS_DENIED The request is rejected due to system has > passed certain boot > + phase. > + > +**/ > +EFI_STATUS > +EFIAPI > +MmUnblockMemoryRequest ( > + IN EFI_PHYSICAL_ADDRESS UnblockAddress, > + IN UINT64 NumberOfPages > + ) > +{ > + return RETURN_UNSUPPORTED; > +} > diff --git a/MdePkg/Include/Library/MmUnblockMemoryLib.h > b/MdePkg/Include/Library/MmUnblockMemoryLib.h > new file mode 100644 > index 000000000000..980afe9a5fd3 > --- /dev/null > +++ b/MdePkg/Include/Library/MmUnblockMemoryLib.h > @@ -0,0 +1,44 @@ > +/** @file > + MM Unblock Memory Library Interface. > + > + This library provides an interface to request non-MMRAM pages to be > mapped/unblocked > + from inside MM environment. > + > + For MM modules that need to access regions outside of MMRAMs, the agents > that set up > + these regions are responsible for invoking this API in order for these > memory areas > + to be accessed from inside MM. > + > + Copyright (c) Microsoft Corporation. > + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#ifndef MM_UNBLOCK_MEMORY_LIB_H_ > +#define MM_UNBLOCK_MEMORY_LIB_H_ > + > +/** > + This API provides a way to unblock certain data pages to be accessible > inside MM environment. > + > + @param UnblockAddress The address of buffer caller requests to > unblock, the address > + has to be page aligned. > + @param NumberOfPages The number of pages requested to be > unblocked from MM > + environment. > + > + @return EFI_SUCCESS The request goes through successfully. > + @return EFI_NOT_AVAILABLE_YET The requested functionality is not > produced yet. > + @return EFI_UNSUPPORTED The requested functionality is not > supported on current platform. > + @return EFI_SECURITY_VIOLATION The requested address failed to pass > security check for > + unblocking. > + @return EFI_INVALID_PARAMETER Input address either NULL pointer or not > page aligned. > + @return EFI_ACCESS_DENIED The request is rejected due to system has > passed certain boot > + phase. > + > +**/ > +EFI_STATUS > +EFIAPI > +MmUnblockMemoryRequest ( > + IN EFI_PHYSICAL_ADDRESS UnblockAddress, > + IN UINT64 NumberOfPages > +); > + > +#endif // MM_UNBLOCK_MEMORY_LIB_H_ > diff --git a/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.inf > b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.inf > new file mode 100644 > index 000000000000..8ecb767ff7bd > --- /dev/null > +++ b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.inf > @@ -0,0 +1,34 @@ > +## @file > +# Null instance of MM Unblock Page Library. > +# > +# This library provides an interface to request non-MMRAM pages to be > mapped/unblocked > +# from inside MM environment. > +# > +# For MM modules that need to access regions outside of MMRAMs, the agents > that set up > +# these regions are responsible for invoking this API in order for these > memory areas > +# to be accessed from inside MM. > +# > +# Copyright (c) Microsoft Corporation. > +# SPDX-License-Identifier: BSD-2-Clause-Patent > +# > +# > +## > + > +[Defines] > + INF_VERSION = 0x0001001B > + BASE_NAME = MmUnblockMemoryLibNull > + MODULE_UNI_FILE = MmUnblockMemoryLibNull.uni > + FILE_GUID = 9E890F68-5C95-4C31-95DD-59E6286F85EA > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = MmUnblockMemoryLib > + > +# > +# VALID_ARCHITECTURES = IA32 X64 > +# > + > +[Sources] > + MmUnblockMemoryLibNull.c > + > +[Packages] > + MdePkg/MdePkg.dec > diff --git a/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.uni > b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.uni > new file mode 100644 > index 000000000000..d7f2709a3dce > --- /dev/null > +++ b/MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.uni > @@ -0,0 +1,21 @@ > +// /** @file > +// Null instance of MM Unblock Page Library. > +// > +// This library provides an interface to request non-MMRAM pages to be > mapped/unblocked > +// from inside MM environment. > +// > +// For MM modules that need to access regions outside of MMRAMs, the agents > that set up > +// these regions are responsible for invoking this API in order for these > memory areas > +// to be accessed from inside MM. > +// > +// Copyright (c) Microsoft Corporation. > +// SPDX-License-Identifier: BSD-2-Clause-Patent > +// > +// **/ > + > + > +#string STR_MODULE_ABSTRACT #language en-US "Null instance of MM > Unblock Page Library." > + > +#string STR_MODULE_DESCRIPTION #language en-US "This library > provides an interface to request non-MMRAM pages to be mapped/unblocked from > inside MM environment.\n" > + "For MM modules that > need to access regions outside of MMRAMs, the agents that set up these > regions are responsible for invoking this API in order for these memory areas > to be accessed from inside MM." > + > diff --git a/MdePkg/MdePkg.dec b/MdePkg/MdePkg.dec > index 3928db65d188..32a9e009c813 100644 > --- a/MdePkg/MdePkg.dec > +++ b/MdePkg/MdePkg.dec > @@ -257,6 +257,11 @@ [LibraryClasses] > # > UnitTestHostBaseLib|Test/UnitTest/Include/Library/UnitTestHostBaseLib.h > > + ## @libraryclass This library provides an interface for DXE drivers to > request MM environment > + # to map/unblock a memory region for accessibility inside MM. > + # > + MmUnblockMemoryLib|Include/Library/MmUnblockMemoryLib.h > + > [LibraryClasses.IA32, LibraryClasses.X64] > ## @libraryclass Abstracts both S/W SMI generation and detection. > ## > diff --git a/MdePkg/MdePkg.dsc b/MdePkg/MdePkg.dsc > index ce009086815f..79629e3f93ba 100644 > --- a/MdePkg/MdePkg.dsc > +++ b/MdePkg/MdePkg.dsc > @@ -168,6 +168,7 @@ [Components.IA32, Components.X64] > MdePkg/Library/SmmPciExpressLib/SmmPciExpressLib.inf > MdePkg/Library/SmiHandlerProfileLibNull/SmiHandlerProfileLibNull.inf > MdePkg/Library/MmServicesTableLib/MmServicesTableLib.inf > + MdePkg/Library/MmUnblockMemoryLib/MmUnblockMemoryLibNull.inf > > [Components.EBC] > MdePkg/Library/BaseIoLibIntrinsic/BaseIoLibIntrinsic.inf > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72403): https://edk2.groups.io/g/devel/message/72403 Mute This Topic: https://groups.io/mt/81035309/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
