* [PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA
@ 2019-11-25 23:12 Ard Biesheuvel
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-25 23:12 ` [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
0 siblings, 2 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 23:12 UTC (permalink / raw)
To: devel
Cc: leif.lindholm, Ard Biesheuvel, awarkentin, jeremy.linton, pete,
samer.el-haj-mahmoud
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 23:12 [PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
@ 2019-11-25 23:12 ` Ard Biesheuvel
2019-11-26 10:28 ` [edk2-devel] " Pete Batard
` (2 more replies)
2019-11-25 23:12 ` [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
1 sibling, 3 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 23:12 UTC (permalink / raw)
To: devel; +Cc: leif.lindholm, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-25 23:12 [PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
@ 2019-11-25 23:12 ` Ard Biesheuvel
2019-11-26 8:19 ` [edk2-devel] " Philippe Mathieu-Daudé
1 sibling, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 23:12 UTC (permalink / raw)
To: devel; +Cc: leif.lindholm, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-25 23:12 ` [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
@ 2019-11-26 8:19 ` Philippe Mathieu-Daudé
2019-11-26 10:28 ` Pete Batard
0 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-26 8:19 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif.lindholm
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
@ 2019-11-26 10:28 ` Pete Batard
2019-11-26 10:44 ` Philippe Mathieu-Daudé
2019-11-26 12:04 ` Leif Lindholm
2 siblings, 0 replies; 12+ messages in thread
From: Pete Batard @ 2019-11-26 10:28 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif.lindholm
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-26 8:19 ` [edk2-devel] " Philippe Mathieu-Daudé
@ 2019-11-26 10:28 ` Pete Batard
0 siblings, 0 replies; 12+ messages in thread
From: Pete Batard @ 2019-11-26 10:28 UTC (permalink / raw)
To: devel, philmd, ard.biesheuvel; +Cc: leif.lindholm
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-26 10:28 ` [edk2-devel] " Pete Batard
@ 2019-11-26 10:44 ` Philippe Mathieu-Daudé
2019-11-26 11:59 ` Philippe Mathieu-Daudé
2019-11-26 12:04 ` Leif Lindholm
2 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-26 10:44 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif.lindholm
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-26 10:44 ` Philippe Mathieu-Daudé
@ 2019-11-26 11:59 ` Philippe Mathieu-Daudé
2019-11-26 12:02 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-11-26 11:59 UTC (permalink / raw)
To: devel, ard.biesheuvel; +Cc: leif.lindholm
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [edk2-devel] [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-26 11:59 ` Philippe Mathieu-Daudé
@ 2019-11-26 12:02 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2019-11-26 12:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: edk2-devel-groups-io, Leif Lindholm
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-26 10:28 ` [edk2-devel] " Pete Batard
2019-11-26 10:44 ` Philippe Mathieu-Daudé
@ 2019-11-26 12:04 ` Leif Lindholm
2019-11-26 12:11 ` Ard Biesheuvel
2 siblings, 1 reply; 12+ messages in thread
From: Leif Lindholm @ 2019-11-26 12:04 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-26 12:04 ` Leif Lindholm
@ 2019-11-26 12:11 ` Ard Biesheuvel
2019-11-26 12:22 ` Leif Lindholm
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2019-11-26 12:11 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-26 12:11 ` Ard Biesheuvel
@ 2019-11-26 12:22 ` Leif Lindholm
0 siblings, 0 replies; 12+ messages in thread
From: Leif Lindholm @ 2019-11-26 12:22 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2019-11-26 12:22 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-25 23:12 [PATCH v2 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
2019-11-25 23:12 ` [PATCH v2 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-26 10:28 ` [edk2-devel] " Pete Batard
2019-11-26 10:44 ` Philippe Mathieu-Daudé
2019-11-26 11:59 ` Philippe Mathieu-Daudé
2019-11-26 12:02 ` Ard Biesheuvel
2019-11-26 12:04 ` Leif Lindholm
2019-11-26 12:11 ` Ard Biesheuvel
2019-11-26 12:22 ` Leif Lindholm
2019-11-25 23:12 ` [PATCH v2 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
2019-11-26 8:19 ` [edk2-devel] " Philippe Mathieu-Daudé
2019-11-26 10:28 ` Pete Batard
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox