Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On 3 September 2012 21:10, Blue Swirl wrote: > On Mon, Sep 3, 2012 at 7:54 PM, Peter Maydell > wrote: >> I don't want the *file* split, I'd just like to see this *patch* >> as 4 or 5 separate patches, not one big one. > > While converting, it's easier to work on whole files but maybe the > resulting patch can be still split. If it really doesn't seem splittable let me know and I'll wade through this big patch. -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On Mon, Sep 3, 2012 at 7:54 PM, Peter Maydell wrote: > On 3 September 2012 19:58, Blue Swirl wrote: >> On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell >> wrote: >>> On 3 September 2012 01:01, Peter Maydell wrote: >>> That's quite hard to cross-reference when the patch is this big. >>> I think it would be helpful if you could split it up into patches >>> touching smaller groups of helpers at once rather than having a >>> single patch that does them all at once. >> >> For x86, Sparc and s390x I used the approach of splitting op_helper.c >> to smaller files first. I didn't do it for ARM since >> target-arm/op_helper.c is alread pretty small (<500 lines). It could >> be split to saturating ops, condition code setting arithmetic ops and >> misc ops, between 100 and 200 lines each. Would that be OK? > > I don't want the *file* split, I'd just like to see this *patch* > as 4 or 5 separate patches, not one big one. While converting, it's easier to work on whole files but maybe the resulting patch can be still split. > > (Patch-splitting is a personal preference thing; I generally favour > lots of little patches over big ones.) That's just common sense. The conversion logic is just not very helpful here. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On 3 September 2012 19:58, Blue Swirl wrote: > On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell > wrote: >> On 3 September 2012 01:01, Peter Maydell wrote: >> That's quite hard to cross-reference when the patch is this big. >> I think it would be helpful if you could split it up into patches >> touching smaller groups of helpers at once rather than having a >> single patch that does them all at once. > > For x86, Sparc and s390x I used the approach of splitting op_helper.c > to smaller files first. I didn't do it for ARM since > target-arm/op_helper.c is alread pretty small (<500 lines). It could > be split to saturating ops, condition code setting arithmetic ops and > misc ops, between 100 and 200 lines each. Would that be OK? I don't want the *file* split, I'd just like to see this *patch* as 4 or 5 separate patches, not one big one. (Patch-splitting is a personal preference thing; I generally favour lots of little patches over big ones.) thanks -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On Mon, Sep 3, 2012 at 12:03 AM, Peter Maydell wrote: > On 3 September 2012 01:01, Peter Maydell wrote: >> On 2 September 2012 18:33, Blue Swirl wrote: >>> Add an explicit CPUState parameter instead of relying on AREG0 >>> and switch to AREG0 free mode. >>> >>> Signed-off-by: Blue Swirl >>> --- >>> configure|2 +- >>> target-arm/Makefile.objs |2 - >>> target-arm/cpu.h | 10 ++- >>> target-arm/helper.c |8 +- >>> target-arm/helper.h | 60 +- >>> target-arm/op_helper.c | 92 +--- >>> target-arm/translate.c | 148 >>> +++--- >>> 7 files changed, 158 insertions(+), 164 deletions(-) >> >> This is too big to easily review -- it's making a change to a lot >> of helpers, and in each case that change affects three places >> (callers, declaration, implementation). That' > > Sorry, finger slip meant I sent that half finished. To continue... > > That's quite hard to cross-reference when the patch is this big. > I think it would be helpful if you could split it up into patches > touching smaller groups of helpers at once rather than having a > single patch that does them all at once. For x86, Sparc and s390x I used the approach of splitting op_helper.c to smaller files first. I didn't do it for ARM since target-arm/op_helper.c is alread pretty small (<500 lines). It could be split to saturating ops, condition code setting arithmetic ops and misc ops, between 100 and 200 lines each. Would that be OK? It looks like helper.c should be split too (maybe VFP, MMU, CPU init, CPR), but that's starting to get beyond the scope of the series. > > thanks > -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On 2 September 2012 18:33, Blue Swirl wrote: > Add an explicit CPUState parameter instead of relying on AREG0 > and switch to AREG0 free mode. My cheesy test harness for running a popular embedded benchmark in system mode (x86-64 host, ARM guest) shows mostly slowdowns of between 2 and 3% with this patch applied. I think that falls into "not fantastic but acceptable for the cleanup". -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On 3 September 2012 01:01, Peter Maydell wrote: > On 2 September 2012 18:33, Blue Swirl wrote: >> Add an explicit CPUState parameter instead of relying on AREG0 >> and switch to AREG0 free mode. >> >> Signed-off-by: Blue Swirl >> --- >> configure|2 +- >> target-arm/Makefile.objs |2 - >> target-arm/cpu.h | 10 ++- >> target-arm/helper.c |8 +- >> target-arm/helper.h | 60 +- >> target-arm/op_helper.c | 92 +--- >> target-arm/translate.c | 148 >> +++--- >> 7 files changed, 158 insertions(+), 164 deletions(-) > > This is too big to easily review -- it's making a change to a lot > of helpers, and in each case that change affects three places > (callers, declaration, implementation). That' Sorry, finger slip meant I sent that half finished. To continue... That's quite hard to cross-reference when the patch is this big. I think it would be helpful if you could split it up into patches touching smaller groups of helpers at once rather than having a single patch that does them all at once. thanks -- PMM
Re: [Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
On 2 September 2012 18:33, Blue Swirl wrote:
> Add an explicit CPUState parameter instead of relying on AREG0
> and switch to AREG0 free mode.
>
> Signed-off-by: Blue Swirl
> ---
> configure|2 +-
> target-arm/Makefile.objs |2 -
> target-arm/cpu.h | 10 ++-
> target-arm/helper.c |8 +-
> target-arm/helper.h | 60 +-
> target-arm/op_helper.c | 92 +---
> target-arm/translate.c | 148
> +++---
> 7 files changed, 158 insertions(+), 164 deletions(-)
This is too big to easily review -- it's making a change to a lot
of helpers, and in each case that change affects three places
(callers, declaration, implementation). That'
> diff --git a/configure b/configure
> index 4fd3b7f..efb5014 100755
> --- a/configure
> +++ b/configure
> @@ -3829,7 +3829,7 @@ symlink "$source_path/Makefile.target"
> "$target_dir/Makefile"
>
>
> case "$target_arch2" in
> - alpha | i386 | lm32 | m68k | or32 | s390x | sparc* | unicore32 | x86_64 |
> xtensa* | ppc*)
> + alpha | arm* | i386 | lm32 | m68k | or32 | s390x | sparc* | unicore32 |
> x86_64 | xtensa* | ppc*)
> echo "CONFIG_TCG_PASS_AREG0=y" >> $config_target_mak
>;;
> esac
> diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
> index f447c4f..b6f1a9e 100644
> --- a/target-arm/Makefile.objs
> +++ b/target-arm/Makefile.objs
> @@ -2,5 +2,3 @@ obj-y += arm-semi.o
> obj-$(CONFIG_SOFTMMU) += machine.o
> obj-y += translate.o op_helper.o helper.o cpu.o
> obj-y += neon_helper.o iwmmxt_helper.o
> -
> -$(obj)/op_helper.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
> diff --git a/target-arm/cpu.h b/target-arm/cpu.h
> index d7f93d9..7fac94f 100644
> --- a/target-arm/cpu.h
> +++ b/target-arm/cpu.h
> @@ -734,9 +734,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env,
> TranslationBlock *tb)
> }
>
> /* Load an instruction and return it in the standard little-endian order */
> -static inline uint32_t arm_ldl_code(uint32_t addr, bool do_swap)
> +static inline uint32_t arm_ldl_code(CPUARMState *env, uint32_t addr,
> +bool do_swap)
> {
> -uint32_t insn = ldl_code(addr);
> +uint32_t insn = cpu_ldl_code(env, addr);
> if (do_swap) {
> return bswap32(insn);
> }
> @@ -744,9 +745,10 @@ static inline uint32_t arm_ldl_code(uint32_t addr, bool
> do_swap)
> }
>
> /* Ditto, for a halfword (Thumb) instruction */
> -static inline uint16_t arm_lduw_code(uint32_t addr, bool do_swap)
> +static inline uint16_t arm_lduw_code(CPUARMState *env, uint32_t addr,
> + bool do_swap)
> {
> -uint16_t insn = lduw_code(addr);
> +uint16_t insn = cpu_lduw_code(env, addr);
> if (do_swap) {
> return bswap16(insn);
> }
> diff --git a/target-arm/helper.c b/target-arm/helper.c
> index dceaa95..f4d711c 100644
> --- a/target-arm/helper.c
> +++ b/target-arm/helper.c
> @@ -1756,7 +1756,7 @@ static void do_interrupt_v7m(CPUARMState *env)
> case EXCP_BKPT:
> if (semihosting_enabled) {
> int nr;
> -nr = arm_lduw_code(env->regs[15], env->bswap_code) & 0xff;
> +nr = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
> if (nr == 0xab) {
> env->regs[15] += 2;
> env->regs[0] = do_arm_semihosting(env);
> @@ -1828,9 +1828,9 @@ void do_interrupt(CPUARMState *env)
> if (semihosting_enabled) {
> /* Check for semihosting interrupt. */
> if (env->thumb) {
> -mask = arm_lduw_code(env->regs[15] - 2, env->bswap_code) &
> 0xff;
> +mask = arm_lduw_code(env, env->regs[15] - 2,
> env->bswap_code) & 0xff;
> } else {
> -mask = arm_ldl_code(env->regs[15] - 4, env->bswap_code)
> +mask = arm_ldl_code(env, env->regs[15] - 4, env->bswap_code)
> & 0xff;
> }
> /* Only intercept calls from privileged modes, to provide some
> @@ -1851,7 +1851,7 @@ void do_interrupt(CPUARMState *env)
> case EXCP_BKPT:
> /* See if this is a semihosting syscall. */
> if (env->thumb && semihosting_enabled) {
> -mask = arm_lduw_code(env->regs[15], env->bswap_code) & 0xff;
> +mask = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
> if (mask == 0xab
>&& (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
> env->regs[15] += 2;
> diff --git a/target-arm/helper.h b/target-arm/helper.h
> index 21e9cfe..afdb2b5 100644
> --- a/target-arm/helper.h
> +++ b/target-arm/helper.h
> @@ -4,12 +4,12 @@ DEF_HELPER_1(clz, i32, i32)
> DEF_HELPER_1(sxtb16, i32, i32)
> DEF_HELPER_1(uxtb16, i32, i32)
>
> -DEF_HELPER_2(add_setq, i32, i32, i32)
> -DEF_HELPER_2(add_saturate, i32, i32, i32)
> -DEF_HELPER_2(sub_saturate, i32, i32, i32)
> -DEF_HELPER_
[Qemu-devel] [PATCH 16/21] target-arm: switch to AREG0 free mode
Add an explicit CPUState parameter instead of relying on AREG0
and switch to AREG0 free mode.
Signed-off-by: Blue Swirl
---
configure|2 +-
target-arm/Makefile.objs |2 -
target-arm/cpu.h | 10 ++-
target-arm/helper.c |8 +-
target-arm/helper.h | 60 +-
target-arm/op_helper.c | 92 +---
target-arm/translate.c | 148 +++---
7 files changed, 158 insertions(+), 164 deletions(-)
diff --git a/configure b/configure
index 4fd3b7f..efb5014 100755
--- a/configure
+++ b/configure
@@ -3829,7 +3829,7 @@ symlink "$source_path/Makefile.target"
"$target_dir/Makefile"
case "$target_arch2" in
- alpha | i386 | lm32 | m68k | or32 | s390x | sparc* | unicore32 | x86_64 |
xtensa* | ppc*)
+ alpha | arm* | i386 | lm32 | m68k | or32 | s390x | sparc* | unicore32 |
x86_64 | xtensa* | ppc*)
echo "CONFIG_TCG_PASS_AREG0=y" >> $config_target_mak
;;
esac
diff --git a/target-arm/Makefile.objs b/target-arm/Makefile.objs
index f447c4f..b6f1a9e 100644
--- a/target-arm/Makefile.objs
+++ b/target-arm/Makefile.objs
@@ -2,5 +2,3 @@ obj-y += arm-semi.o
obj-$(CONFIG_SOFTMMU) += machine.o
obj-y += translate.o op_helper.o helper.o cpu.o
obj-y += neon_helper.o iwmmxt_helper.o
-
-$(obj)/op_helper.o: QEMU_CFLAGS += $(HELPER_CFLAGS)
diff --git a/target-arm/cpu.h b/target-arm/cpu.h
index d7f93d9..7fac94f 100644
--- a/target-arm/cpu.h
+++ b/target-arm/cpu.h
@@ -734,9 +734,10 @@ static inline void cpu_pc_from_tb(CPUARMState *env,
TranslationBlock *tb)
}
/* Load an instruction and return it in the standard little-endian order */
-static inline uint32_t arm_ldl_code(uint32_t addr, bool do_swap)
+static inline uint32_t arm_ldl_code(CPUARMState *env, uint32_t addr,
+bool do_swap)
{
-uint32_t insn = ldl_code(addr);
+uint32_t insn = cpu_ldl_code(env, addr);
if (do_swap) {
return bswap32(insn);
}
@@ -744,9 +745,10 @@ static inline uint32_t arm_ldl_code(uint32_t addr, bool
do_swap)
}
/* Ditto, for a halfword (Thumb) instruction */
-static inline uint16_t arm_lduw_code(uint32_t addr, bool do_swap)
+static inline uint16_t arm_lduw_code(CPUARMState *env, uint32_t addr,
+ bool do_swap)
{
-uint16_t insn = lduw_code(addr);
+uint16_t insn = cpu_lduw_code(env, addr);
if (do_swap) {
return bswap16(insn);
}
diff --git a/target-arm/helper.c b/target-arm/helper.c
index dceaa95..f4d711c 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -1756,7 +1756,7 @@ static void do_interrupt_v7m(CPUARMState *env)
case EXCP_BKPT:
if (semihosting_enabled) {
int nr;
-nr = arm_lduw_code(env->regs[15], env->bswap_code) & 0xff;
+nr = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
if (nr == 0xab) {
env->regs[15] += 2;
env->regs[0] = do_arm_semihosting(env);
@@ -1828,9 +1828,9 @@ void do_interrupt(CPUARMState *env)
if (semihosting_enabled) {
/* Check for semihosting interrupt. */
if (env->thumb) {
-mask = arm_lduw_code(env->regs[15] - 2, env->bswap_code) &
0xff;
+mask = arm_lduw_code(env, env->regs[15] - 2, env->bswap_code)
& 0xff;
} else {
-mask = arm_ldl_code(env->regs[15] - 4, env->bswap_code)
+mask = arm_ldl_code(env, env->regs[15] - 4, env->bswap_code)
& 0xff;
}
/* Only intercept calls from privileged modes, to provide some
@@ -1851,7 +1851,7 @@ void do_interrupt(CPUARMState *env)
case EXCP_BKPT:
/* See if this is a semihosting syscall. */
if (env->thumb && semihosting_enabled) {
-mask = arm_lduw_code(env->regs[15], env->bswap_code) & 0xff;
+mask = arm_lduw_code(env, env->regs[15], env->bswap_code) & 0xff;
if (mask == 0xab
&& (env->uncached_cpsr & CPSR_M) != ARM_CPU_MODE_USR) {
env->regs[15] += 2;
diff --git a/target-arm/helper.h b/target-arm/helper.h
index 21e9cfe..afdb2b5 100644
--- a/target-arm/helper.h
+++ b/target-arm/helper.h
@@ -4,12 +4,12 @@ DEF_HELPER_1(clz, i32, i32)
DEF_HELPER_1(sxtb16, i32, i32)
DEF_HELPER_1(uxtb16, i32, i32)
-DEF_HELPER_2(add_setq, i32, i32, i32)
-DEF_HELPER_2(add_saturate, i32, i32, i32)
-DEF_HELPER_2(sub_saturate, i32, i32, i32)
-DEF_HELPER_2(add_usaturate, i32, i32, i32)
-DEF_HELPER_2(sub_usaturate, i32, i32, i32)
-DEF_HELPER_1(double_saturate, i32, s32)
+DEF_HELPER_3(add_setq, i32, env, i32, i32)
+DEF_HELPER_3(add_saturate, i32, env, i32, i32)
+DEF_HELPER_3(sub_saturate, i32, env, i32, i32)
+DEF_HELPER_3(add_usaturate, i32, env, i32, i32)
+DEF_HELPER_3(sub_usaturate, i32, env, i32, i32)
+DEF_HELPER_2(double_saturate, i32, env, s32)
DEF_HELPER_2(sdiv, s32, s32, s32)
DEF_HELPER_2(ud
