On Thu, Aug 23, 2012 at 3:19 AM, Rusty Russell <rusty@xxxxxxxxxxxxxxx> wrote:
> There is a race (SMP guest and host only) where the instruction we
> faulted on is unmapped before we get to reading it. In this (very
> unlikely!) case, we should simply return to the guest. It will
> probably do the right thing next time.
>
> Signed-off-by: Rusty Russell <rusty.russell@xxxxxxxxxx>
>
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6909a83..89ede94 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -587,11 +587,11 @@ int kvm_handle_mmio_return(struct kvm_vcpu *vcpu, struct kvm_run *run)
> * @len: length to copy
> * @priv: use guest PL1 (ie. kernel) mappings, otherwise use guest PL0 mappings.
> *
> - * Returns 0 on success, or errno.
> + * Returns true on success, false on failure (unlikely, but retry).
> */
> -static int copy_from_guest_va(struct kvm_vcpu *vcpu,
> - void *dest, unsigned long va, size_t len,
> - bool priv)
> +static bool 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;
> @@ -602,8 +602,9 @@ static int copy_from_guest_va(struct kvm_vcpu *vcpu,
> 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;
> + kvm_err("I/O Abort from invalid instruction address"
> + " %#lx!\n", va);
> + return false;
oh the race was in this patch, never mind my previous comment then.
Maybe the print should still be a trace event rather than a print, but
on the other hand, this is hard to provoke even purposely from the
guest I would think.
> }
>
> if (par & (1U << 11)) {
> @@ -626,13 +627,13 @@ static int copy_from_guest_va(struct kvm_vcpu *vcpu,
>
> err = kvm_read_guest(vcpu->kvm, pc_ipa, dest, cur_len);
> if (unlikely(err))
> - return err;
> + return false;
>
> len -= cur_len;
> va += cur_len;
> } while (unlikely(len != 0));
>
> - return 0;
> + return true;
> }
>
> /**
> @@ -649,7 +650,6 @@ static int copy_from_guest_va(struct kvm_vcpu *vcpu,
> static int invalid_io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
> {
> unsigned long instr;
> - int err;
>
> if (vcpu->arch.regs.cpsr & PSR_T_BIT) {
> /* TODO: Check validity of PC IPA and IPA2!!! */
> @@ -658,12 +658,10 @@ static int invalid_io_mem_abort(struct kvm_vcpu *vcpu, phys_addr_t fault_ipa)
> return -EINVAL;
> }
>
> - 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 err;
> - }
> + /* If it fails (SMP race?), we reenter guest for it to retry. */
that question mark really makes this whole patch fall apart ;)
> + if (!copy_from_guest_va(vcpu, &instr, vcpu->arch.regs.pc, sizeof(instr),
> + vcpu_mode_priv(vcpu)))
> + return 1;
>
> return kvm_emulate_mmio_ls(vcpu, fault_ipa, instr);
> }
looks good, thanks.
_______________________________________________
kvmarm mailing list
kvmarm@xxxxxxxxxxxxxxxxxxxxx
https://lists.cs.columbia.edu/cucslists/listinfo/kvmarm
