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

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