* [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
@ 2014-11-19 17:29 Maciej W. Rozycki
2014-12-04 16:50 ` Leon Alrae
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2014-11-19 17:29 UTC (permalink / raw)
To: qemu-devel; +Cc: Leon Alrae, Aurelien Jarno
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
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,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
2014-11-19 17:29 [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping Maciej W. Rozycki
@ 2014-12-04 16:50 ` Leon Alrae
2014-12-05 18:55 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Leon Alrae @ 2014-12-04 16:50 UTC (permalink / raw)
To: Maciej W. Rozycki, qemu-devel; +Cc: Aurelien Jarno
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
2014-12-04 16:50 ` Leon Alrae
@ 2014-12-05 18:55 ` Maciej W. Rozycki
2014-12-12 12:19 ` Leon Alrae
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2014-12-05 18:55 UTC (permalink / raw)
To: Leon Alrae; +Cc: qemu-devel, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
2014-12-05 18:55 ` Maciej W. Rozycki
@ 2014-12-12 12:19 ` Leon Alrae
2014-12-15 18:07 ` Maciej W. Rozycki
0 siblings, 1 reply; 6+ messages in thread
From: Leon Alrae @ 2014-12-12 12:19 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: qemu-devel, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
2014-12-12 12:19 ` Leon Alrae
@ 2014-12-15 18:07 ` Maciej W. Rozycki
2014-12-18 10:16 ` Leon Alrae
0 siblings, 1 reply; 6+ messages in thread
From: Maciej W. Rozycki @ 2014-12-15 18:07 UTC (permalink / raw)
To: Leon Alrae; +Cc: qemu-devel, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping
2014-12-15 18:07 ` Maciej W. Rozycki
@ 2014-12-18 10:16 ` Leon Alrae
0 siblings, 0 replies; 6+ messages in thread
From: Leon Alrae @ 2014-12-18 10:16 UTC (permalink / raw)
To: Maciej W. Rozycki; +Cc: qemu-devel, Aurelien Jarno
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2014-12-18 10:17 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-19 17:29 [Qemu-devel] [PATCH] target-mips: Correct 32-bit address space wrapping Maciej W. Rozycki
2014-12-04 16:50 ` Leon Alrae
2014-12-05 18:55 ` Maciej W. Rozycki
2014-12-12 12:19 ` Leon Alrae
2014-12-15 18:07 ` Maciej W. Rozycki
2014-12-18 10:16 ` Leon Alrae
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.