On Thu, Aug 17, 2017 at 01:45:58PM +0100, Ard Biesheuvel wrote:
> 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.
Haha, yes, it does.
/
Leif
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
