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
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
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
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
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
© 2016 - 2024 Red Hat, Inc.