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

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

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