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

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
>