* [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
@ 2018-12-12 9:12 Ard Biesheuvel
2018-12-12 11:30 ` Leif Lindholm
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-12-12 9:12 UTC (permalink / raw)
To: edk2-devel
Clang does not like the legacy 32-bit assembler syntax used in the
gdbstub exception handling routines, so update them to something
more fashionable.
So switch the order of the conditional suffix and the increment/decrement
specifier, and use decrement-after (da) and increment-before (ib) as
appropriate for an empty descending stack.
Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
index df1543a6d242..924578c7af2f 100644
--- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
+++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
@@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
GCC_ASM_EXPORT(GdbExceptionHandler)
.text
+.syntax unified
.align 3
@@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
cmpne R3, #0x10 @
- stmeqed R2, {lr}^ @ save unbanked lr
+ stmdaeq R2, {lr}^ @ save unbanked lr
@ else
- stmneed R2, {lr} @ save SVC lr
+ stmdane R2, {lr} @ save SVC lr
ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
@@ -245,9 +246,9 @@ GdbExceptionHandler (
and R1, R1, #0x1f @ Check to see if User or System Mode
cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
cmpne R1, #0x10 @
- ldmeqed R2, {lr}^ @ restore unbanked lr
+ ldmibeq R2, {lr}^ @ restore unbanked lr
@ else
- ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
+ ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
ldmfd SP!,{R0-R12} @ Restore general purpose registers
@ Exception handler can not change SP
--
2.19.2
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 9:12 [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions Ard Biesheuvel
@ 2018-12-12 11:30 ` Leif Lindholm
2018-12-12 11:33 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-12-12 11:30 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel
On Wed, Dec 12, 2018 at 10:12:11AM +0100, Ard Biesheuvel wrote:
> Clang does not like the legacy 32-bit assembler syntax used in the
> gdbstub exception handling routines, so update them to something
> more fashionable.
>
> So switch the order of the conditional suffix
Yes, this is essentially a bugfix. The syntax change predates ARM's
involvement in EDK2.
> and the increment/decrement
> specifier, and use decrement-after (da) and increment-before (ib) as
> appropriate for an empty descending stack.
But I very much prefer the FA/FD/EA/ED syntax. It gives symmetry,
which helps me a lot when dealing with 7-character mnemonics.
These aliases are also defined in the ARM ARM and won't be going away.
/
Leif
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> index df1543a6d242..924578c7af2f 100644
> --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> @@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
> GCC_ASM_EXPORT(GdbExceptionHandler)
>
> .text
> +.syntax unified
> .align 3
>
>
> @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> cmpne R3, #0x10 @
> - stmeqed R2, {lr}^ @ save unbanked lr
> + stmdaeq R2, {lr}^ @ save unbanked lr
> @ else
> - stmneed R2, {lr} @ save SVC lr
> + stmdane R2, {lr} @ save SVC lr
>
>
> ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> @@ -245,9 +246,9 @@ GdbExceptionHandler (
> and R1, R1, #0x1f @ Check to see if User or System Mode
> cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> cmpne R1, #0x10 @
> - ldmeqed R2, {lr}^ @ restore unbanked lr
> + ldmibeq R2, {lr}^ @ restore unbanked lr
> @ else
> - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
>
> ldmfd SP!,{R0-R12} @ Restore general purpose registers
> @ Exception handler can not change SP
> --
> 2.19.2
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 11:30 ` Leif Lindholm
@ 2018-12-12 11:33 ` Ard Biesheuvel
2018-12-12 12:31 ` Leif Lindholm
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-12-12 11:33 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org
On Wed, 12 Dec 2018 at 12:30, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Dec 12, 2018 at 10:12:11AM +0100, Ard Biesheuvel wrote:
> > Clang does not like the legacy 32-bit assembler syntax used in the
> > gdbstub exception handling routines, so update them to something
> > more fashionable.
> >
> > So switch the order of the conditional suffix
>
> Yes, this is essentially a bugfix. The syntax change predates ARM's
> involvement in EDK2.
>
> > and the increment/decrement
> > specifier, and use decrement-after (da) and increment-before (ib) as
> > appropriate for an empty descending stack.
>
> But I very much prefer the FA/FD/EA/ED syntax. It gives symmetry,
> which helps me a lot when dealing with 7-character mnemonics.
> These aliases are also defined in the ARM ARM and won't be going away.
>
Clang doesn't support them though.
This code is not used anywhere (it is referenced in BeagleBoardPkg but
commented out), so we could also simply remove it.
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> > EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
> > 1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > index df1543a6d242..924578c7af2f 100644
> > --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > @@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
> > GCC_ASM_EXPORT(GdbExceptionHandler)
> >
> > .text
> > +.syntax unified
> > .align 3
> >
> >
> > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > cmpne R3, #0x10 @
> > - stmeqed R2, {lr}^ @ save unbanked lr
> > + stmdaeq R2, {lr}^ @ save unbanked lr
> > @ else
> > - stmneed R2, {lr} @ save SVC lr
> > + stmdane R2, {lr} @ save SVC lr
> >
> >
> > ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > and R1, R1, #0x1f @ Check to see if User or System Mode
> > cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > cmpne R1, #0x10 @
> > - ldmeqed R2, {lr}^ @ restore unbanked lr
> > + ldmibeq R2, {lr}^ @ restore unbanked lr
> > @ else
> > - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> >
> > ldmfd SP!,{R0-R12} @ Restore general purpose registers
> > @ Exception handler can not change SP
> > --
> > 2.19.2
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 11:33 ` Ard Biesheuvel
@ 2018-12-12 12:31 ` Leif Lindholm
2018-12-12 12:45 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-12-12 12:31 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
On Wed, Dec 12, 2018 at 12:33:07PM +0100, Ard Biesheuvel wrote:
> On Wed, 12 Dec 2018 at 12:30, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Wed, Dec 12, 2018 at 10:12:11AM +0100, Ard Biesheuvel wrote:
> > > Clang does not like the legacy 32-bit assembler syntax used in the
> > > gdbstub exception handling routines, so update them to something
> > > more fashionable.
> > >
> > > So switch the order of the conditional suffix
> >
> > Yes, this is essentially a bugfix. The syntax change predates ARM's
> > involvement in EDK2.
> >
> > > and the increment/decrement
> > > specifier, and use decrement-after (da) and increment-before (ib) as
> > > appropriate for an empty descending stack.
> >
> > But I very much prefer the FA/FD/EA/ED syntax. It gives symmetry,
> > which helps me a lot when dealing with 7-character mnemonics.
> > These aliases are also defined in the ARM ARM and won't be going away.
>
> Clang doesn't support them though.
*Goes to check*
*Boggles*
Yeah, it supports FD/EA, but not FA/ED. That's ... special.
> This code is not used anywhere (it is referenced in BeagleBoardPkg but
> commented out), so we could also simply remove it.
> > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > ---
> > > EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
> > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > index df1543a6d242..924578c7af2f 100644
> > > --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > @@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
> > > GCC_ASM_EXPORT(GdbExceptionHandler)
> > >
> > > .text
> > > +.syntax unified
> > > .align 3
> > >
> > >
> > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > cmpne R3, #0x10 @
> > > - stmeqed R2, {lr}^ @ save unbanked lr
> > > + stmdaeq R2, {lr}^ @ save unbanked lr
Then again, looking closer at these instructions, they're not really
doing stack operations. Just (ab)using the instruction to get at the
banked User mode LR from a different mode. So
a) The symmetry thing doesn't really apply, so the ED is actively
misleading here.
b) This could trivially be changed to use FD anyway, just setting R2's
offset from PC to #0x34 :)
> > > @ else
> > > - stmneed R2, {lr} @ save SVC lr
> > > + stmdane R2, {lr} @ save SVC lr
> > >
> > >
> > > ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> > > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > > and R1, R1, #0x1f @ Check to see if User or System Mode
> > > cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > > cmpne R1, #0x10 @
> > > - ldmeqed R2, {lr}^ @ restore unbanked lr
> > > + ldmibeq R2, {lr}^ @ restore unbanked lr
> > > @ else
> > > - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > >
> > > ldmfd SP!,{R0-R12} @ Restore general purpose registers
> > > @ Exception handler can not change SP
> > > --
> > > 2.19.2
> > >
c) But given a), I would take this one if the comment was updated to
be explicit about how esoteric this operation really is.
I.e.: "(ab)use STM^ to save banked User mode LR from SVC/HYP mode"
and "(ab)use LDM^ to restore banked User mode LR from SVC/HYP mode"
I would also be happy to nuke it (including the commented out
inclusion for BeagleBoardPkg).
/
Leif
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 12:31 ` Leif Lindholm
@ 2018-12-12 12:45 ` Ard Biesheuvel
2018-12-12 14:41 ` Leif Lindholm
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-12-12 12:45 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org
On Wed, 12 Dec 2018 at 13:31, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Dec 12, 2018 at 12:33:07PM +0100, Ard Biesheuvel wrote:
> > On Wed, 12 Dec 2018 at 12:30, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > On Wed, Dec 12, 2018 at 10:12:11AM +0100, Ard Biesheuvel wrote:
> > > > Clang does not like the legacy 32-bit assembler syntax used in the
> > > > gdbstub exception handling routines, so update them to something
> > > > more fashionable.
> > > >
> > > > So switch the order of the conditional suffix
> > >
> > > Yes, this is essentially a bugfix. The syntax change predates ARM's
> > > involvement in EDK2.
> > >
> > > > and the increment/decrement
> > > > specifier, and use decrement-after (da) and increment-before (ib) as
> > > > appropriate for an empty descending stack.
> > >
> > > But I very much prefer the FA/FD/EA/ED syntax. It gives symmetry,
> > > which helps me a lot when dealing with 7-character mnemonics.
> > > These aliases are also defined in the ARM ARM and won't be going away.
> >
> > Clang doesn't support them though.
>
> *Goes to check*
>
> *Boggles*
>
> Yeah, it supports FD/EA, but not FA/ED. That's ... special.
>
> > This code is not used anywhere (it is referenced in BeagleBoardPkg but
> > commented out), so we could also simply remove it.
>
> > > > Contributed-under: TianoCore Contribution Agreement 1.1
> > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > > > ---
> > > > EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S | 9 +++++----
> > > > 1 file changed, 5 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > index df1543a6d242..924578c7af2f 100644
> > > > --- a/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > +++ b/EmbeddedPkg/Library/GdbDebugAgent/Arm/ExceptionSupport.ARMv6.S
> > > > @@ -57,6 +57,7 @@ GCC_ASM_EXPORT(AsmCommonExceptionEntry)
> > > > GCC_ASM_EXPORT(GdbExceptionHandler)
> > > >
> > > > .text
> > > > +.syntax unified
> > > > .align 3
> > > >
> > > >
> > > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > > > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > > cmpne R3, #0x10 @
> > > > - stmeqed R2, {lr}^ @ save unbanked lr
> > > > + stmdaeq R2, {lr}^ @ save unbanked lr
>
> Then again, looking closer at these instructions, they're not really
> doing stack operations. Just (ab)using the instruction to get at the
> banked User mode LR from a different mode. So
> a) The symmetry thing doesn't really apply, so the ED is actively
> misleading here.
> b) This could trivially be changed to use FD anyway, just setting R2's
> offset from PC to #0x34 :)
>
Actually, since there is no writeback, what is the point of the
decrement-after? Can't we just drop it?
The more I look at this code the more broken it seems. I'll just remove it.
> > > > @ else
> > > > - stmneed R2, {lr} @ save SVC lr
> > > > + stmdane R2, {lr} @ save SVC lr
> > > >
> > > >
> > > > ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> > > > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > > > and R1, R1, #0x1f @ Check to see if User or System Mode
> > > > cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > > > cmpne R1, #0x10 @
> > > > - ldmeqed R2, {lr}^ @ restore unbanked lr
> > > > + ldmibeq R2, {lr}^ @ restore unbanked lr
> > > > @ else
> > > > - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > > + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > >
> > > > ldmfd SP!,{R0-R12} @ Restore general purpose registers
> > > > @ Exception handler can not change SP
> > > > --
> > > > 2.19.2
> > > >
>
> c) But given a), I would take this one if the comment was updated to
> be explicit about how esoteric this operation really is.
> I.e.: "(ab)use STM^ to save banked User mode LR from SVC/HYP mode"
> and "(ab)use LDM^ to restore banked User mode LR from SVC/HYP mode"
>
> I would also be happy to nuke it (including the commented out
> inclusion for BeagleBoardPkg).
>
> /
> Leif
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 12:45 ` Ard Biesheuvel
@ 2018-12-12 14:41 ` Leif Lindholm
2018-12-12 14:49 ` Ard Biesheuvel
0 siblings, 1 reply; 8+ messages in thread
From: Leif Lindholm @ 2018-12-12 14:41 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
On Wed, Dec 12, 2018 at 01:45:17PM +0100, Ard Biesheuvel wrote:
> > > > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > > > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > > > > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > > > cmpne R3, #0x10 @
> > > > > - stmeqed R2, {lr}^ @ save unbanked lr
> > > > > + stmdaeq R2, {lr}^ @ save unbanked lr
> >
> > Then again, looking closer at these instructions, they're not really
> > doing stack operations. Just (ab)using the instruction to get at the
> > banked User mode LR from a different mode. So
> > a) The symmetry thing doesn't really apply, so the ED is actively
> > misleading here.
> > b) This could trivially be changed to use FD anyway, just setting R2's
> > offset from PC to #0x34 :)
> >
>
> Actually, since there is no writeback, what is the point of the
> decrement-after? Can't we just drop it?
Possibly picked specifically to _not_ be mistaken for a stack
operation. In this scenario The DA just means don't add 4 to R2 before
using it as address.
> The more I look at this code the more broken it seems. I'll just remove it.
As previously stated, I'm happy with this.
/
Leif
> > > > > @ else
> > > > > - stmneed R2, {lr} @ save SVC lr
> > > > > + stmdane R2, {lr} @ save SVC lr
> > > > >
> > > > >
> > > > > ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> > > > > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > > > > and R1, R1, #0x1f @ Check to see if User or System Mode
> > > > > cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > > > > cmpne R1, #0x10 @
> > > > > - ldmeqed R2, {lr}^ @ restore unbanked lr
> > > > > + ldmibeq R2, {lr}^ @ restore unbanked lr
> > > > > @ else
> > > > > - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > > > + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > > >
> > > > > ldmfd SP!,{R0-R12} @ Restore general purpose registers
> > > > > @ Exception handler can not change SP
> > > > > --
> > > > > 2.19.2
> > > > >
> >
> > c) But given a), I would take this one if the comment was updated to
> > be explicit about how esoteric this operation really is.
> > I.e.: "(ab)use STM^ to save banked User mode LR from SVC/HYP mode"
> > and "(ab)use LDM^ to restore banked User mode LR from SVC/HYP mode"
> >
> > I would also be happy to nuke it (including the commented out
> > inclusion for BeagleBoardPkg).
> >
> > /
> > Leif
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 14:41 ` Leif Lindholm
@ 2018-12-12 14:49 ` Ard Biesheuvel
2018-12-12 14:58 ` Leif Lindholm
0 siblings, 1 reply; 8+ messages in thread
From: Ard Biesheuvel @ 2018-12-12 14:49 UTC (permalink / raw)
To: Leif Lindholm; +Cc: edk2-devel@lists.01.org
On Wed, 12 Dec 2018 at 15:41, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Wed, Dec 12, 2018 at 01:45:17PM +0100, Ard Biesheuvel wrote:
> > > > > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > > > > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > > > > > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > > > > cmpne R3, #0x10 @
> > > > > > - stmeqed R2, {lr}^ @ save unbanked lr
> > > > > > + stmdaeq R2, {lr}^ @ save unbanked lr
> > >
> > > Then again, looking closer at these instructions, they're not really
> > > doing stack operations. Just (ab)using the instruction to get at the
> > > banked User mode LR from a different mode. So
> > > a) The symmetry thing doesn't really apply, so the ED is actively
> > > misleading here.
> > > b) This could trivially be changed to use FD anyway, just setting R2's
> > > offset from PC to #0x34 :)
> > >
> >
> > Actually, since there is no writeback, what is the point of the
> > decrement-after? Can't we just drop it?
>
> Possibly picked specifically to _not_ be mistaken for a stack
> operation. In this scenario The DA just means don't add 4 to R2 before
> using it as address.
>
Indeed. But in the load counterpart below, it means increment-before,
so it will restore lr from the wrong address.
> > The more I look at this code the more broken it seems. I'll just remove it.
>
> As previously stated, I'm happy with this.
>
> /
> Leif
>
> > > > > > @ else
> > > > > > - stmneed R2, {lr} @ save SVC lr
> > > > > > + stmdane R2, {lr} @ save SVC lr
> > > > > >
> > > > > >
> > > > > > ldr R5, [SP, #0x58] @ PC is the LR pushed by srsfd
> > > > > > @@ -245,9 +246,9 @@ GdbExceptionHandler (
> > > > > > and R1, R1, #0x1f @ Check to see if User or System Mode
> > > > > > cmp R1, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1f))
> > > > > > cmpne R1, #0x10 @
> > > > > > - ldmeqed R2, {lr}^ @ restore unbanked lr
> > > > > > + ldmibeq R2, {lr}^ @ restore unbanked lr
> > > > > > @ else
> > > > > > - ldmneed R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > > > > + ldmibne R3, {lr} @ restore SVC lr, via ldmfd SP!, {LR}
> > > > > >
> > > > > > ldmfd SP!,{R0-R12} @ Restore general purpose registers
> > > > > > @ Exception handler can not change SP
> > > > > > --
> > > > > > 2.19.2
> > > > > >
> > >
> > > c) But given a), I would take this one if the comment was updated to
> > > be explicit about how esoteric this operation really is.
> > > I.e.: "(ab)use STM^ to save banked User mode LR from SVC/HYP mode"
> > > and "(ab)use LDM^ to restore banked User mode LR from SVC/HYP mode"
> > >
> > > I would also be happy to nuke it (including the commented out
> > > inclusion for BeagleBoardPkg).
> > >
> > > /
> > > Leif
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
2018-12-12 14:49 ` Ard Biesheuvel
@ 2018-12-12 14:58 ` Leif Lindholm
0 siblings, 0 replies; 8+ messages in thread
From: Leif Lindholm @ 2018-12-12 14:58 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: edk2-devel@lists.01.org
On Wed, Dec 12, 2018 at 03:49:07PM +0100, Ard Biesheuvel wrote:
> On Wed, 12 Dec 2018 at 15:41, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > On Wed, Dec 12, 2018 at 01:45:17PM +0100, Ard Biesheuvel wrote:
> > > > > > > @@ -198,9 +199,9 @@ ASM_PFX(AsmCommonExceptionEntry):
> > > > > > > and R3, R1, #0x1f @ Check CPSR to see if User or System Mode
> > > > > > > cmp R3, #0x1f @ if ((CPSR == 0x10) || (CPSR == 0x1df))
> > > > > > > cmpne R3, #0x10 @
> > > > > > > - stmeqed R2, {lr}^ @ save unbanked lr
> > > > > > > + stmdaeq R2, {lr}^ @ save unbanked lr
> > > >
> > > > Then again, looking closer at these instructions, they're not really
> > > > doing stack operations. Just (ab)using the instruction to get at the
> > > > banked User mode LR from a different mode. So
> > > > a) The symmetry thing doesn't really apply, so the ED is actively
> > > > misleading here.
> > > > b) This could trivially be changed to use FD anyway, just setting R2's
> > > > offset from PC to #0x34 :)
> > > >
> > >
> > > Actually, since there is no writeback, what is the point of the
> > > decrement-after? Can't we just drop it?
> >
> > Possibly picked specifically to _not_ be mistaken for a stack
> > operation. In this scenario The DA just means don't add 4 to R2 before
> > using it as address.
> >
>
> Indeed. But in the load counterpart below, it means increment-before,
> so it will restore lr from the wrong address.
I do believe you are correct :)
Let's nuke this one.
/
Leif
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2018-12-12 14:58 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-12-12 9:12 [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions Ard Biesheuvel
2018-12-12 11:30 ` Leif Lindholm
2018-12-12 11:33 ` Ard Biesheuvel
2018-12-12 12:31 ` Leif Lindholm
2018-12-12 12:45 ` Ard Biesheuvel
2018-12-12 14:41 ` Leif Lindholm
2018-12-12 14:49 ` Ard Biesheuvel
2018-12-12 14:58 ` Leif Lindholm
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox