On Sun, Aug 5, 2012 at 8:41 PM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> [ This time, to the right mailing list addr ]
>
> There's a riveting section of the ARM ARM, B1.14.1:
>
> Hyp traps on instructions that fail their condition code check
> (and the women who love them)
>
> OK, I made that last bit up, but it captures the tone correctly: reading
> this will make you weep.
>
> Of course, this can't really be tested except by hardware which has this
> feature.
>
> Differences from version 1:
> 1) No more double negative: guest_didnt_mean_it => kvm_condition_valid
> 2) Use the generic arm_check_condition as suggested by Peter.
> 3) Fix it to test the cpsr not spsr!
>
> Signed-off-by: Rusty Russell <rusty.russell@xxxxxxxxxx>
>
> diff --git a/arch/arm/include/asm/kvm_arm.h b/arch/arm/include/asm/kvm_arm.h
> index 1efa452..4267257 100644
> --- a/arch/arm/include/asm/kvm_arm.h
> +++ b/arch/arm/include/asm/kvm_arm.h
> @@ -155,6 +155,10 @@
> #define HSR_FSC_TYPE (0x3c)
> #define HSR_SSE (1 << 21)
> #define HSR_WNR (1 << 6)
> +#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 FSC_FAULT (0x04)
> #define FSC_PERM (0x0c)
> diff --git a/arch/arm/kvm/arm.c b/arch/arm/kvm/arm.c
> index f3b206a..b389771 100644
> --- a/arch/arm/kvm/arm.c
> +++ b/arch/arm/kvm/arm.c
> @@ -43,6 +43,7 @@
> #include <asm/kvm_asm.h>
> #include <asm/kvm_mmu.h>
> #include <asm/kvm_emulate.h>
> +#include <asm/opcodes.h>
>
> static DEFINE_PER_CPU(unsigned long, kvm_arm_hyp_stack_page);
>
> @@ -457,6 +458,50 @@ static exit_handle_fn arm_exit_handlers[] = {
> };
>
> /*
> + * A conditional instruction is allowed to trap, even though it
> + * wouldn't be executed. So let's re-implement the hardware, in
> + * software!
> + */
> +static bool kvm_condition_valid(struct kvm_vcpu *vcpu)
> +{
> + unsigned long cpsr, cond, insn;
> +
> + /*
> + * 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. */
> + if (vcpu->arch.hsr >> 30)
> + return true;
> +
> + cpsr = *vcpu_cpsr(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 = ((cpsr >> 8) & 0xFC) | ((cpsr >> 25) & 0x3);
> +
> + /* it == 0 => unconditional. */
> + if (it == 0)
> + return true;
> +
> + /* The cond for this insn works out as the top 4 bits. */
> + cond = (it >> 4);
> + }
> +
> + /* Shift makes it look like an ARM-mode instruction */
> + insn = cond << 28;
> + return arm_check_condition(insn, cpsr) != ARM_OPCODE_CONDTEST_FAIL;
> +}
> +
> +/*
> * Return 0 to return to guest, < 0 on error, exit_reason ( > 0) on proper
> * exit to QEMU.
> */
> @@ -487,6 +532,11 @@ static int handle_exit(struct kvm_vcpu *vcpu, struct kvm_run *run,
> BUG();
> }
>
> + /* See ARM ARM B1.14.1: "Hyp traps on instructions
> + * that fail their condition code check" */
not sure if checkpatch.pl complains, but this is not kernel style
commenting, strictly speaking, is it?
> + if (!kvm_condition_valid(vcpu))
> + return 0;
eh, this just exits to qemu without any further notice or explanation
and the guest will re-execute the instruction and the hardware will
trap again. Should we not fast forward past the instruction and adjust
the ITSTATE accordingly?
and then this would be return 1 now.
if you confirm you agree, I will simply make the adjustment and merge.
> +
> return arm_exit_handlers[hsr_ec](vcpu, run);
> default:
> kvm_pr_unimpl("Unsupported exception type: %d",
Thanks!
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
