Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-04 Thread Richard Henderson 

On 11/04/2016 01:59 AM, Laurent Vivier wrote:

Le 03/11/2016 à 21:47, Richard Henderson a écrit :

On 11/02/2016 03:15 PM, Laurent Vivier wrote:

+for (i = 15; i >= 0; i--, mask >>= 1) {
+if (mask & 1) {
+if ((insn & 7) + 8 == i &&
+m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
+/* M68020+: if the addressing register is the
+ * register moved to memory, the value written
+ * is the initial value decremented by the
size of
+ * the operation
+ * M68000/M68010: the value is the initial value
+ */
+TCGv tmp = tcg_temp_new();
+tcg_gen_sub_i32(tmp, mreg(i), incr);
+gen_store(s, opsize, addr, tmp);
+tcg_temp_free(tmp);
+} else {
+gen_store(s, opsize, addr, mreg(i));
+}
+if (mask != 1) {
+tcg_gen_sub_i32(addr, addr, incr);
+}
+}


One more thing: This is pre-decrement.  Why are you decrementing after
the store?  Seems to me this should be

   if (mask & 1) {
   tcg_gen_sub_i32(addr, addr, incr);
   if (REG(insn, 0) + 8 == i ...)
   ...
   }



Because it has already been decremented by gen_lea()... so this a
problem if we have page fault, except if we use your "areg writeback"
series, and we will.


Ah, I see.  No, gen_lea doesn't writeback; only gen_ea does.  But I wonder if 
this couldn't be cleaned up a tad.  I'll get back to you.



r~

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-04 Thread Laurent Vivier 
Le 03/11/2016 à 21:47, Richard Henderson a écrit :
> On 11/02/2016 03:15 PM, Laurent Vivier wrote:
>> +for (i = 15; i >= 0; i--, mask >>= 1) {
>> +if (mask & 1) {
>> +if ((insn & 7) + 8 == i &&
>> +m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
>> +/* M68020+: if the addressing register is the
>> + * register moved to memory, the value written
>> + * is the initial value decremented by the
>> size of
>> + * the operation
>> + * M68000/M68010: the value is the initial value
>> + */
>> +TCGv tmp = tcg_temp_new();
>> +tcg_gen_sub_i32(tmp, mreg(i), incr);
>> +gen_store(s, opsize, addr, tmp);
>> +tcg_temp_free(tmp);
>> +} else {
>> +gen_store(s, opsize, addr, mreg(i));
>> +}
>> +if (mask != 1) {
>> +tcg_gen_sub_i32(addr, addr, incr);
>> +}
>> +}
> 
> One more thing: This is pre-decrement.  Why are you decrementing after
> the store?  Seems to me this should be
> 
>if (mask & 1) {
>tcg_gen_sub_i32(addr, addr, incr);
>if (REG(insn, 0) + 8 == i ...)
>...
>}
> 

Because it has already been decremented by gen_lea()... so this a
problem if we have page fault, except if we use your "areg writeback"
series, and we will.

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-03 Thread Richard Henderson 

On 11/03/2016 02:11 PM, Laurent Vivier wrote:

Le 03/11/2016 à 20:47, Richard Henderson a écrit :

On 11/02/2016 03:15 PM, Laurent Vivier wrote:

+if ((insn & 7) + 8 == i &&
+m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
+/* M68020+: if the addressing register is the
+ * register moved to memory, the value written
+ * is the initial value decremented by the
size of
+ * the operation
+ * M68000/M68010: the value is the initial value
+ */
+TCGv tmp = tcg_temp_new();
+tcg_gen_sub_i32(tmp, mreg(i), incr);
+gen_store(s, opsize, addr, tmp);
+tcg_temp_free(tmp);


This doesn't look right.  Is the value stored the intermediate value of
the decremented register, or the final value?  What you're storing is
reg-4, which is neither of these things.

I could see, maybe, that reg-4 might well turn out to be the right value
for

movem{a0-a7}, (sp)-

since sp == a7, and therefore stored first.  But I question that's the
correct result for

movem{a0-a7}, (a1)-

If it's the incremental value, then you can just store "addr" and you
don't need a temp.  If it's the final value, then you can compute

tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4);



As it was not clear for me, I have written a test to see what was the
good value.

my test program is:

top:
.space 64,0
stack:
.text
.globl _start
_start:
lea stack,%a4
lea 1,%a0
lea 2,%a1
lea 3,%a2
lea 4,%a3
lea 5,%a5
lea 6,%a6
moveq.l #8, %d0
moveq.l #9, %d1
moveq.l #10, %d2
moveq.l #11, %d3
moveq.l #12, %d4
moveq.l #13, %d5
moveq.l #14, %d6
moveq.l #15, %d7
movem.l %a0-%a7/%d0-%d7,-(%a4)

on a real 68040:

initial value of A4 is 0x800020ec
final value of A4 is   0x800020ac

(gdb) x/15x 0x800020ac
0x800020ac: 0x0008  0x0009  0x000a  0x000b
0x800020bc: 0x000c  0x000d  0x000e  0x000f
0x800020cc: 0x0001  0x0002  0x0003  0x0004
0x800020dc: 0x800020e8  0x0005  0x0006

Stored value is thus 0x800020e8 so this is initial value - 4.
[I have tried the same test with a1, for the same result]


Weird.  But, ok.


r~

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-03 Thread Richard Henderson 

On 11/02/2016 03:15 PM, Laurent Vivier wrote:

+for (i = 15; i >= 0; i--, mask >>= 1) {
+if (mask & 1) {
+if ((insn & 7) + 8 == i &&
+m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
+/* M68020+: if the addressing register is the
+ * register moved to memory, the value written
+ * is the initial value decremented by the size of
+ * the operation
+ * M68000/M68010: the value is the initial value
+ */
+TCGv tmp = tcg_temp_new();
+tcg_gen_sub_i32(tmp, mreg(i), incr);
+gen_store(s, opsize, addr, tmp);
+tcg_temp_free(tmp);
+} else {
+gen_store(s, opsize, addr, mreg(i));
+}
+if (mask != 1) {
+tcg_gen_sub_i32(addr, addr, incr);
+}
+}


One more thing: This is pre-decrement.  Why are you decrementing after the 
store?  Seems to me this should be


   if (mask & 1) {
   tcg_gen_sub_i32(addr, addr, incr);
   if (REG(insn, 0) + 8 == i ...)
   ...
   }


r~

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-03 Thread Laurent Vivier 
Le 03/11/2016 à 20:47, Richard Henderson a écrit :
> On 11/02/2016 03:15 PM, Laurent Vivier wrote:
>> +if ((insn & 7) + 8 == i &&
>> +m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
>> +/* M68020+: if the addressing register is the
>> + * register moved to memory, the value written
>> + * is the initial value decremented by the
>> size of
>> + * the operation
>> + * M68000/M68010: the value is the initial value
>> + */
>> +TCGv tmp = tcg_temp_new();
>> +tcg_gen_sub_i32(tmp, mreg(i), incr);
>> +gen_store(s, opsize, addr, tmp);
>> +tcg_temp_free(tmp);
> 
> This doesn't look right.  Is the value stored the intermediate value of
> the decremented register, or the final value?  What you're storing is
> reg-4, which is neither of these things.
>
> I could see, maybe, that reg-4 might well turn out to be the right value
> for
> 
> movem{a0-a7}, (sp)-
> 
> since sp == a7, and therefore stored first.  But I question that's the
> correct result for
> 
> movem{a0-a7}, (a1)-
> 
> If it's the incremental value, then you can just store "addr" and you
> don't need a temp.  If it's the final value, then you can compute
> 
> tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4);
> 

As it was not clear for me, I have written a test to see what was the
good value.

my test program is:

top:
.space 64,0
stack:
.text
.globl _start
_start:
lea stack,%a4
lea 1,%a0
lea 2,%a1
lea 3,%a2
lea 4,%a3
lea 5,%a5
lea 6,%a6
moveq.l #8, %d0
moveq.l #9, %d1
moveq.l #10, %d2
moveq.l #11, %d3
moveq.l #12, %d4
moveq.l #13, %d5
moveq.l #14, %d6
moveq.l #15, %d7
movem.l %a0-%a7/%d0-%d7,-(%a4)

on a real 68040:

initial value of A4 is 0x800020ec
final value of A4 is   0x800020ac

(gdb) x/15x 0x800020ac
0x800020ac: 0x0008  0x0009  0x000a  0x000b
0x800020bc: 0x000c  0x000d  0x000e  0x000f
0x800020cc: 0x0001  0x0002  0x0003  0x0004
0x800020dc: 0x800020e8  0x0005  0x0006

Stored value is thus 0x800020e8 so this is initial value - 4.
[I have tried the same test with a1, for the same result]

Laurent

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-03 Thread Richard Henderson 

On 11/02/2016 03:15 PM, Laurent Vivier wrote:

+if ((insn & 7) + 8 == i &&
+m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
+/* M68020+: if the addressing register is the
+ * register moved to memory, the value written
+ * is the initial value decremented by the size of
+ * the operation
+ * M68000/M68010: the value is the initial value
+ */
+TCGv tmp = tcg_temp_new();
+tcg_gen_sub_i32(tmp, mreg(i), incr);
+gen_store(s, opsize, addr, tmp);
+tcg_temp_free(tmp);


This doesn't look right.  Is the value stored the intermediate value of the 
decremented register, or the final value?  What you're storing is reg-4, which 
is neither of these things.


I could see, maybe, that reg-4 might well turn out to be the right value for

movem   {a0-a7}, (sp)-

since sp == a7, and therefore stored first.  But I question that's the correct 
result for


movem   {a0-a7}, (a1)-

If it's the incremental value, then you can just store "addr" and you don't 
need a temp.  If it's the final value, then you can compute


tcg_gen_subi_i32(tmp, AREG(insn, 0), ctpop32(mask) * 4);



r~

Re: [Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-03 Thread Richard Henderson 

On 11/02/2016 03:15 PM, Laurent Vivier wrote:

680x0 movem can load/store words and long words
and can use more addressing modes.
Coldfire can only use long words with (Ax) and (d16,Ax)
addressing modes.

Signed-off-by: Laurent Vivier 
---
 target-m68k/translate.c | 96 -
 1 file changed, 79 insertions(+), 17 deletions(-)


Reviewed-by: Richard Henderson 


r~

[Qemu-devel] [PATCH v2 2/3] target-m68k: implement 680x0 movem

2016-11-02 Thread Laurent Vivier 
680x0 movem can load/store words and long words
and can use more addressing modes.
Coldfire can only use long words with (Ax) and (d16,Ax)
addressing modes.

Signed-off-by: Laurent Vivier 
---
 target-m68k/translate.c | 96 -
 1 file changed, 79 insertions(+), 17 deletions(-)

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 1cf88a4..93f1270 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -1667,14 +1667,25 @@ static void gen_push(DisasContext *s, TCGv val)
 tcg_gen_mov_i32(QREG_SP, tmp);
 }
 
+static TCGv mreg(int reg)
+{
+if (reg < 8) {
+/* Dx */
+return cpu_dregs[reg];
+}
+/* Ax */
+return cpu_aregs[reg & 7];
+}
+
 DISAS_INSN(movem)
 {
 TCGv addr;
 int i;
 uint16_t mask;
-TCGv reg;
 TCGv tmp;
-int is_load;
+int is_load = (insn & 0x0400) != 0;
+int opsize = (insn & 0x40) != 0 ? OS_LONG : OS_WORD;
+TCGv incr;
 
 mask = read_im16(env, s);
 tmp = gen_lea(env, s, insn, OS_LONG);
@@ -1682,25 +1693,74 @@ DISAS_INSN(movem)
 gen_addr_fault(s);
 return;
 }
+
 addr = tcg_temp_new();
 tcg_gen_mov_i32(addr, tmp);
-is_load = ((insn & 0x0400) != 0);
-for (i = 0; i < 16; i++, mask >>= 1) {
-if (mask & 1) {
-if (i < 8)
-reg = DREG(i, 0);
-else
-reg = AREG(i, 0);
-if (is_load) {
-tmp = gen_load(s, OS_LONG, addr, 0);
-tcg_gen_mov_i32(reg, tmp);
-} else {
-gen_store(s, OS_LONG, addr, reg);
+incr = tcg_const_i32(opsize_bytes(opsize));
+
+if (is_load) {
+/* memory to register */
+TCGv r[16];
+for (i = 0; i < 16; i++) {
+if ((mask >> i) & 1) {
+r[i] = gen_load(s, opsize, addr, 1);
+tcg_gen_add_i32(addr, addr, incr);
+}
+}
+for (i = 0; i < 16; i++, mask >>= 1) {
+if (mask & 1) {
+tcg_gen_mov_i32(mreg(i), r[i]);
+tcg_temp_free(r[i]);
+}
+}
+if ((insn & 070) == 030) {
+/* movem (An)+,X */
+tcg_gen_mov_i32(AREG(insn, 0), addr);
+}
+
+} else {
+/* register to memory */
+
+if ((insn & 070) == 040) {
+/* movem X,-(An) */
+
+for (i = 15; i >= 0; i--, mask >>= 1) {
+if (mask & 1) {
+if ((insn & 7) + 8 == i &&
+m68k_feature(s->env, M68K_FEATURE_EXT_FULL)) {
+/* M68020+: if the addressing register is the
+ * register moved to memory, the value written
+ * is the initial value decremented by the size of
+ * the operation
+ * M68000/M68010: the value is the initial value
+ */
+TCGv tmp = tcg_temp_new();
+tcg_gen_sub_i32(tmp, mreg(i), incr);
+gen_store(s, opsize, addr, tmp);
+tcg_temp_free(tmp);
+} else {
+gen_store(s, opsize, addr, mreg(i));
+}
+if (mask != 1) {
+tcg_gen_sub_i32(addr, addr, incr);
+}
+}
+}
+tcg_gen_mov_i32(AREG(insn, 0), addr);
+} else {
+/* movem X,(An)+ is not allowed */
+
+for (i = 0; i < 16; i++, mask >>= 1) {
+if (mask & 1) {
+gen_store(s, opsize, addr, mreg(i));
+tcg_gen_add_i32(addr, addr, incr);
+}
 }
-if (mask != 1)
-tcg_gen_addi_i32(addr, addr, 4);
 }
 }
+
+tcg_temp_free(incr);
+tcg_temp_free(addr);
 }
 
 DISAS_INSN(bitop_im)
@@ -3858,7 +3918,9 @@ void register_m68k_insns (CPUM68KState *env)
 BASE(pea,   4840, ffc0);
 BASE(swap,  4840, fff8);
 INSN(bkpt,  4848, fff8, BKPT);
-BASE(movem, 48c0, fbc0);
+INSN(movem, 48d0, fbf8, CF_ISA_A);
+INSN(movem, 48e8, fbf8, CF_ISA_A);
+INSN(movem, 4880, fb80, M68000);
 BASE(ext,   4880, fff8);
 BASE(ext,   48c0, fff8);
 BASE(ext,   49c0, fff8);
-- 
2.7.4