Re: [Qemu-devel] [PATCH 00/26] AREG0 conversion

[Blue Swirl]

On Wed, Oct 19, 2011 at 17:25, Richard Henderson <address@hidden> wrote:
> On 10/09/2011 12:20 PM, Blue Swirl wrote:
>>> I didn't bother to attach the patches, if someone wants to try, the
>>> patch set can be found here:
>>>        git://repo.or.cz/qemu/blueswirl.git
>>>        http://repo.or.cz/r/qemu/blueswirl.git
>>
>> I pushed the patch set to repo.or.cz so if someone wants to comment or
>> test, they are there.
>>
>> It's mostly the same stuff as before, though I split int_helper.c to
>> int32_helper.c and int64_helper.c since they have nothing in common
>> and extracted TCG iargs/oargs changes.
>>
>>> Blue Swirl (26):
>>>  Document softmmu templates
>>>  softmmu_header: pass CPUState to tlb_fill
>>>  Move GETPC from dyngen-exec.h to exec-all.h
>
> I don't see these three in the repo.

Because I applied them earlier...

>>>  Sparc: fix coding style
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split helper.c
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: move trivial functions from op_helper.c
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: avoid AREG0 for raise_exception and helper_debug
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: fix coding style
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split FPU and VIS op helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: avoid AREG0 for float and VIS ops
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split lazy condition code handling op helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: avoid AREG0 for lazy condition code helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split CWP and PSTATE op helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: avoid AREG0 for CWP and PSTATE helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
> An especially nice cleanup too, avoiding all of the saved_env frobbing.
>
>>>  Sparc: avoid AREG0 for softint op helpers and Leon cache control
>
> This one loses do_modify_softint in the move.  Which gets re-instated
> in your following "convert int_helper to trace framework" patch.  But
> in the meantime the series is non-bisectable.

Nice catch, will fix. I think I'll apply the patches before this one
now to shorten the series a bit.

>>>  Sparc: avoid AREG0 for division op helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: fix coding style in helper.c
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split MMU helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: convert mmu_helper to trace framework
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: convert int_helper to trace framework
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: convert win_helper to trace framework
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: split load and store op helpers
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  TCG: add 5 arg helpers to def-helper.h
>
> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  Sparc: avoid AREG0 for memory access helpers
>
>> +#define WRAP_LD(rettype, fn)                                    \
>> +    rettype cpu_ ## fn (CPUState *env1, target_ulong addr)      \
>> +    {                                                           \
>> +        CPUState *saved_env;                                    \
>> +        rettype ret;                                            \
>> +                                                                \
>> +        saved_env = env;                                        \
>> +        env = env1;                                             \
>> +        ret = fn(addr);                                         \
>> +        env = saved_env;                                        \
>> +        return ret;                                             \
>> +    }
>
> I don't think this actually works in the fault case.  In particular GETPC
> will return an incorrect address.  OTOH, I suppose we already have this
> problem from the other ldst helpers, e.g. ld_asi, which is where these new
> routines are going to be called from.  So this doesn't really change the
> state of affairs much.  I have no good ideas for solving this problem.

OK, maybe it's better to leave this and the 5 arg patch from 1.0.

> Reviewed-by: Richard Henderson <address@hidden>
>
>>>  softmmu templates: optionally pass CPUState to memory access
>>>    functions
>>>  Sparc: avoid AREG0 wrappers for memory access helpers
>
> Both look ok.  I'm certainly not fond of the intermediate state.  If we
> convert target-sparc and tcg-i386, we should convert all of them, and
> not leave that intermediate state for long.
>
> Something that's clearly not going to happen for the 1.0 release.

Fully agree. Thanks for the review.