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::231; helo=mail-it0-x231.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-it0-x231.google.com (mail-it0-x231.google.com [IPv6:2607:f8b0:4001:c0b::231]) (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 EA60A20349D91 for ; Mon, 13 Nov 2017 04:42:37 -0800 (PST) Received: by mail-it0-x231.google.com with SMTP id m191so6935432itg.1 for ; Mon, 13 Nov 2017 04:46:44 -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=Q2OKzUkI000YcoH/4cTx/mVLxlVjlEiLzqvceoxwDco=; b=YnXwrp9zHpaJtjRPTYdCF6f7LQ+JyvgAvcqQotlIGWyn4ZACXeMCvbG2n3n1NW/ZGC GN9qlZbzDvnm/YJFRqg1VqmidgQplF2EMhPVYj08J2FieT90aDqjsrDYCuX9UGZDJUAc ED0riKg/aYGv+vbAQ7/9S6iggv4qEBCLMG/3Y= 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=Q2OKzUkI000YcoH/4cTx/mVLxlVjlEiLzqvceoxwDco=; b=Kp1C/gV9Izk1KKXDDFotnMXLbtAqgasL3R56azUd95d9ucVoDcafqozdnyPReA06/+ Ff2w7E7C4a5yeVJAWM/fuOw5o1kJ7xZiXawTfG1ryaajUNzH6lquvPOTwsqAolrXzgT5 Mk+r0OdIwphzJMJK3Fwe78QO3QYgUKygfjcskC1lrPGMFII4uT/LmJuEzAyMcqc1JChu iH8vS6kYZyvw9sglCmnEitzUHCAOd9jczVj7L/o1NPFoFAEkWKSAVif7kTquM6Tf+Hui dLu9WDJj5qCLBp5WUL9Th9gFPLWOxkf1xFe5OQcshvQ0LDUA1DcRL1lazV7VtRYyH8Rz Fgkg== X-Gm-Message-State: AJaThX6gVkoizawZ9geAv22k8W6jm2wX4pUss6bUgenN2d5dbF6Bxr4o Rb0qdcdfeOmhbXGWW6nXUPY+YktLE1ZcYjejPIOSQQ== X-Google-Smtp-Source: AGs4zMappwvzjuQ+Xzu5Jgmf5mSzBqb+0pokJhQKhLN1VILkkDMgrvk/W8JU8dzvzTrUk6e0C+2tUHdtiWIByWs8blE= X-Received: by 10.36.48.4 with SMTP id q4mr10212137itq.34.1510577202849; Mon, 13 Nov 2017 04:46:42 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.104.20 with HTTP; Mon, 13 Nov 2017 04:46:42 -0800 (PST) In-Reply-To: <1510576960.3009.12.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> <1510576960.3009.12.camel@arm.com> From: Ard Biesheuvel Date: Mon, 13 Nov 2017 12:46:42 +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:42:38 -0000 Content-Type: text/plain; charset="UTF-8" On 13 November 2017 at 12:42, Supreeth Venkatesh wrote: > 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? 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.