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

[Blue Swirl]

On Fri, Aug 24, 2012 at 3:05 PM, Aurelien Jarno <address@hidden> 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 <address@hidden>
>> ---
>>  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.

IIRC this was an earlier version, at least regparm() stuff was separated.

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

For 1.2, fixing host support would be most important. It's a good idea
to change the order, but I'd postpone it to 1.3.

>
> Aurelien
>
> --
> Aurelien Jarno                          GPG: 1024D/F1BCDB73
> address@hidden                 http://www.aurel32.net