[PATCH 0/3] target/m68k: Fix a few semihosting bugs

[PATCH 0/3] target/m68k: Fix a few semihosting bugs

[Keith Packard via]

The first two patches mirror similar patches I recently sent for nios2.

 1. Use correct parameter for EXIT (d1 instead of d0)
 2. Fix use of deposit64 in LSEEK (argument order was incorrect)

The second patch has also been submitted by Peter Maydell, it's
included here because it was required to get things working.

The final patch adds semihosting support for non-ColdFire processors
(which don't support the HALT instruction) by using BKPT #0 instead
(as per the m68k semihosting docs).

All of these have been tested using picolibc (patches for m68k support
there are moving upstream as well).

[PATCH 1/3] target/m68k: Pass semihosting arg to exit

[Keith Packard via]

Instead of using d0 (the semihost function number), use d1 (the
provide exit status).

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/m68k/m68k-semi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
index 88ad9ba814..12235759c7 100644
--- a/target/m68k/m68k-semi.c
+++ b/target/m68k/m68k-semi.c
@@ -130,8 +130,8 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
     args = env->dregs[1];
     switch (nr) {
     case HOSTED_EXIT:
-        gdb_exit(env->dregs[0]);
-        exit(env->dregs[0]);
+        gdb_exit(env->dregs[1]);
+        exit(env->dregs[1]);
 
     case HOSTED_OPEN:
         GET_ARG(0);
-- 
2.40.1

[PATCH 2/3] target/m68k: Fix semihost lseek offset computation

[Keith Packard via]

The arguments for deposit64 are (value, start, length, fieldval); this
appears to have thought they were (value, fieldval, start,
length). Reorder the parameters to match the actual function.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/m68k/m68k-semi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
index 12235759c7..12179bde38 100644
--- a/target/m68k/m68k-semi.c
+++ b/target/m68k/m68k-semi.c
@@ -166,7 +166,7 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
         GET_ARG64(2);
         GET_ARG64(3);
         semihost_sys_lseek(cs, m68k_semi_u64_cb, arg0,
-                           deposit64(arg2, arg1, 32, 32), arg3);
+                           deposit64(arg2, 32, 32, arg1), arg3);
         break;
 
     case HOSTED_RENAME:
-- 
2.40.1

[PATCH 3/3] target/m68k: Support semihosting on non-ColdFire targets

[Keith Packard via]

According to the m68k semihosting spec:

"The instruction used to trigger a semihosting request depends on the
 m68k processor variant.  On ColdFire, "halt" is used; on other processors
 (which don't implement "halt"), "bkpt #0" may be used."

Add support for non-CodeFire processors by matching BKPT #0
instructions. When semihosting is disabled, convert those
back to illegal op exceptions.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 target/m68k/cpu.h       |  1 +
 target/m68k/op_helper.c | 16 ++++++++++++++++
 target/m68k/translate.c |  4 ++++
 3 files changed, 21 insertions(+)

diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
index cf70282717..b741c50a8f 100644
--- a/target/m68k/cpu.h
+++ b/target/m68k/cpu.h
@@ -67,6 +67,7 @@
 
 #define EXCP_RTE            0x100
 #define EXCP_HALT_INSN      0x101
+#define EXCP_BKPT_INSN      0x102
 
 #define M68K_DTTR0   0
 #define M68K_DTTR1   1
diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
index 1ce850bbc5..2d89db6dde 100644
--- a/target/m68k/op_helper.c
+++ b/target/m68k/op_helper.c
@@ -295,6 +295,22 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
             /* Return from an exception.  */
             m68k_rte(env);
             return;
+        case EXCP_BKPT_INSN:
+            if (semihosting_enabled((env->sr & SR_S) == 0)
+                    && (env->pc & 3) == 0
+                    && cpu_lduw_code(env, env->pc - 4) == 0x4e71
+                    && cpu_ldl_code(env, env->pc) == 0x4e7bf000) {
+                env->pc += 4;
+                do_m68k_semihosting(env, env->dregs[0]);
+                return;
+            }
+            /*
+             * When semihosting is not enabled, translate this back to
+             * an illegal op exception.
+             */
+            cs->exception_index = EXCP_ILLEGAL;
+            env->pc += 2;
+            break;
         }
     }
 
diff --git a/target/m68k/translate.c b/target/m68k/translate.c
index e07161d76f..d037c57453 100644
--- a/target/m68k/translate.c
+++ b/target/m68k/translate.c
@@ -2640,6 +2640,10 @@ DISAS_INSN(bkpt)
 #if defined(CONFIG_USER_ONLY)
     gen_exception(s, s->base.pc_next, EXCP_DEBUG);
 #else
+    if ((insn & 7) == 0) {
+        gen_exception(s, s->pc, EXCP_BKPT_INSN);
+        return;
+    }
     gen_exception(s, s->base.pc_next, EXCP_ILLEGAL);
 #endif
 }
-- 
2.40.1

Re: [PATCH 0/3] target/m68k: Fix a few semihosting bugs

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> The first two patches mirror similar patches I recently sent for nios2.
>
>  1. Use correct parameter for EXIT (d1 instead of d0)
>  2. Fix use of deposit64 in LSEEK (argument order was incorrect)
>
> The second patch has also been submitted by Peter Maydell, it's
> included here because it was required to get things working.

The usual way we do that is that you include my patch in your
series, and add your signed-off-by line (indicating it has
come via you).

thanks
-- PMM

Re: [PATCH 1/3] target/m68k: Pass semihosting arg to exit

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> Instead of using d0 (the semihost function number), use d1 (the
> provide exit status).
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  target/m68k/m68k-semi.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/target/m68k/m68k-semi.c b/target/m68k/m68k-semi.c
> index 88ad9ba814..12235759c7 100644
> --- a/target/m68k/m68k-semi.c
> +++ b/target/m68k/m68k-semi.c
> @@ -130,8 +130,8 @@ void do_m68k_semihosting(CPUM68KState *env, int nr)
>      args = env->dregs[1];
>      switch (nr) {
>      case HOSTED_EXIT:
> -        gdb_exit(env->dregs[0]);
> -        exit(env->dregs[0]);
> +        gdb_exit(env->dregs[1]);
> +        exit(env->dregs[1]);

I looked at this code but didn't spot the bug, because I
got confused by the 'args = env->dregs[1]' line above somehow.

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 3/3] target/m68k: Support semihosting on non-ColdFire targets

[Peter Maydell]

On Wed, 2 Aug 2023 at 17:20, Keith Packard via <qemu-devel@nongnu.org> wrote:
>
> According to the m68k semihosting spec:
>
> "The instruction used to trigger a semihosting request depends on the
>  m68k processor variant.  On ColdFire, "halt" is used; on other processors
>  (which don't implement "halt"), "bkpt #0" may be used."
>
> Add support for non-CodeFire processors by matching BKPT #0
> instructions. When semihosting is disabled, convert those
> back to illegal op exceptions.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  target/m68k/cpu.h       |  1 +
>  target/m68k/op_helper.c | 16 ++++++++++++++++
>  target/m68k/translate.c |  4 ++++
>  3 files changed, 21 insertions(+)
>
> diff --git a/target/m68k/cpu.h b/target/m68k/cpu.h
> index cf70282717..b741c50a8f 100644
> --- a/target/m68k/cpu.h
> +++ b/target/m68k/cpu.h
> @@ -67,6 +67,7 @@
>
>  #define EXCP_RTE            0x100
>  #define EXCP_HALT_INSN      0x101
> +#define EXCP_BKPT_INSN      0x102
>
>  #define M68K_DTTR0   0
>  #define M68K_DTTR1   1
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 1ce850bbc5..2d89db6dde 100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -295,6 +295,22 @@ static void m68k_interrupt_all(CPUM68KState *env, int is_hw)
>              /* Return from an exception.  */
>              m68k_rte(env);
>              return;
> +        case EXCP_BKPT_INSN:
> +            if (semihosting_enabled((env->sr & SR_S) == 0)
> +                    && (env->pc & 3) == 0
> +                    && cpu_lduw_code(env, env->pc - 4) == 0x4e71
> +                    && cpu_ldl_code(env, env->pc) == 0x4e7bf000) {

This long condition is identical to the one used for
semihosting-via-halt. It could usefully be moved out
to a function, which can then have a comment noting
what it's doing (i.e. checking for the required insns
preceding and following the bkpt or halt).

> +                env->pc += 4;
> +                do_m68k_semihosting(env, env->dregs[0]);
> +                return;
> +            }
> +            /*
> +             * When semihosting is not enabled, translate this back to
> +             * an illegal op exception.
> +             */
> +            cs->exception_index = EXCP_ILLEGAL;
> +            env->pc += 2;
> +            break;
>          }
>      }
>
> diff --git a/target/m68k/translate.c b/target/m68k/translate.c
> index e07161d76f..d037c57453 100644
> --- a/target/m68k/translate.c
> +++ b/target/m68k/translate.c
> @@ -2640,6 +2640,10 @@ DISAS_INSN(bkpt)
>  #if defined(CONFIG_USER_ONLY)
>      gen_exception(s, s->base.pc_next, EXCP_DEBUG);
>  #else
> +    if ((insn & 7) == 0) {
> +        gen_exception(s, s->pc, EXCP_BKPT_INSN);
> +        return;
> +    }
>      gen_exception(s, s->base.pc_next, EXCP_ILLEGAL);

This means that semihosting-via-bpkt will only work
in system emulation, because in user mode the
EXCP_DEBUG will take precedence. To handle that you
need to also check in linux-user/m68k/cpu_loop.c. But
this effectively also implies reverting most of
a638af09b6c6b (where we took out a lot of "m68k
usermode semihosting" because it wasn't reachable).
So it's probably not worthwhile.

-- PMM

Re: [PATCH 0/3] target/m68k: Fix a few semihosting bugs

[Richard Henderson]

On 8/2/23 09:19, Keith Packard via wrote:
> The first two patches mirror similar patches I recently sent for nios2.
> 
>   1. Use correct parameter for EXIT (d1 instead of d0)
>   2. Fix use of deposit64 in LSEEK (argument order was incorrect)
> 
> The second patch has also been submitted by Peter Maydell, it's
> included here because it was required to get things working.
> 
> The final patch adds semihosting support for non-ColdFire processors
> (which don't support the HALT instruction) by using BKPT #0 instead
> (as per the m68k semihosting docs).
> 
> All of these have been tested using picolibc (patches for m68k support
> there are moving upstream as well).


Queued patch 1 to m68k-next.  Patch 2 has already been committed, and I've just sent a 
replacement for patch 3.


r~


PS: I hadn't noticed the picolibc note here at the time; I'll give that a try later today.