[edk2-devel] [PATCH v4 1/7] MdePkg: MmUnblockMemoryLib: Added definition and null instance
Kun Qin
kun.q at outlook.com
Wed Mar 3 19:31:32 UTC 2021
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:lersek at redhat.com>
Sent: Wednesday, March 3, 2021 07:34
To: Kun Qin<mailto:kun.q at outlook.com>; devel at edk2.groups.io<mailto:devel at edk2.groups.io>
Cc: Michael D Kinney<mailto:michael.d.kinney at intel.com>; Liming Gao<mailto:gaoliming at byosoft.com.cn>; Zhiguang Liu<mailto:zhiguang.liu at intel.com>; Hao A Wu<mailto:hao.a.wu at intel.com>; Jiewen Yao<mailto:jiewen.yao at 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.kinney at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu at intel.com>
> Cc: Hao A Wu <hao.a.wu at intel.com>
> Cc: Jiewen Yao <jiewen.yao at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
>
> Signed-off-by: Kun Qin <kun.q at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20210303/cf8cdaac/attachment.htm>
More information about the edk2-devel-archive
mailing list