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

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

[Jiaxun Yang]

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);
+
+    if (!is_hw) {
+        switch (cs->exception_index) {
+        case EXCP_SEMIHOSTING:
+            do_m68k_semihosting(env, env->dregs[0]);
+            return;
+        }
+    }
     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,
-- 
Jiaxun Yang <jiaxun.yang@flygoat.com>

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

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

[Jiaxun Yang]

在2024年12月29日十二月 下午3:15，BALATON Zoltan写道：
[...]
>
> Also why use switch for a single case? Why not write
>
> if (!is_hw && cs->exception_index == EXCP_SEMIHOSTING)

Mostly for clarity and matching the style above, see:

    if (!is_hw) {
        switch (cs->exception_index) {
        case EXCP_RTE:
            /* Return from an exception.  */
            m68k_rte(env);
            return;
        }
    }

Thanks

>
> instead?
>
[...]
>>

-- 
- Jiaxun

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

[Alex Bennée]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 4039 bytes --]

On Sun, 29 Dec 2024, Alex Bennée wrote:
> 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?

Looks like there are two versions of *_interrupt_all, one for ColdFire and 
one for older m68k. The SEMIHOSTING and RTE exceptions are handled at the 
beginning but m86k only handled RTE so far. Both of the versions are 
called from do_interrupt_all so moving this switch there with both cases 
would add SEMIHOSTING to m68k as well which is I think what this patch 
tries to do. So you'd need to move the whole switch with both cases not 
just the SEMIHOSTING one to do_interrupt_all. At least if I understand it 
correctly but maybe I also got lost and did not follow this closely so I 
could be wrong.

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

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

[Jiaxun Yang]

在2024年12月29日十二月 下午10:30，BALATON Zoltan写道：
> On Sun, 29 Dec 2024, Alex Bennée wrote:
>> 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
>>>> ---
[...]
>>
>>  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?

I'm not really deep in this port but I think it's pretty hard to determine
a proper way to do assertion, we have some exceptions that should only be
handled when !is_hw, some should be handled both case, and the switch at
end of handling function have a default clause which makes it even harder
to determine a valid range.

>
> Looks like there are two versions of *_interrupt_all, one for ColdFire and 
> one for older m68k. The SEMIHOSTING and RTE exceptions are handled at the 
> beginning but m86k only handled RTE so far. Both of the versions are 
> called from do_interrupt_all so moving this switch there with both cases 
> would add SEMIHOSTING to m68k as well which is I think what this patch 
> tries to do. So you'd need to move the whole switch with both cases not 
> just the SEMIHOSTING one to do_interrupt_all. At least if I understand it 
> correctly but maybe I also got lost and did not follow this closely so I 
> could be wrong.

Yes, in PATCH v1 I attempted to just add semihosting to m68k case and Richard
suggested that I can move whole semihosting block to do_interrupt_all.

You can't move rte to do_interrupt_all as the handling function is different
for coldfire and m68k.

IMO PATCH v1 is the best to move forward without doing anything awkward.

This is breaking picolibc's CI and I think a quick fix that can be easily
backported to stable would be helpful.

Thanks

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

-- 
- Jiaxun