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 90C6920356886 for ; Mon, 13 Nov 2017 04:38:38 -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 9E22E80D; Mon, 13 Nov 2017 04:42:44 -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 AC34A3F246; Mon, 13 Nov 2017 04:42:43 -0800 (PST) Message-ID: <1510576960.3009.12.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:42:40 -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> <1510575309.2822.14.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:38:38 -0000 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit On Mon, 2017-11-13 at 12:19 +0000, Ard Biesheuvel wrote: > On 13 November 2017 at 12:15, Supreeth Venkatesh > wrote: > > > > 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/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 > > > > > > Signed-off-by: Supreeth Venkatesh > > > > > 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 > > > > > > +#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. > 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? > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > +  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.