Re: [PATCH v4 2/6] target/m68k: Fix fsave/frestore for 68881 FPU

[BALATON Zoltan]

Hello,

On Wed, 19 Aug 2026, Purr Box wrote:
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.

Update your script to also run qemu/scripts/checkpatch.pl or do that 
before submission. See 
https://www.qemu.org/docs/master/devel/submitting-a-patch.html
That should catch things like missing Signed-off-by that I've just 
noticed.

Regards,
BALATON Zoltan

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 <bala...@eik.bme.hu> 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/20260503015756.99176-1-54weas...@gmail.com/) 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