On Thu, 2017-10-12 at 18:29 +0100, Ard Biesheuvel wrote: > Hi Supreeth, Hi Ard,
Thanks for your comments. I have rolled all of them in v2 of this patch series except for "static" comment below. > > On 12 October 2017 at 18:13, Supreeth Venkatesh > <[email protected]> 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 in the SMC Calling Convention specification to > > communicate > > with the standalone MM environment in the secure world. > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Supreeth Venkatesh <[email protected]> > > --- > > .../Drivers/MmCommunicationDxe/MmCommunication.c | 314 > > +++++++++++++++++++++ > > 1 file changed, 314 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..6064c7d345 > > --- /dev/null > > +++ b/ArmPkg/Drivers/MmCommunicationDxe/MmCommunication.c > > @@ -0,0 +1,314 @@ > > +/** @file > > + > > + Copyright (c) 2016-2017, ARM Limited. All rights reserved. > > + > > + This program and the accompanying materials > > + are licensed and made available under the terms and conditions > > of the BSD License > > + which accompanies this distribution. The full text of the > > license may be found at > > + http://opensource.org/licenses/bsd-license.php > > + > > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" > > BASIS, > > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER > > EXPRESS OR IMPLIED. > > + > > +**/ > > + > > +#include <Library/ArmLib.h> > > +#include <Library/ArmSmcLib.h> > > +#include <Library/BaseMemoryLib.h> > > +#include <Library/DebugLib.h> > > +#include <Library/DxeServicesTableLib.h> > > +#include <Library/HobLib.h> > > +#include <Library/PcdLib.h> > > +#include <Library/UefiBootServicesTableLib.h> > > +#include <Library/UefiRuntimeServicesTableLib.h> > > + > > +#include <Protocol/MmCommunication.h> > > + > > +#include <IndustryStandard/ArmStdSmc.h> > > + > > +/** > > + Communicates with a registered handler. > > + > > + This function provides an interface to send and receive messages > > to the > > + Standalone MM environment on behalf of UEFI services. This > > function is part > > + of the MM Communication Protocol that may be called in physical > > mode prior to > > + SetVirtualAddressMap() and in virtual mode after > > SetVirtualAddressMap(). > > + > > + @param[in] This The > > EFI_MM_COMMUNICATION_PROTOCOL instance. > > + @param[in, out] CommBuffer A pointer to the buffer to > > convey into MMRAM. > > + @param[in, out] CommSize The size of the data buffer > > being passed in.On exit, the size of data > > + being returned. Zero if the > > handler does not wish to reply with any data. > > + > > + @retval EFI_SUCCESS The message was successfully > > posted. > > + @retval EFI_INVALID_PARAMETER The CommBuffer was NULL. > > + @retval EFI_BAD_BUFFER_SIZE The buffer is too large for > > the MM implementation. If this error is > > + returned, the MessageLength > > field in the CommBuffer header or the integer > > + pointed by CommSize are > > updated to reflect the maximum payload size the > > + implementation can > > accommodate. > > + @retval EFI_ACCESS_DENIED The CommunicateBuffer > > parameter or CommSize parameter, if not omitted, > > + are in address range that > > cannot be accessed by the MM environment > > +**/ > > +EFI_STATUS > Can this be static? If yes, please remove the forward declaration > entirely, and reorder the definition with the reference. > > > > > +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 = { > Can this be static? > > > > > + MmCommunicationCommunicate > > +}; > > + > > +/** > > + 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. > > + @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 > > +**/ > > +EFI_STATUS > > +EFIAPI > > +MmCommunicationCommunicate ( > > + IN CONST EFI_MM_COMMUNICATION_PROTOCOL *This, > > + IN OUT VOID *CommBuffer, > > + IN OUT UINTN *CommSize > > + ) > > +{ > > + EFI_MM_COMMUNICATE_HEADER *CommunicateHeader; > Please align the * with the other variable names > > > > > + 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) { > > + BufferSize = *CommSize; > > + // > > + // CommSize must hold HeaderGuid and MessageLength > > + // > > + if ( BufferSize < sizeof(EFI_MM_COMMUNICATE_HEADER) ) { > No spaces inside () please > > > > > + return EFI_INVALID_PARAMETER; > > + } > > + } else { > > + BufferSize = CommunicateHeader->MessageLength + > > sizeof(CommunicateHeader->HeaderGuid) + sizeof(CommunicateHeader- > > >MessageLength); > Spaces before ( please, and limit the line length to 80 columns. > > > > > + } > > + > > + // > > + // If the buffer size if 0 or greater than what can be tolerated > > by the MM > > + // environment then return the expected size. > > + // > > + if ( (BufferSize == 0) || > > + (BufferSize > mNsCommBuffMemRegion.Length) ) { > No spaces inside () please > > > > > + CommunicateHeader->MessageLength = > > mNsCommBuffMemRegion.Length; > > + 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; > > + > > + CopyMem((VOID *)mNsCommBuffMemRegion.VirtualBase, CommBuffer, > > BufferSize); > Space before ( > > > > > + > > + // 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; > No spaces after (type) casts please. > > > > > + > > + CommunicateSmcArgs.Arg3 = (UINTN) BufferSize; > > + > > + // Call the Standalone MM environment. > > + ArmCallSmc(&CommunicateSmcArgs); > Space before ( > > > > > + > > + Status = CommunicateSmcArgs.Arg0; > > + switch (Status) { > > + case EFI_SUCCESS: > Please don't overload the EFI_STATUS type. If EFI_SUCCESS and > ARM_SMC_MM_RET_SUCCESS happen to have the same numerical value, that > doesn't mean you should cut corners like this. So instead, > > switch (CommunicateSmcArgs.Arg0) { > case ARM_SMC_MM_RET_SUCCESS: > Status = EFI_SUCCESS; > > > > > + 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: > > + ASSERT (0); > and don't forget to assign Status here. > > > > > > + } > > + > > + 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 > VOID on a separate line please > > > > > +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 ((EFI_D_ERROR, "NotifySetVirtualAddressMap(): Unable to > > convert MM runtime pointer. Status:0x%x\n", Status)); > please use DEBUG_xxx not EFI_D_xxx (those are deprecated) > > > > > + } > > + > > +} > > + > > +/** > > + 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; > > + > > + mNsCommBuffMemRegion.PhysicalBase = PcdGet64(PcdMmBufferBase); > Space before ( > > > > > + // During boot , Virtual and Physical are same > > + mNsCommBuffMemRegion.VirtualBase = > > mNsCommBuffMemRegion.PhysicalBase; > > + mNsCommBuffMemRegion.Length = PcdGet64(PcdMmBufferSize); > and here > > > > > + > > + if (mNsCommBuffMemRegion.PhysicalBase == 0) { > > + DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Invalid MM > > Buffer Base Address.\n")); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + if (mNsCommBuffMemRegion.Length == 0) { > > + DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Maximum Buffer > > Size is zero.\n")); > > + return EFI_INVALID_PARAMETER; > > + } > > + > > + Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, > > + mNsCommBuffMemRegion.PhysicalBase, > > + mNsCommBuffMemRegion.Length, > > + EFI_MEMORY_UC | > > EFI_MEMORY_RUNTIME); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to add > > MM-NS Buffer Memory Space\n")); > Are you sure it makes sense to proceed after this error? > > > > > + } > > + > > + Status = gDS- > > >SetMemorySpaceAttributes(mNsCommBuffMemRegion.PhysicalBase, > > + mNsCommBuffMemRegion.Leng > > th, > > + EFI_MEMORY_UC); > > + if (EFI_ERROR (Status)) { > > + DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to set > > MM-NS Buffer Memory attributes\n")); > And here > > > > > + } > > + > > + Status = gBS->AllocatePages (AllocateAddress, > > + EfiRuntimeServicesData, > > + EFI_SIZE_TO_PAGES > > (mNsCommBuffMemRegion.Length), > > + &mNsCommBuffMemRegion.PhysicalBase) > > ; > > + if (EFI_ERROR (Status)) { > > + DEBUG ((EFI_D_ERROR, "MmCommunicateInitialize: Failed to > > allocate MM-NS Buffer Memory Space\n")); > And here > > > > > + } > > + > > + DEBUG ((DEBUG_INFO, "MmCommunicateInitialize: NsBufferAddress - > > 0x%x, mNsCommBuffMemRegion.Length - 0x%x\n", > > mNsCommBuffMemRegion.PhysicalBase, mNsCommBuffMemRegion.Length)); > Line length < 80 columns please > > > > > + > > + // Install the communication protocol > > + Status = gBS->InstallProtocolInterface (&mMmCommunicateHandle, > > + &gEfiMmCommunicationProt > > ocolGuid, > > + EFI_NATIVE_INTERFACE, > > + &mMmCommunication); > > + if (EFI_ERROR(Status)) { > > + DEBUG((EFI_D_ERROR, "MmCommunicationInitialize: Failed to > > install MM communication protocol\n")); > return Status > > > > > + } > > + > > + // 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); > > + > > + return Status; > > +} > > -- > > 2.14.1 > > > > _______________________________________________ > > edk2-devel mailing list > > [email protected] > > https://lists.01.org/mailman/listinfo/edk2-devel _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
