From: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>,
Leif Lindholm <leif.lindholm@linaro.org>,
Achin Gupta <achin.gupta@arm.com>
Subject: Re: [PATCH v2 2/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
Date: Mon, 13 Nov 2017 06:15:09 -0600 [thread overview]
Message-ID: <1510575309.2822.14.camel@arm.com> (raw)
In-Reply-To: <CAKv+Gu90pMk81F1ysEzWeAucMYRFA_atY1x+CSMTnWJqqh7ffA@mail.gmail.com>
On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote:
> On 13 November 2017 at 11:40, Supreeth Venkatesh
> <supreeth.venkatesh@arm.com> wrote:
> >
> > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote:
> > >
> > > On 25 October 2017 at 17:32, Supreeth Venkatesh
> > > <supreeth.venkatesh@arm.com> wrote:
> > > >
> > > >
> > > > PI v1.5 Specification Volume 4 defines Management Mode Core
> > > > Interface
> > > > and defines EFI_MM_COMMUNICATION_PROTOCOL. This protocol
> > > > provides a
> > > > means of communicating between drivers outside of MM and MMI
> > > > handlers inside of MM.
> > > >
> > > > This patch implements the EFI_MM_COMMUNICATION_PROTOCOL DXE
> > > > runtime
> > > > driver for AARCH64 platforms. It uses SMCs allocated from the
> > > > standard
> > > > SMC range defined in
> > > > http://infocenter.arm.com/help/topic/com.arm.doc.den0060a/DEN00
> > > > 60A_
> > > > ARM_MM_Interface_Specification.pdf
> > > > to communicate with the standalone MM environment in the secure
> > > > world.
> > > >
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Achin Gupta <achin.gupta@arm.com>
> > > > Signed-off-by: Supreeth Venkatesh <supreeth.venkatesh@arm.com>
> > > > ---
> > > > .../Drivers/MmCommunicationDxe/MmCommunication.c | 339
> > > > +++++++++++++++++++++
> > > No need to separate the .inf and .c changes, so this patch can be
> > > merged with the previous one.
> > Thanks for more comments/feedback.
> > Ok. Please wait for v3.
> > >
> > >
> > > >
> > > >
> > > > 1 file changed, 339 insertions(+)
> > > > create mode 100644
> > > > ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > >
> > > > diff --git
> > > > a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > new file mode 100644
> > > > index 0000000000..ecf70e666c
> > > > --- /dev/null
> > > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
> > > > @@ -0,0 +1,339 @@
> > > > +/** @file
> > > > +
> > > > + Copyright (c) 2016-2017, ARM Limited. All rights reserved.
> > > > +
> > > > + 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/ArmLib.h>
> > > > +#include <Library/ArmSmcLib.h>
> > > > +#include <Library/BaseMemoryLib.h>
> > > > +#include <Library/DebugLib.h>
> > > > +#include <Library/DxeServicesTableLib.h>
> > > > +#include <Library/HobLib.h>
> > > > +#include <Library/PcdLib.h>
> > > > +#include <Library/UefiBootServicesTableLib.h>
> > > > +#include <Library/UefiRuntimeServicesTableLib.h>
> > > > +
> > > > +#include <Protocol/MmCommunication.h>
> > > > +
> > > > +#include <IndustryStandard/ArmStdSmc.h>
> > > > +
> > > > +/**
> > > > + Communicates with a registered handler.
> > > > +
> > > > + This function provides an interface to send and receive
> > > > messages
> > > > to the
> > > > + Standalone MM environment on behalf of UEFI services. This
> > > > function is part
> > > > + of the MM Communication Protocol that may be called in
> > > > physical
> > > > mode prior to
> > > > + SetVirtualAddressMap() and in virtual mode after
> > > > SetVirtualAddressMap().
> > > > +
> > > > + @param[in] This The
> > > > EFI_MM_COMMUNICATION_PROTOCOL instance.
> > > > + @param[in, out] CommBuffer A pointer to the buffer
> > > > to
> > > > convey into MMRAM.
> > > > + @param[in, out] CommSize The size of the data
> > > > buffer
> > > > being passed in.On exit, the size of data
> > > > + being returned. Zero if
> > > > the
> > > > handler does not wish to reply with any data.
> > > > +
> > > > + @retval EFI_SUCCESS The message was
> > > > successfully
> > > > posted.
> > > > + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL.
> > > > + @retval EFI_BAD_BUFFER_SIZE The buffer is too large
> > > > for
> > > > the MM implementation. If this error is
> > > > + returned, the
> > > > MessageLength
> > > > field in the CommBuffer header or the integer
> > > > + pointed by CommSize are
> > > > updated to reflect the maximum payload size the
> > > > + implementation can
> > > > accommodate.
> > > > + @retval EFI_ACCESS_DENIED The CommunicateBuffer
> > > > parameter or CommSize parameter, if not omitted,
> > > > + are in address range
> > > > that
> > > > cannot be accessed by the MM environment
> > > > +**/
> > > Please don't use lines > 80 characters.
> > Ok.
> > >
> > >
> > > >
> > > >
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +MmCommunicationCommunicate (
> > > > + IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
> > > > + IN OUT VOID *CommBuffer,
> > > > + IN OUT
> > > > UINTN *CommSize OPTIONAL
> > > > + );
> > > > +
> > > > +//
> > > > +// Address, Length of the pre-allocated buffer for
> > > > communication
> > > > with the secure
> > > > +// world.
> > > > +//
> > > > +STATIC ARM_MEMORY_REGION_DESCRIPTOR mNsCommBuffMemRegion;
> > > > +
> > > > +// Notification event when virtual address map is set.
> > > > +STATIC EFI_EVENT mSetVirtualAddressMapEvent;
> > > > +
> > > > +//
> > > > +// Handle to install the MM Communication Protocol
> > > > +//
> > > > +STATIC EFI_HANDLE mMmCommunicateHandle;
> > > > +
> > > > +//
> > > > +// MM Communication Protocol instance
> > > > +//
> > > > +EFI_MM_COMMUNICATION_PROTOCOL mMmCommunication = {
> > > > + MmCommunicationCommunicate
> > > > +};
> > > > +
> > > Please move this below the definition of
> > > MmCommunicationCommunicate(),
> > > and drop the forward declaration.
> > Ok.
> > >
> > >
> > > >
> > > >
> > > > +/**
> > > > + Communicates with a registered handler.
> > > > +
> > > > + This function provides an interface to send and receive
> > > > messages
> > > > to the
> > > > + Standalone MM environment on behalf of UEFI services. This
> > > > function is part
> > > > + of the MM Communication Protocol that may be called in
> > > > physical
> > > > mode prior to
> > > > + SetVirtualAddressMap() and in virtual mode after
> > > > SetVirtualAddressMap().
> > > > +
> > > > + @param[in] This The
> > > > EFI_MM_COMMUNICATION_PROTOCOL instance.
> > > > + @param[in, out] CommBuffer A pointer to the buffer
> > > > to
> > > > convey into SMRAM.
> > > What does 'convey into SMRAM' mean?
> > >
This is to mean Management Mode RAM.
There is a concept of System Management RAM which is separate from
Normal World UEFI.
> > > >
> > > >
> > > > + @param[in, out] CommSize The size of the data
> > > > buffer
> > > > being passed in.On exit, the size of data
> > > > + being returned. Zero if
> > > > the
> > > > handler does not wish to reply with any data.
> > > > +
> > > CommSize is a pointer so it could be NULL but not zero. Or do you
> > > mean
> > > *CommSize?
> > >
> > CommSize is an optional parameter.
> OK, please add OPTIONAL the prototype
Ok.
>
> >
> > Yeah, if Commsize is used, then its
> > value (*CommSize) will be zero, if there is no data. This is
> > something
> > the specification implicitly states.
> OK
>
>
> >
> > >
> > > >
> > > >
> > > > + @retval EFI_SUCCESS The message was
> > > > successfully
> > > > posted.
> > > > + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL.
> > > > + @retval EFI_BAD_BUFFER_SIZE The buffer is too large
> > > > for
> > > > the MM implementation. If this error is
> > > > + returned, the
> > > > MessageLength
> > > > field in the CommBuffer header or the integer
> > > > + pointed by CommSize are
> > > > updated to reflect the maximum payload size the
> > > > + implementation can
> > > > accommodate.
> > > > + @retval EFI_ACCESS_DENIED The CommunicateBuffer
> > > > parameter or CommSize parameter, if not omitted,
> > > > + are in address range
> > > > that
> > > > cannot be accessed by the MM environment
> > > > +**/
> > > STATIC ?
> > >
> > This implements an EFIAPI for the protocol
> > MM_COMMUNICATION_PROTOCOL in
> > MdePkg.Not sure why you want this function static?
> Any function that is only callable via function pointers should be
> static.
>
Ok.
> >
> > >
> > > >
> > > >
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +MmCommunicationCommunicate (
> > > > + IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
> > > > + IN OUT VOID *CommBuffer,
> > > > + IN OUT UINTN *CommSize
> > > > + )
> > > > +{
> > > > + EFI_MM_COMMUNICATE_HEADER *CommunicateHeader;
> > > > + ARM_SMC_ARGS CommunicateSmcArgs;
> > > > + EFI_STATUS Status;
> > > > + UINTN BufferSize;
> > > > +
> > > > + CommunicateHeader = CommBuffer;
> > > > + Status = EFI_SUCCESS;
> > > > + BufferSize = 0;
> > > > +
> > > > + ZeroMem (&CommunicateSmcArgs, sizeof (ARM_SMC_ARGS));
> > > > +
> > > > + //
> > > > + // Check parameters
> > > > + //
> > > > + if (CommBuffer == NULL) {
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + // If the length of the CommBuffer is 0 then return the
> > > > expected
> > > > length.
> > > > + if (CommSize) {
> > > > + if (*CommSize == 0) {
> > > > + *CommSize = mNsCommBuffMemRegion.Length;
> > > > + return EFI_BAD_BUFFER_SIZE;
> > > Here you return EFI_BAD_BUFFER_SIZE for a 0 sized buffer, while
> > > the
> > > comment says is it returned when the buffer is too large.
> > >
> > Specification states this:
> > "If the CommSize parameter is passed into the call, but the integer
> > it
> > points to, has a value of 0, then this must be updated to reflect
> > the
> > maximum size of the CommBuffer that the implementation can
> > tolerate."
> > Also, Return Values are
> > EFI_INVALID_PARAMETER - The buffer was NULL.
> > EFI_BAD_BUFFER_SIZE - The buffer is too large for the MM
> > implementation. If this error is returned, the MessageLength field
> > in
> > the CommBuffer header or the integer pointed by CommSize, are
> > updated
> > to reflect the maximum payload size the implementation can
> > accommodate.
> > See the function description above for more details
> >
> > So, it does not specify what should be the return value, when
> > *CommSize
> > is zero. It is open to interpretation between EFI_INVALID_PARAMETER
> > and
> > EFI_BAD_BUFFER_SIZE. I choose EFI_BAD_BUFFER_SIZE.
> > Let me know what you think.
> >
> If the specification does not state that an error should be returned,
> we should clarify that first.
I have this in my to-do list, its an error condition (no doubt about
that), its just that the return code in the specification need to have
an updated statement to reflect this.
>
> >
> > >
> > > >
> > > >
> > > > + }
> > > > + //
> > > > + // CommSize must hold HeaderGuid and MessageLength
> > > > + //
> > > > + if (*CommSize < sizeof (EFI_MM_COMMUNICATE_HEADER)) {
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > + BufferSize = *CommSize;
> > > > + } else {
> > > > + BufferSize = CommunicateHeader->MessageLength +
> > > > + sizeof (CommunicateHeader->HeaderGuid) +
> > > > + sizeof (CommunicateHeader->MessageLength);
> > > > + }
> > > > +
> > > > + //
> > > > + // If the buffer size is 0 or greater than what can be
> > > > tolerated
> > > > by the MM
> > > > + // environment then return the expected size.
> > > > + //
> > > > + if ((BufferSize == 0) ||
> > > > + (BufferSize > mNsCommBuffMemRegion.Length)) {
> > > > + CommunicateHeader->MessageLength =
> > > > mNsCommBuffMemRegion.Length
> > > > -
> > > > + sizeof
> > > > (CommunicateHeader-
> > > > >
> > > > > HeaderGuid) -
> > > > + sizeof
> > > > (CommunicateHeader-
> > > > >
> > > > > MessageLength);
> > > > + return EFI_BAD_BUFFER_SIZE;
> > > > + }
> > > > +
> > > > + // SMC Function ID
> > > > + CommunicateSmcArgs.Arg0 = ARM_SMC_ID_MM_COMMUNICATE_AARCH64;
> > > > +
> > > > + // Reserved for Future. Must be Zero.
> > > > + CommunicateSmcArgs.Arg1 = 0;
> > > > +
> > > > + if (mNsCommBuffMemRegion.VirtualBase) {
> > > > + CopyMem ((VOID *)mNsCommBuffMemRegion.VirtualBase,
> > > > CommBuffer,
> > > > BufferSize);
> > > > + }
> > > > +
> > > else? What happens if VirtualBase == NULL? Can we still proceed
> > > without copying?
> > Yup. This was Achin's comment as well. Please wait for v3.
> > >
> > >
> > > >
> > > >
> > > > + // For the SMC64 version, this parameter is a 64-bit
> > > > Physical
> > > > Address (PA)
> > > > + // or Intermediate Physical Address (IPA).
> > > > + // For the SMC32 version, this parameter is a 32-bit PA or
> > > > IPA.
> > > > + CommunicateSmcArgs.Arg2 =
> > > > (UINTN)mNsCommBuffMemRegion.PhysicalBase;
> > > > +
> > > > + // comm_size_address is a PA or an IPA that holds the size
> > > > of
> > > > the
> > > > + // communication buffer being passed in. This parameter is
> > > > optional
> > > > + // and can be omitted by passing a zero.
> > > > + // ARM does not recommend using it since this might require
> > > > the
> > > > + // implementation to create a separate memory mapping for
> > > > the
> > > > parameter.
> > > > + // ARM recommends storing the buffer size in the buffer
> > > > itself.
> > > > + CommunicateSmcArgs.Arg3 = 0;
> > > > +
> > > > + // Call the Standalone MM environment.
> > > > + ArmCallSmc (&CommunicateSmcArgs);
> > > > +
> > > > + Status = CommunicateSmcArgs.Arg0;
> > > > + switch (Status) {
> > > I think I mentioned this before, but please don't use Status for
> > > the
> > > ARM_SMC_MM_xxx enum space. Use a separate, correctly typed
> > > variable
> > > instead.
> > Oh. your last email asks to replace EFI_SUCCESS with MM Specific
> > RETURN
> > SUCCESS. Status is still "UINTN" type variable for both MM and
> > general
> > UEFI status codes.
> > Let me know if you feel strongly about it, I will have two
> > different
> > "UINTN" type variables.
> Status should be EFI_STATUS not UINTN.
>
> The ARM_SMC_MM_xxx symbolic constants are fundamentally a different
> type, and you are relying on the fact that EFI_SUCCESS and
> ARM_SMC_MM_RET_SUCCESS happen to be #defined to the same numeric
> value. That is very bad coding style.
No. I am *not* relying on the fact ARM_SMC_MM_RET_SUCCESS and
EFI_SUCCESS are same. I am just relying on the fact that
EFI_STATUS and MM return Status happen to be "UINTN" type ultimately.
EFI_STATUS -> RETURN_STATUS -> UINTN (MdePkg/Include/Base.h).
However, I get that you are trying to say these can be implemented as
"enums" and hence the reason to have two different variables to convert
from one type to the other. I will change it.
>
> >
> > >
> > >
> > > >
> > > >
> > > > + case ARM_SMC_MM_RET_SUCCESS:
> > > > + // On exit, the size of data being returned is inferred
> > > > from
> > > > + // CommSize or MessageLength + Header.
> > > > + CopyMem (CommBuffer, (const VOID
> > > > *)mNsCommBuffMemRegion.VirtualBase, BufferSize);
> > > > + break;
> > > > +
> > > > + case ARM_SMC_MM_RET_NOT_SUPPORTED:
> > > > + case ARM_SMC_MM_RET_INVALID_PARAMS:
> > > > + Status = EFI_INVALID_PARAMETER;
> > > > + break;
> > > > +
> > > > + case ARM_SMC_MM_RET_DENIED:
> > > > + Status = EFI_ACCESS_DENIED;
> > > > + break;
> > > > +
> > > > + case ARM_SMC_MM_RET_NO_MEMORY:
> > > > + // Unexpected error since the CommSize was checked for
> > > > zero
> > > > length
> > > > + // prior to issuing the SMC
> > > > + default:
> > > > + Status = EFI_ACCESS_DENIED;
> > > > + ASSERT (0);
> > > > + }
> > > > +
> > > > + return Status;
> > > > +}
> > > > +
> > > > +/**
> > > > + Notification callback on SetVirtualAddressMap event.
> > > > +
> > > > + This function notifies the MM communication protocol
> > > > interface
> > > > on
> > > > + SetVirtualAddressMap event and converts pointers used in
> > > > this
> > > > driver
> > > > + from physical to virtual address.
> > > > +
> > > > + @param Event SetVirtualAddressMap event.
> > > > + @param Context A context when the
> > > > SetVirtualAddressMap
> > > > triggered.
> > > > +
> > > > + @retval EFI_SUCCESS The function executed successfully.
> > > > + @retval Other Some error occurred when executing
> > > > this
> > > > function.
> > > > +
> > > > +**/
> > > > +STATIC
> > > > +VOID
> > > > +EFIAPI
> > > > +NotifySetVirtualAddressMap (
> > > > + IN EFI_EVENT Event,
> > > > + IN VOID *Context
> > > > + )
> > > > +{
> > > > + EFI_STATUS Status;
> > > > +
> > > > + Status = gRT->ConvertPointer (EFI_OPTIONAL_PTR,
> > > > + (VOID
> > > > **)&mNsCommBuffMemRegion.VirtualBase
> > > > + );
> > > > + if (EFI_ERROR (Status)) {
> > > > + DEBUG ((DEBUG_ERROR, "NotifySetVirtualAddressMap(): Unable
> > > > to
> > > > convert MM runtime pointer. Status:0x%x\n", Status));
> > > Please use %r for reporting EFI_STATUS values.
> > >
> > Ok.
> > >
> > > >
> > > >
> > > > + }
> > > > +
> > > > +}
> > > > +
> > > > +/**
> > > > + The Entry Point for MM Communication
> > > > +
> > > > + This function installs the MM communication protocol
> > > > interface
> > > > and finds out
> > > > + what type of buffer management will be required prior to
> > > > invoking the
> > > > + communication SMC.
> > > > +
> > > > + @param ImageHandle The firmware allocated handle for the
> > > > EFI
> > > > image.
> > > > + @param SystemTable A pointer to the EFI System Table.
> > > > +
> > > > + @retval EFI_SUCCESS The entry point is executed
> > > > successfully.
> > > > + @retval Other Some error occurred when executing
> > > > this
> > > > entry point.
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +MmCommunicationInitialize (
> > > > + IN EFI_HANDLE ImageHandle,
> > > > + IN EFI_SYSTEM_TABLE *SystemTable
> > > > + )
> > > > +{
> > > > + EFI_STATUS Status;
> > > > +
> > > > + mNsCommBuffMemRegion.PhysicalBase = 0;
> > > > + mNsCommBuffMemRegion.VirtualBase = 0;
> > > > + mNsCommBuffMemRegion.Length = 0;
> > > > +
> > > Please drop the 0 assignmentsm since they are overridden
> > > immediately.
> > Ok.
> > >
> > >
> > > >
> > > >
> > > > + mNsCommBuffMemRegion.PhysicalBase = PcdGet64
> > > > (PcdMmBufferBase);
> > > > + // During boot , Virtual and Physical are same
> > > > + mNsCommBuffMemRegion.VirtualBase =
> > > > mNsCommBuffMemRegion.PhysicalBase;
> > > > + mNsCommBuffMemRegion.Length = PcdGet64 (PcdMmBufferSize);
> > > > +
> > > > + if (mNsCommBuffMemRegion.PhysicalBase == 0) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Invalid MM
> > > > Buffer Base Address.\n"));
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + if (mNsCommBuffMemRegion.Length == 0) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Maximum
> > > > Buffer
> > > > Size is zero.\n"));
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory,
> > > > + mNsCommBuffMemRegion.PhysicalB
> > > > ase,
> > > > + mNsCommBuffMemRegion.Length,
> > > > + EFI_MEMORY_WB |
> > > > + EFI_MEMORY_XP |
> > > > + EFI_MEMORY_RUNTIME);
> > > > + if (EFI_ERROR (Status)) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to
> > > > add
> > > > MM-NS Buffer Memory Space\n"));
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + Status = gDS-
> > > > >
> > > > > SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase,
> > > > + mNsCommBuffMemRegion.
> > > > Leng
> > > > th,
> > > > + EFI_MEMORY_WB |
> > > > EFI_MEMORY_XP);
> > > > + if (EFI_ERROR (Status)) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to
> > > > set
> > > > MM-NS Buffer Memory attributes\n"));
> > > cleanup?
> > Yup. Wait for v3.
> > >
> > >
> > > >
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + Status = gBS->AllocatePages (AllocateAddress,
> > > > + EfiRuntimeServicesData,
> > > > + EFI_SIZE_TO_PAGES
> > > > (mNsCommBuffMemRegion.Length),
> > > > + &mNsCommBuffMemRegion.PhysicalB
> > > > ase)
> > > > ;
> > > > + if (EFI_ERROR (Status)) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicateInitialize: Failed to
> > > > allocate MM-NS Buffer Memory Space\n"));
> > > cleanup?
> > >
> > This was Udit's comment as well. Wait for v3.
> > >
> > > >
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + // Install the communication protocol
> > > > + Status = gBS->InstallProtocolInterface
> > > > (&mMmCommunicateHandle,
> > > > + &gEfiMmCommunication
> > > > Prot
> > > > ocolGuid,
> > > > + EFI_NATIVE_INTERFACE
> > > > ,
> > > > + &mMmCommunication);
> > > > + if (EFI_ERROR(Status)) {
> > > > + DEBUG ((DEBUG_ERROR, "MmCommunicationInitialize: Failed to
> > > > install MM communication protocol\n"));
> > > cleanup?
> > Yup. Wait for v3.
> > >
> > >
> > > >
> > > >
> > > > + return EFI_INVALID_PARAMETER;
> > > > + }
> > > > +
> > > > + // Register notification callback when virtual address is
> > > > associated
> > > > + // with the physical address.
> > > > + // Create a Set Virtual Address Map event.
> > > > + //
> > > > + Status = gBS->CreateEvent
> > > > (EVT_SIGNAL_VIRTUAL_ADDRESS_CHANGE, // Type
> > > > + TPL_NOTIFY,
> > > > /
> > > > / NotifyTpl
> > > > + NotifySetVirtualAddressMap,
> > > > /
> > > > / NotifyFunction
> > > > + NULL,
> > > > /
> > > > / NotifyContext
> > > > + &mSetVirtualAddressMapEvent
> > > > /
> > > > / Event
> > > > + );
> > > > + ASSERT_EFI_ERROR (Status);
> > > > +
> > > cleanup?
> > Not required. Since it will still work at boot time.
> If gBS->CreateEvent () fails in a RELEASE build, it will ignore the
> ASSERT () and return the error code, which will result in the driver
> to be unloaded. Without cleanup, that will leave protocol pointers
> pointing into memory which has now been freed. Also, the buffer
> allocation will not be freed either.
Ok. So the cleanup here will be to free the buffer and uninstall the
protocol. Is that what you had in mind?
next prev parent reply other threads:[~2017-11-13 12:11 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-10-25 16:32 [PATCH v2 0/3] *** EFI_MM_COMMUNICATION_PROTOCOL *** Supreeth Venkatesh
2017-10-25 16:32 ` [PATCH v2 1/3] ArmPkg: Add PCDs needed for MM communication driver Supreeth Venkatesh
2017-11-13 10:17 ` Ard Biesheuvel
2017-11-13 11:00 ` Supreeth Venkatesh
2017-10-25 16:32 ` [PATCH v2 2/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver Supreeth Venkatesh
2017-10-26 5:05 ` Udit Kumar
2017-10-26 15:14 ` Supreeth Venkatesh
2017-10-26 10:13 ` Achin Gupta
2017-10-26 16:03 ` Supreeth Venkatesh
2017-11-13 10:30 ` Ard Biesheuvel
2017-11-13 11:40 ` Supreeth Venkatesh
2017-11-13 11:48 ` Ard Biesheuvel
2017-11-13 12:15 ` Supreeth Venkatesh [this message]
2017-11-13 12:19 ` Ard Biesheuvel
2017-11-13 12:42 ` Supreeth Venkatesh
2017-11-13 12:46 ` Ard Biesheuvel
2017-10-25 16:32 ` [PATCH v2 3/3] ArmPkg/Drivers: Add ArmMmCommunication Driver information file Supreeth Venkatesh
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=1510575309.2822.14.camel@arm.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