Hi Zoltan, Thanks for the review! I've refactored the logic for v5. I removed the redundant M68040 checks in frestore (keeping just the FIXME), and streamlined fsave so that it uniformly checks for M68K_FEATURE_FPU, writing an IDLE frame (0x41000000) for the 68040 and a NULL frame (0x00000000) for the 68881/68882. Also, thanks for the tip on the Context: tag. I've updated my script to inject it below the --- separator in the .patch files so it stays off the permanent git commit log while remaining visible to reviewers.
I'll wait a bit longer to see if there are comments on the rest of the series before spinning new patches. Dan. On Tue, Aug 18, 2026 at 3:52 AM BALATON Zoltan <[email protected]> wrote: > On Mon, 17 Aug 2026, 54weasels wrote: > > High level description: > > The 68881/68882 FPUs require fsave/frestore instructions to read/write > state frames. QEMU only fully implemented this for the 68040. The Sun-3 > uses a 68881 FPU, and the boot ROM executes `fsave` to probe FPU presence. > This patch adds basic 68881 FPU state frame handling (NULL frame) to > accurately simulate an idle FPU state. > > > > Impact on existing functionality: > > Fixes FPU detection for 68881/68882 across all M68k boards without > affecting 68040 specific state logic. > > Comments such as the next Context: should go below the --- as everything > before that is part of the commit message. In QEMU a Context: tag is not > commonly used so I think you want this as a comment not part of the > commit message. > > > Context: This patch was originally submitted as part of the monolithic > Sun-3 Machine Emulation series ( > https://patchew.org/QEMU/[email protected]/) and > has been split into atomic components. > > --- > > target/m68k/translate.c | 20 ++++++++++++++++---- > > 1 file changed, 16 insertions(+), 4 deletions(-) > > > > diff --git a/target/m68k/translate.c b/target/m68k/translate.c > > index 138c89d3e5..bdf619883c 100644 > > --- a/target/m68k/translate.c > > +++ b/target/m68k/translate.c > > @@ -5364,11 +5364,19 @@ DISAS_INSN(frestore) > > gen_exception(s, s->base.pc_next, EXCP_PRIVILEGE); > > return; > > } > > - if (m68k_feature(s->env, M68K_FEATURE_M68040)) { > > + if (m68k_feature(s->env, M68K_FEATURE_M68040) || > > + m68k_feature(s->env, M68K_FEATURE_FPU)) { > > M68020+ (including M68040) has FPU set so no need to keep that test and > can be replaced with M68K_FEATURE_FPU. > > > SRC_EA(env, addr, OS_LONG, 0, NULL); > > - /* FIXME: check the state frame */ > > + if (m68k_feature(s->env, M68K_FEATURE_M68040)) { > > + /* FIXME: check the state frame */ > > + } else { > > What about M68060 or other CPUs with built-in FPU? As none of these do > anything maybe the if/else is not needed and can just keep the FIXME > comment? > > > + /* > > + * 68881/68882 FRESTORE: read the state frame > > + * (NULL frame is 4 bytes) > > + */ > > + } > > } else { > > - disas_undef(env, s, insn); > > + disas_undef_fpu(env, s, insn); > > } > > } > > > > @@ -5383,8 +5391,12 @@ DISAS_INSN(fsave) > > /* always write IDLE */ > > TCGv idle = tcg_constant_i32(0x41000000); > > DEST_EA(env, insn, OS_LONG, idle, NULL); > > + } else if (m68k_feature(s->env, M68K_FEATURE_FPU)) { > > + /* 68881/68882 FSAVE: always write NULL frame */ > > + TCGv null_frame = tcg_constant_i32(0x00000000); > > + DEST_EA(env, insn, OS_LONG, null_frame, NULL); > > Same comment as above about CPUs with FPU. Maybe there should be only one > block for M68K_FEATURE_FPU replacing M68K_FEATURE_M68040 and a switch for > setting the frame value? > > Regards, > BALATON Zoltan > > > } else { > > - disas_undef(env, s, insn); > > + disas_undef_fpu(env, s, insn); > > } > > } > > #endif > > >
