Re: [Qemu-devel] [PATCH 20/20] ppc: move load and store helpers, switch to AREG0 free mode

[Blue Swirl]

On Mon, Apr 16, 2012 at 16:52, Alexander Graf <address@hidden> wrote:
>
> On 31.03.2012, at 18:32, Blue Swirl wrote:
>
>> Add an explicit CPUPPCState parameter instead of relying on AREG0
>> and rename op_helper.c (which only contains load and store helpers)
>> to mem_helper.c. Remove AREG0 swapping in
>> tlb_fill().
>>
>> Switch to AREG0 free mode. Use cpu_ld{l,uw}_code in translation
>> and interrupt handling.
>>
>> Signed-off-by: Blue Swirl <address@hidden>
>> ---
>> Makefile.target                          |    6 +-
>> configure                                |    2 +-
>> target-ppc/excp_helper.c                 |    3 +-
>> target-ppc/helper.h                      |   30 ++++----
>> target-ppc/{op_helper.c => mem_helper.c} |  117 
>> ++++++++++++++++--------------
>> target-ppc/translate.c                   |   30 ++++----
>> 6 files changed, 100 insertions(+), 88 deletions(-)
>> rename target-ppc/{op_helper.c => mem_helper.c} (66%)
>>
>> diff --git a/Makefile.target b/Makefile.target
>> index 96b4c05..b45b773 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -82,10 +82,12 @@ libobj-y += tcg/tcg.o tcg/optimize.o
>> libobj-$(CONFIG_TCG_INTERPRETER) += tci.o
>> libobj-y += fpu/softfloat.o
>> ifneq ($(TARGET_BASE_ARCH), sparc)
>> +ifneq ($(TARGET_BASE_ARCH), ppc)
>> ifneq ($(TARGET_BASE_ARCH), alpha)
>> libobj-y += op_helper.o
>> endif
>> endif
>> +endif
>> libobj-y += helper.o
>> ifeq ($(TARGET_BASE_ARCH), i386)
>> libobj-y += cpuid.o
>> @@ -104,7 +106,7 @@ libobj-$(TARGET_UNICORE32) += cpu.o
>> libobj-$(TARGET_ALPHA) += int_helper.o fpu_helper.o sys_helper.o mem_helper.o
>> ifeq ($(TARGET_BASE_ARCH), ppc)
>> libobj-y += fpu_helper.o int_helper.o mmu_helper.o
>> -libobj-y += mmu_helper.o excp_helper.o timebase_helper.o misc_helper.o
>> +libobj-y += mmu_helper.o excp_helper.o timebase_helper.o
>> misc_helper.o mem_helper.o
>> endif
>>
>> libobj-y += disas.o
>> @@ -116,7 +118,7 @@ $(libobj-y): $(GENERATED_HEADERS)
>>
>> # HELPER_CFLAGS is used for all the legacy code compiled with static register
>> # variables
>> -ifneq ($(TARGET_BASE_ARCH), sparc)
>> +ifndef CONFIG_TCG_PASS_AREG0
>> op_helper.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
>> endif
>> user-exec.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
>> diff --git a/configure b/configure
>> index b51a749..de19ca4 100755
>> --- a/configure
>> +++ b/configure
>> @@ -3616,7 +3616,7 @@ case "$target_arch2" in
>> esac
>>
>> case "$target_arch2" in
>> -  alpha | sparc*)
>> +  alpha | ppc* | sparc*)
>>     echo "CONFIG_TCG_PASS_AREG0=y" >> $config_target_mak
>>   ;;
>> esac
>> diff --git a/target-ppc/excp_helper.c b/target-ppc/excp_helper.c
>> index 945cd66..0f2ad4e 100644
>> --- a/target-ppc/excp_helper.c
>> +++ b/target-ppc/excp_helper.c
>> @@ -179,7 +179,8 @@ static inline void powerpc_excp(CPUPPCState *env,
>> int excp_model, int excp)
>>         }
>>         /* XXX: this is false */
>>         /* Get rS/rD and rA from faulting opcode */
>> -        env->spr[SPR_DSISR] |= (ldl_code((env->nip - 4)) & 0x03FF0000) >> 
>> 16;
>> +        env->spr[SPR_DSISR] |= (cpu_ldl_code(env, (env->nip - 4))
>> +                                & 0x03FF0000) >> 16;
>
> Mind to explain why we need the change from ldl to cpu_ldl?

Function signatures are different, ldl_code() does not take any
CPUState *env argument but uses AREG0 directly.

>>         goto store_current;
>>     case POWERPC_EXCP_PROGRAM:   /* Program exception                        
>> */
>>         switch (env->error_code & ~0xF) {
>> diff --git a/target-ppc/helper.h b/target-ppc/helper.h
>> index 1939c66..a4f033b 100644
>> --- a/target-ppc/helper.h
>> +++ b/target-ppc/helper.h
>> @@ -20,15 +20,15 @@ DEF_HELPER_1(hrfid, void, env)
>> #endif
>> #endif
>>
>> -DEF_HELPER_2(lmw, void, tl, i32)
>> -DEF_HELPER_2(stmw, void, tl, i32)
>> -DEF_HELPER_3(lsw, void, tl, i32, i32)
>> -DEF_HELPER_4(lswx, void, tl, i32, i32, i32)
>> -DEF_HELPER_3(stsw, void, tl, i32, i32)
>> -DEF_HELPER_1(dcbz, void, tl)
>> -DEF_HELPER_1(dcbz_970, void, tl)
>> -DEF_HELPER_1(icbi, void, tl)
>> -DEF_HELPER_4(lscbx, tl, tl, i32, i32, i32)
>> +DEF_HELPER_3(lmw, void, env, tl, i32)
>> +DEF_HELPER_3(stmw, void, env, tl, i32)
>> +DEF_HELPER_4(lsw, void, env, tl, i32, i32)
>> +DEF_HELPER_5(lswx, void, env, tl, i32, i32, i32)
>> +DEF_HELPER_4(stsw, void, env, tl, i32, i32)
>> +DEF_HELPER_2(dcbz, void, env, tl)
>> +DEF_HELPER_2(dcbz_970, void, env, tl)
>> +DEF_HELPER_2(icbi, void, env, tl)
>> +DEF_HELPER_5(lscbx, tl, env, tl, i32, i32, i32)
>>
>> #if defined(TARGET_PPC64)
>> DEF_HELPER_FLAGS_2(mulhd, TCG_CALL_CONST | TCG_CALL_PURE, i64, i64, i64)
>> @@ -226,12 +226,12 @@ DEF_HELPER_5(vmsumshm, void, env, avr, avr, avr, avr)
>> DEF_HELPER_5(vmsumshs, void, env, avr, avr, avr, avr)
>> DEF_HELPER_4(vmladduhm, void, avr, avr, avr, avr)
>> DEF_HELPER_2(mtvscr, void, env, avr);
>> -DEF_HELPER_2(lvebx, void, avr, tl)
>> -DEF_HELPER_2(lvehx, void, avr, tl)
>> -DEF_HELPER_2(lvewx, void, avr, tl)
>> -DEF_HELPER_2(stvebx, void, avr, tl)
>> -DEF_HELPER_2(stvehx, void, avr, tl)
>> -DEF_HELPER_2(stvewx, void, avr, tl)
>> +DEF_HELPER_3(lvebx, void, env, avr, tl)
>> +DEF_HELPER_3(lvehx, void, env, avr, tl)
>> +DEF_HELPER_3(lvewx, void, env, avr, tl)
>> +DEF_HELPER_3(stvebx, void, env, avr, tl)
>> +DEF_HELPER_3(stvehx, void, env, avr, tl)
>> +DEF_HELPER_3(stvewx, void, env, avr, tl)
>> DEF_HELPER_4(vsumsws, void, env, avr, avr, avr)
>> DEF_HELPER_4(vsum2sws, void, env, avr, avr, avr)
>> DEF_HELPER_4(vsum4sbs, void, env, avr, avr, avr)
>> diff --git a/target-ppc/op_helper.c b/target-ppc/mem_helper.c
>> similarity index 66%
>> rename from target-ppc/op_helper.c
>> rename to target-ppc/mem_helper.c
>> index dcdbf5f..11eca68 100644
>> --- a/target-ppc/op_helper.c
>> +++ b/target-ppc/mem_helper.c
>> @@ -1,5 +1,5 @@
>> /*
>> - *  PowerPC emulation helpers for QEMU.
>> + *  PowerPC memory access emulation helpers for QEMU.
>>  *
>>  *  Copyright (c) 2003-2007 Jocelyn Mayer
>>  *
>> @@ -16,9 +16,7 @@
>>  * 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/>.
>>  */
>> -#include <string.h>
>> #include "cpu.h"
>> -#include "dyngen-exec.h"
>> #include "host-utils.h"
>> #include "helper.h"
>>
>> @@ -26,12 +24,21 @@
>>
>> #if !defined(CONFIG_USER_ONLY)
>> #include "softmmu_exec.h"
>> +#else
>> +/* ??? Put these somewhere else? */
>> +#define cpu_ldub_data(env, addr) ldub_raw(addr)
>> +#define cpu_lduw_data(env, addr) lduw_raw(addr)
>> +#define cpu_ldl_data(env, addr) ldl_raw(addr)
>> +#define cpu_stb_data(env, addr, data) stb_raw(addr, data)
>> +#define cpu_stw_data(env, addr, data) stw_raw(addr, data)
>> +#define cpu_stl_data(env, addr, data) stl_raw(addr, data)
>
> Please avoid double negation - just use
>
>  #ifdef CONFIG_USER_ONLY
>  #else
>  #endif
>
> which makes the code more readable.

OK.

>
> Also, why not use ldl and friends here?

This is for user emulation case. In softmmu_exec.h, ldl() is defined
as ldl_data(). Some other similar defines are in cpu-all.h. Actually I
think that would be a better place than here.

>
>
> Alex
>