diff mbox

[edk2,v2] ArmPlatformPkg/NorFlashDxe: use strictly aligned CopyMem()

Message ID 1473423502-7061-1-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit bdb10148fcf58f03933e9c25c19e618b972f8ccb
Headers show

Commit Message

Ard Biesheuvel Sept. 9, 2016, 12:18 p.m. UTC 
The UEFI spec stipulates that unaligned accesses should be enabled
on CPUs that support them, which means all of them, given that we
no longer support pre-v7 ARM cores, and the AARCH64 bindings mandate
support for unaligned accesses unconditionally.

This means that one should not assume that CopyMem () is safe to call
on regions that may be mapped using device attributes, which is the
case for the NOR flash. Since we have no control over the mappings when
running under the OS, and given that write accesses require device
mappings, we should not call CopyMem () in the read path either, but
use our own implementation that is guaranteed to take alignment into
account.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

---
v2:
- simplify the AlignedCopyMem () implementation, given that we do not require
  memmove() semantics for copying between NOR flash and memory
- document the above
- add macro to perform alignment check
- drop redundant/unnecessary casts and parentheses

 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 63 +++++++++++++++++++-
 1 file changed, 61 insertions(+), 2 deletions(-)

-- 
2.7.4

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

Comments

Leif Lindholm Sept. 9, 2016, 12:34 p.m. UTC | #1 
On Fri, Sep 09, 2016 at 01:18:22PM +0100, Ard Biesheuvel wrote:
> The UEFI spec stipulates that unaligned accesses should be enabled

> on CPUs that support them, which means all of them, given that we

> no longer support pre-v7 ARM cores, and the AARCH64 bindings mandate

> support for unaligned accesses unconditionally.

> 

> This means that one should not assume that CopyMem () is safe to call

> on regions that may be mapped using device attributes, which is the

> case for the NOR flash. Since we have no control over the mappings when

> running under the OS, and given that write accesses require device

> mappings, we should not call CopyMem () in the read path either, but

> use our own implementation that is guaranteed to take alignment into

> account.

> 

> Contributed-under: TianoCore Contribution Agreement 1.0

> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>


That's a lot less complex, thanks.
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>


> ---

> v2:

> - simplify the AlignedCopyMem () implementation, given that we do not require

>   memmove() semantics for copying between NOR flash and memory

> - document the above

> - add macro to perform alignment check

> - drop redundant/unnecessary casts and parentheses

> 

>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 63 +++++++++++++++++++-

>  1 file changed, 61 insertions(+), 2 deletions(-)

> 

> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c

> index 3abbe5cb32bc..ca61ac5e1983 100644

> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c

> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c

> @@ -744,6 +744,65 @@ NorFlashWriteBlocks (

>    return Status;

>  }

>  

> +#define BOTH_ALIGNED(a, b, align) ((((UINTN)(a) | (UINTN)(b)) & ((align) - 1)) == 0)

> +

> +/**

> +  Copy Length bytes from Source to Destination, using aligned accesses only.

> +  Note that this implementation uses memcpy() semantics rather then memmove()

> +  semantics, i.e., SourceBuffer and DestinationBuffer should not overlap.

> +

> +  @param  DestinationBuffer The target of the copy request.

> +  @param  SourceBuffer      The place to copy from.

> +  @param  Length            The number of bytes to copy.

> +

> +  @return Destination

> +

> +**/

> +STATIC

> +VOID *

> +AlignedCopyMem (

> +  OUT     VOID                      *DestinationBuffer,

> +  IN      CONST VOID                *SourceBuffer,

> +  IN      UINTN                     Length

> +  )

> +{

> +  UINT8             *Destination8;

> +  CONST UINT8       *Source8;

> +  UINT32            *Destination32;

> +  CONST UINT32      *Source32;

> +  UINT64            *Destination64;

> +  CONST UINT64      *Source64;

> +

> +  if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 8) && Length >= 8) {

> +    Destination64 = DestinationBuffer;

> +    Source64 = SourceBuffer;

> +    while (Length >= 8) {

> +      *Destination64++ = *Source64++;

> +      Length -= 8;

> +    }

> +

> +    Destination8 = (UINT8 *)Destination64;

> +    Source8 = (CONST UINT8 *)Source64;

> +  } else if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 4) && Length >= 4) {

> +    Destination32 = DestinationBuffer;

> +    Source32 = SourceBuffer;

> +    while (Length >= 4) {

> +      *Destination32++ = *Source32++;

> +      Length -= 4;

> +    }

> +

> +    Destination8 = (UINT8 *)Destination32;

> +    Source8 = (CONST UINT8 *)Source32;

> +  } else {

> +    Destination8 = DestinationBuffer;

> +    Source8 = SourceBuffer;

> +  }

> +  while (Length-- != 0) {

> +    *Destination8++ = *Source8++;

> +  }

> +  return DestinationBuffer;

> +}

> +

>  EFI_STATUS

>  NorFlashReadBlocks (

>    IN NOR_FLASH_INSTANCE   *Instance,

> @@ -791,7 +850,7 @@ NorFlashReadBlocks (

>    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);

>  

>    // Readout the data

> -  CopyMem(Buffer, (UINTN *)StartAddress, BufferSizeInBytes);

> +  AlignedCopyMem (Buffer, (VOID *)StartAddress, BufferSizeInBytes);

>  

>    return EFI_SUCCESS;

>  }

> @@ -832,7 +891,7 @@ NorFlashRead (

>    SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);

>  

>    // Readout the data

> -  CopyMem (Buffer, (UINTN *)(StartAddress + Offset), BufferSizeInBytes);

> +  AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);

>  

>    return EFI_SUCCESS;

>  }

> -- 

> 2.7.4

> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 9, 2016, 12:59 p.m. UTC | #2 
On 9 September 2016 at 13:34, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Fri, Sep 09, 2016 at 01:18:22PM +0100, Ard Biesheuvel wrote:

>> The UEFI spec stipulates that unaligned accesses should be enabled

>> on CPUs that support them, which means all of them, given that we

>> no longer support pre-v7 ARM cores, and the AARCH64 bindings mandate

>> support for unaligned accesses unconditionally.

>>

>> This means that one should not assume that CopyMem () is safe to call

>> on regions that may be mapped using device attributes, which is the

>> case for the NOR flash. Since we have no control over the mappings when

>> running under the OS, and given that write accesses require device

>> mappings, we should not call CopyMem () in the read path either, but

>> use our own implementation that is guaranteed to take alignment into

>> account.

>>

>> Contributed-under: TianoCore Contribution Agreement 1.0

>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

>

> That's a lot less complex, thanks.

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

>


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

Patch

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 3abbe5cb32bc..ca61ac5e1983 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -744,6 +744,65 @@  NorFlashWriteBlocks (
   return Status;
 }
 
+#define BOTH_ALIGNED(a, b, align) ((((UINTN)(a) | (UINTN)(b)) & ((align) - 1)) == 0)
+
+/**
+  Copy Length bytes from Source to Destination, using aligned accesses only.
+  Note that this implementation uses memcpy() semantics rather then memmove()
+  semantics, i.e., SourceBuffer and DestinationBuffer should not overlap.
+
+  @param  DestinationBuffer The target of the copy request.
+  @param  SourceBuffer      The place to copy from.
+  @param  Length            The number of bytes to copy.
+
+  @return Destination
+
+**/
+STATIC
+VOID *
+AlignedCopyMem (
+  OUT     VOID                      *DestinationBuffer,
+  IN      CONST VOID                *SourceBuffer,
+  IN      UINTN                     Length
+  )
+{
+  UINT8             *Destination8;
+  CONST UINT8       *Source8;
+  UINT32            *Destination32;
+  CONST UINT32      *Source32;
+  UINT64            *Destination64;
+  CONST UINT64      *Source64;
+
+  if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 8) && Length >= 8) {
+    Destination64 = DestinationBuffer;
+    Source64 = SourceBuffer;
+    while (Length >= 8) {
+      *Destination64++ = *Source64++;
+      Length -= 8;
+    }
+
+    Destination8 = (UINT8 *)Destination64;
+    Source8 = (CONST UINT8 *)Source64;
+  } else if (BOTH_ALIGNED(DestinationBuffer, SourceBuffer, 4) && Length >= 4) {
+    Destination32 = DestinationBuffer;
+    Source32 = SourceBuffer;
+    while (Length >= 4) {
+      *Destination32++ = *Source32++;
+      Length -= 4;
+    }
+
+    Destination8 = (UINT8 *)Destination32;
+    Source8 = (CONST UINT8 *)Source32;
+  } else {
+    Destination8 = DestinationBuffer;
+    Source8 = SourceBuffer;
+  }
+  while (Length-- != 0) {
+    *Destination8++ = *Source8++;
+  }
+  return DestinationBuffer;
+}
+
 EFI_STATUS
 NorFlashReadBlocks (
   IN NOR_FLASH_INSTANCE   *Instance,
@@ -791,7 +850,7 @@  NorFlashReadBlocks (
   SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
 
   // Readout the data
-  CopyMem(Buffer, (UINTN *)StartAddress, BufferSizeInBytes);
+  AlignedCopyMem (Buffer, (VOID *)StartAddress, BufferSizeInBytes);
 
   return EFI_SUCCESS;
 }
@@ -832,7 +891,7 @@  NorFlashRead (
   SEND_NOR_COMMAND (Instance->DeviceBaseAddress, 0, P30_CMD_READ_ARRAY);
 
   // Readout the data
-  CopyMem (Buffer, (UINTN *)(StartAddress + Offset), BufferSizeInBytes);
+  AlignedCopyMem (Buffer, (VOID *)StartAddress + Offset, BufferSizeInBytes);
 
   return EFI_SUCCESS;
 }