Re: [PATCH v2 3/4] ArmPkg/ArmDmaLib: clean up abuse of device address

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

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
>