[PATCH 0/6] ArmPkg EmbeddedPkg: clean up DmaLib implementations

[PATCH 0/6] ArmPkg EmbeddedPkg: clean up DmaLib implementations

[Ard Biesheuvel]

Currently, we have two DmaLib implementations: a cache coherent one called
'NullDmaLib' residing in EmbeddedPkg, and a non-cache coherent one called
'ArmDmaLib', residinh in ArmPkg.

In both cases, this is slightly awkward: NullDmaLib suggests no functionality
whatsoever, which is slightly misleading because 'nothing' is the correct
action in case of cache coherent DMA, rather than a lack of action. As for
ArmDmalib, this was never specific to ARM, and no longer depends on anything
that ArmPkg provides, so it does not really belong in ArmPkg anymore.

So let's rename them to CoherentDmaLib and NonCoherentDmaLib, respectively,
and move that latter into EmbeddedPkg where it arguably belongs. To align
the two further, add support for non-1:1 DMA mappings to CoherentDmaLib as
well.

Note that the final patch can only be merged after out-of-tree platforms
have switched from ArmDmaLib to NonCoherentDmaLib.

Ard Biesheuvel (6):
  EmbeddedPkg: rename NullDmaLib to CoherentDmaLib
  EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation
  EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib
  Omap35xxPkg: switch to EmbeddedPkg's NonCoherentDmaLib
  BeagleBoardPkg: switch to generic non-coherent DmaLib
  ArmPkg: remove ArmDmaLib

 ArmPkg/ArmPkg.dsc                                                                                 |   2 -
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf                                                            |  49 ---------
 BeagleBoardPkg/BeagleBoardPkg.dsc                                                                 |   2 +-
 EmbeddedPkg/EmbeddedPkg.dec                                                                       |   7 ++
 EmbeddedPkg/EmbeddedPkg.dsc                                                                       |   3 +-
 EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c}                  |  10 +-
 EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf}              |  19 ++--
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c => EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c | 105 +++++++++++---------
 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf                                       |  50 ++++++++++
 Omap35xxPkg/Omap35xxPkg.dsc                                                                       |   2 +-
 10 files changed, 136 insertions(+), 113 deletions(-)
 delete mode 100644 ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
 rename EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c} (94%)
 rename EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf} (75%)
 rename ArmPkg/Library/ArmDmaLib/ArmDmaLib.c => EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c (82%)
 create mode 100644 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf

-- 
2.11.0

[PATCH 1/6] EmbeddedPkg: rename NullDmaLib to CoherentDmaLib

[Ard Biesheuvel]

Rename NullDmaLib to CoherentDmaLib to better reflect its nature, and
to better convey the fact that it actually serves a useful purpose,
i.e., as a DmaLib library class resolution for drivers that control
hardware that may only be cache coherent or in some cases (i.e., on
some platforms but not on others).

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dsc                                                          |  2 +-
 EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c}     |  0
 EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf} | 20 ++++++--------------
 3 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index 4a34e34843ad..84c5a842e37e 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -250,7 +250,7 @@ [Components.common]
   EmbeddedPkg/Library/TemplateResetSystemLib/TemplateResetSystemLib.inf
   EmbeddedPkg/Library/TemplateRealTimeClockLib/TemplateRealTimeClockLib.inf
   EmbeddedPkg/Library/LzmaHobCustomDecompressLib/LzmaHobCustomDecompressLib.inf
-  EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
+  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
   EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
 
   EmbeddedPkg/Ebl/Ebl.inf
diff --git a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.c b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
similarity index 100%
rename from EmbeddedPkg/Library/NullDmaLib/NullDmaLib.c
rename to EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
diff --git a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
similarity index 78%
rename from EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
rename to EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
index 38261d5ede2b..c40a600cf6a3 100644
--- a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
+++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
@@ -1,6 +1,8 @@
 #/** @file
 #
 #  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+#  Copyright (c) 2017, 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
@@ -12,15 +14,15 @@
 #**/
 
 [Defines]
-  INF_VERSION                    = 0x00010005
-  BASE_NAME                      = NullDmaLib
+  INF_VERSION                    = 0x00010019
+  BASE_NAME                      = CoherentDmaLib
   FILE_GUID                      = 0F2A0816-D319-4ee7-A6B8-D58524E4428F
   MODULE_TYPE                    = BASE
   VERSION_STRING                 = 1.0
   LIBRARY_CLASS                  = DmaLib
 
-[Sources.common]
-  NullDmaLib.c
+[Sources]
+  CoherentDmaLib.c
 
 [Packages]
   MdePkg/MdePkg.dec
@@ -29,13 +31,3 @@ [Packages]
 [LibraryClasses]
   DebugLib
   MemoryAllocationLib
-
-
-[Protocols]
-
-[Guids]
-
-[Pcd]
-
-[Depex]
-  TRUE
-- 
2.11.0

[PATCH 2/6] EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation

[Ard Biesheuvel]

Bring CoherentDmaLib in line with ArmDmaLib, and add support for
defining a static offset between the host's and the bus master's
view of memory.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dec                           |  7 +++++++
 EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c   | 10 +++++++++-
 EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
index 8ad2a84c045c..ccdf38e36a8c 100644
--- a/EmbeddedPkg/EmbeddedPkg.dec
+++ b/EmbeddedPkg/EmbeddedPkg.dec
@@ -208,3 +208,10 @@ [PcdsFixedAtBuild.X64]
 
 [PcdsFixedAtBuild.common, PcdsDynamic.common]
   gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths|L""|VOID*|0x00000055
+
+  #
+  # Value to add to a host address to obtain a device address, using
+  # unsigned 64-bit integer arithmetic. This means we can rely on
+  # truncation on overflow to specify negative offsets.
+  #
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
diff --git a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
index 4cbe349190a9..564db83c901c 100644
--- a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
+++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
@@ -19,6 +19,14 @@
 #include <Library/MemoryAllocationLib.h>
 
 
+STATIC
+PHYSICAL_ADDRESS
+HostToDeviceAddress (
+  IN  VOID      *Address
+  )
+{
+  return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
+}
 
 /**
   Provides the DMA controller-specific addresses needed to access system memory.
@@ -50,7 +58,7 @@ DmaMap (
   OUT    VOID                           **Mapping
   )
 {
-  *DeviceAddress = (PHYSICAL_ADDRESS)(UINTN)HostAddress;
+  *DeviceAddress = HostToDeviceAddress (HostAddress);
   *Mapping = NULL;
   return EFI_SUCCESS;
 }
diff --git a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
index c40a600cf6a3..f64d780e16ed 100644
--- a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
+++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
@@ -31,3 +31,6 @@ [Packages]
 [LibraryClasses]
   DebugLib
   MemoryAllocationLib
+
+[Pcd]
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
-- 
2.11.0

[PATCH 3/6] EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib

[Ard Biesheuvel]

The non-coherent DmaLib implementation in ArmDmaLib no longer relies on
anything in ArmPkg. So clone it into EmbeddedPkg, and rename it to
NonCoherentDmaLib.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 EmbeddedPkg/EmbeddedPkg.dsc                                 |   1 +
 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c   | 491 ++++++++++++++++++++
 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf |  50 ++
 3 files changed, 542 insertions(+)

diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
index 84c5a842e37e..012721a332f4 100644
--- a/EmbeddedPkg/EmbeddedPkg.dsc
+++ b/EmbeddedPkg/EmbeddedPkg.dsc
@@ -251,6 +251,7 @@ [Components.common]
   EmbeddedPkg/Library/TemplateRealTimeClockLib/TemplateRealTimeClockLib.inf
   EmbeddedPkg/Library/LzmaHobCustomDecompressLib/LzmaHobCustomDecompressLib.inf
   EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
+  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
   EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
 
   EmbeddedPkg/Ebl/Ebl.inf
diff --git a/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
new file mode 100644
index 000000000000..08b9c017f426
--- /dev/null
+++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c
@@ -0,0 +1,491 @@
+/** @file
+
+  Generic non-coherent implementation of DmaLib.h
+
+  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+  Copyright (c) 2015 - 2017, 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/DxeServicesTableLib.h>
+#include <Library/MemoryAllocationLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+#include <Library/IoLib.h>
+#include <Library/BaseMemoryLib.h>
+
+#include <Protocol/Cpu.h>
+
+typedef struct {
+  EFI_PHYSICAL_ADDRESS      HostAddress;
+  VOID                      *BufferAddress;
+  UINTN                     NumberOfBytes;
+  DMA_MAP_OPERATION         Operation;
+  BOOLEAN                   DoubleBuffer;
+} MAP_INFO_INSTANCE;
+
+
+typedef struct {
+  LIST_ENTRY          Link;
+  VOID                *HostAddress;
+  UINTN               NumPages;
+  UINT64              Attributes;
+} UNCACHED_ALLOCATION;
+
+STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
+STATIC LIST_ENTRY                 UncachedAllocationList;
+
+STATIC
+PHYSICAL_ADDRESS
+HostToDeviceAddress (
+  IN  VOID      *Address
+  )
+{
+  return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
+}
+
+/**
+  Provides the DMA controller-specific addresses needed to access system memory.
+
+  Operation is relative to the DMA bus master.
+
+  @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 DMA
+                                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
+                                controller to use to access the host's
+                                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.
+
+**/
+EFI_STATUS
+EFIAPI
+DmaMap (
+  IN     DMA_MAP_OPERATION              Operation,
+  IN     VOID                           *HostAddress,
+  IN OUT UINTN                          *NumberOfBytes,
+  OUT    PHYSICAL_ADDRESS               *DeviceAddress,
+  OUT    VOID                           **Mapping
+  )
+{
+  EFI_STATUS                      Status;
+  MAP_INFO_INSTANCE               *Map;
+  VOID                            *Buffer;
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
+  UINTN                           AllocSize;
+
+  if (HostAddress == NULL ||
+      NumberOfBytes == NULL ||
+      DeviceAddress == NULL ||
+      Mapping == NULL ) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (Operation >= MapOperationMaximum) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  *DeviceAddress = HostToDeviceAddress (HostAddress);
+
+  // Remember range so we can flush on the other side
+  Map = AllocatePool (sizeof (MAP_INFO_INSTANCE));
+  if (Map == NULL) {
+    return  EFI_OUT_OF_RESOURCES;
+  }
+
+  if (Operation != MapOperationBusMasterRead &&
+      ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
+       ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
+
+    // Get the cacheability of the region
+    Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
+    if (EFI_ERROR(Status)) {
+      goto FreeMapInfo;
+    }
+
+    // If the mapped buffer is not an uncached buffer
+    if ((GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) != 0) {
+      //
+      // Operations of type MapOperationBusMasterCommonBuffer are only allowed
+      // 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;
+      }
+
+      //
+      // If the buffer does not fill entire cache lines we must double buffer
+      // into a suitably aligned allocation that allows us to invalidate the
+      // cache without running the risk of corrupting adjacent unrelated data.
+      // Note that pool allocations are guaranteed to be 8 byte aligned, so
+      // we only have to add (alignment - 8) worth of padding.
+      //
+      Map->DoubleBuffer = TRUE;
+      AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
+                  (mCpu->DmaBufferAlignment - 8);
+      Map->BufferAddress = AllocatePool (AllocSize);
+      if (Map->BufferAddress == NULL) {
+        Status = EFI_OUT_OF_RESOURCES;
+        goto FreeMapInfo;
+      }
+
+      Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
+      *DeviceAddress = HostToDeviceAddress (Buffer);
+
+      //
+      // Get rid of any dirty cachelines covering the double buffer. This
+      // prevents them from being written back unexpectedly, potentially
+      // overwriting the data we receive from the device.
+      //
+      mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes,
+              EfiCpuFlushTypeWriteBack);
+    } else {
+      Map->DoubleBuffer  = FALSE;
+    }
+  } else {
+    Map->DoubleBuffer  = FALSE;
+
+    DEBUG_CODE_BEGIN ();
+
+    //
+    // The operation type check above only executes if the buffer happens to be
+    // misaligned with respect to CWG, but even if it is aligned, we should not
+    // allow arbitrary buffers to be used for creating consistent mappings.
+    // So duplicate the check here when running in DEBUG mode, just to assert
+    // that we are not trying to create a consistent mapping for cached memory.
+    //
+    Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
+    ASSERT_EFI_ERROR(Status);
+
+    ASSERT (Operation != MapOperationBusMasterCommonBuffer ||
+            (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0);
+
+    DEBUG_CODE_END ();
+
+    // Flush the Data Cache (should not have any effect if the memory region is
+    // uncached)
+    mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
+            EfiCpuFlushTypeWriteBackInvalidate);
+  }
+
+  Map->HostAddress   = (UINTN)HostAddress;
+  Map->NumberOfBytes = *NumberOfBytes;
+  Map->Operation     = Operation;
+
+  *Mapping = Map;
+
+  return EFI_SUCCESS;
+
+FreeMapInfo:
+  FreePool (Map);
+
+  return Status;
+}
+
+
+/**
+  Completes the DmaMapBusMasterRead(), DmaMapBusMasterWrite(), or
+  DmaMapBusMasterCommonBuffer() operation and releases any corresponding
+  resources.
+
+  @param  Mapping               The mapping value returned from DmaMap*().
+
+  @retval EFI_SUCCESS           The range was unmapped.
+  @retval EFI_DEVICE_ERROR      The data was not committed to the target system
+                                memory.
+  @retval EFI_INVALID_PARAMETER An inconsistency was detected between the
+                                mapping type and the DoubleBuffer field
+
+**/
+EFI_STATUS
+EFIAPI
+DmaUnmap (
+  IN  VOID                         *Mapping
+  )
+{
+  MAP_INFO_INSTANCE *Map;
+  EFI_STATUS        Status;
+  VOID              *Buffer;
+
+  if (Mapping == NULL) {
+    ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  Map = (MAP_INFO_INSTANCE *)Mapping;
+
+  Status = EFI_SUCCESS;
+  if (Map->DoubleBuffer) {
+    ASSERT (Map->Operation == MapOperationBusMasterWrite);
+
+    if (Map->Operation != MapOperationBusMasterWrite) {
+      Status = EFI_INVALID_PARAMETER;
+    } else {
+      Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
+
+      mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes,
+              EfiCpuFlushTypeInvalidate);
+
+      CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes);
+
+      FreePool (Map->BufferAddress);
+    }
+  } else {
+    if (Map->Operation == MapOperationBusMasterWrite) {
+      //
+      // Make sure we read buffer from uncached memory and not the cache
+      //
+      mCpu->FlushDataCache (mCpu, Map->HostAddress, Map->NumberOfBytes,
+              EfiCpuFlushTypeInvalidate);
+    }
+  }
+
+  FreePool (Map);
+
+  return Status;
+}
+
+/**
+  Allocates pages that are suitable for an DmaMap() of type
+  MapOperationBusMasterCommonBuffer mapping.
+
+  @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.
+
+  @retval EFI_SUCCESS           The requested memory pages were allocated.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
+
+**/
+EFI_STATUS
+EFIAPI
+DmaAllocateBuffer (
+  IN  EFI_MEMORY_TYPE              MemoryType,
+  IN  UINTN                        Pages,
+  OUT VOID                         **HostAddress
+  )
+{
+  return DmaAllocateAlignedBuffer (MemoryType, Pages, 0, HostAddress);
+}
+
+/**
+  Allocates pages that are suitable for an DmaMap() of type
+  MapOperationBusMasterCommonBuffer mapping, at the requested alignment.
+
+  @param  MemoryType            The type of memory to allocate,
+                                EfiBootServicesData or EfiRuntimeServicesData.
+  @param  Pages                 The number of pages to allocate.
+  @param  Alignment             Alignment in bytes of the base of the returned
+                                buffer (must be a power of 2)
+  @param  HostAddress           A pointer to store the base system memory
+                                address of the allocated range.
+
+  @retval EFI_SUCCESS           The requested memory pages were allocated.
+  @retval EFI_INVALID_PARAMETER One or more parameters are invalid.
+  @retval EFI_OUT_OF_RESOURCES  The memory pages could not be allocated.
+
+**/
+EFI_STATUS
+EFIAPI
+DmaAllocateAlignedBuffer (
+  IN  EFI_MEMORY_TYPE              MemoryType,
+  IN  UINTN                        Pages,
+  IN  UINTN                        Alignment,
+  OUT VOID                         **HostAddress
+  )
+{
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   GcdDescriptor;
+  VOID                              *Allocation;
+  UINT64                            MemType;
+  UNCACHED_ALLOCATION               *Alloc;
+  EFI_STATUS                        Status;
+
+  if (Alignment == 0) {
+    Alignment = EFI_PAGE_SIZE;
+  }
+
+  if (HostAddress == NULL ||
+      (Alignment & (Alignment - 1)) != 0) {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (MemoryType == EfiBootServicesData) {
+    Allocation = AllocateAlignedPages (Pages, Alignment);
+  } else if (MemoryType == EfiRuntimeServicesData) {
+    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
+  } else {
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if (Allocation == NULL) {
+    return EFI_OUT_OF_RESOURCES;
+  }
+
+  // Get the cacheability of the region
+  Status = gDS->GetMemorySpaceDescriptor ((UINTN)Allocation, &GcdDescriptor);
+  if (EFI_ERROR(Status)) {
+    goto FreeBuffer;
+  }
+
+  // Choose a suitable uncached memory type that is supported by the region
+  if (GcdDescriptor.Capabilities & EFI_MEMORY_WC) {
+    MemType = EFI_MEMORY_WC;
+  } else if (GcdDescriptor.Capabilities & EFI_MEMORY_UC) {
+    MemType = EFI_MEMORY_UC;
+  } else {
+    Status = EFI_UNSUPPORTED;
+    goto FreeBuffer;
+  }
+
+  Alloc = AllocatePool (sizeof *Alloc);
+  if (Alloc == NULL) {
+    goto FreeBuffer;
+  }
+
+  Alloc->HostAddress = Allocation;
+  Alloc->NumPages = Pages;
+  Alloc->Attributes = GcdDescriptor.Attributes;
+
+  InsertHeadList (&UncachedAllocationList, &Alloc->Link);
+
+  // Remap the region with the new attributes
+  Status = gDS->SetMemorySpaceAttributes ((PHYSICAL_ADDRESS)(UINTN)Allocation,
+                                          EFI_PAGES_TO_SIZE (Pages),
+                                          MemType);
+  if (EFI_ERROR (Status)) {
+    goto FreeAlloc;
+  }
+
+  Status = mCpu->FlushDataCache (mCpu,
+                                 (PHYSICAL_ADDRESS)(UINTN)Allocation,
+                                 EFI_PAGES_TO_SIZE (Pages),
+                                 EfiCpuFlushTypeInvalidate);
+  if (EFI_ERROR (Status)) {
+    goto FreeAlloc;
+  }
+
+  *HostAddress = Allocation;
+
+  return EFI_SUCCESS;
+
+FreeAlloc:
+  RemoveEntryList (&Alloc->Link);
+  FreePool (Alloc);
+
+FreeBuffer:
+  FreePages (Allocation, Pages);
+  return Status;
+}
+
+
+/**
+  Frees memory that was allocated with DmaAllocateBuffer().
+
+  @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
+                                DmaAllocateBuffer().
+
+**/
+EFI_STATUS
+EFIAPI
+DmaFreeBuffer (
+  IN  UINTN                        Pages,
+  IN  VOID                         *HostAddress
+  )
+{
+  LIST_ENTRY                       *Link;
+  UNCACHED_ALLOCATION              *Alloc;
+  BOOLEAN                          Found;
+  EFI_STATUS                       Status;
+
+  if (HostAddress == NULL) {
+     return EFI_INVALID_PARAMETER;
+  }
+
+  for (Link = GetFirstNode (&UncachedAllocationList), Found = FALSE;
+       !IsNull (&UncachedAllocationList, Link);
+       Link = GetNextNode (&UncachedAllocationList, Link)) {
+
+    Alloc = BASE_CR (Link, UNCACHED_ALLOCATION, Link);
+    if (Alloc->HostAddress == HostAddress && Alloc->NumPages == Pages) {
+      Found = TRUE;
+      break;
+    }
+  }
+
+  if (!Found) {
+    ASSERT (FALSE);
+    return EFI_INVALID_PARAMETER;
+  }
+
+  RemoveEntryList (&Alloc->Link);
+
+  Status = gDS->SetMemorySpaceAttributes ((PHYSICAL_ADDRESS)(UINTN)HostAddress,
+                                          EFI_PAGES_TO_SIZE (Pages),
+                                          Alloc->Attributes);
+  if (EFI_ERROR (Status)) {
+    goto FreeAlloc;
+  }
+
+  //
+  // If we fail to restore the original attributes, it is better to leak the
+  // memory than to return it to the heap
+  //
+  FreePages (HostAddress, Pages);
+
+FreeAlloc:
+  FreePool (Alloc);
+  return Status;
+}
+
+
+EFI_STATUS
+EFIAPI
+NonCoherentDmaLibConstructor (
+  IN EFI_HANDLE       ImageHandle,
+  IN EFI_SYSTEM_TABLE *SystemTable
+  )
+{
+  InitializeListHead (&UncachedAllocationList);
+
+  // 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
new file mode 100644
index 000000000000..9f430d6c3721
--- /dev/null
+++ b/EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
@@ -0,0 +1,50 @@
+#/** @file
+#
+#  Generic non-coherent implementation of DmaLib.h
+#
+#  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
+#  Copyright (c) 2015 - 2017, 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                 = 0x00010019
+  BASE_NAME                   = NonCoherentDmaLib
+  FILE_GUID                   = 43ad4920-db15-4e24-9889-2db568431fbd
+  MODULE_TYPE                 = DXE_DRIVER
+  VERSION_STRING              = 1.0
+  LIBRARY_CLASS               = DmaLib
+  CONSTRUCTOR                 = NonCoherentDmaLibConstructor
+
+[Sources]
+  NonCoherentDmaLib.c
+
+[Packages]
+  EmbeddedPkg/EmbeddedPkg.dec
+  MdePkg/MdePkg.dec
+
+[LibraryClasses]
+  BaseMemoryLib
+  DebugLib
+  DxeServicesTableLib
+  IoLib
+  MemoryAllocationLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gEfiCpuArchProtocolGuid
+
+[Pcd]
+  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
+
+[Depex]
+  gEfiCpuArchProtocolGuid
-- 
2.11.0

[PATCH 4/6] Omap35xxPkg: switch to EmbeddedPkg's NonCoherentDmaLib

[Ard Biesheuvel]

Replace the reference to the ARM specific ArmDmaLib with a reference
to the generic NonCoherentDmaLib.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 Omap35xxPkg/Omap35xxPkg.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Omap35xxPkg/Omap35xxPkg.dsc b/Omap35xxPkg/Omap35xxPkg.dsc
index 941bc97060b9..c5d9746104e6 100644
--- a/Omap35xxPkg/Omap35xxPkg.dsc
+++ b/Omap35xxPkg/Omap35xxPkg.dsc
@@ -61,7 +61,7 @@ [LibraryClasses.common]
   DxeServicesTableLib|MdePkg/Library/DxeServicesTableLib/DxeServicesTableLib.inf
   UefiDriverEntryPoint|MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
-  DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
+  DmaLib|EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
 
   TimerLib|Omap35xxPkg/Library/Omap35xxTimerLib/Omap35xxTimerLib.inf
 
-- 
2.11.0

[PATCH 5/6] BeagleBoardPkg: switch to generic non-coherent DmaLib

[Ard Biesheuvel]

Replace the reference to the ARM specific ArmDmaLib with a reference
to the generic NonCoherentDmaLib.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 BeagleBoardPkg/BeagleBoardPkg.dsc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/BeagleBoardPkg/BeagleBoardPkg.dsc b/BeagleBoardPkg/BeagleBoardPkg.dsc
index 84aae84ff52d..30f6fd02e82f 100644
--- a/BeagleBoardPkg/BeagleBoardPkg.dsc
+++ b/BeagleBoardPkg/BeagleBoardPkg.dsc
@@ -122,7 +122,7 @@ [LibraryClasses.common]
   GdbSerialLib|Omap35xxPkg/Library/GdbSerialLib/GdbSerialLib.inf
   ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
   DebugAgentLib|MdeModulePkg/Library/DebugAgentLibNull/DebugAgentLibNull.inf
-  DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
+  DmaLib|EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
 
   NetLib|MdeModulePkg/Library/DxeNetLib/DxeNetLib.inf
   FdtLib|EmbeddedPkg/Library/FdtLib/FdtLib.inf
-- 
2.11.0

[PATCH 6/6] ArmPkg: remove ArmDmaLib

[Ard Biesheuvel]

Now that we have a generic DmaLib implementation for non-coherent DMA,
let's get rid of the ARM specific one.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/ArmPkg.dsc                      |   2 -
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c   | 478 --------------------
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf |  49 --
 3 files changed, 529 deletions(-)

diff --git a/ArmPkg/ArmPkg.dsc b/ArmPkg/ArmPkg.dsc
index ff2b0c074dc1..cf86f89bd702 100644
--- a/ArmPkg/ArmPkg.dsc
+++ b/ArmPkg/ArmPkg.dsc
@@ -72,7 +72,6 @@ [LibraryClasses.common]
   ArmGenericTimerCounterLib|ArmPkg/Library/ArmGenericTimerPhyCounterLib/ArmGenericTimerPhyCounterLib.inf
   ArmSmcLib|ArmPkg/Library/ArmSmcLib/ArmSmcLib.inf
   ArmDisassemblerLib|ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
-  DmaLib|ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
 
   UefiApplicationEntryPoint|MdePkg/Library/UefiApplicationEntryPoint/UefiApplicationEntryPoint.inf
   PerformanceLib|MdePkg/Library/BasePerformanceLibNull/BasePerformanceLibNull.inf
@@ -106,7 +105,6 @@ [LibraryClasses.ARM, LibraryClasses.AARCH64]
 [Components.common]
   ArmPkg/Library/ArmCacheMaintenanceLib/ArmCacheMaintenanceLib.inf
   ArmPkg/Library/ArmDisassemblerLib/ArmDisassemblerLib.inf
-  ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
   ArmPkg/Library/BdsLib/BdsLib.inf
   ArmPkg/Library/CompilerIntrinsicsLib/CompilerIntrinsicsLib.inf
   ArmPkg/Library/DebugAgentSymbolsBaseLib/DebugAgentSymbolsBaseLib.inf
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
deleted file mode 100644
index 2a8cf0fe21a4..000000000000
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ /dev/null
@@ -1,478 +0,0 @@
-/** @file
-  Generic ARM implementation of DmaLib.h
-
-  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
-  Copyright (c) 2015 - 2017, 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/DxeServicesTableLib.h>
-#include <Library/MemoryAllocationLib.h>
-#include <Library/UefiBootServicesTableLib.h>
-#include <Library/IoLib.h>
-#include <Library/BaseMemoryLib.h>
-
-#include <Protocol/Cpu.h>
-
-typedef struct {
-  EFI_PHYSICAL_ADDRESS      HostAddress;
-  VOID                      *BufferAddress;
-  UINTN                     NumberOfBytes;
-  DMA_MAP_OPERATION         Operation;
-  BOOLEAN                   DoubleBuffer;
-} MAP_INFO_INSTANCE;
-
-
-typedef struct {
-  LIST_ENTRY          Link;
-  VOID                *HostAddress;
-  UINTN               NumPages;
-  UINT64              Attributes;
-} UNCACHED_ALLOCATION;
-
-STATIC EFI_CPU_ARCH_PROTOCOL      *mCpu;
-STATIC LIST_ENTRY                 UncachedAllocationList;
-
-STATIC
-PHYSICAL_ADDRESS
-HostToDeviceAddress (
-  IN  VOID      *Address
-  )
-{
-  return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdArmDmaDeviceOffset);
-}
-
-/**
-  Provides the DMA controller-specific addresses needed to access system memory.
-
-  Operation is relative to the DMA bus master.
-
-  @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 DMA 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 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.
-
-**/
-EFI_STATUS
-EFIAPI
-DmaMap (
-  IN     DMA_MAP_OPERATION              Operation,
-  IN     VOID                           *HostAddress,
-  IN OUT UINTN                          *NumberOfBytes,
-  OUT    PHYSICAL_ADDRESS               *DeviceAddress,
-  OUT    VOID                           **Mapping
-  )
-{
-  EFI_STATUS                      Status;
-  MAP_INFO_INSTANCE               *Map;
-  VOID                            *Buffer;
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
-  UINTN                           AllocSize;
-
-  if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if (Operation >= MapOperationMaximum) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  *DeviceAddress = HostToDeviceAddress (HostAddress);
-
-  // Remember range so we can flush on the other side
-  Map = AllocatePool (sizeof (MAP_INFO_INSTANCE));
-  if (Map == NULL) {
-    return  EFI_OUT_OF_RESOURCES;
-  }
-
-  if (Operation != MapOperationBusMasterRead &&
-      ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
-       ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
-
-    // Get the cacheability of the region
-    Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
-    if (EFI_ERROR(Status)) {
-      goto FreeMapInfo;
-    }
-
-    // If the mapped buffer is not an uncached buffer
-    if ((GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) != 0) {
-      //
-      // Operations of type MapOperationBusMasterCommonBuffer are only allowed
-      // on uncached buffers.
-      //
-      if (Operation == MapOperationBusMasterCommonBuffer) {
-        DEBUG ((EFI_D_ERROR,
-          "%a: Operation type 'MapOperationBusMasterCommonBuffer' is only supported\n"
-          "on memory regions that were allocated using DmaAllocateBuffer ()\n",
-          __FUNCTION__));
-        Status = EFI_UNSUPPORTED;
-        goto FreeMapInfo;
-      }
-
-      //
-      // If the buffer does not fill entire cache lines we must double buffer
-      // into a suitably aligned allocation that allows us to invalidate the
-      // cache without running the risk of corrupting adjacent unrelated data.
-      // Note that pool allocations are guaranteed to be 8 byte aligned, so
-      // we only have to add (alignment - 8) worth of padding.
-      //
-      Map->DoubleBuffer = TRUE;
-      AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
-                  (mCpu->DmaBufferAlignment - 8);
-      Map->BufferAddress = AllocatePool (AllocSize);
-      if (Map->BufferAddress == NULL) {
-        Status = EFI_OUT_OF_RESOURCES;
-        goto FreeMapInfo;
-      }
-
-      Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
-      *DeviceAddress = HostToDeviceAddress (Buffer);
-
-      //
-      // Get rid of any dirty cachelines covering the double buffer. This
-      // prevents them from being written back unexpectedly, potentially
-      // overwriting the data we receive from the device.
-      //
-      mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes,
-              EfiCpuFlushTypeWriteBack);
-    } else {
-      Map->DoubleBuffer  = FALSE;
-    }
-  } else {
-    Map->DoubleBuffer  = FALSE;
-
-    DEBUG_CODE_BEGIN ();
-
-    //
-    // The operation type check above only executes if the buffer happens to be
-    // misaligned with respect to CWG, but even if it is aligned, we should not
-    // allow arbitrary buffers to be used for creating consistent mappings.
-    // So duplicate the check here when running in DEBUG mode, just to assert
-    // that we are not trying to create a consistent mapping for cached memory.
-    //
-    Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
-    ASSERT_EFI_ERROR(Status);
-
-    ASSERT (Operation != MapOperationBusMasterCommonBuffer ||
-            (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0);
-
-    DEBUG_CODE_END ();
-
-    // Flush the Data Cache (should not have any effect if the memory region is uncached)
-    mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
-            EfiCpuFlushTypeWriteBackInvalidate);
-  }
-
-  Map->HostAddress   = (UINTN)HostAddress;
-  Map->NumberOfBytes = *NumberOfBytes;
-  Map->Operation     = Operation;
-
-  *Mapping = Map;
-
-  return EFI_SUCCESS;
-
-FreeMapInfo:
-  FreePool (Map);
-
-  return Status;
-}
-
-
-/**
-  Completes the DmaMapBusMasterRead(), DmaMapBusMasterWrite(), or DmaMapBusMasterCommonBuffer()
-  operation and releases any corresponding resources.
-
-  @param  Mapping               The mapping value returned from DmaMap*().
-
-  @retval EFI_SUCCESS           The range was unmapped.
-  @retval EFI_DEVICE_ERROR      The data was not committed to the target system memory.
-  @retval EFI_INVALID_PARAMETER An inconsistency was detected between the mapping type
-                                and the DoubleBuffer field
-
-**/
-EFI_STATUS
-EFIAPI
-DmaUnmap (
-  IN  VOID                         *Mapping
-  )
-{
-  MAP_INFO_INSTANCE *Map;
-  EFI_STATUS        Status;
-  VOID              *Buffer;
-
-  if (Mapping == NULL) {
-    ASSERT (FALSE);
-    return EFI_INVALID_PARAMETER;
-  }
-
-  Map = (MAP_INFO_INSTANCE *)Mapping;
-
-  Status = EFI_SUCCESS;
-  if (Map->DoubleBuffer) {
-    ASSERT (Map->Operation == MapOperationBusMasterWrite);
-
-    if (Map->Operation != MapOperationBusMasterWrite) {
-      Status = EFI_INVALID_PARAMETER;
-    } else {
-      Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
-
-      mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes,
-              EfiCpuFlushTypeInvalidate);
-
-      CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes);
-
-      FreePool (Map->BufferAddress);
-    }
-  } else {
-    if (Map->Operation == MapOperationBusMasterWrite) {
-      //
-      // Make sure we read buffer from uncached memory and not the cache
-      //
-      mCpu->FlushDataCache (mCpu, Map->HostAddress, Map->NumberOfBytes,
-              EfiCpuFlushTypeInvalidate);
-    }
-  }
-
-  FreePool (Map);
-
-  return Status;
-}
-
-/**
-  Allocates pages that are suitable for an DmaMap() of type MapOperationBusMasterCommonBuffer.
-  mapping.
-
-  @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.
-
-  @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.
-
-**/
-EFI_STATUS
-EFIAPI
-DmaAllocateBuffer (
-  IN  EFI_MEMORY_TYPE              MemoryType,
-  IN  UINTN                        Pages,
-  OUT VOID                         **HostAddress
-  )
-{
-  return DmaAllocateAlignedBuffer (MemoryType, Pages, 0, HostAddress);
-}
-
-/**
-  Allocates pages that are suitable for an DmaMap() of type
-  MapOperationBusMasterCommonBuffer mapping, at the requested alignment.
-
-  @param  MemoryType            The type of memory to allocate, EfiBootServicesData or
-                                EfiRuntimeServicesData.
-  @param  Pages                 The number of pages to allocate.
-  @param  Alignment             Alignment in bytes of the base of the returned
-                                buffer (must be a power of 2)
-  @param  HostAddress           A pointer to store the base system memory address of 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.
-
-**/
-EFI_STATUS
-EFIAPI
-DmaAllocateAlignedBuffer (
-  IN  EFI_MEMORY_TYPE              MemoryType,
-  IN  UINTN                        Pages,
-  IN  UINTN                        Alignment,
-  OUT VOID                         **HostAddress
-  )
-{
-  EFI_GCD_MEMORY_SPACE_DESCRIPTOR   GcdDescriptor;
-  VOID                              *Allocation;
-  UINT64                            MemType;
-  UNCACHED_ALLOCATION               *Alloc;
-  EFI_STATUS                        Status;
-
-  if (Alignment == 0) {
-    Alignment = EFI_PAGE_SIZE;
-  }
-
-  if (HostAddress == NULL ||
-      (Alignment & (Alignment - 1)) != 0) {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if (MemoryType == EfiBootServicesData) {
-    Allocation = AllocateAlignedPages (Pages, Alignment);
-  } else if (MemoryType == EfiRuntimeServicesData) {
-    Allocation = AllocateAlignedRuntimePages (Pages, Alignment);
-  } else {
-    return EFI_INVALID_PARAMETER;
-  }
-
-  if (Allocation == NULL) {
-    return EFI_OUT_OF_RESOURCES;
-  }
-
-  // Get the cacheability of the region
-  Status = gDS->GetMemorySpaceDescriptor ((UINTN)Allocation, &GcdDescriptor);
-  if (EFI_ERROR(Status)) {
-    goto FreeBuffer;
-  }
-
-  // Choose a suitable uncached memory type that is supported by the region
-  if (GcdDescriptor.Capabilities & EFI_MEMORY_WC) {
-    MemType = EFI_MEMORY_WC;
-  } else if (GcdDescriptor.Capabilities & EFI_MEMORY_UC) {
-    MemType = EFI_MEMORY_UC;
-  } else {
-    Status = EFI_UNSUPPORTED;
-    goto FreeBuffer;
-  }
-
-  Alloc = AllocatePool (sizeof *Alloc);
-  if (Alloc == NULL) {
-    goto FreeBuffer;
-  }
-
-  Alloc->HostAddress = Allocation;
-  Alloc->NumPages = Pages;
-  Alloc->Attributes = GcdDescriptor.Attributes;
-
-  InsertHeadList (&UncachedAllocationList, &Alloc->Link);
-
-  // Remap the region with the new attributes
-  Status = gDS->SetMemorySpaceAttributes ((PHYSICAL_ADDRESS)(UINTN)Allocation,
-                                          EFI_PAGES_TO_SIZE (Pages),
-                                          MemType);
-  if (EFI_ERROR (Status)) {
-    goto FreeAlloc;
-  }
-
-  Status = mCpu->FlushDataCache (mCpu,
-                                 (PHYSICAL_ADDRESS)(UINTN)Allocation,
-                                 EFI_PAGES_TO_SIZE (Pages),
-                                 EfiCpuFlushTypeInvalidate);
-  if (EFI_ERROR (Status)) {
-    goto FreeAlloc;
-  }
-
-  *HostAddress = Allocation;
-
-  return EFI_SUCCESS;
-
-FreeAlloc:
-  RemoveEntryList (&Alloc->Link);
-  FreePool (Alloc);
-
-FreeBuffer:
-  FreePages (Allocation, Pages);
-  return Status;
-}
-
-
-/**
-  Frees memory that was allocated with DmaAllocateBuffer().
-
-  @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 DmaAllocateBuffer().
-
-**/
-EFI_STATUS
-EFIAPI
-DmaFreeBuffer (
-  IN  UINTN                        Pages,
-  IN  VOID                         *HostAddress
-  )
-{
-  LIST_ENTRY                       *Link;
-  UNCACHED_ALLOCATION              *Alloc;
-  BOOLEAN                          Found;
-  EFI_STATUS                       Status;
-
-  if (HostAddress == NULL) {
-     return EFI_INVALID_PARAMETER;
-  }
-
-  for (Link = GetFirstNode (&UncachedAllocationList), Found = FALSE;
-       !IsNull (&UncachedAllocationList, Link);
-       Link = GetNextNode (&UncachedAllocationList, Link)) {
-
-    Alloc = BASE_CR (Link, UNCACHED_ALLOCATION, Link);
-    if (Alloc->HostAddress == HostAddress && Alloc->NumPages == Pages) {
-      Found = TRUE;
-      break;
-    }
-  }
-
-  if (!Found) {
-    ASSERT (FALSE);
-    return EFI_INVALID_PARAMETER;
-  }
-
-  RemoveEntryList (&Alloc->Link);
-
-  Status = gDS->SetMemorySpaceAttributes ((PHYSICAL_ADDRESS)(UINTN)HostAddress,
-                                          EFI_PAGES_TO_SIZE (Pages),
-                                          Alloc->Attributes);
-  if (EFI_ERROR (Status)) {
-    goto FreeAlloc;
-  }
-
-  //
-  // If we fail to restore the original attributes, it is better to leak the
-  // memory than to return it to the heap
-  //
-  FreePages (HostAddress, Pages);
-
-FreeAlloc:
-  FreePool (Alloc);
-  return Status;
-}
-
-
-EFI_STATUS
-EFIAPI
-ArmDmaLibConstructor (
-  IN EFI_HANDLE       ImageHandle,
-  IN EFI_SYSTEM_TABLE *SystemTable
-  )
-{
-  InitializeListHead (&UncachedAllocationList);
-
-  // Get the Cpu protocol for later use
-  return gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
-}
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
deleted file mode 100644
index e33ed92c9d20..000000000000
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
+++ /dev/null
@@ -1,49 +0,0 @@
-#/** @file
-#
-#  Copyright (c) 2008 - 2010, Apple Inc. 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                    = 0x00010005
-  BASE_NAME                      = ArmDmaLib
-  FILE_GUID                      = F1BD6B36-B705-43aa-8A28-33F58ED85EFB
-  MODULE_TYPE                    = UEFI_DRIVER
-  VERSION_STRING                 = 1.0
-  LIBRARY_CLASS                  = DmaLib
-  CONSTRUCTOR                    = ArmDmaLibConstructor
-
-[Sources.common]
-  ArmDmaLib.c
-
-[Packages]
-  MdePkg/MdePkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
-  ArmPkg/ArmPkg.dec
-
-
-[LibraryClasses]
-  DebugLib
-  DxeServicesTableLib
-  UefiBootServicesTableLib
-  MemoryAllocationLib
-  IoLib
-  BaseMemoryLib
-
-[Protocols]
-  gEfiCpuArchProtocolGuid
-
-[Guids]
-
-[Pcd]
-  gArmTokenSpaceGuid.PcdArmDmaDeviceOffset
-
-[Depex]
-  gEfiCpuArchProtocolGuid
-- 
2.11.0

Re: [PATCH 1/6] EmbeddedPkg: rename NullDmaLib to CoherentDmaLib

[Leif Lindholm]

On Wed, Aug 30, 2017 at 09:21:03AM +0100, Ard Biesheuvel wrote:
> Rename NullDmaLib to CoherentDmaLib to better reflect its nature, and
> to better convey the fact that it actually serves a useful purpose,
> i.e., as a DmaLib library class resolution for drivers that control
> hardware that may only be cache coherent or in some cases (i.e., on
> some platforms but not on others).

The above doesn't read very well (and I'm not 100% certain what it's
trying to say, so can't really propose an improvement).
No other issues with patch.

/
    Leif

> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dsc                                                          |  2 +-
>  EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c}     |  0
>  EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf} | 20 ++++++--------------
>  3 files changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dsc b/EmbeddedPkg/EmbeddedPkg.dsc
> index 4a34e34843ad..84c5a842e37e 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dsc
> +++ b/EmbeddedPkg/EmbeddedPkg.dsc
> @@ -250,7 +250,7 @@ [Components.common]
>    EmbeddedPkg/Library/TemplateResetSystemLib/TemplateResetSystemLib.inf
>    EmbeddedPkg/Library/TemplateRealTimeClockLib/TemplateRealTimeClockLib.inf
>    EmbeddedPkg/Library/LzmaHobCustomDecompressLib/LzmaHobCustomDecompressLib.inf
> -  EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
> +  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
>    EmbeddedPkg/Library/DxeDtPlatformDtbLoaderLibDefault/DxeDtPlatformDtbLoaderLibDefault.inf
>  
>    EmbeddedPkg/Ebl/Ebl.inf
> diff --git a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.c b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
> similarity index 100%
> rename from EmbeddedPkg/Library/NullDmaLib/NullDmaLib.c
> rename to EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
> diff --git a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> similarity index 78%
> rename from EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
> rename to EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> index 38261d5ede2b..c40a600cf6a3 100644
> --- a/EmbeddedPkg/Library/NullDmaLib/NullDmaLib.inf
> +++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> @@ -1,6 +1,8 @@
>  #/** @file
>  #
>  #  Copyright (c) 2008 - 2010, Apple Inc. All rights reserved.<BR>
> +#  Copyright (c) 2017, 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
> @@ -12,15 +14,15 @@
>  #**/
>  
>  [Defines]
> -  INF_VERSION                    = 0x00010005
> -  BASE_NAME                      = NullDmaLib
> +  INF_VERSION                    = 0x00010019
> +  BASE_NAME                      = CoherentDmaLib
>    FILE_GUID                      = 0F2A0816-D319-4ee7-A6B8-D58524E4428F
>    MODULE_TYPE                    = BASE
>    VERSION_STRING                 = 1.0
>    LIBRARY_CLASS                  = DmaLib
>  
> -[Sources.common]
> -  NullDmaLib.c
> +[Sources]
> +  CoherentDmaLib.c
>  
>  [Packages]
>    MdePkg/MdePkg.dec
> @@ -29,13 +31,3 @@ [Packages]
>  [LibraryClasses]
>    DebugLib
>    MemoryAllocationLib
> -
> -
> -[Protocols]
> -
> -[Guids]
> -
> -[Pcd]
> -
> -[Depex]
> -  TRUE
> -- 
> 2.11.0
> 

Re: [PATCH 2/6] EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation

[Leif Lindholm]

On Wed, Aug 30, 2017 at 09:21:04AM +0100, Ard Biesheuvel wrote:
> Bring CoherentDmaLib in line with ArmDmaLib, and add support for
> defining a static offset between the host's and the bus master's
> view of memory.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  EmbeddedPkg/EmbeddedPkg.dec                           |  7 +++++++
>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c   | 10 +++++++++-
>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf |  3 +++
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> index 8ad2a84c045c..ccdf38e36a8c 100644
> --- a/EmbeddedPkg/EmbeddedPkg.dec
> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> @@ -208,3 +208,10 @@ [PcdsFixedAtBuild.X64]
>  
>  [PcdsFixedAtBuild.common, PcdsDynamic.common]
>    gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths|L""|VOID*|0x00000055
> +
> +  #
> +  # Value to add to a host address to obtain a device address, using
> +  # unsigned 64-bit integer arithmetic. This means we can rely on
> +  # truncation on overflow to specify negative offsets.

Is that promotion-safe on 32-bit archs?

/
    Leif

> +  #
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset|0x0|UINT64|0x0000058
> diff --git a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
> index 4cbe349190a9..564db83c901c 100644
> --- a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
> +++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c
> @@ -19,6 +19,14 @@
>  #include <Library/MemoryAllocationLib.h>
>  
>  
> +STATIC
> +PHYSICAL_ADDRESS
> +HostToDeviceAddress (
> +  IN  VOID      *Address
> +  )
> +{
> +  return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);
> +}
>  
>  /**
>    Provides the DMA controller-specific addresses needed to access system memory.
> @@ -50,7 +58,7 @@ DmaMap (
>    OUT    VOID                           **Mapping
>    )
>  {
> -  *DeviceAddress = (PHYSICAL_ADDRESS)(UINTN)HostAddress;
> +  *DeviceAddress = HostToDeviceAddress (HostAddress);
>    *Mapping = NULL;
>    return EFI_SUCCESS;
>  }
> diff --git a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> index c40a600cf6a3..f64d780e16ed 100644
> --- a/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> +++ b/EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf
> @@ -31,3 +31,6 @@ [Packages]
>  [LibraryClasses]
>    DebugLib
>    MemoryAllocationLib
> +
> +[Pcd]
> +  gEmbeddedTokenSpaceGuid.PcdDmaDeviceOffset
> -- 
> 2.11.0
> 

Re: [PATCH 1/6] EmbeddedPkg: rename NullDmaLib to CoherentDmaLib

[Ard Biesheuvel]

On 30 August 2017 at 11:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Aug 30, 2017 at 09:21:03AM +0100, Ard Biesheuvel wrote:
>> Rename NullDmaLib to CoherentDmaLib to better reflect its nature, and
>> to better convey the fact that it actually serves a useful purpose,
>> i.e., as a DmaLib library class resolution for drivers that control
>> hardware that may only be cache coherent or in some cases (i.e., on
>> some platforms but not on others).
>
> The above doesn't read very well (and I'm not 100% certain what it's
> trying to say, so can't really propose an improvement).
> No other issues with patch.
>

How about

"""
The name NullDmaLib suggests that this library is a placeholder that
only exists to fulfil formal dependencies on the DmaLib library class
without providing an actual implementation (*). This is not the case,
though: NullDmaLib does implement DmaLib fully, but doing so simply
requires very little effort on a cache coherent platform. So let's
rename it to CoherentDmaLib instead.
"""

* there are such instances in MdeModulePkg that do nothing and
ASSERT() in every function.

Re: [PATCH 2/6] EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation

[Ard Biesheuvel]

On 30 August 2017 at 11:51, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Aug 30, 2017 at 09:21:04AM +0100, Ard Biesheuvel wrote:
>> Bring CoherentDmaLib in line with ArmDmaLib, and add support for
>> defining a static offset between the host's and the bus master's
>> view of memory.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  EmbeddedPkg/EmbeddedPkg.dec                           |  7 +++++++
>>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c   | 10 +++++++++-
>>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf |  3 +++
>>  3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
>> index 8ad2a84c045c..ccdf38e36a8c 100644
>> --- a/EmbeddedPkg/EmbeddedPkg.dec
>> +++ b/EmbeddedPkg/EmbeddedPkg.dec
>> @@ -208,3 +208,10 @@ [PcdsFixedAtBuild.X64]
>>
>>  [PcdsFixedAtBuild.common, PcdsDynamic.common]
>>    gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths|L""|VOID*|0x00000055
>> +
>> +  #
>> +  # Value to add to a host address to obtain a device address, using
>> +  # unsigned 64-bit integer arithmetic. This means we can rely on
>> +  # truncation on overflow to specify negative offsets.
>
> Is that promotion-safe on 32-bit archs?
>

Yes. EFI_PHYSICAL_ADDRESS is always 64-bits, and so is this PCD, so
whether it is a 32-bit platform or not should not make any difference.

Re: [PATCH 1/6] EmbeddedPkg: rename NullDmaLib to CoherentDmaLib

[Leif Lindholm]

On Wed, Aug 30, 2017 at 11:52:26AM +0100, Ard Biesheuvel wrote:
> On 30 August 2017 at 11:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Aug 30, 2017 at 09:21:03AM +0100, Ard Biesheuvel wrote:
> >> Rename NullDmaLib to CoherentDmaLib to better reflect its nature, and
> >> to better convey the fact that it actually serves a useful purpose,
> >> i.e., as a DmaLib library class resolution for drivers that control
> >> hardware that may only be cache coherent or in some cases (i.e., on
> >> some platforms but not on others).
> >
> > The above doesn't read very well (and I'm not 100% certain what it's
> > trying to say, so can't really propose an improvement).
> > No other issues with patch.
> >
> 
> How about
> 
> """
> The name NullDmaLib suggests that this library is a placeholder that
> only exists to fulfil formal dependencies on the DmaLib library class
> without providing an actual implementation (*). This is not the case,
> though: NullDmaLib does implement DmaLib fully, but doing so simply
> requires very little effort on a cache coherent platform. So let's
> rename it to CoherentDmaLib instead.
> """
> 
> * there are such instances in MdeModulePkg that do nothing and
> ASSERT() in every function.

Indeed.
Yes, this looks fine to me:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH 2/6] EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation

[Leif Lindholm]

On Wed, Aug 30, 2017 at 11:54:05AM +0100, Ard Biesheuvel wrote:
> On 30 August 2017 at 11:51, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Aug 30, 2017 at 09:21:04AM +0100, Ard Biesheuvel wrote:
> >> Bring CoherentDmaLib in line with ArmDmaLib, and add support for
> >> defining a static offset between the host's and the bus master's
> >> view of memory.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.1
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  EmbeddedPkg/EmbeddedPkg.dec                           |  7 +++++++
> >>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.c   | 10 +++++++++-
> >>  EmbeddedPkg/Library/CoherentDmaLib/CoherentDmaLib.inf |  3 +++
> >>  3 files changed, 19 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/EmbeddedPkg/EmbeddedPkg.dec b/EmbeddedPkg/EmbeddedPkg.dec
> >> index 8ad2a84c045c..ccdf38e36a8c 100644
> >> --- a/EmbeddedPkg/EmbeddedPkg.dec
> >> +++ b/EmbeddedPkg/EmbeddedPkg.dec
> >> @@ -208,3 +208,10 @@ [PcdsFixedAtBuild.X64]
> >>
> >>  [PcdsFixedAtBuild.common, PcdsDynamic.common]
> >>    gEmbeddedTokenSpaceGuid.PcdFdtDevicePaths|L""|VOID*|0x00000055
> >> +
> >> +  #
> >> +  # Value to add to a host address to obtain a device address, using
> >> +  # unsigned 64-bit integer arithmetic. This means we can rely on
> >> +  # truncation on overflow to specify negative offsets.
> >
> > Is that promotion-safe on 32-bit archs?
> 
> Yes. EFI_PHYSICAL_ADDRESS is always 64-bits, and so is this PCD, so
> whether it is a 32-bit platform or not should not make any difference.

Right.

Well, EFI_PHYSICAL_ADDRESS is. PHYSICAL_ADDRESS appears to also be
(they are not derived from each other).

+  return (PHYSICAL_ADDRESS)(UINTN)Address + PcdGet64 (PcdDmaDeviceOffset);

(I think I misparsed the above as
   return (PHYSICAL_ADDRESS)((UINTN)Address + PcdGet64 (PcdDmaDeviceOffset));
)

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

Re: [PATCH 3/6] EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib

[Leif Lindholm]

On Wed, Aug 30, 2017 at 09:21:05AM +0100, Ard Biesheuvel wrote:
> The non-coherent DmaLib implementation in ArmDmaLib no longer relies on
> anything in ArmPkg. So clone it into EmbeddedPkg, and rename it to
> NonCoherentDmaLib.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Re: [PATCH 0/6] ArmPkg EmbeddedPkg: clean up DmaLib implementations

[Leif Lindholm]

On Wed, Aug 30, 2017 at 09:21:02AM +0100, Ard Biesheuvel wrote:
> Currently, we have two DmaLib implementations: a cache coherent one called
> 'NullDmaLib' residing in EmbeddedPkg, and a non-cache coherent one called
> 'ArmDmaLib', residinh in ArmPkg.
> 
> In both cases, this is slightly awkward: NullDmaLib suggests no functionality
> whatsoever, which is slightly misleading because 'nothing' is the correct
> action in case of cache coherent DMA, rather than a lack of action. As for
> ArmDmalib, this was never specific to ARM, and no longer depends on anything
> that ArmPkg provides, so it does not really belong in ArmPkg anymore.
> 
> So let's rename them to CoherentDmaLib and NonCoherentDmaLib, respectively,
> and move that latter into EmbeddedPkg where it arguably belongs. To align
> the two further, add support for non-1:1 DMA mappings to CoherentDmaLib as
> well.
> 
> Note that the final patch can only be merged after out-of-tree platforms
> have switched from ArmDmaLib to NonCoherentDmaLib.

For 4-6/6:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> Ard Biesheuvel (6):
>   EmbeddedPkg: rename NullDmaLib to CoherentDmaLib
>   EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation
>   EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib
>   Omap35xxPkg: switch to EmbeddedPkg's NonCoherentDmaLib
>   BeagleBoardPkg: switch to generic non-coherent DmaLib
>   ArmPkg: remove ArmDmaLib
> 
>  ArmPkg/ArmPkg.dsc                                                                                 |   2 -
>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf                                                            |  49 ---------
>  BeagleBoardPkg/BeagleBoardPkg.dsc                                                                 |   2 +-
>  EmbeddedPkg/EmbeddedPkg.dec                                                                       |   7 ++
>  EmbeddedPkg/EmbeddedPkg.dsc                                                                       |   3 +-
>  EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c}                  |  10 +-
>  EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf}              |  19 ++--
>  ArmPkg/Library/ArmDmaLib/ArmDmaLib.c => EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c | 105 +++++++++++---------
>  EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf                                       |  50 ++++++++++
>  Omap35xxPkg/Omap35xxPkg.dsc                                                                       |   2 +-
>  10 files changed, 136 insertions(+), 113 deletions(-)
>  delete mode 100644 ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
>  rename EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.c => CoherentDmaLib/CoherentDmaLib.c} (94%)
>  rename EmbeddedPkg/Library/{NullDmaLib/NullDmaLib.inf => CoherentDmaLib/CoherentDmaLib.inf} (75%)
>  rename ArmPkg/Library/ArmDmaLib/ArmDmaLib.c => EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.c (82%)
>  create mode 100644 EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf
> 
> -- 
> 2.11.0
> 

Re: [PATCH 0/6] ArmPkg EmbeddedPkg: clean up DmaLib implementations

[Ard Biesheuvel]

On 30 August 2017 at 14:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Aug 30, 2017 at 09:21:02AM +0100, Ard Biesheuvel wrote:
>> Currently, we have two DmaLib implementations: a cache coherent one called
>> 'NullDmaLib' residing in EmbeddedPkg, and a non-cache coherent one called
>> 'ArmDmaLib', residinh in ArmPkg.
>>
>> In both cases, this is slightly awkward: NullDmaLib suggests no functionality
>> whatsoever, which is slightly misleading because 'nothing' is the correct
>> action in case of cache coherent DMA, rather than a lack of action. As for
>> ArmDmalib, this was never specific to ARM, and no longer depends on anything
>> that ArmPkg provides, so it does not really belong in ArmPkg anymore.
>>
>> So let's rename them to CoherentDmaLib and NonCoherentDmaLib, respectively,
>> and move that latter into EmbeddedPkg where it arguably belongs. To align
>> the two further, add support for non-1:1 DMA mappings to CoherentDmaLib as
>> well.
>>
>> Note that the final patch can only be merged after out-of-tree platforms
>> have switched from ArmDmaLib to NonCoherentDmaLib.
>
> For 4-6/6:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Thanks,

#1 - #5 merged as

7385d2543e2a EmbeddedPkg: rename NullDmaLib to CoherentDmaLib
0bcb80106762 EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation
723102c72fb0 EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib
c878cd95e132 Omap35xxPkg: switch to EmbeddedPkg's NonCoherentDmaLib
877f4460b3e3 BeagleBoardPkg: switch to generic non-coherent DmaLib

#6 needs to wait until the edk2-platforms changes are in.

Re: [PATCH 0/6] ArmPkg EmbeddedPkg: clean up DmaLib implementations

[Ard Biesheuvel]

On 30 August 2017 at 14:17, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 30 August 2017 at 14:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Wed, Aug 30, 2017 at 09:21:02AM +0100, Ard Biesheuvel wrote:
>>> Currently, we have two DmaLib implementations: a cache coherent one called
>>> 'NullDmaLib' residing in EmbeddedPkg, and a non-cache coherent one called
>>> 'ArmDmaLib', residinh in ArmPkg.
>>>
>>> In both cases, this is slightly awkward: NullDmaLib suggests no functionality
>>> whatsoever, which is slightly misleading because 'nothing' is the correct
>>> action in case of cache coherent DMA, rather than a lack of action. As for
>>> ArmDmalib, this was never specific to ARM, and no longer depends on anything
>>> that ArmPkg provides, so it does not really belong in ArmPkg anymore.
>>>
>>> So let's rename them to CoherentDmaLib and NonCoherentDmaLib, respectively,
>>> and move that latter into EmbeddedPkg where it arguably belongs. To align
>>> the two further, add support for non-1:1 DMA mappings to CoherentDmaLib as
>>> well.
>>>
>>> Note that the final patch can only be merged after out-of-tree platforms
>>> have switched from ArmDmaLib to NonCoherentDmaLib.
>>
>> For 4-6/6:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>
> Thanks,
>
> #1 - #5 merged as
>
> 7385d2543e2a EmbeddedPkg: rename NullDmaLib to CoherentDmaLib
> 0bcb80106762 EmbeddedPkg/CoherentDmaLib: add support for non-1:1 DMA translation
> 723102c72fb0 EmbeddedPkg: implement NonCoherentDmaLib based on ArmDmaLib
> c878cd95e132 Omap35xxPkg: switch to EmbeddedPkg's NonCoherentDmaLib
> 877f4460b3e3 BeagleBoardPkg: switch to generic non-coherent DmaLib
>
> #6 needs to wait until the edk2-platforms changes are in.

#6 pushed as 63ed4d2757eb

Thanks.