Re: [edk2] [PATCH 2/6] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
On Mon, Jul 09, 2018 at 02:05:32PM +0530, Sughosh Ganu wrote: > hi Leif, > > On Tue, Jul 3, 2018 at 7:42 PM, Leif Lindholm > wrote: > > On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf > > > > I would prefer the document to be referred to by its official name and > > its document number: > > ARM Management Mode Interface Specification (ARM DEN0060A) > > > >> to communicate with the standalone MM environment in the secure world. > >> > >> This patch also adds the MM Communication driver (.inf) file to > >> define entry point for this driver and other compile > >> related information the driver needs. > >> > >> Contributed-under: TianoCore Contribution Agreement 1.1 > >> Signed-off-by: Achin Gupta > >> Signed-off-by: Supreeth Venkatesh > > > > Oh, and only one Signed-off-by per patch please. > > If authorship is to be indicated, ensure that's correct in git before > > calling format-patch. > > Supreeth has moved onto some other work, hence I will be working on > the upstreaming of these patches henceforth. Splendid, welcome aboard. > Will handle your comments > on all the patches and send an updated version. Regarding the > inclusion of a single Signed-off-By, i have a doubt. Work on these > patches was initially done by Achin, and then Supreeth. I will be > handling your review comments and posting the updated version. You > have posted a comment saying that we can have only a single s-o-b in > any given patch. In such a scenario, how can we attribute the work > done by all the engineers for these patches. Can you please let me > know on this. As per my understanding, other projects do allow > multiple s-o-b's per patch. Thanks. We do permit multiple s-o-b. When multiple developers are working together in public, adding non-trivial bits to a patch in flight, you can add multiple s-o-b. What is not appropriate is to post a patch containing company-internal details about who handled the patch before it was sent public. The signed-off-by is a statement regarding the suitability of the contribution - it is not attribution. The Author field is the only attribution. Regards, Leif p.s. I presume you're taking the set over because you work for ARM - so please communicate with the mailing list from your @arm.com account. ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 2/6] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
hi Leif, On Tue, Jul 3, 2018 at 7:42 PM, Leif Lindholm wrote: > On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf > > I would prefer the document to be referred to by its official name and > its document number: > ARM Management Mode Interface Specification (ARM DEN0060A) > >> to communicate with the standalone MM environment in the secure world. >> >> This patch also adds the MM Communication driver (.inf) file to >> define entry point for this driver and other compile >> related information the driver needs. >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Achin Gupta >> Signed-off-by: Supreeth Venkatesh > > Oh, and only one Signed-off-by per patch please. > If authorship is to be indicated, ensure that's correct in git before > calling format-patch. Supreeth has moved onto some other work, hence I will be working on the upstreaming of these patches henceforth. Will handle your comments on all the patches and send an updated version. Regarding the inclusion of a single Signed-off-By, i have a doubt. Work on these patches was initially done by Achin, and then Supreeth. I will be handling your review comments and posting the updated version. You have posted a comment saying that we can have only a single s-o-b in any given patch. In such a scenario, how can we attribute the work done by all the engineers for these patches. Can you please let me know on this. As per my understanding, other projects do allow multiple s-o-b's per patch. Thanks. -sughosh ___ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH 2/6] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
On Tue, Jul 03, 2018 at 03:25:11PM +0530, 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 DEN0060A_ARM_MM_Interface_Specification.pdf I would prefer the document to be referred to by its official name and its document number: ARM Management Mode Interface Specification (ARM DEN0060A) > to communicate with the standalone MM environment in the secure world. > > This patch also adds the MM Communication driver (.inf) file to > define entry point for this driver and other compile > related information the driver needs. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Achin Gupta > Signed-off-by: Supreeth Venkatesh Oh, and only one Signed-off-by per patch please. If authorship is to be indicated, ensure that's correct in git before calling format-patch. > Cc: Leif Lindholm > Cc: Ard Biesheuvel > --- > .../Drivers/MmCommunicationDxe/MmCommunication.c | 408 > + > .../Drivers/MmCommunicationDxe/MmCommunication.inf | 50 +++ And --stat/--stat-graph-width from https://github.com/tianocore/tianocore.github.io/wiki/Laszlo's-unkempt-git-guide-for-edk2-contributors-and-maintainers#contrib-23 would mean I wouldn't have to cross-reference below to see which files are being added/deleted/modified. Also, the diff order makes a difference. So please look into the orderfile as described there - noting that this can now be set permanently in newer revisions of git with git config diff.orderFile > 2 files changed, 458 insertions(+) > create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf > > diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > new file mode 100644 > index 000..8ba1790 > --- /dev/null > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > @@ -0,0 +1,408 @@ > +/** @file > + > + Copyright (c) 2016-2018, 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 > + > +#define MM_MAJOR_VER_MASK0x Should this not actually be 0xEFFF? > +#define MM_MINOR_VER_MASK0x > +#define MM_MAJOR_VER_SHIFT 16 Nothing wrong with this, but later code would be more easy to read if these were exposed through macros rather than explicitly extracted: #define MM_MAJOR_VER(x) (((x) & MM_MAJOR_VER_MASK) >> MM_MAJOR_VER_SHIFT) #define MM_MINOR_VER(x) ((x) & MM_MINOR_VER_MASK) > + > +const UINT32 MM_MAJOR_VER = 1; > +const UINT32 MM_MINOR_VER = 0; Meanwhile, these 1) Don't need to be given explicit variables (#define would work fine) 2) Have too generic names. What we're really talking about is MM_CALLER_MAJOR_VER and MM_CALLER_MINOR_VER? Finally please put these macros into a local .h file to be included. > + > +// > +// 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; > + > +/** > + 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] ThisThe EFI_MM_COMMUNICATION_PROTOCOL > + instance. > + @param[in, out] CommBuffer A pointer to the buffer to convey > + into MMRAM. > + @param[in, out] CommSizeThe size of the data buffer being > + passed in. This is optional. > + > + @retval
[edk2] [PATCH 2/6] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.
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 DEN0060A_ARM_MM_Interface_Specification.pdf
to communicate with the standalone MM environment in the secure world.
This patch also adds the MM Communication driver (.inf) file to
define entry point for this driver and other compile
related information the driver needs.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Achin Gupta
Signed-off-by: Supreeth Venkatesh
Cc: Leif Lindholm
Cc: Ard Biesheuvel
---
.../Drivers/MmCommunicationDxe/MmCommunication.c | 408 +
.../Drivers/MmCommunicationDxe/MmCommunication.inf | 50 +++
2 files changed, 458 insertions(+)
create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
create mode 100644 ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.inf
diff --git a/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
new file mode 100644
index 000..8ba1790
--- /dev/null
+++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c
@@ -0,0 +1,408 @@
+/** @file
+
+ Copyright (c) 2016-2018, 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
+
+#define MM_MAJOR_VER_MASK0x
+#define MM_MINOR_VER_MASK0x
+#define MM_MAJOR_VER_SHIFT 16
+
+const UINT32 MM_MAJOR_VER = 1;
+const UINT32 MM_MINOR_VER = 0;
+
+//
+// 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;
+
+/**
+ 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] ThisThe EFI_MM_COMMUNICATION_PROTOCOL
+ instance.
+ @param[in, out] CommBuffer A pointer to the buffer to convey
+ into MMRAM.
+ @param[in, out] CommSizeThe size of the data buffer being
+ passed in. This is optional.
+
+ @retval EFI_SUCCESS The message was successfully posted.
+ @retval EFI_INVALID_PARAMETER The CommBuffer was NULL.
+ @retval EFI_BAD_BUFFER_SIZE The buffer size is incorrect 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
+EFI_STATUS
+EFIAPI
+MmCommunicationCommunicate (
+ IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This,
+ IN OUT VOID *CommBuffer,
+ IN OUT UINTN*CommSize OPTIONAL
+ )
+{
+ EFI_MM_COMMUNICATE_HEADER *CommunicateHeader;
+ ARM_SMC_ARGSCommunicateSmcArgs;
+ EFI_STATUS Status;
+ UINTN BufferSize;
+
+ Status = EFI_ACCESS_DENIED;
+ BufferSize = 0;
+
+ ZeroMem (, sizeof (ARM_SMC_ARGS));
+
+ //
+ // Check parameters
+ //
+ if (CommBuffer == NULL) {
+return EFI_INVALID_PARAMETER;
+ }
+
+ CommunicateHeader = CommBuffer;
+ // CommBuffer is a
