All of lore.kernel.org
 help / color / mirror / Atom feed
From: dave.martin@linaro.org (Dave Martin)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
Date: Thu, 8 Mar 2012 12:17:55 +0000	[thread overview]
Message-ID: <20120308121755.GA2703@linaro.org> (raw)
In-Reply-To: <4F5882BF.1030104@arm.com>

On Thu, Mar 08, 2012 at 09:58:23AM +0000, Richard Earnshaw wrote:
> On 02/03/12 21:15, Nicolas Pitre wrote:
> > [ coming back from vacation and trying to catch up ]
> >
> > On Wed, 29 Feb 2012, Dave Martin wrote:
> >
> >> Just had a chat with some tools guys -- apparently, when passing register
> >> arguments to gcc inline asms there really isn't a guarantee that those
> >> variables will be in the expected registers on entry to the inline asm.
> >>
> >> If gcc reorders other function calls or other code around the inline asm
> >> (which it can do, except under certain controlled situations), then
> >> intervening code can clobber any registers in general.
> >
> > I'm hearing this argument about once every year or so for the last 8
> > years.  I think that the tools people are getting confused between
> > themselves as you may get a different interpretation of what gcc should
> > do depending to whom you happen to talk to.
> >
> > I did submit a bug to gcc in 2004 about this:
> >
> >       http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089
> >
> > You can see the confusion among gcc developers lurking there.
> >
> > So let's quote the relevant gcc documentation:
> >
> > -> * C Extensions::    GNU extensions to the C language family.
> >    -> * Explicit Reg Vars::   Defining variables residing in specified registers.
> >
> > |GNU C allows you to put a few global variables into specified hardware
> > |registers.  You can also specify the register in which an ordinary
> > |register variable should be allocated.
> > |
> > |   * Global register variables reserve registers throughout the program.
> > |     This may be useful in programs such as programming language
> > |     interpreters which have a couple of global variables that are
> > |     accessed very often.
> > |
> > |   * Local register variables in specific registers do not reserve the
> > |     registers, except at the point where they are used as input or
> > |     output operands in an `asm' statement and the `asm' statement
> > |     itself is not deleted.  The compiler's data flow analysis is
> > |     capable of determining where the specified registers contain live
> > |     values, and where they are available for other uses.  Stores into
> > |     local register variables may be deleted when they appear to be
> > |     dead according to dataflow analysis.  References to local register
> > |     variables may be deleted or moved or simplified.
> > |
> > |     These local variables are sometimes convenient for use with the
> > |     extended `asm' feature (*note Extended Asm::), if you want to
> > |     write one output of the assembler instruction directly into a
> > |     particular register.  (This will work provided the register you
> > |     specify fits the constraints specified for that operand in the
> > |     `asm'.)
> >
> >       -> * Local Reg Vars::
> >
> > [...]
> >
> > | Defining such a register variable does not reserve the register; it
> > |remains available for other uses in places where flow control
> > |determines the variable's value is not live.
> > |
> > | This option does not guarantee that GCC will generate code that has
> > |this variable in the register you specify at all times.  You may not
> > |code an explicit reference to this register in the _assembler
> > |instruction template_ part of an `asm' statement and assume it will
> > |always refer to this variable.  However, using the variable as an `asm'
> > |_operand_ guarantees that the specified register is used for the
> > |operand.

Hmmm, it's a while since I saw that documentation, and it had clearly
fallen out of my head when I made my previous statements...

> >
> > So, to me, the gcc documentation is perfectly clear on this topic.
> > there really _is_ a guarantee that those asm marked variables will be in
> > the expected registers on entry to the inline asm, given that the
> > variable is _also_ listed as an operand to the asm statement.  But only
> > in that case.
> >
> > It is true that gcc may reorder other function calls or other code
> > around the inline asm and then intervening code can clobber any
> > registers.  Then it is up to gcc to preserve the variable's content
> > elsewhere when its register is used for other purposes, and restore it
> > when some inline asm statement is referring to it.
> >
> > And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc was
> > buggy.  No other gcc versions in the last 7 years had such a problem or
> > the __asmeq macro in the kernel would have told us.
> >
> >> Or, to summarise another way, there is no way to control which register
> >> is used to pass something to an inline asm in general (often we get away
> >> with this, and there are a lot of inline asms in the kernel that assume
> >> it works, but the more you inline the more likely you are to get nasty
> >> surprises).
> >
> > This statement is therefore unfounded and wrong.  Please direct the
> > tools guy who mislead you to the above gcc documentation.
> >
> 
> The problem is not really about re-ordering functions but about implicit
> functions that come from the source code; for example
> 
> int foo (int a, int b)
> {
>   register int x __asm__("r0") = 33;
> 
>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0 with
> division function call.  */
> 
>   asm ("svc 0" : : "r" (x));
> }

|   * Local register variables in specific registers do not reserve the
|     registers, except at the point where they are used as input or
|     output operands in an `asm' statement and the `asm' statement
|     itself is not deleted.  The compiler's data flow analysis is

So, I guess the issue is how to interpret this statement in the context
of the above code:  i.e., what does it mean for a register to be reserved
for a local register variable?

"The above paragraph says that Local register variables [do] reserve the
registers _[at] the point_ where they are used as input or output
operands in an `asm' statement and the `asm' statement itself is not
deleted." (my emphasis)

Under that reading, r0 must be reserved for x on entry to the asm, but
not necessarily at points preceding that.  If the asm sees anything in
r0 except for x, that would be noncompliant with the above paragraph.

Nevertheless, a slightly modified version of the above which does not
allow gcc to optimise the asm away does trigger just the kind of
behaviour you describe:

int foo(int a, int b)
{
	register int x asm("r0") = 33;
	register int c asm("r1") = a / b;

	asm("svc 0" : "+r" (x) : "r" (c));

	return x;
}

 -->

00000000 <foo>:
   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <__aeabi_idiv>
   8:   e1a01000        mov     r1, r0
   c:   ef000000        svc     0x00000000
  10:   e8bd8008        pop     {r3, pc}


This is doubly weird: x is an I/O to the asm with a "+r" constraint,
so even if the asm("rX") assignments are not guaranteed, then x should
be _somewhere_ on entry to the asm (even if not in r0).  But it is
completely gone.

Is this allowed, or wrong?  I don't see how this can be rationalised
with the gcc documentation that Nico quoted.

Have I missed something?

Cheers
---Dave

WARNING: multiple messages have this Message-ID (diff)
From: Dave Martin <dave.martin@linaro.org>
To: Richard Earnshaw <Richard.Earnshaw@arm.com>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>,
	"xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>,
	"linaro-dev@lists.linaro.org" <linaro-dev@lists.linaro.org>,
	Ian Campbell <Ian.Campbell@citrix.com>,
	"arnd@arndb.de" <arnd@arndb.de>,
	Catalin Marinas <Catalin.Marinas@arm.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	David Vrabel <david.vrabel@citrix.com>,
	"kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor
Date: Thu, 8 Mar 2012 12:17:55 +0000	[thread overview]
Message-ID: <20120308121755.GA2703@linaro.org> (raw)
In-Reply-To: <4F5882BF.1030104@arm.com>

On Thu, Mar 08, 2012 at 09:58:23AM +0000, Richard Earnshaw wrote:
> On 02/03/12 21:15, Nicolas Pitre wrote:
> > [ coming back from vacation and trying to catch up ]
> >
> > On Wed, 29 Feb 2012, Dave Martin wrote:
> >
> >> Just had a chat with some tools guys -- apparently, when passing register
> >> arguments to gcc inline asms there really isn't a guarantee that those
> >> variables will be in the expected registers on entry to the inline asm.
> >>
> >> If gcc reorders other function calls or other code around the inline asm
> >> (which it can do, except under certain controlled situations), then
> >> intervening code can clobber any registers in general.
> >
> > I'm hearing this argument about once every year or so for the last 8
> > years.  I think that the tools people are getting confused between
> > themselves as you may get a different interpretation of what gcc should
> > do depending to whom you happen to talk to.
> >
> > I did submit a bug to gcc in 2004 about this:
> >
> >       http://gcc.gnu.org/bugzilla/show_bug.cgi?id=15089
> >
> > You can see the confusion among gcc developers lurking there.
> >
> > So let's quote the relevant gcc documentation:
> >
> > -> * C Extensions::    GNU extensions to the C language family.
> >    -> * Explicit Reg Vars::   Defining variables residing in specified registers.
> >
> > |GNU C allows you to put a few global variables into specified hardware
> > |registers.  You can also specify the register in which an ordinary
> > |register variable should be allocated.
> > |
> > |   * Global register variables reserve registers throughout the program.
> > |     This may be useful in programs such as programming language
> > |     interpreters which have a couple of global variables that are
> > |     accessed very often.
> > |
> > |   * Local register variables in specific registers do not reserve the
> > |     registers, except at the point where they are used as input or
> > |     output operands in an `asm' statement and the `asm' statement
> > |     itself is not deleted.  The compiler's data flow analysis is
> > |     capable of determining where the specified registers contain live
> > |     values, and where they are available for other uses.  Stores into
> > |     local register variables may be deleted when they appear to be
> > |     dead according to dataflow analysis.  References to local register
> > |     variables may be deleted or moved or simplified.
> > |
> > |     These local variables are sometimes convenient for use with the
> > |     extended `asm' feature (*note Extended Asm::), if you want to
> > |     write one output of the assembler instruction directly into a
> > |     particular register.  (This will work provided the register you
> > |     specify fits the constraints specified for that operand in the
> > |     `asm'.)
> >
> >       -> * Local Reg Vars::
> >
> > [...]
> >
> > | Defining such a register variable does not reserve the register; it
> > |remains available for other uses in places where flow control
> > |determines the variable's value is not live.
> > |
> > | This option does not guarantee that GCC will generate code that has
> > |this variable in the register you specify at all times.  You may not
> > |code an explicit reference to this register in the _assembler
> > |instruction template_ part of an `asm' statement and assume it will
> > |always refer to this variable.  However, using the variable as an `asm'
> > |_operand_ guarantees that the specified register is used for the
> > |operand.

Hmmm, it's a while since I saw that documentation, and it had clearly
fallen out of my head when I made my previous statements...

> >
> > So, to me, the gcc documentation is perfectly clear on this topic.
> > there really _is_ a guarantee that those asm marked variables will be in
> > the expected registers on entry to the inline asm, given that the
> > variable is _also_ listed as an operand to the asm statement.  But only
> > in that case.
> >
> > It is true that gcc may reorder other function calls or other code
> > around the inline asm and then intervening code can clobber any
> > registers.  Then it is up to gcc to preserve the variable's content
> > elsewhere when its register is used for other purposes, and restore it
> > when some inline asm statement is referring to it.
> >
> > And if gcc does not do this then it is buggy.  Version 3.4.0 of gcc was
> > buggy.  No other gcc versions in the last 7 years had such a problem or
> > the __asmeq macro in the kernel would have told us.
> >
> >> Or, to summarise another way, there is no way to control which register
> >> is used to pass something to an inline asm in general (often we get away
> >> with this, and there are a lot of inline asms in the kernel that assume
> >> it works, but the more you inline the more likely you are to get nasty
> >> surprises).
> >
> > This statement is therefore unfounded and wrong.  Please direct the
> > tools guy who mislead you to the above gcc documentation.
> >
> 
> The problem is not really about re-ordering functions but about implicit
> functions that come from the source code; for example
> 
> int foo (int a, int b)
> {
>   register int x __asm__("r0") = 33;
> 
>   register int c __asm__("r1") = a / b; /* Ooops, clobbers r0 with
> division function call.  */
> 
>   asm ("svc 0" : : "r" (x));
> }

|   * Local register variables in specific registers do not reserve the
|     registers, except at the point where they are used as input or
|     output operands in an `asm' statement and the `asm' statement
|     itself is not deleted.  The compiler's data flow analysis is

So, I guess the issue is how to interpret this statement in the context
of the above code:  i.e., what does it mean for a register to be reserved
for a local register variable?

"The above paragraph says that Local register variables [do] reserve the
registers _[at] the point_ where they are used as input or output
operands in an `asm' statement and the `asm' statement itself is not
deleted." (my emphasis)

Under that reading, r0 must be reserved for x on entry to the asm, but
not necessarily at points preceding that.  If the asm sees anything in
r0 except for x, that would be noncompliant with the above paragraph.

Nevertheless, a slightly modified version of the above which does not
allow gcc to optimise the asm away does trigger just the kind of
behaviour you describe:

int foo(int a, int b)
{
	register int x asm("r0") = 33;
	register int c asm("r1") = a / b;

	asm("svc 0" : "+r" (x) : "r" (c));

	return x;
}

 -->

00000000 <foo>:
   0:   e92d4008        push    {r3, lr}
   4:   ebfffffe        bl      0 <__aeabi_idiv>
   8:   e1a01000        mov     r1, r0
   c:   ef000000        svc     0x00000000
  10:   e8bd8008        pop     {r3, pc}


This is doubly weird: x is an I/O to the asm with a "+r" constraint,
so even if the asm("rX") assignments are not guaranteed, then x should
be _somewhere_ on entry to the asm (even if not in r0).  But it is
completely gone.

Is this allowed, or wrong?  I don't see how this can be rationalised
with the gcc documentation that Nico quoted.

Have I missed something?

Cheers
---Dave

  reply	other threads:[~2012-03-08 12:17 UTC|newest]

Thread overview: 121+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-02-23 17:47 [PATCH-WIP 00/13] xen/arm: receive Xen events and initialize xenbus Stefano Stabellini
2012-02-23 17:47 ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 01/13] xen/arm: use r12 to pass the hypercall number to the hypervisor Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-27 16:27   ` Ian Campbell
2012-02-27 16:27     ` Ian Campbell
2012-02-27 18:03     ` Dave Martin
2012-02-27 18:03       ` Dave Martin
2012-02-27 18:03       ` Dave Martin
2012-02-27 19:33       ` Ian Campbell
2012-02-27 19:33         ` Ian Campbell
2012-02-28 10:20         ` Dave Martin
2012-02-28 10:20           ` Dave Martin
2012-02-28 10:20           ` Dave Martin
2012-02-28 10:48           ` Ian Campbell
2012-02-28 10:48             ` Ian Campbell
2012-02-28 12:28             ` Stefano Stabellini
2012-02-28 12:28               ` Stefano Stabellini
2012-02-28 12:28               ` Stefano Stabellini
2012-02-29  9:34               ` Dave Martin
2012-02-29  9:34                 ` Dave Martin
2012-02-29  9:34                 ` Dave Martin
2012-02-29  9:56                 ` Ian Campbell
2012-02-29  9:56                   ` Ian Campbell
2012-02-29  9:56                   ` Ian Campbell
2012-02-29 11:47                   ` Dave Martin
2012-02-29 11:47                     ` Dave Martin
2012-02-29 12:58                   ` Dave Martin
2012-02-29 12:58                     ` Dave Martin
2012-02-29 12:58                     ` Dave Martin
2012-02-29 14:44                     ` Ian Campbell
2012-02-29 14:44                       ` Ian Campbell
2012-03-01  9:35                       ` Dave Martin
2012-03-01  9:35                         ` Dave Martin
2012-03-01  9:35                         ` Dave Martin
2012-03-01 10:12                       ` Russell King - ARM Linux
2012-03-01 10:12                         ` Russell King - ARM Linux
2012-03-02 21:19                       ` Nicolas Pitre
2012-03-02 21:19                         ` Nicolas Pitre
2012-02-29 14:52                     ` Stefano Stabellini
2012-02-29 14:52                       ` Stefano Stabellini
2012-03-01  9:51                       ` Dave Martin
2012-03-01  9:51                         ` Dave Martin
2012-03-01  9:51                         ` Dave Martin
2012-03-01 10:10                     ` Russell King - ARM Linux
2012-03-01 10:10                       ` Russell King - ARM Linux
2012-03-01 10:27                       ` Dave Martin
2012-03-01 10:27                         ` Dave Martin
2012-03-01 10:27                         ` Dave Martin
2012-03-01 10:35                         ` Russell King - ARM Linux
2012-03-01 10:35                           ` Russell King - ARM Linux
2012-03-01 12:12                           ` Stefano Stabellini
2012-03-01 12:12                             ` Stefano Stabellini
2012-03-01 12:12                             ` Stefano Stabellini
2012-03-02 21:15                     ` Nicolas Pitre
2012-03-02 21:15                       ` Nicolas Pitre
2012-03-02 21:15                       ` Nicolas Pitre
2012-03-08  9:58                       ` Richard Earnshaw
2012-03-08  9:58                         ` Richard Earnshaw
2012-03-08 12:17                         ` Dave Martin [this message]
2012-03-08 12:17                           ` Dave Martin
2012-03-08 17:21                         ` Nicolas Pitre
2012-03-08 17:21                           ` Nicolas Pitre
2012-03-08 18:47                           ` Richard Earnshaw
2012-03-08 18:47                             ` Richard Earnshaw
2012-03-08 18:47                             ` Richard Earnshaw
2012-03-09 15:58                             ` Dave Martin
2012-03-09 15:58                               ` Dave Martin
2012-03-09 15:58                               ` Dave Martin
2012-03-09 16:20                               ` Nicolas Pitre
2012-03-09 16:20                                 ` Nicolas Pitre
2012-03-09 17:38                                 ` Richard Earnshaw
2012-03-09 17:38                                   ` Richard Earnshaw
2012-03-09 17:38                                   ` Richard Earnshaw
2012-02-27 21:05     ` Peter Maydell
2012-02-27 21:05       ` Peter Maydell
2012-02-27 21:05       ` Peter Maydell
2012-02-28 10:12       ` Ian Campbell
2012-02-28 10:12         ` Ian Campbell
2012-02-27 17:53   ` Dave Martin
2012-02-27 17:53     ` Dave Martin
2012-02-27 17:53     ` Dave Martin
2012-02-27 19:48     ` Ian Campbell
2012-02-27 19:48       ` Ian Campbell
2012-02-28  9:46       ` Dave Martin
2012-02-28  9:46         ` Dave Martin
2012-02-28  9:46         ` Dave Martin
2012-02-28 10:07         ` Ian Campbell
2012-02-28 10:07           ` Ian Campbell
2012-02-28 12:21         ` Stefano Stabellini
2012-02-28 12:21           ` Stefano Stabellini
2012-02-28 12:21           ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 02/13] xen/arm: introduce privcmp, physdev_op and memory_op hypercalls Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 03/13] xen/arm: mmu.h and page.h related definitions Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 04/13] xen/arm: sync_bitops Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 05/13] xen/arm: empty implementation of grant_table arch specific functions Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 06/13] xen/arm: missing includes Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 07/13] xen/arm: receive xen events on arm Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-24 11:12   ` David Vrabel
2012-02-24 11:12     ` David Vrabel
2012-02-24 12:23     ` Stefano Stabellini
2012-02-24 12:23       ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 08/13] xen/arm: fix arm xen guest handle definitions Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 09/13] xen/arm: shared_info and start_info Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 10/13] xen/arm: empty implementation of xen_remap_domain_mfn_range Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 11/13] xen/arm: Introduce xen_pfn_t for pfn and mfn types Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 12/13] xen/arm: compile and run xenbus Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini
2012-02-23 17:48 ` [PATCH-WIP 13/13] xen/arm: compile grant-table features events and xenbus, do not compile pci Stefano Stabellini
2012-02-23 17:48   ` Stefano Stabellini

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=20120308121755.GA2703@linaro.org \
    --to=dave.martin@linaro.org \
    --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.