From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=217.140.101.70; helo=foss.arm.com; envelope-from=supreeth.venkatesh@arm.com; receiver=edk2-devel@lists.01.org Received: from foss.arm.com (foss.arm.com [217.140.101.70]) by ml01.01.org (Postfix) with ESMTP id A6E1E20349DAA for ; Mon, 13 Nov 2017 04:11:06 -0800 (PST) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 8254480D; Mon, 13 Nov 2017 04:15:12 -0800 (PST) Received: from [10.0.2.15] (u203142.usa.arm.com [10.1.28.183]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id DB4613F246; Mon, 13 Nov 2017 04:15:10 -0800 (PST) Message-ID: <1510575309.2822.14.camel@arm.com> From: Supreeth Venkatesh To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Achin Gupta Date: Mon, 13 Nov 2017 06:15:09 -0600 In-Reply-To: References: <20171025163258.47961-1-supreeth.venkatesh@arm.com> <20171025163258.47961-3-supreeth.venkatesh@arm.com> <1510573245.2942.17.camel@arm.com> X-Mailer: Evolution 3.18.5.2-0ubuntu3.2 Mime-Version: 1.0 Subject: Re: [PATCH v2 2/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.22 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 13 Nov 2017 12:11:06 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Mon, 2017-11-13 at 11:48 +0000, Ard Biesheuvel wrote: > On 13 November 2017 at 11:40, Supreeth Venkatesh > wrote: > > > > On Mon, 2017-11-13 at 10:30 +0000, Ard Biesheuvel wrote: > > > > > > On 25 October 2017 at 17:32, Supreeth Venkatesh > > > 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 > > > > Signed-off-by: Supreeth Venkatesh > > > > --- > > > >  .../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 > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > +#include > > > > + > > > > +#include > > > > + > > > > +#include > > > > + > > > > +/** > > > > +  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?