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

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

[Maciej W. Rozycki]

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 <macro@codesourcery.com>
---
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)) {
+                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:

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 <macro@codesourcery.com>
> ---
> 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 <leon.alrae@imgtec.com>

Regards,
Leon

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

[Maciej W. Rozycki]

On Wed, 5 Nov 2014, Leon Alrae wrote:

> > 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 :)

 Good point, CP0.Status.CU1 being writable on a non-FPU MIPS architecture 
(non-legacy) processor looks like a bug to me, I missed that detail.  
Here's what the original 5K manual[1] says:

"Coprocessor 1 and 2 can only be marked as usable if a
coprocessor is actually attached to the CPU.  For example,
if no coprocessor 2 is attached, software cannot set CU2.
Note that COP1X instructions are enabled by CU1.

"CU3 is unused by the processor but is implemented as a
read/write bit for backwards-compatibility.  This bit can
only be set to 1 if coprocessor 1 is attached to the CPU."

 I only have a bunch of 5KEf boards I'm afraid.  I could have a look at a 
5Kc sometime, but I don't have the system handy.  It may be easier for you 
to track down a 5K core card within Imagination somewhere.  I think the 
reference above should be enough though, so I'll cook up an update patch 
in the few next days, I think as an incremental one.

> 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.

 That sounds reasonable to me and thanks for the heads-up about the 
impending code freeze.  I think I'll post all the outstanding changes 
regardless to get the reviews rolling as the less obvious stuff may 
require further work and I'd like to get them integrated as soon as 2.3 
opens then.

 Now as to CP0.Status.CU1, while fixing the 5Kc and 5KEc processors is an 
obvious change, I think the removal of the extra check may not be such.  
The thing is in the original architecture -- and it still stands for CP2 
-- these bits used to control external coprocessors that may or may not 
have been present.  For example to have an R3000 processor with an FPU you 
wired an external R3010 unit to it.  Consequently all the CP0.Status.CUx 
bits were always writable and the relevant logic to intereact with the 
external chip enabled when requested.

 And there is software that relies on this property as prior to the 
introduction of the modern MIPS architecture there was no CP0.Config1 
register present to check the presence of an FPU with.  Instead what 
software was supposed to do was to enable CP1, execute a CFC instruction 
to read CP1.FIR that was supposed not to trap under these circumstances, 
and check the value obtained in the Imp field (as it was then called) aka 
ProcessorID.  If that was non-zero, an FPU was present, if it was zero 
(due to the floating bus being strapped IIUC), no FPU was available. [2] 
For example we have code in Linux doing this when run on the relevant 
hardware.

 Obviously COP1X instructions would trap on CP0.Status.CU3 instead and for 
example the FP emulator that we have in Linux is prepared for this 
situation (whether it should really emulate a full MIPS IV if not higher 
FP instruction set on a lower-ISA processor is another question).

 And I think all this draws the "right" implementation that we can make, 
across all the three coprocessors actually, e.g. for CP1, except stores 
and transfers from:

    check_cp1_enabled(ctx);
    if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
        handle_the_op();
    }

for stores and transfers from:

    check_cp1_enabled(ctx);
    if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
        write_destination(read(source));
    } else {
        write_destination(0);
    }

That is for an absent unit stores and transfers write 0 to their 
destination and all other instructions are treated as NOPs.

 For CP2 that we don't have support for our implementation would be dummy, 
omitting the conditional check along the following "then" part, and for 
CP3 the corresponding enable check would have to take ISA specifics into 
account.

 There's one further complication in that doubleword load/store major 
opcodes cause a reserved instruction exception instead on MIPS I 
processors.  Though actually I have a patch outstanding that handles this 
along overall tightening of instruction ISA level validity checks.

 Of course right now we don't have such legacy processors included in 
configuration, but the clean-up looks to me straightforward and little 
involving.  Actually including an R3000 processor with an FPU would be a 
bit trickier as they used a general interrupt line for exception delivery 
and we'd have to implement that; some that had the FPU coupled with the 
CPU on a single die, such as the R3081, even had it software-configurable, 
with a field in a CP0.Config register in the latter case (a different 
CP0.Config to what it is these days, at $3). [3]

 Then of course we'll have to make sure CP0.Status.CUx bits are hardwired 
to 0 where appropriate on modern MIPS architecture processors.

 Thanks for your review.

References:

[1] "MIPS64 5K Processor Core Family Software User's Manual",
     MIPS Technologies, Inc., Document Number: MD00012, Revision 02.08, 
     May 28, 2002

[2] "IDT MIPS Microprocessor Family Software Reference Manual",
    Integrated Device Technology, Inc., Version 2.0, October 1996

[3] "IDT R30xx Family Software Reference Manual",
    Integrated Device Technology, Inc., Revision 1.0, 1994

  Maciej

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

[Leon Alrae]

On 05/11/2014 20:16, Maciej W. Rozycki wrote:
>  Now as to CP0.Status.CU1, while fixing the 5Kc and 5KEc processors is an 
> obvious change, I think the removal of the extra check may not be such.  
> The thing is in the original architecture -- and it still stands for CP2 
> -- these bits used to control external coprocessors that may or may not 
> have been present.  For example to have an R3000 processor with an FPU you 
> wired an external R3010 unit to it.  Consequently all the CP0.Status.CUx 
> bits were always writable and the relevant logic to intereact with the 
> external chip enabled when requested.
> 
>  And there is software that relies on this property as prior to the 
> introduction of the modern MIPS architecture there was no CP0.Config1 
> register present to check the presence of an FPU with.  Instead what 
> software was supposed to do was to enable CP1, execute a CFC instruction 
> to read CP1.FIR that was supposed not to trap under these circumstances, 
> and check the value obtained in the Imp field (as it was then called) aka 
> ProcessorID.  If that was non-zero, an FPU was present, if it was zero 
> (due to the floating bus being strapped IIUC), no FPU was available. [2] 
> For example we have code in Linux doing this when run on the relevant 
> hardware.
> 
>  Obviously COP1X instructions would trap on CP0.Status.CU3 instead and for 
> example the FP emulator that we have in Linux is prepared for this 
> situation (whether it should really emulate a full MIPS IV if not higher 
> FP instruction set on a lower-ISA processor is another question).
> 
>  And I think all this draws the "right" implementation that we can make, 
> across all the three coprocessors actually, e.g. for CP1, except stores 
> and transfers from:
> 
>     check_cp1_enabled(ctx);
>     if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
>         handle_the_op();
>     }
> 
> for stores and transfers from:
> 
>     check_cp1_enabled(ctx);
>     if (ctx->CP0_Config1 & (1 << CP0C1_FP)) {
>         write_destination(read(source));
>     } else {
>         write_destination(0);
>     }

Yes, if we take into account also legacy CPUs, then this sounds like a
good way to go (instead of just removing extra check).

Thanks,
Leon