Re: [Qemu-devel] [PATCH] mips: Respect CP0.Status.CU1 for microMIPS FP branches

[Leon Alrae]

On 03/11/2014 19:08, Maciej W. Rozycki wrote:
> Make microMIPS FP branches respect CP0.Status.CU1 and trap with a 
> Coprocessor Unusable exception if COP1 has been disabled; also trap if 
> no FPU is present at all.
> 
> Standard MIPS FP instruction encodings have a more regular structure and 
> branches are covered with a single umbrella along other instructions.  
> This is not the case with the microMIPS encoding, this case has to be 
> taken care of explicitly here.  Code to do so has been copied from the 
> standard MIPS code handler for OPC_CP1, in `decode_opc'.
> 
> Problems arising from this bug will generally only show up on user 
> context switches in operating systems making use of lazy FP context 
> switches, such as Linux.  It will also more readily trigger if software 
> FPU emulation is used, either implicitly on a non-float CPU, or forced 
> on a hard-float CPU such as with the "nofpu" Linux kernel command line 
> argument.
> 
> The problem may have been easily missed because we have no hard-float 
> microMIPS CPU configuration present; in fact we have no microMIPS CPU 
> configuration of any kind present.
> 
> Signed-off-by: Maciej W. Rozycki <address@hidden>
> ---
> The latter problem is easily fixed though, with a patch I'll be sending 
> right away.  Meanwhile please apply this one.
> 
>   Maciej
> 
> qemu-umips-cu1-ex.diff
> Index: qemu-git-trunk/target-mips/translate.c
> ===================================================================
> --- qemu-git-trunk.orig/target-mips/translate.c       2014-10-27 
> 04:26:57.000000000 +0000
> +++ qemu-git-trunk/target-mips/translate.c    2014-10-27 04:45:22.838923200 
> +0000
> @@ -13170,8 +13170,13 @@ static void decode_micromips32_opc (CPUM
>              check_insn(ctx, ASE_MIPS3D);
>              /* Fall through */
>          do_cp1branch:
> -            gen_compute_branch1(ctx, mips32_op,
> -                                (ctx->opcode >> 18) & 0x7, imm << 1);
> +            if (env->CP0_Config1 & (1 << CP0C1_FP)) {

I'm wondering if this test is needed at all, I would expect that
check_cp1_enabled(ctx) is enough. Is it ever possible to have Status.CU1
set to 1 if FPU isn't present? In translate_init.c the 5Kc CPU (and 5KEc
you are introducing) is the only CPU without FPU that has Status.CU1
writeable, which I'm not sure if it's correct. Probably the best way
would be to check that on the real 5KEc which you seem to have handy :)

Since this test is used also in other places in existing code and
potential cleanup won't happen before 2.2 release due to incoming
hard-freeze, this patch looks good to me.

> +                check_cp1_enabled(ctx);
> +                gen_compute_branch1(ctx, mips32_op,
> +                                    (ctx->opcode >> 18) & 0x7, imm << 1);
> +            } else {
> +                generate_exception_err(ctx, EXCP_CpU, 1);
> +            }
>              break;
>          case BPOSGE64:
>          case BPOSGE32:
> 

Reviewed-by: Leon Alrae <address@hidden>

Regards,
Leon