All of lore.kernel.org
 help / color / mirror / Atom feed
From: catalin.marinas@arm.com (Catalin Marinas)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v4 1/2] arm64: adjust el0_sync so that a function can be called
Date: Fri, 23 May 2014 15:44:57 +0100	[thread overview]
Message-ID: <20140523144457.GN9252@arm.com> (raw)
In-Reply-To: <20140522223520.GA7953@ubuette>

On Thu, May 22, 2014 at 11:35:20PM +0100, Larry Bassel wrote:
> > On 05/22/2014 03:27 PM, Larry Bassel wrote:
> > > To implement the context tracker properly on arm64,
> > > a function call needs to be made after debugging and
> > > interrupts are turned on, but before the lr is changed
> > > to point to ret_to_user(). If the function call
> > > is made after the lr is changed the function will not
> > > return to the correct place.
> > > 
> > > For similar reasons, defer the setting of x0 so that
> > > it doesn't need to be saved around the function call
> > > (save far_el1 in x26 temporarily instead).
> > > 
> > > Signed-off-by: Larry Bassel <larry.bassel@linaro.org>
> > > ---
> > >  arch/arm64/kernel/entry.S | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index e8b23a3..20b336e 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -354,7 +354,6 @@ el0_sync:
> > >  	lsr	x24, x25, #ESR_EL1_EC_SHIFT	// exception class
> > >  	cmp	x24, #ESR_EL1_EC_SVC64		// SVC in 64-bit state
> > >  	b.eq	el0_svc
> > > -	adr	lr, ret_to_user
> > >  	cmp	x24, #ESR_EL1_EC_DABT_EL0	// data abort in EL0
> > >  	b.eq	el0_da
> > >  	cmp	x24, #ESR_EL1_EC_IABT_EL0	// instruction abort in EL0
> > > @@ -383,7 +382,6 @@ el0_sync_compat:
> > >  	lsr	x24, x25, #ESR_EL1_EC_SHIFT	// exception class
> > >  	cmp	x24, #ESR_EL1_EC_SVC32		// SVC in 32-bit state
> > >  	b.eq	el0_svc_compat
> > > -	adr	lr, ret_to_user
> > >  	cmp	x24, #ESR_EL1_EC_DABT_EL0	// data abort in EL0
> > >  	b.eq	el0_da
> > >  	cmp	x24, #ESR_EL1_EC_IABT_EL0	// instruction abort in EL0
> > > @@ -426,22 +424,26 @@ el0_da:
> > >  	/*
> > >  	 * Data abort handling
> > >  	 */
> > > -	mrs	x0, far_el1
> > > -	bic	x0, x0, #(0xff << 56)
> > > +	mrs	x26, far_el1
> > >  	// enable interrupts before calling the main handler
> > >  	enable_dbg_and_irq
> > > +	mov	x0, x26
> > > +	bic	x0, x0, #(0xff << 56)
> > 
> > Nit: I believe you can bit clear with x26 as the source register and omit the
> > move instruction.
> 
> Is that really an improvement (assuming it works)? Are we saving
> any cycles here? If so, does it matter? It is easy to see what
> the move instruction is doing.

Even if it's not noticeable, I would still reduce the number of lines by
one. BIC with immediate is just an alias for AND and it supports
different source and destination.

-- 
Catalin

WARNING: multiple messages have this Message-ID (diff)
From: Catalin Marinas <catalin.marinas@arm.com>
To: Larry Bassel <larry.bassel@linaro.org>
Cc: Christopher Covington <cov@codeaurora.org>,
	Will Deacon <Will.Deacon@arm.com>,
	"khilman@linaro.org" <khilman@linaro.org>,
	"linaro-kernel@lists.linaro.org" <linaro-kernel@lists.linaro.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 1/2] arm64: adjust el0_sync so that a function can be called
Date: Fri, 23 May 2014 15:44:57 +0100	[thread overview]
Message-ID: <20140523144457.GN9252@arm.com> (raw)
In-Reply-To: <20140522223520.GA7953@ubuette>

On Thu, May 22, 2014 at 11:35:20PM +0100, Larry Bassel wrote:
> > On 05/22/2014 03:27 PM, Larry Bassel wrote:
> > > To implement the context tracker properly on arm64,
> > > a function call needs to be made after debugging and
> > > interrupts are turned on, but before the lr is changed
> > > to point to ret_to_user(). If the function call
> > > is made after the lr is changed the function will not
> > > return to the correct place.
> > > 
> > > For similar reasons, defer the setting of x0 so that
> > > it doesn't need to be saved around the function call
> > > (save far_el1 in x26 temporarily instead).
> > > 
> > > Signed-off-by: Larry Bassel <larry.bassel@linaro.org>
> > > ---
> > >  arch/arm64/kernel/entry.S | 24 +++++++++++++++++-------
> > >  1 file changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > > index e8b23a3..20b336e 100644
> > > --- a/arch/arm64/kernel/entry.S
> > > +++ b/arch/arm64/kernel/entry.S
> > > @@ -354,7 +354,6 @@ el0_sync:
> > >  	lsr	x24, x25, #ESR_EL1_EC_SHIFT	// exception class
> > >  	cmp	x24, #ESR_EL1_EC_SVC64		// SVC in 64-bit state
> > >  	b.eq	el0_svc
> > > -	adr	lr, ret_to_user
> > >  	cmp	x24, #ESR_EL1_EC_DABT_EL0	// data abort in EL0
> > >  	b.eq	el0_da
> > >  	cmp	x24, #ESR_EL1_EC_IABT_EL0	// instruction abort in EL0
> > > @@ -383,7 +382,6 @@ el0_sync_compat:
> > >  	lsr	x24, x25, #ESR_EL1_EC_SHIFT	// exception class
> > >  	cmp	x24, #ESR_EL1_EC_SVC32		// SVC in 32-bit state
> > >  	b.eq	el0_svc_compat
> > > -	adr	lr, ret_to_user
> > >  	cmp	x24, #ESR_EL1_EC_DABT_EL0	// data abort in EL0
> > >  	b.eq	el0_da
> > >  	cmp	x24, #ESR_EL1_EC_IABT_EL0	// instruction abort in EL0
> > > @@ -426,22 +424,26 @@ el0_da:
> > >  	/*
> > >  	 * Data abort handling
> > >  	 */
> > > -	mrs	x0, far_el1
> > > -	bic	x0, x0, #(0xff << 56)
> > > +	mrs	x26, far_el1
> > >  	// enable interrupts before calling the main handler
> > >  	enable_dbg_and_irq
> > > +	mov	x0, x26
> > > +	bic	x0, x0, #(0xff << 56)
> > 
> > Nit: I believe you can bit clear with x26 as the source register and omit the
> > move instruction.
> 
> Is that really an improvement (assuming it works)? Are we saving
> any cycles here? If so, does it matter? It is easy to see what
> the move instruction is doing.

Even if it's not noticeable, I would still reduce the number of lines by
one. BIC with immediate is just an alias for AND and it supports
different source and destination.

-- 
Catalin

  reply	other threads:[~2014-05-23 14:44 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-05-22 19:27 [PATCH v4 0/2] context tracker support for arm64 Larry Bassel
2014-05-22 19:27 ` Larry Bassel
2014-05-22 19:27 ` [PATCH v4 1/2] arm64: adjust el0_sync so that a function can be called Larry Bassel
2014-05-22 19:27   ` Larry Bassel
2014-05-22 20:23   ` Christopher Covington
2014-05-22 20:23     ` Christopher Covington
2014-05-22 22:35     ` Larry Bassel
2014-05-22 22:35       ` Larry Bassel
2014-05-23 14:44       ` Catalin Marinas [this message]
2014-05-23 14:44         ` Catalin Marinas
2014-05-23 17:04         ` Larry Bassel
2014-05-23 17:04           ` Larry Bassel
2014-05-22 19:27 ` [PATCH v4 2/2] arm64: enable context tracking Larry Bassel
2014-05-22 19:27   ` Larry Bassel
2014-05-22 20:29   ` Christopher Covington
2014-05-22 20:29     ` Christopher Covington
2014-05-23  0:11     ` Kevin Hilman
2014-05-23  0:11       ` Kevin Hilman
2014-05-23 14:51       ` Catalin Marinas
2014-05-23 14:51         ` Catalin Marinas
2014-05-23 15:55         ` Mark Rutland
2014-05-23 15:55           ` Mark Rutland
2014-05-23 16:12           ` Will Deacon
2014-05-23 16:12             ` Will Deacon
2014-05-24  0:03           ` Kevin Hilman
2014-05-24  0:03             ` Kevin Hilman
2014-05-22 20:19 ` [PATCH v4 0/2] context tracker support for arm64 Kevin Hilman
2014-05-22 20:19   ` Kevin Hilman

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-all 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=20140523144457.GN9252@arm.com \
    --to=catalin.marinas@arm.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.