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: 
>>>> 20241229-m68k-semihosting-v1-1-db131e2b5212@flygoat.com">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