On Thu, Jun 14, 2012 at 5:58 AM, Marc Zyngier <marc.zyngier at arm.com> wrote:
> 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.
>
whoops, yeah. Blame it on the late night.
>> + ? ? 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.
>
if in ARM mode the ITSTATE block really should be zero and the adjust
itstate should just exit.
>> ? ? ? return 0;
>> ?}
>
> I've used the attached patch to get my ARM kernel back to a working state.
>
thanks!
-Christoffer
