Many thanks for your review, Maciej,
> I have been more thorough on this occasion, and I do hope I have caught
> everything. See the notes below, in addition to what the others wrote.
>
> Please apply to v3 accordingly; I started writing this before you sent
> that version.
Sure, next version submitted as a patch series will be a lot better, thanks!
> I think this has to be (CPU_MIPS3 | INSN_R5900) really.
>
> While the CPU does support MIPS IV MOVN, MOVZ and PREF instructions,
> that's all it has from that ISA. Even the Tx79, which was supposed to
> have a regular IEEE-754 FPU, did not have the extra FP condition bits,
> MOVN.fmt, MOVZ.fmt or any of the MOVF* or MOVT* instructions, indexed
> load/store instructions, multiply-accumulate instructions, etc. defined.
>
> So I think the R5900 has to be treated as a MIPS III implementation, with
> some aberrations. I don't expect it to be a big deal to add INSN_R5900
> qualification to MOVN, MOVZ and PREF instruction emulation code right
> away.
Trivial to add, in fact. :)
> Then presumably you are going to add emulation for all the missing MIPS
> III instructions to the Linux kernel eventually, to match the MIPS III
> Linux user ABI? Apart from LLD and SCD discussed previously this will
> have to include DDIV, DDIVU, DMULT and DMULTU.
That sounds reasonable!
> Eventually you'll have to remove all these instructions (plus LL and SC)
> from the system emulation mode. In fact I think it would make sense to do
> that right away, because I believe it will be a reasonably simple update.
> However if it turns out to be a can of worms after all, then I think we
> can defer it, because we do not have a bare-metal environment ready for
> this CPU anyway (or do we?).
Indeed, very simple. I added a new check function check_insn_opc_user_only,
similar to check_insn_opc_removed, conditional on USER_ONLY and so a simple
check_insn_opc_user_only(ctx, INSN_R5900);
has to be added for these OPC cases. To test this I tried to build GCC with
the target mips64r5900el but it unfortunately didn't work as GCC failed to
compile itself:
../sysdeps/mips/mips64/mul_1.S:49: Error: opcode not supported on
this processor: r5900 (mips3) `dmultu $8,$7'
> I think there is no point in executing the multiplication twice if `rd !=
> 0' and also we want to continue trapping on `sa != 0' for the non-Vr54xx
> case, so how about:
>
> if (sa) {
> check_insn(ctx, INSN_VR54XX);
> op1 = MASK_MUL_VR54XX(ctx->opcode);
> gen_mul_vr54xx(ctx, op1, rd, rs, rt);
> } else if (rd && (ctx->insn_flags & INSN_R5900)) {
> gen_mul_r5900(ctx, op1, rd, rs, rt);
> } else {
> gen_muldiv(ctx, op1, rd & 3, rs, rt);
> }
>
> ?
Agreed, that's better. The point was to reuse a bit of code, but now I've
folded all of it into gen_mul_r5900. There are also R5900 specific MULT1 and
MULTU1 instructions which might use it, so I folded the rd != 0 test into it
as well.
> > + /* No L2 cache, icache size 32k, dcache size 32k, uncached
> > coherency. */
> > + .CP0_Config0 = (1 << 17) | (0x3 << 9) | (0x3 << 6) | (0x2 <<
> > CP0C0_K0),
>
> Why ICE set but DCE clear? I guess this corresponds to the incorrect L2
> cache reference as bit #17 is SC on the original R4000. That reference
> has to go, obviously, as there's no L2 cache indication in the R5900's
> Config register.
>
> As to ICE and DCE their reset defaults are both 0, so we might as well
> use that, as we don't emulate caches anyway. If it turns out to matter to
> any software, then we'll have to provide a more correct Config register
> emulation as its writable bits are processor-specific.
Agreed, thanks for finding this!
> > + /* Note: Config1 is only used internally, the R5900 has only
> > Config0. */
> > + .CP0_Status_rw_bitmask = 0xF4C79C1F,
>
> Maybe swap the two lines so that the Config1 register reference does not
> confusingly seem to describe the Status r/w bitmask?
Done.
> > +#ifdef CONFIG_USER_ONLY
> > + /*
> > + * R5900 hardware traps to the Linux kernel for IEEE 754-1985 and LL/SC
> > + * emulation. For user-only, qemu is the kernel, so we emulate the traps
> > + * by simply emulating the instructions directly.
> > + */
>
> Please use spaces rather than tabs for indentation, as per QEMU's coding
> standard.
Done.
> > + .SEGBITS = 19,
>
> This has to be 32, as with any MIPS CPU that implements 32-bit virtual
> addressing only. Specifically EntryHi register's VPN2 field goes up to
> bit #31, which is (SEGBITS - 1).
>
> > + .PABITS = 20,
>
> And this has to be 32 as well, reflecting EntryLo0/1 registers' PFN
> fields going up to bit #25. This corresponds to bit #31 on the external
> address bus, which is (PABITS - 1).
Right. I was unaware of these details, thanks!
Fredrik
