Re: [PATCH v1 1/3] ArmPkg/Drivers: Add EFI_MM_COMMUNICATION_PROTOCOL DXE driver.

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

 Hi Supreeth,

On 12 October 2017 at 18:13, Supreeth Venkatesh
<supreeth.venkatesh@arm.com> 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 <supreeth.venkatesh@arm.com>
> ---
>  .../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.Length,
> +                                         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,
> +                                          &gEfiMmCommunicationProtocolGuid,
> +                                          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
> edk2-devel@lists.01.org
> https://lists.01.org/mailman/listinfo/edk2-devel