[v2,2/2] get rid of hostregs_helper.h

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

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
>
>

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?

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

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

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