[Qemu-devel] [PATCH] i386 target: fix ARPL

[Qemu-devel] [PATCH] i386 target: fix ARPL

[Laurent Desnogues]

[-- Attachment #1: Type: text/plain, Size: 227 bytes --]

Hello,

The arpl implementation in target-i386/translate.c uses cpu_A0
temporary across a brcond op.  This patch fixes that issue.

Note I didn't test it, I only looked at generated code to check it
was making sense.


Laurent

[-- Attachment #2: i386-arpl.patch --]
[-- Type: text/x-diff, Size: 1747 bytes --]

diff --git a/target-i386/translate.c b/target-i386/translate.c
index e3cb49f..807707f 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7305,13 +7305,14 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
 #endif
         {
             int label1;
-            TCGv t0, t1, t2;
+            TCGv t0, t1, t2, a0;
 
             if (!s->pe || s->vm86)
                 goto illegal_op;
             t0 = tcg_temp_local_new();
             t1 = tcg_temp_local_new();
             t2 = tcg_temp_local_new();
+            a0 = tcg_temp_local_new();
             ot = OT_WORD;
             modrm = ldub_code(s->pc++);
             reg = (modrm >> 3) & 7;
@@ -7320,6 +7321,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             if (mod != 3) {
                 gen_lea_modrm(s, modrm, &reg_addr, &offset_addr);
                 gen_op_ld_v(ot + s->mem_index, t0, cpu_A0);
+                tcg_gen_mov_tl(a0, cpu_A0);
             } else {
                 gen_op_mov_v_reg(ot, t0, rm);
             }
@@ -7334,7 +7336,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             tcg_gen_movi_tl(t2, CC_Z);
             gen_set_label(label1);
             if (mod != 3) {
-                gen_op_st_v(ot + s->mem_index, t0, cpu_A0);
+                gen_op_st_v(ot + s->mem_index, t0, a0);
             } else {
                 gen_op_mov_reg_v(ot, rm, t0);
             }
@@ -7347,6 +7349,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             tcg_temp_free(t0);
             tcg_temp_free(t1);
             tcg_temp_free(t2);
+            tcg_temp_free(a0);
         }
         break;
     case 0x102: /* lar */

Re: [Qemu-devel] [PATCH] i386 target: fix ARPL

[Aurelien Jarno]

On Mon, Oct 05, 2009 at 05:18:26PM +0200, Laurent Desnogues wrote:
> Hello,
> 
> The arpl implementation in target-i386/translate.c uses cpu_A0
> temporary across a brcond op.  This patch fixes that issue.
> 
> Note I didn't test it, I only looked at generated code to check it
> was making sense.

This looks indeed correct. I wonder however if it would be better to do
the tcg_temp_local_new() / tcg_temp_free() in the if (mod != 3) path
only.

Also this patch needs a Signed-of-by:

Aurelien
 
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index e3cb49f..807707f 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c
> @@ -7305,13 +7305,14 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>  #endif
>          {
>              int label1;
> -            TCGv t0, t1, t2;
> +            TCGv t0, t1, t2, a0;
>  
>              if (!s->pe || s->vm86)
>                  goto illegal_op;
>              t0 = tcg_temp_local_new();
>              t1 = tcg_temp_local_new();
>              t2 = tcg_temp_local_new();
> +            a0 = tcg_temp_local_new();
>              ot = OT_WORD;
>              modrm = ldub_code(s->pc++);
>              reg = (modrm >> 3) & 7;
> @@ -7320,6 +7321,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>              if (mod != 3) {
>                  gen_lea_modrm(s, modrm, &reg_addr, &offset_addr);
>                  gen_op_ld_v(ot + s->mem_index, t0, cpu_A0);
> +                tcg_gen_mov_tl(a0, cpu_A0);
>              } else {
>                  gen_op_mov_v_reg(ot, t0, rm);
>              }
> @@ -7334,7 +7336,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>              tcg_gen_movi_tl(t2, CC_Z);
>              gen_set_label(label1);
>              if (mod != 3) {
> -                gen_op_st_v(ot + s->mem_index, t0, cpu_A0);
> +                gen_op_st_v(ot + s->mem_index, t0, a0);
>              } else {
>                  gen_op_mov_reg_v(ot, rm, t0);
>              }
> @@ -7347,6 +7349,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
>              tcg_temp_free(t0);
>              tcg_temp_free(t1);
>              tcg_temp_free(t2);
> +            tcg_temp_free(a0);
>          }
>          break;
>      case 0x102: /* lar */


-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] i386 target: fix ARPL

[Anthony Liguori]

Laurent Desnogues wrote:
> Hello,
>
> The arpl implementation in target-i386/translate.c uses cpu_A0
> temporary across a brcond op.  This patch fixes that issue.
>
> Note I didn't test it, I only looked at generated code to check it
> was making sense.
>   

Missing Signed-off-by.

> Laurent
>   

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH] i386 target: fix ARPL

[Laurent Desnogues]

[-- Attachment #1: Type: text/plain, Size: 429 bytes --]

On Mon, Oct 5, 2009 at 11:56 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
[...]
> This looks indeed correct. I wonder however if it would be better to do
> the tcg_temp_local_new() / tcg_temp_free() in the if (mod != 3) path
> only.

I'm not sure it will make any difference (arpl is probably not used
extensively).  Anywyay here is an updated patch.


Laurent

Signed-off-by: Laurent Desnogues <laurent.desnogues@gmail.com>

[-- Attachment #2: i386-arpl-v2.patch --]
[-- Type: text/x-diff, Size: 1476 bytes --]

diff --git a/target-i386/translate.c b/target-i386/translate.c
index e3cb49f..2511943 100644
--- a/target-i386/translate.c
+++ b/target-i386/translate.c
@@ -7305,7 +7305,7 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
 #endif
         {
             int label1;
-            TCGv t0, t1, t2;
+            TCGv t0, t1, t2, a0;
 
             if (!s->pe || s->vm86)
                 goto illegal_op;
@@ -7320,8 +7320,11 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             if (mod != 3) {
                 gen_lea_modrm(s, modrm, &reg_addr, &offset_addr);
                 gen_op_ld_v(ot + s->mem_index, t0, cpu_A0);
+                a0 = tcg_temp_local_new();
+                tcg_gen_mov_tl(a0, cpu_A0);
             } else {
                 gen_op_mov_v_reg(ot, t0, rm);
+                TCGV_UNUSED(a0);
             }
             gen_op_mov_v_reg(ot, t1, reg);
             tcg_gen_andi_tl(cpu_tmp0, t0, 3);
@@ -7334,8 +7337,9 @@ static target_ulong disas_insn(DisasContext *s, target_ulong pc_start)
             tcg_gen_movi_tl(t2, CC_Z);
             gen_set_label(label1);
             if (mod != 3) {
-                gen_op_st_v(ot + s->mem_index, t0, cpu_A0);
-            } else {
+                gen_op_st_v(ot + s->mem_index, t0, a0);
+                tcg_temp_free(a0);
+           } else {
                 gen_op_mov_reg_v(ot, rm, t0);
             }
             if (s->cc_op != CC_OP_DYNAMIC)