| Message ID | 20241229-m68k-semihosting-v2-1-8a08b2d199a5@flygoat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] target/m68k: Handle EXCP_SEMIHOSTING for m68k class CPU | expand |
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, >
在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? > [...] >>
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, >>
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, >>> > >
在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, >>>> >> >>
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;
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(-) --- base-commit: 2b7a80e07a29074530a0ebc8005a418ee07b1faf change-id: 20241229-m68k-semihosting-2c49c86d3e3c Best regards,