[51/52] target-m68k: add cmpm
diff mbox

Message ID 1462396869-22424-12-git-send-email-laurent@vivier.eu
State New
Headers show

Commit Message

Laurent Vivier May 4, 2016, 9:21 p.m. UTC
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 target-m68k/translate.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

Comments

Richard Henderson May 6, 2016, 10 p.m. UTC | #1
On 05/04/2016 11:21 AM, Laurent Vivier wrote:
> +    reg = AREG(insn, 0);
> +    src = gen_load(s, opsize, reg, 1);
> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
> +
> +    reg = AREG(insn, 9);
> +    dest = gen_load(s, opsize, reg, 1);
> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));

Delay the writeback to the first areg until after the second load.


r~
Laurent Vivier May 7, 2016, 7:01 p.m. UTC | #2
Le 07/05/2016 à 00:00, Richard Henderson a écrit :
> On 05/04/2016 11:21 AM, Laurent Vivier wrote:
>> +    reg = AREG(insn, 0);
>> +    src = gen_load(s, opsize, reg, 1);
>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>> +
>> +    reg = AREG(insn, 9);
>> +    dest = gen_load(s, opsize, reg, 1);
>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
> 
> Delay the writeback to the first areg until after the second load.

We can't delay because we can have "cmpm (%a0)+,(%a0)+" that is used to
compare two consecutive memory contents.

Laurent
Peter Maydell May 7, 2016, 9:50 p.m. UTC | #3
On 7 May 2016 at 20:01, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 07/05/2016 à 00:00, Richard Henderson a écrit :
>> On 05/04/2016 11:21 AM, Laurent Vivier wrote:
>>> +    reg = AREG(insn, 0);
>>> +    src = gen_load(s, opsize, reg, 1);
>>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>>> +
>>> +    reg = AREG(insn, 9);
>>> +    dest = gen_load(s, opsize, reg, 1);
>>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>>
>> Delay the writeback to the first areg until after the second load.
>
> We can't delay because we can have "cmpm (%a0)+,(%a0)+" that is used to
> compare two consecutive memory contents.

If you write back to the first areg before the second
load, don't you get the wrong value as seen by the
exception handler if the second load faults?

Usually you want to use the updated value for the
purposes of calculating the address to use in the
second load, but you don't want to actually update
the guest CPU register until after the load has
happened, in case it faults.

(Disclaimer: I'm just assuming that on a fault no
registers are updated, but if that wasn't the case the OS
wouldn't be able to cleanly restart the instruction after
fixing up a page fault, so it seems like a good guess.)

thanks
-- PMM
Laurent Vivier May 8, 2016, 9:07 a.m. UTC | #4
Le 07/05/2016 à 23:50, Peter Maydell a écrit :
> On 7 May 2016 at 20:01, Laurent Vivier <laurent@vivier.eu> wrote:
>>
>>
>> Le 07/05/2016 à 00:00, Richard Henderson a écrit :
>>> On 05/04/2016 11:21 AM, Laurent Vivier wrote:
>>>> +    reg = AREG(insn, 0);
>>>> +    src = gen_load(s, opsize, reg, 1);
>>>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>>>> +
>>>> +    reg = AREG(insn, 9);
>>>> +    dest = gen_load(s, opsize, reg, 1);
>>>> +    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
>>>
>>> Delay the writeback to the first areg until after the second load.
>>
>> We can't delay because we can have "cmpm (%a0)+,(%a0)+" that is used to
>> compare two consecutive memory contents.
> 
> If you write back to the first areg before the second
> load, don't you get the wrong value as seen by the
> exception handler if the second load faults?
> 
> Usually you want to use the updated value for the
> purposes of calculating the address to use in the
> second load, but you don't want to actually update
> the guest CPU register until after the load has
> happened, in case it faults.
> 
> (Disclaimer: I'm just assuming that on a fault no
> registers are updated, but if that wasn't the case the OS
> wouldn't be able to cleanly restart the instruction after
> fixing up a page fault, so it seems like a good guess.)

You're right.

But I didn't really care here of the page faults because m68k only
supports non privileged instructions: coldfire is used in semi-hosting
mode (no MMU) and 680x0-like machine has no hardware (I use it in
linux-user mode).

I'm working on a Quadra 800 [1] emulation where I have added all the
needed hardware and privileged instructions, and of course, I have to
re-work all the memory access because of what you have described above...

But I'm going to try to update this series taking care of the memory
access fault as much as possible.

Laurent

[1] https://github.com/vivier/qemu-m68k/tree/q800-v2.4.0
Peter Maydell May 8, 2016, 10:44 a.m. UTC | #5
On 8 May 2016 at 10:07, Laurent Vivier <laurent@vivier.eu> wrote:
> But I didn't really care here of the page faults because m68k only
> supports non privileged instructions: coldfire is used in semi-hosting
> mode (no MMU) and 680x0-like machine has no hardware (I use it in
> linux-user mode).

It does still affect linux-user if your guest program has a
SIGSEGV handler and uses it to fix up and restart faulting
instructions, but I think only a few obscure JIT-type apps do
this.

thanks
-- PMM

Patch
diff mbox

diff --git a/target-m68k/translate.c b/target-m68k/translate.c
index 029c166..2d92bdd 100644
--- a/target-m68k/translate.c
+++ b/target-m68k/translate.c
@@ -2758,6 +2758,26 @@  DISAS_INSN(cmpa)
     gen_update_cc_cmp(s, reg, src, opsize);
 }
 
+DISAS_INSN(cmpm)
+{
+    TCGv src;
+    TCGv dest;
+    TCGv reg;
+    int opsize;
+
+    opsize = insn_opsize(insn);
+
+    reg = AREG(insn, 0);
+    src = gen_load(s, opsize, reg, 1);
+    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
+
+    reg = AREG(insn, 9);
+    dest = gen_load(s, opsize, reg, 1);
+    tcg_gen_addi_i32(reg, reg, opsize_bytes(opsize));
+
+    gen_update_cc_cmp(s, dest, src, opsize);
+}
+
 DISAS_INSN(eor)
 {
     TCGv src;
@@ -4876,6 +4896,7 @@  void register_m68k_insns (CPUM68KState *env)
     INSN(cmpa,      b1c0, f1c0, CF_ISA_A);
     INSN(cmp,       b000, f100, M68000);
     INSN(eor,       b100, f100, M68000);
+    INSN(cmpm,      b108, f138, M68000);
     INSN(cmpa,      b0c0, f0c0, M68000);
     INSN(eor,       b180, f1c0, CF_ISA_A);
     BASE(and,       c000, f000);