From: "Zeng, Star" <star.zeng@intel.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>, edk2-devel@lists.01.org
Cc: Hao Wu <hao.a.wu@intel.com>, Liming Gao <liming.gao@intel.com>,
Michael D Kinney <michael.d.kinney@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
star.zeng@intel.com
Subject: Re: [PATCH 5/6] MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses
Date: Thu, 10 Jan 2019 15:19:56 +0800 [thread overview]
Message-ID: <7a32163a-e90c-95ef-f231-d9b796e8bd39@intel.com> (raw)
In-Reply-To: <20190103182825.32231-7-ard.biesheuvel@linaro.org>
Hi Ard,
Some minor feedback added inline.
On 2019/1/4 2:28, Ard Biesheuvel wrote:
> In preparation of providing a standalone MM based variable runtime
> driver, move the existing SMM driver to the new MM services table,
> and factor out some pieces that are specific to the traditional
> driver, mainly related to the use of UEFI boot services, which are
> not accessible to standalone MM drivers.
>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c | 18 +---
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 50 +++++++++
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 59 ++++------
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 5 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c | 114 ++++++++++++++++++++
> 5 files changed, 187 insertions(+), 59 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> index 28aa2893c6f8..009d96c3a65e 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/TcgMorLockSmm.c
> @@ -21,7 +21,6 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Library/DebugLib.h>
> #include <Library/BaseLib.h>
> #include <Library/BaseMemoryLib.h>
> -#include <Library/UefiBootServicesTableLib.h>
> #include "Variable.h"
>
> typedef struct {
> @@ -419,8 +418,6 @@ MorLockInitAtEndOfDxe (
> {
> UINTN MorSize;
> EFI_STATUS MorStatus;
> - EFI_STATUS TcgStatus;
> - VOID *TcgInterface;
>
> if (!mMorLockInitializationRequired) {
> //
> @@ -458,20 +455,7 @@ MorLockInitAtEndOfDxe (
> // can be deduced from the absence of the TCG / TCG2 protocols, as edk2's
> // MOR implementation depends on (one of) those protocols.
> //
> - TcgStatus = gBS->LocateProtocol (
> - &gEfiTcg2ProtocolGuid,
> - NULL, // Registration
> - &TcgInterface
> - );
> - if (EFI_ERROR (TcgStatus)) {
> - TcgStatus = gBS->LocateProtocol (
> - &gEfiTcgProtocolGuid,
> - NULL, // Registration
> - &TcgInterface
> - );
> - }
> -
> - if (!EFI_ERROR (TcgStatus)) {
> + if (VariableHaveTcgProtocols ()) {
> //
> // The MOR variable originates from the platform firmware; set the MOR
> // Control Lock variable to report the locking capability to the OS.
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> index 938eb5de61fa..11822575ac4d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> @@ -924,4 +924,54 @@ VariableExLibAtRuntime (
> VOID
> );
>
> +/**
> + Notify the system that the SMM variable driver is ready
> +**/
> +VOID
> +VariableNotifySmmReady (
> + VOID
> + );
> +
> +/**
> + Notify the system that the SMM variable write driver is ready
> +**/
> +VOID
> +VariableNotifySmmWriteReady (
> + VOID
> + );
> +
> +/**
> + Variable service MM driver entry point
> +**/
> +EFI_STATUS
> +EFIAPI
> +MmVariableServiceInitialize (
> + VOID
> + );
> +
> +/**
> + This function check if the buffer is valid per processor architecture and not overlap with SMRAM.
> +
> + @param Buffer The buffer start address to be checked.
> + @param Length The buffer length to be checked.
> +
> + @retval TRUE This buffer is valid per processor architecture and not overlap with SMRAM.
> + @retval FALSE This buffer is not valid per processor architecture or overlap with SMRAM.
> +**/
> +BOOLEAN
> +VariableSmmIsBufferOutsideSmmValid (
> + IN EFI_PHYSICAL_ADDRESS Buffer,
> + IN UINT64 Length
> + );
> +
> +/**
> + Whether the TCG or TCG2 protocols are installed in the UEFI protocol database.
> + This information is used by the MorLock code to infer whether an existing
> + MOR variable is legitimate or not.
Add a line for return description?
> +**/
> +BOOLEAN
> +VariableHaveTcgProtocols (
> + VOID
> + );
> +
> #endif
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 8c53f84ff6e8..7245587052df 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -15,6 +15,7 @@
> SmmVariableGetStatistics() should also do validation based on its own knowledge.
>
> Copyright (c) 2010 - 2016, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) 2018, Linaro, Ltd. All rights reserved.<BR>
> This program and the accompanying materials
> are licensed and made available under the terms and conditions of the BSD License
> which accompanies this distribution. The full text of the license may be found at
> @@ -28,18 +29,15 @@ WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> #include <Protocol/SmmVariable.h>
> #include <Protocol/SmmFirmwareVolumeBlock.h>
> #include <Protocol/SmmFaultTolerantWrite.h>
> -#include <Protocol/SmmEndOfDxe.h>
> +#include <Protocol/MmEndOfDxe.h>
> #include <Protocol/SmmVarCheck.h>
>
> -#include <Library/SmmServicesTableLib.h>
> -#include <Library/SmmMemLib.h>
> +#include <Library/MmServicesTableLib.h>
>
> #include <Guid/SmmVariableCommon.h>
> #include "Variable.h"
>
> extern VARIABLE_INFO_ENTRY *gVariableInfo;
> -EFI_HANDLE mSmmVariableHandle = NULL;
> -EFI_HANDLE mVariableHandle = NULL;
> BOOLEAN mAtRuntime = FALSE;
> UINT8 *mVariableBufferPayload = NULL;
> UINTN mVariableBufferPayloadSize;
> @@ -218,7 +216,7 @@ GetFtwProtocol (
> //
> // Locate Smm Fault Tolerent Write protocol
> //
> - Status = gSmst->SmmLocateProtocol (
> + Status = gMmst->MmLocateProtocol (
> &gEfiSmmFaultTolerantWriteProtocolGuid,
> NULL,
> FtwProtocol
> @@ -248,7 +246,7 @@ GetFvbByHandle (
> //
> // To get the SMM FVB protocol interface on the handle
> //
> - return gSmst->SmmHandleProtocol (
> + return gMmst->MmHandleProtocol (
> FvBlockHandle,
> &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> (VOID **) FvBlock
> @@ -287,7 +285,7 @@ GetFvbCountAndBuffer (
> BufferSize = 0;
> *NumberHandles = 0;
> *Buffer = NULL;
> - Status = gSmst->SmmLocateHandle (
> + Status = gMmst->MmLocateHandle (
> ByProtocol,
> &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> NULL,
> @@ -303,7 +301,7 @@ GetFvbCountAndBuffer (
> return EFI_OUT_OF_RESOURCES;
> }
>
> - Status = gSmst->SmmLocateHandle (
> + Status = gMmst->MmLocateHandle (
> ByProtocol,
> &gEfiSmmFirmwareVolumeBlockProtocolGuid,
> NULL,
> @@ -500,7 +498,7 @@ SmmVariableHandler (
> return EFI_SUCCESS;
> }
>
> - if (!SmmIsBufferOutsideSmmValid ((UINTN)CommBuffer, TempCommBufferSize)) {
> + if (!VariableSmmIsBufferOutsideSmmValid ((UINTN)CommBuffer, TempCommBufferSize)) {
> DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer in SMRAM or overflow!\n"));
> return EFI_SUCCESS;
> }
> @@ -911,13 +909,7 @@ SmmFtwNotificationEvent (
> //
> // Notify the variable wrapper driver the variable write service is ready
> //
> - Status = gBS->InstallProtocolInterface (
> - &mSmmVariableHandle,
> - &gSmmVariableWriteGuid,
> - EFI_NATIVE_INTERFACE,
> - NULL
> - );
> - ASSERT_EFI_ERROR (Status);
> + VariableNotifySmmWriteReady ();
>
> return EFI_SUCCESS;
> }
> @@ -928,18 +920,11 @@ SmmFtwNotificationEvent (
> runtime services in the EFI System Table and installs arch protocols
> for variable read and write services being available. It also registers
> a notification function for an EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE event.
> -
> - @param[in] ImageHandle The firmware allocated handle for the EFI image.
> - @param[in] SystemTable A pointer to the EFI System Table.
> -
> - @retval EFI_SUCCESS Variable service successfully initialized.
> -
> **/
> EFI_STATUS
> EFIAPI
> -VariableServiceInitialize (
> - IN EFI_HANDLE ImageHandle,
> - IN EFI_SYSTEM_TABLE *SystemTable
> +MmVariableServiceInitialize (
> + VOID
> )
> {
> EFI_STATUS Status;
> @@ -957,7 +942,7 @@ VariableServiceInitialize (
> // Install the Smm Variable Protocol on a new handle.
> //
> VariableHandle = NULL;
> - Status = gSmst->SmmInstallProtocolInterface (
> + Status = gMmst->MmInstallProtocolInterface (
> &VariableHandle,
> &gEfiSmmVariableProtocolGuid,
> EFI_NATIVE_INTERFACE,
> @@ -965,7 +950,7 @@ VariableServiceInitialize (
> );
> ASSERT_EFI_ERROR (Status);
>
> - Status = gSmst->SmmInstallProtocolInterface (
> + Status = gMmst->MmInstallProtocolInterface (
> &VariableHandle,
> &gEdkiiSmmVarCheckProtocolGuid,
> EFI_NATIVE_INTERFACE,
> @@ -976,7 +961,7 @@ VariableServiceInitialize (
> mVariableBufferPayloadSize = GetMaxVariableSize () +
> OFFSET_OF (SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY, Name) - GetVariableHeaderSize ();
>
> - Status = gSmst->SmmAllocatePool (
> + Status = gMmst->MmAllocatePool (
> EfiRuntimeServicesData,
> mVariableBufferPayloadSize,
> (VOID **)&mVariableBufferPayload
> @@ -987,25 +972,19 @@ VariableServiceInitialize (
> /// Register SMM variable SMI handler
> ///
> VariableHandle = NULL;
> - Status = gSmst->SmiHandlerRegister (SmmVariableHandler, &gEfiSmmVariableProtocolGuid, &VariableHandle);
> + Status = gMmst->MmiHandlerRegister (SmmVariableHandler, &gEfiSmmVariableProtocolGuid, &VariableHandle);
> ASSERT_EFI_ERROR (Status);
>
> //
> // Notify the variable wrapper driver the variable service is ready
> //
> - Status = SystemTable->BootServices->InstallProtocolInterface (
> - &mVariableHandle,
> - &gEfiSmmVariableProtocolGuid,
> - EFI_NATIVE_INTERFACE,
> - &gSmmVariable
> - );
> - ASSERT_EFI_ERROR (Status);
> + VariableNotifySmmReady ();
>
> //
> // Register EFI_SMM_END_OF_DXE_PROTOCOL_GUID notify function.
> //
> - Status = gSmst->SmmRegisterProtocolNotify (
> - &gEfiSmmEndOfDxeProtocolGuid,
> + Status = gMmst->MmRegisterProtocolNotify (
> + &gEfiMmEndOfDxeProtocolGuid,
> SmmEndOfDxeCallback,
> &SmmEndOfDxeRegistration
> );
> @@ -1014,7 +993,7 @@ VariableServiceInitialize (
> //
> // Register FtwNotificationEvent () notify function.
> //
> - Status = gSmst->SmmRegisterProtocolNotify (
> + Status = gMmst->MmRegisterProtocolNotify (
> &gEfiSmmFaultTolerantWriteProtocolGuid,
> SmmFtwNotificationEvent,
> &SmmFtwRegistration
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index db7d220e06df..ed7392cbcffc 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -48,6 +48,7 @@ [Defines]
> [Sources]
> Reclaim.c
> Variable.c
> + VariableTraditionalMm.c
> VariableSmm.c
> VarCheck.c
> Variable.h
> @@ -66,7 +67,7 @@ [LibraryClasses]
> BaseLib
> SynchronizationLib
> UefiLib
> - SmmServicesTableLib
> + MmServicesTableLib
> BaseMemoryLib
> DebugLib
> DxeServicesTableLib
> @@ -85,7 +86,7 @@ [Protocols]
> ## PRODUCES
> ## UNDEFINED # SmiHandlerRegister
> gEfiSmmVariableProtocolGuid
> - gEfiSmmEndOfDxeProtocolGuid ## NOTIFY
> + gEfiMmEndOfDxeProtocolGuid ## NOTIFY
> gEdkiiSmmVarCheckProtocolGuid ## PRODUCES
> gEfiTcgProtocolGuid ## SOMETIMES_CONSUMES
> gEfiTcg2ProtocolGuid ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c
> new file mode 100644
> index 000000000000..2143d3337e87
> --- /dev/null
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableTraditionalMm.c
> @@ -0,0 +1,114 @@
> +/** @file
> +
> + Parts of the SMM/MM implementation that are specific to traditional MM
> +
> +Copyright (c) 2011 - 2018, Intel Corporation. All rights reserved. <BR>
> +Copyright (c) 2018, Linaro, Ltd. All rights reserved. <BR>
> +This program and the accompanying materials
> +are licensed and made available under the terms and conditions of the BSD License
> +which accompanies this distribution. The full text of the license may be found at
> +http://opensource.org/licenses/bsd-license.php
> +
> +THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
> +WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +**/
> +
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Library/SmmMemLib.h>
> +#include "Variable.h"
> +
> +BOOLEAN
> +VariableSmmIsBufferOutsideSmmValid (
> + IN EFI_PHYSICAL_ADDRESS Buffer,
> + IN UINT64 Length
> + )
> +{
> + if (!SmmIsBufferOutsideSmmValid (Buffer, Length)) {
> + DEBUG ((EFI_D_ERROR, "SmmVariableHandler: SMM communication buffer in SMRAM or overflow!\n"));
Remove this debug message printing code?
> + return FALSE;
> + }
> + return TRUE;
> +}
Please add function comment header for it.
> +
> +/**
> + Notify the system that the SMM variable driver is ready
> +**/
> +VOID
> +VariableNotifySmmReady (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE Handle;
> +
> + Handle = NULL;
> + Status = gBS->InstallProtocolInterface (
> + &Handle,
> + &gEfiSmmVariableProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +}
> +
> +/**
> + Notify the system that the SMM variable write driver is ready
> +**/
> +VOID
> +VariableNotifySmmWriteReady (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + EFI_HANDLE Handle;
> +
> + Handle = NULL;
> + Status = gBS->InstallProtocolInterface (
> + &Handle,
> + &gSmmVariableWriteGuid,
> + EFI_NATIVE_INTERFACE,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +}
> +
> +EFI_STATUS
> +EFIAPI
> +VariableServiceInitialize (
> + IN EFI_HANDLE ImageHandle,
> + IN EFI_SYSTEM_TABLE *SystemTable
> + )
> +{
> + return MmVariableServiceInitialize ();
> +}
Please add function comment header for it.
> +
> +/**
> + Whether the TCG or TCG2 protocols are installed in the UEFI protocol database.
> + This information is used by the MorLock code to infer whether an existing
> + MOR variable is legitimate or not.
Add a line for return description?
Thanks,
Star
> +**/
> +BOOLEAN
> +VariableHaveTcgProtocols (
> + VOID
> + )
> +{
> + EFI_STATUS Status;
> + VOID *Interface;
> +
> + Status = gBS->LocateProtocol (
> + &gEfiTcg2ProtocolGuid,
> + NULL, // Registration
> + &Interface
> + );
> + if (!EFI_ERROR (Status)) {
> + return TRUE;
> + }
> +
> + Status = gBS->LocateProtocol (
> + &gEfiTcgProtocolGuid,
> + NULL, // Registration
> + &Interface
> + );
> + return !EFI_ERROR (Status);
> +}
>
next prev parent reply other threads:[~2019-01-10 7:20 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-01-03 18:28 [PATCH 0/6] implement standalone MM versions of the variable runtime drivers Ard Biesheuvel
2019-01-03 18:28 ` [PATCH] BaseTools/GenFds: permit stripped MM_CORE_STANDALONE binaries Ard Biesheuvel
2019-01-04 5:51 ` Feng, Bob C
2019-01-03 18:28 ` [PATCH 1/6] MdePkg/Include: add MmServicesTableLib header file Ard Biesheuvel
2019-01-10 6:06 ` Zeng, Star
2019-01-03 18:28 ` [PATCH 2/6] MdePkg: implement MmServicesTableLib based on traditional SMM Ard Biesheuvel
2019-01-10 1:35 ` Wang, Jian J
[not found] ` <9bfb4d7c-3d4e-c05c-49a1-1959ddc902e3@intel.com>
2019-01-10 6:54 ` Zeng, Star
2019-01-03 18:28 ` [PATCH 3/6] MdeModulePkg/FaultTolerantWriteDxe: factor out boot service accesses Ard Biesheuvel
2019-01-10 1:36 ` Wang, Jian J
2019-01-10 6:45 ` Zeng, Star
2019-01-03 18:28 ` [PATCH 4/6] MdeModulePkg/FaultTolerantWriteDxe: implement standalone MM version Ard Biesheuvel
2019-01-10 1:41 ` Wang, Jian J
2019-01-10 1:48 ` Wang, Jian J
2019-01-10 6:31 ` Zeng, Star
2019-01-10 6:47 ` Zeng, Star
2019-01-10 7:29 ` Zeng, Star
2019-01-10 7:33 ` Ard Biesheuvel
2019-01-10 7:59 ` Zeng, Star
2019-01-10 12:28 ` Wang, Jian J
2019-01-10 13:03 ` Laszlo Ersek
2019-01-10 16:23 ` Ard Biesheuvel
2019-01-11 2:18 ` Zeng, Star
2019-01-03 18:28 ` [PATCH 5/6] MdeModulePkg/VariableRuntimeDxe: factor out boot service accesses Ard Biesheuvel
2019-01-08 15:38 ` Laszlo Ersek
2019-01-10 2:33 ` Wang, Jian J
2019-01-10 7:17 ` Zeng, Star
2019-01-10 7:19 ` Zeng, Star [this message]
2019-01-03 18:28 ` [PATCH 6/6] MdeModulePkg/VariableRuntimeDxe: implement standalone MM version Ard Biesheuvel
2019-01-10 1:49 ` Wang, Jian J
2019-01-10 1:50 ` Wang, Jian J
2019-01-10 7:28 ` Zeng, Star
2019-01-03 19:13 ` [PATCH 0/6] implement standalone MM versions of the variable runtime drivers Ard Biesheuvel
2019-01-07 12:44 ` Gao, Liming
2019-01-07 13:05 ` Ard Biesheuvel
2019-01-07 19:08 ` Laszlo Ersek
2019-01-09 13:56 ` Gao, Liming
2019-01-09 15:29 ` Ard Biesheuvel
2019-01-14 2:55 ` Gao, Liming
2019-01-14 8:26 ` Ard Biesheuvel
2019-01-14 15:33 ` Gao, Liming
2019-01-09 9:44 ` Laszlo Ersek
2019-01-09 10:28 ` Ard Biesheuvel
2019-01-09 15:04 ` Laszlo Ersek
2019-01-09 21:46 ` Laszlo Ersek
2019-01-09 21:56 ` Ard Biesheuvel
2019-01-10 8:24 ` Zeng, Star
2019-01-13 15:42 ` Zeng, Star
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7a32163a-e90c-95ef-f231-d9b796e8bd39@intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox