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

[Leif Lindholm <leif.lindholm@linaro.org>]

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