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

[Avi Kivity <avi@redhat.com>]

On 04/20/2010 02:03 PM, Takuya Yoshikawa wrote:
> We can now export the addree of the bitmap created by do_mmap()
> to user space. For the sake of this, we introduce a new API:
>
>    KVM_SWITCH_DIRTY_LOG: application can use this to trigger the
>    switch of the bitmaps and get the address of the bitmap which
>    has been used until now. This reduces the copy of the dirty
>    bitmap from the kernel.
>
>
> diff --git a/Documentation/kvm/api.txt b/Documentation/kvm/api.txt
> index baa8fde..f3d1c2a 100644
> --- a/Documentation/kvm/api.txt
> +++ b/Documentation/kvm/api.txt
> @@ -848,6 +848,29 @@ function properly, this is the place to put them.
>          __u8  pad[64];
>   };
>
> +4.37 KVM_SWITCH_DIRTY_LOG (vm ioctl)
> +
> +Capability: basic
>    

Capability: basic means that this is available on all kvm versions, 
which isn't the case.  This should say KVM_CAP_USER_DIRTY_BITMAP.
> +Architectures: x86
>    

All architecures...

> +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.

> +};
> +
> +Given a memory slot, return the address of a bitmap containing any
> +pages dirtied since the last call to this ioctl.  Bit 0 is the first
> +page in the memory slot.  Ensure the entire structure is cleared to
> +avoid padding issues.
>    

Document that bitmaps but be aligned and padded to 64-bits to avoid 
32-on-64 issues.  Explain that dirty_bitmap will be the next returned 
(do we actually need the return value?  userspace can simply remember it 
from the last call).

How is the dirty log set when we start logging?  With kernel allocated 
bitmaps this isn't a problem, but with user allocated bitmaps?

> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index ad55353..45b728c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2718,11 +2718,9 @@ static int kvm_vm_ioctl_reinject(struct kvm *kvm,
>   	return 0;
>   }
>
> -/*
> - * Get (and clear) the dirty memory log for a memory slot.
> - */
> -int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
> -				      struct kvm_dirty_log *log)
> +static int kvm_x86_update_dirty_log(struct kvm *kvm,
> +				    struct kvm_dirty_log *log,
> +				    bool need_copy)
>   {
>   	int r;
>   	struct kvm_memory_slot *memslot;
> @@ -2773,12 +2771,34 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>   		dirty_bitmap_old = dirty_bitmap;
>   	}
>
> -	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?

> diff --git a/include/linux/kvm.h b/include/linux/kvm.h
> index 23ea022..9fa6f1e 100644
> --- a/include/linux/kvm.h
> +++ b/include/linux/kvm.h
> @@ -12,7 +12,7 @@
>   #include<linux/ioctl.h>
>   #include<asm/kvm.h>
>
> -#define KVM_API_VERSION 12
> +#define KVM_API_VERSION 13
>    

As Alex says, this breaks all known userspace.

>
>   /* *** Deprecated interfaces *** */
>
> @@ -318,7 +318,7 @@ struct kvm_dirty_log {
>   	__u32 padding1;
>   	union {
>   		void __user *dirty_bitmap; /* one bit per page */
> -		__u64 padding2;
> +		__u64 addr;
>   	};
>   };
>    

Update the comment above to note it applies to KVM_SWITCH_DIRTY_LOG.

> @@ -1699,6 +1714,21 @@ static long kvm_vm_ioctl(struct file *filp,
>   			goto out;
>   		break;
>   	}
> +	case KVM_SWITCH_DIRTY_LOG: {
> +		struct kvm_dirty_log log;
> +
> +		r = -EFAULT;
> +		if (copy_from_user(&log, argp, sizeof log))
> +			goto out;
> +		r = kvm_vm_ioctl_switch_dirty_log(kvm,&log);
> +		if (r)
> +			goto out;
> +		r = -EFAULT;
> +		if (copy_to_user(argp,&log, sizeof log))
> +			goto out;
> +		r = 0;
>    

You return 0, contrary to the documentation...

-- 
error compiling committee.c: too many arguments to function