| Message ID | 1461141628-20896-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | Accepted |
| Commit | b64e44cc098ffd03ccbf1a635ae405ae98971419 |
| Headers | show |
On 20 April 2016 at 09:40, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote: > DmaMap () only allows uncached mappings to be used for creating consistent > mappings with operation type MapOperationBusMasterCommonBuffer. However, > if the buffer passed to DmaMap () happens to be aligned to the CWG, there > is no need for a bounce buffer, and we perform the cache maintenance > directly without ever checking if the memory attributes of the buffer > adhere to the API. > > So add some debug code that asserts that the operation type and the memory > attributes are consistent. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Tested-by: Ryan Harkin <ryan.harkin@linaro.org> Tested on TC2, Juno R0, R1 and R2, FVP Base AEMv8 and FVP Foundation models. On FVP models, I just checked that they still booted. On my Juno boards, I checked that the USB disks were still readable and that I could read my SATA drive. On TC2, I made sure I could read an MMC card; there is USB support on TC2. I'm not even sure if the MMC card driver uses DMA, but I thought I'd give it a spin anyway. > --- > > This applies on top of the ArmDmaLib patches that I sent out yesterday. > Based on the original code which contained some dodgy looking workarounds, > I expect this to actually break something, but Olivier is not around anymore > to tell me what he was trying to fix (and the broken 'promote cache > mainantenance by VA to set/way above a certain threshold' may well have been > the culprit here) > > Since the PCI emulation for non-coherent devices relies on this DMA code, > any non-coherent real platform that uses the SATA or EHCI/UCHI code could > be used to test this, but I don't have access to any of those. Suggestions > are welcome, as are donations of a Juno or a TC2. > > Thanks. > > ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > index 83f4d38a8a60..329756efc268 100644 > --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > @@ -135,6 +135,23 @@ DmaMap ( > } else { > Map->DoubleBuffer = FALSE; > > + DEBUG_CODE_BEGIN (); > + > + // > + // The operation type check above only executes if the buffer happens to be > + // misaligned with respect to CWG, but even if it is aligned, we should not > + // allow arbitrary buffers to be used for creating consistent mappings. > + // 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); > + ASSERT_EFI_ERROR(Status); > + > + ASSERT (Operation != MapOperationBusMasterCommonBuffer || > + (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0); > + > + DEBUG_CODE_END (); > + > // Flush the Data Cache (should not have any effect if the memory region is uncached) > gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, EfiCpuFlushTypeWriteBackInvalidate); > } > -- > 2.5.0 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
On Wed, Apr 20, 2016 at 10:40:28AM +0200, Ard Biesheuvel wrote: > DmaMap () only allows uncached mappings to be used for creating consistent > mappings with operation type MapOperationBusMasterCommonBuffer. However, > if the buffer passed to DmaMap () happens to be aligned to the CWG, there > is no need for a bounce buffer, and we perform the cache maintenance > directly without ever checking if the memory attributes of the buffer > adhere to the API. > > So add some debug code that asserts that the operation type and the memory > attributes are consistent. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> Looks sane, thanks. Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org> > --- > > This applies on top of the ArmDmaLib patches that I sent out yesterday. > Based on the original code which contained some dodgy looking workarounds, > I expect this to actually break something, but Olivier is not around anymore > to tell me what he was trying to fix (and the broken 'promote cache > mainantenance by VA to set/way above a certain threshold' may well have been > the culprit here) > > Since the PCI emulation for non-coherent devices relies on this DMA code, > any non-coherent real platform that uses the SATA or EHCI/UCHI code could > be used to test this, but I don't have access to any of those. Suggestions > are welcome, as are donations of a Juno or a TC2. > > Thanks. > > ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > index 83f4d38a8a60..329756efc268 100644 > --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c > @@ -135,6 +135,23 @@ DmaMap ( > } else { > Map->DoubleBuffer = FALSE; > > + DEBUG_CODE_BEGIN (); > + > + // > + // The operation type check above only executes if the buffer happens to be > + // misaligned with respect to CWG, but even if it is aligned, we should not > + // allow arbitrary buffers to be used for creating consistent mappings. > + // 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); > + ASSERT_EFI_ERROR(Status); > + > + ASSERT (Operation != MapOperationBusMasterCommonBuffer || > + (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0); > + > + DEBUG_CODE_END (); > + > // Flush the Data Cache (should not have any effect if the memory region is uncached) > gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, EfiCpuFlushTypeWriteBackInvalidate); > } > -- > 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 83f4d38a8a60..329756efc268 100644 --- a/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c +++ b/ArmPkg/Library/ArmDmaLib/ArmDmaLib.c @@ -135,6 +135,23 @@ DmaMap ( } else { Map->DoubleBuffer = FALSE; + DEBUG_CODE_BEGIN (); + + // + // The operation type check above only executes if the buffer happens to be + // misaligned with respect to CWG, but even if it is aligned, we should not + // allow arbitrary buffers to be used for creating consistent mappings. + // 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); + ASSERT_EFI_ERROR(Status); + + ASSERT (Operation != MapOperationBusMasterCommonBuffer || + (GcdDescriptor.Attributes & (EFI_MEMORY_WB | EFI_MEMORY_WT)) == 0); + + DEBUG_CODE_END (); + // Flush the Data Cache (should not have any effect if the memory region is uncached) gCpu->FlushDataCache (gCpu, *DeviceAddress, *NumberOfBytes, EfiCpuFlushTypeWriteBackInvalidate); }
DmaMap () only allows uncached mappings to be used for creating consistent mappings with operation type MapOperationBusMasterCommonBuffer. However, if the buffer passed to DmaMap () happens to be aligned to the CWG, there is no need for a bounce buffer, and we perform the cache maintenance directly without ever checking if the memory attributes of the buffer adhere to the API. So add some debug code that asserts that the operation type and the memory attributes are consistent. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- This applies on top of the ArmDmaLib patches that I sent out yesterday. Based on the original code which contained some dodgy looking workarounds, I expect this to actually break something, but Olivier is not around anymore to tell me what he was trying to fix (and the broken 'promote cache mainantenance by VA to set/way above a certain threshold' may well have been the culprit here) Since the PCI emulation for non-coherent devices relies on this DMA code, any non-coherent real platform that uses the SATA or EHCI/UCHI code could be used to test this, but I don't have access to any of those. Suggestions are welcome, as are donations of a Juno or a TC2. Thanks. ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) -- 2.5.0 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel