On Mon, 12 Mar 2012 23:15:08 -0400, Christoffer Dall <c.dall at virtualopensystems.com> wrote: > It feels to me like if the kernel doesn't support it for undefined > instructions then maybe neither do we. In any case perhaps a bit > premature, and I agree on the untested and prefer if we hold on to > this until we have some kind of silicon to test this on. Yep, I'll replace this patch with a simple test to see if we need it, and print a big warning if so... > > @@ -11,44 +14,26 @@ ENTRY(kvm_cond_match) > > > > ? ? ? ?/* Now, jump to table depending on cond, return via there. */ > > ? ? ? ?mov ? ? r0, #0 > > - ? ? ? add ? ? r2, r0, r1, LSL #3 > > - ? ? ? sub ? ? r2, #4 > > + ? ? ? mov ? ? r2, r1, lsl #3 > > ? ? ? ?add ? ? pc, r2 > > > > -/* This is "mov<cond> ?r0, #1" */ > > -#define COND_MOVEQ_R0_1(cond) .word ? ?((cond << 28) | 0x03a00001) > > + ? ? ? /* Adding 0 to pc means skip forward one insn on ARM. */ > > + ? ? ? nop > > most ARM developers will be able to read this (I for example don't > think this is commented in the user access fixup table code). However, > I like the comment for clarity and educational purposes, but I think > it's too narrow, as by the same token you should write that "adding 4 > to pc means skip forward two insn on ARM" etc.... :) > > perhaps > > /* ARM reads of PC returns current instr addr + 8; we adjust with a nop *' > > not great, maybe you can improve... is this getting out of hand? OK, so I knew that that PC was 8 in front of the current insn, but didn't appreciated the implication for add (until I debugged this code). Perhaps you're right, all ARM-o-philes know this :) Thanks, Rusty. -- How could I marry someone with more hair than me? http://baldalex.org
