Re: [PATCH v2 2/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 13 November 2017 at 12:42, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> wrote:
> On Mon, 2017-11-13 at 12:19 +0000, Ard Biesheuvel wrote:
>> On 13 November 2017 at 12:15, Supreeth Venkatesh
>> <supreeth.venkatesh@arm.com> wrote:
>> >
>> > 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/D
>> > > > > > EN00
>> > > > > > 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.c
>> > > > > > om>
>> > > > > > ---
>> > > > > >  .../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.
>> I know what SMRAM means. But what does the sentence 'A pointer to the
>> buffer to convey into SMRAM' mean?
>>
> So, the contents of this buffer will be conveyed into the space which
> is set apart for MM foundation to run. (Which is basically portion of
> RAM used by Management Mode foundation.) In AARCH64 case, Management
> Mode is run in S-EL0, hence a portion of the Memory which is separated
> by trust zone and allowed for use in Management Mode firmware.
> Management Mode Memory is isolated from Normal World Memory and the
> mapping is platform dependent and done in privileged firmware.
> if you need more information, we can discuss this off-line.
>> >
>> > >
>> > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  @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.
>> OK
>>
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +    }
>> > > > > > +    //
>> > > > > > +    // 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).
>> >
>> Yes you are.
>>
>> If Status == ARM_SMC_MM_RET_SUCCESS, you return 'Status' from the
>> function, where it means EFI_SUCCESS.
>>
>> >
>> > 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.
>> >
>> I am simply saying that Status should only be used to hold EFI_xxx
>> constants, and that you should use a different variable to hold
>> ARM_SMC_MM_RET_xxx constants, and any conversion between the two
>> should involve if () or switch () statements.
> Sorry, you missed the switch statement for the conversion below, the
> part that needs change is to have  EFI_Status variable (to hold
> converted Status) and UINTN MmStatus or something similar to hold MM
> Return Status, since both of them are technically separate status. Is
> this what you mean?


Your code does

EFI_STATUS                  Status;
...
Status = CommunicateSmcArgs.Arg0;
switch (Status) {
case ARM_SMC_MM_RET_SUCCESS:
...
break;
...
return Status;

so it ultimately returns the value of Arg0 directly from the function.


Instead, you could do

switch (CommunicateSmcArgs.Arg0){
...
Status = EFI_xxx

and it would be fine.




>>
>> >
>> >
>> >
>> > >
>> > >
>> > > >
>> > > >
>> > > > >
>> > > > >
>> > > > >
>> > > > > >
>> > > > > >
>> > > > > >
>> > > > > > +  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.Physi
>> > > > > > calB
>> > > > > > ase,
>> > > > > > +                                mNsCommBuffMemRegion.Lengt
>> > > > > > h,
>> > > > > > +                                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.PhysicalBas
>> > > > > > > e,
>> > > > > > +                                         mNsCommBuffMemReg
>> > > > > > ion.
>> > > > > > 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.Physi
>> > > > > > calB
>> > > > > > 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,
>> > > > > > +                                          &gEfiMmCommunica
>> > > > > > tion
>> > > > > > Prot
>> > > > > > ocolGuid,
>> > > > > > +                                          EFI_NATIVE_INTER
>> > > > > > FACE
>> > > > > > ,
>> > > > > > +                                          &mMmCommunicatio
>> > > > > > n);
>> > > > > > +  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?
>> Yes.