[PATCH 0/2] target/m68k: Enable halt insn for 68060

[PATCH 0/2] target/m68k: Enable halt insn for 68060

[Richard Henderson]

While looking at semihosting, I noticed that 060 does have halt.

Then I was a bit surprised that it wasn't being disassembled and
thought trying to exactly match disassembly to cpu was a bit
frought with peril -- one also wants to know what the insn was
*supposed* to be when it raises SIGILL.


r~


Richard Henderson (2):
  target/m68k: Clear mach in m68k_cpu_disas_set_info
  target/m68k: Enable halt insn for 68060

 target/m68k/cpu.c       | 6 +-----
 target/m68k/translate.c | 1 +
 2 files changed, 2 insertions(+), 5 deletions(-)

-- 
2.34.1

[PATCH 1/2] target/m68k: Clear mach in m68k_cpu_disas_set_info

[Richard Henderson]

Zero selects all cpu features in disas/m68k.c,
which is really what we want -- not limited to 68040.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/cpu.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
index c7aeb7da9c..5671067923 100644
--- a/target/m68k/cpu.c
+++ b/target/m68k/cpu.c
@@ -75,12 +75,8 @@ static void m68k_cpu_reset(DeviceState *dev)
 
 static void m68k_cpu_disas_set_info(CPUState *s, disassemble_info *info)
 {
-    M68kCPU *cpu = M68K_CPU(s);
-    CPUM68KState *env = &cpu->env;
     info->print_insn = print_insn_m68k;
-    if (m68k_feature(env, M68K_FEATURE_M68000)) {
-        info->mach = bfd_mach_m68040;
-    }
+    info->mach = 0;
 }
 
 /* CPU models */
-- 
2.34.1

Re: [PATCH 1/2] target/m68k: Clear mach in m68k_cpu_disas_set_info

[Laurent Vivier]

Le 30/04/2022 à 19:02, Richard Henderson a écrit :
> Zero selects all cpu features in disas/m68k.c,
> which is really what we want -- not limited to 68040.

But what happens when an instruction has to be decoded differently between 680x0 and coldfire?

for instance in disas/m68k.c, we have:

{"addil", 6,    one(0003200),   one(0177700), "#l$s", m68000up },
{"addil", 6,    one(0003200),   one(0177700), "#lDs", mcfisa_a },

{"addl", 6, one(0003200),   one(0177700), "#l$s", m68000up },
{"addl", 6, one(0003200),   one(0177700), "#lDs", mcfisa_a },

{"andil", 6,    one(0001200),   one(0177700), "#l$s", m68000up },
{"andil", 6,    one(0001200),   one(0177700), "#lDs", mcfisa_a },

{"andl", 6, one(0001200),   one(0177700), "#l$s", m68000up },
{"andl", 6, one(0001200),   one(0177700), "#lDs", mcfisa_a },

{"bchg", 4, one(0004100),   one(0177700), "#b$s", m68000up },
{"bchg", 4, one(0004100),   one(0177700), "#bqs", mcfisa_a },

{"bclr", 4, one(0004200),   one(0177700), "#b$s", m68000up },
{"bclr", 4, one(0004200),   one(0177700), "#bqs", mcfisa_a },

{"bset", 2, one(0000700),   one(0170700), "Dd$s", m68000up | mcfisa_a },
{"bset", 2, one(0000700),   one(0170700), "Ddvs", mcfisa_a },
{"bset", 4, one(0004300),   one(0177700), "#b$s", m68000up },
{"bset", 4, one(0004300),   one(0177700), "#bqs", mcfisa_a },

{"btst", 4, one(0004000),   one(0177700), "#b@s", m68000up },
{"btst", 4, one(0004000),   one(0177700), "#bqs", mcfisa_a },

{"cmpib", 4,    one(0006000),   one(0177700), "#b@s", m68000up },
{"cmpib", 4,    one(0006000),   one(0177700), "#bDs", mcfisa_b },
{"cmpiw", 4,    one(0006100),   one(0177700), "#w@s", m68000up },
{"cmpiw", 4,    one(0006100),   one(0177700), "#wDs", mcfisa_b },
{"cmpil", 6,    one(0006200),   one(0177700), "#l@s", m68000up },
{"cmpil", 6,    one(0006200),   one(0177700), "#lDs", mcfisa_a },

...

Thanks,
Laurent

> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/cpu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index c7aeb7da9c..5671067923 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -75,12 +75,8 @@ static void m68k_cpu_reset(DeviceState *dev)
>   
>   static void m68k_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>   {
> -    M68kCPU *cpu = M68K_CPU(s);
> -    CPUM68KState *env = &cpu->env;
>       info->print_insn = print_insn_m68k;
> -    if (m68k_feature(env, M68K_FEATURE_M68000)) {
> -        info->mach = bfd_mach_m68040;
> -    }
> +    info->mach = 0;
>   }
>   
>   /* CPU models */

Re: [PATCH 1/2] target/m68k: Clear mach in m68k_cpu_disas_set_info

[Richard Henderson]

On 5/17/22 07:06, Laurent Vivier wrote:
> Le 30/04/2022 à 19:02, Richard Henderson a écrit :
>> Zero selects all cpu features in disas/m68k.c,
>> which is really what we want -- not limited to 68040.
> 
> But what happens when an instruction has to be decoded differently between 680x0 and 
> coldfire?
> 
> for instance in disas/m68k.c, we have:
> 
> {"addil", 6,    one(0003200),   one(0177700), "#l$s", m68000up },
> {"addil", 6,    one(0003200),   one(0177700), "#lDs", mcfisa_a },
> 
> {"addl", 6, one(0003200),   one(0177700), "#l$s", m68000up },
> {"addl", 6, one(0003200),   one(0177700), "#lDs", mcfisa_a },
> 
> {"andil", 6,    one(0001200),   one(0177700), "#l$s", m68000up },
> {"andil", 6,    one(0001200),   one(0177700), "#lDs", mcfisa_a },
> 
> {"andl", 6, one(0001200),   one(0177700), "#l$s", m68000up },
> {"andl", 6, one(0001200),   one(0177700), "#lDs", mcfisa_a },
> 
> {"bchg", 4, one(0004100),   one(0177700), "#b$s", m68000up },
> {"bchg", 4, one(0004100),   one(0177700), "#bqs", mcfisa_a },
> 
> {"bclr", 4, one(0004200),   one(0177700), "#b$s", m68000up },
> {"bclr", 4, one(0004200),   one(0177700), "#bqs", mcfisa_a },
> 
> {"bset", 2, one(0000700),   one(0170700), "Dd$s", m68000up | mcfisa_a },
> {"bset", 2, one(0000700),   one(0170700), "Ddvs", mcfisa_a },
> {"bset", 4, one(0004300),   one(0177700), "#b$s", m68000up },
> {"bset", 4, one(0004300),   one(0177700), "#bqs", mcfisa_a },
> 
> {"btst", 4, one(0004000),   one(0177700), "#b@s", m68000up },
> {"btst", 4, one(0004000),   one(0177700), "#bqs", mcfisa_a },
> 
> {"cmpib", 4,    one(0006000),   one(0177700), "#b@s", m68000up },
> {"cmpib", 4,    one(0006000),   one(0177700), "#bDs", mcfisa_b },
> {"cmpiw", 4,    one(0006100),   one(0177700), "#w@s", m68000up },
> {"cmpiw", 4,    one(0006100),   one(0177700), "#wDs", mcfisa_b },
> {"cmpil", 6,    one(0006200),   one(0177700), "#l@s", m68000up },
> {"cmpil", 6,    one(0006200),   one(0177700), "#lDs", mcfisa_a },

They're all compatible encodings, it's just that the CF ones are more restricted.  When 
debugging a SIGILL on CF, it would be nicer to see

	addl	a0@, d0

than

	.byte	xx, yy

in the -d in_asm dump.

r~

Re: [PATCH 1/2] target/m68k: Clear mach in m68k_cpu_disas_set_info

[Laurent Vivier]

Le 30/04/2022 à 19:02, Richard Henderson a écrit :
> Zero selects all cpu features in disas/m68k.c,
> which is really what we want -- not limited to 68040.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/cpu.c | 6 +-----
>   1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/target/m68k/cpu.c b/target/m68k/cpu.c
> index c7aeb7da9c..5671067923 100644
> --- a/target/m68k/cpu.c
> +++ b/target/m68k/cpu.c
> @@ -75,12 +75,8 @@ static void m68k_cpu_reset(DeviceState *dev)
>   
>   static void m68k_cpu_disas_set_info(CPUState *s, disassemble_info *info)
>   {
> -    M68kCPU *cpu = M68K_CPU(s);
> -    CPUM68KState *env = &cpu->env;
>       info->print_insn = print_insn_m68k;
> -    if (m68k_feature(env, M68K_FEATURE_M68000)) {
> -        info->mach = bfd_mach_m68040;
> -    }
> +    info->mach = 0;
>   }
>   
>   /* CPU models */

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 2/2] target/m68k: Enable halt insn for 68060

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 target/m68k/translate.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index 4026572ed8..e4efd988d2 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -6003,6 +6003,7 @@ void register_m68k_insns (CPUM68KState *env)
     INSN(tas,       4ac0, ffc0, M68000);
 #if defined(CONFIG_SOFTMMU)
     INSN(halt,      4ac8, ffff, CF_ISA_A);
+    INSN(halt,      4ac8, ffff, M68060);
 #endif
     INSN(pulse,     4acc, ffff, CF_ISA_A);
     BASE(illegal,   4afc, ffff);
-- 
2.34.1

Re: [PATCH 2/2] target/m68k: Enable halt insn for 68060

[Laurent Vivier]

Le 30/04/2022 à 19:02, Richard Henderson a écrit :
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   target/m68k/translate.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index 4026572ed8..e4efd988d2 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -6003,6 +6003,7 @@ void register_m68k_insns (CPUM68KState *env)
>       INSN(tas,       4ac0, ffc0, M68000);
>   #if defined(CONFIG_SOFTMMU)
>       INSN(halt,      4ac8, ffff, CF_ISA_A);
> +    INSN(halt,      4ac8, ffff, M68060);
>   #endif
>       INSN(pulse,     4acc, ffff, CF_ISA_A);
>       BASE(illegal,   4afc, ffff);


Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 0/2] target/m68k: Enable halt insn for 68060

[Laurent Vivier]

Le 30/04/2022 à 19:02, Richard Henderson a écrit :
> While looking at semihosting, I noticed that 060 does have halt.
> 
> Then I was a bit surprised that it wasn't being disassembled and
> thought trying to exactly match disassembly to cpu was a bit
> frought with peril -- one also wants to know what the insn was
> *supposed* to be when it raises SIGILL.
> 
> 
> r~
> 
> 
> Richard Henderson (2):
>    target/m68k: Clear mach in m68k_cpu_disas_set_info
>    target/m68k: Enable halt insn for 68060
> 
>   target/m68k/cpu.c       | 6 +-----
>   target/m68k/translate.c | 1 +
>   2 files changed, 2 insertions(+), 5 deletions(-)
> 

Series applied to my m68k-for-7.1 branch

Thanks,
Laurent