diff mbox series

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

Message ID 1487756301-15646-2-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 91231b55d5302065fac71bf5040809aec65bb2f7
Headers show
Series AARCH64: enable stack alignment check | expand

Commit Message

Ard Biesheuvel Feb. 22, 2017, 9:38 a.m. UTC 
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(-)

-- 
2.7.4

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

Comments

Leif Lindholm Feb. 22, 2017, 12:06 p.m. UTC | #1 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Feb. 22, 2017, 12:52 p.m. UTC | #2 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Leif Lindholm Feb. 22, 2017, 1:11 p.m. UTC | #3 
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

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

Patch

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