On 19 April 2016 at 16:55, Ard Biesheuvel <[email protected]> wrote: > In the DmaMap () operation, if the region to be mapped happens to be > aligned to the Cache Writeback Granule (CWG) (whose value is typically > 64 or 128 bytes and 2 KB maximum), we remap the memory as uncached. > > Since remapping memory occurs at page granularity, while the buffer and the > CWG may be much smaller, there is no telling what other memory we affect > by doing this, especially since the operation is not reverted in DmaUnmap(). > > So remove the remapping call. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <[email protected]> > --- > ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 6 ------ > 1 file changed, 6 deletions(-) > > diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > index 7e518ed3b83e..83f4d38a8a60 100644 > --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > @@ -137,12 +137,6 @@ DmaMap ( > > // Flush the Data Cache (should not have any effect if the memory region > is uncached) > gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, > EfiCpuFlushTypeWriteBackInvalidate); > - > - if ((Operation == MapOperationBusMasterRead) || (Operation == > MapOperationBusMasterCommonBuffer)) { > - // In case the buffer is used for instance to send command to a PCI > controller, we must ensure the memory is uncached > - Status = gDS->SetMemorySpaceAttributes (*DeviceAddress & ~(BASE_4KB - > 1), ALIGN_VALUE (*NumberOfBytes, BASE_4KB), EFI_MEMORY_WC); > - ASSERT_EFI_ERROR (Status); > - } > } > > Map->HostAddress = (UINTN)HostAddress;
Actually, I have a better fix for this (or a followup patch on top) The problem here is that a misbehaving client of this code may attempt to create a consistent DMA mapping of a region that happens to be aligned sufficiently to the CWG, in which case we never check if the mapping is cached or not. Instead, we should always check if a mapping is cached, and reject consistent mappings rather than remap them as uncached on the fly (like above) without regard of what else may be affected. Unfortunately, it is difficult for me to test these changes, and I am quite sure they will break something somewhere, or these dodgy things would not have been added in the first place. _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
