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

[Alexander Graf]

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?

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

Also, why not use ldl and friends here?


Alex