On Thu, Aug 23, 2012 at 7:20 AM, Christoffer Dall
<c.dall@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On Thu, Aug 23, 2012 at 3:19 AM, Rusty Russell <rusty.russell@xxxxxxxxxx> wrote:
>> Create a helper to do the guest va->pa mapping. Then take the code out of the
>> exit path, and use normal C.
>>
>> copy_from_guest_va() is a bit more generic than it needs to be, but
>> not much.
>>
>> Signed-off-by: Rusty Russell <rusty.russell@xxxxxxxxxx>
>>
>> diff --git a/arch/arm/include/asm/kvm_asm.h b/arch/arm/include/asm/kvm_asm.h
>> index 85bd676..ee21cd2 100644
>> --- a/arch/arm/include/asm/kvm_asm.h
>> +++ b/arch/arm/include/asm/kvm_asm.h
>> @@ -54,6 +54,8 @@ extern void __kvm_flush_vm_context(void);
>> extern void __kvm_tlb_flush_vmid(struct kvm *kvm);
>>
>> extern int __kvm_vcpu_run(struct kvm_vcpu *vcpu);
>> +
>> +extern u64 __kvm_va_to_pa(struct kvm_vcpu *vcpu, u32 va, bool priv);
>> #endif
>>
>> #endif /* __ARM_KVM_ASM_H__ */
>> diff --git a/arch/arm/include/asm/kvm_host.h b/arch/arm/include/asm/kvm_host.h
>> index 778d2af..79b5318 100644
>> --- a/arch/arm/include/asm/kvm_host.h
>> +++ b/arch/arm/include/asm/kvm_host.h
>> @@ -131,9 +131,6 @@ struct kvm_vcpu_arch {
>> u32 hdfar; /* Hyp Data Fault Address Register */
>> u32 hifar; /* Hyp Inst. Fault Address Register */
>> u32 hpfar; /* Hyp IPA Fault Address Register */
>> - u64 pc_ipa; /* IPA for the current PC (VA to PA result) */
>> - u64 pc_ipa2; /* same as above, but for non-aligned wide thumb
>> - instructions */
>>
>> /* Floating point registers (VFP and Advanced SIMD/NEON) */
>> struct vfp_hard_struct vfp_guest;
>> diff --git a/arch/arm/kernel/asm-offsets.c b/arch/arm/kernel/asm-offsets.c
>> index aca6c2c..bb18fd4 100644
>> --- a/arch/arm/kernel/asm-offsets.c
>> +++ b/arch/arm/kernel/asm-offsets.c
>> @@ -184,8 +184,6 @@ int main(void)
>> DEFINE(VCPU_HDFAR, offsetof(struct kvm_vcpu, arch.hdfar));
>> DEFINE(VCPU_HIFAR, offsetof(struct kvm_vcpu, arch.hifar));
>> DEFINE(VCPU_HPFAR, offsetof(struct kvm_vcpu, arch.hpfar));
>> - DEFINE(VCPU_PC_IPA, offsetof(struct kvm_vcpu, arch.pc_ipa));
>> - DEFINE(VCPU_PC_IPA2, offsetof(struct kvm_vcpu, arch.pc_ipa2));
>> DEFINE(VCPU_HYP_PC, offsetof(struct kvm_vcpu, arch.hyp_pc));
>> DEFINE(KVM_VTTBR, offsetof(struct kvm, arch.vttbr));
>> #endif
>> diff --git a/arch/arm/kvm/interrupts.S b/arch/arm/kvm/interrupts.S
>> index b70394a..d282860 100644
>> --- a/arch/arm/kvm/interrupts.S
>> +++ b/arch/arm/kvm/interrupts.S
>> @@ -510,6 +510,37 @@ after_vfp_restore:
>> clrex @ Clear exclusive monitor
>> bx lr @ return to IOCTL
>>
>> +@ Arguments:
>> +@ r0: pointer to vcpu struct
>> +@ r1: virtual address to map (rounded to page)
>> +@ r2: 1 = P1 (read) mapping, 0 = P0 (read) mapping.
>> +@ Returns 64 bit PAR value.
>> +ENTRY(__kvm_va_to_pa)
>> + hvc #0 @ switch to hyp-mode
>> +
>> + push {r4-r12}
>> +
>> + @ Fold flag into r1, easier than using stack.
>
> In case some bonehead calls this with r1 not a bool but some other
> non-zero value, we would never see errors, but really weird bugs,
> wouldn't it be more safe to do:
>
> cmp r1, #0
> movne r1, #1
>
>> + orr r1, r1, r2
>> +
>> + @ This swaps too many registers, but we're in the slow path anyway.
>> + read_cp15_state
>> + write_cp15_state 1, r0
>> +
>> + ands r2, r1, #1
>> + sub r1, r1, r2
>
> I think "bic r1, r1, r2" is clearer
>
>> + mcrne p15, 0, r1, c7, c8, 0 @ VA to PA, ATS1CPR
>> + mcreq p15, 0, r1, c7, c8, 2 @ VA to PA, ATS1CUR
>> +
>
> Page B4-1730 in the arm arm seems to suggest we need an ISB here (yes
> I know, it's my bug, not yours)
>
>> + @ Restore host state.
>> + read_cp15_state 1, r0
>> + write_cp15_state
>> +
>> + mrrc p15, 0, r0, r1, c7 @ PAR
>> + pop {r4-r12}
>> + hvc #0 @ Back to SVC
>> + bx lr
>> +
>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> @ Hypervisor exception vector and handlers
>> @@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@
>> @@ -686,34 +717,6 @@ guest_trap:
>> cmpne r2, #HSR_EC_DABT
>> bne 1f
>>
>> - @ For non-valid data aborts, get the offending instr. PA
>> - lsr r2, r0, #HSR_ISV_SHIFT
>> - ands r2, r2, #1
>> - bne 2f
>> - mrs r3, ELR_hyp
>> - mrs r7, spsr
>> - and r7, r7, #0xf
>> - cmp r7, #0 @ fault happened in user mode?
>> - mcreq p15, 0, r3, c7, c8, 2 @ VA to PA, ATS1CUR
>> - mcrne p15, 0, r3, c7, c8, 0 @ VA to PA, ATS1CPR
>> - mrrc p15, 0, r4, r5, c7 @ PAR
>> - add r6, r1, #VCPU_PC_IPA
>> - strd r4, r5, [r6]
>> -
>> - @ Check if we might have a wide thumb instruction spill-over
>> - ldr r5, =0xfff
>> - bic r4, r3, r5 @ clear page mask
>> - sub r5, r5, #1 @ last 2-byte page bounday, 0xffe
>> - cmp r4, r5
>> - bne 2f
>> - add r4, r3, #2 @ _really_ unlikely!
>> - cmp r7, #0 @ fault happened in user mode?
>> - mcreq p15, 0, r4, c7, c8, 2 @ VA to PA, ATS1CUR
>> - mcrne p15, 0, r4, c7, c8, 0 @ VA to PA, ATS1CPR
>> - mrrc p15, 0, r4, r5, c7 @ PAR
>> - add r6, r1, #VCPU_PC_IPA2
>> - strd r4, r5, [r6]
>> -
>> 2: mrc p15, 4, r2, c6, c0, 0 @ HDFAR
>> mrc p15, 4, r3, c6, c0, 2 @ HIFAR
>> mrc p15, 4, r4, c6, c0, 4 @ HPFAR
>> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
>> index 3df4fa8..6909a83 100644
>> --- a/arch/arm/kvm/mmu.c
>> +++ b/arch/arm/kvm/mmu.c
>> @@ -580,6 +580,62 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> }
>>
>> /**
>> + * copy_from_guest_va - copy memory from guest (very slow!)
>> + * @vcpu: vcpu pointer
>> + * @dest: memory to copy into
>> + * @va: virtual address in guest to copy from
>
> nit: gva might even be more kvm'ish and clearer
>
>> + * @len: length to copy
>> + * @priv: use guest PL1 (ie. kernel) mappings, otherwise use guest PL0 mappings.
>> + *
>> + * Returns 0 on success, or errno.
>> + */
>> +static int copy_from_guest_va(struct kvm_vcpu *vcpu,
>> + void *dest, unsigned long va, size_t len,
>> + bool priv)
>> +{
>> + u64 par;
>> + phys_addr_t pc_ipa;
>> + size_t cur_len;
>> + int err;
>> +
>> + /* A 32-bit thumb2 instruction can actually go over a page boundary! */
>> + do {
>> + par = __kvm_va_to_pa(vcpu, va & PAGE_MASK, priv);
>> + if (par & 1) {
>> + kvm_err("I/O Abort from invalid instruction address? Wrong!\n");
>> + return -EINVAL;
>
> what if it was unmapped from under us? Do we really want to print
> Wrong! and write to the kernel error log?
>
>> + }
>> +
>> + if (par & (1U << 11)) {
>> + /* LPAE PAR format */
>> + /*
>> + * TODO: Check if this ever happens
>> + * - called from Hyp mode
>> + */
>
> Getting a chance to revisit this, surely it has to happen if the guest
> uses LPAE - otherwise you wouldn't be able to get the top bits from
> the VA to PA operation executed from Hyp mode.
>
> Peter/Marc, can you verify?
>
>> + pc_ipa = par & PAGE_MASK & ((1ULL << 32) - 1);
>> + } else {
>> + /* VMSAv7 PAR format */
>> + pc_ipa = par & PAGE_MASK & ((1ULL << 40) - 1);
>> + }
>> + pc_ipa += va & ~PAGE_MASK;
>> +
>> + /* Only copy up to end of page. */
>> + cur_len = (pc_ipa & PAGE_MASK) + PAGE_SIZE - pc_ipa;
>> + if (likely(cur_len > len))
>> + cur_len = len;
>> +
>> + err = kvm_read_guest(vcpu->kvm, pc_ipa, dest, cur_len);
>> + if (unlikely(err))
>> + return err;
>> +
>> + len -= cur_len;
>> + va += cur_len;
>> + } while (unlikely(len != 0));
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> * invalid_io_mem_abort -- Handle I/O aborts ISV bit is clear
>> *
>> * @vcpu: The vcpu pointer
>> @@ -593,22 +649,7 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
>> static int invalid_io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>> {
>> unsigned long instr;
>> - phys_addr_t pc_ipa;
>> -
>> - if (vcpu->arch.pc_ipa & 1) {
>> - kvm_err("I/O Abort from invalid instruction address? Wrong!\n");
>> - return -EINVAL;
>> - }
>> -
>> - if (vcpu->arch.pc_ipa & (1U << 11)) {
>> - /* LPAE PAR format */
>> - /* TODO: Check if this ever happens - called from Hyp mode */
>> - pc_ipa = vcpu->arch.pc_ipa & PAGE_MASK & ((1ULL << 32) - 1);
>> - } else {
>> - /* VMSAv7 PAR format */
>> - pc_ipa = vcpu->arch.pc_ipa & PAGE_MASK & ((1ULL << 40) - 1);
>> - }
>> - pc_ipa += vcpu->arch.regs.pc & ~PAGE_MASK;
>> + int err;
>>
>> if (vcpu->arch.regs.cpsr & PSR_T_BIT) {
>> /* TODO: Check validity of PC IPA and IPA2!!! */
>> @@ -617,9 +658,11 @@ static int invalid_io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
>> return -EINVAL;
>> }
>>
>> - if (kvm_read_guest(vcpu->kvm, pc_ipa, &instr, sizeof(instr))) {
>> + err = copy_from_guest_va(vcpu, &instr, vcpu->arch.regs.pc, sizeof(instr),
>> + vcpu_mode_priv(vcpu));
>> + if (err) {
>> kvm_err("Could not copy guest instruction\n");
>> - return -EFAULT;
>> + return err;
>> }
>>
>> return kvm_emulate_mmio_ls(vcpu, fault_ipa, instr);
>
>
> awesome cleanup!
I've pushed a fixup for this one incorporating these comments to the
v11-stage branch.
Peter/Marc, do you have some insight on the /* LPAE PAR format */ comment above?
Thanks,
-Christoffer
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
