* [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write
@ 2017-08-17 12:25 Ard Biesheuvel
2017-08-17 12:26 ` Ard Biesheuvel
2017-08-17 12:43 ` Leif Lindholm
0 siblings, 2 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-08-17 12:25 UTC (permalink / raw)
To: edk2-devel, leif.lindholm, lersek; +Cc: Ard Biesheuvel
The ArmPkg implementation of DmaLib uses double buffering to ensure
that any attempt to perform non-coherent DMA on unaligned buffers cannot
corrupt adjacent unrelated data which happens to share cachelines with
the data we are exchanging with the device.
Such corruption can only occur on bus master read, in which case we have
to invalidate the caches to ensure the CPU will see the data written to
memory by the device. In the bus master write case, we can simply clean
and invalidate at the same time, which may purge unrelated adjacent data
from the caches, but will not corrupt its contents.
Also, this double buffer does not necessarily have to be allocated from
uncached memory: by the same reasoning, we can perform cache invalidation
on an ordinary pool allocation as long as we take the same alignment
constraints into account.
So update our code accordingly: remove double buffering from the bus
master read path, and switch to a pool allocation for the double buffer.
Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++--------
1 file changed, 28 insertions(+), 19 deletions(-)
diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
index f4ee9e4c5ea2..61d70614bff0 100644
--- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
+++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
@@ -80,6 +80,7 @@ DmaMap (
MAP_INFO_INSTANCE *Map;
VOID *Buffer;
EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
+ UINTN AllocSize;
if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
return EFI_INVALID_PARAMETER;
@@ -104,8 +105,9 @@ DmaMap (
return EFI_OUT_OF_RESOURCES;
}
- if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
- ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
+ if (Operation != MapOperationBusMasterRead &&
+ ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
+ ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
// Get the cacheability of the region
Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
@@ -129,21 +131,24 @@ DmaMap (
}
//
- // If the buffer does not fill entire cache lines we must double buffer into
- // uncached memory. Device (PCI) address becomes uncached page.
+ // If the buffer does not fill entire cache lines we must double buffer
+ // into a suitably aligned allocation that allows us to invalidate the
+ // cache without running the risk of corrupting adjacent unrelated data.
+ // Note that pool allocations are guaranteed to be 8 byte aligned, so
+ // we only have to add (alignment - 8) worth of padding.
//
- Map->DoubleBuffer = TRUE;
- Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
- if (EFI_ERROR (Status)) {
+ Map->DoubleBuffer = TRUE;
+ AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
+ (mCpu->DmaBufferAlignment - 8);
+ Map->BufferAddress = AllocatePool (AllocSize);
+ if (Map->BufferAddress == NULL) {
+ Status = EFI_OUT_OF_RESOURCES;
goto FreeMapInfo;
}
- if (Operation == MapOperationBusMasterRead) {
- CopyMem (Buffer, HostAddress, *NumberOfBytes);
- }
-
+ Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
*DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer));
- Map->BufferAddress = Buffer;
+
} else {
Map->DoubleBuffer = FALSE;
}
@@ -207,6 +212,7 @@ DmaUnmap (
{
MAP_INFO_INSTANCE *Map;
EFI_STATUS Status;
+ VOID *Buffer;
if (Mapping == NULL) {
ASSERT (FALSE);
@@ -217,17 +223,20 @@ DmaUnmap (
Status = EFI_SUCCESS;
if (Map->DoubleBuffer) {
- ASSERT (Map->Operation != MapOperationBusMasterCommonBuffer);
+ ASSERT (Map->Operation == MapOperationBusMasterWrite);
- if (Map->Operation == MapOperationBusMasterCommonBuffer) {
+ if (Map->Operation != MapOperationBusMasterWrite) {
Status = EFI_INVALID_PARAMETER;
- } else if (Map->Operation == MapOperationBusMasterWrite) {
- CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
- Map->NumberOfBytes);
- }
+ } else {
+ Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
+
+ mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes,
+ EfiCpuFlushTypeInvalidate);
- DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress);
+ CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes);
+ FreePool (Map->BufferAddress);
+ }
} else {
if (Map->Operation == MapOperationBusMasterWrite) {
//
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write
2017-08-17 12:25 [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write Ard Biesheuvel
@ 2017-08-17 12:26 ` Ard Biesheuvel
2017-08-17 12:43 ` Leif Lindholm
1 sibling, 0 replies; 5+ messages in thread
From: Ard Biesheuvel @ 2017-08-17 12:26 UTC (permalink / raw)
To: edk2-devel@lists.01.org, Leif Lindholm, Laszlo Ersek; +Cc: Ard Biesheuvel
On 17 August 2017 at 13:25, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> The ArmPkg implementation of DmaLib uses double buffering to ensure
> that any attempt to perform non-coherent DMA on unaligned buffers cannot
> corrupt adjacent unrelated data which happens to share cachelines with
> the data we are exchanging with the device.
>
> Such corruption can only occur on bus master read, in which case we have
*write* not read, apologies.
> to invalidate the caches to ensure the CPU will see the data written to
> memory by the device. In the bus master write case, we can simply clean
> and invalidate at the same time, which may purge unrelated adjacent data
> from the caches, but will not corrupt its contents.
>
> Also, this double buffer does not necessarily have to be allocated from
> uncached memory: by the same reasoning, we can perform cache invalidation
> on an ordinary pool allocation as long as we take the same alignment
> constraints into account.
>
> So update our code accordingly: remove double buffering from the bus
> master read path, and switch to a pool allocation for the double buffer.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++--------
> 1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index f4ee9e4c5ea2..61d70614bff0 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -80,6 +80,7 @@ DmaMap (
> MAP_INFO_INSTANCE *Map;
> VOID *Buffer;
> EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
> + UINTN AllocSize;
>
> if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
> return EFI_INVALID_PARAMETER;
> @@ -104,8 +105,9 @@ DmaMap (
> return EFI_OUT_OF_RESOURCES;
> }
>
> - if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> - ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
> + if (Operation != MapOperationBusMasterRead &&
> + ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>
> // Get the cacheability of the region
> Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
> @@ -129,21 +131,24 @@ DmaMap (
> }
>
> //
> - // If the buffer does not fill entire cache lines we must double buffer into
> - // uncached memory. Device (PCI) address becomes uncached page.
> + // If the buffer does not fill entire cache lines we must double buffer
> + // into a suitably aligned allocation that allows us to invalidate the
> + // cache without running the risk of corrupting adjacent unrelated data.
> + // Note that pool allocations are guaranteed to be 8 byte aligned, so
> + // we only have to add (alignment - 8) worth of padding.
> //
> - Map->DoubleBuffer = TRUE;
> - Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
> - if (EFI_ERROR (Status)) {
> + Map->DoubleBuffer = TRUE;
> + AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
> + (mCpu->DmaBufferAlignment - 8);
> + Map->BufferAddress = AllocatePool (AllocSize);
> + if (Map->BufferAddress == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> goto FreeMapInfo;
> }
>
> - if (Operation == MapOperationBusMasterRead) {
> - CopyMem (Buffer, HostAddress, *NumberOfBytes);
> - }
> -
> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer));
> - Map->BufferAddress = Buffer;
> +
> } else {
> Map->DoubleBuffer = FALSE;
> }
> @@ -207,6 +212,7 @@ DmaUnmap (
> {
> MAP_INFO_INSTANCE *Map;
> EFI_STATUS Status;
> + VOID *Buffer;
>
> if (Mapping == NULL) {
> ASSERT (FALSE);
> @@ -217,17 +223,20 @@ DmaUnmap (
>
> Status = EFI_SUCCESS;
> if (Map->DoubleBuffer) {
> - ASSERT (Map->Operation != MapOperationBusMasterCommonBuffer);
> + ASSERT (Map->Operation == MapOperationBusMasterWrite);
>
> - if (Map->Operation == MapOperationBusMasterCommonBuffer) {
> + if (Map->Operation != MapOperationBusMasterWrite) {
> Status = EFI_INVALID_PARAMETER;
> - } else if (Map->Operation == MapOperationBusMasterWrite) {
> - CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> - Map->NumberOfBytes);
> - }
> + } else {
> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> +
> + mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes,
> + EfiCpuFlushTypeInvalidate);
>
> - DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress);
> + CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes);
>
> + FreePool (Map->BufferAddress);
> + }
> } else {
> if (Map->Operation == MapOperationBusMasterWrite) {
> //
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write
2017-08-17 12:25 [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write Ard Biesheuvel
2017-08-17 12:26 ` Ard Biesheuvel
@ 2017-08-17 12:43 ` Leif Lindholm
2017-08-17 12:45 ` Ard Biesheuvel
1 sibling, 1 reply; 5+ messages in thread
From: Leif Lindholm @ 2017-08-17 12:43 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel, lersek
On Thu, Aug 17, 2017 at 01:25:46PM +0100, Ard Biesheuvel wrote:
> The ArmPkg implementation of DmaLib uses double buffering to ensure
> that any attempt to perform non-coherent DMA on unaligned buffers cannot
> corrupt adjacent unrelated data which happens to share cachelines with
> the data we are exchanging with the device.
>
> Such corruption can only occur on bus master read, in which case we have
> to invalidate the caches to ensure the CPU will see the data written to
> memory by the device. In the bus master write case, we can simply clean
> and invalidate at the same time, which may purge unrelated adjacent data
> from the caches, but will not corrupt its contents.
>
> Also, this double buffer does not necessarily have to be allocated from
> uncached memory: by the same reasoning, we can perform cache invalidation
> on an ordinary pool allocation as long as we take the same alignment
> constraints into account.
>
> So update our code accordingly: remove double buffering from the bus
> master read path, and switch to a pool allocation for the double buffer.
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++--------
> 1 file changed, 28 insertions(+), 19 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> index f4ee9e4c5ea2..61d70614bff0 100644
> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> @@ -80,6 +80,7 @@ DmaMap (
> MAP_INFO_INSTANCE *Map;
> VOID *Buffer;
> EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
> + UINTN AllocSize;
>
> if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
> return EFI_INVALID_PARAMETER;
> @@ -104,8 +105,9 @@ DmaMap (
> return EFI_OUT_OF_RESOURCES;
> }
>
> - if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> - ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
> + if (Operation != MapOperationBusMasterRead &&
> + ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>
> // Get the cacheability of the region
> Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
> @@ -129,21 +131,24 @@ DmaMap (
> }
>
> //
> - // If the buffer does not fill entire cache lines we must double buffer into
> - // uncached memory. Device (PCI) address becomes uncached page.
> + // If the buffer does not fill entire cache lines we must double buffer
> + // into a suitably aligned allocation that allows us to invalidate the
> + // cache without running the risk of corrupting adjacent unrelated data.
> + // Note that pool allocations are guaranteed to be 8 byte aligned, so
> + // we only have to add (alignment - 8) worth of padding.
> //
> - Map->DoubleBuffer = TRUE;
> - Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
> - if (EFI_ERROR (Status)) {
> + Map->DoubleBuffer = TRUE;
> + AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
> + (mCpu->DmaBufferAlignment - 8);
> + Map->BufferAddress = AllocatePool (AllocSize);
> + if (Map->BufferAddress == NULL) {
> + Status = EFI_OUT_OF_RESOURCES;
> goto FreeMapInfo;
> }
>
> - if (Operation == MapOperationBusMasterRead) {
> - CopyMem (Buffer, HostAddress, *NumberOfBytes);
> - }
> -
> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer));
> - Map->BufferAddress = Buffer;
> +
Nothing wrong with a blank line here, but I don't seem to recall you
doing that in general. Accidental?
Anyway, folding in your own commit message correction, and possibly
one whitespace change:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
/
Leif
> } else {
> Map->DoubleBuffer = FALSE;
> }
> @@ -207,6 +212,7 @@ DmaUnmap (
> {
> MAP_INFO_INSTANCE *Map;
> EFI_STATUS Status;
> + VOID *Buffer;
>
> if (Mapping == NULL) {
> ASSERT (FALSE);
> @@ -217,17 +223,20 @@ DmaUnmap (
>
> Status = EFI_SUCCESS;
> if (Map->DoubleBuffer) {
> - ASSERT (Map->Operation != MapOperationBusMasterCommonBuffer);
> + ASSERT (Map->Operation == MapOperationBusMasterWrite);
>
> - if (Map->Operation == MapOperationBusMasterCommonBuffer) {
> + if (Map->Operation != MapOperationBusMasterWrite) {
> Status = EFI_INVALID_PARAMETER;
> - } else if (Map->Operation == MapOperationBusMasterWrite) {
> - CopyMem ((VOID *)(UINTN)Map->HostAddress, Map->BufferAddress,
> - Map->NumberOfBytes);
> - }
> + } else {
> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> +
> + mCpu->FlushDataCache (mCpu, (UINTN)Buffer, Map->NumberOfBytes,
> + EfiCpuFlushTypeInvalidate);
>
> - DmaFreeBuffer (EFI_SIZE_TO_PAGES (Map->NumberOfBytes), Map->BufferAddress);
> + CopyMem ((VOID *)(UINTN)Map->HostAddress, Buffer, Map->NumberOfBytes);
>
> + FreePool (Map->BufferAddress);
> + }
> } else {
> if (Map->Operation == MapOperationBusMasterWrite) {
> //
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write
2017-08-17 12:43 ` Leif Lindholm
@ 2017-08-17 12:45 ` Ard Biesheuvel
2017-08-17 12:59 ` Leif Lindholm
0 siblings, 1 reply; 5+ messages in thread
From: Ard Biesheuvel @ 2017-08-17 12:45 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On 17 August 2017 at 13:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Aug 17, 2017 at 01:25:46PM +0100, Ard Biesheuvel wrote:
>> The ArmPkg implementation of DmaLib uses double buffering to ensure
>> that any attempt to perform non-coherent DMA on unaligned buffers cannot
>> corrupt adjacent unrelated data which happens to share cachelines with
>> the data we are exchanging with the device.
>>
>> Such corruption can only occur on bus master read, in which case we have
>> to invalidate the caches to ensure the CPU will see the data written to
>> memory by the device. In the bus master write case, we can simply clean
>> and invalidate at the same time, which may purge unrelated adjacent data
>> from the caches, but will not corrupt its contents.
>>
>> Also, this double buffer does not necessarily have to be allocated from
>> uncached memory: by the same reasoning, we can perform cache invalidation
>> on an ordinary pool allocation as long as we take the same alignment
>> constraints into account.
>>
>> So update our code accordingly: remove double buffering from the bus
>> master read path, and switch to a pool allocation for the double buffer.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++--------
>> 1 file changed, 28 insertions(+), 19 deletions(-)
>>
>> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> index f4ee9e4c5ea2..61d70614bff0 100644
>> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
>> @@ -80,6 +80,7 @@ DmaMap (
>> MAP_INFO_INSTANCE *Map;
>> VOID *Buffer;
>> EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
>> + UINTN AllocSize;
>>
>> if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
>> return EFI_INVALID_PARAMETER;
>> @@ -104,8 +105,9 @@ DmaMap (
>> return EFI_OUT_OF_RESOURCES;
>> }
>>
>> - if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
>> - ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
>> + if (Operation != MapOperationBusMasterRead &&
>> + ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
>> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
>>
>> // Get the cacheability of the region
>> Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
>> @@ -129,21 +131,24 @@ DmaMap (
>> }
>>
>> //
>> - // If the buffer does not fill entire cache lines we must double buffer into
>> - // uncached memory. Device (PCI) address becomes uncached page.
>> + // If the buffer does not fill entire cache lines we must double buffer
>> + // into a suitably aligned allocation that allows us to invalidate the
>> + // cache without running the risk of corrupting adjacent unrelated data.
>> + // Note that pool allocations are guaranteed to be 8 byte aligned, so
>> + // we only have to add (alignment - 8) worth of padding.
>> //
>> - Map->DoubleBuffer = TRUE;
>> - Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
>> - if (EFI_ERROR (Status)) {
>> + Map->DoubleBuffer = TRUE;
>> + AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
>> + (mCpu->DmaBufferAlignment - 8);
>> + Map->BufferAddress = AllocatePool (AllocSize);
>> + if (Map->BufferAddress == NULL) {
>> + Status = EFI_OUT_OF_RESOURCES;
>> goto FreeMapInfo;
>> }
>>
>> - if (Operation == MapOperationBusMasterRead) {
>> - CopyMem (Buffer, HostAddress, *NumberOfBytes);
>> - }
>> -
>> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
>> *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer));
>> - Map->BufferAddress = Buffer;
>> +
>
> Nothing wrong with a blank line here, but I don't seem to recall you
> doing that in general. Accidental?
>
Ehm, no ...
Actually, what I intended to put there was
//
// Get rid of any dirty cachelines covering the double buffer. This
// prevents them from being written back unexpectedly, potentially
// overwriting the data we receive from the device.
//
mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes,
EfiCpuFlushTypeWriteBack);
but I put a blank line instead :-)
> Anyway, folding in your own commit message correction, and possibly
> one whitespace change:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
Thanks. Please confirm that this covers the addition above.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write
2017-08-17 12:45 ` Ard Biesheuvel
@ 2017-08-17 12:59 ` Leif Lindholm
0 siblings, 0 replies; 5+ messages in thread
From: Leif Lindholm @ 2017-08-17 12:59 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org, Laszlo Ersek
On Thu, Aug 17, 2017 at 01:45:58PM +0100, Ard Biesheuvel wrote:
> On 17 August 2017 at 13:43, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Thu, Aug 17, 2017 at 01:25:46PM +0100, Ard Biesheuvel wrote:
> >> The ArmPkg implementation of DmaLib uses double buffering to ensure
> >> that any attempt to perform non-coherent DMA on unaligned buffers cannot
> >> corrupt adjacent unrelated data which happens to share cachelines with
> >> the data we are exchanging with the device.
> >>
> >> Such corruption can only occur on bus master read, in which case we have
> >> to invalidate the caches to ensure the CPU will see the data written to
> >> memory by the device. In the bus master write case, we can simply clean
> >> and invalidate at the same time, which may purge unrelated adjacent data
> >> from the caches, but will not corrupt its contents.
> >>
> >> Also, this double buffer does not necessarily have to be allocated from
> >> uncached memory: by the same reasoning, we can perform cache invalidation
> >> on an ordinary pool allocation as long as we take the same alignment
> >> constraints into account.
> >>
> >> So update our code accordingly: remove double buffering from the bus
> >> master read path, and switch to a pool allocation for the double buffer.
> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >> ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 47 ++++++++++++--------
> >> 1 file changed, 28 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> index f4ee9e4c5ea2..61d70614bff0 100644
> >> --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c
> >> @@ -80,6 +80,7 @@ DmaMap (
> >> MAP_INFO_INSTANCE *Map;
> >> VOID *Buffer;
> >> EFI_GCD_MEMORY_SPACE_DESCRIPTOR GcdDescriptor;
> >> + UINTN AllocSize;
> >>
> >> if (HostAddress == NULL || NumberOfBytes == NULL || DeviceAddress == NULL || Mapping == NULL ) {
> >> return EFI_INVALID_PARAMETER;
> >> @@ -104,8 +105,9 @@ DmaMap (
> >> return EFI_OUT_OF_RESOURCES;
> >> }
> >>
> >> - if ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> >> - ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0)) {
> >> + if (Operation != MapOperationBusMasterRead &&
> >> + ((((UINTN)HostAddress & (mCpu->DmaBufferAlignment - 1)) != 0) ||
> >> + ((*NumberOfBytes & (mCpu->DmaBufferAlignment - 1)) != 0))) {
> >>
> >> // Get the cacheability of the region
> >> Status = gDS->GetMemorySpaceDescriptor ((UINTN)HostAddress, &GcdDescriptor);
> >> @@ -129,21 +131,24 @@ DmaMap (
> >> }
> >>
> >> //
> >> - // If the buffer does not fill entire cache lines we must double buffer into
> >> - // uncached memory. Device (PCI) address becomes uncached page.
> >> + // If the buffer does not fill entire cache lines we must double buffer
> >> + // into a suitably aligned allocation that allows us to invalidate the
> >> + // cache without running the risk of corrupting adjacent unrelated data.
> >> + // Note that pool allocations are guaranteed to be 8 byte aligned, so
> >> + // we only have to add (alignment - 8) worth of padding.
> >> //
> >> - Map->DoubleBuffer = TRUE;
> >> - Status = DmaAllocateBuffer (EfiBootServicesData, EFI_SIZE_TO_PAGES (*NumberOfBytes), &Buffer);
> >> - if (EFI_ERROR (Status)) {
> >> + Map->DoubleBuffer = TRUE;
> >> + AllocSize = ALIGN_VALUE (*NumberOfBytes, mCpu->DmaBufferAlignment) +
> >> + (mCpu->DmaBufferAlignment - 8);
> >> + Map->BufferAddress = AllocatePool (AllocSize);
> >> + if (Map->BufferAddress == NULL) {
> >> + Status = EFI_OUT_OF_RESOURCES;
> >> goto FreeMapInfo;
> >> }
> >>
> >> - if (Operation == MapOperationBusMasterRead) {
> >> - CopyMem (Buffer, HostAddress, *NumberOfBytes);
> >> - }
> >> -
> >> + Buffer = ALIGN_POINTER (Map->BufferAddress, mCpu->DmaBufferAlignment);
> >> *DeviceAddress = HostToDeviceAddress (ConvertToPhysicalAddress (Buffer));
> >> - Map->BufferAddress = Buffer;
> >> +
> >
> > Nothing wrong with a blank line here, but I don't seem to recall you
> > doing that in general. Accidental?
> >
>
> Ehm, no ...
>
> Actually, what I intended to put there was
>
> //
> // Get rid of any dirty cachelines covering the double buffer. This
> // prevents them from being written back unexpectedly, potentially
> // overwriting the data we receive from the device.
> //
> mCpu->FlushDataCache (mCpu, (UINTN)Buffer, *NumberOfBytes,
> EfiCpuFlushTypeWriteBack);
>
> but I put a blank line instead :-)
>
> > Anyway, folding in your own commit message correction, and possibly
> > one whitespace change:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
>
> Thanks. Please confirm that this covers the addition above.
Haha, yes, it does.
/
Leif
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-08-17 12:56 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-17 12:25 [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write Ard Biesheuvel
2017-08-17 12:26 ` Ard Biesheuvel
2017-08-17 12:43 ` Leif Lindholm
2017-08-17 12:45 ` Ard Biesheuvel
2017-08-17 12:59 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox