* [PATCH v2 1/4] ArmPkg/ArmDmaLib: use DMA buffer alignment from CPU arch protocol
2016-11-12 13:02 [PATCH v2 0/4] ArmPkg: more ArmDmaLib fixes Ard Biesheuvel
@ 2016-11-12 13:02 ` Ard Biesheuvel
2016-11-14 15:18 ` Leif Lindholm
2016-11-12 13:02 ` [PATCH v2 2/4] ArmPkg/ArmDmaLib: fix incorrect device address of double buffer Ard Biesheuvel
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-12 13:02 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
Instead of depending on ArmLib to retrieve the CWG directly, use
the DMA buffer alignment exposed by the CPU arch protocol. This
removes our dependency on ArmLib, which makes the library a bit
more architecture independent.
While we're in there, rename gCpu to mCpu to better reflect its
local scope, and reflow some lines that we're modifying anyway.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 18 ++++++++----------
ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf | 2 --
2 files changed, 8 insertions(+), 12 deletions(-)
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index d48d6ff6dbbb..03fd9f3278e6 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -22,7 +22,6 @@
#include <Library/UncachedMemoryAllocationLib.h>
#include <Library/IoLib.h>
#include <Library/BaseMemoryLib.h>
-#include <Library/ArmLib.h>
#include <Protocol/Cpu.h>
@@ -36,8 +35,7 @@ typedef struct {
-EFI_CPU_ARCH_PROTOCOL *gCpu;
-UINTN gCacheAlignment = 0;
+STATIC EFI_CPU_ARCH_PROTOCOL *mCpu;
/**
Provides the DMA controller-specific addresses needed to access system memory.
@@ -92,8 +90,8 @@ DmaMap (
*Mapping = Map;
- if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
- ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
+ if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
+ ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
// Get the cacheability of the region
Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
@@ -154,7 +152,8 @@ DmaMap (
DEBUG_CODE_END ();
// Flush the Data Cache (should not have any effect if the memory region is uncached)
- gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, EfiCpuFlushTypeWriteBackInvalidate);
+ mCpu->FlushDataCache (mCpu, *DeviceAddress, *NumberOfBytes,
+ EfiCpuFlushTypeWriteBackInvalidate);
}
Map->HostAddress = (UINTN)HostAddress;
@@ -211,7 +210,8 @@ DmaUnmap (
//
// Make sure we read buffer from uncached memory and not the cache
//
- gCpu->FlushDataCache (gCpu, Map->HostAddress, Map->NumberOfBytes, EfiCpuFlushTypeInvalidate);
+ mCpu->FlushDataCache (mCpu, Map->HostAddress, Map->NumberOfBytes,
+ EfiCpuFlushTypeInvalidate);
}
}
@@ -311,11 +311,9 @@ ArmDmaLibConstructor (
EFI_STATUS Status;
// Get the Cpu protocol for later use
- Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu);
+ Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
ASSERT_EFI_ERROR(Status);
- gCacheAlignment = ArmCacheWritebackGranule ();
-
return Status;
}
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
index 95c13006eaac..31de3cfd828c 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
@@ -37,8 +37,6 @@ [LibraryClasses]
UncachedMemoryAllocationLib
IoLib
BaseMemoryLib
- ArmLib
-
[Protocols]
gEfiCpuArchProtocolGuid
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/4] ArmPkg/ArmDmaLib: use DMA buffer alignment from CPU arch protocol
2016-11-12 13:02 ` [PATCH v2 1/4] ArmPkg/ArmDmaLib: use DMA buffer alignment from CPU arch protocol Ard Biesheuvel
@ 2016-11-14 15:18 ` Leif Lindholm
0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2016-11-14 15:18 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Sat, Nov 12, 2016 at 02:02:25PM +0100, Ard Biesheuvel wrote:
> Instead of depending on ArmLib to retrieve the CWG directly, use
> the DMA buffer alignment exposed by the CPU arch protocol. This
> removes our dependency on ArmLib, which makes the library a bit
> more architecture independent.
>
> While we're in there, rename gCpu to mCpu to better reflect its
> local scope, and reflow some lines that we're modifying anyway.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 18 ++++++++----------
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf | 2 --
> 2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index d48d6ff6dbbb..03fd9f3278e6 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -22,7 +22,6 @@
> #include <Library/UncachedMemoryAllocationLib.h>
> #include <Library/IoLib.h>
> #include <Library/BaseMemoryLib.h>
> -#include <Library/ArmLib.h>
>
> #include <Protocol/Cpu.h>
>
> @@ -36,8 +35,7 @@ typedef struct {
>
>
>
> -EFI_CPU_ARCH_PROTOCOL *gCpu;
> -UINTN gCacheAlignment = 0;
> +STATIC EFI_CPU_ARCH_PROTOCOL *mCpu;
>
> /**
> Provides the DMA controller-specific addresses needed to access system memory.
> @@ -92,8 +90,8 @@ DmaMap (
>
> *Mapping = Map;
>
> - if ((((UINTN)HostAddress & (gCacheAlignment - 1)) != 0) ||
> - ((*NumberOfBytes & (gCacheAlignment - 1)) != 0)) {
> + if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
>
> // Get the cacheability of the region
> Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
> @@ -154,7 +152,8 @@ DmaMap (
> DEBUG_CODE_END ();
>
> // Flush the Data Cache (should not have any effect if the memory region is uncached)
> - gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, EfiCpuFlushTypeWriteBackInvalidate);
> + mCpu->FlushDataCache (mCpu, *DeviceAddress, *NumberOfBytes,
> + EfiCpuFlushTypeWriteBackInvalidate);
> }
>
> Map->HostAddress = (UINTN)HostAddress;
> @@ -211,7 +210,8 @@ DmaUnmap (
> //
> // Make sure we read buffer from uncached memory and not the cache
> //
> - gCpu->FlushDataCache (gCpu, Map->HostAddress, Map->NumberOfBytes, EfiCpuFlushTypeInvalidate);
> + mCpu->FlushDataCache (mCpu, Map->HostAddress, Map->NumberOfBytes,
> + EfiCpuFlushTypeInvalidate);
> }
> }
>
> @@ -311,11 +311,9 @@ ArmDmaLibConstructor (
> EFI_STATUS Status;
>
> // Get the Cpu protocol for later use
> - Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&gCpu);
> + Status = gBS->LocateProtocol (&gEfiCpuArchProtocolGuid, NULL, (VOID **)&mCpu);
> ASSERT_EFI_ERROR(Status);
>
> - gCacheAlignment = ArmCacheWritebackGranule ();
> -
> return Status;
> }
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> index 95c13006eaac..31de3cfd828c 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> @@ -37,8 +37,6 @@ [LibraryClasses]
> UncachedMemoryAllocationLib
> IoLib
> BaseMemoryLib
> - ArmLib
> -
>
> [Protocols]
> gEfiCpuArchProtocolGuid
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/4] ArmPkg/ArmDmaLib: fix incorrect device address of double buffer
2016-11-12 13:02 [PATCH v2 0/4] ArmPkg: more ArmDmaLib fixes Ard Biesheuvel
2016-11-12 13:02 ` [PATCH v2 1/4] ArmPkg/ArmDmaLib: use DMA buffer alignment from CPU arch protocol Ard Biesheuvel
@ 2016-11-12 13:02 ` Ard Biesheuvel
2016-11-14 15:13 ` Leif Lindholm
2016-11-12 13:02 ` [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address Ard Biesheuvel
2016-11-12 13:02 ` [PATCH v2 4/4] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset Ard Biesheuvel
3 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-12 13:02 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
If double buffering is not required in DmaMap(), the returned device
address is passed through ConvertToPhysicalAddress () to convert the
host address (which in case of DebugUncachedMemoryAllocationLib is not
1:1 mapped) to a physical address, which is what a device would expect
to be able to perform DMA.
By the same reasoning, a double buffer allocated using DmaAllocateBuffer ()
should be converted in the same way, considering that the buffer is allocated
using UncachedAllocatePages (), to which the above equally applies.
So add the missing ConvertToPhysicalAddress () invocation.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index 03fd9f3278e6..c2a44398d25a 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -127,7 +127,7 @@ DmaMap (
CopyMem (Buffer, HostAddress, *NumberOfBytes);
}
- *DeviceAddress = (PHYSICAL_ADDRESS)(UINTN)Buffer;
+ *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
} else {
Map->DoubleBuffer = FALSE;
}
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/4] ArmPkg/ArmDmaLib: fix incorrect device address of double buffer
2016-11-12 13:02 ` [PATCH v2 2/4] ArmPkg/ArmDmaLib: fix incorrect device address of double buffer Ard Biesheuvel
@ 2016-11-14 15:13 ` Leif Lindholm
0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2016-11-14 15:13 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Sat, Nov 12, 2016 at 02:02:26PM +0100, Ard Biesheuvel wrote:
> If double buffering is not required in DmaMap(), the returned device
> address is passed through ConvertToPhysicalAddress () to convert the
> host address (which in case of DebugUncachedMemoryAllocationLib is not
> 1:1 mapped) to a physical address, which is what a device would expect
> to be able to perform DMA.
>
> By the same reasoning, a double buffer allocated using DmaAllocateBuffer ()
> should be converted in the same way, considering that the buffer is allocated
> using UncachedAllocatePages (), to which the above equally applies.
>
> So add the missing ConvertToPhysicalAddress () invocation.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index 03fd9f3278e6..c2a44398d25a 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -127,7 +127,7 @@ DmaMap (
> CopyMem (Buffer, HostAddress, *NumberOfBytes);
> }
>
> - *DeviceAddress = (PHYSICAL_ADDRESS)(UINTN)Buffer;
> + *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
> } else {
> Map->DoubleBuffer = FALSE;
> }
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-12 13:02 [PATCH v2 0/4] ArmPkg: more ArmDmaLib fixes Ard Biesheuvel
2016-11-12 13:02 ` [PATCH v2 1/4] ArmPkg/ArmDmaLib: use DMA buffer alignment from CPU arch protocol Ard Biesheuvel
2016-11-12 13:02 ` [PATCH v2 2/4] ArmPkg/ArmDmaLib: fix incorrect device address of double buffer Ard Biesheuvel
@ 2016-11-12 13:02 ` Ard Biesheuvel
2016-11-14 15:16 ` Leif Lindholm
2016-11-12 13:02 ` [PATCH v2 4/4] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset Ard Biesheuvel
3 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-12 13:02 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
In preparation of adding support to ArmDmalib for DMA bus masters whose
view of memory is offset by a constant compared to the CPU's view, clean
up some abuse of the device address.
The device address is not defined in terms of the CPU's address space,
and so it should not be used in CopyMem () or cache maintenance operations
that require a valid mapping. This not only applies to the above use case,
but also to the DebugUncachedMemoryAllocationLib that unmaps the
primary, cached mapping of an allocation, and returns a host address
which is an uncached alias offset by a constant.
Since we should never access the device address from the CPU, there is
no need to record it in the MAPINFO struct. Instead, record the buffer
address in case of double buffering, since we do need to copy the contents
(in case of a bus master write) and free the buffer (in all cases) when
DmaUnmap() is called.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index c2a44398d25a..7321388de63e 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -27,7 +27,7 @@
typedef struct {
EFI_PHYSICAL_ADDRESS HostAddress;
- EFI_PHYSICAL_ADDRESS DeviceAddress;
+ VOID *BufferAddress;
UINTN NumberOfBytes;
DMA_MAP_OPERATION Operation;
BOOLEAN DoubleBuffer;
@@ -94,7 +94,7 @@ DmaMap (
((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
// Get the cacheability of the region
- Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
+ Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
if (EFI_ERROR(Status)) {
return Status;
}
@@ -128,6 +128,7 @@ DmaMap (
}
*DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
+ Map->BufferAddress = Buffer;
} else {
Map->DoubleBuffer = FALSE;
}
@@ -143,7 +144,7 @@ DmaMap (
// So duplicate the check here when running in DEBUG mode, just to assert
// that we are not trying to create a consistent mapping for cached memory.
//
- Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
+ Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
ASSERT_EFI_ERROR(Status);
ASSERT (Operation != MapOperationBusMasterCommonBuffer ||
@@ -152,12 +153,11 @@ DmaMap (
DEBUG_CODE_END ();
// Flush the Data Cache (should not have any effect if the memory region is uncached)
- mCpu->FlushDataCache (mCpu, *DeviceAddress, *NumberOfBytes,
+ mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
EfiCpuFlushTypeWriteBackInvalidate);
}
Map->HostAddress = (UINTN)HostAddress;
- Map->DeviceAddress = *DeviceAddress;
Map->NumberOfBytes = *NumberOfBytes;
Map->Operation = Operation;
@@ -200,10 +200,11 @@ DmaUnmap (
if (Map->Operation == MapOperationBusMasterCommonBuffer) {
Status = EFI_INVALID_PARAMETER;
} else if (Map->Operation == MapOperationBusMasterWrite) {
- CopyMem ((VOID *)(UINTN)Map->HostAddress, (VOID *)(UINTN)Map->DeviceAddress, Map->NumberOfBytes);
+ CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
+ Map->NumberOfBytes);
}
- DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), (VOID *)(UINTN)Map->DeviceAddress);
+ DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress);
} else {
if (Map->Operation == MapOperationBusMasterWrite) {
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-12 13:02 ` [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address Ard Biesheuvel
@ 2016-11-14 15:16 ` Leif Lindholm
2016-11-15 9:19 ` Ard Biesheuvel
0 siblings, 1 reply; 14+ messages in thread
From: Leif Lindholm @ 2016-11-14 15:16 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote:
> In preparation of adding support to ArmDmalib for DMA bus masters whose
> view of memory is offset by a constant compared to the CPU's view, clean
> up some abuse of the device address.
>
> The device address is not defined in terms of the CPU's address space,
> and so it should not be used in CopyMem () or cache maintenance operations
> that require a valid mapping. This not only applies to the above use case,
> but also to the DebugUncachedMemoryAllocationLib that unmaps the
> primary, cached mapping of an allocation, and returns a host address
> which is an uncached alias offset by a constant.
>
> Since we should never access the device address from the CPU, there is
> no need to record it in the MAPINFO struct. Instead, record the buffer
> address in case of double buffering, since we do need to copy the contents
> (in case of a bus master write) and free the buffer (in all cases) when
> DmaUnmap() is called.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
For the fix itself:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
However, can we wait for a few Tested-by:s to ensure this fix does not
reveal any companion bugs?
> ---
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index c2a44398d25a..7321388de63e 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -27,7 +27,7 @@
>
> typedef struct {
> EFI_PHYSICAL_ADDRESS HostAddress;
> - EFI_PHYSICAL_ADDRESS DeviceAddress;
> + VOID *BufferAddress;
> UINTN NumberOfBytes;
> DMA_MAP_OPERATION Operation;
> BOOLEAN DoubleBuffer;
> @@ -94,7 +94,7 @@ DmaMap (
> ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
>
> // Get the cacheability of the region
> - Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
> + Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
> if (EFI_ERROR(Status)) {
> return Status;
> }
> @@ -128,6 +128,7 @@ DmaMap (
> }
>
> *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
> + Map->BufferAddress = Buffer;
> } else {
> Map->DoubleBuffer = FALSE;
> }
> @@ -143,7 +144,7 @@ DmaMap (
> // So duplicate the check here when running in DEBUG mode, just to assert
> // that we are not trying to create a consistent mapping for cached memory.
> //
> - Status = gDS->GetMemorySpaceDescriptor (*DeviceAddress, &GcdDescriptor);
> + Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
> ASSERT_EFI_ERROR(Status);
>
> ASSERT (Operation != MapOperationBusMasterCommonBuffer ||
> @@ -152,12 +153,11 @@ DmaMap (
> DEBUG_CODE_END ();
>
> // Flush the Data Cache (should not have any effect if the memory region is uncached)
> - mCpu->FlushDataCache (mCpu, *DeviceAddress, *NumberOfBytes,
> + mCpu->FlushDataCache (mCpu, (UINTN)HostAddress, *NumberOfBytes,
> EfiCpuFlushTypeWriteBackInvalidate);
> }
>
> Map->HostAddress = (UINTN)HostAddress;
> - Map->DeviceAddress = *DeviceAddress;
> Map->NumberOfBytes = *NumberOfBytes;
> Map->Operation = Operation;
>
> @@ -200,10 +200,11 @@ DmaUnmap (
> if (Map->Operation == MapOperationBusMasterCommonBuffer) {
> Status = EFI_INVALID_PARAMETER;
> } else if (Map->Operation == MapOperationBusMasterWrite) {
> - CopyMem ((VOID *)(UINTN)Map->HostAddress, (VOID *)(UINTN)Map->DeviceAddress, Map->NumberOfBytes);
> + CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> + Map->NumberOfBytes);
> }
>
> - DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), (VOID *)(UINTN)Map->DeviceAddress);
> + DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress);
>
> } else {
> if (Map->Operation == MapOperationBusMasterWrite) {
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-14 15:16 ` Leif Lindholm
@ 2016-11-15 9:19 ` Ard Biesheuvel
2016-11-15 11:34 ` Ryan Harkin
0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-15 9:19 UTC (permalink / raw)
To: Leif Lindholm, Ryan Harkin, Jeremy Linton; +Cc: edk2-devel-01
On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote:
>> In preparation of adding support to ArmDmalib for DMA bus masters whose
>> view of memory is offset by a constant compared to the CPU's view, clean
>> up some abuse of the device address.
>>
>> The device address is not defined in terms of the CPU's address space,
>> and so it should not be used in CopyMem () or cache maintenance operations
>> that require a valid mapping. This not only applies to the above use case,
>> but also to the DebugUncachedMemoryAllocationLib that unmaps the
>> primary, cached mapping of an allocation, and returns a host address
>> which is an uncached alias offset by a constant.
>>
>> Since we should never access the device address from the CPU, there is
>> no need to record it in the MAPINFO struct. Instead, record the buffer
>> address in case of double buffering, since we do need to copy the contents
>> (in case of a bus master write) and free the buffer (in all cases) when
>> DmaUnmap() is called.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> For the fix itself:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
> However, can we wait for a few Tested-by:s to ensure this fix does not
> reveal any companion bugs?
>
Perhaps, yes.
In case anyone is up to doing that, please find the branch here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset
However, given that the split CPU/bus master view is introduced in the
next patch, the only use case where the device address differs from
the host address is when using the DebugUncachedMemoryAllocationLib,
which is currently broken AFAICT (it attempts to unmap the linear
mapping of the allocation by setting the memory attributes to '0',
which triggers an assert in the ArmPkg MMU code)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-15 9:19 ` Ard Biesheuvel
@ 2016-11-15 11:34 ` Ryan Harkin
2016-11-15 13:07 ` Ard Biesheuvel
0 siblings, 1 reply; 14+ messages in thread
From: Ryan Harkin @ 2016-11-15 11:34 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Leif Lindholm, Jeremy Linton, edk2-devel-01
On 15 November 2016 at 09:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote:
>>> In preparation of adding support to ArmDmalib for DMA bus masters whose
>>> view of memory is offset by a constant compared to the CPU's view, clean
>>> up some abuse of the device address.
>>>
>>> The device address is not defined in terms of the CPU's address space,
>>> and so it should not be used in CopyMem () or cache maintenance operations
>>> that require a valid mapping. This not only applies to the above use case,
>>> but also to the DebugUncachedMemoryAllocationLib that unmaps the
>>> primary, cached mapping of an allocation, and returns a host address
>>> which is an uncached alias offset by a constant.
>>>
>>> Since we should never access the device address from the CPU, there is
>>> no need to record it in the MAPINFO struct. Instead, record the buffer
>>> address in case of double buffering, since we do need to copy the contents
>>> (in case of a bus master write) and free the buffer (in all cases) when
>>> DmaUnmap() is called.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>
>> For the fix itself:
>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>
>> However, can we wait for a few Tested-by:s to ensure this fix does not
>> reveal any companion bugs?
>>
>
> Perhaps, yes.
>
> In case anyone is up to doing that, please find the branch here
> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset
>
I tested your branch on the usual victims (R0/1/2, FVP Foundation &
AEMv8 and TC2) and they all work fine for me.
Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
> However, given that the split CPU/bus master view is introduced in the
> next patch, the only use case where the device address differs from
> the host address is when using the DebugUncachedMemoryAllocationLib,
> which is currently broken AFAICT (it attempts to unmap the linear
> mapping of the allocation by setting the memory attributes to '0',
> which triggers an assert in the ArmPkg MMU code)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-15 11:34 ` Ryan Harkin
@ 2016-11-15 13:07 ` Ard Biesheuvel
2016-11-15 18:01 ` Leif Lindholm
0 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-15 13:07 UTC (permalink / raw)
To: Ryan Harkin; +Cc: Leif Lindholm, Jeremy Linton, edk2-devel-01
On 15 November 2016 at 11:34, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 15 November 2016 at 09:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>>> On Sat, Nov 12, 2016 at 02:02:27PM +0100, Ard Biesheuvel wrote:
>>>> In preparation of adding support to ArmDmalib for DMA bus masters whose
>>>> view of memory is offset by a constant compared to the CPU's view, clean
>>>> up some abuse of the device address.
>>>>
>>>> The device address is not defined in terms of the CPU's address space,
>>>> and so it should not be used in CopyMem () or cache maintenance operations
>>>> that require a valid mapping. This not only applies to the above use case,
>>>> but also to the DebugUncachedMemoryAllocationLib that unmaps the
>>>> primary, cached mapping of an allocation, and returns a host address
>>>> which is an uncached alias offset by a constant.
>>>>
>>>> Since we should never access the device address from the CPU, there is
>>>> no need to record it in the MAPINFO struct. Instead, record the buffer
>>>> address in case of double buffering, since we do need to copy the contents
>>>> (in case of a bus master write) and free the buffer (in all cases) when
>>>> DmaUnmap() is called.
>>>>
>>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>>
>>> For the fix itself:
>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>>>
>>> However, can we wait for a few Tested-by:s to ensure this fix does not
>>> reveal any companion bugs?
>>>
>>
>> Perhaps, yes.
>>
>> In case anyone is up to doing that, please find the branch here
>> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset
>>
>
> I tested your branch on the usual victims (R0/1/2, FVP Foundation &
> AEMv8 and TC2) and they all work fine for me.
>
> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
>
Thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address
2016-11-15 13:07 ` Ard Biesheuvel
@ 2016-11-15 18:01 ` Leif Lindholm
2016-11-30 16:45 ` Leif Lindholm
0 siblings, 1 reply; 14+ messages in thread
From: Leif Lindholm @ 2016-11-15 18:01 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Ryan Harkin, edk2-devel-01
On Tue, Nov 15, 2016 at 01:07:37PM +0000, Ard Biesheuvel wrote:
> On 15 November 2016 at 11:34, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> >>>> Since we should never access the device address from the CPU, there is
> >>>> no need to record it in the MAPINFO struct. Instead, record the buffer
> >>>> address in case of double buffering, since we do need to copy the contents
> >>>> (in case of a bus master write) and free the buffer (in all cases) when
> >>>> DmaUnmap() is called.
> >>>>
> >>>> Contributed-under: TianoCore Contribution Agreement 1.0
> >>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>>
> >>> For the fix itself:
> >>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >>>
> >>> However, can we wait for a few Tested-by:s to ensure this fix does not
> >>> reveal any companion bugs?
> >>>
> >>
> >> Perhaps, yes.
> >>
> >> In case anyone is up to doing that, please find the branch here
> >> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset
> >>
> >
> > I tested your branch on the usual victims (R0/1/2, FVP Foundation &
> > AEMv8 and TC2) and they all work fine for me.
> >
> > Tested-by: Ryan Harkin <ryan.harkin@linaro.org>
OK, so on the off-hand chance that this does break some non-upstream
drivers, I'll wait for further comments until I'm back from holiday,
28 November, before pushing this.
Additional Tested-by:s would still be appreciated.
Regards,
Leif
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/4] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset
2016-11-12 13:02 [PATCH v2 0/4] ArmPkg: more ArmDmaLib fixes Ard Biesheuvel
` (2 preceding siblings ...)
2016-11-12 13:02 ` [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address Ard Biesheuvel
@ 2016-11-12 13:02 ` Ard Biesheuvel
2016-11-14 15:17 ` Leif Lindholm
3 siblings, 1 reply; 14+ messages in thread
From: Ard Biesheuvel @ 2016-11-12 13:02 UTC (permalink / raw)
To: edk2-devel, leif.lindholm; +Cc: Ard Biesheuvel
Some devices, such as the Raspberry Pi3, have a fixed offset between memory
addresses as seen by the host and as seen by the other bus masters. So add
a new PCD that allows this fixed offset to be recorded, and to be used when
returning device addresses from the DmaLib mapping routines.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/ArmPkg.dec | 8 ++++++++
ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 20 ++++++++++++++++++--
ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf | 1 +
3 files changed, 27 insertions(+), 2 deletions(-)
diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
index 3cdb5da3d4f3..090ed9951366 100644
--- a/ArmPkg/ArmPkg.dec
+++ b/ArmPkg/ArmPkg.dec
@@ -134,6 +134,14 @@ [PcdsFixedAtBuild.common]
gArmTokenSpaceGuid.PcdFdSize|0|UINT32|0x0000002C
gArmTokenSpaceGuid.PcdFvSize|0|UINT32|0x0000002E
+ #
+ # Value to add to a host address to obtain a device address, using
+ # unsigned 64-bit integer arithmetic on both ARM and AArch64. This
+ # means we can rely on truncation on overflow to specify negative
+ # offsets.
+ #
+ gArmTokenSpaceGuid.PcdArmDmaDeviceOffset|0x0|UINT64|0x0000044
+
[PcdsFixedAtBuild.common, PcdsPatchableInModule.common]
gArmTokenSpaceGuid.PcdFdBaseAddress|0|UINT64|0x0000002B
gArmTokenSpaceGuid.PcdFvBaseAddress|0|UINT64|0x0000002D
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index 7321388de63e..1de4a6e5b04f 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -37,6 +37,15 @@ typedef struct {
STATIC EFI_CPU_ARCH_PROTOCOL *mCpu;
+STATIC
+PHYSICAL_ADDRESS
+HostToDeviceAddress (
+ IN PHYSICAL_ADDRESS HostAddress
+ )
+{
+ return HostAddress + PcdGet64 (PcdArmDmaDeviceOffset);
+}
+
/**
Provides the DMA controller-specific addresses needed to access system memory.
@@ -80,7 +89,14 @@ DmaMap (
return EFI_INVALID_PARAMETER;
}
- *DeviceAddress = ConvertToPhysicalAddress (HostAddress);
+ //
+ // The debug implementation of UncachedMemoryAllocationLib in ArmPkg returns
+ // a virtual uncached alias, and unmaps the cached ID mapping of the buffer,
+ // in order to catch inadvertent references to the cached mapping.
+ // Since HostToDeviceAddress () expects ID mapped input addresses, convert
+ // the host address to an ID mapped address first.
+ //
+ *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (HostAddress));
// Remember range so we can flush on the other side
Map = AllocatePool (sizeof (MAP_INFO_INSTANCE));
@@ -127,7 +143,7 @@ DmaMap (
CopyMem (Buffer, HostAddress, *NumberOfBytes);
}
- *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
+ *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress ((UINTN)Buffer));
Map->BufferAddress = Buffer;
} else {
Map->DoubleBuffer = FALSE;
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
index 31de3cfd828c..9b7dad114b18 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
@@ -44,6 +44,7 @@ [Protocols]
[Guids]
[Pcd]
+ gArmTokenSpaceGuid.PcdArmDmaDeviceOffset
[Depex]
gEfiCpuArchProtocolGuid
--
2.7.4
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 4/4] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset
2016-11-12 13:02 ` [PATCH v2 4/4] ArmPkg/ArmDmaLib: add support for fixed host-to-device DMA offset Ard Biesheuvel
@ 2016-11-14 15:17 ` Leif Lindholm
0 siblings, 0 replies; 14+ messages in thread
From: Leif Lindholm @ 2016-11-14 15:17 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Sat, Nov 12, 2016 at 02:02:28PM +0100, Ard Biesheuvel wrote:
> Some devices, such as the Raspberry Pi3, have a fixed offset between memory
> addresses as seen by the host and as seen by the other bus masters. So add
> a new PCD that allows this fixed offset to be recorded, and to be used when
> returning device addresses from the DmaLib mapping routines.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Much more Leif-friendly, thanks.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
> ArmPkg/ArmPkg.dec | 8 ++++++++
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 20 ++++++++++++++++++--
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf | 1 +
> 3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/ArmPkg.dec b/ArmPkg/ArmPkg.dec
> index 3cdb5da3d4f3..090ed9951366 100644
> --- a/ArmPkg/ArmPkg.dec
> +++ b/ArmPkg/ArmPkg.dec
> @@ -134,6 +134,14 @@ [PcdsFixedAtBuild.common]
> gArmTokenSpaceGuid.PcdFdSize|0|UINT32|0x0000002C
> gArmTokenSpaceGuid.PcdFvSize|0|UINT32|0x0000002E
>
> + #
> + # Value to add to a host address to obtain a device address, using
> + # unsigned 64-bit integer arithmetic on both ARM and AArch64. This
> + # means we can rely on truncation on overflow to specify negative
> + # offsets.
> + #
> + gArmTokenSpaceGuid.PcdArmDmaDeviceOffset|0x0|UINT64|0x0000044
> +
> [PcdsFixedAtBuild.common, PcdsPatchableInModule.common]
> gArmTokenSpaceGuid.PcdFdBaseAddress|0|UINT64|0x0000002B
> gArmTokenSpaceGuid.PcdFvBaseAddress|0|UINT64|0x0000002D
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index 7321388de63e..1de4a6e5b04f 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -37,6 +37,15 @@ typedef struct {
>
> STATIC EFI_CPU_ARCH_PROTOCOL *mCpu;
>
> +STATIC
> +PHYSICAL_ADDRESS
> +HostToDeviceAddress (
> + IN PHYSICAL_ADDRESS HostAddress
> + )
> +{
> + return HostAddress + PcdGet64 (PcdArmDmaDeviceOffset);
> +}
> +
> /**
> Provides the DMA controller-specific addresses needed to access system memory.
>
> @@ -80,7 +89,14 @@ DmaMap (
> return EFI_INVALID_PARAMETER;
> }
>
> - *DeviceAddress = ConvertToPhysicalAddress (HostAddress);
> + //
> + // The debug implementation of UncachedMemoryAllocationLib in ArmPkg returns
> + // a virtual uncached alias, and unmaps the cached ID mapping of the buffer,
> + // in order to catch inadvertent references to the cached mapping.
> + // Since HostToDeviceAddress () expects ID mapped input addresses, convert
> + // the host address to an ID mapped address first.
> + //
> + *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (HostAddress));
>
> // Remember range so we can flush on the other side
> Map = AllocatePool (sizeof (MAP_INFO_INSTANCE));
> @@ -127,7 +143,7 @@ DmaMap (
> CopyMem (Buffer, HostAddress, *NumberOfBytes);
> }
>
> - *DeviceAddress = ConvertToPhysicalAddress ((UINTN)Buffer);
> + *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress ((UINTN)Buffer));
> Map->BufferAddress = Buffer;
> } else {
> Map->DoubleBuffer = FALSE;
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> index 31de3cfd828c..9b7dad114b18 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.inf
> @@ -44,6 +44,7 @@ [Protocols]
> [Guids]
>
> [Pcd]
> + gArmTokenSpaceGuid.PcdArmDmaDeviceOffset
>
> [Depex]
> gEfiCpuArchProtocolGuid
> --
> 2.7.4
>
^ permalink raw reply [flat|nested] 14+ messages in thread