Re: [PATCH] ArmPkg/ArmDmaLib: use double buffering only for bus master write

[Leif Lindholm <leif.lindholm@linaro.org>]

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
>