[PATCH 0/4] AARCH64: enable stack alignment check

[PATCH 0/4] AARCH64: enable stack alignment check

[Ard Biesheuvel]

This series enables the stack alignment check (SA) bit in the MMU for
AArch64 platforms, as mandated by the UEFI spec. No fixes were required
to make the existing asm code adhere to this requirement, but some issues
were spotted in review nonetheless, so these are fixed as well.

Ard Biesheuvel (4):
  ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers
  ArmPkg/ArmLib: AARCH64: set frame pointer in cache maintenance routine
  ArmPkg/ArmLib: AARCH64: allow the stack aligment (SA) bit to be
    managed
  ArmPkg/ArmMmuLib: AARCH64: enable stack alignment checking

 ArmPkg/Include/Chipset/AArch64.h                                         | 12 ++++++
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                           | 38 ++++++++++++++++-
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                         |  1 +
 ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
 4 files changed, 68 insertions(+), 26 deletions(-)

-- 
2.7.4

[PATCH 1/4] ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers

[Ard Biesheuvel]

The entry code in ArmPlatformStackSet () is a 1:1 transliteration of the
ARM version, which uses the callee preserved registers r3 - r7 (*) to
preserve the function arguments and the link register across a call to
ArmPlatformIsPrimaryCore ().

However, x4 - x7 are not callee preserved on AARCH64, and so we should use
registers >= x18 instead. While we're at it, drop an unnecessary preserve
of the link register, and simplify/deobfuscate the calculation of the
secondary stack position.

(*) On ARM, r3 is not callee preserved either, but this should be addressed
    in a separate patch.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
index 65d7d6c6d686..e219d53cb71d 100644
--- a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
+++ b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
@@ -22,13 +22,13 @@
 //  );
 ASM_FUNC(ArmPlatformStackSet)
   // Save parameters
-  mov   x6, x3
-  mov   x5, x2
-  mov   x4, x1
-  mov   x3, x0
+  mov   x26, x3
+  mov   x25, x2
+  mov   x24, x1
+  mov   x23, x0
 
   // Save the Link register
-  mov   x7, x30
+  mov   x27, x30
 
   // Identify Stack
   mov   x0, x1
@@ -36,13 +36,13 @@ ASM_FUNC(ArmPlatformStackSet)
   cmp   x0, #1
 
   // Restore parameters
-  mov   x0, x3
-  mov   x1, x4
-  mov   x2, x5
-  mov   x3, x6
+  mov   x0, x23
+  mov   x1, x24
+  mov   x2, x25
+  mov   x3, x26
 
   // Restore the Link register
-  mov   x30, x7
+  mov   x30, x27
 
   b.ne  0f
 
@@ -57,10 +57,7 @@ ASM_FUNC(ArmPlatformStackSet)
 //  IN UINTN SecondaryStackSize
 //  );
 ASM_FUNC(ArmPlatformStackSetPrimary)
-  // Save the Link register
-  mov   x4, x30
-
-  // Add stack of primary stack to StackBase
+  // Add size of primary stack to StackBase
   add   x0, x0, x2
 
   // Compute SecondaryCoresCount * SecondaryCoreStackSize
@@ -70,7 +67,7 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
   // Set Primary Stack ((StackBase + PrimaryStackSize) + (SecondaryCoresCount * SecondaryCoreStackSize))
   add   sp, x0, x3
 
-  br    x4
+  ret
 
 //VOID
 //ArmPlatformStackSetSecondary (
@@ -81,30 +78,28 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
 //  );
 ASM_FUNC(ArmPlatformStackSetSecondary)
   // Save the Link register
-  mov   x4, x30
+  mov   x24, x30
   mov   sp, x0
 
   // Get Core Position
   mov   x0, x1
   bl    ASM_PFX(ArmPlatformGetCorePosition)
-  mov   x5, x0
+  mov   x25, x0
 
   // Get Primary Core Position
   bl    ASM_PFX(ArmPlatformGetPrimaryCoreMpId)
   bl    ASM_PFX(ArmPlatformGetCorePosition)
 
   // Get Secondary Core Position. We should get consecutive secondary stack number from 1...(CoreCount-1)
-  cmp   x5, x0
-  b.ls  1f
+  cmp   x25, x0
+
   // Decrement the position if after the primary core
-  sub   x5, x5, #1
-1:
-  add   x5, x5, #1
+  cinc  x25, x25, ls
 
   // Compute top of the secondary stack
-  mul   x3, x3, x5
+  mul   x3, x3, x25
 
   // Set stack
   add   sp, sp, x3
 
-  br    x4
+  ret   x24
-- 
2.7.4

[PATCH 2/4] ArmPkg/ArmLib: AARCH64: set frame pointer in cache maintenance routine

[Ard Biesheuvel]

Stack and unstack the frame pointer according to the AAPCS in
AArch64AllDataCachesOperation ().

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index 5cee7c1519c3..c35c05fdf681 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -273,7 +273,7 @@ ASM_FUNC(ArmDisableBranchPrediction)
 ASM_FUNC(AArch64AllDataCachesOperation)
 // We can use regs 0-7 and 9-15 without having to save/restore.
 // Save our link register on the stack. - The stack must always be quad-word aligned
-  str   x30, [sp, #-16]!
+  stp   x29, x30, [sp, #-16]!
   mov   x1, x0                  // Save Function call in x1
   mrs   x6, clidr_el1           // Read EL1 CLIDR
   and   x3, x6, #0x7000000      // Mask out all but Level of Coherency (LoC)
@@ -324,7 +324,7 @@ L_Skip:
 L_Finished:
   dsb   sy
   isb
-  ldr   x30, [sp], #0x10
+  ldp   x29, x30, [sp], #0x10
   ret
 
 
-- 
2.7.4

[PATCH 3/4] ArmPkg/ArmLib: AARCH64: allow the stack aligment (SA) bit to be managed

[Ard Biesheuvel]

In preparation of enabling stack alignment checking, which is mandated
by the UEFI spec for AARCH64, add the code to manage this bit to ArmLib.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Include/Chipset/AArch64.h               | 12 +++++++
 ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 34 ++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
index 9aecb1df81e0..cebfc5da426a 100644
--- a/ArmPkg/Include/Chipset/AArch64.h
+++ b/ArmPkg/Include/Chipset/AArch64.h
@@ -194,6 +194,18 @@ ArmEnableAlignmentCheck (
 
 VOID
 EFIAPI
+ArmDisableStackAlignmentCheck (
+  VOID
+  );
+
+VOID
+EFIAPI
+ArmEnableStackAlignmentCheck (
+  VOID
+  );
+
+VOID
+EFIAPI
 ArmDisableAllExceptions (
   VOID
   );
diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
index c35c05fdf681..e53b5fdc5986 100644
--- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
+++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
@@ -20,6 +20,7 @@
 .set CTRL_M_BIT,      (1 << 0)
 .set CTRL_A_BIT,      (1 << 1)
 .set CTRL_C_BIT,      (1 << 2)
+.set CTRL_SA_BIT,     (1 << 3)
 .set CTRL_I_BIT,      (1 << 12)
 .set CTRL_V_BIT,      (1 << 12)
 .set CPACR_VFP_BITS,  (3 << 20)
@@ -259,6 +260,39 @@ ASM_FUNC(ArmDisableAlignmentCheck)
    isb
    ret
 
+ASM_FUNC(ArmEnableStackAlignmentCheck)
+   EL1_OR_EL2(x1)
+1: mrs     x0, sctlr_el1        // Get control register EL1
+   b       3f
+2: mrs     x0, sctlr_el2        // Get control register EL2
+3: orr     x0, x0, #CTRL_SA_BIT // Set SA (stack alignment check) bit
+   EL1_OR_EL2(x1)
+1: msr     sctlr_el1, x0        // Write back control register
+   b       3f
+2: msr     sctlr_el2, x0        // Write back control register
+3: dsb     sy
+   isb
+   ret
+
+
+ASM_FUNC(ArmDisableStackAlignmentCheck)
+   EL1_OR_EL2_OR_EL3(x1)
+1: mrs     x0, sctlr_el1        // Get control register EL1
+   b       4f
+2: mrs     x0, sctlr_el2        // Get control register EL2
+   b       4f
+3: mrs     x0, sctlr_el3        // Get control register EL3
+4: bic     x0, x0, #CTRL_SA_BIT // Clear SA (stack alignment check) bit
+   EL1_OR_EL2_OR_EL3(x1)
+1: msr     sctlr_el1, x0        // Write back control register
+   b       4f
+2: msr     sctlr_el2, x0        // Write back control register
+   b       4f
+3: msr     sctlr_el3, x0        // Write back control register
+4: dsb     sy
+   isb
+   ret
+
 
 // Always turned on in AArch64. Else implementation specific. Leave in for C compatibility for now
 ASM_FUNC(ArmEnableBranchPrediction)
-- 
2.7.4

[PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: enable stack alignment checking

[Ard Biesheuvel]

Enable the hardware stack alignment check, as mandated by the UEFI spec.
This ensures that the stack pointer is 16 byte aligned at each instance
where it is used as the base address in a load/store operation.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
index 9e0593ce598b..2f8f99d44a31 100644
--- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
+++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
@@ -734,6 +734,7 @@ ArmConfigureMmu (
               MAIR_ATTR(TT_ATTR_INDX_MEMORY_WRITE_BACK, MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK));       // mapped to EFI_MEMORY_WB
 
   ArmDisableAlignmentCheck ();
+  ArmEnableStackAlignmentCheck ();
   ArmEnableInstructionCache ();
   ArmEnableDataCache ();
 
-- 
2.7.4

Re: [PATCH 1/4] ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers

[Leif Lindholm]

On Wed, Feb 22, 2017 at 09:38:18AM +0000, Ard Biesheuvel wrote:
> The entry code in ArmPlatformStackSet () is a 1:1 transliteration of the
> ARM version, which uses the callee preserved registers r3 - r7 (*) to
> preserve the function arguments and the link register across a call to
> ArmPlatformIsPrimaryCore ().
> 
> However, x4 - x7 are not callee preserved on AARCH64, and so we should use
> registers >= x18 instead.

Err ... >= x19 really, no?
I mean, I guess it technically does not matter here, but the PCS
explicitly recommends against manual use of x18.

> While we're at it, drop an unnecessary preserve
> of the link register, and simplify/deobfuscate the calculation of the
> secondary stack position.
> 
> (*) On ARM, r3 is not callee preserved either, but this should be addressed
>     in a separate patch.

Agreed, but doesn't need to go into the final commit message?

> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
>  1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> index 65d7d6c6d686..e219d53cb71d 100644
> --- a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> +++ b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> @@ -22,13 +22,13 @@
>  //  );
>  ASM_FUNC(ArmPlatformStackSet)
>    // Save parameters
> -  mov   x6, x3
> -  mov   x5, x2
> -  mov   x4, x1
> -  mov   x3, x0
> +  mov   x26, x3
> +  mov   x25, x2
> +  mov   x24, x1
> +  mov   x23, x0

Hah, I was going to bikeshed about why you weren't starting from x19,
then realised you're just prepending the 2 and avoiding typos.
I approve.

If you fold in my comments on commit message:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

>  
>    // Save the Link register
> -  mov   x7, x30
> +  mov   x27, x30
>  
>    // Identify Stack
>    mov   x0, x1
> @@ -36,13 +36,13 @@ ASM_FUNC(ArmPlatformStackSet)
>    cmp   x0, #1
>  
>    // Restore parameters
> -  mov   x0, x3
> -  mov   x1, x4
> -  mov   x2, x5
> -  mov   x3, x6
> +  mov   x0, x23
> +  mov   x1, x24
> +  mov   x2, x25
> +  mov   x3, x26
>  
>    // Restore the Link register
> -  mov   x30, x7
> +  mov   x30, x27
>  
>    b.ne  0f
>  
> @@ -57,10 +57,7 @@ ASM_FUNC(ArmPlatformStackSet)
>  //  IN UINTN SecondaryStackSize
>  //  );
>  ASM_FUNC(ArmPlatformStackSetPrimary)
> -  // Save the Link register
> -  mov   x4, x30
> -
> -  // Add stack of primary stack to StackBase
> +  // Add size of primary stack to StackBase
>    add   x0, x0, x2
>  
>    // Compute SecondaryCoresCount * SecondaryCoreStackSize
> @@ -70,7 +67,7 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
>    // Set Primary Stack ((StackBase + PrimaryStackSize) + (SecondaryCoresCount * SecondaryCoreStackSize))
>    add   sp, x0, x3
>  
> -  br    x4
> +  ret
>  
>  //VOID
>  //ArmPlatformStackSetSecondary (
> @@ -81,30 +78,28 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
>  //  );
>  ASM_FUNC(ArmPlatformStackSetSecondary)
>    // Save the Link register
> -  mov   x4, x30
> +  mov   x24, x30
>    mov   sp, x0
>  
>    // Get Core Position
>    mov   x0, x1
>    bl    ASM_PFX(ArmPlatformGetCorePosition)
> -  mov   x5, x0
> +  mov   x25, x0
>  
>    // Get Primary Core Position
>    bl    ASM_PFX(ArmPlatformGetPrimaryCoreMpId)
>    bl    ASM_PFX(ArmPlatformGetCorePosition)
>  
>    // Get Secondary Core Position. We should get consecutive secondary stack number from 1...(CoreCount-1)
> -  cmp   x5, x0
> -  b.ls  1f
> +  cmp   x25, x0
> +
>    // Decrement the position if after the primary core
> -  sub   x5, x5, #1
> -1:
> -  add   x5, x5, #1
> +  cinc  x25, x25, ls
>  
>    // Compute top of the secondary stack
> -  mul   x3, x3, x5
> +  mul   x3, x3, x25
>  
>    // Set stack
>    add   sp, sp, x3
>  
> -  br    x4
> +  ret   x24
> -- 
> 2.7.4

Re: [PATCH 3/4] ArmPkg/ArmLib: AARCH64: allow the stack aligment (SA) bit to be managed

[Leif Lindholm]

On Wed, Feb 22, 2017 at 09:38:20AM +0000, Ard Biesheuvel wrote:
> In preparation of enabling stack alignment checking, which is mandated
> by the UEFI spec for AARCH64, add the code to manage this bit to ArmLib.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

> ---
>  ArmPkg/Include/Chipset/AArch64.h               | 12 +++++++
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 34 ++++++++++++++++++++
>  2 files changed, 46 insertions(+)
> 
> diff --git a/ArmPkg/Include/Chipset/AArch64.h b/ArmPkg/Include/Chipset/AArch64.h
> index 9aecb1df81e0..cebfc5da426a 100644
> --- a/ArmPkg/Include/Chipset/AArch64.h
> +++ b/ArmPkg/Include/Chipset/AArch64.h
> @@ -194,6 +194,18 @@ ArmEnableAlignmentCheck (
>  
>  VOID
>  EFIAPI
> +ArmDisableStackAlignmentCheck (
> +  VOID
> +  );
> +
> +VOID
> +EFIAPI
> +ArmEnableStackAlignmentCheck (
> +  VOID
> +  );
> +
> +VOID
> +EFIAPI
>  ArmDisableAllExceptions (
>    VOID
>    );
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index c35c05fdf681..e53b5fdc5986 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -20,6 +20,7 @@
>  .set CTRL_M_BIT,      (1 << 0)
>  .set CTRL_A_BIT,      (1 << 1)
>  .set CTRL_C_BIT,      (1 << 2)
> +.set CTRL_SA_BIT,     (1 << 3)
>  .set CTRL_I_BIT,      (1 << 12)
>  .set CTRL_V_BIT,      (1 << 12)
>  .set CPACR_VFP_BITS,  (3 << 20)
> @@ -259,6 +260,39 @@ ASM_FUNC(ArmDisableAlignmentCheck)
>     isb
>     ret
>  
> +ASM_FUNC(ArmEnableStackAlignmentCheck)
> +   EL1_OR_EL2(x1)
> +1: mrs     x0, sctlr_el1        // Get control register EL1
> +   b       3f
> +2: mrs     x0, sctlr_el2        // Get control register EL2
> +3: orr     x0, x0, #CTRL_SA_BIT // Set SA (stack alignment check) bit
> +   EL1_OR_EL2(x1)
> +1: msr     sctlr_el1, x0        // Write back control register
> +   b       3f
> +2: msr     sctlr_el2, x0        // Write back control register
> +3: dsb     sy
> +   isb
> +   ret
> +
> +
> +ASM_FUNC(ArmDisableStackAlignmentCheck)
> +   EL1_OR_EL2_OR_EL3(x1)
> +1: mrs     x0, sctlr_el1        // Get control register EL1
> +   b       4f
> +2: mrs     x0, sctlr_el2        // Get control register EL2
> +   b       4f
> +3: mrs     x0, sctlr_el3        // Get control register EL3
> +4: bic     x0, x0, #CTRL_SA_BIT // Clear SA (stack alignment check) bit
> +   EL1_OR_EL2_OR_EL3(x1)
> +1: msr     sctlr_el1, x0        // Write back control register
> +   b       4f
> +2: msr     sctlr_el2, x0        // Write back control register
> +   b       4f
> +3: msr     sctlr_el3, x0        // Write back control register
> +4: dsb     sy
> +   isb
> +   ret
> +
>  
>  // Always turned on in AArch64. Else implementation specific. Leave in for C compatibility for now
>  ASM_FUNC(ArmEnableBranchPrediction)
> -- 
> 2.7.4
> 

Re: [PATCH 4/4] ArmPkg/ArmMmuLib: AARCH64: enable stack alignment checking

[Leif Lindholm]

On Wed, Feb 22, 2017 at 09:38:21AM +0000, Ard Biesheuvel wrote:
> Enable the hardware stack alignment check, as mandated by the UEFI spec.
> This ensures that the stack pointer is 16 byte aligned at each instance
> where it is used as the base address in a load/store operation.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

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

> ---
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> index 9e0593ce598b..2f8f99d44a31 100644
> --- a/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> +++ b/ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c
> @@ -734,6 +734,7 @@ ArmConfigureMmu (
>                MAIR_ATTR(TT_ATTR_INDX_MEMORY_WRITE_BACK, MAIR_ATTR_NORMAL_MEMORY_WRITE_BACK));       // mapped to EFI_MEMORY_WB
>  
>    ArmDisableAlignmentCheck ();
> +  ArmEnableStackAlignmentCheck ();
>    ArmEnableInstructionCache ();
>    ArmEnableDataCache ();
>  
> -- 
> 2.7.4
> 

Re: [PATCH 1/4] ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers

[Ard Biesheuvel]

On 22 February 2017 at 12:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Feb 22, 2017 at 09:38:18AM +0000, Ard Biesheuvel wrote:
>> The entry code in ArmPlatformStackSet () is a 1:1 transliteration of the
>> ARM version, which uses the callee preserved registers r3 - r7 (*) to
>> preserve the function arguments and the link register across a call to
>> ArmPlatformIsPrimaryCore ().
>>
>> However, x4 - x7 are not callee preserved on AARCH64, and so we should use
>> registers >= x18 instead.
>
> Err ... >= x19 really, no?
> I mean, I guess it technically does not matter here, but the PCS
> explicitly recommends against manual use of x18.
>

It recommends against it on portable code, because it is up to the
OS/execution environment to define the semantics of x18. Since the
UEFI spec does not mention x18 at all (except for the definition of
EFI_SYSTEM_CONTEXT_AARCH64), it indeed makes sense to avoid it.

>> While we're at it, drop an unnecessary preserve
>> of the link register, and simplify/deobfuscate the calculation of the
>> secondary stack position.
>>
>> (*) On ARM, r3 is not callee preserved either, but this should be addressed
>>     in a separate patch.
>
> Agreed, but doesn't need to go into the final commit message?
>

Nope, but stating that r3 - r7 are callee save on ARM without
mentioning that this is a mistake feels wrong as well.

>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
>>  1 file changed, 19 insertions(+), 24 deletions(-)
>>
>> diff --git a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
>> index 65d7d6c6d686..e219d53cb71d 100644
>> --- a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
>> +++ b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
>> @@ -22,13 +22,13 @@
>>  //  );
>>  ASM_FUNC(ArmPlatformStackSet)
>>    // Save parameters
>> -  mov   x6, x3
>> -  mov   x5, x2
>> -  mov   x4, x1
>> -  mov   x3, x0
>> +  mov   x26, x3
>> +  mov   x25, x2
>> +  mov   x24, x1
>> +  mov   x23, x0
>
> Hah, I was going to bikeshed about why you weren't starting from x19,
> then realised you're just prepending the 2 and avoiding typos.
> I approve.
>
> If you fold in my comments on commit message:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
>>
>>    // Save the Link register
>> -  mov   x7, x30
>> +  mov   x27, x30
>>
>>    // Identify Stack
>>    mov   x0, x1
>> @@ -36,13 +36,13 @@ ASM_FUNC(ArmPlatformStackSet)
>>    cmp   x0, #1
>>
>>    // Restore parameters
>> -  mov   x0, x3
>> -  mov   x1, x4
>> -  mov   x2, x5
>> -  mov   x3, x6
>> +  mov   x0, x23
>> +  mov   x1, x24
>> +  mov   x2, x25
>> +  mov   x3, x26
>>
>>    // Restore the Link register
>> -  mov   x30, x7
>> +  mov   x30, x27
>>
>>    b.ne  0f
>>
>> @@ -57,10 +57,7 @@ ASM_FUNC(ArmPlatformStackSet)
>>  //  IN UINTN SecondaryStackSize
>>  //  );
>>  ASM_FUNC(ArmPlatformStackSetPrimary)
>> -  // Save the Link register
>> -  mov   x4, x30
>> -
>> -  // Add stack of primary stack to StackBase
>> +  // Add size of primary stack to StackBase
>>    add   x0, x0, x2
>>
>>    // Compute SecondaryCoresCount * SecondaryCoreStackSize
>> @@ -70,7 +67,7 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
>>    // Set Primary Stack ((StackBase + PrimaryStackSize) + (SecondaryCoresCount * SecondaryCoreStackSize))
>>    add   sp, x0, x3
>>
>> -  br    x4
>> +  ret
>>
>>  //VOID
>>  //ArmPlatformStackSetSecondary (
>> @@ -81,30 +78,28 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
>>  //  );
>>  ASM_FUNC(ArmPlatformStackSetSecondary)
>>    // Save the Link register
>> -  mov   x4, x30
>> +  mov   x24, x30
>>    mov   sp, x0
>>
>>    // Get Core Position
>>    mov   x0, x1
>>    bl    ASM_PFX(ArmPlatformGetCorePosition)
>> -  mov   x5, x0
>> +  mov   x25, x0
>>
>>    // Get Primary Core Position
>>    bl    ASM_PFX(ArmPlatformGetPrimaryCoreMpId)
>>    bl    ASM_PFX(ArmPlatformGetCorePosition)
>>
>>    // Get Secondary Core Position. We should get consecutive secondary stack number from 1...(CoreCount-1)
>> -  cmp   x5, x0
>> -  b.ls  1f
>> +  cmp   x25, x0
>> +
>>    // Decrement the position if after the primary core
>> -  sub   x5, x5, #1
>> -1:
>> -  add   x5, x5, #1
>> +  cinc  x25, x25, ls
>>
>>    // Compute top of the secondary stack
>> -  mul   x3, x3, x5
>> +  mul   x3, x3, x25
>>
>>    // Set stack
>>    add   sp, sp, x3
>>
>> -  br    x4
>> +  ret   x24
>> --
>> 2.7.4

Re: [PATCH 2/4] ArmPkg/ArmLib: AARCH64: set frame pointer in cache maintenance routine

[Ard Biesheuvel]

On 22 February 2017 at 09:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Stack and unstack the frame pointer according to the AAPCS in
> AArch64AllDataCachesOperation ().
>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> index 5cee7c1519c3..c35c05fdf681 100644
> --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> @@ -273,7 +273,7 @@ ASM_FUNC(ArmDisableBranchPrediction)
>  ASM_FUNC(AArch64AllDataCachesOperation)
>  // We can use regs 0-7 and 9-15 without having to save/restore.
>  // Save our link register on the stack. - The stack must always be quad-word aligned
> -  str   x30, [sp, #-16]!
> +  stp   x29, x30, [sp, #-16]!

As discussed over IRC, this needs

mov x29, sp

appended as well.

This ensures that this function will show up in a backtrace when an
exception is taken (and not handled) in the callback invoked by this
function. Without setting (and preserving) x29, it will still point to
the caller of AArch64AllDataCachesOperation()

>    mov   x1, x0                  // Save Function call in x1
>    mrs   x6, clidr_el1           // Read EL1 CLIDR
>    and   x3, x6, #0x7000000      // Mask out all but Level of Coherency (LoC)
> @@ -324,7 +324,7 @@ L_Skip:
>  L_Finished:
>    dsb   sy
>    isb
> -  ldr   x30, [sp], #0x10
> +  ldp   x29, x30, [sp], #0x10
>    ret
>
>
> --
> 2.7.4
>

Re: [PATCH 1/4] ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers

[Leif Lindholm]

On Wed, Feb 22, 2017 at 12:52:10PM +0000, Ard Biesheuvel wrote:
> On 22 February 2017 at 12:06, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > On Wed, Feb 22, 2017 at 09:38:18AM +0000, Ard Biesheuvel wrote:
> >> The entry code in ArmPlatformStackSet () is a 1:1 transliteration of the
> >> ARM version, which uses the callee preserved registers r3 - r7 (*) to
> >> preserve the function arguments and the link register across a call to
> >> ArmPlatformIsPrimaryCore ().
> >>
> >> However, x4 - x7 are not callee preserved on AARCH64, and so we should use
> >> registers >= x18 instead.
> >
> > Err ... >= x19 really, no?
> > I mean, I guess it technically does not matter here, but the PCS
> > explicitly recommends against manual use of x18.
> >
> 
> It recommends against it on portable code, because it is up to the
> OS/execution environment to define the semantics of x18. Since the
> UEFI spec does not mention x18 at all (except for the definition of
> EFI_SYSTEM_CONTEXT_AARCH64), it indeed makes sense to avoid it.
> 
> >> While we're at it, drop an unnecessary preserve
> >> of the link register, and simplify/deobfuscate the calculation of the
> >> secondary stack position.
> >>
> >> (*) On ARM, r3 is not callee preserved either, but this should be addressed
> >>     in a separate patch.
> >
> > Agreed, but doesn't need to go into the final commit message?
> >
> 
> Nope, but stating that r3 - r7 are callee save on ARM without
> mentioning that this is a mistake feels wrong as well.

Fair enough.
You can drop that one and use the reviewed-by.

> >>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> ---
> >>  ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
> >>  1 file changed, 19 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> >> index 65d7d6c6d686..e219d53cb71d 100644
> >> --- a/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> >> +++ b/ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S
> >> @@ -22,13 +22,13 @@
> >>  //  );
> >>  ASM_FUNC(ArmPlatformStackSet)
> >>    // Save parameters
> >> -  mov   x6, x3
> >> -  mov   x5, x2
> >> -  mov   x4, x1
> >> -  mov   x3, x0
> >> +  mov   x26, x3
> >> +  mov   x25, x2
> >> +  mov   x24, x1
> >> +  mov   x23, x0
> >
> > Hah, I was going to bikeshed about why you weren't starting from x19,
> > then realised you're just prepending the 2 and avoiding typos.
> > I approve.
> >
> > If you fold in my comments on commit message:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
> >>
> >>    // Save the Link register
> >> -  mov   x7, x30
> >> +  mov   x27, x30
> >>
> >>    // Identify Stack
> >>    mov   x0, x1
> >> @@ -36,13 +36,13 @@ ASM_FUNC(ArmPlatformStackSet)
> >>    cmp   x0, #1
> >>
> >>    // Restore parameters
> >> -  mov   x0, x3
> >> -  mov   x1, x4
> >> -  mov   x2, x5
> >> -  mov   x3, x6
> >> +  mov   x0, x23
> >> +  mov   x1, x24
> >> +  mov   x2, x25
> >> +  mov   x3, x26
> >>
> >>    // Restore the Link register
> >> -  mov   x30, x7
> >> +  mov   x30, x27
> >>
> >>    b.ne  0f
> >>
> >> @@ -57,10 +57,7 @@ ASM_FUNC(ArmPlatformStackSet)
> >>  //  IN UINTN SecondaryStackSize
> >>  //  );
> >>  ASM_FUNC(ArmPlatformStackSetPrimary)
> >> -  // Save the Link register
> >> -  mov   x4, x30
> >> -
> >> -  // Add stack of primary stack to StackBase
> >> +  // Add size of primary stack to StackBase
> >>    add   x0, x0, x2
> >>
> >>    // Compute SecondaryCoresCount * SecondaryCoreStackSize
> >> @@ -70,7 +67,7 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
> >>    // Set Primary Stack ((StackBase + PrimaryStackSize) + (SecondaryCoresCount * SecondaryCoreStackSize))
> >>    add   sp, x0, x3
> >>
> >> -  br    x4
> >> +  ret
> >>
> >>  //VOID
> >>  //ArmPlatformStackSetSecondary (
> >> @@ -81,30 +78,28 @@ ASM_FUNC(ArmPlatformStackSetPrimary)
> >>  //  );
> >>  ASM_FUNC(ArmPlatformStackSetSecondary)
> >>    // Save the Link register
> >> -  mov   x4, x30
> >> +  mov   x24, x30
> >>    mov   sp, x0
> >>
> >>    // Get Core Position
> >>    mov   x0, x1
> >>    bl    ASM_PFX(ArmPlatformGetCorePosition)
> >> -  mov   x5, x0
> >> +  mov   x25, x0
> >>
> >>    // Get Primary Core Position
> >>    bl    ASM_PFX(ArmPlatformGetPrimaryCoreMpId)
> >>    bl    ASM_PFX(ArmPlatformGetCorePosition)
> >>
> >>    // Get Secondary Core Position. We should get consecutive secondary stack number from 1...(CoreCount-1)
> >> -  cmp   x5, x0
> >> -  b.ls  1f
> >> +  cmp   x25, x0
> >> +
> >>    // Decrement the position if after the primary core
> >> -  sub   x5, x5, #1
> >> -1:
> >> -  add   x5, x5, #1
> >> +  cinc  x25, x25, ls
> >>
> >>    // Compute top of the secondary stack
> >> -  mul   x3, x3, x5
> >> +  mul   x3, x3, x25
> >>
> >>    // Set stack
> >>    add   sp, sp, x3
> >>
> >> -  br    x4
> >> +  ret   x24
> >> --
> >> 2.7.4

Re: [PATCH 2/4] ArmPkg/ArmLib: AARCH64: set frame pointer in cache maintenance routine

[Leif Lindholm]

On Wed, Feb 22, 2017 at 12:56:04PM +0000, Ard Biesheuvel wrote:
> On 22 February 2017 at 09:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > Stack and unstack the frame pointer according to the AAPCS in
> > AArch64AllDataCachesOperation ().
> >
> > Contributed-under: TianoCore Contribution Agreement 1.0
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> > index 5cee7c1519c3..c35c05fdf681 100644
> > --- a/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> > +++ b/ArmPkg/Library/ArmLib/AArch64/AArch64Support.S
> > @@ -273,7 +273,7 @@ ASM_FUNC(ArmDisableBranchPrediction)
> >  ASM_FUNC(AArch64AllDataCachesOperation)
> >  // We can use regs 0-7 and 9-15 without having to save/restore.
> >  // Save our link register on the stack. - The stack must always be quad-word aligned
> > -  str   x30, [sp, #-16]!
> > +  stp   x29, x30, [sp, #-16]!
> 
> As discussed over IRC, this needs
> 
> mov x29, sp
> 
> appended as well.
> 
> This ensures that this function will show up in a backtrace when an
> exception is taken (and not handled) in the callback invoked by this
> function. Without setting (and preserving) x29, it will still point to
> the caller of AArch64AllDataCachesOperation()

Sounds good.
With that addition:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> >    mov   x1, x0                  // Save Function call in x1
> >    mrs   x6, clidr_el1           // Read EL1 CLIDR
> >    and   x3, x6, #0x7000000      // Mask out all but Level of Coherency (LoC)
> > @@ -324,7 +324,7 @@ L_Skip:
> >  L_Finished:
> >    dsb   sy
> >    isb
> > -  ldr   x30, [sp], #0x10
> > +  ldp   x29, x30, [sp], #0x10
> >    ret
> >
> >
> > --
> > 2.7.4
> >

Re: [PATCH 0/4] AARCH64: enable stack alignment check

[Ard Biesheuvel]

On 22 February 2017 at 09:38, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> This series enables the stack alignment check (SA) bit in the MMU for
> AArch64 platforms, as mandated by the UEFI spec. No fixes were required
> to make the existing asm code adhere to this requirement, but some issues
> were spotted in review nonetheless, so these are fixed as well.
>

All pushed, thanks.

> Ard Biesheuvel (4):
>   ArmPlatformPkg/ArmPlatformStackLib: use callee preserved registers
>   ArmPkg/ArmLib: AARCH64: set frame pointer in cache maintenance routine
>   ArmPkg/ArmLib: AARCH64: allow the stack aligment (SA) bit to be
>     managed
>   ArmPkg/ArmMmuLib: AARCH64: enable stack alignment checking
>
>  ArmPkg/Include/Chipset/AArch64.h                                         | 12 ++++++
>  ArmPkg/Library/ArmLib/AArch64/AArch64Support.S                           | 38 ++++++++++++++++-
>  ArmPkg/Library/ArmMmuLib/AArch64/ArmMmuLibCore.c                         |  1 +
>  ArmPlatformPkg/Library/ArmPlatformStackLib/AArch64/ArmPlatformStackLib.S | 43 +++++++++-----------
>  4 files changed, 68 insertions(+), 26 deletions(-)
>
> --
> 2.7.4
>