From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c0b::230; helo=mail-it0-x230.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x230.google.com (mail-it0-x230.google.com [IPv6:2607:f8b0:4001:c0b::230]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CF7C921B00DC7 for ; Mon, 13 Nov 2017 04:15:22 -0800 (PST) Received: by mail-it0-x230.google.com with SMTP id x28so3429752ita.0 for ; Mon, 13 Nov 2017 04:19:28 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=t0rvQtoGI0o1WdTUcehEK7Y3KOlg52U1KSi5WZZkY1w=; b=cta4zST9YRM0+/G+yRlOq4XaRTJ8aSz2H4NT1+TLIRbvyTJRlJOWSd6lxIgeGDYlW7 jJtOsdLkCmS2iSJuVfkGb5evzV7wGDwqIipnFkgaCj720l8Z1Y+iG3Hh11jc7egcoN9j +Qt+ibO2zhdpJPsMQJZ6ltrbyxPjvezUp/n/0= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=t0rvQtoGI0o1WdTUcehEK7Y3KOlg52U1KSi5WZZkY1w=; b=oh6Cy5Kwi3yamUfnqYQ1o0KMi9HV/ajL5H0BTqoXCV2H7NQmAgBNJunTahGdYKKgh/ T+kaKJkcbNitZKX/cEKQ6RTVU1JPTRQ0G8YmRsOGDAnTQ2bH1F8D5zFbKCiTBzA7sUb0 nrbO+S/uUdf66RqaZcYu9IZA4mUbropGiqpqF7AH6pWs8DcbBf8LglirtOKwM4vp1d1c 8hOF0ylG6iqnoBbVY6G8POEezCMWPtKqaZ9besEbwpGtLsLyqsbuK4BusxFh/mQjwFZd Nz9I0cLRiBms1+x/5fpVgo//TO+DbPy68r/YtT0HZyzAIsaPaovkriC/7EigJ5Ii4fHk 6NEA== X-Gm-Message-State: AJaThX6W+KfPHLBhfXYRZw9KzV4fljU3MxPWQLaxZTfFz1P++93EvBzP wDrGo9aIUyh9s4UBfrDwux3xxrNqcJEivin2Qnl/Bg== X-Google-Smtp-Source: AGs4zMY2hYnl3z/H6SF+iPQcZYmo4jFA4NSk/bHOvOfFGgNDOdXqjTmD2L1pnalZZpq87Me9QT21k1zCgp1EtHuvx28= X-Received: by 10.36.141.70 with SMTP id w67mr10203624itd.58.1510575567619; Mon, 13 Nov 2017 04:19:27 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.20 with HTTP; Mon, 13 Nov 2017 04:19:27 -0800 (PST) In-Reply-To: <1510575309.2822.14.camel@arm.com> 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> From: Ard Biesheuvel Date: Mon, 13 Nov 2017 12:19:27 +0000 Message-ID: To: Supreeth Venkatesh Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Achin Gupta 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:15:23 -0000 Content-Type: text/plain; charset="UTF-8" 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/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. I know what SMRAM means. But what does the sentence 'A pointer to the buffer to convey into SMRAM' mean? >> > > > >> > > > >> > > > + @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. > > >> >> > >> > > >> > > >> > > > >> > > > >> > > > + 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? Yes.