Re: [kvm-devel] [BUG] Oops with KVM-27

[Avi Kivity <avi@qumranet.com>]

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.

> cmpxchg_emulated; unfortunately this means that the non-locked write
> path (write_emulated) can't do this optimization, unless I change its
> signature to include the old value.
>
> The first patch makes the writeback step uncoditional whenever we have a
> destination operand (the mov check (d & Mov) may be superfluous, yes?).
> The write-to-registry path still has the optimization that skips the
> write if possible.
>   

The mov check is in done since the destination is not read for moves; so
the check for change would read uninitialized memory.

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().

> Next one: I've splitted emulator_write_phys into emulator_write_phys_mem
> (for normal RAM) and emulator_write_phys_mmio (for the rest). The
>   

This is in order to properly emulate cmpxchg(), right?  I don't think
it's necessary since all that memory is write protected and the
modifications happen under kvm->lock.

>
> I'm a bit confused about this test, found in emulator_write_phys
> (original code):
>
> if (((gpa + bytes - 1) >> PAGE_SHIFT) != (gpa >> PAGE_SHIFT))
>         return 0;
>
> AFAICT is makes the emulator skip the write if the modified area spans
> across two different (physical) pages. When this happens write_emulated
> does a MMIO write. I'd expect the function to load the 2 pages and do the
> memory write on both instead.
>   

This isn't skipping the write; instead it uses the mmio path to update
memory instead of the direct path, as I was too lazy to code split page
writes.  It's also wrong in case someone uses nonaligned writes to
update page tables, but no-one does that.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.