From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::444; helo=mail-wr1-x444.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wr1-x444.google.com (mail-wr1-x444.google.com [IPv6:2a00:1450:4864:20::444]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 884CE2119C8AF for ; Wed, 12 Dec 2018 04:31:41 -0800 (PST) Received: by mail-wr1-x444.google.com with SMTP id l9so17469044wrt.13 for ; Wed, 12 Dec 2018 04:31:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=vR1EhPtUiP4fs75ST6/DDt6sYJ4uQRi/lYyybQ2rR2k=; b=eHi/Vgn2q+SXD9qnalkStj0h2CQ5DDZKo5V2EkwF7ihq04vdix6GDF4s4t4skvpVsD /MMSIrRtucV8wiSO0IuRF8s+GXknHyzAbEU3XCvJ4C4o5IiGmLABys8P6BdQLmZaamoK W+a03iA9IHaCuKNj8OY0dW7CjLVHJeHp4Jk4Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=vR1EhPtUiP4fs75ST6/DDt6sYJ4uQRi/lYyybQ2rR2k=; b=G/nX3mfUpJnflFxngMj5uLhgEe09Xn88VTZMYk22L7OUFeyrhlllqn8JjWArnqAuIp qRxL8P2mW0VKqYFCiVAPBo/xXCQxJwDf+urgUZ7Car3MXxVUvNg9Rsb8idvvTrg3faTX VrKy2vORK+kwMnD9Fk0IM4g/0aVUwGjmX7bOabpHmC331PrI9YaoF8l6FMaNXT3czYlg e5LOw6xF+qSyDvamXhhQyq4vqTmmECyNas3VEloBKyr9m0z0ygXOGgkWIEewdVEvMfXw 11nWMNatWZDmzfCpJ5Gu58RVSXFiOIwBDYe6dhYO2ZtGcEPXKNb3CLsTjKmxLVBGH6mk vxMA== X-Gm-Message-State: AA+aEWZczODuoxqSQVTxxAbJQOXsurzFHfevPVKPw5lshP/GxXOPRZ1A yyfMSdYrHwatdDnWTY6lqtZIEw== X-Google-Smtp-Source: AFSGD/WjSrMhWnNagIjbaxAhtU5qM0+h7cOGgeEznaGanmdOfhKi+e/XqS8zTmrbx2dD4kRsTqlong== X-Received: by 2002:a05:6000:51:: with SMTP id k17mr16190074wrx.259.1544617899915; Wed, 12 Dec 2018 04:31:39 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id t63sm2938172wmt.8.2018.12.12.04.31.39 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Wed, 12 Dec 2018 04:31:39 -0800 (PST) Date: Wed, 12 Dec 2018 12:31:37 +0000 From: Leif Lindholm To: Ard Biesheuvel Cc: "edk2-devel@lists.01.org" Message-ID: <20181212123137.bzwi46gsszljlsvw@bivouac.eciton.net> References: <20181212091211.13258-1-ard.biesheuvel@linaro.org> <20181212113009.vmsv37swaui3z4os@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [PATCH] EmbeddedPkg/GdbDebugAgent ARM: use modern dialect for ldm/stm instructions X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Dec 2018 12:31:41 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline On Wed, Dec 12, 2018 at 12:33:07PM +0100, Ard Biesheuvel wrote: > On Wed, 12 Dec 2018 at 12:30, Leif Lindholm 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 > > > --- > > > 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