On 14/06/12 05:48, Christoffer Dall wrote:
> When an exception is taken from a guest running in Thumb-2 mode and
> that instruction is emulated and skipped, the ITSTATE field stored
> inside the CPSR must be updated by KVM, as the instruction is never
> advanced by hardware.
>
> This can cause bugs like ending up in a state where you incorrectly don't or do
> execute instructions, or even worse, as was the case in some GIC i/o code,
> where one IT-block immediately follows another one, causes the IT-block to
> generate an undefined exception on the IT instruction itself.
Nice catch.
> Note: This patch does touch a tiny-bit of unrelated code, but it's going to be
> flattened for next KVM release anyhow.
This patch breaks my ARM guest kernel though, see below.
> Signed-off-by: Christoffer Dall <c.dall at virtualopensystems.com>
> ---
> arch/arm/include/asm/kvm_emulate.h | 1 +
> arch/arm/kvm/emulate.c | 49 ++++++++++++++++++++++++++++++++++--
> arch/arm/kvm/mmu.c | 1 +
> 3 files changed, 48 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm/include/asm/kvm_emulate.h b/arch/arm/include/asm/kvm_emulate.h
> index 5b19196..1f93e71 100644
> --- a/arch/arm/include/asm/kvm_emulate.h
> +++ b/arch/arm/include/asm/kvm_emulate.h
> @@ -49,6 +49,7 @@ 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);
> +void kvm_adjust_itstate(struct kvm_vcpu *vcpu);
>
> /*
> * Return the SPSR for the specified mode of the virtual CPU.
> diff --git a/arch/arm/kvm/emulate.c b/arch/arm/kvm/emulate.c
> index 36f591b..98ae05a 100644
> --- a/arch/arm/kvm/emulate.c
> +++ b/arch/arm/kvm/emulate.c
> @@ -657,7 +657,7 @@ static int kvm_ls_length(struct kvm_vcpu *vcpu, u32 instr)
> int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
> unsigned long instr)
> {
> - unsigned long rd, rn, offset, len;
> + unsigned long rd, rn, offset, len, instr_len;
> int index;
> bool is_write;
>
> @@ -694,8 +694,51 @@ int kvm_emulate_mmio_ls(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa,
>
> /*
> * The MMIO instruction is emulated and should not be re-executed
> - * in the guest. (XXX We don't support Thumb instructions yet).
> + * in the guest.
> */
> - *vcpu_pc(vcpu) += 4;
> + if (*vcpu_cpsr(vcpu) & PSR_T_BIT && !is_wide_instruction(instr))
> + instr_len = 4;
This bit looks wrong. Your instruction length is 4 if you're in ARM mode
or the instruction is wide.
> + else
> + instr_len = 2;
> +
> + *vcpu_pc(vcpu) += instr_len;
> + kvm_adjust_itstate(vcpu);
> return 0;
> }
> +
> +/**
> + * adjust_itstate - adjust ITSTATE when emulating instructions in IT-block
> + * @vcpu: The VCPU pointer
> + *
> + * When exceptions occur while instructions are executed in Thumb IF-THEN
> + * blocks, the ITSTATE field of the CPSR is not advanved (updated), so we have
> + * to do this little bit of work manually. The fields map like this:
> + *
> + * IT[7:0] -> CPSR[26:25],CPSR[15:10]
> + */
> +void kvm_adjust_itstate(struct kvm_vcpu *vcpu)
> +{
> + unsigned long itbits, cond;
> + unsigned long cpsr = *vcpu_cpsr(vcpu);
> +
> + if (!(cpsr & PSR_IT_MASK))
> + return;
> +
> + BUG_ON(!(cpsr & PSR_T_BIT));
> +
> + cond = (cpsr & 0xe000) >> 13;
> + itbits = (cpsr & 0x1c00) >> (10 - 2);
> + itbits |= (cpsr & (0x3 << 25)) >> 25;
> +
> + /* Perform ITAdvance (see page A-52 in ARM DDI 0406C) */
> + if ((itbits & 0x7) == 0)
> + itbits = cond = 0;
> + else
> + itbits = (itbits << 1) & 0x1f;
> +
> + cpsr &= ~PSR_IT_MASK;
> + cpsr |= cond << 13;
> + cpsr |= (itbits & 0x1c) << (10 - 2);
> + cpsr |= (itbits & 0x3) << 25;
> + *vcpu_cpsr(vcpu) = cpsr;
> +}
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 094035a..64e93c5 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -493,6 +493,7 @@ static int io_mem_abort(struct kvm_vcpu *vcpu, struct kvm_run *run,
> * in the guest.
> */
> *vcpu_pc(vcpu) += instr_len;
> + kvm_adjust_itstate(vcpu);
This should probably only be called if in Thumb mode.
> return 0;
> }
I've used the attached patch to get my ARM kernel back to a working state.
Cheers,
M.
--
Jazz is not dead. It just smells funny...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: thumb2-fix.patch
Type: text/x-diff
Size: 1919 bytes
Desc: not available
Url : https://lists.cs.columbia.edu/pipermail/android-virt/attachments/20120614/6a790354/thumb2-fix-0001.bin
