Re: [PATCH] Support for MIPS R5900 (Sony Playstation 2)

["Jürgen Urban" <JuergenUrban at gmx dot de>]

Hello Richard,

> > It would be nice when this can be added to the offical binutils. Please
> > tell me if this acceptable or there is a problem.
> 
> I suppose the main potential barrier is whether the copyright can still
> be assigned to the FSF.  I imagine this code has been worked on by
> several people over the years.  Is that right?  If so, I think they
> would all need to sign a copyright assignment.
> 
> Are there still remnants of the original Cygnus implementation, or was
> this written from scratch?  There was often talk at Cygnus/Red Hat of
> formally submitting the port (along with the tx79), but it never happened
> due to lack of time.
> 
> I'll hold off a full review until the copyright situation is clearer,
> but generally the patch looks good.  There shouldn't be any major
> technical obstacles to getting the support in.

Sony delivered the source code on one of the 2 DVDs of Sony's Linux Toolkit. This was the first and only release outside Japan. I thought that the copyright is by Sony, but Sony seems to use the Copyright statement of the FSF and claims not to be responsible for any code on this DVD. I don't know if Cygnus was involved in that, but it is mentioned in the patches.

I used this code as base. ragnarok2040 also contributed code.

The copyright of the following lines of the patch are unclear:
line 423-467
line 477-948
line 1255-1331
line 1377-1398
line 1533-1563
line 2027-2028
line 2359-2362
line 1573-1754
line 1781-1918

I think this was nearby completely done by ragnarok2040 alone and I am waiting for an answer from him.

The following lines were delivered by Sony SCEI on the DVD and later changed by me or ragnarok2040:
line 1040-1179
line 1781-1918
line 1919-2430

The other lines have no copyright problem and are enough to get some basic support with additional ~3 rewritten lines.

The tests are written from scratch.

I am trying to get an copyright assignment from Sony.
I think there is no problem to get one from ragnarok2040.
I can can sign the copyright assignment, but this has no relevance in Germany; i.e. the signing is invalid by law. According to the dictionary "copyright" translates to "Urheberrecht". The German law says that this can only be passed by the last will, but the GPL was confirmed by the court.

> The mips64*-ps2-elf* stanzas in config.bfd include the n64 vectors,
> which is good.  So even if o64 and n64 aren't supported at runtime,
> we should still be able to assemble o64 and n64 code.  The "n32native"
> part of the testsuite patch doesn't look right from that point of view,
> although it looks like "!n32native" might actually be skipping based
> on something other than the ABI.

Maybe just the term is confusing. I have chosen the term n32native to describe the architecture of the r5900. Maybe I should just use "hardldsd" and "singlefloat", because the most problems where caused by the "-mhardldsd" flag and the single float in 64 bit.

> 
> > - Support for VU instructions of the COP 2 (VU0 in Macro Mode). This
> > includes special register suffixes.
> 
> We're running very short of single character operand markers, so for:
> 
> +   VU0 Macromode:
> +   "+m" vu0 immediate for viaddi (OP_*_VADDI)
> +   "+5" vu0 fp reg position 1 (OP_*_VUTREG)
> +   "+6" vu0 fp reg position 2 (OP_*_VUSREG)
> +   "+7" vu0 fp reg position 3 (OP_*_VUDREG)
> +   "+8" vu0 int reg position 1 (OP_*_VUTREG)
> +   "+9" vu0 int reg position 2 (OP_*_VUSREG)
> +   "+0" vu0 int reg position 3 (OP_*_VURREG)
> +   "+d" vu0 fp reg with ftf modifier (OP_*_VUTREG and OP_*_VUFTF)
> +   "+e" vu0 fp reg with fsf modifier (OP_*_VUSREG and OP_*_VUFSF)
> +   "+f" Immediate operand for vcallms instruction. (OP_*_VUCALLMS)
> +   "+i" vu0 I register
> +   "+q" vu0 Q register
> +   "+r" vu0 R register
> +   "#" optional suffix that must match if present
> +   "=" dest operant completer, must match previous dest if present
> +   "?" dest instruction completer (OP_*_VUDEST)
> +   ";" dest instruction completer, must by xyz
> +   "_" vu0 ACC register
> 
> I think all the operands should have a prefix (i.e. the last 5 too).
> Same for the ++/-- formats.

OK, currently I don't know how to do that. This was implemented by ragnarok2040. I need to check if he can change this.

> 
> > - Detecting of short loops because of a CPU bug (minimum number of
> > instructions needed in a conditional loop).
> 
> I haven't looked at this in detail, but I noticed:
> 
> +  /* On R5900 short loops need to be fixed by inserting a nop in
> +     the branch delay slots.
> +     A short loop can be terminated too early.  */
> +  if (mips_opts.arch == CPU_R5900
> +      /* Check if instruction has a parameter, ignore "j $31". */
> +      && (address_expr != NULL)
> +      /* Parameter must be 16 bit. */
> +      && (*reloc_type == BFD_RELOC_16_PCREL_S2)
> +      /* Branch to same segment. */
> +      && (S_GET_SEGMENT(address_expr->X_add_symbol) == now_seg)
> +      /* Branch to same code fragment. */
> +      && (symbol_get_frag(address_expr->X_add_symbol) == frag_now)
> 
> Even tight loops can be split over several frags.  The start of the loop
> might have been at the very end of the buffer allocated for one frag, or
> the loop might contain a macro-derived variant instruction, etc.

OK, maybe it is not perfect, but it is better than nothing. It was already difficult to implement this. I would need a hint how to fix this.

> > - New parameter "-mhard-ldsd" to support ld and sd instructions in MIPS
> > ABI o32. This is needed to compile the Linux kernel with support for 64
> > bit registers, because otherwise the instructions ld and sd are replaced
> > by lw and sw.
> 
> It seems dangerous to ignore HAVE_64BIT_GPRs in this one instance
> (and only this one instance).  Could you explain in more detail
> in which situations the kernel needs to use LD and SD?

The kernel mainly needs this in the exception handler to save registers and restore registers. The compiler doesn't use it, but the registers shouldn't be destroyed by the o32 kernel, because the user space could run a n32 program. It is also planned to be used in optimized functions like memcpy() or memset().

> 
> > - The "-mtune=r5900" parameter forces "-march=r5900", because this is
> > the only configurable option of GCC to support R5900 as default with
> > mips64el-linux-gnu or mipsel-linux-gnu.
> 
> FWIW, the defaults for both -march and -mtune can be set when GCC is
> configured (using --with-arch and --with-tune respectively).

I need to check this with GCC 4.3. If this works, the change is not needed.

Best regards
JÃrgen Urban