Message ID | alpine.DEB.1.10.1411031852060.2881@tp.orcam.me.uk |
---|---|
State | New |
Headers | show |
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
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
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
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:
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