| Message ID | 1461077734-4327-5-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | 32e5fb76e5692911d5176ac4dc9cedafbe7fa5c9 |
| Headers | show |
On 19 April 2016 at 16:55, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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 <ard.biesheuvel@linaro.org> > --- > 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 edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Tue, Apr 19, 2016 at 04:55:34PM +0200, Ard Biesheuvel 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 <ard.biesheuvel@linaro.org> > --- > 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); > - } I would be interested in knowing what problem this was intended to solve, but regardless the above won't have been a safe way of doing it. Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > } > > Map->HostAddress = (UINTN)HostAddress; > -- > 2.5.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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;
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 <ard.biesheuvel@linaro.org> --- ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 6 ------ 1 file changed, 6 deletions(-) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel