[PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA

[PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA

[Ard Biesheuvel]

This set implements two changes to the support we have in EmbeddedPkg
for non-coherent DMA, which together should allow the Raspberry Pi4 to
use both the platform and PCI based DMA devices.

Patch #1 implements support for limiting DMA to a certain memory region.
This is necessary given that the RPi4 ships with more than 1 GB of memory
in some configurations, but uses DMA translation for the platform devices
in a way that puts that memory out of reach for 32-bit DMA (i.e., the DMA
translation is +3 GB). By setting the device DMA limit to MAX_UINT32, the
library will infer a host address limit of 1 GB, and use bounce buffering
if any buffers are mapped outside that region.

Patch #2 implements a trivial wrapper around DmaLib that exposes the EDK2
IoMmu protocol. This is used by the generic PCI infrastructure instead of
the builtin DMA routines when the protocol exists, so it is a natural place
to implement the non-cache coherent DMA handling we need for the RPi4.

Tested on Raspberry Pi 4.

Changes since v1:
- fix a few major issues in #1
- add Leif's ack to #2

Cc: awarkentin@vmware.com
Cc: jeremy.linton@arm.com
Cc: pete@akeo.ie
Cc: samer.el-haj-mahmoud@arm.com

Ard Biesheuvel (2):
  EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
  EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib

 .../NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c | 257 ++++++++++++++++++
 .../NonCoherentIoMmuDxe.inf                   |  43 +++
 EmbeddedPkg/EmbeddedPkg.dec                   |   6 +
 EmbeddedPkg/EmbeddedPkg.dsc                   |   5 +
 .../NonCoherentDmaLib/NonCoherentDmaLib.c     | 165 ++++++++++-
 .../NonCoherentDmaLib/NonCoherentDmaLib.inf   |   1 +
 6 files changed, 465 insertions(+), 12 deletions(-)
 create mode 100644 EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
 create mode 100644 EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf

-- 
2.17.1

[PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Ard Biesheuvel]

Implement support for driving peripherals with limited DMA ranges to
NonCoherentDmaLib, by adding a device address limit, and taking it,
along with the device offset, into account when allocating or mapping
DMA buffers.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
 3 files changed, 160 insertions(+), 12 deletions(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 8812a6db7c30..69922802f473 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
   #
   gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
 
+  #
+  # Highest address value supported by the device for DMA addressing. Note
+  # that this value should be strictly greater than PcdDmaDeviceOffset.
+  #
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
+
   #
   # Selection between DT and ACPI as a default
   #
diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
index 78220f6358aa..115345765435 100644
--- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
+++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
@@ -40,6 +40,8 @@ typedef struct {
 STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
 STATIC LIST_ENTRY                 UncachedAllocationList;
 
+STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
+
 STATIC
 PHYSICAL_ADDRESS
 HostToDeviceAddress (
@@ -49,6 +51,102 @@ HostToDeviceAddress (
   return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
 }
 
+/**
+  Allocates one or more 4KB pages of a certain memory type at a specified
+  alignment.
+
+  Allocates the number of 4KB pages specified by Pages of a certain memory type
+  with an alignment specified by Alignment. The allocated buffer is returned.
+  If Pages is 0, then NULL is returned. If there is not enough memory at the
+  specified alignment remaining to satisfy the request, then NULL is returned.
+  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
+  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
+
+  @param  MemoryType            The type of memory to allocate.
+  @param  Pages                 The number of 4 KB pages to allocate.
+  @param  Alignment             The requested alignment of the allocation.
+                                Must be a power of two.
+                                If Alignment is zero, then byte alignment is
+                                used.
+
+  @return A pointer to the allocated buffer or NULL if allocation fails.
+
+**/
+STATIC
+VOID *
+InternalAllocateAlignedPages (
+  IN EFI_MEMORY_TYPE  MemoryType,
+  IN UINTN            Pages,
+  IN UINTN            Alignment
+  )
+{
+  EFI_STATUS            Status;
+  EFI_PHYSICAL_ADDRESS  Memory;
+  UINTN                 AlignedMemory;
+  UINTN                 AlignmentMask;
+  UINTN                 UnalignedPages;
+  UINTN                 RealPages;
+
+  //
+  // Alignment must be a power of two or zero.
+  //
+  ASSERT ((Alignment & (Alignment - 1)) == 0);
+
+  if (Pages == 0) {
+    return NULL;
+  }
+  if (Alignment > EFI_PAGE_SIZE) {
+    //
+    // Calculate the total number of pages since alignment is larger than page
+    // size.
+    //
+    AlignmentMask  = Alignment - 1;
+    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
+    //
+    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
+    // overflow.
+    //
+    ASSERT (RealPages > Pages);
+
+    Memory = mDmaHostAddressLimit;
+    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, RealPages,
+                    &Memory);
+    if (EFI_ERROR (Status)) {
+      return NULL;
+    }
+    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
+    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);
+    if (UnalignedPages > 0) {
+      //
+      // Free first unaligned page(s).
+      //
+      Status = gBS->FreePages (Memory, UnalignedPages);
+      ASSERT_EFI_ERROR (Status);
+    }
+    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
+    UnalignedPages = RealPages - Pages - UnalignedPages;
+    if (UnalignedPages > 0) {
+      //
+      // Free last unaligned page(s).
+      //
+      Status = gBS->FreePages (Memory, UnalignedPages);
+      ASSERT_EFI_ERROR (Status);
+    }
+  } else {
+    //
+    // Do not over-allocate pages in this case.
+    //
+    Memory = mDmaHostAddressLimit;
+    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
+                    &Memory);
+    if (EFI_ERROR (Status)) {
+      return NULL;
+    }
+    AlignedMemory = (UINTN)Memory;
+  }
+  return (VOID *)AlignedMemory;
+}
+
 /**
   Provides the DMA controller-specific addresses needed to access system memory.
 
@@ -111,7 +209,30 @@ DmaMap (
     return  EFI_OUT_OF_RESOURCES;
   }
 
-  if (Operation != MapOperationBusMasterRead &&
+  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
+
+    if (Operation == MapOperationBusMasterCommonBuffer) {
+      goto CommonBufferError;
+    }
+
+    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
+    Map->BufferAddress = InternalAllocateAlignedPages (EfiBootServicesData,
+                           EFI_SIZE_TO_PAGES (AllocSize),
+                           mCpu->DmaBufferAlignment);
+    if (Map->BufferAddress == NULL) {
+      Status = EFI_OUT_OF_RESOURCES;
+      goto FreeMapInfo;
+    }
+
+    if (Map->Operation == MapOperationBusMasterRead) {
+      CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
+        *NumberOfBytes);
+    }
+    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
+            EfiCpuFlushTypeWriteBack);
+
+    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
+  } else if (Operation != MapOperationBusMasterRead &&
       ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
        ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
 
@@ -128,12 +249,7 @@ DmaMap (
       // on uncached buffers.
       //
       if (Operation == MapOperationBusMasterCommonBuffer) {
-        DEBUG ((DEBUG_ERROR,
-          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
-          "supported\non memory regions that were allocated using "
-          "DmaAllocateBuffer ()\n", __FUNCTION__));
-        Status = EFI_UNSUPPORTED;
-        goto FreeMapInfo;
+        goto CommonBufferError;
       }
 
       //
@@ -199,6 +315,12 @@ DmaMap (
 
   return EFI_SUCCESS;
 
+CommonBufferError:
+  DEBUG ((DEBUG_ERROR,
+    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
+    "supported\non memory regions that were allocated using "
+    "DmaAllocateBuffer ()\n", __FUNCTION__));
+  Status = EFI_UNSUPPORTED;
 FreeMapInfo:
   FreePool (Map);
 
@@ -229,6 +351,7 @@ DmaUnmap (
   MAP_INFO_INSTANCE *Map;
   EFI_STATUS        Status;
   VOID              *Buffer;
+  UINTN             AllocSize;
 
   if (Mapping == NULL) {
     ASSERT (FALSE);
@@ -238,7 +361,17 @@ DmaUnmap (
   Map = (MAP_INFO_INSTANCE *)Mapping;
 
   Status = EFI_SUCCESS;
-  if (Map->DoubleBuffer) {
+  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
+    AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
+    if (Map->Operation == MapOperationBusMasterWrite) {
+      mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
+              EfiCpuFlushTypeInvalidate);
+      CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
+        Map->NumberOfBytes);
+    }
+    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
+  } else if (Map->DoubleBuffer) {
+
     ASSERT (Map->Operation == MapOperationBusMasterWrite);
 
     if (Map->Operation != MapOperationBusMasterWrite) {
@@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer (
     return EFI_INVALID_PARAMETER;
   }
 
-  if (MemoryType == EfiBootServicesData) {
-    Allocation = AllocateAlignedPages (Pages, Alignment);
-  } else if (MemoryType == EfiRuntimeServicesData) {
-    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
+  if (MemoryType == EfiBootServicesData ||
+      MemoryType == EfiRuntimeServicesData) {
+    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment);
   } else {
     return EFI_INVALID_PARAMETER;
   }
@@ -479,6 +611,15 @@ NonCoherentDmaLibConstructor (
 {
   InitializeListHead (&UncachedAllocationList);
 
+  //
+  // Ensure that the combination of DMA addressing offset and limit produces
+  // a sane value.
+  //
+  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
+
+  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
+                         PcdGet64 (PcdDmaDeviceOffset);
+
   // Get the Cpu protocol for later use
   return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
 }
diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
index 2db751afee91..1a21cfe4ff23 100644
--- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
+++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
@@ -38,6 +38,7 @@ [Protocols]
 
 [Pcd]
   gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
 
 [Depex]
   gEfiCpuArchProtocolGuid
-- 
2.17.1

Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Pete Batard]

On 2019.11.25 23:12, Ard Biesheuvel wrote:
> Implement support for driving peripherals with limited DMA ranges to
> NonCoherentDmaLib, by adding a device address limit, and taking it,
> along with the device offset, into account when allocating or mapping
> DMA buffers.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
>   3 files changed, 160 insertions(+), 12 deletions(-)

Tested-by: Pete Batard <pete@akeo.ie>

Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Philippe Mathieu-Daudé]

On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io wrote:
> Implement support for driving peripherals with limited DMA ranges to
> NonCoherentDmaLib, by adding a device address limit, and taking it,
> along with the device offset, into account when allocating or mapping
> DMA buffers.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
>   3 files changed, 160 insertions(+), 12 deletions(-)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 8812a6db7c30..69922802f473 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
>     #
>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
>   
> +  #
> +  # Highest address value supported by the device for DMA addressing. Note
> +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> +  #
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> +
>     #
>     # Selection between DT and ACPI as a default
>     #
> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> index 78220f6358aa..115345765435 100644
> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> @@ -40,6 +40,8 @@ typedef struct {
>   STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
>   STATIC LIST_ENTRY                 UncachedAllocationList;
>   
> +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> +
>   STATIC
>   PHYSICAL_ADDRESS
>   HostToDeviceAddress (
> @@ -49,6 +51,102 @@ HostToDeviceAddress (
>     return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
>   }
>   
> +/**
> +  Allocates one or more 4KB pages of a certain memory type at a specified
> +  alignment.
> +
> +  Allocates the number of 4KB pages specified by Pages of a certain memory type
> +  with an alignment specified by Alignment. The allocated buffer is returned.
> +  If Pages is 0, then NULL is returned. If there is not enough memory at the
> +  specified alignment remaining to satisfy the request, then NULL is returned.
> +  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> +
> +  @param  MemoryType            The type of memory to allocate.
> +  @param  Pages                 The number of 4 KB pages to allocate.
> +  @param  Alignment             The requested alignment of the allocation.
> +                                Must be a power of two.
> +                                If Alignment is zero, then byte alignment is
> +                                used.
> +
> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +STATIC
> +VOID *
> +InternalAllocateAlignedPages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages,
> +  IN UINTN            Alignment
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  UINTN                 AlignedMemory;
> +  UINTN                 AlignmentMask;
> +  UINTN                 UnalignedPages;
> +  UINTN                 RealPages;
> +
> +  //
> +  // Alignment must be a power of two or zero.
> +  //
> +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> +
> +  if (Pages == 0) {
> +    return NULL;
> +  }
> +  if (Alignment > EFI_PAGE_SIZE) {
> +    //
> +    // Calculate the total number of pages since alignment is larger than page
> +    // size.
> +    //
> +    AlignmentMask  = Alignment - 1;
> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> +    //
> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> +    // overflow.
> +    //
> +    ASSERT (RealPages > Pages);
> +
> +    Memory = mDmaHostAddressLimit;
> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, RealPages,
> +                    &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return NULL;
> +    }
> +    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free first unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
> +    UnalignedPages = RealPages - Pages - UnalignedPages;
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free last unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  } else {
> +    //
> +    // Do not over-allocate pages in this case.
> +    //
> +    Memory = mDmaHostAddressLimit;
> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
> +                    &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return NULL;
> +    }
> +    AlignedMemory = (UINTN)Memory;
> +  }
> +  return (VOID *)AlignedMemory;
> +}
> +
>   /**
>     Provides the DMA controller-specific addresses needed to access system memory.
>   
> @@ -111,7 +209,30 @@ DmaMap (
>       return  EFI_OUT_OF_RESOURCES;
>     }
>   
> -  if (Operation != MapOperationBusMasterRead &&
> +  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
> +
> +    if (Operation == MapOperationBusMasterCommonBuffer) {
> +      goto CommonBufferError;
> +    }
> +
> +    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
> +    Map->BufferAddress = InternalAllocateAlignedPages (EfiBootServicesData,
> +                           EFI_SIZE_TO_PAGES (AllocSize),
> +                           mCpu->DmaBufferAlignment);
> +    if (Map->BufferAddress == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
> +      goto FreeMapInfo;
> +    }
> +
> +    if (Map->Operation == MapOperationBusMasterRead) {
> +      CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
> +        *NumberOfBytes);
> +    }
> +    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> +            EfiCpuFlushTypeWriteBack);
> +
> +    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> +  } else if (Operation != MapOperationBusMasterRead &&
>         ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
>          ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>   
> @@ -128,12 +249,7 @@ DmaMap (
>         // on uncached buffers.
>         //
>         if (Operation == MapOperationBusMasterCommonBuffer) {
> -        DEBUG ((DEBUG_ERROR,
> -          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> -          "supported\non memory regions that were allocated using "
> -          "DmaAllocateBuffer ()\n", __FUNCTION__));
> -        Status = EFI_UNSUPPORTED;
> -        goto FreeMapInfo;
> +        goto CommonBufferError;
>         }
>   
>         //
> @@ -199,6 +315,12 @@ DmaMap (
>   
>     return EFI_SUCCESS;
>   
> +CommonBufferError:
> +  DEBUG ((DEBUG_ERROR,
> +    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> +    "supported\non memory regions that were allocated using "
> +    "DmaAllocateBuffer ()\n", __FUNCTION__));
> +  Status = EFI_UNSUPPORTED;
>   FreeMapInfo:
>     FreePool (Map);
>   
> @@ -229,6 +351,7 @@ DmaUnmap (
>     MAP_INFO_INSTANCE *Map;
>     EFI_STATUS        Status;
>     VOID              *Buffer;
> +  UINTN             AllocSize;
>   
>     if (Mapping == NULL) {
>       ASSERT (FALSE);
> @@ -238,7 +361,17 @@ DmaUnmap (
>     Map = (MAP_INFO_INSTANCE *)Mapping;
>   
>     Status = EFI_SUCCESS;
> -  if (Map->DoubleBuffer) {
> +  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
> +    AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
> +    if (Map->Operation == MapOperationBusMasterWrite) {
> +      mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> +              EfiCpuFlushTypeInvalidate);
> +      CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> +        Map->NumberOfBytes);
> +    }
> +    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> +  } else if (Map->DoubleBuffer) {
> +
>       ASSERT (Map->Operation == MapOperationBusMasterWrite);
>   
>       if (Map->Operation != MapOperationBusMasterWrite) {
> @@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer (
>       return EFI_INVALID_PARAMETER;
>     }
>   
> -  if (MemoryType == EfiBootServicesData) {
> -    Allocation = AllocateAlignedPages (Pages, Alignment);
> -  } else if (MemoryType == EfiRuntimeServicesData) {
> -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
> +  if (MemoryType == EfiBootServicesData ||
> +      MemoryType == EfiRuntimeServicesData) {
> +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment);
>     } else {
>       return EFI_INVALID_PARAMETER;
>     }
> @@ -479,6 +611,15 @@ NonCoherentDmaLibConstructor (
>   {
>     InitializeListHead (&UncachedAllocationList);
>   
> +  //
> +  // Ensure that the combination of DMA addressing offset and limit produces
> +  // a sane value.
> +  //
> +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> +
> +  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> +                         PcdGet64 (PcdDmaDeviceOffset);
> +
>     // Get the Cpu protocol for later use
>     return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
>   }
> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> index 2db751afee91..1a21cfe4ff23 100644
> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> @@ -38,6 +38,7 @@ [Protocols]
>   
>   [Pcd]
>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
>   
>   [Depex]
>     gEfiCpuArchProtocolGuid
> 

For the 82% of the patch I understood:
Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Philippe Mathieu-Daudé]

On 11/26/19 11:44 AM, Philippe Mathieu-Daudé wrote:
> On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io wrote:
>> Implement support for driving peripherals with limited DMA ranges to
>> NonCoherentDmaLib, by adding a device address limit, and taking it,
>> along with the device offset, into account when allocating or mapping
>> DMA buffers.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
>>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 
>> ++++++++++++++++++--
>>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
>>   3 files changed, 160 insertions(+), 12 deletions(-)
>>
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>> index 8812a6db7c30..69922802f473 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
>>     #
>>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
>> +  #
>> +  # Highest address value supported by the device for DMA addressing. 
>> Note
>> +  # that this value should be strictly greater than PcdDmaDeviceOffset.
>> +  #
>> +  
>> gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A 
>>
>> +
>>     #
>>     # Selection between DT and ACPI as a default
>>     #
>> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c 
>> b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
>> index 78220f6358aa..115345765435 100644
>> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
>> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
>> @@ -40,6 +40,8 @@ typedef struct {
>>   STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
>>   STATIC LIST_ENTRY                 UncachedAllocationList;
>> +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
>> +
>>   STATIC
>>   PHYSICAL_ADDRESS
>>   HostToDeviceAddress (
>> @@ -49,6 +51,102 @@ HostToDeviceAddress (
>>     return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 
>> (PcdDmaDeviceOffset);
>>   }
>> +/**
>> +  Allocates one or more 4KB pages of a certain memory type at a 
>> specified
>> +  alignment.
>> +
>> +  Allocates the number of 4KB pages specified by Pages of a certain 
>> memory type
>> +  with an alignment specified by Alignment. The allocated buffer is 
>> returned.
>> +  If Pages is 0, then NULL is returned. If there is not enough memory 
>> at the
>> +  specified alignment remaining to satisfy the request, then NULL is 
>> returned.
>> +  If Alignment is not a power of two and Alignment is not zero, then 
>> ASSERT().
>> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
>> +
>> +  @param  MemoryType            The type of memory to allocate.
>> +  @param  Pages                 The number of 4 KB pages to allocate.
>> +  @param  Alignment             The requested alignment of the 
>> allocation.
>> +                                Must be a power of two.
>> +                                If Alignment is zero, then byte 
>> alignment is
>> +                                used.
>> +
>> +  @return A pointer to the allocated buffer or NULL if allocation fails.
>> +
>> +**/
>> +STATIC
>> +VOID *
>> +InternalAllocateAlignedPages (
>> +  IN EFI_MEMORY_TYPE  MemoryType,
>> +  IN UINTN            Pages,
>> +  IN UINTN            Alignment
>> +  )
>> +{
>> +  EFI_STATUS            Status;
>> +  EFI_PHYSICAL_ADDRESS  Memory;
>> +  UINTN                 AlignedMemory;
>> +  UINTN                 AlignmentMask;
>> +  UINTN                 UnalignedPages;
>> +  UINTN                 RealPages;
>> +
>> +  //
>> +  // Alignment must be a power of two or zero.
>> +  //
>> +  ASSERT ((Alignment & (Alignment - 1)) == 0);
>> +
>> +  if (Pages == 0) {
>> +    return NULL;
>> +  }
>> +  if (Alignment > EFI_PAGE_SIZE) {
>> +    //
>> +    // Calculate the total number of pages since alignment is larger 
>> than page
>> +    // size.
>> +    //
>> +    AlignmentMask  = Alignment - 1;
>> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
>> +    //
>> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
>> +    // overflow.
>> +    //
>> +    ASSERT (RealPages > Pages);
>> +
>> +    Memory = mDmaHostAddressLimit;
>> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, 
>> RealPages,
>> +                    &Memory);
>> +    if (EFI_ERROR (Status)) {
>> +      return NULL;
>> +    }
>> +    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
>> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);

The previous line made me think twice (due to the cast), but I can't 
find a simpler way to write the same code.

>> +    if (UnalignedPages > 0) {
>> +      //
>> +      // Free first unaligned page(s).
>> +      //
>> +      Status = gBS->FreePages (Memory, UnalignedPages);
>> +      ASSERT_EFI_ERROR (Status);
>> +    }
>> +    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
>> +    UnalignedPages = RealPages - Pages - UnalignedPages;
>> +    if (UnalignedPages > 0) {
>> +      //
>> +      // Free last unaligned page(s).
>> +      //
>> +      Status = gBS->FreePages (Memory, UnalignedPages);
>> +      ASSERT_EFI_ERROR (Status);
>> +    }
>> +  } else {
>> +    //
>> +    // Do not over-allocate pages in this case.
>> +    //
>> +    Memory = mDmaHostAddressLimit;
>> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
>> +                    &Memory);
>> +    if (EFI_ERROR (Status)) {
>> +      return NULL;
>> +    }
>> +    AlignedMemory = (UINTN)Memory;
>> +  }
>> +  return (VOID *)AlignedMemory;
>> +}
>> +
>>   /**
>>     Provides the DMA controller-specific addresses needed to access 
>> system memory.
>> @@ -111,7 +209,30 @@ DmaMap (
>>       return  EFI_OUT_OF_RESOURCES;
>>     }
>> -  if (Operation != MapOperationBusMasterRead &&
>> +  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
>> +
>> +    if (Operation == MapOperationBusMasterCommonBuffer) {
>> +      goto CommonBufferError;
>> +    }
>> +
>> +    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
>> +    Map->BufferAddress = InternalAllocateAlignedPages 
>> (EfiBootServicesData,
>> +                           EFI_SIZE_TO_PAGES (AllocSize),
>> +                           mCpu->DmaBufferAlignment);
>> +    if (Map->BufferAddress == NULL) {
>> +      Status = EFI_OUT_OF_RESOURCES;
>> +      goto FreeMapInfo;
>> +    }
>> +
>> +    if (Map->Operation == MapOperationBusMasterRead) {
>> +      CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
>> +        *NumberOfBytes);
>> +    }
>> +    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
>> +            EfiCpuFlushTypeWriteBack);
>> +
>> +    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
>> +  } else if (Operation != MapOperationBusMasterRead &&
>>         ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
>>          ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>> @@ -128,12 +249,7 @@ DmaMap (
>>         // on uncached buffers.
>>         //
>>         if (Operation == MapOperationBusMasterCommonBuffer) {
>> -        DEBUG ((DEBUG_ERROR,
>> -          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is 
>> only "
>> -          "supported\non memory regions that were allocated using "
>> -          "DmaAllocateBuffer ()\n", __FUNCTION__));
>> -        Status = EFI_UNSUPPORTED;
>> -        goto FreeMapInfo;
>> +        goto CommonBufferError;
>>         }
>>         //
>> @@ -199,6 +315,12 @@ DmaMap (
>>     return EFI_SUCCESS;
>> +CommonBufferError:
>> +  DEBUG ((DEBUG_ERROR,
>> +    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
>> +    "supported\non memory regions that were allocated using "
>> +    "DmaAllocateBuffer ()\n", __FUNCTION__));
>> +  Status = EFI_UNSUPPORTED;
>>   FreeMapInfo:
>>     FreePool (Map);
>> @@ -229,6 +351,7 @@ DmaUnmap (
>>     MAP_INFO_INSTANCE *Map;
>>     EFI_STATUS        Status;
>>     VOID              *Buffer;
>> +  UINTN             AllocSize;
>>     if (Mapping == NULL) {
>>       ASSERT (FALSE);
>> @@ -238,7 +361,17 @@ DmaUnmap (
>>     Map = (MAP_INFO_INSTANCE *)Mapping;
>>     Status = EFI_SUCCESS;
>> -  if (Map->DoubleBuffer) {
>> +  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > 
>> mDmaHostAddressLimit) {
>> +    AllocSize = ALIGN_VALUE (Map->NumberOfBytes, 
>> mCpu->DmaBufferAlignment);
>> +    if (Map->Operation == MapOperationBusMasterWrite) {
>> +      mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
>> +              EfiCpuFlushTypeInvalidate);
>> +      CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
>> +        Map->NumberOfBytes);
>> +    }
>> +    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
>> +  } else if (Map->DoubleBuffer) {
>> +
>>       ASSERT (Map->Operation == MapOperationBusMasterWrite);
>>       if (Map->Operation != MapOperationBusMasterWrite) {
>> @@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer (
>>       return EFI_INVALID_PARAMETER;
>>     }
>> -  if (MemoryType == EfiBootServicesData) {
>> -    Allocation = AllocateAlignedPages (Pages, Alignment);
>> -  } else if (MemoryType == EfiRuntimeServicesData) {
>> -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
>> +  if (MemoryType == EfiBootServicesData ||
>> +      MemoryType == EfiRuntimeServicesData) {
>> +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, 
>> Alignment);
>>     } else {
>>       return EFI_INVALID_PARAMETER;
>>     }
>> @@ -479,6 +611,15 @@ NonCoherentDmaLibConstructor (
>>   {
>>     InitializeListHead (&UncachedAllocationList);
>> +  //
>> +  // Ensure that the combination of DMA addressing offset and limit 
>> produces
>> +  // a sane value.
>> +  //
>> +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
>> +
>> +  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
>> +                         PcdGet64 (PcdDmaDeviceOffset);
>> +
>>     // Get the Cpu protocol for later use
>>     return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID 
>> **)&mCpu);
>>   }
>> diff --git 
>> a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf 
>> b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
>> index 2db751afee91..1a21cfe4ff23 100644
>> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
>> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
>> @@ -38,6 +38,7 @@ [Protocols]
>>   [Pcd]
>>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
>> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
>>   [Depex]
>>     gEfiCpuArchProtocolGuid
>>
> 
> For the 82% of the patch I understood:
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Leif explained me that "I've reviewed it, but don't fully understand it, 
but I think it's valuable" translate into a Acked-by on EDK2, so please 
replace the previous Reviewed-by by:
Acked-by: Philippe Mathieu-Daude <philmd@redhat.com>

Thanks,

Phil.

Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Ard Biesheuvel]

On Tue, 26 Nov 2019 at 12:59, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> On 11/26/19 11:44 AM, Philippe Mathieu-Daudé wrote:
> > On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io wrote:
> >> Implement support for driving peripherals with limited DMA ranges to
> >> NonCoherentDmaLib, by adding a device address limit, and taking it,
> >> along with the device offset, into account when allocating or mapping
> >> DMA buffers.
> >>
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>   EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
> >>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165
> >> ++++++++++++++++++--
> >>   EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
> >>   3 files changed, 160 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> >> index 8812a6db7c30..69922802f473 100644
> >> --- a/EmbeddedPkg/EmbeddedPkg.dec
> >> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> >> @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
> >>     #
> >>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
> >> +  #
> >> +  # Highest address value supported by the device for DMA addressing.
> >> Note
> >> +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> >> +  #
> >> +
> >> gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> >>
> >> +
> >>     #
> >>     # Selection between DT and ACPI as a default
> >>     #
> >> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> >> b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> >> index 78220f6358aa..115345765435 100644
> >> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> >> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> >> @@ -40,6 +40,8 @@ typedef struct {
> >>   STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
> >>   STATIC LIST_ENTRY                 UncachedAllocationList;
> >> +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> >> +
> >>   STATIC
> >>   PHYSICAL_ADDRESS
> >>   HostToDeviceAddress (
> >> @@ -49,6 +51,102 @@ HostToDeviceAddress (
> >>     return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64
> >> (PcdDmaDeviceOffset);
> >>   }
> >> +/**
> >> +  Allocates one or more 4KB pages of a certain memory type at a
> >> specified
> >> +  alignment.
> >> +
> >> +  Allocates the number of 4KB pages specified by Pages of a certain
> >> memory type
> >> +  with an alignment specified by Alignment. The allocated buffer is
> >> returned.
> >> +  If Pages is 0, then NULL is returned. If there is not enough memory
> >> at the
> >> +  specified alignment remaining to satisfy the request, then NULL is
> >> returned.
> >> +  If Alignment is not a power of two and Alignment is not zero, then
> >> ASSERT().
> >> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> >> +
> >> +  @param  MemoryType            The type of memory to allocate.
> >> +  @param  Pages                 The number of 4 KB pages to allocate.
> >> +  @param  Alignment             The requested alignment of the
> >> allocation.
> >> +                                Must be a power of two.
> >> +                                If Alignment is zero, then byte
> >> alignment is
> >> +                                used.
> >> +
> >> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> >> +
> >> +**/
> >> +STATIC
> >> +VOID *
> >> +InternalAllocateAlignedPages (
> >> +  IN EFI_MEMORY_TYPE  MemoryType,
> >> +  IN UINTN            Pages,
> >> +  IN UINTN            Alignment
> >> +  )
> >> +{
> >> +  EFI_STATUS            Status;
> >> +  EFI_PHYSICAL_ADDRESS  Memory;
> >> +  UINTN                 AlignedMemory;
> >> +  UINTN                 AlignmentMask;
> >> +  UINTN                 UnalignedPages;
> >> +  UINTN                 RealPages;
> >> +
> >> +  //
> >> +  // Alignment must be a power of two or zero.
> >> +  //
> >> +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> >> +
> >> +  if (Pages == 0) {
> >> +    return NULL;
> >> +  }
> >> +  if (Alignment > EFI_PAGE_SIZE) {
> >> +    //
> >> +    // Calculate the total number of pages since alignment is larger
> >> than page
> >> +    // size.
> >> +    //
> >> +    AlignmentMask  = Alignment - 1;
> >> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> >> +    //
> >> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> >> +    // overflow.
> >> +    //
> >> +    ASSERT (RealPages > Pages);
> >> +
> >> +    Memory = mDmaHostAddressLimit;
> >> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType,
> >> RealPages,
> >> +                    &Memory);
> >> +    if (EFI_ERROR (Status)) {
> >> +      return NULL;
> >> +    }
> >> +    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
> >> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);
>
> The previous line made me think twice (due to the cast), but I can't
> find a simpler way to write the same code.
>
> >> +    if (UnalignedPages > 0) {
> >> +      //
> >> +      // Free first unaligned page(s).
> >> +      //
> >> +      Status = gBS->FreePages (Memory, UnalignedPages);
> >> +      ASSERT_EFI_ERROR (Status);
> >> +    }
> >> +    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
> >> +    UnalignedPages = RealPages - Pages - UnalignedPages;
> >> +    if (UnalignedPages > 0) {
> >> +      //
> >> +      // Free last unaligned page(s).
> >> +      //
> >> +      Status = gBS->FreePages (Memory, UnalignedPages);
> >> +      ASSERT_EFI_ERROR (Status);
> >> +    }
> >> +  } else {
> >> +    //
> >> +    // Do not over-allocate pages in this case.
> >> +    //
> >> +    Memory = mDmaHostAddressLimit;
> >> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
> >> +                    &Memory);
> >> +    if (EFI_ERROR (Status)) {
> >> +      return NULL;
> >> +    }
> >> +    AlignedMemory = (UINTN)Memory;
> >> +  }
> >> +  return (VOID *)AlignedMemory;
> >> +}
> >> +
> >>   /**
> >>     Provides the DMA controller-specific addresses needed to access
> >> system memory.
> >> @@ -111,7 +209,30 @@ DmaMap (
> >>       return  EFI_OUT_OF_RESOURCES;
> >>     }
> >> -  if (Operation != MapOperationBusMasterRead &&
> >> +  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
> >> +
> >> +    if (Operation == MapOperationBusMasterCommonBuffer) {
> >> +      goto CommonBufferError;
> >> +    }
> >> +
> >> +    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
> >> +    Map->BufferAddress = InternalAllocateAlignedPages
> >> (EfiBootServicesData,
> >> +                           EFI_SIZE_TO_PAGES (AllocSize),
> >> +                           mCpu->DmaBufferAlignment);
> >> +    if (Map->BufferAddress == NULL) {
> >> +      Status = EFI_OUT_OF_RESOURCES;
> >> +      goto FreeMapInfo;
> >> +    }
> >> +
> >> +    if (Map->Operation == MapOperationBusMasterRead) {
> >> +      CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
> >> +        *NumberOfBytes);
> >> +    }
> >> +    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> >> +            EfiCpuFlushTypeWriteBack);
> >> +
> >> +    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> >> +  } else if (Operation != MapOperationBusMasterRead &&
> >>         ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> >>          ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
> >> @@ -128,12 +249,7 @@ DmaMap (
> >>         // on uncached buffers.
> >>         //
> >>         if (Operation == MapOperationBusMasterCommonBuffer) {
> >> -        DEBUG ((DEBUG_ERROR,
> >> -          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is
> >> only "
> >> -          "supported\non memory regions that were allocated using "
> >> -          "DmaAllocateBuffer ()\n", __FUNCTION__));
> >> -        Status = EFI_UNSUPPORTED;
> >> -        goto FreeMapInfo;
> >> +        goto CommonBufferError;
> >>         }
> >>         //
> >> @@ -199,6 +315,12 @@ DmaMap (
> >>     return EFI_SUCCESS;
> >> +CommonBufferError:
> >> +  DEBUG ((DEBUG_ERROR,
> >> +    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> >> +    "supported\non memory regions that were allocated using "
> >> +    "DmaAllocateBuffer ()\n", __FUNCTION__));
> >> +  Status = EFI_UNSUPPORTED;
> >>   FreeMapInfo:
> >>     FreePool (Map);
> >> @@ -229,6 +351,7 @@ DmaUnmap (
> >>     MAP_INFO_INSTANCE *Map;
> >>     EFI_STATUS        Status;
> >>     VOID              *Buffer;
> >> +  UINTN             AllocSize;
> >>     if (Mapping == NULL) {
> >>       ASSERT (FALSE);
> >> @@ -238,7 +361,17 @@ DmaUnmap (
> >>     Map = (MAP_INFO_INSTANCE *)Mapping;
> >>     Status = EFI_SUCCESS;
> >> -  if (Map->DoubleBuffer) {
> >> +  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) >
> >> mDmaHostAddressLimit) {
> >> +    AllocSize = ALIGN_VALUE (Map->NumberOfBytes,
> >> mCpu->DmaBufferAlignment);
> >> +    if (Map->Operation == MapOperationBusMasterWrite) {
> >> +      mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> >> +              EfiCpuFlushTypeInvalidate);
> >> +      CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> >> +        Map->NumberOfBytes);
> >> +    }
> >> +    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> >> +  } else if (Map->DoubleBuffer) {
> >> +
> >>       ASSERT (Map->Operation == MapOperationBusMasterWrite);
> >>       if (Map->Operation != MapOperationBusMasterWrite) {
> >> @@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer (
> >>       return EFI_INVALID_PARAMETER;
> >>     }
> >> -  if (MemoryType == EfiBootServicesData) {
> >> -    Allocation = AllocateAlignedPages (Pages, Alignment);
> >> -  } else if (MemoryType == EfiRuntimeServicesData) {
> >> -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
> >> +  if (MemoryType == EfiBootServicesData ||
> >> +      MemoryType == EfiRuntimeServicesData) {
> >> +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages,
> >> Alignment);
> >>     } else {
> >>       return EFI_INVALID_PARAMETER;
> >>     }
> >> @@ -479,6 +611,15 @@ NonCoherentDmaLibConstructor (
> >>   {
> >>     InitializeListHead (&UncachedAllocationList);
> >> +  //
> >> +  // Ensure that the combination of DMA addressing offset and limit
> >> produces
> >> +  // a sane value.
> >> +  //
> >> +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> >> +
> >> +  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> >> +                         PcdGet64 (PcdDmaDeviceOffset);
> >> +
> >>     // Get the Cpu protocol for later use
> >>     return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID
> >> **)&mCpu);
> >>   }
> >> diff --git
> >> a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> >> b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> >> index 2db751afee91..1a21cfe4ff23 100644
> >> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> >> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> >> @@ -38,6 +38,7 @@ [Protocols]
> >>   [Pcd]
> >>     gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
> >> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
> >>   [Depex]
> >>     gEfiCpuArchProtocolGuid
> >>
> >
> > For the 82% of the patch I understood:
> > Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>
>
> Leif explained me that "I've reviewed it, but don't fully understand it,
> but I think it's valuable" translate into a Acked-by on EDK2, so please
> replace the previous Reviewed-by by:
> Acked-by: Philippe Mathieu-Daude <philmd@redhat.com>
>

Thanks!

Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Leif Lindholm]

On Tue, Nov 26, 2019 at 00:12:41 +0100, Ard Biesheuvel wrote:
> Implement support for driving peripherals with limited DMA ranges to
> NonCoherentDmaLib, by adding a device address limit, and taking it,
> along with the device offset, into account when allocating or mapping
> DMA buffers.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
>  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
>  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
>  3 files changed, 160 insertions(+), 12 deletions(-)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 8812a6db7c30..69922802f473 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
>    #
>    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
>  
> +  #
> +  # Highest address value supported by the device for DMA addressing. Note
> +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> +  #
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> +
>    #
>    # Selection between DT and ACPI as a default
>    #
> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> index 78220f6358aa..115345765435 100644
> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> @@ -40,6 +40,8 @@ typedef struct {
>  STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
>  STATIC LIST_ENTRY                 UncachedAllocationList;
>  
> +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> +
>  STATIC
>  PHYSICAL_ADDRESS
>  HostToDeviceAddress (
> @@ -49,6 +51,102 @@ HostToDeviceAddress (
>    return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
>  }
>  
> +/**
> +  Allocates one or more 4KB pages of a certain memory type at a specified
> +  alignment.
> +
> +  Allocates the number of 4KB pages specified by Pages of a certain memory type
> +  with an alignment specified by Alignment. The allocated buffer is returned.
> +  If Pages is 0, then NULL is returned. If there is not enough memory at the
> +  specified alignment remaining to satisfy the request, then NULL is returned.
> +  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
> +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> +
> +  @param  MemoryType            The type of memory to allocate.
> +  @param  Pages                 The number of 4 KB pages to allocate.
> +  @param  Alignment             The requested alignment of the allocation.
> +                                Must be a power of two.
> +                                If Alignment is zero, then byte alignment is
> +                                used.
> +
> +  @return A pointer to the allocated buffer or NULL if allocation fails.
> +
> +**/
> +STATIC
> +VOID *
> +InternalAllocateAlignedPages (
> +  IN EFI_MEMORY_TYPE  MemoryType,
> +  IN UINTN            Pages,
> +  IN UINTN            Alignment
> +  )
> +{
> +  EFI_STATUS            Status;
> +  EFI_PHYSICAL_ADDRESS  Memory;
> +  UINTN                 AlignedMemory;
> +  UINTN                 AlignmentMask;
> +  UINTN                 UnalignedPages;
> +  UINTN                 RealPages;
> +
> +  //
> +  // Alignment must be a power of two or zero.
> +  //
> +  ASSERT ((Alignment & (Alignment - 1)) == 0);

Sorry, slight mental glitch (and I realise this is in  copied code) -
the above also matches for an Alignment of 1, which contradicts the
comment. Clearly requesting allocation explicitly aligned to 1 is a)
silly, b) the same as what happens for any value <= EFI_PAGE_SIZE
in the below code, and c) harmless, but could you update the comment?

If so:
Acked-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> +
> +  if (Pages == 0) {
> +    return NULL;
> +  }
> +  if (Alignment > EFI_PAGE_SIZE) {
> +    //
> +    // Calculate the total number of pages since alignment is larger than page
> +    // size.
> +    //
> +    AlignmentMask  = Alignment - 1;
> +    RealPages      = Pages + EFI_SIZE_TO_PAGES (Alignment);
> +    //
> +    // Make sure that Pages plus EFI_SIZE_TO_PAGES (Alignment) does not
> +    // overflow.
> +    //
> +    ASSERT (RealPages > Pages);
> +
> +    Memory = mDmaHostAddressLimit;
> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, RealPages,
> +                    &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return NULL;
> +    }
> +    AlignedMemory  = ((UINTN)Memory + AlignmentMask) & ~AlignmentMask;
> +    UnalignedPages = EFI_SIZE_TO_PAGES (AlignedMemory - (UINTN)Memory);
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free first unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +    Memory         = AlignedMemory + EFI_PAGES_TO_SIZE (Pages);
> +    UnalignedPages = RealPages - Pages - UnalignedPages;
> +    if (UnalignedPages > 0) {
> +      //
> +      // Free last unaligned page(s).
> +      //
> +      Status = gBS->FreePages (Memory, UnalignedPages);
> +      ASSERT_EFI_ERROR (Status);
> +    }
> +  } else {
> +    //
> +    // Do not over-allocate pages in this case.
> +    //
> +    Memory = mDmaHostAddressLimit;
> +    Status = gBS->AllocatePages (AllocateMaxAddress, MemoryType, Pages,
> +                    &Memory);
> +    if (EFI_ERROR (Status)) {
> +      return NULL;
> +    }
> +    AlignedMemory = (UINTN)Memory;
> +  }
> +  return (VOID *)AlignedMemory;
> +}
> +
>  /**
>    Provides the DMA controller-specific addresses needed to access system memory.
>  
> @@ -111,7 +209,30 @@ DmaMap (
>      return  EFI_OUT_OF_RESOURCES;
>    }
>  
> -  if (Operation != MapOperationBusMasterRead &&
> +  if (((UINTN)HostAddress + *NumberOfBytes) > mDmaHostAddressLimit) {
> +
> +    if (Operation == MapOperationBusMasterCommonBuffer) {
> +      goto CommonBufferError;
> +    }
> +
> +    AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment);
> +    Map->BufferAddress = InternalAllocateAlignedPages (EfiBootServicesData,
> +                           EFI_SIZE_TO_PAGES (AllocSize),
> +                           mCpu->DmaBufferAlignment);
> +    if (Map->BufferAddress == NULL) {
> +      Status = EFI_OUT_OF_RESOURCES;
> +      goto FreeMapInfo;
> +    }
> +
> +    if (Map->Operation == MapOperationBusMasterRead) {
> +      CopyMem (Map->BufferAddress, (VOID *)(UINTN)Map->HostAddress,
> +        *NumberOfBytes);
> +    }
> +    mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> +            EfiCpuFlushTypeWriteBack);
> +
> +    *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> +  } else if (Operation != MapOperationBusMasterRead &&
>        ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
>         ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>  
> @@ -128,12 +249,7 @@ DmaMap (
>        // on uncached buffers.
>        //
>        if (Operation == MapOperationBusMasterCommonBuffer) {
> -        DEBUG ((DEBUG_ERROR,
> -          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> -          "supported\non memory regions that were allocated using "
> -          "DmaAllocateBuffer ()\n", __FUNCTION__));
> -        Status = EFI_UNSUPPORTED;
> -        goto FreeMapInfo;
> +        goto CommonBufferError;
>        }
>  
>        //
> @@ -199,6 +315,12 @@ DmaMap (
>  
>    return EFI_SUCCESS;
>  
> +CommonBufferError:
> +  DEBUG ((DEBUG_ERROR,
> +    "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only "
> +    "supported\non memory regions that were allocated using "
> +    "DmaAllocateBuffer ()\n", __FUNCTION__));
> +  Status = EFI_UNSUPPORTED;
>  FreeMapInfo:
>    FreePool (Map);
>  
> @@ -229,6 +351,7 @@ DmaUnmap (
>    MAP_INFO_INSTANCE *Map;
>    EFI_STATUS        Status;
>    VOID              *Buffer;
> +  UINTN             AllocSize;
>  
>    if (Mapping == NULL) {
>      ASSERT (FALSE);
> @@ -238,7 +361,17 @@ DmaUnmap (
>    Map = (MAP_INFO_INSTANCE *)Mapping;
>  
>    Status = EFI_SUCCESS;
> -  if (Map->DoubleBuffer) {
> +  if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
> +    AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
> +    if (Map->Operation == MapOperationBusMasterWrite) {
> +      mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, AllocSize,
> +              EfiCpuFlushTypeInvalidate);
> +      CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> +        Map->NumberOfBytes);
> +    }
> +    FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> +  } else if (Map->DoubleBuffer) {
> +
>      ASSERT (Map->Operation == MapOperationBusMasterWrite);
>  
>      if (Map->Operation != MapOperationBusMasterWrite) {
> @@ -335,10 +468,9 @@ DmaAllocateAlignedBuffer (
>      return EFI_INVALID_PARAMETER;
>    }
>  
> -  if (MemoryType == EfiBootServicesData) {
> -    Allocation = AllocateAlignedPages (Pages, Alignment);
> -  } else if (MemoryType == EfiRuntimeServicesData) {
> -    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
> +  if (MemoryType == EfiBootServicesData ||
> +      MemoryType == EfiRuntimeServicesData) {
> +    Allocation = InternalAllocateAlignedPages (MemoryType, Pages, Alignment);
>    } else {
>      return EFI_INVALID_PARAMETER;
>    }
> @@ -479,6 +611,15 @@ NonCoherentDmaLibConstructor (
>  {
>    InitializeListHead (&UncachedAllocationList);
>  
> +  //
> +  // Ensure that the combination of DMA addressing offset and limit produces
> +  // a sane value.
> +  //
> +  ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> +
> +  mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> +                         PcdGet64 (PcdDmaDeviceOffset);
> +
>    // Get the Cpu protocol for later use
>    return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
>  }
> diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> index 2db751afee91..1a21cfe4ff23 100644
> --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> @@ -38,6 +38,7 @@ [Protocols]
>  
>  [Pcd]
>    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit
>  
>  [Depex]
>    gEfiCpuArchProtocolGuid
> -- 
> 2.17.1
> 

Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Ard Biesheuvel]

On Tue, 26 Nov 2019 at 13:04, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Tue, Nov 26, 2019 at 00:12:41 +0100, Ard Biesheuvel wrote:
> > Implement support for driving peripherals with limited DMA ranges to
> > NonCoherentDmaLib, by adding a device address limit, and taking it,
> > along with the device offset, into account when allocating or mapping
> > DMA buffers.
> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
> >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
> >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
> >  3 files changed, 160 insertions(+), 12 deletions(-)
> >
> > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> > index 8812a6db7c30..69922802f473 100644
> > --- a/EmbeddedPkg/EmbeddedPkg.dec
> > +++ b/EmbeddedPkg/EmbeddedPkg.dec
> > @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
> >    #
> >    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
> >
> > +  #
> > +  # Highest address value supported by the device for DMA addressing. Note
> > +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> > +  #
> > +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> > +
> >    #
> >    # Selection between DT and ACPI as a default
> >    #
> > diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > index 78220f6358aa..115345765435 100644
> > --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > @@ -40,6 +40,8 @@ typedef struct {
> >  STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
> >  STATIC LIST_ENTRY                 UncachedAllocationList;
> >
> > +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> > +
> >  STATIC
> >  PHYSICAL_ADDRESS
> >  HostToDeviceAddress (
> > @@ -49,6 +51,102 @@ HostToDeviceAddress (
> >    return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
> >  }
> >
> > +/**
> > +  Allocates one or more 4KB pages of a certain memory type at a specified
> > +  alignment.
> > +
> > +  Allocates the number of 4KB pages specified by Pages of a certain memory type
> > +  with an alignment specified by Alignment. The allocated buffer is returned.
> > +  If Pages is 0, then NULL is returned. If there is not enough memory at the
> > +  specified alignment remaining to satisfy the request, then NULL is returned.
> > +  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
> > +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> > +
> > +  @param  MemoryType            The type of memory to allocate.
> > +  @param  Pages                 The number of 4 KB pages to allocate.
> > +  @param  Alignment             The requested alignment of the allocation.
> > +                                Must be a power of two.
> > +                                If Alignment is zero, then byte alignment is
> > +                                used.
> > +
> > +  @return A pointer to the allocated buffer or NULL if allocation fails.
> > +
> > +**/
> > +STATIC
> > +VOID *
> > +InternalAllocateAlignedPages (
> > +  IN EFI_MEMORY_TYPE  MemoryType,
> > +  IN UINTN            Pages,
> > +  IN UINTN            Alignment
> > +  )
> > +{
> > +  EFI_STATUS            Status;
> > +  EFI_PHYSICAL_ADDRESS  Memory;
> > +  UINTN                 AlignedMemory;
> > +  UINTN                 AlignmentMask;
> > +  UINTN                 UnalignedPages;
> > +  UINTN                 RealPages;
> > +
> > +  //
> > +  // Alignment must be a power of two or zero.
> > +  //
> > +  ASSERT ((Alignment & (Alignment - 1)) == 0);
>
> Sorry, slight mental glitch (and I realise this is in  copied code) -
> the above also matches for an Alignment of 1, which contradicts the
> comment. Clearly requesting allocation explicitly aligned to 1 is a)
> silly, b) the same as what happens for any value <= EFI_PAGE_SIZE
> in the below code, and c) harmless, but could you update the comment?
>
> If so:
> Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Given that 1 == 2^0, it is also a power of 2, and so the comment is
accurate imho

Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits

[Leif Lindholm]

On Tue, Nov 26, 2019 at 13:11:34 +0100, Ard Biesheuvel wrote:
> On Tue, 26 Nov 2019 at 13:04, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Tue, Nov 26, 2019 at 00:12:41 +0100, Ard Biesheuvel wrote:
> > > Implement support for driving peripherals with limited DMA ranges to
> > > NonCoherentDmaLib, by adding a device address limit, and taking it,
> > > along with the device offset, into account when allocating or mapping
> > > DMA buffers.
> > >
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > >  EmbeddedPkg/EmbeddedPkg.dec                                 |   6 +
> > >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 165 ++++++++++++++++++--
> > >  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |   1 +
> > >  3 files changed, 160 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> > > index 8812a6db7c30..69922802f473 100644
> > > --- a/EmbeddedPkg/EmbeddedPkg.dec
> > > +++ b/EmbeddedPkg/EmbeddedPkg.dec
> > > @@ -186,6 +186,12 @@ [PcdsFixedAtBuild.common, PcdsDynamic.common]
> > >    #
> > >    gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
> > >
> > > +  #
> > > +  # Highest address value supported by the device for DMA addressing. Note
> > > +  # that this value should be strictly greater than PcdDmaDeviceOffset.
> > > +  #
> > > +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceLimit|0xFFFFFFFFFFFFFFFF|UINT64|0x000005A
> > > +
> > >    #
> > >    # Selection between DT and ACPI as a default
> > >    #
> > > diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > > index 78220f6358aa..115345765435 100644
> > > --- a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > > +++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
> > > @@ -40,6 +40,8 @@ typedef struct {
> > >  STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
> > >  STATIC LIST_ENTRY                 UncachedAllocationList;
> > >
> > > +STATIC PHYSICAL_ADDRESS           mDmaHostAddressLimit;
> > > +
> > >  STATIC
> > >  PHYSICAL_ADDRESS
> > >  HostToDeviceAddress (
> > > @@ -49,6 +51,102 @@ HostToDeviceAddress (
> > >    return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
> > >  }
> > >
> > > +/**
> > > +  Allocates one or more 4KB pages of a certain memory type at a specified
> > > +  alignment.
> > > +
> > > +  Allocates the number of 4KB pages specified by Pages of a certain memory type
> > > +  with an alignment specified by Alignment. The allocated buffer is returned.
> > > +  If Pages is 0, then NULL is returned. If there is not enough memory at the
> > > +  specified alignment remaining to satisfy the request, then NULL is returned.
> > > +  If Alignment is not a power of two and Alignment is not zero, then ASSERT().
> > > +  If Pages plus EFI_SIZE_TO_PAGES (Alignment) overflows, then ASSERT().
> > > +
> > > +  @param  MemoryType            The type of memory to allocate.
> > > +  @param  Pages                 The number of 4 KB pages to allocate.
> > > +  @param  Alignment             The requested alignment of the allocation.
> > > +                                Must be a power of two.
> > > +                                If Alignment is zero, then byte alignment is
> > > +                                used.
> > > +
> > > +  @return A pointer to the allocated buffer or NULL if allocation fails.
> > > +
> > > +**/
> > > +STATIC
> > > +VOID *
> > > +InternalAllocateAlignedPages (
> > > +  IN EFI_MEMORY_TYPE  MemoryType,
> > > +  IN UINTN            Pages,
> > > +  IN UINTN            Alignment
> > > +  )
> > > +{
> > > +  EFI_STATUS            Status;
> > > +  EFI_PHYSICAL_ADDRESS  Memory;
> > > +  UINTN                 AlignedMemory;
> > > +  UINTN                 AlignmentMask;
> > > +  UINTN                 UnalignedPages;
> > > +  UINTN                 RealPages;
> > > +
> > > +  //
> > > +  // Alignment must be a power of two or zero.
> > > +  //
> > > +  ASSERT ((Alignment & (Alignment - 1)) == 0);
> >
> > Sorry, slight mental glitch (and I realise this is in  copied code) -
> > the above also matches for an Alignment of 1, which contradicts the
> > comment. Clearly requesting allocation explicitly aligned to 1 is a)
> > silly, b) the same as what happens for any value <= EFI_PAGE_SIZE
> > in the below code, and c) harmless, but could you update the comment?
> >
> > If so:
> > Acked-by: Leif Lindholm <leif.lindholm@linaro.org>
> 
> Given that 1 == 2^0, it is also a power of 2, and so the comment is
> accurate imho

I ... guess that's true.

Sure.

/
    Leif

[PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib

[Ard Biesheuvel]

Implement a version of the EDK2 IoMmu protocol that is a simple wrapper
around DmaLib. This is intended to be used to wrap NonCoherentDmaLib so
that the generic PCI infrastructure can be used to implement support for
non cache-coherent DMA.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c   | 257 ++++++++++++++++++++
 EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf |  43 ++++
 EmbeddedPkg/EmbeddedPkg.dsc                                     |   5 +
 3 files changed, 305 insertions(+)

diff --git a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
new file mode 100644
index 000000000000..4b0afe47de4c
--- /dev/null
+++ b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c
@@ -0,0 +1,257 @@
+/** @file
+
+  Copyright (c) 2019, Linaro, Ltd. All rights reserved.<BR>
+
+  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 <PiDxe.h>
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
+#include <Library/DmaLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Protocol/IoMmu.h>
+
+/**
+  Set IOMMU attribute for a system memory.
+
+  If the IOMMU protocol exists, the system memory cannot be used
+  for DMA by default.
+
+  When a device requests a DMA access for a system memory,
+  the device driver need use SetAttribute() to update the IOMMU
+  attribute to request DMA access (read and/or write).
+
+  The DeviceHandle is used to identify which device submits the request.
+  The IOMMU implementation need translate the device path to an IOMMU device
+  ID, and set IOMMU hardware register accordingly.
+  1) DeviceHandle can be a standard PCI device.
+     The memory for BusMasterRead need set EDKII_IOMMU_ACCESS_READ.
+     The memory for BusMasterWrite need set EDKII_IOMMU_ACCESS_WRITE.
+     The memory for BusMasterCommonBuffer need set
+     EDKII_IOMMU_ACCESS_READ|EDKII_IOMMU_ACCESS_WRITE.
+     After the memory is used, the memory need set 0 to keep it being
+     protected.
+  2) DeviceHandle can be an ACPI device (ISA, I2C, SPI, etc).
+     The memory for DMA access need set EDKII_IOMMU_ACCESS_READ and/or
+     EDKII_IOMMU_ACCESS_WRITE.
+
+  @param[in]  This              The protocol instance pointer.
+  @param[in]  DeviceHandle      The device who initiates the DMA access
+                                request.
+  @param[in]  Mapping           The mapping value returned from Map().
+  @param[in]  IoMmuAccess       The IOMMU access.
+
+  @retval EFI_SUCCESS            The IoMmuAccess is set for the memory range
+                                 specified by DeviceAddress and Length.
+  @retval EFI_INVALID_PARAMETER  DeviceHandle is an invalid handle.
+  @retval EFI_INVALID_PARAMETER  Mapping is not a value that was returned by
+                                 Map().
+  @retval EFI_INVALID_PARAMETER  IoMmuAccess specified an illegal combination
+                                 of access.
+  @retval EFI_UNSUPPORTED        DeviceHandle is unknown by the IOMMU.
+  @retval EFI_UNSUPPORTED        The bit mask of IoMmuAccess is not supported
+                                 by the IOMMU.
+  @retval EFI_UNSUPPORTED        The IOMMU does not support the memory range
+                                 specified by Mapping.
+  @retval EFI_OUT_OF_RESOURCES   There are not enough resources available to
+                                 modify the IOMMU access.
+  @retval EFI_DEVICE_ERROR       The IOMMU device reported an error while
+                                 attempting the operation.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuSetAttribute (
+  IN EDKII_IOMMU_PROTOCOL  *This,
+  IN EFI_HANDLE            DeviceHandle,
+  IN VOID                  *Mapping,
+  IN UINT64                IoMmuAccess
+  )
+{
+  return EFI_UNSUPPORTED;
+}
+
+/**
+  Provides the controller-specific addresses required to access system memory
+  from a DMA bus master. On SEV guest, the DMA operations must be performed on
+  shared buffer hence we allocate a bounce buffer to map the HostAddress to a
+  DeviceAddress. The Encryption attribute is removed from the DeviceAddress
+  buffer.
+
+  @param  This                  The protocol instance pointer.
+  @param  Operation             Indicates if the bus master is going to read or
+                                write to system memory.
+  @param  HostAddress           The system memory address to map to the PCI
+                                controller.
+  @param  NumberOfBytes         On input the number of bytes to map. On output
+                                the number of bytes that were mapped.
+  @param  DeviceAddress         The resulting map address for the bus master
+                                PCI controller to use to access the hosts
+                                HostAddress.
+  @param  Mapping               A resulting value to pass to Unmap().
+
+  @retval EFI_SUCCESS           The range was mapped for the returned
+                                NumberOfBytes.
+  @retval EFI_UNSUPPORTED       The HostAddress cannot be mapped as a common
+                                buffer.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The request could not be completed due to a
+                                lack of resources.
+  @retval EFI_DEVICE_ERROR      The system hardware could not map the requested
+                                address.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuMap (
+  IN     EDKII_IOMMU_PROTOCOL                       *This,
+  IN     EDKII_IOMMU_OPERATION                      Operation,
+  IN     VOID                                       *HostAddress,
+  IN OUT UINTN                                      *NumberOfBytes,
+  OUT    EFI_PHYSICAL_ADDRESS                       *DeviceAddress,
+  OUT    VOID                                       **Mapping
+  )
+{
+  DMA_MAP_OPERATION     DmaOperation;
+
+  switch (Operation) {
+    case EdkiiIoMmuOperationBusMasterRead:
+    case EdkiiIoMmuOperationBusMasterRead64:
+      DmaOperation = MapOperationBusMasterRead;
+      break;
+
+    case EdkiiIoMmuOperationBusMasterWrite:
+    case EdkiiIoMmuOperationBusMasterWrite64:
+      DmaOperation = MapOperationBusMasterWrite;
+      break;
+
+    case EdkiiIoMmuOperationBusMasterCommonBuffer:
+    case EdkiiIoMmuOperationBusMasterCommonBuffer64:
+      DmaOperation = MapOperationBusMasterCommonBuffer;
+      break;
+
+    default:
+      ASSERT (FALSE);
+      return EFI_INVALID_PARAMETER;
+  }
+
+  return DmaMap (DmaOperation, HostAddress, NumberOfBytes,
+           DeviceAddress, Mapping);
+}
+
+/**
+  Completes the Map() operation and releases any corresponding resources.
+
+  @param  This                  The protocol instance pointer.
+  @param  Mapping               The mapping value returned from Map().
+
+  @retval EFI_SUCCESS           The range was unmapped.
+  @retval EFI_INVALID_PARAMETER Mapping is not a value that was returned by
+                                Map().
+  @retval EFI_DEVICE_ERROR      The data was not committed to the target system
+                                memory.
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuUnmap (
+  IN  EDKII_IOMMU_PROTOCOL                     *This,
+  IN  VOID                                     *Mapping
+  )
+{
+  return DmaUnmap (Mapping);
+}
+
+/**
+  Allocates pages that are suitable for an OperationBusMasterCommonBuffer or
+  OperationBusMasterCommonBuffer64 mapping.
+
+  @param  This                  The protocol instance pointer.
+  @param  Type                  This parameter is not used and must be ignored.
+  @param  MemoryType            The type of memory to allocate,
+                                EfiBootServicesData or EfiRuntimeServicesData.
+  @param  Pages                 The number of pages to allocate.
+  @param  HostAddress           A pointer to store the base system memory
+                                address of the allocated range.
+  @param  Attributes            The requested bit mask of attributes for the
+                                allocated range.
+
+  @retval EFI_SUCCESS           The requested memory pages were allocated.
+  @retval EFI_UNSUPPORTED       Attributes is unsupported. The only legal
+                                attribute bits are MEMORY_WRITE_COMBINE and
+                                MEMORY_CACHED.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuAllocateBuffer (
+  IN     EDKII_IOMMU_PROTOCOL                     *This,
+  IN     EFI_ALLOCATE_TYPE                        Type,
+  IN     EFI_MEMORY_TYPE                          MemoryType,
+  IN     UINTN                                    Pages,
+  IN OUT VOID                                     **HostAddress,
+  IN     UINT64                                   Attributes
+  )
+{
+  return DmaAllocateBuffer (MemoryType, Pages, HostAddress);
+}
+
+/**
+  Frees memory that was allocated with AllocateBuffer().
+
+  @param  This                  The protocol instance pointer.
+  @param  Pages                 The number of pages to free.
+  @param  HostAddress           The base system memory address of the allocated
+                                range.
+
+  @retval EFI_SUCCESS           The requested memory pages were freed.
+  @retval EFI_INVALID_PARAMETER The memory range specified by HostAddress and
+                                Pages was not allocated with AllocateBuffer().
+
+**/
+STATIC
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuFreeBuffer (
+  IN  EDKII_IOMMU_PROTOCOL                     *This,
+  IN  UINTN                                    Pages,
+  IN  VOID                                     *HostAddress
+  )
+{
+  return DmaFreeBuffer (Pages, HostAddress);
+}
+
+STATIC EDKII_IOMMU_PROTOCOL  mNonCoherentIoMmuOps = {
+  EDKII_IOMMU_PROTOCOL_REVISION,
+  NonCoherentIoMmuSetAttribute,
+  NonCoherentIoMmuMap,
+  NonCoherentIoMmuUnmap,
+  NonCoherentIoMmuAllocateBuffer,
+  NonCoherentIoMmuFreeBuffer,
+};
+
+
+EFI_STATUS
+EFIAPI
+NonCoherentIoMmuDxeEntryPoint (
+  IN EFI_HANDLE         ImageHandle,
+  IN EFI_SYSTEM_TABLE   *SystemTable
+  )
+{
+  return gBS->InstallMultipleProtocolInterfaces (&ImageHandle,
+                &gEdkiiIoMmuProtocolGuid, &mNonCoherentIoMmuOps,
+                NULL);
+}
diff --git a/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf
new file mode 100644
index 000000000000..de70cfb4cad7
--- /dev/null
+++ b/EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf
@@ -0,0 +1,43 @@
+#/** @file
+#
+#  Copyright (c) 2019, Linaro, Ltd. All rights reserved.<BR>
+#
+#  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.
+#
+#**/
+
+[Defines]
+  INF_VERSION                    = 1.27
+  BASE_NAME                      = NonCoherentIoMmuDxe
+  FILE_GUID                      = 7ed510aa-9cdc-49d2-a306-6e11e359f9b3
+  MODULE_TYPE                    = DXE_DRIVER
+  VERSION_STRING                 = 1.0
+  ENTRY_POINT                    = NonCoherentIoMmuDxeEntryPoint
+
+[Sources]
+  NonCoherentIoMmuDxe.c
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdeModulePkg/MdeModulePkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  DmaLib
+  UefiBootServicesTableLib
+  UefiDriverEntryPoint
+
+[Protocols]
+  gEdkiiIoMmuProtocolGuid                     ## PRODUCES
+
+[Depex]
+  TRUE
diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index a8a151eb40cb..8842acc4cbf4 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -238,6 +238,11 @@ [Components.common]
   EmbeddedPkg/Drivers/ConsolePrefDxe/ConsolePrefDxe.inf
   EmbeddedPkg/Drivers/DtPlatformDxe/DtPlatformDxe.inf
 
+  EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf {
+    <LibraryClasses>
+      DmaLib|EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
+  }
+
 [Components.ARM]
   EmbeddedPkg/Drivers/Isp1761UsbDxe/Isp1761UsbDxe.inf
 
-- 
2.17.1

Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib

[Philippe Mathieu-Daudé]

On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io wrote:
> Implement a version of the EDK2 IoMmu protocol that is a simple wrapper
> around DmaLib. This is intended to be used to wrap NonCoherentDmaLib so
> that the generic PCI infrastructure can be used to implement support for
> non cache-coherent DMA.
> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>   EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c   | 257 ++++++++++++++++++++
>   EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf |  43 ++++
>   EmbeddedPkg/EmbeddedPkg.dsc                                     |   5 +
>   3 files changed, 305 insertions(+)

Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib

[Pete Batard]

On 2019.11.26 08:19, Philippe Mathieu-Daudé wrote:
> On 11/26/19 12:12 AM, Ard Biesheuvel via Groups.Io wrote:
>> Implement a version of the EDK2 IoMmu protocol that is a simple wrapper
>> around DmaLib. This is intended to be used to wrap NonCoherentDmaLib so
>> that the generic PCI infrastructure can be used to implement support for
>> non cache-coherent DMA.
>>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>> ---
>>   EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.c   | 
>> 257 ++++++++++++++++++++
>>   EmbeddedPkg/Drivers/NonCoherentIoMmuDxe/NonCoherentIoMmuDxe.inf |  
>> 43 ++++
>>   EmbeddedPkg/EmbeddedPkg.dsc                                     |   5 +
>>   3 files changed, 305 insertions(+)
> 
> Reviewed-by: Philippe Mathieu-Daude <philmd@redhat.com>

Tested-by: Pete Batard <pete@akeo.ie>