Re: [PATCH RFC v2 6/6] KVM: introduce a new API for getting dirty bitmaps

[Takuya Yoshikawa <yoshikawa.takuya-gVGce1chcLdL9jVzuh4AOg@public.gmane.org>]

Thanks, I can know basic rules about kvm/api .

(2010/04/21 20:46), Avi Kivity wrote:

>
>> +Type: vm ioctl
>> +Parameters: struct kvm_dirty_log (in/out)
>> +Returns: 0 on success, -1 on error
>> +
>> +/* for KVM_SWITCH_DIRTY_LOG */
>> +struct kvm_dirty_log {
>> + __u32 slot;
>> + __u32 padding;
>
> Please put a flags field here (and verify it is zero in the ioctl
> implementation). This allows us to extend it later.
>
>> + union {
>> + void __user *dirty_bitmap; /* one bit per page */
>> + __u64 addr;
>> + };
>
> Just make dirty_bitmap a __u64. With the union there is the risk that
> someone forgets to zero the structure so we get some random bits in the
> pointer, and also issues with big endian and s390 compat.
>
> Reserve some extra space here for future expansion.
>
> Hm. I see that this is the existing kvm_dirty_log structure. So we can't
> play with it, ignore my comments about it.

So, introducing a new structure to export(and get) two bitmap addresses
with a flag is the thing?

1. Qemu calls ioctl to get the two addresses.
2. Qemu calls ioctl to make KVM switch the dirty bitmaps.
    --> in this case, qemu have to change the address got in step 1.
    ...
3. Qemu calls ioctl(we can reuse 1. with different command flag) to free the bitmaps.


In my plan, we don't let qemu malloc() dirty bitmaps: at least, I want to make that
another patch set because this patch set already has some dependencies to other issues.

But, yes, I can make the struct considering the future expansion if it needed.


>>
>> - r = kvm_copy_dirty_bitmap(log->dirty_bitmap, dirty_bitmap_old, n);
>> + if (need_copy) {
>> + r = kvm_copy_dirty_bitmap(log->dirty_bitmap,
>> + dirty_bitmap_old, n);
>> + } else {
>> + log->addr = (unsigned long)dirty_bitmap_old;
>> + r = 0;
>> + }
>
> Instead of passing need_copy everywhere, you might check a flag in
> log->. You'll need a flag to know whether to munmap() or not, no?

To judge munmap() or not, putting in the memory slot's flag may be
more useful.

But as you suggest, we won't use kvm_dirty_log so parameters will change.