Re: [PATCH v4] Fixups of Loongson2F

[Richard Sandiford <rdsandiford at googlemail dot com>]

Nick Clifton <nickc@redhat.com> writes:
> Hi Andreas, Hi Wu,
>
>  > On 24/02/10 00:09, Andreas Barth wrote:
>> * Wu Zhangjin (wuzhangjin@gmail.com) [091119 08:06]:
>>> This patch adds two new options for binutils: -mfix-loongson2f-nop and
>>> -mfix-loongson2f-jump to work around the NOP and Jump instruction Issues of
>>> Loongson2F.
>>
>> Any chance that this patch gets incorporated into binutils, i.e.
>> commited to cvs?
>
> Oops - sorry about that, the email must have slipped through my net.
>
> Anyway I have now applied Wu's patch.

Sorry for the late reply, but can I ask you to reconsider?  I don't
think the patch is ready to go in yet.  Although we could just make
any follow-on changes relative to the patches that went in, I think
it'd be better to deal with the whole change as a unit.  It's then
easier to make sure that dead code doesn't accidentally get left in.

For the record, I don't think it slipped through your net.  Wu Zhangjin
did post the original patch series a while back, but we then had to
pause so that the copyright assignments could be filled in and processed.
I assume the fact that the patch is being posted now is a sign that that
has happened.  (Would you mind checking thought, just to make sure the
FSF are happy?  I'm sure Wu Zhangjin wouldn't have posted the patch if
he didn't think the assignment had been sorted, but I know I've had a
GCC case where the submitter thought that in good faith, but where it
turned out that something had gone wrong along the lines.)

TBH, I didn't want to carry on reviewing the patch until I knew the
copyright had been sorted, hence not saying anything until now.

Anyway, the reason I don't think it's ready is that I think the
jump changes should be integrated into the existing macro code,
rather than tacked in at the beginning of append_insn.  As an
example of why that matters, the patch silently assembles:

	.set	noreorder
	.set	nomacro
	jr	$31
	nop
	.set	macro
	.set	reorder

as:

00000000 <.text>:
   0:   3c01cfff        lui     at,0xcfff
   4:   3421ffff        ori     at,at,0xffff
   8:   03e1f824        and     ra,ra,at
   c:   03e00008        jr      ra
  10:   00000000        nop

when passed -mfix-loongson2f-jump.  That's a big no-no: the
nomacro/noreorder combination _must_ issue a diagnostic if it
ever assembles a single macro into multiple instructions.

I also think that it should error for:

	.set	noat
	jr	$1
	.set	at

rather than silently skip the workaround.  This is consistent
with the error you get for something like:

	.set	noat
	sw	$1,0x12345678
	.set	at

In short, I think the jump instructions should be treated as macros.

I'd also like to see the same thing for nops if possible, since
append_insn really is supposed to add the instruction it's told
to add.  Version 3 of the patch did that, and looked much better
structurally.  Presumably it didn't work though. ;-)
It'd be interesting to know why.  The main gotcha I see is that
these macros (nops and jumps) could be used in other macros,
but I think that's solvable.

I also think the documentation could be worded slightly better.

+Eliminate instruction fetch from outside 256M region to work around the
+Loongson2F @samp{jump} instructions.

First, as a minor point, there's no instruction called "jump", so the
@samp isn't really appropriate.  More importantly, I think we should
explicitly say that we're masking the address with 0xcffffffff because:

1. We're not actually preventing jumps outside the 256M region in some
   cases.  E.g. jumps from TLB-mapped memory to directly-mapped memory
   could easily cross a 256M region even with the workaround.  As I
   understand it, the unwritten (but natural) assumption is that kernel
   code should not jump from a TLB-mapped region.  (And as the covering
   message said, only targets in the directly-mapped region matter,
   since TLBs shouldn't point to the physical memory in question.)

2. The workaround only deals with kernels that run in ckseg,
   but the current documentation doesn't make that clear.

We should also say that the fix doesn't apply to jumps via K0 or K1.

Richard