On Fri, Mar 9, 2012 at 10:44 PM, Rusty Russell <rusty at rustcorp.com.au> wrote:
>
> From: Rusty Russell <rusty at rustcorp.com.au>
>
> Conditional instructions may trap even if the condition isn't true, so a good
> hypervisor will check this. ?The same can happen with undefined instructions,
> so the kernel should be doing this dance already (it isn't).
>
> (See p1206 of ARMv7-A ARM)
>
> This is difficult to test, since the fastmodel doesn't give such spurious
> traps, but it does verify that conditional traps are intended.
>
> Signed-off-by: Rusty Russell <rusty at rustcorp.com.au>
> ---
> ?arch/arm/include/asm/kvm_arm.h ? ? | ? ?4 +++
> ?arch/arm/include/asm/kvm_emulate.h | ? ?2 +
> ?arch/arm/kvm/Makefile ? ? ? ? ? ? ?| ? ?2 +
> ?arch/arm/kvm/arm.c ? ? ? ? ? ? ? ? | ? ?3 ++
> ?arch/arm/kvm/emulate.c ? ? ? ? ? ? | ? 41 +++++++++++++++++++++++++++
> ?arch/arm/kvm/kvm_cond_match.S ? ? ?| ? 54 ++++++++++++++++++++++++++++++++++++
> ?6 files changed, 105 insertions(+), 1 deletions(-)
> ?create mode 100644 arch/arm/kvm/kvm_cond_match.S
>
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index 1769187..156284a 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -107,6 +107,10 @@
> ?#define HSR_ISS ? ? ? ? ? ? ? ?(HSR_IL - 1)
> ?#define HSR_ISV_SHIFT ?(24)
> ?#define HSR_ISV ? ? ? ? ? ? ? ?(1U << HSR_ISV_SHIFT)
> +#define HSR_CV_SHIFT ? (24)
> +#define HSR_CV ? ? ? ? (1U << HSR_CV_SHIFT)
> +#define HSR_COND_SHIFT (20)
> +#define HSR_COND ? ? ? (0xfU << HSR_COND_SHIFT)
>
> ?#define HSR_EC_UNKNOWN (0x00)
> ?#define HSR_EC_WFI ? ? (0x01)
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index fa54247..fe21e86 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -49,6 +49,8 @@ int kvm_handle_cp15_64(struct kvm_vcpu *vcpu, struct kvm_run *run);
> ?int kvm_handle_wfi(struct kvm_vcpu *vcpu, struct kvm_run *run);
> ?int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> ? ? ? ? ? ? ? ? ? ? ? ?unsigned long instr);
> +bool kvm_should_not_have_trapped(struct kvm_vcpu *vcpu);
> +bool kvm_cond_match(u32 flags, u32 cond);
>
> ?/*
> ?* Return the SPSR for the specified mode of the virtual CPU.
> diff --git a/arch/arm/kvm/Makefile b/arch/arm/kvm/Makefile
> index e69a8e1..68f05b9 100644
> --- a/arch/arm/kvm/Makefile
> +++ b/arch/arm/kvm/Makefile
> @@ -12,6 +12,6 @@ obj-$(CONFIG_KVM_ARM_HOST) += init.o interrupts.o exports.o
>
> ?kvm-arm-y += $(addprefix ../../../virt/kvm/, kvm_main.o coalesced_mmio.o)
>
> -kvm-arm-y += arm.o guest.o mmu.o emulate.o
> +kvm-arm-y += arm.o guest.o mmu.o emulate.o kvm_cond_match.o
>
> ?obj-$(CONFIG_KVM) += kvm-arm.o
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index a6cf02b..1724657 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -414,6 +414,9 @@ static inline int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> ? ? ? ? ? ? ? ?BUG();
> ? ? ? ?}
>
> + ? ? ? if (kvm_should_not_have_trapped(vcpu))
> + ? ? ? ? ? ? ? return 0;
> +
> ? ? ? ?return arm_exit_handlers[hsr_ec](vcpu, run);
> ?}
>
> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> index bd2c3dc..11e2dab 100644
> --- a/arch/arm/kvm/emulate.c
> +++ b/arch/arm/kvm/emulate.c
> @@ -659,3 +659,44 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> ? ? ? ?*vcpu_reg(vcpu, 15) += 4;
> ? ? ? ?return 0;
> ?}
> +
> +/* 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?
> +{
> + ? ? ? 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...?
> + ? ? ? if (vcpu->arch.hsr >> 30)
> + ? ? ? ? ? ? ? return false;
> +
> + ? ? ? spsr = *vcpu_spsr(vcpu);
> +
> + ? ? ? /* Is condition field valid? */
> + ? ? ? if ((vcpu->arch.hsr & HSR_CV) >> HSR_CV_SHIFT)
> + ? ? ? ? ? ? ? cond = (vcpu->arch.hsr & HSR_COND) >> HSR_COND_SHIFT;
> + ? ? ? else {
> + ? ? ? ? ? ? ? /* This can happen in Thumb mode: examine IT state. */
> + ? ? ? ? ? ? ? unsigned long it;
> +
> + ? ? ? ? ? ? ? it = ((spsr >> 8) & 0xFC)
> + ? ? ? ? ? ? ? ? ? ? ? | ((spsr >> 25) & 0x3);
I'm thinking if it was nicer with an SPSR_ define here, but it's kind
of messy with the split field. Up to you.
> +
> + ? ? ? ? ? ? ? /* it == 0 => unconditional. */
> + ? ? ? ? ? ? ? if (it == 0)
> + ? ? ? ? ? ? ? ? ? ? ? return false;
> +
> + ? ? ? ? ? ? ? /* The cond for this insn works out as the top 4 bits. */
> + ? ? ? ? ? ? ? cond = (it >> 4);
> + ? ? ? }
> +
> + ? ? ? flags = spsr & ~0x07FFFFFF;
> +
> + ? ? ? return !kvm_cond_match(flags, cond);
> +}
> diff --git a/arch/arm/kvm/kvm_cond_match.S b/arch/arm/kvm/kvm_cond_match.S
> new file mode 100644
> index 0000000..cf48786
> --- /dev/null
> +++ b/arch/arm/kvm/kvm_cond_match.S
> @@ -0,0 +1,54 @@
> +#include <linux/linkage.h>
> +
> +/* 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/
> + ? ? ? 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" ?
> + ? ? ? 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...
> + ? ? ? 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...
Also, why not have the definition be:
.word ? ?((cond << 28) | 0x03a00001); mov pc, lr
and you don't need to replicate all those lines
> +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
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.
> +ENDPROC(kvm_cond_match)
>
since this is not in the critical path, I'm not going to merge this
right now, but wait for your comments.
Thanks,
Christoffer
