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

["Alex Bennée" <alex.bennee@linaro.org>]

BALATON Zoltan <balaton@eik.bme.hu> writes:

> 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: 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?

I'm getting confused at cs->exception_index already being looked at in
multiple places:

  -*- mode:grep; default-directory: "/home/alex/lsrc/qemu.git/target/m68k/" -*-


  12 candidates:
  ./op_helper.c:200:        switch (cs->exception_index) {
  ./op_helper.c:211:    vector = cs->exception_index << 2;
  ./op_helper.c:217:                 ++count, m68k_exception_name(cs->exception_index),
  ./op_helper.c:266:        cpu_stw_mmuidx_ra(env, *sp, (format << 12) + (cs->exception_index << 2),
  ./op_helper.c:283:        switch (cs->exception_index) {
  ./op_helper.c:291:    vector = cs->exception_index << 2;
  ./op_helper.c:297:                 ++count, m68k_exception_name(cs->exception_index),
  ./op_helper.c:322:    switch (cs->exception_index) {

So I'm not sure splitting a case makes it easier to follow. Exceptions
are under the control of the translator - is it possible to re-factor
the code to keep the switch of all cs->exception_index cases in one
place and assert if the translator has generated one it shouldn't have?


>
> 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,
>>

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro