target-mips: Correct 32-bit address space wrapping

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

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