* [PATCH 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA
@ 2019-11-21 8:32 Ard Biesheuvel
2019-11-21 8:32 ` [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-21 8:32 ` [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
0 siblings, 2 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-21 8:32 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.
Patch #1 is build tested only. Patch #2 was tested on actual h/w.
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 | 176 ++++++++++--
.../NonCoherentDmaLib/NonCoherentDmaLib.inf | 1 +
6 files changed, 463 insertions(+), 25 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] 10+ messages in thread
* [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-21 8:32 [PATCH 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
@ 2019-11-21 8:32 ` Ard Biesheuvel
2019-11-25 12:46 ` Leif Lindholm
2019-11-21 8:32 ` [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
1 sibling, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-21 8:32 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 | 176 +++++++++++++++++---
EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf | 1 +
3 files changed, 158 insertions(+), 25 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..dd3d111e7eb9 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,22 @@ 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;
+ }
+ *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
+ } else if (Operation != MapOperationBusMasterRead &&
((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
@@ -128,12 +241,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;
}
//
@@ -154,14 +262,6 @@ DmaMap (
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;
}
@@ -184,13 +284,13 @@ DmaMap (
(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);
}
+ // Flush the Data Cache (should not have any effect if the memory region is
+ // uncached)
+ mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
+ EfiCpuFlushTypeWriteBack);
+
Map->HostAddress = (UINTN)HostAddress;
Map->NumberOfBytes = *NumberOfBytes;
Map->Operation = Operation;
@@ -199,6 +299,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 +335,7 @@ DmaUnmap (
MAP_INFO_INSTANCE *Map;
EFI_STATUS Status;
VOID *Buffer;
+ UINTN AllocSize;
if (Mapping == NULL) {
ASSERT (FALSE);
@@ -238,7 +345,18 @@ DmaUnmap (
Map = (MAP_INFO_INSTANCE *)Mapping;
Status = EFI_SUCCESS;
- if (Map->DoubleBuffer) {
+ if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
+
+ mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, Map->NumberOfBytes,
+ EfiCpuFlushTypeInvalidate);
+
+ CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
+ Map->NumberOfBytes);
+
+ AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
+ FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
+ } else if (Map->DoubleBuffer) {
+
ASSERT (Map->Operation == MapOperationBusMasterWrite);
if (Map->Operation != MapOperationBusMasterWrite) {
@@ -335,10 +453,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 +596,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] 10+ messages in thread
* [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-21 8:32 [PATCH 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
2019-11-21 8:32 ` [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
@ 2019-11-21 8:32 ` Ard Biesheuvel
2019-11-25 12:49 ` Leif Lindholm
1 sibling, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-21 8:32 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>
---
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] 10+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-21 8:32 ` [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
@ 2019-11-25 12:46 ` Leif Lindholm
2019-11-25 12:52 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2019-11-25 12:46 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
On Thu, Nov 21, 2019 at 09:32:26 +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 | 176 +++++++++++++++++---
> EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf | 1 +
> 3 files changed, 158 insertions(+), 25 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..dd3d111e7eb9 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,22 @@ 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;
> + }
> + *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> + } else if (Operation != MapOperationBusMasterRead &&
> ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>
> @@ -128,12 +241,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;
> }
>
> //
> @@ -154,14 +262,6 @@ DmaMap (
>
> 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;
> }
> @@ -184,13 +284,13 @@ DmaMap (
> (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);
> }
>
> + // Flush the Data Cache (should not have any effect if the memory region is
> + // uncached)
> + mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
> + EfiCpuFlushTypeWriteBack);
> +
> Map->HostAddress = (UINTN)HostAddress;
> Map->NumberOfBytes = *NumberOfBytes;
> Map->Operation = Operation;
> @@ -199,6 +299,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 +335,7 @@ DmaUnmap (
> MAP_INFO_INSTANCE *Map;
> EFI_STATUS Status;
> VOID *Buffer;
> + UINTN AllocSize;
>
> if (Mapping == NULL) {
> ASSERT (FALSE);
> @@ -238,7 +345,18 @@ DmaUnmap (
> Map = (MAP_INFO_INSTANCE *)Mapping;
>
> Status = EFI_SUCCESS;
> - if (Map->DoubleBuffer) {
> + if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
> +
> + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, Map->NumberOfBytes,
> + EfiCpuFlushTypeInvalidate);
> +
> + CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> + Map->NumberOfBytes);
> +
> + AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
> + FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> + } else if (Map->DoubleBuffer) {
> +
> ASSERT (Map->Operation == MapOperationBusMasterWrite);
>
> if (Map->Operation != MapOperationBusMasterWrite) {
> @@ -335,10 +453,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 +596,15 @@ NonCoherentDmaLibConstructor (
> {
> InitializeListHead (&UncachedAllocationList);
>
> + //
> + // Ensure that the combination of DMA addressing offset and limit produces
> + // a sane value.
> + //
> + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
Is this worth turning into a hard conditional and error return rather
than an assert? The following statement will end up wrapping downwards
if this condition is not true.
> +
> + mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> + PcdGet64 (PcdDmaDeviceOffset);
> +
/
Leif
> // 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] 10+ messages in thread
* Re: [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-21 8:32 ` [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
@ 2019-11-25 12:49 ` Leif Lindholm
2019-11-25 12:53 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2019-11-25 12:49 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: devel
On Thu, Nov 21, 2019 at 09:32:27 +0100, Ard Biesheuvel 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(+)
>
> 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 12:46 ` Leif Lindholm
@ 2019-11-25 12:52 ` Ard Biesheuvel
2019-11-25 12:58 ` Leif Lindholm
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 12:52 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io
On Mon, 25 Nov 2019 at 13:46, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 09:32:26 +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 | 176 +++++++++++++++++---
> > EmbeddedPkg/Library/NonCoherentDmaLib/NonCoherentDmaLib.inf | 1 +
> > 3 files changed, 158 insertions(+), 25 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..dd3d111e7eb9 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,22 @@ 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;
> > + }
> > + *DeviceAddress = HostToDeviceAddress (Map->BufferAddress);
> > + } else if (Operation != MapOperationBusMasterRead &&
> > ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> > ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
> >
> > @@ -128,12 +241,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;
> > }
> >
> > //
> > @@ -154,14 +262,6 @@ DmaMap (
> >
> > 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;
> > }
> > @@ -184,13 +284,13 @@ DmaMap (
> > (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);
> > }
> >
> > + // Flush the Data Cache (should not have any effect if the memory region is
> > + // uncached)
> > + mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
> > + EfiCpuFlushTypeWriteBack);
> > +
> > Map->HostAddress = (UINTN)HostAddress;
> > Map->NumberOfBytes = *NumberOfBytes;
> > Map->Operation = Operation;
> > @@ -199,6 +299,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 +335,7 @@ DmaUnmap (
> > MAP_INFO_INSTANCE *Map;
> > EFI_STATUS Status;
> > VOID *Buffer;
> > + UINTN AllocSize;
> >
> > if (Mapping == NULL) {
> > ASSERT (FALSE);
> > @@ -238,7 +345,18 @@ DmaUnmap (
> > Map = (MAP_INFO_INSTANCE *)Mapping;
> >
> > Status = EFI_SUCCESS;
> > - if (Map->DoubleBuffer) {
> > + if (((UINTN)Map->HostAddress + Map->NumberOfBytes) > mDmaHostAddressLimit) {
> > +
> > + mCpu->FlushDataCache (mCpu, (UINTN)Map->BufferAddress, Map->NumberOfBytes,
> > + EfiCpuFlushTypeInvalidate);
> > +
> > + CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> > + Map->NumberOfBytes);
> > +
> > + AllocSize = ALIGN_VALUE (Map->NumberOfBytes, mCpu->DmaBufferAlignment);
> > + FreePages (Map->BufferAddress, EFI_SIZE_TO_PAGES (AllocSize));
> > + } else if (Map->DoubleBuffer) {
> > +
> > ASSERT (Map->Operation == MapOperationBusMasterWrite);
> >
> > if (Map->Operation != MapOperationBusMasterWrite) {
> > @@ -335,10 +453,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 +596,15 @@ NonCoherentDmaLibConstructor (
> > {
> > InitializeListHead (&UncachedAllocationList);
> >
> > + //
> > + // Ensure that the combination of DMA addressing offset and limit produces
> > + // a sane value.
> > + //
> > + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
>
> Is this worth turning into a hard conditional and error return rather
> than an assert? The following statement will end up wrapping downwards
> if this condition is not true.
>
Constructor return values are only used in ASSERT_EFI_ERROR() calls in
the ProcessLibraryConstructorList() routine that gets generated by the
build tools (in AutoGen.c), so returning an error here would
essentially be dead code, given that we would only make it to this
point on builds that are known to ignore it.
> > +
> > + mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> > + PcdGet64 (PcdDmaDeviceOffset);
> > +
>
> /
> Leif
>
> > // 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] 10+ messages in thread
* Re: [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib
2019-11-25 12:49 ` Leif Lindholm
@ 2019-11-25 12:53 ` Ard Biesheuvel
0 siblings, 0 replies; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 12:53 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io
On Mon, 25 Nov 2019 at 13:49, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Thu, Nov 21, 2019 at 09:32:27 +0100, Ard Biesheuvel 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>
>
Cheers.
Note that 1/2 of this set actually has problems, so I'll resend shortly.
> > ---
> > 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 12:52 ` Ard Biesheuvel
@ 2019-11-25 12:58 ` Leif Lindholm
2019-11-25 13:13 ` Ard Biesheuvel
0 siblings, 1 reply; 10+ messages in thread
From: Leif Lindholm @ 2019-11-25 12:58 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io
On Mon, Nov 25, 2019 at 13:52:24 +0100, Ard Biesheuvel wrote:
> > > - 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 +596,15 @@ NonCoherentDmaLibConstructor (
> > > {
> > > InitializeListHead (&UncachedAllocationList);
> > >
> > > + //
> > > + // Ensure that the combination of DMA addressing offset and limit produces
> > > + // a sane value.
> > > + //
> > > + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> >
> > Is this worth turning into a hard conditional and error return rather
> > than an assert? The following statement will end up wrapping downwards
> > if this condition is not true.
>
>
> Constructor return values are only used in ASSERT_EFI_ERROR() calls in
> the ProcessLibraryConstructorList() routine that gets generated by the
> build tools (in AutoGen.c), so returning an error here would
> essentially be dead code, given that we would only make it to this
> point on builds that are known to ignore it.
Hmm, ack.
Print error message?
/
Leif
> > > +
> > > + mDmaHostAddressLimit = PcdGet64 (PcdDmaDeviceLimit) -
> > > + PcdGet64 (PcdDmaDeviceOffset);
> > > +
> >
> > /
> > Leif
> >
> > > // 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] 10+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 12:58 ` Leif Lindholm
@ 2019-11-25 13:13 ` Ard Biesheuvel
2019-11-25 14:38 ` Leif Lindholm
0 siblings, 1 reply; 10+ messages in thread
From: Ard Biesheuvel @ 2019-11-25 13:13 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel-groups-io
On Mon, 25 Nov 2019 at 13:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Mon, Nov 25, 2019 at 13:52:24 +0100, Ard Biesheuvel wrote:
> > > > - 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 +596,15 @@ NonCoherentDmaLibConstructor (
> > > > {
> > > > InitializeListHead (&UncachedAllocationList);
> > > >
> > > > + //
> > > > + // Ensure that the combination of DMA addressing offset and limit produces
> > > > + // a sane value.
> > > > + //
> > > > + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> > >
> > > Is this worth turning into a hard conditional and error return rather
> > > than an assert? The following statement will end up wrapping downwards
> > > if this condition is not true.
> >
> >
> > Constructor return values are only used in ASSERT_EFI_ERROR() calls in
> > the ProcessLibraryConstructorList() routine that gets generated by the
> > build tools (in AutoGen.c), so returning an error here would
> > essentially be dead code, given that we would only make it to this
> > point on builds that are known to ignore it.
>
> Hmm, ack.
> Print error message?
>
How? Using DEBUG()? That would be dead code as well, since it is only
active in DEBUG builds, which means the ASSERT() would fire first and
prevent the DEBUG() from ever being reached.
So that leaves printing to the console using boot services, which is
not something we should be doing from a DXE_DRIVER type module.
Given that these are both platform specific values defined at platform
creation time, I think policing them only in a DEBUG build is quite
appropriate, tbh.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits
2019-11-25 13:13 ` Ard Biesheuvel
@ 2019-11-25 14:38 ` Leif Lindholm
0 siblings, 0 replies; 10+ messages in thread
From: Leif Lindholm @ 2019-11-25 14:38 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel-groups-io
On Mon, Nov 25, 2019 at 14:13:19 +0100, Ard Biesheuvel wrote:
> On Mon, 25 Nov 2019 at 13:58, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Mon, Nov 25, 2019 at 13:52:24 +0100, Ard Biesheuvel wrote:
> > > > > - 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 +596,15 @@ NonCoherentDmaLibConstructor (
> > > > > {
> > > > > InitializeListHead (&UncachedAllocationList);
> > > > >
> > > > > + //
> > > > > + // Ensure that the combination of DMA addressing offset and limit produces
> > > > > + // a sane value.
> > > > > + //
> > > > > + ASSERT (PcdGet64 (PcdDmaDeviceLimit) > PcdGet64 (PcdDmaDeviceOffset));
> > > >
> > > > Is this worth turning into a hard conditional and error return rather
> > > > than an assert? The following statement will end up wrapping downwards
> > > > if this condition is not true.
> > >
> > >
> > > Constructor return values are only used in ASSERT_EFI_ERROR() calls in
> > > the ProcessLibraryConstructorList() routine that gets generated by the
> > > build tools (in AutoGen.c), so returning an error here would
> > > essentially be dead code, given that we would only make it to this
> > > point on builds that are known to ignore it.
> >
> > Hmm, ack.
> > Print error message?
>
> How? Using DEBUG()? That would be dead code as well, since it is only
> active in DEBUG builds, which means the ASSERT() would fire first and
> prevent the DEBUG() from ever being reached.
Clearly not.
> So that leaves printing to the console using boot services, which is
> not something we should be doing from a DXE_DRIVER type module.
Yeah, maybe that's too urgh.
> Given that these are both platform specific values defined at platform
> creation time, I think policing them only in a DEBUG build is quite
> appropriate, tbh.
I dunno, it just feels to me like it's following the pattern of the
ArmPlatformGetVirtualMemoryMap() implementations (which do the
same). This is a hard error detectable at compile time.
My preferred way to handle it would really be a compilation error, but
that brings us back to last week's discussion on runtime
vs. preprocessor...
/
Leif
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-11-25 14:38 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-11-21 8:32 [PATCH 0/2] EmbeddedPkg: support for RPi4 PCI and platform DMA Ard Biesheuvel
2019-11-21 8:32 ` [PATCH 1/2] EmbeddedPkg/NonCoherentDmaLib: implement support for DMA range limits Ard Biesheuvel
2019-11-25 12:46 ` Leif Lindholm
2019-11-25 12:52 ` Ard Biesheuvel
2019-11-25 12:58 ` Leif Lindholm
2019-11-25 13:13 ` Ard Biesheuvel
2019-11-25 14:38 ` Leif Lindholm
2019-11-21 8:32 ` [PATCH 2/2] EmbeddedPkg: implement EDK2 IoMmu protocol wrapping DmaLib Ard Biesheuvel
2019-11-25 12:49 ` Leif Lindholm
2019-11-25 12:53 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox