diff mbox

[edk2,v2,3/4] ArmPkg/ArmDmaLib: clean up abuse of device address

Message ID 1478955748-14819-4-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit df8c2668d7277030cf98e7b218549e0466fa5f6b
Headers show

Commit Message

Ard Biesheuvel Nov. 12, 2016, 1:02 p.m. UTC 
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 <ard.biesheuvel@linaro.org>

---
 ArmPkg/Library/ArmDmaLib/ArmDmaLib.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Leif Lindholm Nov. 14, 2016, 3:16 p.m. UTC | #1 
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 <ard.biesheuvel@linaro.org>


For the fix itself:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


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
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 15, 2016, 9:19 a.m. UTC | #2 
On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> 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 <ard.biesheuvel@linaro.org>

>

> For the fix itself:

> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>

> However, can we wait for a few Tested-by:s to ensure this fix does not

> reveal any companion bugs?

>


Perhaps, yes.

In case anyone is up to doing that, please find the branch here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset

However, given that the split CPU/bus master view is introduced in the
next patch, the only use case where the device address differs from
the host address is when using the DebugUncachedMemoryAllocationLib,
which is currently broken AFAICT (it attempts to unmap the linear
mapping of the allocation by setting the memory attributes to '0',
which triggers an assert in the ArmPkg MMU code)
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ryan Harkin Nov. 15, 2016, 11:34 a.m. UTC | #3 
On 15 November 2016 at 09:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>> 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 <ard.biesheuvel@linaro.org>

>>

>> For the fix itself:

>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>>

>> However, can we wait for a few Tested-by:s to ensure this fix does not

>> reveal any companion bugs?

>>

>

> Perhaps, yes.

>

> In case anyone is up to doing that, please find the branch here

> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset

>


I tested your branch on the usual victims (R0/1/2, FVP Foundation &
AEMv8 and TC2) and they all work fine for me.

Tested-by: Ryan Harkin <ryan.harkin@linaro.org>



> However, given that the split CPU/bus master view is introduced in the

> next patch, the only use case where the device address differs from

> the host address is when using the DebugUncachedMemoryAllocationLib,

> which is currently broken AFAICT (it attempts to unmap the linear

> mapping of the allocation by setting the memory attributes to '0',

> which triggers an assert in the ArmPkg MMU code)

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Nov. 15, 2016, 1:07 p.m. UTC | #4 
On 15 November 2016 at 11:34, Ryan Harkin <ryan.harkin@linaro.org> wrote:
> On 15 November 2016 at 09:19, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:

>> On 14 November 2016 at 16:16, Leif Lindholm <leif.lindholm@linaro.org> wrote:

>>> 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 <ard.biesheuvel@linaro.org>

>>>

>>> For the fix itself:

>>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>>>

>>> However, can we wait for a few Tested-by:s to ensure this fix does not

>>> reveal any companion bugs?

>>>

>>

>> Perhaps, yes.

>>

>> In case anyone is up to doing that, please find the branch here

>> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset

>>

>

> I tested your branch on the usual victims (R0/1/2, FVP Foundation &

> AEMv8 and TC2) and they all work fine for me.

>

> Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

>


Thanks!
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Nov. 15, 2016, 6:01 p.m. UTC | #5 
On Tue, Nov 15, 2016 at 01:07:37PM +0000, Ard Biesheuvel wrote:
> On 15 November 2016 at 11:34, Ryan Harkin <ryan.harkin@linaro.org> wrote:

> >>>> 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 <ard.biesheuvel@linaro.org>

> >>>

> >>> For the fix itself:

> >>> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> >>>

> >>> However, can we wait for a few Tested-by:s to ensure this fix does not

> >>> reveal any companion bugs?

> >>>

> >>

> >> Perhaps, yes.

> >>

> >> In case anyone is up to doing that, please find the branch here

> >> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset

> >>

> >

> > I tested your branch on the usual victims (R0/1/2, FVP Foundation &

> > AEMv8 and TC2) and they all work fine for me.

> >

> > Tested-by: Ryan Harkin <ryan.harkin@linaro.org>


OK, so on the off-hand chance that this does break some non-upstream
drivers, I'll wait for further comments until I'm back from holiday,
28 November, before pushing this.

Additional Tested-by:s would still be appreciated.

Regards,

Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Nov. 30, 2016, 4:45 p.m. UTC | #6 
On 15 November 2016 at 18:01, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>> >> In case anyone is up to doing that, please find the branch here

>> >> https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/log/?h=armdmalib-offset

>> >

>> > I tested your branch on the usual victims (R0/1/2, FVP Foundation &

>> > AEMv8 and TC2) and they all work fine for me.

>> >

>> > Tested-by: Ryan Harkin <ryan.harkin@linaro.org>

>

> OK, so on the off-hand chance that this does break some non-upstream

> drivers, I'll wait for further comments until I'm back from holiday,

> 28 November, before pushing this.


Series pushed as 3571884..bfe3427 - thanks!

/
    Leif
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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) {