On Sun, 11 Mar 2012 22:05:47 -0400, Christoffer Dall <c.dall at virtualopensystems.com> wrote:
> > +/* Some implementations may not check condition codes before trapping! */
> > +bool kvm_should_not_have_trapped(struct kvm_vcpu *vcpu)
>
> cute name:)
>
> it's kind of a double negation though, so how about
> kvm_trap_inst_cond_pass... too convoluted?
The double negative bugs me too (in the implementation it makes it a bit
weird). But I really wanted to emphasize the bizarre nature of the
check.
We can check whether this is required at all, by testing an undefined
instruction (in the host). The ARM ARM says you can only false-trap on
these isns if you false-trap on undefined insns.
So it's tempting to rip this code out, and just print a big warning and
fail if we need it. I hate untested code!
But I suspect it wouldn't be so explicit in the manual if there wasn't
some silicon coming which actually needs this...
> > +{
> > + ? ? ? unsigned long spsr, cond, flags;
> > +
> > + ? ? ? /*
> > + ? ? ? ?* Exception Code 0 can only happen if we set HCR.TGE to 1, to
> > + ? ? ? ?* catch undefined instructions, and then we won't get past
> > + ? ? ? ?* the arm_exit_handlers test anyway.
> > + ? ? ? ?*/
> > + ? ? ? BUG_ON(((vcpu->arch.hsr & HSR_EC) >> HSR_EC_SHIFT) == 0);
> > +
> > + ? ? ? /* Top two bits non-zero? ?Unconditional. */
>
> the point here is that this would never trap to Hyp mode if the
> condition code didn't pass right? Not that it is assumed that the
> instructions only exist in unconditional form. Perhaps a clearer
> comment...?
This is directly from the ARM, I've included the reference.
> > +/* extern bool kvm_cond_match(u32 flags, u32 cond); */
> > +ENTRY(kvm_cond_match)
> > +.arm
> > + ? ? ? /* First, set our condition flags to the arg. */
>
> s/our/the cpsr/
I guess I was thinking at a higher level when I wrote that comment.
> > + ? ? ? mrs ? ? r2, cpsr
> > + ? ? ? and ? ? r2, r2, #0x07FFFFFF
> > + ? ? ? orr ? ? r2, r2, r0
> > + ? ? ? msr ? ? cpsr, r2
> > +
> > + ? ? ? /* Now, jump to table depending on cond, return via there. */
> > + ? ? ? mov ? ? r0, #0
> > + ? ? ? add ? ? r2, r0, r1, LSL #3
>
> Nit: Couldn't this be: "mov r2, r1, lsl #3" ?
Certainly less weird than my version.
> > + ? ? ? sub ? ? r2, #4
>
> Super nit: I prefer the full version of these instructions, i.e. "sub
> r2, r2, #4", but I don't know if there's an ARM community feeling
> towards this.
>
> Also, instead of this sub, you could just have a noop after the add
> instruction...
Both are kind of odd. One would naively think that adding 0 to the pc
would be a noop. Either way, deserves a comment.
> > + ? ? ? add ? ? pc, r2
> > +
> > +/* This is "mov<cond> ?r0, #1" */
> > +#define COND_MOVEQ_R0_1(cond) .word ? ?((cond << 28) | 0x03a00001)
> > +
>
> I would move this define up above the code as the table must
> immediately follow the code and we wouldn't want to to give anyone the
> illusion that that's not the case...
Yep.
> Also, why not have the definition be:
> .word ? ?((cond << 28) | 0x03a00001); mov pc, lr
>
> and you don't need to replicate all those lines
Yes, done.
> > + ? ? ? COND_MOVEQ_R0_1(15)
> > + ? ? ? mov ? ? pc, lr
>
> this is not a mov, but a SIMD instruction, if I'm not mistaken. You
> probably want to have a BUG_ON(cond == 15) or something in the C code.
Ah, I see. Commented and fixed.
Thanks!
Rusty.
--
How could I marry someone with more hair than me? http://baldalex.org
ARM: KVM: Clean up condition checking code.
From: Rusty Russell <rusty at rustcorp.com.au>
Based on feedback by Christoffer.
---
arch/arm/kvm/emulate.c | 12 ++++++++
arch/arm/kvm/kvm_cond_match.S | 59 +++++++++++++++--------------------------
2 files changed, 33 insertions(+), 38 deletions(-)
diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
index 22b7743..d047003 100644
--- a/arch/arm/kvm/emulate.c
+++ b/arch/arm/kvm/emulate.c
@@ -685,7 +685,14 @@ bool kvm_should_not_have_trapped(struct kvm_vcpu *vcpu)
*/
BUG_ON(((vcpu->arch.hsr & HSR_EC) >> HSR_EC_SHIFT) == 0);
- /* Top two bits non-zero? Unconditional. */
+ /*
+ * From ARM ARM DDI0406C, pp1416-1417:
+ *
+ * For EC values that are nonzero and have the two most-significant
+ * bits 0b00, ISS[24:20] provides the condition code field for the
+ * trapped instruction...
+ *
+ * So, if top two bits are non-zero, it's unconditional. */
if (vcpu->arch.hsr >> 30)
return false;
@@ -711,5 +718,8 @@ bool kvm_should_not_have_trapped(struct kvm_vcpu *vcpu)
flags = spsr & ~0x07FFFFFF;
+ /* 0xF isn't a valid condition, it's an escape for other opcodes. */
+ BUG_ON(cond >= 15);
+
return !kvm_cond_match(flags, cond);
}
diff --git a/arch/arm/kvm/kvm_cond_match.S b/arch/arm/kvm/kvm_cond_match.S
index cf48786..b4079dc 100644
--- a/arch/arm/kvm/kvm_cond_match.S
+++ b/arch/arm/kvm/kvm_cond_match.S
@@ -1,9 +1,12 @@
#include <linux/linkage.h>
+/* This is "mov<cond> r0, #1" */
+#define COND_MOV_R0_1_RET(cond) .word ((cond << 28) | 0x03a00001) ; mov pc, lr
+
/* extern bool kvm_cond_match(u32 flags, u32 cond); */
ENTRY(kvm_cond_match)
.arm
- /* First, set our condition flags to the arg. */
+ /* First, set the cpsr condition flags to the arg. */
mrs r2, cpsr
and r2, r2, #0x07FFFFFF
orr r2, r2, r0
@@ -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
cond_table:
- COND_MOVEQ_R0_1(0)
- mov pc, lr
- COND_MOVEQ_R0_1(1)
- mov pc, lr
- COND_MOVEQ_R0_1(2)
- mov pc, lr
- COND_MOVEQ_R0_1(3)
- mov pc, lr
- COND_MOVEQ_R0_1(4)
- mov pc, lr
- COND_MOVEQ_R0_1(5)
- mov pc, lr
- COND_MOVEQ_R0_1(6)
- mov pc, lr
- COND_MOVEQ_R0_1(7)
- mov pc, lr
- COND_MOVEQ_R0_1(8)
- mov pc, lr
- COND_MOVEQ_R0_1(9)
- mov pc, lr
- COND_MOVEQ_R0_1(10)
- mov pc, lr
- COND_MOVEQ_R0_1(11)
- mov pc, lr
- COND_MOVEQ_R0_1(12)
- mov pc, lr
- COND_MOVEQ_R0_1(13)
- mov pc, lr
- COND_MOVEQ_R0_1(14)
- mov pc, lr
- COND_MOVEQ_R0_1(15)
- mov pc, lr
+ COND_MOV_R0_1_RET(0)
+ COND_MOV_R0_1_RET(1)
+ COND_MOV_R0_1_RET(2)
+ COND_MOV_R0_1_RET(3)
+ COND_MOV_R0_1_RET(4)
+ COND_MOV_R0_1_RET(5)
+ COND_MOV_R0_1_RET(6)
+ COND_MOV_R0_1_RET(7)
+ COND_MOV_R0_1_RET(8)
+ COND_MOV_R0_1_RET(9)
+ COND_MOV_R0_1_RET(10)
+ COND_MOV_R0_1_RET(11)
+ COND_MOV_R0_1_RET(12)
+ COND_MOV_R0_1_RET(13)
+ COND_MOV_R0_1_RET(14)
ENDPROC(kvm_cond_match)
