[Qemu-devel] [PATCH 0/8] sparc64 fixes

[Qemu-devel] [PATCH 0/8] sparc64 fixes

[Igor V. Kovalenko]

assorted changes, linux kernel can now work with 32bit init task

---

Igor V. Kovalenko (8):
      sparc64: fix tag access register on mmu traps
      sparc64: fix missing address masking
      sparc64: fix 32bit load sign extension
      sparc64: fix ldxfsr insn
      sparc64: use symbolic name for MMU index
      sparc64: improve ldf and stf insns
      sparc64: fix udiv and sdiv insns
      sparc64: fix umul and smul insns


 softmmu_header.h         |    2 -
 target-sparc/helper.c    |    5 ++
 target-sparc/op_helper.c |  107 ++++++++++++++++++++++++++++++++++++++++------
 target-sparc/translate.c |   65 ++++++++++++++++------------
 4 files changed, 137 insertions(+), 42 deletions(-)

-- 

[Qemu-devel] [PATCH 1/8] sparc64: fix tag access register on mmu traps

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- set mmu tag access register on FAULT and PROT traps as well

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/helper.c |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/target-sparc/helper.c b/target-sparc/helper.c
index 96a22f3..aa1fd63 100644
--- a/target-sparc/helper.c
+++ b/target-sparc/helper.c
@@ -495,6 +495,9 @@ static int get_physical_address_data(CPUState *env,
             env->dmmu.sfsr |= (fault_type << 7);
 
             env->dmmu.sfar = address; /* Fault address register */
+
+            env->dmmu.tag_access = (address & ~0x1fffULL) | context;
+
             return 1;
         }
     }
@@ -544,6 +547,8 @@ static int get_physical_address_code(CPUState *env,
                 env->immu.sfsr |= (is_user << 3) | 1;
                 env->exception_index = TT_TFAULT;
 
+                env->immu.tag_access = (address & ~0x1fffULL) | context;
+
                 DPRINTF_MMU("TFAULT at %" PRIx64 " context %" PRIx64 "\n",
                             address, context);
 

[Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- address masking for ldqf and stqf insns
- address masking for lddf and stdf insns
- address masking for translating ASI (Ultrasparc IIi)

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/op_helper.c |   47 ++++++++++++++++++++++++++++++++++++++++++++++
 target-sparc/translate.c |    4 ++++
 2 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index ef3504f..f5e153d 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -2315,6 +2315,25 @@ void helper_st_asi(target_ulong addr, target_ulong val, int asi, int size)
 
 #else /* CONFIG_USER_ONLY */
 
+/* Ultrasparc IIi translating asi
+   - note this list is defined by cpu implementation
+ */
+static inline int is_translating_asi(int asi)
+{
+    switch (asi) {
+    case 0x04 ... 0x11:
+    case 0x18 ... 0x19:
+    case 0x24 ... 0x2C:
+    case 0x70 ... 0x73:
+    case 0x78 ... 0x79:
+    case 0x80 ... 0xFF:
+        return 1;
+
+    default:
+        return 0;
+    }
+}
+
 uint64_t helper_ld_asi(target_ulong addr, int asi, int size, int sign)
 {
     uint64_t ret = 0;
@@ -2330,7 +2349,12 @@ uint64_t helper_ld_asi(target_ulong addr, int asi, int size, int sign)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+
     helper_check_align(addr, size - 1);
+
     switch (asi) {
     case 0x82: // Primary no-fault
     case 0x8a: // Primary no-fault LE
@@ -2681,7 +2705,12 @@ void helper_st_asi(target_ulong addr, target_ulong val, int asi, int size)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+
     helper_check_align(addr, size - 1);
+
     /* Convert to little endian */
     switch (asi) {
     case 0x0c: // Nucleus Little Endian (LE)
@@ -3056,6 +3085,12 @@ void helper_ldda_asi(target_ulong addr, int asi, int rd)
             && !(env->hpstate & HS_PRIV)))
         raise_exception(TT_PRIV_ACT);
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     switch (asi) {
 #if !defined(CONFIG_USER_ONLY)
     case 0x24: // Nucleus quad LDD 128 bit atomic
@@ -3102,6 +3137,12 @@ void helper_ldf_asi(target_ulong addr, int asi, int size, int rd)
     unsigned int i;
     target_ulong val;
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     helper_check_align(addr, 3);
     switch (asi) {
     case 0xf0: // Block load primary
@@ -3144,6 +3185,12 @@ void helper_stf_asi(target_ulong addr, int asi, int size, int rd)
     unsigned int i;
     target_ulong val = 0;
 
+#if defined (CONFIG_SPARC64)
+    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
+        addr &= 0xffffffffULL;
+    }
+#endif
+
     helper_check_align(addr, 3);
     switch (asi) {
     case 0xe0: // UA2007 Block commit store primary (cache flush)
diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 72ca0b4..eff64d4 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4490,6 +4490,7 @@ static void disas_sparc_insn(DisasContext * dc)
 
                         CHECK_FPU_FEATURE(dc, FLOAT128);
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_ldqf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                         gen_op_store_QT0_fpr(QFPREG(rd));
@@ -4500,6 +4501,7 @@ static void disas_sparc_insn(DisasContext * dc)
                         TCGv_i32 r_const;
 
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_lddf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                         gen_op_store_DT0_fpr(DFPREG(rd));
@@ -4635,6 +4637,7 @@ static void disas_sparc_insn(DisasContext * dc)
                         CHECK_FPU_FEATURE(dc, FLOAT128);
                         gen_op_load_fpr_QT0(QFPREG(rd));
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_stqf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                     }
@@ -4657,6 +4660,7 @@ static void disas_sparc_insn(DisasContext * dc)
 
                         gen_op_load_fpr_DT0(DFPREG(rd));
                         r_const = tcg_const_i32(dc->mem_idx);
+                        gen_address_mask(dc, cpu_addr);
                         gen_helper_stdf(cpu_addr, r_const);
                         tcg_temp_free_i32(r_const);
                     }

[Qemu-devel] [PATCH 3/8] sparc64: fix 32bit load sign extension

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- change return type of ldl_* to uint32_t to prevent unwanted sign extension
  visible in sparc64 load alternate address space methods
- note this change makes ldl_* softmmu implementations match ldl_phys one
Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 softmmu_header.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/softmmu_header.h b/softmmu_header.h
index 6a36e01..2f95c33 100644
--- a/softmmu_header.h
+++ b/softmmu_header.h
@@ -60,7 +60,7 @@
 #if DATA_SIZE == 8
 #define RES_TYPE uint64_t
 #else
-#define RES_TYPE int
+#define RES_TYPE uint32_t
 #endif
 
 #if ACCESS_TYPE == (NB_MMU_MODES + 1)

[Qemu-devel] [PATCH 4/8] sparc64: fix ldxfsr insn

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- rearrange code to break from switch when appropriate
- allow deprecated ldfsr insn

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/translate.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index eff64d4..0bc1a82 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -4476,7 +4476,11 @@ static void disas_sparc_insn(DisasContext * dc)
                     if (rd == 1) {
                         tcg_gen_qemu_ld64(cpu_tmp64, cpu_addr, dc->mem_idx);
                         gen_helper_ldxfsr(cpu_tmp64);
-                    } else
+                    } else {
+                        tcg_gen_qemu_ld32u(cpu_tmp0, cpu_addr, dc->mem_idx);
+                        tcg_gen_trunc_tl_i32(cpu_tmp32, cpu_tmp0);
+                        gen_helper_ldfsr(cpu_tmp32);
+                    }
 #else
                     {
                         tcg_gen_qemu_ld32u(cpu_tmp32, cpu_addr, dc->mem_idx);

[Qemu-devel] [PATCH 5/8] sparc64: use symbolic name for MMU index

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/op_helper.c |   28 ++++++++++++++++------------
 1 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index f5e153d..b9af52b 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -3322,18 +3322,19 @@ void helper_stdf(target_ulong addr, int mem_idx)
     helper_check_align(addr, 7);
 #if !defined(CONFIG_USER_ONLY)
     switch (mem_idx) {
-    case 0:
+    case MMU_USER_IDX:
         stfq_user(addr, DT0);
         break;
-    case 1:
+    case MMU_KERNEL_IDX:
         stfq_kernel(addr, DT0);
         break;
 #ifdef TARGET_SPARC64
-    case 2:
+    case MMU_HYPV_IDX:
         stfq_hypv(addr, DT0);
         break;
 #endif
     default:
+        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
         break;
     }
 #else
@@ -3346,18 +3347,19 @@ void helper_lddf(target_ulong addr, int mem_idx)
     helper_check_align(addr, 7);
 #if !defined(CONFIG_USER_ONLY)
     switch (mem_idx) {
-    case 0:
+    case MMU_USER_IDX:
         DT0 = ldfq_user(addr);
         break;
-    case 1:
+    case MMU_KERNEL_IDX:
         DT0 = ldfq_kernel(addr);
         break;
 #ifdef TARGET_SPARC64
-    case 2:
+    case MMU_HYPV_IDX:
         DT0 = ldfq_hypv(addr);
         break;
 #endif
     default:
+        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
         break;
     }
 #else
@@ -3373,24 +3375,25 @@ void helper_ldqf(target_ulong addr, int mem_idx)
     helper_check_align(addr, 7);
 #if !defined(CONFIG_USER_ONLY)
     switch (mem_idx) {
-    case 0:
+    case MMU_USER_IDX:
         u.ll.upper = ldq_user(addr);
         u.ll.lower = ldq_user(addr + 8);
         QT0 = u.q;
         break;
-    case 1:
+    case MMU_KERNEL_IDX:
         u.ll.upper = ldq_kernel(addr);
         u.ll.lower = ldq_kernel(addr + 8);
         QT0 = u.q;
         break;
 #ifdef TARGET_SPARC64
-    case 2:
+    case MMU_HYPV_IDX:
         u.ll.upper = ldq_hypv(addr);
         u.ll.lower = ldq_hypv(addr + 8);
         QT0 = u.q;
         break;
 #endif
     default:
+        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
         break;
     }
 #else
@@ -3408,24 +3411,25 @@ void helper_stqf(target_ulong addr, int mem_idx)
     helper_check_align(addr, 7);
 #if !defined(CONFIG_USER_ONLY)
     switch (mem_idx) {
-    case 0:
+    case MMU_USER_IDX:
         u.q = QT0;
         stq_user(addr, u.ll.upper);
         stq_user(addr + 8, u.ll.lower);
         break;
-    case 1:
+    case MMU_KERNEL_IDX:
         u.q = QT0;
         stq_kernel(addr, u.ll.upper);
         stq_kernel(addr + 8, u.ll.lower);
         break;
 #ifdef TARGET_SPARC64
-    case 2:
+    case MMU_HYPV_IDX:
         u.q = QT0;
         stq_hypv(addr, u.ll.upper);
         stq_hypv(addr + 8, u.ll.lower);
         break;
 #endif
     default:
+        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
         break;
     }
 #else

[Qemu-devel] [PATCH 6/8] sparc64: improve ldf and stf insns

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- implemented block load/store primary/secondary with user privilege

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/op_helper.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index b9af52b..83067ae 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -3161,6 +3161,20 @@ void helper_ldf_asi(target_ulong addr, int asi, int size, int rd)
         }
 
         return;
+    case 0x70: // Block load primary, user privilege
+    case 0x71: // Block load secondary, user privilege
+        if (rd & 7) {
+            raise_exception(TT_ILL_INSN);
+            return;
+        }
+        helper_check_align(addr, 0x3f);
+        for (i = 0; i < 16; i++) {
+            *(uint32_t *)&env->fpr[rd++] = helper_ld_asi(addr, asi & 0x1f, 4,
+                                                         0);
+            addr += 4;
+        }
+
+        return;
     default:
         break;
     }
@@ -3211,6 +3225,20 @@ void helper_stf_asi(target_ulong addr, int asi, int size, int rd)
         }
 
         return;
+    case 0x70: // Block store primary, user privilege
+    case 0x71: // Block store secondary, user privilege
+        if (rd & 7) {
+            raise_exception(TT_ILL_INSN);
+            return;
+        }
+        helper_check_align(addr, 0x3f);
+        for (i = 0; i < 16; i++) {
+            val = *(uint32_t *)&env->fpr[rd++];
+            helper_st_asi(addr, val, asi & 0x1f, 4);
+            addr += 4;
+        }
+
+        return;
     default:
         break;
     }

[Qemu-devel] [PATCH 7/8] sparc64: fix udiv and sdiv insns

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- truncate second operand to 32bit

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/op_helper.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
index 83067ae..4c5155f 100644
--- a/target-sparc/op_helper.c
+++ b/target-sparc/op_helper.c
@@ -3307,7 +3307,7 @@ target_ulong helper_udiv(target_ulong a, target_ulong b)
     uint32_t x1;
 
     x0 = (a & 0xffffffff) | ((int64_t) (env->y) << 32);
-    x1 = b;
+    x1 = (b & 0xffffffff);
 
     if (x1 == 0) {
         raise_exception(TT_DIV_ZERO);
@@ -3329,7 +3329,7 @@ target_ulong helper_sdiv(target_ulong a, target_ulong b)
     int32_t x1;
 
     x0 = (a & 0xffffffff) | ((int64_t) (env->y) << 32);
-    x1 = b;
+    x1 = (b & 0xffffffff);
 
     if (x1 == 0) {
         raise_exception(TT_DIV_ZERO);

[Qemu-devel] [PATCH 8/8] sparc64: fix umul and smul insns

[Igor V. Kovalenko]

From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>

- truncate and sign or zero extend operands before multiplication
- factor out common code to gen_op_multiply() with parameter to sign/zero extend
- call gen_op_multiply from gen_op_umul and gen_op_smul

Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
---
 target-sparc/translate.c |   55 ++++++++++++++++++++++++----------------------
 1 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/target-sparc/translate.c b/target-sparc/translate.c
index 0bc1a82..23f9519 100644
--- a/target-sparc/translate.c
+++ b/target-sparc/translate.c
@@ -662,50 +662,53 @@ static inline void gen_op_mulscc(TCGv dst, TCGv src1, TCGv src2)
     tcg_gen_mov_tl(dst, cpu_cc_dst);
 }
 
-static inline void gen_op_umul(TCGv dst, TCGv src1, TCGv src2)
+static inline void gen_op_multiply(TCGv dst, TCGv src1, TCGv src2, int sign_ext)
 {
+    TCGv_i32 r_src1, r_src2;
     TCGv_i64 r_temp, r_temp2;
 
+    r_src1 = tcg_temp_new_i32();
+    r_src2 = tcg_temp_new_i32();
+
+    tcg_gen_trunc_tl_i32(r_src1, src1);
+    tcg_gen_trunc_tl_i32(r_src2, src2);
+
     r_temp = tcg_temp_new_i64();
     r_temp2 = tcg_temp_new_i64();
 
-    tcg_gen_extu_tl_i64(r_temp, src2);
-    tcg_gen_extu_tl_i64(r_temp2, src1);
+    if (sign_ext) {
+        tcg_gen_ext_i32_i64(r_temp, r_src2);
+        tcg_gen_ext_i32_i64(r_temp2, r_src1);
+    } else {
+        tcg_gen_extu_i32_i64(r_temp, r_src2);
+        tcg_gen_extu_i32_i64(r_temp2, r_src1);
+    }
+
     tcg_gen_mul_i64(r_temp2, r_temp, r_temp2);
 
     tcg_gen_shri_i64(r_temp, r_temp2, 32);
     tcg_gen_trunc_i64_tl(cpu_tmp0, r_temp);
     tcg_temp_free_i64(r_temp);
     tcg_gen_andi_tl(cpu_y, cpu_tmp0, 0xffffffff);
-#ifdef TARGET_SPARC64
-    tcg_gen_mov_i64(dst, r_temp2);
-#else
+
     tcg_gen_trunc_i64_tl(dst, r_temp2);
-#endif
+
     tcg_temp_free_i64(r_temp2);
+
+    tcg_temp_free_i32(r_src1);
+    tcg_temp_free_i32(r_src2);
 }
 
-static inline void gen_op_smul(TCGv dst, TCGv src1, TCGv src2)
+static inline void gen_op_umul(TCGv dst, TCGv src1, TCGv src2)
 {
-    TCGv_i64 r_temp, r_temp2;
-
-    r_temp = tcg_temp_new_i64();
-    r_temp2 = tcg_temp_new_i64();
-
-    tcg_gen_ext_tl_i64(r_temp, src2);
-    tcg_gen_ext_tl_i64(r_temp2, src1);
-    tcg_gen_mul_i64(r_temp2, r_temp, r_temp2);
+    /* zero-extend truncated operands before multiplication */
+    gen_op_multiply(dst, src1, src2, 0);
+}
 
-    tcg_gen_shri_i64(r_temp, r_temp2, 32);
-    tcg_gen_trunc_i64_tl(cpu_tmp0, r_temp);
-    tcg_temp_free_i64(r_temp);
-    tcg_gen_andi_tl(cpu_y, cpu_tmp0, 0xffffffff);
-#ifdef TARGET_SPARC64
-    tcg_gen_mov_i64(dst, r_temp2);
-#else
-    tcg_gen_trunc_i64_tl(dst, r_temp2);
-#endif
-    tcg_temp_free_i64(r_temp2);
+static inline void gen_op_smul(TCGv dst, TCGv src1, TCGv src2)
+{
+    /* sign-extend truncated operands before multiplication */
+    gen_op_multiply(dst, src1, src2, 1);
 }
 
 #ifdef TARGET_SPARC64

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Richard Henderson]

On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
> +        addr &= 0xffffffffULL;
> +    }

I suggest that these be written instead as

  if (is_translating_asi(asi)) {
    addr = address_mask(addr);
  }

That should allow you to remove some of the ifdefs.


r~

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Igor Kovalenko]

On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>> +        addr &= 0xffffffffULL;
>> +    }
>
> I suggest that these be written instead as
>
>  if (is_translating_asi(asi)) {
>    addr = address_mask(addr);
>  }
>
> That should allow you to remove some of the ifdefs.

All address masking is done for sparc64 target only, sparc32 does not
have the notion of translating asi.
I think it's better to do debug printf macro trick then but I see no
real benefit at the moment.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Richard Henderson]

On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>> +        addr &= 0xffffffffULL;
>>> +    }
>>
>> I suggest that these be written instead as
>>
>>  if (is_translating_asi(asi)) {
>>    addr = address_mask(addr);
>>  }
>>
>> That should allow you to remove some of the ifdefs.
> 
> All address masking is done for sparc64 target only, sparc32 does not
> have the notion of translating asi.

Of course I know that.

> I think it's better to do debug printf macro trick ...

... with no evidence.  The compiler is happy to optimize away
the entire if statement without having to resort to macros.

> ... then but I see no real benefit at the moment.

Avoiding ifdefs isn't a benefit?


r~

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Blue Swirl]

On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>> +        addr &= 0xffffffffULL;
>>>> +    }
>>>
>>> I suggest that these be written instead as
>>>
>>>  if (is_translating_asi(asi)) {
>>>    addr = address_mask(addr);
>>>  }
>>>
>>> That should allow you to remove some of the ifdefs.
>>
>> All address masking is done for sparc64 target only, sparc32 does not
>> have the notion of translating asi.
>
> Of course I know that.
>
>> I think it's better to do debug printf macro trick ...
>
> ... with no evidence.  The compiler is happy to optimize away
> the entire if statement without having to resort to macros.
>
>> ... then but I see no real benefit at the moment.
>
> Avoiding ifdefs isn't a benefit?

I agree macros would make the code more tidy, perhaps it could swallow
both the check and the masking. The macro can be empty for Sparc32.

Re: [Qemu-devel] [PATCH 5/8] sparc64: use symbolic name for MMU index

[Blue Swirl]

On Tue, Jun 1, 2010 at 8:12 PM, Igor V. Kovalenko
<igor.v.kovalenko@gmail.com> wrote:
> From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>
> Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
> ---
>  target-sparc/op_helper.c |   28 ++++++++++++++++------------
>  1 files changed, 16 insertions(+), 12 deletions(-)
>
> diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
> index f5e153d..b9af52b 100644
> --- a/target-sparc/op_helper.c
> +++ b/target-sparc/op_helper.c
> @@ -3322,18 +3322,19 @@ void helper_stdf(target_ulong addr, int mem_idx)
>     helper_check_align(addr, 7);
>  #if !defined(CONFIG_USER_ONLY)
>     switch (mem_idx) {
> -    case 0:
> +    case MMU_USER_IDX:
>         stfq_user(addr, DT0);
>         break;
> -    case 1:
> +    case MMU_KERNEL_IDX:
>         stfq_kernel(addr, DT0);
>         break;
>  #ifdef TARGET_SPARC64
> -    case 2:
> +    case MMU_HYPV_IDX:
>         stfq_hypv(addr, DT0);
>         break;
>  #endif
>     default:
> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);

Are these going to be useful or just leftover debugging?

>         break;
>     }
>  #else
> @@ -3346,18 +3347,19 @@ void helper_lddf(target_ulong addr, int mem_idx)
>     helper_check_align(addr, 7);
>  #if !defined(CONFIG_USER_ONLY)
>     switch (mem_idx) {
> -    case 0:
> +    case MMU_USER_IDX:
>         DT0 = ldfq_user(addr);
>         break;
> -    case 1:
> +    case MMU_KERNEL_IDX:
>         DT0 = ldfq_kernel(addr);
>         break;
>  #ifdef TARGET_SPARC64
> -    case 2:
> +    case MMU_HYPV_IDX:
>         DT0 = ldfq_hypv(addr);
>         break;
>  #endif
>     default:
> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);

The function name is not correct for this and other cases below.

>         break;
>     }
>  #else
> @@ -3373,24 +3375,25 @@ void helper_ldqf(target_ulong addr, int mem_idx)
>     helper_check_align(addr, 7);
>  #if !defined(CONFIG_USER_ONLY)
>     switch (mem_idx) {
> -    case 0:
> +    case MMU_USER_IDX:
>         u.ll.upper = ldq_user(addr);
>         u.ll.lower = ldq_user(addr + 8);
>         QT0 = u.q;
>         break;
> -    case 1:
> +    case MMU_KERNEL_IDX:
>         u.ll.upper = ldq_kernel(addr);
>         u.ll.lower = ldq_kernel(addr + 8);
>         QT0 = u.q;
>         break;
>  #ifdef TARGET_SPARC64
> -    case 2:
> +    case MMU_HYPV_IDX:
>         u.ll.upper = ldq_hypv(addr);
>         u.ll.lower = ldq_hypv(addr + 8);
>         QT0 = u.q;
>         break;
>  #endif
>     default:
> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>         break;
>     }
>  #else
> @@ -3408,24 +3411,25 @@ void helper_stqf(target_ulong addr, int mem_idx)
>     helper_check_align(addr, 7);
>  #if !defined(CONFIG_USER_ONLY)
>     switch (mem_idx) {
> -    case 0:
> +    case MMU_USER_IDX:
>         u.q = QT0;
>         stq_user(addr, u.ll.upper);
>         stq_user(addr + 8, u.ll.lower);
>         break;
> -    case 1:
> +    case MMU_KERNEL_IDX:
>         u.q = QT0;
>         stq_kernel(addr, u.ll.upper);
>         stq_kernel(addr + 8, u.ll.lower);
>         break;
>  #ifdef TARGET_SPARC64
> -    case 2:
> +    case MMU_HYPV_IDX:
>         u.q = QT0;
>         stq_hypv(addr, u.ll.upper);
>         stq_hypv(addr + 8, u.ll.lower);
>         break;
>  #endif
>     default:
> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>         break;
>     }
>  #else
>
>
>

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Andreas Färber]

Am 02.06.2010 um 18:10 schrieb Blue Swirl:

> On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net>  
> wrote:
>> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson  
>>> <rth@twiddle.net> wrote:
>>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>>> +        addr &= 0xffffffffULL;
>>>>> +    }
>>>>
>>>> I suggest that these be written instead as
>>>>
>>>>  if (is_translating_asi(asi)) {
>>>>    addr = address_mask(addr);
>>>>  }
>>>>
>>>> That should allow you to remove some of the ifdefs.
>>
>>> I think it's better to do debug printf macro trick ...
>>
>> ... with no evidence.  The compiler is happy to optimize away
>> the entire if statement without having to resort to macros.
>>
>>> ... then but I see no real benefit at the moment.
>>
>> Avoiding ifdefs isn't a benefit?
>
> I agree macros would make the code more tidy, perhaps it could swallow
> both the check and the masking. The macro can be empty for Sparc32.

I usually prefer static inline functions over multi-line macros.  
Probably a matter of taste.

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Igor Kovalenko]

On Wed, Jun 2, 2010 at 8:46 PM, Andreas Färber <andreas.faerber@web.de> wrote:
> Am 02.06.2010 um 18:10 schrieb Blue Swirl:
>
>> On Wed, Jun 2, 2010 at 1:47 PM, Richard Henderson <rth@twiddle.net> wrote:
>>>
>>> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>>>>
>>>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net>
>>>> wrote:
>>>>>
>>>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>>>>
>>>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>>>> +        addr &= 0xffffffffULL;
>>>>>> +    }
>>>>>
>>>>> I suggest that these be written instead as
>>>>>
>>>>>  if (is_translating_asi(asi)) {
>>>>>   addr = address_mask(addr);
>>>>>  }
>>>>>
>>>>> That should allow you to remove some of the ifdefs.
>>>
>>>> I think it's better to do debug printf macro trick ...
>>>
>>> ... with no evidence.  The compiler is happy to optimize away
>>> the entire if statement without having to resort to macros.
>>>
>>>> ... then but I see no real benefit at the moment.
>>>
>>> Avoiding ifdefs isn't a benefit?
>>
>> I agree macros would make the code more tidy, perhaps it could swallow
>> both the check and the masking. The macro can be empty for Sparc32.
>
> I usually prefer static inline functions over multi-line macros. Probably a
> matter of taste.

I'll resend this one updated to have less ifdefs.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH 5/8] sparc64: use symbolic name for MMU index

[Igor Kovalenko]

On Wed, Jun 2, 2010 at 8:16 PM, Blue Swirl <blauwirbel@gmail.com> wrote:
> On Tue, Jun 1, 2010 at 8:12 PM, Igor V. Kovalenko
> <igor.v.kovalenko@gmail.com> wrote:
>> From: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>>
>> Signed-off-by: Igor V. Kovalenko <igor.v.kovalenko@gmail.com>
>> ---
>>  target-sparc/op_helper.c |   28 ++++++++++++++++------------
>>  1 files changed, 16 insertions(+), 12 deletions(-)
>>
>> diff --git a/target-sparc/op_helper.c b/target-sparc/op_helper.c
>> index f5e153d..b9af52b 100644
>> --- a/target-sparc/op_helper.c
>> +++ b/target-sparc/op_helper.c
>> @@ -3322,18 +3322,19 @@ void helper_stdf(target_ulong addr, int mem_idx)
>>     helper_check_align(addr, 7);
>>  #if !defined(CONFIG_USER_ONLY)
>>     switch (mem_idx) {
>> -    case 0:
>> +    case MMU_USER_IDX:
>>         stfq_user(addr, DT0);
>>         break;
>> -    case 1:
>> +    case MMU_KERNEL_IDX:
>>         stfq_kernel(addr, DT0);
>>         break;
>>  #ifdef TARGET_SPARC64
>> -    case 2:
>> +    case MMU_HYPV_IDX:
>>         stfq_hypv(addr, DT0);
>>         break;
>>  #endif
>>     default:
>> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>
> Are these going to be useful or just leftover debugging?
>
>>         break;
>>     }
>>  #else
>> @@ -3346,18 +3347,19 @@ void helper_lddf(target_ulong addr, int mem_idx)
>>     helper_check_align(addr, 7);
>>  #if !defined(CONFIG_USER_ONLY)
>>     switch (mem_idx) {
>> -    case 0:
>> +    case MMU_USER_IDX:
>>         DT0 = ldfq_user(addr);
>>         break;
>> -    case 1:
>> +    case MMU_KERNEL_IDX:
>>         DT0 = ldfq_kernel(addr);
>>         break;
>>  #ifdef TARGET_SPARC64
>> -    case 2:
>> +    case MMU_HYPV_IDX:
>>         DT0 = ldfq_hypv(addr);
>>         break;
>>  #endif
>>     default:
>> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>
> The function name is not correct for this and other cases below.
>
>>         break;
>>     }
>>  #else
>> @@ -3373,24 +3375,25 @@ void helper_ldqf(target_ulong addr, int mem_idx)
>>     helper_check_align(addr, 7);
>>  #if !defined(CONFIG_USER_ONLY)
>>     switch (mem_idx) {
>> -    case 0:
>> +    case MMU_USER_IDX:
>>         u.ll.upper = ldq_user(addr);
>>         u.ll.lower = ldq_user(addr + 8);
>>         QT0 = u.q;
>>         break;
>> -    case 1:
>> +    case MMU_KERNEL_IDX:
>>         u.ll.upper = ldq_kernel(addr);
>>         u.ll.lower = ldq_kernel(addr + 8);
>>         QT0 = u.q;
>>         break;
>>  #ifdef TARGET_SPARC64
>> -    case 2:
>> +    case MMU_HYPV_IDX:
>>         u.ll.upper = ldq_hypv(addr);
>>         u.ll.lower = ldq_hypv(addr + 8);
>>         QT0 = u.q;
>>         break;
>>  #endif
>>     default:
>> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>>         break;
>>     }
>>  #else
>> @@ -3408,24 +3411,25 @@ void helper_stqf(target_ulong addr, int mem_idx)
>>     helper_check_align(addr, 7);
>>  #if !defined(CONFIG_USER_ONLY)
>>     switch (mem_idx) {
>> -    case 0:
>> +    case MMU_USER_IDX:
>>         u.q = QT0;
>>         stq_user(addr, u.ll.upper);
>>         stq_user(addr + 8, u.ll.lower);
>>         break;
>> -    case 1:
>> +    case MMU_KERNEL_IDX:
>>         u.q = QT0;
>>         stq_kernel(addr, u.ll.upper);
>>         stq_kernel(addr + 8, u.ll.lower);
>>         break;
>>  #ifdef TARGET_SPARC64
>> -    case 2:
>> +    case MMU_HYPV_IDX:
>>         u.q = QT0;
>>         stq_hypv(addr, u.ll.upper);
>>         stq_hypv(addr + 8, u.ll.lower);
>>         break;
>>  #endif
>>     default:
>> +        printf("helper_stdf: need to check MMU idx %d\n", mem_idx);
>>         break;
>>     }
>>  #else

I'll fix names, turn printf into DPRINTF_MMU and resend.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH 2/8] sparc64: fix missing address masking

[Igor Kovalenko]

On Wed, Jun 2, 2010 at 5:47 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/01/2010 09:29 PM, Igor Kovalenko wrote:
>> On Wed, Jun 2, 2010 at 12:44 AM, Richard Henderson <rth@twiddle.net> wrote:
>>> On 06/01/2010 01:12 PM, Igor V. Kovalenko wrote:
>>>> +    if ((env->pstate & PS_AM) && is_translating_asi(asi)) {
>>>> +        addr &= 0xffffffffULL;
>>>> +    }
>>>
>>> I suggest that these be written instead as
>>>
>>>  if (is_translating_asi(asi)) {
>>>    addr = address_mask(addr);
>>>  }
>>>
>>> That should allow you to remove some of the ifdefs.
>>
>> All address masking is done for sparc64 target only, sparc32 does not
>> have the notion of translating asi.
>
> Of course I know that.
>
>> I think it's better to do debug printf macro trick ...
>
> ... with no evidence.  The compiler is happy to optimize away
> the entire if statement without having to resort to macros.
>
>> ... then but I see no real benefit at the moment.
>
> Avoiding ifdefs isn't a benefit?

Reading through the code you will have false evidence of possible
extra steps taken by hardware we emulate.
Looking at sparcv8 docs it is clear that similar steps are done there
as well so I'll drop ifdefs at call sites and redo the patch.

-- 
Kind regards,
Igor V. Kovalenko

Re: [Qemu-devel] [PATCH 0/8] sparc64 fixes

[Blue Swirl]

Thanks, applied all.

On Tue, Jun 1, 2010 at 8:12 PM, Igor V. Kovalenko
<igor.v.kovalenko@gmail.com> wrote:
> assorted changes, linux kernel can now work with 32bit init task
>
> ---
>
> Igor V. Kovalenko (8):
>      sparc64: fix tag access register on mmu traps
>      sparc64: fix missing address masking
>      sparc64: fix 32bit load sign extension
>      sparc64: fix ldxfsr insn
>      sparc64: use symbolic name for MMU index
>      sparc64: improve ldf and stf insns
>      sparc64: fix udiv and sdiv insns
>      sparc64: fix umul and smul insns
>
>
>  softmmu_header.h         |    2 -
>  target-sparc/helper.c    |    5 ++
>  target-sparc/op_helper.c |  107 ++++++++++++++++++++++++++++++++++++++++------
>  target-sparc/translate.c |   65 ++++++++++++++++------------
>  4 files changed, 137 insertions(+), 42 deletions(-)
>
> --
>
>

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Paolo Bonzini]

On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>
> - change return type of ldl_* to uint32_t to prevent unwanted sign extension
>    visible in sparc64 load alternate address space methods
> - note this change makes ldl_* softmmu implementations match ldl_phys one

This patch breaks -kernel/-initrd.

Paolo

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Alexander Graf]

Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:

> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>
>> - change return type of ldl_* to uint32_t to prevent unwanted sign  
>> extension
>>   visible in sparc64 load alternate address space methods
>> - note this change makes ldl_* softmmu implementations match  
>> ldl_phys one
>
> This patch breaks -kernel/-initrd.

Breaks it where and when?

Alex
>

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Paolo Bonzini]

On 06/03/2010 05:25 PM, Alexander Graf wrote:
>
> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>
>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>
>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>> extension
>>> visible in sparc64 load alternate address space methods
>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>> one
>>
>> This patch breaks -kernel/-initrd.
>
> Breaks it where and when?

x86_64 TCG reboots after the "Probing EDD" step.

Paolo

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Igor Kovalenko]

On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>
>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini <pbonzini@redhat.com>:
>>
>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>
>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>
>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>> extension
>>>> visible in sparc64 load alternate address space methods
>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>> one
>>>
>>> This patch breaks -kernel/-initrd.
>>
>> Breaks it where and when?
>
> x86_64 TCG reboots after the "Probing EDD" step.

My local build appears to work, qemu-system-x86_64 loads my gentoo linux setup.
I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
--prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu

-- 
Kind regards,
Igor V. Kovalenko

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Paolo Bonzini]

On 06/03/2010 09:59 PM, Igor Kovalenko wrote:
> On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini<pbonzini@redhat.com>  wrote:
>> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>>
>>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini<pbonzini@redhat.com>:
>>>
>>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>>
>>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>>
>>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>>> extension
>>>>> visible in sparc64 load alternate address space methods
>>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>>> one
>>>>
>>>> This patch breaks -kernel/-initrd.
>>>
>>> Breaks it where and when?
>>
>> x86_64 TCG reboots after the "Probing EDD" step.
>
> My local build appears to work, qemu-system-x86_64 loads my gentoo linux setup.
> I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
> --prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu

Normal boot works.  Only -kernel/-initrd fails.

Paolo

[Qemu-devel] Re: [PATCH 3/8] sparc64: fix 32bit load sign extension

[Paolo Bonzini]

On 06/04/2010 09:53 AM, Paolo Bonzini wrote:
> On 06/03/2010 09:59 PM, Igor Kovalenko wrote:
>> On Thu, Jun 3, 2010 at 7:42 PM, Paolo Bonzini<pbonzini@redhat.com> wrote:
>>> On 06/03/2010 05:25 PM, Alexander Graf wrote:
>>>>
>>>> Am 03.06.2010 um 15:18 schrieb Paolo Bonzini<pbonzini@redhat.com>:
>>>>
>>>>> On 06/01/2010 10:12 PM, Igor V. Kovalenko wrote:
>>>>>>
>>>>>> From: Igor V. Kovalenko<igor.v.kovalenko@gmail.com>
>>>>>>
>>>>>> - change return type of ldl_* to uint32_t to prevent unwanted sign
>>>>>> extension
>>>>>> visible in sparc64 load alternate address space methods
>>>>>> - note this change makes ldl_* softmmu implementations match ldl_phys
>>>>>> one
>>>>>
>>>>> This patch breaks -kernel/-initrd.
>>>>
>>>> Breaks it where and when?
>>>
>>> x86_64 TCG reboots after the "Probing EDD" step.
>>
>> My local build appears to work, qemu-system-x86_64 loads my gentoo
>> linux setup.
>> I use x86_64 host, gcc 4.4.3, qemu configured with ./configure
>> --prefix=/inst --target-list=sparc64-softmmu,x86_64-softmmu
>
> Normal boot works. Only -kernel/-initrd fails.

Hmm, PEBKAC.  Boot of Fedora and RHEL5 guests always fails, so it's not 
related to -kernel/-initrd.  (Of course, without -kernel/-initrd it 
reboots into GRUB rather than looping quickly).

I've placed a failing vmlinuz at 
http://people.redhat.com/people/vmlinuz-fail -- if it fails it should 
reboot continuously.  The failure happens pretty soon after the kernel 
starts running.  The sequence is:

   lock_kernel
   -> __lock_kernel
   -> preempt_disable
   -> current_thread_info()

   IN:
   0xffffffff80063064:  push   %rbp
   0xffffffff80063065:  mov    %rsp,%rbp
   0xffffffff80063068:  mov    %gs:0x10,%rax
   0xffffffff80063071:  mov    -0x1fc8(%rax),%eax
   0xffffffff80063077:  test   $0x8,%al
   0xffffffff80063079:  je     0xffffffff800630a2

%rax is 0xffffffff803f1fd8, but it page faults with 
%cr2=0x00000000803f0010.  The reason is that in the generated x86 
assembly -0x1fc8 is erroneously zero extended:

0x4180347b:  mov    %rbp,%rbx
0x4180347e:  mov    $0xffffe038,%r12d
0x41803484:  add    %r12,%rbx

so it gives the wrong address:

(gdb) info reg rbp
rbp            0xffffffff803f1fd8	0xffffffff803f1fd8
(gdb) info reg r12
r12            0xffffe038	4294959160
(gdb) info reg rbx
rbx            0x803f0010	2151612432

 From there it's obvious: general protection, double fault, general 
protection, triple fault.

So it's a TCG bug that is expecting ldl_* to sign extend.  I'll send a 
patch after I come back from lunch.

Paolo

[Qemu-devel] [PATCH] target-i386: fix decoding of negative 4-byte displacements

[Paolo Bonzini]

Negative four byte displacements need to be sign-extended after
c086b783eb7a578993d6d2ab62c4c2666800b63d.  Do so.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
        There are quite a few other ldl's to audit after the patch
        (about 70 in target-*).  Any volunteers? :-)

 target-i386/translate.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/target-i386/translate.c b/target-i386/translate.c
index 38c6016..708b0a1 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -2016,7 +2016,7 @@ static void gen_lea_modrm(DisasContext *s, int modrm, int *reg_ptr, int *offset_
             break;
         default:
         case 2:
-            disp = ldl_code(s->pc);
+            disp = (int32_t)ldl_code(s->pc);
             s->pc += 4;
             break;
         }
-- 
1.7.0.1

Re: [Qemu-devel] [PATCH] target-i386: fix decoding of negative 4-byte displacements

[Richard Henderson]

On 06/04/2010 07:27 AM, Paolo Bonzini wrote:
> Negative four byte displacements need to be sign-extended after
> c086b783eb7a578993d6d2ab62c4c2666800b63d.  Do so.

Acked-by: Richard Henderson  <rth@twiddle.net>


>         There are quite a few other ldl's to audit after the patch
>         (about 70 in target-*).  Any volunteers? :-)

I've looked over all the uses of ldl_code.  Thankfully 95% of them
are immediately stored into an explicit 32-bit variable.  I do not
see any other problematic uses of that particular identifier.


r~

Re: [Qemu-devel] [PATCH] target-i386: fix decoding of negative 4-byte displacements

[Blue Swirl]

Thanks, applied.

On Fri, Jun 4, 2010 at 4:23 PM, Richard Henderson <rth@twiddle.net> wrote:
> On 06/04/2010 07:27 AM, Paolo Bonzini wrote:
>> Negative four byte displacements need to be sign-extended after
>> c086b783eb7a578993d6d2ab62c4c2666800b63d.  Do so.
>
> Acked-by: Richard Henderson  <rth@twiddle.net>
>
>
>>         There are quite a few other ldl's to audit after the patch
>>         (about 70 in target-*).  Any volunteers? :-)
>
> I've looked over all the uses of ldl_code.  Thankfully 95% of them
> are immediately stored into an explicit 32-bit variable.  I do not
> see any other problematic uses of that particular identifier.
>
>
> r~
>