public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: Leif Lindholm <leif.lindholm@linaro.org>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: "edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Subject: Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions
Date: Wed, 12 Dec 2018 12:31:37 +0000	[thread overview]
Message-ID: <20181212123137.bzwi46gsszljlsvw@bivouac.eciton.net> (raw)
In-Reply-To: <CAKv+Gu9sOtFpW2UYi5H7koFOM_2G1AMn12dhx+LG59Lj8PHvZQ@mail.gmail.com>

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


  reply	other threads:[~2018-12-12 12:31 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20181212123137.bzwi46gsszljlsvw@bivouac.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox