[edk2] ArmPkg/ArmDmaLib: assert that consistent mappings are uncached

Message ID 1461141628-20896-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit b64e44cc098ffd03ccbf1a635ae405ae98971419
Headers show

Commit Message

Ard Biesheuvel April 20, 2016, 8:40 a.m.
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

Comments

Ryan Harkin May 3, 2016, 11:42 a.m. | #1
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
Leif Lindholm May 9, 2016, 4:51 p.m. | #2
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

Patch

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);
   }