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 <[email protected]>
For the fix itself: Reviewed-by: Leif Lindholm <[email protected]> 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 > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
