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

[Ard Biesheuvel <ard.biesheuvel@linaro.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