lkml.org 
[lkml]   [2007]   [Jun]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [kvm-devel] [BUG] Oops with KVM-27
    Il Sat, Jun 16, 2007 at 10:43:23AM +0300, Avi Kivity ha scritto: 
    > Luca Tettamanti wrote:
    > > Il Fri, Jun 15, 2007 at 12:06:50PM +0300, Avi Kivity ha scritto:
    > >
    > >>> After a bit of thinking: it's correct but removes an optimization;
    > >>> furthermore it may miss other instructions that write to memory mapped
    > >>> areas.
    > >>> A more proper fix should be "force the writeback if dst.ptr is in some
    > >>> kind of mmio area".
    > >>>
    > >>>
    > >> I think we can just disable the optimization, and (in a separate patch)
    > >> add it back in emulator_write_phys(), where we know we're modifying
    > >> memory and not hitting mmio.
    > >>
    > >
    > > Problem is that in emulator_write_phys() we've already lost track of the
    > > previous (dst.orig_val) value. I moved up the decision in
    > >
    >
    > Actually we haven't; just before the memcpy(), we can put a memcmp() to
    > guard the kvm_mmu_pte_write(), which is the really expensive operation,
    > especially with guest smp.

    Yup, but it seemed wasteful to map (at least when highmem is in use) a
    page just to check for something that we already knew. That was a
    preemptive optmization though, I haven't actually benchmarked the cost
    of setting up the mapping ;-)

    [...]
    > I think we can simply remove the if (). For the register case, the
    > check is more expensive that the write; for mmio, we don't want it; and
    > for memory writes, we can put it in emulator_write_phys().

    Ok, this way it's simpler. How does this look:

    --- a/kernel/x86_emulate.c 2007-06-15 21:13:51.000000000 +0200
    +++ b/kernel/x86_emulate.c 2007-06-17 16:57:50.000000000 +0200
    @@ -1057,40 +1057,38 @@
    }

    writeback:
    - if ((d & Mov) || (dst.orig_val != dst.val)) {
    - switch (dst.type) {
    - case OP_REG:
    - /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
    - switch (dst.bytes) {
    - case 1:
    - *(u8 *)dst.ptr = (u8)dst.val;
    - break;
    - case 2:
    - *(u16 *)dst.ptr = (u16)dst.val;
    - break;
    - case 4:
    - *dst.ptr = (u32)dst.val;
    - break; /* 64b: zero-ext */
    - case 8:
    - *dst.ptr = dst.val;
    - break;
    - }
    + switch (dst.type) {
    + case OP_REG:
    + /* The 4-byte case *is* correct: in 64-bit mode we zero-extend. */
    + switch (dst.bytes) {
    + case 1:
    + *(u8 *)dst.ptr = (u8)dst.val;
    break;
    - case OP_MEM:
    - if (lock_prefix)
    - rc = ops->cmpxchg_emulated((unsigned long)dst.
    - ptr, &dst.orig_val,
    - &dst.val, dst.bytes,
    - ctxt);
    - else
    - rc = ops->write_emulated((unsigned long)dst.ptr,
    - &dst.val, dst.bytes,
    - ctxt);
    - if (rc != 0)
    - goto done;
    - default:
    + case 2:
    + *(u16 *)dst.ptr = (u16)dst.val;
    + break;
    + case 4:
    + *dst.ptr = (u32)dst.val;
    + break; /* 64b: zero-ext */
    + case 8:
    + *dst.ptr = dst.val;
    break;
    }
    + break;
    + case OP_MEM:
    + if (lock_prefix)
    + rc = ops->cmpxchg_emulated((unsigned long)dst.
    + ptr, &dst.orig_val,
    + &dst.val, dst.bytes,
    + ctxt);
    + else
    + rc = ops->write_emulated((unsigned long)dst.ptr,
    + &dst.val, dst.bytes,
    + ctxt);
    + if (rc != 0)
    + goto done;
    + default:
    + break;
    }

    /* Commit shadow register state. */
    --- a/kernel/kvm_main.c 2007-06-15 21:18:08.000000000 +0200
    +++ b/kernel/kvm_main.c 2007-06-17 16:59:33.000000000 +0200
    @@ -1139,8 +1139,10 @@
    return 0;
    mark_page_dirty(vcpu->kvm, gpa >> PAGE_SHIFT);
    virt = kmap_atomic(page, KM_USER0);
    - kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
    - memcpy(virt + offset_in_page(gpa), val, bytes);
    + if (memcmp(virt + offset_in_page(gpa), val, bytes)) {
    + kvm_mmu_pte_write(vcpu, gpa, virt + offset, val, bytes);
    + memcpy(virt + offset_in_page(gpa), val, bytes);
    + }
    kunmap_atomic(virt, KM_USER0);
    return 1;
    }


    Luca
    --
    Runtime error 6D at f000:a12f : user incompetente
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-06-17 17:17    [W:0.032 / U:2.264 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site