On 11/05/2014 08:09 AM, Paolo Bonzini wrote:
>
>
> On 01/11/2014 11:12, James Hogan wrote:
>> AFAICT all of the arch implementations of kvm_vm_ioctl_get_dirty_log()
>> except x86 and ppc hv (i.e. ia60, mips, ppc pv, s390) already make use
>> of the existing generic function kvm_get_dirty_log() to help implement
>> their kvm_vm_ioctl_get_dirty_log functions, which all look pretty
>> similar now except for TLB flushing.
>>
>> Would they not be a better base for a generic
>> kvm_vm_ioctl_get_dirty_log()?
>>
>> It feels a bit wrong to add a generic higher level function which
>> doesn't make use of the existing generic lower level abstraction.
>>
>> (Appologies if this has already been brought up in previous versions of
>> the patchset, I haven't been tracking them).
>
> I agree that we should make the interface look more like
> kvm_get_dirty_log(). Here the steps are:
>
> + * 1. Take a snapshot of the bit and clear it if needed.
> + * 2. Write protect the corresponding page.
> + * 3. Flush TLB's if needed.
> + * 4. Copy the snapshot to the userspace.
Hi Paolo,
thanks for breaking it down between generic/architecture layers,
helps a lot. Initially I thought we could get TLB flushing to
generic layer, previous x86 version worked for ARM. But looking
deeper other architectures either use non-generic flush or none
at all. Right now we would have x86, ARM, IA64 using generic TLB flush.
I'll restructure for another version.
>
> and I believe we can swap 3 and 4, since (4) is just a copy of data and
> it has no ordering relationship with the action of the guest. Once we
> do that, we can rewrite code to look a lot like kvm_get_dirty_log(),
> let's call it kvm_get_dirty_log_protect():
>
> r = -EINVAL;
> if (log->slot >= KVM_USER_MEM_SLOTS)
> goto out;
>
> memslot = id_to_memslot(kvm->memslots, log->slot);
>
> dirty_bitmap = memslot->dirty_bitmap;
> r = -ENOENT;
> if (!dirty_bitmap)
> goto out;
>
> n = kvm_dirty_bitmap_bytes(memslot);
>
> dirty_bitmap_buffer = dirty_bitmap + n / sizeof(long);
> memset(dirty_bitmap_buffer, 0, n);
>
> spin_lock(&kvm->mmu_lock);
>
> *is_dirty = false;
> for (i = 0; i < n / sizeof(long); i++) {
> unsigned long mask;
> gfn_t offset;
>
> if (!dirty_bitmap[i])
> continue;
>
> *is_dirty = true;
>
> mask = xchg(&dirty_bitmap[i], 0);
> dirty_bitmap_buffer[i] = mask;
>
> offset = i * BITS_PER_LONG;
> kvm_arch_write_protect_pt_masked(kvm, memslot, offset,
> mask);
> }
>
> spin_unlock(&kvm->mmu_lock);
>
> r = -EFAULT;
> if (copy_to_user(log->dirty_bitmap, dirty_bitmap_buffer, n))
> goto out;
>
> r = 0;
> out:
> return r;
>
> where the TLB flushing is moved to the caller as in kvm_get_dirty_log
> callers. Taking the slots lock would also be kept in the per-arch
> kvm_vm_ioctl_get_dirty_log, again similar to PPC/MIPS/S390.
Ok.
>
> You can add a new Kconfig symbol, or define an implementation of
> kvm_arch_write_protect_pt_masked that BUG()s for ia64/PPC/MIPS/S390.
Ok will do.
>
> BTW, you can leave the function in kvm_main.c.
Ok.
>
> Paolo
>
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
