Re: [PATCH v2] target/m68k: Handle EXCP_SEMIHOSTING for m68k class CPU

[BALATON Zoltan]

On Sun, 29 Dec 2024, Jiaxun Yang wrote:
> EXCP_SEMIHOSTING can be generated by m68k class CPU with
> HALT instruction, but it is never handled properly and cause
> guest fall into deadlock.
>
> Moving EXCE_SEMIHOSTING handling code to common do_interrupt_all
> routine to ensure it's handled for both CPU classes.
>
> Fixes: f161e723fdfd ("target/m68k: Perform the semihosting test during 
> translate")
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
> Changes in v2:
> - hoist both calls to do_interrupt_all (Richard)
> - Link to v1: 
> 20241229-m68k-semihosting-v1-1-db131e2b5212@flygoat.com">https://lore.kernel.org/r/20241229-m68k-semihosting-v1-1-db131e2b5212@flygoat.com
> ---
> target/m68k/op_helper.c | 12 +++++++++---
> 1 file changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/target/m68k/op_helper.c b/target/m68k/op_helper.c
> index 
> 15bad5dd46518c6e86b6273d4a2b26b3b6f991de..9dd76f540b4871d3d0ab0e95747c85434e5d677d
>  100644
> --- a/target/m68k/op_helper.c
> +++ b/target/m68k/op_helper.c
> @@ -202,9 +202,6 @@ static void cf_interrupt_all(CPUM68KState *env, int is_hw)
>             /* Return from an exception.  */
>             cf_rte(env);
>             return;
> -        case EXCP_SEMIHOSTING:
> -            do_m68k_semihosting(env, env->dregs[0]);
> -            return;
>         }
>     }
>
> @@ -422,6 +419,15 @@ static void m68k_interrupt_all(CPUM68KState *env, int 
> is_hw)
>
> static void do_interrupt_all(CPUM68KState *env, int is_hw)
> {
> +    CPUState *cs = env_cpu(env);

This could be within the if block if not needed elsewhere.

> +
> +    if (!is_hw) {
> +        switch (cs->exception_index) {
> +        case EXCP_SEMIHOSTING:
> +            do_m68k_semihosting(env, env->dregs[0]);
> +            return;

Also why use switch for a single case? Why not write

if (!is_hw && cs->exception_index == EXCP_SEMIHOSTING)

instead?

Regards,
BALATON Zoltan

> +        }
> +    }
>     if (m68k_feature(env, M68K_FEATURE_M68K)) {
>         m68k_interrupt_all(env, is_hw);
>         return;
>
> ---
> base-commit: 2b7a80e07a29074530a0ebc8005a418ee07b1faf
> change-id: 20241229-m68k-semihosting-2c49c86d3e3c
>
> Best regards,