On Mon, Mar 12, 2012 at 10:43 PM, Rusty Russell <rusty at rustcorp.com.au> wrote:
> 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...
>
not sure what the process is here, maybe they just don't want to
dictate that architecture, if that for some reason can clutter some
hardware implementation logic magic. Who knows.
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.
>> > +{
>> > + ? ? ? 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.
>
yeah, that does the trick
>> > +/* 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.
>
it kind of made sense, but since we have several copies of condition
flags floating around on the vcpu structs etc. I wanted to avoid any
confusion.
>> > + ? ? ? 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
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?
>
> ?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)
