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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51253): https://edk2.groups.io/g/devel/message/51253
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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> -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#51269): https://edk2.groups.io/g/devel/message/51269 Mute This Topic: https://groups.io/mt/61950463/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
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
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51279): https://edk2.groups.io/g/devel/message/51279
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51280): https://edk2.groups.io/g/devel/message/51280
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51281): https://edk2.groups.io/g/devel/message/51281
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51271): https://edk2.groups.io/g/devel/message/51271
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51277): https://edk2.groups.io/g/devel/message/51277
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
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!
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#51278): https://edk2.groups.io/g/devel/message/51278
Mute This Topic: https://groups.io/mt/61950463/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2026 Red Hat, Inc.