On 17 August 2017 at 13:43, Leif Lindholm <[email protected]> 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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
Thanks. Please confirm that this covers the addition above.
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
