[Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paolo Bonzini]

Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
register that is actually used is AREG0, so the complexity of
hostregs_helper.h is unwarranted.

Let's just say that env should be the only global register.  AREG1 and
AREG2 in principle could still be used to work around bad register
allocation in GCC, so I'm leaving them in dyngen-exec.h.

Blue Swirl, can you check whether changing AREG0 to another register
in dyngen-exec.h would fix the "annoying glibc bugs mangling global
register variables"?  Or maybe we can remove the workaround altogether,
considering the bug was fixed in version 2.3 of glibc dated 2001-11-29
(at least that's what I'd guess from the history)?

Paolo Bonzini (2):
  remove dead m68k global register definitions
  get rid of hostregs_helper.h

 cpu-exec.c         |   15 +++++++-----
 hostregs_helper.h  |   61 ----------------------------------------------------
 qemu-common.h      |    2 +
 target-m68k/exec.h |    4 ---
 4 files changed, 11 insertions(+), 71 deletions(-)
 delete mode 100644 hostregs_helper.h

[Qemu-devel] [PATCH 1/2] remove dead m68k global register definitions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-m68k/exec.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/target-m68k/exec.h b/target-m68k/exec.h
index 1267bb6..ece9aa0 100644
--- a/target-m68k/exec.h
+++ b/target-m68k/exec.h
@@ -20,10 +20,6 @@
 #include "dyngen-exec.h"
 
 register struct CPUM68KState *env asm(AREG0);
-/* This is only used for tb lookup.  */
-register uint32_t T0 asm(AREG1);
-/* ??? We don't use T1, but common code expects it to exist  */
-#define T1 env->t1
 
 #include "cpu.h"
 #include "exec-all.h"
-- 
1.6.6

[Qemu-devel] [PATCH 2/2] get rid of hostregs_helper.h

[Paolo Bonzini]

Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
register that is used is AREG0, so the complexity of hostregs_helper.h
is unused.  Use regular assignments and a compiler optimization barrier.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpu-exec.c        |   15 +++++++-----
 hostregs_helper.h |   61 -----------------------------------------------------
 qemu-common.h     |    2 +
 3 files changed, 11 insertions(+), 67 deletions(-)
 delete mode 100644 hostregs_helper.h

diff --git a/cpu-exec.c b/cpu-exec.c
index 0256edf..2553d1d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -210,8 +210,7 @@ static void cpu_handle_debug_exception(CPUState *env)
 
 int cpu_exec(CPUState *env1)
 {
-#define DECLARE_HOST_REGS 1
-#include "hostregs_helper.h"
+    host_reg_t saved_env_reg;
     int ret, interrupt_request;
     TranslationBlock *tb;
     uint8_t *tc_ptr;
@@ -222,9 +221,12 @@ int cpu_exec(CPUState *env1)
 
     cpu_single_env = env1;
 
-    /* first we save global registers */
-#define SAVE_HOST_REGS 1
-#include "hostregs_helper.h"
+    /* the access to env below is actually saving the global register's
+       value, so that files not including target-xyz/exec.h are free to
+       use it.  */
+    BUILD_BUG_ON (sizeof (saved_env_reg) != sizeof (env));
+    saved_env_reg = (host_reg_t) env;
+    asm("");
     env = env1;
 
 #if defined(TARGET_I386)
@@ -668,7 +670,8 @@ int cpu_exec(CPUState *env1)
 #endif
 
     /* restore global registers */
-#include "hostregs_helper.h"
+    asm("");
+    env = (void *) saved_env_reg;
 
     /* fail safe : never use cpu_single_env outside cpu_exec() */
     cpu_single_env = NULL;
diff --git a/hostregs_helper.h b/hostregs_helper.h
deleted file mode 100644
index 3a0bece..0000000
--- a/hostregs_helper.h
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- *  Save/restore host registers.
- *
- *  Copyright (c) 2007 CodeSourcery
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-
-/* The GCC global register variable extension is used to reserve some
-   host registers for use by generated code.  However only the core parts of
-   the translation engine are compiled with these settings.  We must manually
-   save/restore these registers when called from regular code.
-   It is not sufficient to save/restore T0 et. al. as these may be declared
-   with a datatype smaller than the actual register.  */
-
-#if defined(DECLARE_HOST_REGS)
-
-#define DO_REG(REG)					\
-    register host_reg_t reg_AREG##REG asm(AREG##REG);	\
-    volatile host_reg_t saved_AREG##REG;
-
-#elif defined(SAVE_HOST_REGS)
-
-#define DO_REG(REG)					\
-    __asm__ __volatile__ ("" : "=r" (reg_AREG##REG));	\
-    saved_AREG##REG = reg_AREG##REG;
-
-#else
-
-#define DO_REG(REG)                                     \
-    reg_AREG##REG = saved_AREG##REG;		        \
-    __asm__ __volatile__ ("" : : "r" (reg_AREG##REG));
-
-#endif
-
-#ifdef AREG0
-DO_REG(0)
-#endif
-
-#ifdef AREG1
-DO_REG(1)
-#endif
-
-#ifdef AREG2
-DO_REG(2)
-#endif
-
-#undef SAVE_HOST_REGS
-#undef DECLARE_HOST_REGS
-#undef DO_REG
diff --git a/qemu-common.h b/qemu-common.h
index b09f717..b791148 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -11,6 +11,8 @@
 #define QEMU_WARN_UNUSED_RESULT
 #endif
 
+#define BUILD_BUG_ON(x) typedef char __build_bug_on__##__LINE__[(x)?-1:1];
+
 /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
    cannot include the following headers without conflicts. This condition has
    to be removed once dyngen is gone. */
-- 
1.6.6

Re: [Qemu-devel] [PATCH 2/2] get rid of hostregs_helper.h

[Blue Swirl]

On Thu, Feb 11, 2010 at 1:26 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
> register that is used is AREG0, so the complexity of hostregs_helper.h
> is unused.  Use regular assignments and a compiler optimization barrier.

In file included from /src/qemu/linux-user/syscall.c:58:
/src/qemu/qemu-common.h:14:1: error: "BUILD_BUG_ON" redefined
In file included from /usr/include/sys/sysinfo.h:25,
                 from /src/qemu/linux-user/syscall.c:53:
/usr/include/linux/kernel.h:28:1: error: this is the location of the
previous definition

Please add something like QEMU_ prefix.

[Qemu-devel] [PATCH v2 1/2] remove dead m68k definitions

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-m68k/exec.h |    4 ----
 1 files changed, 0 insertions(+), 4 deletions(-)

diff --git a/target-m68k/exec.h b/target-m68k/exec.h
index 1267bb6..ece9aa0 100644
--- a/target-m68k/exec.h
+++ b/target-m68k/exec.h
@@ -20,10 +20,6 @@
 #include "dyngen-exec.h"
 
 register struct CPUM68KState *env asm(AREG0);
-/* This is only used for tb lookup.  */
-register uint32_t T0 asm(AREG1);
-/* ??? We don't use T1, but common code expects it to exist  */
-#define T1 env->t1
 
 #include "cpu.h"
 #include "exec-all.h"
-- 
1.6.6

[Qemu-devel] [PATCH v2 2/2] get rid of hostregs_helper.h

[Paolo Bonzini]

Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
register that is used is AREG0, so the complexity of hostregs_helper.h
is unused.  Use regular assignments and a compiler optimization barrier.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        v1->v2: added QEMU prefix to BUILD_BUG_ON

 cpu-exec.c        |   15 +++++++-----
 hostregs_helper.h |   61 -----------------------------------------------------
 qemu-common.h     |    2 +
 3 files changed, 11 insertions(+), 67 deletions(-)
 delete mode 100644 hostregs_helper.h

diff --git a/cpu-exec.c b/cpu-exec.c
index 6a290fd..ab6defc 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -210,8 +210,7 @@ static void cpu_handle_debug_exception(CPUState *env)
 
 int cpu_exec(CPUState *env1)
 {
-#define DECLARE_HOST_REGS 1
-#include "hostregs_helper.h"
+    host_reg_t saved_env_reg;
     int ret, interrupt_request;
     TranslationBlock *tb;
     uint8_t *tc_ptr;
@@ -222,9 +221,12 @@ int cpu_exec(CPUState *env1)
 
     cpu_single_env = env1;
 
-    /* first we save global registers */
-#define SAVE_HOST_REGS 1
-#include "hostregs_helper.h"
+    /* the access to env below is actually saving the global register's
+       value, so that files not including target-xyz/exec.h are free to
+       use it.  */
+    QEMU_BUILD_BUG_ON (sizeof (saved_env_reg) != sizeof (env));
+    saved_env_reg = (host_reg_t) env;
+    asm("");
     env = env1;
 
 #if defined(TARGET_I386)
@@ -669,7 +671,8 @@ int cpu_exec(CPUState *env1)
 #endif
 
     /* restore global registers */
-#include "hostregs_helper.h"
+    asm("");
+    env = (void *) saved_env_reg;
 
     /* fail safe : never use cpu_single_env outside cpu_exec() */
     cpu_single_env = NULL;
diff --git a/hostregs_helper.h b/hostregs_helper.h
deleted file mode 100644
index 3a0bece..0000000
--- a/hostregs_helper.h
+++ /dev/null
@@ -1,61 +0,0 @@
-/*
- *  Save/restore host registers.
- *
- *  Copyright (c) 2007 CodeSourcery
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-
-/* The GCC global register variable extension is used to reserve some
-   host registers for use by generated code.  However only the core parts of
-   the translation engine are compiled with these settings.  We must manually
-   save/restore these registers when called from regular code.
-   It is not sufficient to save/restore T0 et. al. as these may be declared
-   with a datatype smaller than the actual register.  */
-
-#if defined(DECLARE_HOST_REGS)
-
-#define DO_REG(REG)					\
-    register host_reg_t reg_AREG##REG asm(AREG##REG);	\
-    volatile host_reg_t saved_AREG##REG;
-
-#elif defined(SAVE_HOST_REGS)
-
-#define DO_REG(REG)					\
-    __asm__ __volatile__ ("" : "=r" (reg_AREG##REG));	\
-    saved_AREG##REG = reg_AREG##REG;
-
-#else
-
-#define DO_REG(REG)                                     \
-    reg_AREG##REG = saved_AREG##REG;		        \
-    __asm__ __volatile__ ("" : : "r" (reg_AREG##REG));
-
-#endif
-
-#ifdef AREG0
-DO_REG(0)
-#endif
-
-#ifdef AREG1
-DO_REG(1)
-#endif
-
-#ifdef AREG2
-DO_REG(2)
-#endif
-
-#undef SAVE_HOST_REGS
-#undef DECLARE_HOST_REGS
-#undef DO_REG
diff --git a/qemu-common.h b/qemu-common.h
index b09f717..a98fccd 100644
--- a/qemu-common.h
+++ b/qemu-common.h
@@ -11,6 +11,8 @@
 #define QEMU_WARN_UNUSED_RESULT
 #endif
 
+#define QEMU_BUILD_BUG_ON(x) typedef char __build_bug_on__##__LINE__[(x)?-1:1];
+
 /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
    cannot include the following headers without conflicts. This condition has
    to be removed once dyngen is gone. */
-- 
1.6.6

[Qemu-devel] Re: [PATCH v2 2/2] get rid of hostregs_helper.h

[Blue Swirl]

Thanks, applied both.

On Thu, Feb 18, 2010 at 10:25 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
> register that is used is AREG0, so the complexity of hostregs_helper.h
> is unused.  Use regular assignments and a compiler optimization barrier.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>        v1->v2: added QEMU prefix to BUILD_BUG_ON
>
>  cpu-exec.c        |   15 +++++++-----
>  hostregs_helper.h |   61 -----------------------------------------------------
>  qemu-common.h     |    2 +
>  3 files changed, 11 insertions(+), 67 deletions(-)
>  delete mode 100644 hostregs_helper.h
>
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 6a290fd..ab6defc 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -210,8 +210,7 @@ static void cpu_handle_debug_exception(CPUState *env)
>
>  int cpu_exec(CPUState *env1)
>  {
> -#define DECLARE_HOST_REGS 1
> -#include "hostregs_helper.h"
> +    host_reg_t saved_env_reg;
>     int ret, interrupt_request;
>     TranslationBlock *tb;
>     uint8_t *tc_ptr;
> @@ -222,9 +221,12 @@ int cpu_exec(CPUState *env1)
>
>     cpu_single_env = env1;
>
> -    /* first we save global registers */
> -#define SAVE_HOST_REGS 1
> -#include "hostregs_helper.h"
> +    /* the access to env below is actually saving the global register's
> +       value, so that files not including target-xyz/exec.h are free to
> +       use it.  */
> +    QEMU_BUILD_BUG_ON (sizeof (saved_env_reg) != sizeof (env));
> +    saved_env_reg = (host_reg_t) env;
> +    asm("");
>     env = env1;
>
>  #if defined(TARGET_I386)
> @@ -669,7 +671,8 @@ int cpu_exec(CPUState *env1)
>  #endif
>
>     /* restore global registers */
> -#include "hostregs_helper.h"
> +    asm("");
> +    env = (void *) saved_env_reg;
>
>     /* fail safe : never use cpu_single_env outside cpu_exec() */
>     cpu_single_env = NULL;
> diff --git a/hostregs_helper.h b/hostregs_helper.h
> deleted file mode 100644
> index 3a0bece..0000000
> --- a/hostregs_helper.h
> +++ /dev/null
> @@ -1,61 +0,0 @@
> -/*
> - *  Save/restore host registers.
> - *
> - *  Copyright (c) 2007 CodeSourcery
> - *
> - * This library is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Lesser General Public
> - * License as published by the Free Software Foundation; either
> - * version 2 of the License, or (at your option) any later version.
> - *
> - * This library is distributed in the hope that it will be useful,
> - * but WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> - * Lesser General Public License for more details.
> - *
> - * You should have received a copy of the GNU Lesser General Public
> - * License along with this library; if not, see <http://www.gnu.org/licenses/>.
> - */
> -
> -/* The GCC global register variable extension is used to reserve some
> -   host registers for use by generated code.  However only the core parts of
> -   the translation engine are compiled with these settings.  We must manually
> -   save/restore these registers when called from regular code.
> -   It is not sufficient to save/restore T0 et. al. as these may be declared
> -   with a datatype smaller than the actual register.  */
> -
> -#if defined(DECLARE_HOST_REGS)
> -
> -#define DO_REG(REG)                                    \
> -    register host_reg_t reg_AREG##REG asm(AREG##REG);  \
> -    volatile host_reg_t saved_AREG##REG;
> -
> -#elif defined(SAVE_HOST_REGS)
> -
> -#define DO_REG(REG)                                    \
> -    __asm__ __volatile__ ("" : "=r" (reg_AREG##REG));  \
> -    saved_AREG##REG = reg_AREG##REG;
> -
> -#else
> -
> -#define DO_REG(REG)                                     \
> -    reg_AREG##REG = saved_AREG##REG;                   \
> -    __asm__ __volatile__ ("" : : "r" (reg_AREG##REG));
> -
> -#endif
> -
> -#ifdef AREG0
> -DO_REG(0)
> -#endif
> -
> -#ifdef AREG1
> -DO_REG(1)
> -#endif
> -
> -#ifdef AREG2
> -DO_REG(2)
> -#endif
> -
> -#undef SAVE_HOST_REGS
> -#undef DECLARE_HOST_REGS
> -#undef DO_REG
> diff --git a/qemu-common.h b/qemu-common.h
> index b09f717..a98fccd 100644
> --- a/qemu-common.h
> +++ b/qemu-common.h
> @@ -11,6 +11,8 @@
>  #define QEMU_WARN_UNUSED_RESULT
>  #endif
>
> +#define QEMU_BUILD_BUG_ON(x) typedef char __build_bug_on__##__LINE__[(x)?-1:1];
> +
>  /* Hack around the mess dyngen-exec.h causes: We need QEMU_NORETURN in files that
>    cannot include the following headers without conflicts. This condition has
>    to be removed once dyngen is gone. */
> --
> 1.6.6
>
>

[Qemu-devel] Re: [PATCH v2 2/2] get rid of hostregs_helper.h

[Michael S. Tsirkin]

On Thu, Feb 18, 2010 at 11:28:14PM +0200, Blue Swirl wrote:
> >     /* restore global registers */
> > -#include "hostregs_helper.h"
> > +    asm("");
> > +    env = (void *) saved_env_reg;
> >

Is this sufficient?
I see __asm__ __volatile__("": : :"memory") in virtio.
Is memory clobber implied? What about volatile?

-- 
MST

[Qemu-devel] Re: [PATCH v2 2/2] get rid of hostregs_helper.h

[Paolo Bonzini]

On 02/25/2010 12:40 PM, Michael S. Tsirkin wrote:
> On Thu, Feb 18, 2010 at 11:28:14PM +0200, Blue Swirl wrote:
>>>      /* restore global registers */
>>> -#include "hostregs_helper.h"
>>> +    asm("");
>>> +    env = (void *) saved_env_reg;
>>>
>
> Is this sufficient?
> I see __asm__ __volatile__("": : :"memory") in virtio.
> Is memory clobber implied? What about volatile?

All asms without colons ("old-style") are volatile.  Clobbering memory 
is not necessary since we are only caring about blocking assignments of 
"env", which is by definition in a register (hostregs_helper.h wasn't 
clobbering memory either).

Paolo

[Qemu-devel] Re: [PATCH v2 2/2] get rid of hostregs_helper.h

[Michael S. Tsirkin]

On Thu, Feb 25, 2010 at 01:50:56PM +0100, Paolo Bonzini wrote:
> On 02/25/2010 12:40 PM, Michael S. Tsirkin wrote:
>> On Thu, Feb 18, 2010 at 11:28:14PM +0200, Blue Swirl wrote:
>>>>      /* restore global registers */
>>>> -#include "hostregs_helper.h"
>>>> +    asm("");
>>>> +    env = (void *) saved_env_reg;
>>>>
>>
>> Is this sufficient?
>> I see __asm__ __volatile__("": : :"memory") in virtio.
>> Is memory clobber implied? What about volatile?
>
> All asms without colons ("old-style") are volatile.  Clobbering memory  
> is not necessary since we are only caring about blocking assignments of  
> "env", which is by definition in a register

Then I think you should add that as a clobber. Otherwise what prevents the
compiler from reordering this asm wrt assignments?

> (hostregs_helper.h wasn't  
> clobbering memory either).


Maybe it was buggy :)

> Paolo

[Qemu-devel] Re: [PATCH v2 2/2] get rid of hostregs_helper.h

[Paolo Bonzini]

On 02/25/2010 02:04 PM, Michael S. Tsirkin wrote:
>> Clobbering memory
>> >  is not necessary since we are only caring about blocking assignments of
>> >  "env", which is by definition in a register
>
> Then I think you should add that as a clobber. Otherwise what prevents the
> compiler from reordering this asm wrt assignments?

That old-style asms block scheduling of _anything_ across it (like 
"barrier()" in the Linux kernel).  So, it is actually slightly stricter 
than what was there before.

I should have made clear in the commit message that this is the only 
change wrt hostregs_helper.h.

Paolo

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Blue Swirl]

On Thu, Feb 11, 2010 at 1:26 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
> register that is actually used is AREG0, so the complexity of
> hostregs_helper.h is unwarranted.
>
> Let's just say that env should be the only global register.  AREG1 and
> AREG2 in principle could still be used to work around bad register
> allocation in GCC, so I'm leaving them in dyngen-exec.h.
>
> Blue Swirl, can you check whether changing AREG0 to another register
> in dyngen-exec.h would fix the "annoying glibc bugs mangling global
> register variables"?  Or maybe we can remove the workaround altogether,
> considering the bug was fixed in version 2.3 of glibc dated 2001-11-29
> (at least that's what I'd guess from the history)?

The problem is with global register use by the system libraries. Sparc
V8 ABI reserves %g5 to %g7 for system. System may not use %g2 to %g4.
V9 ABI gives more registers to application use. For more information
see for example this page at Oracle:
http://developers.sun.com/solaris/articles/sparcv9abi.html

One part of the problem is that libraries are not compiled with
appropriate CFLAGS so that they avoid using global registers. The
second part is that the libraries may contain assembly language code
which directly uses the registers (especially setjmp/longjmp, which
are critical for QEMU).

Here are some statistics on Debian Lenny, with libc6 version 2.7-18lenny2:

$ objdump -d /lib/libc.so.6 |grep %g1|wc -l
73753
$ objdump -d /lib/libc.so.6 |grep %g2|wc -l
37571
$ objdump -d /lib/libc.so.6 |grep %g3|wc -l
23205
$ objdump -d /lib/libc.so.6 |grep %g4|wc -l
12267
$ objdump -d /lib/libc.so.6 |grep %g5|wc -l
444
$ objdump -d /lib/libc.so.6 |grep %g6|wc -l
150
$ objdump -d /lib/libc.so.6 |grep %g7|wc -l
2776

Same for OpenBSD 4.6, which is pure Sparc V9.

$ objdump -d /usr/lib/libc.so.51.0 |grep %g1|wc -l
   41040
$ objdump -d /usr/lib/libc.so.51.0 |grep %g2|wc -l
   21115
$ objdump -d /usr/lib/libc.so.51.0 |grep %g3|wc -l
   10945
$ objdump -d /usr/lib/libc.so.51.0 |grep %g4|wc -l
    6682
$ objdump -d /usr/lib/libc.so.51.0 |grep %g5|wc -l
    3712
$ objdump -d /usr/lib/libc.so.51.0 |grep %g6|wc -l
       4
$ objdump -d /usr/lib/libc.so.51.0 |grep %g7|wc -l
      20

So I guess changing the global register would not benefit Linux much.

If I remove the workaround from cpu-exec.c, a bus error is generated
almost immediately. Otherwise on Linux, sparc-softmmu can boot Linux
(sparc-test) image, but QEMU crashes just before command line. On
OpenBSD, the same test reaches command prompt.

[Qemu-devel] Re: [PATCH 0/2] simplify global register save/restore

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 472 bytes --]

On 02/13/2010 06:58 PM, Blue Swirl wrote:
> V9 ABI gives more registers to application use.

Except that glibc uses those---in theory, as I see it, it should be 
compiled with fixed g2 and g3 to leave them to the application.  I get 
it now.

It may be possible to make the workaround a big less ugly (I'm thinking 
of avoiding #undef/#define by using assembly).  I made a patch (see 
attachment, just FYI), maybe sometime I'll try it using self-virtualized 
qemu.

Paolo

[-- Attachment #2: sparc --]
[-- Type: text/plain, Size: 2730 bytes --]

commit 59ca12838278bed97ce5cc311f90ddfec7953047
Author: Paolo Bonzini <pbonzini@redhat.com>
Date:   Sat Feb 13 21:13:12 2010 +0100

    make sparc workaround less ugly
    
    Not-quite-signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/cpu-exec.c b/cpu-exec.c
index badd5d7..01b7143 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -39,9 +39,14 @@
 #endif
 
 #if defined(__sparc__) && !defined(CONFIG_SOLARIS)
-// Work around ugly bugs in glibc that mangle global register contents
-#undef env
+/* glibc will mangle global register contents.  To work around this,
+ * we avoid using the global register in this file, and place back
+ * cpu_single_env in AREG0 before giving control to target-* routines.
+ */
+#define export_env()	asm ("mov %0, %%" AREG0 : : "r" (cpu_single_env) : AREG0); 
 #define env cpu_single_env
+#else
+#define export_env()
 #endif
 
 int tb_invalidated_flag;
@@ -257,11 +262,7 @@ int cpu_exec(CPUState *env1)
     /* prepare setjmp context for exception handling */
     for(;;) {
         if (setjmp(env->jmp_env) == 0) {
-#if defined(__sparc__) && !defined(CONFIG_SOLARIS)
-#undef env
-                    env = cpu_single_env;
-#define env cpu_single_env
-#endif
+            export_env();
             /* if an exception is pending, we execute it here */
             if (env->exception_index >= 0) {
                 if (env->exception_index >= EXCP_INTERRUPT) {
@@ -387,11 +388,7 @@ int cpu_exec(CPUState *env1)
                             env->interrupt_request &= ~(CPU_INTERRUPT_HARD | CPU_INTERRUPT_VIRQ);
                             intno = cpu_get_pic_interrupt(env);
                             qemu_log_mask(CPU_LOG_TB_IN_ASM, "Servicing hardware INT=0x%02x\n", intno);
-#if defined(__sparc__) && !defined(CONFIG_SOLARIS)
-#undef env
-                    env = cpu_single_env;
-#define env cpu_single_env
-#endif
+                            export_env();
                             do_interrupt(intno, 0, 0, 0, 1);
                             /* ensure that no TB jump will be modified as
                                the program flow was changed */
@@ -603,12 +600,8 @@ int cpu_exec(CPUState *env1)
                 if (!unlikely (env->exit_request)) {
                     env->current_tb = tb;
                     tc_ptr = tb->tc_ptr;
-                /* execute the generated code */
-#if defined(__sparc__) && !defined(CONFIG_SOLARIS)
-#undef env
-                    env = cpu_single_env;
-#define env cpu_single_env
-#endif
+                    /* execute the generated code */
+                    export_env();
                     next_tb = tcg_qemu_tb_exec(tc_ptr);
                     env->current_tb = NULL;
                     if ((next_tb & 3) == 2) {

[Qemu-devel] Re: [PATCH 0/2] simplify global register save/restore

[Blue Swirl]

On Sat, Feb 13, 2010 at 10:26 PM, Paolo Bonzini <bonzini@gnu.org> wrote:
> On 02/13/2010 06:58 PM, Blue Swirl wrote:
>>
>> V9 ABI gives more registers to application use.
>
> Except that glibc uses those---in theory, as I see it, it should be compiled
> with fixed g2 and g3 to leave them to the application.  I get it now.
>
> It may be possible to make the workaround a big less ugly (I'm thinking of
> avoiding #undef/#define by using assembly).  I made a patch (see attachment,
> just FYI), maybe sometime I'll try it using self-virtualized qemu.

Yes, that's much better.

[Qemu-devel] Re: [PATCH 0/2] simplify global register save/restore

[Paolo Bonzini]

On 02/13/2010 09:57 PM, Blue Swirl wrote:
> On Sat, Feb 13, 2010 at 10:26 PM, Paolo Bonzini<bonzini@gnu.org>  wrote:
>> On 02/13/2010 06:58 PM, Blue Swirl wrote:
>>>
>>> V9 ABI gives more registers to application use.
>>
>> Except that glibc uses those---in theory, as I see it, it should be compiled
>> with fixed g2 and g3 to leave them to the application.  I get it now.
>>
>> It may be possible to make the workaround a big less ugly (I'm thinking of
>> avoiding #undef/#define by using assembly).  I made a patch (see attachment,
>> just FYI), maybe sometime I'll try it using self-virtualized qemu.
>
> Yes, that's much better.

If it works. :-)

Paolo

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paul Brook]

> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
> register that is actually used is AREG0, so the complexity of
> hostregs_helper.h is unwarranted.
> 
> Let's just say that env should be the only global register.  AREG1 and
> AREG2 in principle could still be used to work around bad register
> allocation in GCC, so I'm leaving them in dyngen-exec.h.

I think AREG[12] should be removed too. If we aren't saving them then they 
can't be safely used.

Paul

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paolo Bonzini]

On 02/26/2010 12:30 PM, Paul Brook wrote:
>> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
>> register that is actually used is AREG0, so the complexity of
>> hostregs_helper.h is unwarranted.
>>
>> Let's just say that env should be the only global register.  AREG1 and
>> AREG2 in principle could still be used to work around bad register
>> allocation in GCC, so I'm leaving them in dyngen-exec.h.
>
> I think AREG[12] should be removed too. If we aren't saving them then they
> can't be safely used.

You could still use them for local register variables, but I can prepare 
a patch to remove them (unless you do that yourself).

Paolo

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paul Brook]

> On 02/26/2010 12:30 PM, Paul Brook wrote:
> >> Since b567b38 (target-arm: remove T0 and T1, 2009-10-16) the only global
> >> register that is actually used is AREG0, so the complexity of
> >> hostregs_helper.h is unwarranted.
> >>
> >> Let's just say that env should be the only global register.  AREG1 and
> >> AREG2 in principle could still be used to work around bad register
> >> allocation in GCC, so I'm leaving them in dyngen-exec.h.
> >
> > I think AREG[12] should be removed too. If we aren't saving them then
> > they can't be safely used.
> 
> You could still use them for local register variables, but I can prepare
> a patch to remove them (unless you do that yourself).

I'm not sure what you mean by a "local register variable". If you're entirely 
within TCG then the AREG defines aren't used anyway. If you're communicating 
between TCG and C helpers then you need the cpu-exec.c save/restore.

I suspect you're thinking of TCG_AREG[12] in tcg-target.h. We should probably 
remove those and allow the register allocator to reclaim those registers.

Paul

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paolo Bonzini]

On 02/26/2010 07:32 PM, Paul Brook wrote:
>> >  You could still use them for local register variables, but I can prepare
>> >  a patch to remove them (unless you do that yourself).
> I'm not sure what you mean by a "local register variable".

I'm thinking of using

   register blah blah asm(AREG1);

as a local variable to work around deficiencies in GCC's register 
allocator.  I've seen that elsewhere though not in QEMU.

If you prefer to remove everything, fine.

Paolo

Re: [Qemu-devel] [PATCH 0/2] simplify global register save/restore

[Paul Brook]

> On 02/26/2010 07:32 PM, Paul Brook wrote:
> >> >  You could still use them for local register variables, but I can
> >> > prepare a patch to remove them (unless you do that yourself).
> >
> > I'm not sure what you mean by a "local register variable".
> 
> I'm thinking of using
> 
>    register blah blah asm(AREG1);

> as a local variable to work around deficiencies in GCC's register
> allocator.  I've seen that elsewhere though not in QEMU.

This probably doesn't do what you (or others) think it does. Local register 
variables are only honoured when used as operands to asm statements. This 
implies you already have cpu specific code, so there's no point generalising.
 
> If you prefer to remove everything, fine.

I would prefer they are removed.

Paul