* [Qemu-devel] [PATCH 2/5] target/hppa: fix '</<=' conditions
2019-02-11 18:19 [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Sven Schnelle
@ 2019-02-11 18:19 ` Sven Schnelle
2019-02-12 4:26 ` Richard Henderson
2019-02-11 18:19 ` [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions Sven Schnelle
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Sven Schnelle @ 2019-02-11 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: deller, Sven Schnelle, Richard Henderson
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions
2019-02-11 18:19 [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Sven Schnelle
2019-02-11 18:19 ` [Qemu-devel] [PATCH 2/5] target/hppa: fix '</<=' conditions Sven Schnelle
@ 2019-02-11 18:19 ` Sven Schnelle
2019-02-12 4:30 ` Richard Henderson
2019-02-11 18:19 ` [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions Sven Schnelle
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Sven Schnelle @ 2019-02-11 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: deller, Sven Schnelle, Richard Henderson
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions
2019-02-11 18:19 ` [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions Sven Schnelle
@ 2019-02-12 4:30 ` Richard Henderson
0 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2019-02-12 4:30 UTC (permalink / raw)
To: Sven Schnelle, qemu-devel; +Cc: deller, 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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions
2019-02-11 18:19 [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Sven Schnelle
2019-02-11 18:19 ` [Qemu-devel] [PATCH 2/5] target/hppa: fix '</<=' conditions Sven Schnelle
2019-02-11 18:19 ` [Qemu-devel] [PATCH 3/5] target/hppa: fix log conditions Sven Schnelle
@ 2019-02-11 18:19 ` Sven Schnelle
2019-02-12 4:27 ` Richard Henderson
2019-02-11 18:19 ` [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction Sven Schnelle
2019-02-12 0:07 ` [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Richard Henderson
4 siblings, 1 reply; 11+ messages in thread
From: Sven Schnelle @ 2019-02-11 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: deller, Sven Schnelle, Richard Henderson
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions
2019-02-11 18:19 ` [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions Sven Schnelle
@ 2019-02-12 4:27 ` Richard Henderson
2019-02-14 8:10 ` Sven Schnelle
0 siblings, 1 reply; 11+ messages in thread
From: Richard Henderson @ 2019-02-12 4:27 UTC (permalink / raw)
To: Sven Schnelle, qemu-devel; +Cc: deller, 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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions
2019-02-12 4:27 ` Richard Henderson
@ 2019-02-14 8:10 ` Sven Schnelle
0 siblings, 0 replies; 11+ messages in thread
From: Sven Schnelle @ 2019-02-14 8:10 UTC (permalink / raw)
To: Richard Henderson; +Cc: qemu-devel, deller, Richard Henderson
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction
2019-02-11 18:19 [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Sven Schnelle
` (2 preceding siblings ...)
2019-02-11 18:19 ` [Qemu-devel] [PATCH 4/5] target/hppa: fix sed conditions Sven Schnelle
@ 2019-02-11 18:19 ` Sven Schnelle
2019-02-12 4:30 ` Richard Henderson
2019-02-12 0:07 ` [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Richard Henderson
4 siblings, 1 reply; 11+ messages in thread
From: Sven Schnelle @ 2019-02-11 18:19 UTC (permalink / raw)
To: qemu-devel; +Cc: deller, Sven Schnelle, Richard Henderson
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction
2019-02-11 18:19 ` [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction Sven Schnelle
@ 2019-02-12 4:30 ` Richard Henderson
0 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2019-02-12 4:30 UTC (permalink / raw)
To: Sven Schnelle, qemu-devel; +Cc: deller, 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~
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions
2019-02-11 18:19 [Qemu-devel] [PATCH 1/5] target/hppa: move GETPC to HELPER() functions Sven Schnelle
` (3 preceding siblings ...)
2019-02-11 18:19 ` [Qemu-devel] [PATCH 5/5] target/hppa: fix dcor instruction Sven Schnelle
@ 2019-02-12 0:07 ` Richard Henderson
4 siblings, 0 replies; 11+ messages in thread
From: Richard Henderson @ 2019-02-12 0:07 UTC (permalink / raw)
To: Sven Schnelle, qemu-devel; +Cc: deller, 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~
^ permalink raw reply [flat|nested] 11+ messages in thread