Re: [Qemu-devel] [PATCH 2/5] softmmu templates: optionally pass CPUState to memory access functions

[malc <av1474@comtv.ru>]

On Fri, 24 Aug 2012, Aurelien Jarno wrote:

> On Sun, Mar 11, 2012 at 10:24:03PM +0000, Blue Swirl wrote:
> > Optionally, make memory access helpers take a parameter for CPUState
> > instead of relying on global env.
> > 
> > On most targets, perform simple moves to reorder registers. On i386,
> > switch from regparm(3) calling convention to standard stack-based
> > version.
> > 
> > Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
> > ---
> >  cpu-all.h              |    9 +++++
> >  exec-all.h             |    2 +
> >  exec.c                 |    4 ++
> >  softmmu_defs.h         |   28 ++++++++++++++++
> >  softmmu_header.h       |   60 ++++++++++++++++++++++++++--------
> >  softmmu_template.h     |   84 ++++++++++++++++++++++++++++++++---------------
> >  tcg/arm/tcg-target.c   |   53 ++++++++++++++++++++++++++++++
> >  tcg/hppa/tcg-target.c  |   44 +++++++++++++++++++++++++
> >  tcg/i386/tcg-target.c  |   57 ++++++++++++++++++++++++++++++++
> >  tcg/ia64/tcg-target.c  |   46 ++++++++++++++++++++++++++
> >  tcg/mips/tcg-target.c  |   44 +++++++++++++++++++++++++
> >  tcg/ppc/tcg-target.c   |   45 +++++++++++++++++++++++++
> >  tcg/ppc64/tcg-target.c |   44 +++++++++++++++++++++++++
> >  tcg/s390/tcg-target.c  |   44 +++++++++++++++++++++++++
> >  tcg/sparc/tcg-target.c |   50 +++++++++++++++++++++++++++--
> >  tcg/tci/tcg-target.c   |    6 +++
> >  16 files changed, 576 insertions(+), 44 deletions(-)
> > 
> 
> This commit completely broke arm and mips host support, not only for 64
> bit targets as written in the comments, but even for 32 bit targets as
> shifting arguments one by one doesn't work for qemu_st64 which needs 5
> values, while only 4 can be passed in registers.
> 
> Moreover even on x86_64, this introduces some performance regressions by
> emitting 4 additional moves in the slow path and adding some more
> constraints on the registers that can be used for passing arguments to
> ld/st ops.
> 
> While more and more targets needs AREG0 to be passed, I have started to
> work on fixing that. I came to the conclusion that passing AREG0 as the
> first argument, even if it is look the nice way to do it in C is
> probably not the best option:
> - On 32 bit hosts, which usually need register alignments for 64-bit
>   values (at least on arm and mips), given AREG0 is a 32-bit value this
            ditto ppc32

>   makes the register usage very inefficient when the address or the value
>   are 64 bits, in addition to making the code to handle quite complex. It
>   would be better to place it close to mem_idx which is also 32 bits.
> - On at least ppc, ppc64, sparc64 and x86_64, this adds some more 
>   constraints to the ld/st ops arguments.
> - On x86_64 This also means the address loading of the first argument done
>   in the  TLB function can't be reused easily (it's not a problem right now
>   due to registers shifting, but this problem appears when trying to clean
>   the code).
> - Finally on all hosts, this make the AREG0 / nonAREG0 load/store
>   different, and thus the load/store code much more complex (this is
>   something that should disappear when all targets are using the AREG0
>   case).
> 
> That's why I would propose to move the env argument to the last
> argument. It's better to place it after mem_idx, as it is usually easier
> to store a register on the stack than an immediate value. It also means
> we don't need any register shifting, the code change for most hosts 
> would be only a few lines to either copy a value from one register to 
> another, or to store a register on the stack, that is without additional
> constraints (there is a call after that so the argument registers are 
> already clobbered).
> 
> What do you think of that? If that seems the way to go, I can start
> writing patches to do the changes and fix most hosts support.
> 
> Aurelien
> 
> 

-- 
mailto:av1474@comtv.ru