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

On 04/22/2010 12:34 PM, Takuya Yoshikawa wrote:
> 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.

I guess it's better, since this patch is a "future expansion" of 
KVN_GET_DIRTY_LOG.  If we had a flags field and some room there, we 
could keep the old ioctl.

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