[Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions

[Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions

[Sven Schnelle]

When QEMU is compiled with -O0, these functions are inlined
which will cause a wrong restart address generated for the TB.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/op_helper.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/target/hppa/op_helper.c b/target/hppa/op_helper.c
index 6bf478e7b0..268caaaa20 100644
--- a/target/hppa/op_helper.c
+++ b/target/hppa/op_helper.c
@@ -81,10 +81,8 @@ static void atomic_store_3(CPUHPPAState *env, target_ulong addr, uint32_t val,
 }
 
 static void do_stby_b(CPUHPPAState *env, target_ulong addr, target_ureg val,
-                      bool parallel)
+                      bool parallel, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
-
     switch (addr & 3) {
     case 3:
         cpu_stb_data_ra(env, addr, val, ra);
@@ -109,20 +107,18 @@ static void do_stby_b(CPUHPPAState *env, target_ulong addr, target_ureg val,
 
 void HELPER(stby_b)(CPUHPPAState *env, target_ulong addr, target_ureg val)
 {
-    do_stby_b(env, addr, val, false);
+    do_stby_b(env, addr, val, false, GETPC());
 }
 
 void HELPER(stby_b_parallel)(CPUHPPAState *env, target_ulong addr,
                              target_ureg val)
 {
-    do_stby_b(env, addr, val, true);
+    do_stby_b(env, addr, val, true, GETPC());
 }
 
 static void do_stby_e(CPUHPPAState *env, target_ulong addr, target_ureg val,
-                      bool parallel)
+                      bool parallel, uintptr_t ra)
 {
-    uintptr_t ra = GETPC();
-
     switch (addr & 3) {
     case 3:
         /* The 3 byte store must appear atomic.  */
@@ -151,13 +147,13 @@ static void do_stby_e(CPUHPPAState *env, target_ulong addr, target_ureg val,
 
 void HELPER(stby_e)(CPUHPPAState *env, target_ulong addr, target_ureg val)
 {
-    do_stby_e(env, addr, val, false);
+    do_stby_e(env, addr, val, false, GETPC());
 }
 
 void HELPER(stby_e_parallel)(CPUHPPAState *env, target_ulong addr,
                              target_ureg val)
 {
-    do_stby_e(env, addr, val, true);
+    do_stby_e(env, addr, val, true, GETPC());
 }
 
 target_ureg HELPER(probe)(CPUHPPAState *env, target_ulong addr,
-- 
2.20.1

[Qemu-devel] [PATCH 2/5] target/hppa: fix '</<=' conditions

[Sven Schnelle]

These condition include the signed overflow bit. See Page 5-3 of
the Parisc 1.1 Architecture Reference manual for details.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 51bfd9849d..0e8cc8117a 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -404,6 +404,11 @@ static DisasCond cond_make_n(void)
     };
 }
 
+static bool cond_need_sv(int c)
+{
+        return c == 2 || c == 3 || c == 6;
+}
+
 static DisasCond cond_make_0(TCGCond c, TCGv_reg a0)
 {
     DisasCond r = { .c = c, .a1 = NULL, .a1_is_0 = true };
@@ -910,11 +915,17 @@ static DisasCond do_cond(unsigned cf, TCGv_reg res,
     case 1: /* = / <>        (Z / !Z) */
         cond = cond_make_0(TCG_COND_EQ, res);
         break;
-    case 2: /* < / >=        (N / !N) */
-        cond = cond_make_0(TCG_COND_LT, res);
+    case 2: /* < / >=        (N ^ V / !(N ^ V)) */
+        tmp = tcg_temp_new();
+        tcg_gen_xor_reg(tmp, res, sv);
+        cond = cond_make_0(TCG_COND_LT, tmp);
+        tcg_temp_free(tmp);
         break;
     case 3: /* <= / >        (N | Z / !N & !Z) */
-        cond = cond_make_0(TCG_COND_LE, res);
+        tmp = tcg_temp_new();
+        tcg_gen_xor_reg(tmp, res, sv);
+        cond = cond_make_0(TCG_COND_LE, tmp);
+        tcg_temp_free(tmp);
         break;
     case 4: /* NUV / UV      (!C / C) */
         cond = cond_make_0(TCG_COND_EQ, cb_msb);
@@ -1159,7 +1170,7 @@ static DisasJumpType do_add(DisasContext *ctx, unsigned rt, TCGv_reg in1,
 
     /* Compute signed overflow if required.  */
     sv = NULL;
-    if (is_tsv || c == 6) {
+    if (is_tsv || cond_need_sv(c)) {
         sv = do_add_sv(ctx, dest, in1, in2);
         if (is_tsv) {
             /* ??? Need to include overflow from shift.  */
@@ -1223,7 +1234,7 @@ static DisasJumpType do_sub(DisasContext *ctx, unsigned rt, TCGv_reg in1,
 
     /* Compute signed overflow if required.  */
     sv = NULL;
-    if (is_tsv || c == 6) {
+    if (is_tsv || cond_need_sv(c)) {
         sv = do_sub_sv(ctx, dest, in1, in2);
         if (is_tsv) {
             gen_helper_tsv(cpu_env, sv);
@@ -1263,13 +1274,14 @@ static DisasJumpType do_cmpclr(DisasContext *ctx, unsigned rt, TCGv_reg in1,
 {
     TCGv_reg dest, sv;
     DisasCond cond;
+    int c = cf >> 1;
 
     dest = tcg_temp_new();
     tcg_gen_sub_reg(dest, in1, in2);
 
     /* Compute signed overflow if required.  */
     sv = NULL;
-    if ((cf >> 1) == 6) {
+    if (cond_need_sv(c)) {
         sv = do_sub_sv(ctx, dest, in1, in2);
     }
 
@@ -2808,7 +2820,7 @@ static DisasJumpType trans_ds(DisasContext *ctx, uint32_t insn,
     /* Install the new nullification.  */
     if (cf) {
         TCGv_reg sv = NULL;
-        if (cf >> 1 == 6) {
+        if (cond_need_sv(cf >> 1)) {
             /* ??? The lshift is supposed to contribute to overflow.  */
             sv = do_add_sv(ctx, dest, add1, add2);
         }
@@ -3369,7 +3381,7 @@ static DisasJumpType trans_cmpb(DisasContext *ctx, uint32_t insn,
     tcg_gen_sub_reg(dest, in1, in2);
 
     sv = NULL;
-    if (c == 6) {
+    if (cond_need_sv(c)) {
         sv = do_sub_sv(ctx, dest, in1, in2);
     }
 
@@ -3409,6 +3421,8 @@ static DisasJumpType trans_addb(DisasContext *ctx, uint32_t insn,
         tcg_gen_movi_reg(cb_msb, 0);
         tcg_gen_add2_reg(dest, cb_msb, in1, cb_msb, in2, cb_msb);
         break;
+    case 2:
+    case 3:
     case 6:
         tcg_gen_add_reg(dest, in1, in2);
         sv = do_add_sv(ctx, dest, in1, in2);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 2/5] target/hppa: fix '</<=' conditions

[Richard Henderson]

On 2/11/19 10:19 AM, Sven Schnelle wrote:
>      case 3: /* <= / >        (N | Z / !N & !Z) */
> -        cond = cond_make_0(TCG_COND_LE, res);
> +        tmp = tcg_temp_new();
> +        tcg_gen_xor_reg(tmp, res, sv);
> +        cond = cond_make_0(TCG_COND_LE, tmp);
> +        tcg_temp_free(tmp);
>          break;

So the test here should be (N ^ V) | Z, which is not what you're testing.  I've
fixed this up locally.


r~

[Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions

[Sven Schnelle]

Now that do_cond() uses sign overflow for some condition matches we
need to roll our own version without sign overflow checks.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index 0e8cc8117a..bce8773b1a 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -996,12 +996,35 @@ static DisasCond do_sub_cond(unsigned cf, TCGv_reg res,
 
 static DisasCond do_log_cond(unsigned cf, TCGv_reg res)
 {
+    DisasCond cond;
+    TCGv_reg tmp;
+
     switch (cf >> 1) {
-    case 4: case 5: case 6:
-        cf &= 1;
+    case 0: /* never */
+        cond = cond_make_f();
+        break;
+    case 1: /* = all bits are zero */
+        cond = cond_make_0(TCG_COND_EQ, res);
+        break;
+    case 2: /* < leftmost bit is 1 */
+        cond = cond_make_0(TCG_COND_LT, res);
+        break;
+    case 3: /* <= leftmost bit is 1 or all bits 0 */
+        cond = cond_make_0(TCG_COND_LE, res);
+        break;
+    case 7: /* OD rightmost bit is 1 */
+        tmp = tcg_temp_new();
+        tcg_gen_andi_reg(tmp, res, 1);
+        cond = cond_make_0(TCG_COND_NE, tmp);
+        tcg_temp_free(tmp);
+        break;
+    default:
         break;
     }
-    return do_cond(cf, res, res, res);
+    if (cf & 1) {
+        cond.c = tcg_invert_cond(cond.c);
+    }
+    return cond;
 }
 
 /* Similar, but for shift/extract/deposit conditions.  */
-- 
2.20.1

Re: [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions

[Richard Henderson]

On 2/11/19 10:19 AM, Sven Schnelle wrote:
>      switch (cf >> 1) {
> -    case 4: case 5: case 6:
> -        cf &= 1;
> +    case 0: /* never */
> +        cond = cond_make_f();
> +        break;
> +    case 1: /* = all bits are zero */
> +        cond = cond_make_0(TCG_COND_EQ, res);
> +        break;
> +    case 2: /* < leftmost bit is 1 */
> +        cond = cond_make_0(TCG_COND_LT, res);
> +        break;
> +    case 3: /* <= leftmost bit is 1 or all bits 0 */
> +        cond = cond_make_0(TCG_COND_LE, res);
> +        break;
> +    case 7: /* OD rightmost bit is 1 */
> +        tmp = tcg_temp_new();
> +        tcg_gen_andi_reg(tmp, res, 1);
> +        cond = cond_make_0(TCG_COND_NE, tmp);
> +        tcg_temp_free(tmp);
> +        break;
> +    default:
>          break;
>      }

You can't do nothing for cases 4,5,6.  That lets a bad guest crash qemu, since
cond will be uninitialized.  Also, this patch has to be sorted before the
previous, as otherwise you introduce a regression during bisection.

I've fixed this up locally.


r~

[Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions

[Sven Schnelle]

Now that do_cond() uses sign overflow for some condition matches we
need to roll our own version without sign overflow checks.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 34 +++++++++++++++++++++++-----------
 1 file changed, 23 insertions(+), 11 deletions(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index bce8773b1a..d858fabd3a 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -1029,20 +1029,32 @@ static DisasCond do_log_cond(unsigned cf, TCGv_reg res)
 
 /* Similar, but for shift/extract/deposit conditions.  */
 
-static DisasCond do_sed_cond(unsigned orig, TCGv_reg res)
+static DisasCond do_sed_cond(unsigned c, TCGv_reg res)
 {
-    unsigned c, f;
+    DisasCond cond;
+    TCGv_reg tmp;
 
-    /* Convert the compressed condition codes to standard.
-       0-2 are the same as logicals (nv,<,<=), while 3 is OD.
-       4-7 are the reverse of 0-3.  */
-    c = orig & 3;
-    if (c == 3) {
-        c = 7;
+    switch(c & 3) {
+    case 0: /* never */
+        cond = cond_make_f();
+        break;
+    case 1: /* = all bits are zero */
+        cond = cond_make_0(TCG_COND_EQ, res);
+        break;
+    case 2: /* < leftmost bit is 1 */
+        cond = cond_make_0(TCG_COND_LT, res);
+        break;
+    case 3: /* OD rightmost bit is 1 */
+        tmp = tcg_temp_new();
+        tcg_gen_andi_reg(tmp, res, 1);
+        cond = cond_make_0(TCG_COND_NE, tmp);
+        tcg_temp_free(tmp);
+        break;
     }
-    f = (orig & 4) / 4;
-
-    return do_log_cond(c * 2 + f, res);
+    if (c & 4) {
+        cond.c = tcg_invert_cond(cond.c);
+    }
+    return cond;
 }
 
 /* Similar, but for unit conditions.  */
-- 
2.20.1

Re: [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions

[Richard Henderson]

On 2/11/19 10:19 AM, Sven Schnelle wrote:
> -    f = (orig & 4) / 4;
> -
> -    return do_log_cond(c * 2 + f, res);

Given that this used to reference do_log_cond, and you've fixed do_log_cond,
why is there any reason for a change here?


r~

Re: [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions

[Sven Schnelle]

Hi Richard,


On Mon, Feb 11, 2019 at 08:27:32PM -0800, Richard Henderson wrote:
> On 2/11/19 10:19 AM, Sven Schnelle wrote:
> > -    f = (orig & 4) / 4;
> > -
> > -    return do_log_cond(c * 2 + f, res);
> 
> Given that this used to reference do_log_cond, and you've fixed do_log_cond,
> why is there any reason for a change here?

You're right, this patch can be dropped.

Regards
Sven

[Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction

[Sven Schnelle]

It looks like the operands where exchanged. HP bootrom tests the
following sequence:

0x00000000f0004064:  ldil L%-66666800,r7
0x00000000f0004068:  addi 19f,r7,r7
0x00000000f000406c:  addi -1,r0,rp
0x00000000f0004070:  addi f,r0,r4
0x00000000f0004074:  addi 1,r4,r5
0x00000000f0004078:  dcor rp,r6
0x00000000f000407c:  cmpb,<>,n r6,r7,0xf000411

This returned 0x66666661 instead of the expected 0x9999999f in QEMU.

Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
 target/hppa/translate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/hppa/translate.c b/target/hppa/translate.c
index d858fabd3a..69c5a558fc 100644
--- a/target/hppa/translate.c
+++ b/target/hppa/translate.c
@@ -2797,7 +2797,7 @@ static DisasJumpType trans_dcor(DisasContext *ctx, uint32_t insn,
     }
     tcg_gen_andi_reg(tmp, tmp, 0x11111111);
     tcg_gen_muli_reg(tmp, tmp, 6);
-    ret = do_unit(ctx, rt, tmp, load_gpr(ctx, r2), cf, false,
+    ret = do_unit(ctx, rt, load_gpr(ctx, r2), tmp, cf, false,
                   is_i ? tcg_gen_add_reg : tcg_gen_sub_reg);
 
     return nullify_end(ctx, ret);
-- 
2.20.1

Re: [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction

[Richard Henderson]

On 2/11/19 10:19 AM, Sven Schnelle wrote:
> It looks like the operands where exchanged. HP bootrom tests the
> following sequence:
> 
> 0x00000000f0004064:  ldil L%-66666800,r7
> 0x00000000f0004068:  addi 19f,r7,r7
> 0x00000000f000406c:  addi -1,r0,rp
> 0x00000000f0004070:  addi f,r0,r4
> 0x00000000f0004074:  addi 1,r4,r5
> 0x00000000f0004078:  dcor rp,r6
> 0x00000000f000407c:  cmpb,<>,n r6,r7,0xf000411
> 
> This returned 0x66666661 instead of the expected 0x9999999f in QEMU.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  target/hppa/translate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks, queued.


r~

Re: [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions

[Richard Henderson]

On 2/11/19 10:19 AM, Sven Schnelle wrote:
> When QEMU is compiled with -O0, these functions are inlined
> which will cause a wrong restart address generated for the TB.
> 
> Signed-off-by: Sven Schnelle <svens@stackframe.org>
> ---
>  target/hppa/op_helper.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Queued, thanks.


r~