Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions

[Leif Lindholm <leif.lindholm@linaro.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