From: will.deacon@arm.com (Will Deacon)
To: linux-arm-kernel@lists.infradead.org
Subject: arm64: add missing early clobber in atomic64_dec_if_positive()
Date: Mon, 21 May 2018 18:21:08 +0100 [thread overview]
Message-ID: <20180521172108.GH21034@arm.com> (raw)
In-Reply-To: <c540dd519065316c859e5cb1290de7f5041cb816.camel@redhat.com>
On Mon, May 21, 2018 at 01:18:39PM -0400, Mark Salter wrote:
> On Mon, 2018-05-21 at 18:00 +0100, Will Deacon wrote:
> > Hi Mark,
> >
> > Thanks for reporting this.
> >
> > On Sat, May 19, 2018 at 08:17:26PM -0400, Mark Salter wrote:
> > > When running a kernel compiled with gcc8 on a machine using LSE, I
> > > get:
> > >
> > > Unable to handle kernel paging request at virtual address 11111122222221
> >
> > [...]
> >
> > > The fault happens at the casal insn of inlined atomic64_dec_if_positive().
> > > The inline asm code in that function has:
> > >
> > > "1: ldr x30, %[v]\n"
> > > " subs %[ret], x30, #1\n"
> > > " b.lt 2f\n"
> > > " casal x30, %[ret], %[v]\n"
> > > " sub x30, x30, #1\n"
> > > " sub x30, x30, %[ret]\n"
> > > " cbnz x30, 1b\n"
> > > "2:")
> > > : [ret] "+r" (x0), [v] "+Q" (v->counter)
> > >
> > > gcc8 used register x0 for both [ret] and [v] and the subs was
> > > clobbering [v] before it was used for casal. Gcc is free to do
> > > this because [ret] lacks an early clobber modifier. So add one
> > > to tell gcc a separate register is needed for [v].
> >
> > Oh blimey, it looks like GCC is realising that counter is at offset 0
> > of atomic_t and therefore assigns the same register for [ret] and [v],
> > which is actually forced to be x0 by the 'register' local variable in
> > C code. The "+Q" constraint only says that the memory is read/write, so
> > the pointer is fair game.
> >
> > I agree with your fix, but we also need to fix up the other places relying
> > on this. Patch below -- please yell if you think I missed any.
>
> I looked at the other places but figured they were okay because we're
> explicitly using separate registers. But I suppose the early clobber
> is the right thing to do in any case.
I was worried about silly things like a caller doing:
atomic64_and((long)v, v);
and then GCC figuring out that the two values were equal and allocating
the same register..
Will
WARNING: multiple messages have this Message-ID (diff)
From: Will Deacon <will.deacon@arm.com>
To: Mark Salter <msalter@redhat.com>
Cc: linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, dave.martin@arm.com,
robin.murphy@arm.com, peterz@infradead.org
Subject: Re: arm64: add missing early clobber in atomic64_dec_if_positive()
Date: Mon, 21 May 2018 18:21:08 +0100 [thread overview]
Message-ID: <20180521172108.GH21034@arm.com> (raw)
In-Reply-To: <c540dd519065316c859e5cb1290de7f5041cb816.camel@redhat.com>
On Mon, May 21, 2018 at 01:18:39PM -0400, Mark Salter wrote:
> On Mon, 2018-05-21 at 18:00 +0100, Will Deacon wrote:
> > Hi Mark,
> >
> > Thanks for reporting this.
> >
> > On Sat, May 19, 2018 at 08:17:26PM -0400, Mark Salter wrote:
> > > When running a kernel compiled with gcc8 on a machine using LSE, I
> > > get:
> > >
> > > Unable to handle kernel paging request at virtual address 11111122222221
> >
> > [...]
> >
> > > The fault happens at the casal insn of inlined atomic64_dec_if_positive().
> > > The inline asm code in that function has:
> > >
> > > "1: ldr x30, %[v]\n"
> > > " subs %[ret], x30, #1\n"
> > > " b.lt 2f\n"
> > > " casal x30, %[ret], %[v]\n"
> > > " sub x30, x30, #1\n"
> > > " sub x30, x30, %[ret]\n"
> > > " cbnz x30, 1b\n"
> > > "2:")
> > > : [ret] "+r" (x0), [v] "+Q" (v->counter)
> > >
> > > gcc8 used register x0 for both [ret] and [v] and the subs was
> > > clobbering [v] before it was used for casal. Gcc is free to do
> > > this because [ret] lacks an early clobber modifier. So add one
> > > to tell gcc a separate register is needed for [v].
> >
> > Oh blimey, it looks like GCC is realising that counter is at offset 0
> > of atomic_t and therefore assigns the same register for [ret] and [v],
> > which is actually forced to be x0 by the 'register' local variable in
> > C code. The "+Q" constraint only says that the memory is read/write, so
> > the pointer is fair game.
> >
> > I agree with your fix, but we also need to fix up the other places relying
> > on this. Patch below -- please yell if you think I missed any.
>
> I looked at the other places but figured they were okay because we're
> explicitly using separate registers. But I suppose the early clobber
> is the right thing to do in any case.
I was worried about silly things like a caller doing:
atomic64_and((long)v, v);
and then GCC figuring out that the two values were equal and allocating
the same register..
Will
next prev parent reply other threads:[~2018-05-21 17:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-05-20 0:17 [RHEL-8] arm64: add missing early clobber in atomic64_dec_if_positive() Mark Salter
2018-05-20 0:17 ` Mark Salter
2018-05-21 17:00 ` Will Deacon
2018-05-21 17:00 ` Will Deacon
2018-05-21 17:18 ` Mark Salter
2018-05-21 17:18 ` Mark Salter
2018-05-21 17:21 ` Will Deacon [this message]
2018-05-21 17:21 ` Will Deacon
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=20180521172108.GH21034@arm.com \
--to=will.deacon@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.