Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode

[Aleksandar Markovic]

> From: Peter Maydell <address@hidden>
> Subject: Re: [Qemu-devel] [PATCH 5/6] Determine the desired FPU mode
> 
> On 26 October 2018 at 15:21, Stefan Markovic <address@hidden> wrote:
> > From: Stefan Markovic <address@hidden>
> >
> > Floating-point mode is calculated from MIPS.abiflags FP ABI value
> > (based on kernel implementation). Illegal combinations are rejected.
> >
> > Signed-off-by: Stefan Markovic <address@hidden>
> > ---
> >  linux-user/mips/cpu_loop.c | 75 
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 75 insertions(+)
> 
> > +     if ((info->fp_abi > MAX_FP_ABI && info->fp_abi != MIPS_ABI_FP_UNKNOWN)
> > +        || (info->interp_fp_abi > MAX_FP_ABI &&
> > +            info->interp_fp_abi != MIPS_ABI_FP_UNKNOWN)) {
> > +        fprintf(stderr, "qemu: Program and interpreter have "
> > +                        "unexpected FPU modes\n");
> > +        exit(137);
> 
> Why are we exit()ing with a funny exit status code here?
> 
> If this is a "can't happen" case, then we should assert(). If
> it is a "can happen if fed an odd binary" case, then we should just
> exit(1) as we do already in this function for an unsupported NaN mode.
> 

Thanks for the review.

This is a "can happen if fed an odd binary" case. Or, in other words, and more 
precisely, an executable compiled with one FP option attempts to load a library 
compiled with another, incompatible, FP option.

Kernel counterpart lines are:

https://elixir.bootlin.com/linux/v4.19/source/arch/mips/kernel/elf.c#L211
https://elixir.bootlin.com/linux/v4.19/source/arch/mips/kernel/elf.c#L263

I think the error code is important for MIPS loader to work as designed in such 
cases. Stefan should be best positioned to explain and analyze the cases, since 
he worked on verifying and fixing involved scenarios, not only from QEMU 
perspective. However, he will be back most likely only on Monday.

Thanks again,
Aleksandar

> > +    }
> > +
> > +    prog_req = (info->fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> > +                                            : fpu_reqs[info->fp_abi];
> > +    interp_req = (info->interp_fp_abi == MIPS_ABI_FP_UNKNOWN) ? none_req
> > +                                            : 
> > fpu_reqs[info->interp_fp_abi];
> > +
> > +    prog_req.single &= interp_req.single;
> > +    prog_req.soft &= interp_req.soft;
> > +    prog_req.fr1 &= interp_req.fr1;
> > +    prog_req.frdefault &= interp_req.frdefault;
> > +    prog_req.fre &= interp_req.fre;
> > +
> > +    bool cpu_has_mips_r2_r6 = env->insn_flags & ISA_MIPS32R2 ||
> > +                              env->insn_flags & ISA_MIPS64R2 ||
> > +                              env->insn_flags & ISA_MIPS32R6 ||
> > +                              env->insn_flags & ISA_MIPS64R6;
> > +
> > +    if (prog_req.fre && !prog_req.frdefault && !prog_req.fr1) {
> > +        env->CP0_Config5 |= (1 << CP0C5_FRE);
> > +        if (env->active_fpu.fcr0 & (1 << FCR0_FREP)) {
> > +            env->hflags |= MIPS_HFLAG_FRE;
> > +        }
> > +    } else if ((prog_req.fr1 && prog_req.frdefault) ||
> > +         (prog_req.single && !prog_req.frdefault)) {
> > +        if ((env->active_fpu.fcr0 & (1 << FCR0_F64)
> > +            && cpu_has_mips_r2_r6) || prog_req.fr1) {
> > +            env->CP0_Status |= (1 << CP0St_FR);
> > +            env->hflags |= MIPS_HFLAG_F64;
> > +        }
> > +    } else  if (!prog_req.fre && !prog_req.frdefault &&
> > +          !prog_req.fr1 && !prog_req.single && !prog_req.soft) {
> > +        exit(137);
> > +    }
> 
> Ditto here (and we haven't printed any error message here...)
> 
> thanks
> -- PMM
>