This is the mail archive of the
binutils@sourceware.org
mailing list for the binutils project.
Re: [PATCH v4] Fixups of Loongson2F
- From: Richard Sandiford <rdsandiford at googlemail dot com>
- To: Nick Clifton <nickc at redhat dot com>
- Cc: Andreas Barth <aba at not dot so dot argh dot org>, Wu Zhangjin <wuzhangjin at gmail dot com>, zhangfx at lemote dot com, binutils at sourceware dot org, "Maciej W. Rozycki" <macro at linux-mips dot org>, yanh at lemote dot com, admin at heihaier dot org, = <liushiwei at gmail dot com>, Zhang Le <r0bertz at gentoo dot org>, wuzj at lemote dot com
- Date: Thu, 25 Feb 2010 20:05:14 +0000
- Subject: Re: [PATCH v4] Fixups of Loongson2F
- References: <1258617915-9563-1-git-send-email-wuzhangjin@gmail.com> <20100224000937.GA21981@mails.so.argh.org> <4B865C31.3060501@redhat.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