Message ID | alpine.DEB.1.10.1411191702010.2881@tp.orcam.me.uk |
---|---|
State | New |
Headers | show |
On 19/11/2014 17:29, Maciej W. Rozycki wrote: > qemu-mips32-addr.diff > Index: qemu-git-trunk/target-mips/cpu.h > =================================================================== > --- qemu-git-trunk.orig/target-mips/cpu.h 2014-11-12 07:41:26.597542010 +0000 > +++ qemu-git-trunk/target-mips/cpu.h 2014-11-12 07:41:26.597542010 +0000 > @@ -843,10 +843,12 @@ static inline void compute_hflags(CPUMIP > env->hflags |= MIPS_HFLAG_64; > } > > - if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) && > - !(env->CP0_Status & (1 << CP0St_UX))) { > + if (!(env->insn_flags & ISA_MIPS3)) { > env->hflags |= MIPS_HFLAG_AWRAP; > - } else if (env->insn_flags & ISA_MIPS32R6) { > + } else if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) && > + !(env->CP0_Status & (1 << CP0St_UX))) { > + env->hflags |= MIPS_HFLAG_AWRAP; > + } else if (env->insn_flags & ISA_MIPS64R6) { > /* Address wrapping for Supervisor and Kernel is specified in R6 */ > if ((((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_SM) && > !(env->CP0_Status & (1 << CP0St_SX))) || > Index: qemu-git-trunk/target-mips/translate.c > =================================================================== > --- qemu-git-trunk.orig/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 > +++ qemu-git-trunk/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 > @@ -10724,6 +10724,7 @@ static void gen_mips16_save (DisasContex > { > TCGv t0 = tcg_temp_new(); > TCGv t1 = tcg_temp_new(); > + TCGv t2 = tcg_temp_new(); > int args, astatic; > > switch (aregs) { > @@ -10782,7 +10783,8 @@ static void gen_mips16_save (DisasContex > gen_load_gpr(t0, 29); > > #define DECR_AND_STORE(reg) do { \ > - tcg_gen_subi_tl(t0, t0, 4); \ > + tcg_gen_movi_tl(t2, -4); \ Wouldn't it be better to move this line outside of the macro to avoid generating unnecessary tcg ops? DECR_AND_STORE is called multiple times and t2 doesn't change in-between. > + gen_op_addr_add(ctx, t0, t0, t2); \ > gen_load_gpr(t1, reg); \ > tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL); \ > } while (0) > @@ -10866,9 +10868,11 @@ static void gen_mips16_save (DisasContex > } > #undef DECR_AND_STORE > > - tcg_gen_subi_tl(cpu_gpr[29], cpu_gpr[29], framesize); > + tcg_gen_movi_tl(t2, -framesize); > + gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2); > tcg_temp_free(t0); > tcg_temp_free(t1); > + tcg_temp_free(t2); > } > > static void gen_mips16_restore (DisasContext *ctx, > @@ -10879,11 +10883,14 @@ static void gen_mips16_restore (DisasCon > int astatic; > TCGv t0 = tcg_temp_new(); > TCGv t1 = tcg_temp_new(); > + TCGv t2 = tcg_temp_new(); > > - tcg_gen_addi_tl(t0, cpu_gpr[29], framesize); > + tcg_gen_movi_tl(t2, framesize); > + gen_op_addr_add(ctx, t0, cpu_gpr[29], t2); > > #define DECR_AND_LOAD(reg) do { \ > - tcg_gen_subi_tl(t0, t0, 4); \ > + tcg_gen_movi_tl(t2, -4); \ The same here. > + gen_op_addr_add(ctx, t0, t0, t2); \ > tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL); \ > gen_store_gpr(t1, reg); \ > } while (0) > @@ -10967,9 +10974,11 @@ static void gen_mips16_restore (DisasCon > } > #undef DECR_AND_LOAD > > - tcg_gen_addi_tl(cpu_gpr[29], cpu_gpr[29], framesize); > + tcg_gen_movi_tl(t2, framesize); > + gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2); > tcg_temp_free(t0); > tcg_temp_free(t1); > + tcg_temp_free(t2); > } > > static void gen_addiupc (DisasContext *ctx, int rx, int imm, > Otherwise, Reviewed-by: Leon Alrae <leon.alrae@imgtec.com>
On Thu, 4 Dec 2014, Leon Alrae wrote: > > Index: qemu-git-trunk/target-mips/translate.c > > =================================================================== > > --- qemu-git-trunk.orig/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 > > +++ qemu-git-trunk/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 > > @@ -10724,6 +10724,7 @@ static void gen_mips16_save (DisasContex > > { > > TCGv t0 = tcg_temp_new(); > > TCGv t1 = tcg_temp_new(); > > + TCGv t2 = tcg_temp_new(); > > int args, astatic; > > > > switch (aregs) { > > @@ -10782,7 +10783,8 @@ static void gen_mips16_save (DisasContex > > gen_load_gpr(t0, 29); > > > > #define DECR_AND_STORE(reg) do { \ > > - tcg_gen_subi_tl(t0, t0, 4); \ > > + tcg_gen_movi_tl(t2, -4); \ > > Wouldn't it be better to move this line outside of the macro to avoid > generating unnecessary tcg ops? DECR_AND_STORE is called multiple times > and t2 doesn't change in-between. It would. This code isn't wrong though unlike our current version, this is merely a pessimisation. An update will require a costly regression test rerun and therefore I'll give the priority to the outstanding changes I have before going back to this change. Thanks for the tip and your review. Maciej
On 05/12/2014 18:55, Maciej W. Rozycki wrote: > On Thu, 4 Dec 2014, Leon Alrae wrote: > >>> Index: qemu-git-trunk/target-mips/translate.c >>> =================================================================== >>> --- qemu-git-trunk.orig/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 >>> +++ qemu-git-trunk/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 >>> @@ -10724,6 +10724,7 @@ static void gen_mips16_save (DisasContex >>> { >>> TCGv t0 = tcg_temp_new(); >>> TCGv t1 = tcg_temp_new(); >>> + TCGv t2 = tcg_temp_new(); >>> int args, astatic; >>> >>> switch (aregs) { >>> @@ -10782,7 +10783,8 @@ static void gen_mips16_save (DisasContex >>> gen_load_gpr(t0, 29); >>> >>> #define DECR_AND_STORE(reg) do { \ >>> - tcg_gen_subi_tl(t0, t0, 4); \ >>> + tcg_gen_movi_tl(t2, -4); \ >> >> Wouldn't it be better to move this line outside of the macro to avoid >> generating unnecessary tcg ops? DECR_AND_STORE is called multiple times >> and t2 doesn't change in-between. > > It would. This code isn't wrong though unlike our current version, > this is merely a pessimisation. An update will require a costly > regression test rerun and therefore I'll give the priority to the > outstanding changes I have before going back to this change. Thanks for > the tip and your review. Since above issue is minor we can correct it with incremental patch later. I applied all the patches (excluding two patches touching machine.c) which were sent prior to IEEE 754-2008 series to mips-next branch, thanks. I'm intending to send out mips-next branch next week. Regards, Leon
On Fri, 12 Dec 2014, Leon Alrae wrote: > > It would. This code isn't wrong though unlike our current version, > > this is merely a pessimisation. An update will require a costly > > regression test rerun and therefore I'll give the priority to the > > outstanding changes I have before going back to this change. Thanks for > > the tip and your review. > > Since above issue is minor we can correct it with incremental patch later. Good! > I applied all the patches (excluding two patches touching machine.c) > which were sent prior to IEEE 754-2008 series to mips-next branch, > thanks. I'm intending to send out mips-next branch next week. Great! I have now posted all the changes I had outstanding, there will be no more. Maciej
On 15/12/2014 18:07, Maciej W. Rozycki wrote: > Great! I have now posted all the changes I had outstanding, there will > be no more. Thanks for the patches, they are very valuable - especially IEEE 754-2008, it's a significant improvement for MIPS! I'll take a closer look at the remaining ones, but most likely not earlier than at the beginning of a new year. Regards, Leon
Index: qemu-git-trunk/target-mips/cpu.h =================================================================== --- qemu-git-trunk.orig/target-mips/cpu.h 2014-11-12 07:41:26.597542010 +0000 +++ qemu-git-trunk/target-mips/cpu.h 2014-11-12 07:41:26.597542010 +0000 @@ -843,10 +843,12 @@ static inline void compute_hflags(CPUMIP env->hflags |= MIPS_HFLAG_64; } - if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) && - !(env->CP0_Status & (1 << CP0St_UX))) { + if (!(env->insn_flags & ISA_MIPS3)) { env->hflags |= MIPS_HFLAG_AWRAP; - } else if (env->insn_flags & ISA_MIPS32R6) { + } else if (((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_UM) && + !(env->CP0_Status & (1 << CP0St_UX))) { + env->hflags |= MIPS_HFLAG_AWRAP; + } else if (env->insn_flags & ISA_MIPS64R6) { /* Address wrapping for Supervisor and Kernel is specified in R6 */ if ((((env->hflags & MIPS_HFLAG_KSU) == MIPS_HFLAG_SM) && !(env->CP0_Status & (1 << CP0St_SX))) || Index: qemu-git-trunk/target-mips/translate.c =================================================================== --- qemu-git-trunk.orig/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 +++ qemu-git-trunk/target-mips/translate.c 2014-11-12 07:41:26.597542010 +0000 @@ -10724,6 +10724,7 @@ static void gen_mips16_save (DisasContex { TCGv t0 = tcg_temp_new(); TCGv t1 = tcg_temp_new(); + TCGv t2 = tcg_temp_new(); int args, astatic; switch (aregs) { @@ -10782,7 +10783,8 @@ static void gen_mips16_save (DisasContex gen_load_gpr(t0, 29); #define DECR_AND_STORE(reg) do { \ - tcg_gen_subi_tl(t0, t0, 4); \ + tcg_gen_movi_tl(t2, -4); \ + gen_op_addr_add(ctx, t0, t0, t2); \ gen_load_gpr(t1, reg); \ tcg_gen_qemu_st_tl(t1, t0, ctx->mem_idx, MO_TEUL); \ } while (0) @@ -10866,9 +10868,11 @@ static void gen_mips16_save (DisasContex } #undef DECR_AND_STORE - tcg_gen_subi_tl(cpu_gpr[29], cpu_gpr[29], framesize); + tcg_gen_movi_tl(t2, -framesize); + gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2); tcg_temp_free(t0); tcg_temp_free(t1); + tcg_temp_free(t2); } static void gen_mips16_restore (DisasContext *ctx, @@ -10879,11 +10883,14 @@ static void gen_mips16_restore (DisasCon int astatic; TCGv t0 = tcg_temp_new(); TCGv t1 = tcg_temp_new(); + TCGv t2 = tcg_temp_new(); - tcg_gen_addi_tl(t0, cpu_gpr[29], framesize); + tcg_gen_movi_tl(t2, framesize); + gen_op_addr_add(ctx, t0, cpu_gpr[29], t2); #define DECR_AND_LOAD(reg) do { \ - tcg_gen_subi_tl(t0, t0, 4); \ + tcg_gen_movi_tl(t2, -4); \ + gen_op_addr_add(ctx, t0, t0, t2); \ tcg_gen_qemu_ld_tl(t1, t0, ctx->mem_idx, MO_TESL); \ gen_store_gpr(t1, reg); \ } while (0) @@ -10967,9 +10974,11 @@ static void gen_mips16_restore (DisasCon } #undef DECR_AND_LOAD - tcg_gen_addi_tl(cpu_gpr[29], cpu_gpr[29], framesize); + tcg_gen_movi_tl(t2, framesize); + gen_op_addr_add(ctx, cpu_gpr[29], cpu_gpr[29], t2); tcg_temp_free(t0); tcg_temp_free(t1); + tcg_temp_free(t2); } static void gen_addiupc (DisasContext *ctx, int rx, int imm,
Make sure the address space is unconditionally wrapped on 32-bit processors, that is ones that do not implement at least the MIPS III ISA. Also make MIPS16 SAVE and RESTORE instructions use address calculation rather than plain arithmetic operations for stack pointer manipulation so that their semantics for stack accesses follows the architecture specification. That in particular applies to user software run on 64-bit processors with the CP0.Status.UX bit clear where the address space is wrapped to 32 bits. Signed-off-by: Maciej W. Rozycki <macro@codesourcery.com> --- Hi, This change was also tested by running full GCC, G++ and glibc mips-linux-gnu toolchain test suites under Linux (Debian Wheezy) run in QEMU in the system emulation mode, for the following multilibs: -EB -EB -mips16 -EB -mmicromips -EB -mabi=n32 -EB -mabi=64 and the -EL variants of same. Of these standard MIPS o32 testing was run on a 24Kf processor and n64/n32 testing was run on a 5KEf processor, using a 32-bit and a 64-bit kernel respectively. MIPS16 o32 testing was run on an artificial 5KEf-mips16 processor -- like a real 5KEf one, but with the MIPS16 ASE enabled, and a 64-bit kernel. Finally microMIPS o32 testing was run on an artificial 24Kf-micromips processor -- like a real 24Kf one, but with the microMIPS ASE enabled, and a 32-bit kernel built as microMIPS code itself. The original test result counts were as follows: Result Count ERROR 20 FAIL 300 PASS 1732076 XPASS 335 XFAIL 6527 UNRESOLVED 26 UNSUPPORTED 50854 Total 1790138 After the change they were: Result Count ERROR 20 FAIL 298 PASS 1732078 XPASS 336 XFAIL 6526 UNRESOLVED 26 UNSUPPORTED 50854 Total 1790138 The changes in results were as follows: PASS -> FAIL: qemu-n32-el/glibc.sum: nptl/tst-cancel17 FAIL -> PASS: qemu-micromips/glibc.sum: nptl/tst-cancel17 FAIL -> PASS: qemu-micromips/glibc.sum: nptl/tst-setuid3 FAIL -> PASS: qemu-mips16-el/glibc.sum: nptl/tst-cancelx17 XFAIL -> XPASS: qemu-micromips/glibc.sum: nptl/tst-cancel7 -- as you can see glibc results continue fluctuating although this time they are slightly better than originally. Please apply. Maciej qemu-mips32-addr.diff