Skip to contentSkip to conversation

Fix [ls][wd][lr] instructions

PATCH In review Aurelien Jarno 2 participants 6 messages Sep 26, 2007 – Oct 6, 2007 Kbuild

lore
Share & tools
lore

Aurelien Jarno

author
#0 lore raw

PATCH 1/1 Fix [ls][wd][lr] instructions

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr, sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.

Bye,
Aurelien

Index: Makefile.target
===================================================================
RCS file: /sources/qemu/qemu/Makefile.target,v
retrieving revision 1.197

Makefile.target +1 -1
diff -u -d -p -r1.197 Makefile.target
@@ -633,7 +633,7 @@ endif
 ifeq ($(TARGET_BASE_ARCH), mips)
 helper.o: cpu.h exec-all.h
 op.o: op_template.c fop_template.c op_mem.c exec.h cpu.h
-op_helper.o: op_helper_mem.c exec.h softmmu_template.h cpu.h
+op_helper.o: exec.h softmmu_template.h cpu.h
 translate.o: translate_init.c exec-all.h disas.h
 endif

Index: target-mips/exec.h
===================================================================
RCS file: /sources/qemu/qemu/target-mips/exec.h,v
retrieving revision 1.33

exec.h +0 -30
diff -u -d -p -r1.33 exec.h
@@ -100,36 +100,6 @@ void fpu_dump_state(CPUState *env, FILE 
                     int (*fpu_fprintf)(FILE *f, const char *fmt, ...),
                     int flags);
 void dump_sc (void);
-void do_lwl_raw (uint32_t);
-void do_lwr_raw (uint32_t);
-uint32_t do_swl_raw (uint32_t);
-uint32_t do_swr_raw (uint32_t);
-#ifdef TARGET_MIPS64
-void do_ldl_raw (uint64_t);
-void do_ldr_raw (uint64_t);
-uint64_t do_sdl_raw (uint64_t);
-uint64_t do_sdr_raw (uint64_t);
-#endif
-#if !defined(CONFIG_USER_ONLY)
-void do_lwl_user (uint32_t);
-void do_lwl_kernel (uint32_t);
-void do_lwr_user (uint32_t);
-void do_lwr_kernel (uint32_t);
-uint32_t do_swl_user (uint32_t);
-uint32_t do_swl_kernel (uint32_t);
-uint32_t do_swr_user (uint32_t);
-uint32_t do_swr_kernel (uint32_t);
-#ifdef TARGET_MIPS64
-void do_ldl_user (uint64_t);
-void do_ldl_kernel (uint64_t);
-void do_ldr_user (uint64_t);
-void do_ldr_kernel (uint64_t);
-uint64_t do_sdl_user (uint64_t);
-uint64_t do_sdl_kernel (uint64_t);
-uint64_t do_sdr_user (uint64_t);
-uint64_t do_sdr_kernel (uint64_t);
-#endif
-#endif
 void do_pmon (int function);
 
 void dump_sc (void);

Index: target-mips/op_helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/op_helper.c,v retrieving revision 1.60

op_helper.c +0 -12
diff -u -d -p -r1.60 op_helper.c
@@ -62,18 +62,6 @@ void do_raise_exception_direct (uint32_t
     do_raise_exception_direct_err (exception, 0);
 }
 
-#define MEMSUFFIX _raw
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#if !defined(CONFIG_USER_ONLY)
-#define MEMSUFFIX _user
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#define MEMSUFFIX _kernel
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#endif
-
 #ifdef TARGET_MIPS64
 #if TARGET_LONG_BITS > HOST_LONG_BITS
 /* Those might call libgcc functions.  */

Index: target-mips/op_helper_mem.c
===================================================================
RCS file: target-mips/op_helper_mem.c

op_helper_mem.c deleted +0 -301

301 lines not on this page · raw message

Index: target-mips/op_mem.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/op_mem.c,v retrieving revision 1.12

op_mem.c +202 -21

269 lines not on this page · raw message

Index: target-mips/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/translate.c,v retrieving revision 1.102

translate.c +4 -4
diff -u -d -p -r1.102 translate.c
@@ -889,7 +889,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LDL:
         GEN_LOAD_REG_TN(T1, rt);
         op_ldst(ldl);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "ldl";
         break;
     case OPC_SDL:

@@ -900,7 +900,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LDR:
         GEN_LOAD_REG_TN(T1, rt);
         op_ldst(ldr);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "ldr";
         break;
     case OPC_SDR:

@@ -952,7 +952,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LWL:
 	GEN_LOAD_REG_TN(T1, rt);
         op_ldst(lwl);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "lwl";
         break;
     case OPC_SWL:

@@ -963,7 +963,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LWR:
 	GEN_LOAD_REG_TN(T1, rt);
         op_ldst(lwr);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "lwr";
         break;
     case OPC_SWR:
Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net
#1 lore raw
Quoted 6 lines · Aurelien Jarno

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr,
sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

Quoted 8 lines

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.

Bye,
Aurelien

[...]

BTW, you could simplify a lot the implementation :

    switch (GET_LMASK(T0)) {
    case 0:
        T0 = (int32_t)tmp;
        break;
    case 1:
        T0 = (int32_t)((tmp << 8) | (T1 & 0x000000FF));
        break;
    case 2:
        T0 = (int32_t)((tmp << 16) | (T1 & 0x0000FFFF));
        break;
    case 3:
        T0 = (int32_t)((tmp << 24) | (T1 & 0x00FFFFFF));
        break;
    }

->

v = GET_LMASK(T0);

if (v == 0) {
	T0 = tmp;
} else {
	TO = (int32_t)((tmp << (8 * v)) | (T1 & ((1 << (v * 8)) - 1)));
}

Fabrice.

author
#2 lore raw
Quoted 37 lines · Fabrice Bellard
Quoted 11 lines · Aurelien Jarno

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr, sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

Quoted 5 lines

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.

Bye,
Aurelien

[...]

BTW, you could simplify a lot the implementation :

     switch (GET_LMASK(T0)) {
     case 0:
         T0 = (int32_t)tmp;
         break;
     case 1:
         T0 = (int32_t)((tmp << 8) | (T1 & 0x000000FF));
         break;
     case 2:
         T0 = (int32_t)((tmp << 16) | (T1 & 0x0000FFFF));
         break;
     case 3:
         T0 = (int32_t)((tmp << 24) | (T1 & 0x00FFFFFF));
         break;
     }
Quoted 9 lines

->

v = GET_LMASK(T0);

if (v == 0) {
	T0 = tmp;
} else {
	TO = (int32_t)((tmp << (8 * v)) | (T1 & ((1 << (v * 8)) - 1)));
}

Agreed, but this code is actually removed and not added by the patch I posted.

Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net
#3 lore raw
In reply to Aurelien Jarno
Quoted 6 lines · Aurelien Jarno

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr,
sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

Quoted 6 lines

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.
[...]

It would be a lot more efficient to add specific code in the MIPS exception handling.

Fabrice.

author
#4 lore raw
Quoted 12 lines · Fabrice Bellard
Quoted 11 lines · Aurelien Jarno

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr, sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

Quoted 7 lines
Quoted 3 lines

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.
[...]

It would be a lot more efficient to add specific code in the MIPS exception handling.

That was the first approach I tried to follow, but it is not that easy. Basically the solution is to add a new flag to CPUMIPSState to mark instructions that need fix when an exception occurs and then fix the exception type and address if needed.

Given those instructions are not used that often (unaligned memory accesses have to be avoided), I am not sure it is worth adding too much complexity in the mmu fault and exception handling code, which is used by all instructions.

Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net
author
#5 lore raw
In reply to Aurelien Jarno
Quoted 7 lines · Aurelien Jarno

Hi,

As written in the MIPS TODO file, the lwl, lwr, ldl, ldr, swl, swr, sdl and sdr instructions are not correctly implemented. In case of exception the BadVAddr register gets the aligned address instead of the unaligned original address.

Quoted 7 lines

In addition to that, the store instructions are generating the wrong exception, AdEl instead of AdEs, because the current implementation first do a load.

The patch below fixes that by accessing the bytes one by one, starting by the unaligned original address.

The patch doesn't apply anymore to the current CVS. Please find below an updated version.

Index: Makefile.target
===================================================================
RCS file: /sources/qemu/qemu/Makefile.target,v
retrieving revision 1.204

Makefile.target +1 -1
diff -u -d -p -r1.204 Makefile.target
@@ -641,7 +641,7 @@ endif
 ifeq ($(TARGET_BASE_ARCH), mips)
 helper.o: cpu.h exec-all.h
 op.o: op_template.c fop_template.c op_mem.c exec.h cpu.h
-op_helper.o: op_helper_mem.c exec.h softmmu_template.h cpu.h
+op_helper.o: exec.h softmmu_template.h cpu.h
 translate.o: translate_init.c exec-all.h disas.h
 endif

Index: target-mips/exec.h
===================================================================
RCS file: /sources/qemu/qemu/target-mips/exec.h,v
retrieving revision 1.36

exec.h +0 -30
diff -u -d -p -r1.36 exec.h
@@ -100,36 +100,6 @@ void fpu_dump_state(CPUState *env, FILE 
                     int (*fpu_fprintf)(FILE *f, const char *fmt, ...),
                     int flags);
 void dump_sc (void);
-void do_lwl_raw (uint32_t);
-void do_lwr_raw (uint32_t);
-uint32_t do_swl_raw (uint32_t);
-uint32_t do_swr_raw (uint32_t);
-#if defined(TARGET_MIPSN32) || defined(TARGET_MIPS64)
-void do_ldl_raw (uint64_t);
-void do_ldr_raw (uint64_t);
-uint64_t do_sdl_raw (uint64_t);
-uint64_t do_sdr_raw (uint64_t);
-#endif
-#if !defined(CONFIG_USER_ONLY)
-void do_lwl_user (uint32_t);
-void do_lwl_kernel (uint32_t);
-void do_lwr_user (uint32_t);
-void do_lwr_kernel (uint32_t);
-uint32_t do_swl_user (uint32_t);
-uint32_t do_swl_kernel (uint32_t);
-uint32_t do_swr_user (uint32_t);
-uint32_t do_swr_kernel (uint32_t);
-#if defined(TARGET_MIPSN32) || defined(TARGET_MIPS64)
-void do_ldl_user (uint64_t);
-void do_ldl_kernel (uint64_t);
-void do_ldr_user (uint64_t);
-void do_ldr_kernel (uint64_t);
-uint64_t do_sdl_user (uint64_t);
-uint64_t do_sdl_kernel (uint64_t);
-uint64_t do_sdr_user (uint64_t);
-uint64_t do_sdr_kernel (uint64_t);
-#endif
-#endif
 void do_pmon (int function);
 
 void dump_sc (void);

Index: target-mips/op_helper.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/op_helper.c,v retrieving revision 1.63

op_helper.c +0 -12
diff -u -d -p -r1.63 op_helper.c
@@ -62,18 +62,6 @@ void do_raise_exception_direct (uint32_t
     do_raise_exception_direct_err (exception, 0);
 }
 
-#define MEMSUFFIX _raw
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#if !defined(CONFIG_USER_ONLY)
-#define MEMSUFFIX _user
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#define MEMSUFFIX _kernel
-#include "op_helper_mem.c"
-#undef MEMSUFFIX
-#endif
-
 #if defined(TARGET_MIPSN32) || defined(TARGET_MIPS64)
 #if TARGET_LONG_BITS > HOST_LONG_BITS
 /* Those might call libgcc functions.  */

Index: target-mips/op_helper_mem.c
===================================================================
RCS file: target-mips/op_helper_mem.c

op_helper_mem.c deleted +0 -301

301 lines not on this page · raw message

Index: target-mips/op_mem.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/op_mem.c,v retrieving revision 1.13

op_mem.c +202 -21

269 lines not on this page · raw message

Index: target-mips/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/translate.c,v retrieving revision 1.104

translate.c +4 -4
diff -u -d -p -r1.104 translate.c
@@ -889,7 +889,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LDL:
         GEN_LOAD_REG_TN(T1, rt);
         op_ldst(ldl);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "ldl";
         break;
     case OPC_SDL:

@@ -900,7 +900,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LDR:
         GEN_LOAD_REG_TN(T1, rt);
         op_ldst(ldr);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "ldr";
         break;
     case OPC_SDR:

@@ -952,7 +952,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LWL:
 	GEN_LOAD_REG_TN(T1, rt);
         op_ldst(lwl);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "lwl";
         break;
     case OPC_SWL:

@@ -963,7 +963,7 @@ static void gen_ldst (DisasContext *ctx,
     case OPC_LWR:
 	GEN_LOAD_REG_TN(T1, rt);
         op_ldst(lwr);
-        GEN_STORE_TN_REG(rt, T0);
+        GEN_STORE_TN_REG(rt, T1);
         opn = "lwr";
         break;
     case OPC_SWR:
Signature · 4 lines
  .''`.  Aurelien Jarno	            | GPG: 1024D/F1BCDB73
 : :' :  Debian developer           | Electrical Engineer
 `. `'   [email protected]         | [email protected]
   `-    people.debian.org/~aurel32 | www.aurel32.net