Re: [PATCH RFC 5/5] KVM: This is the main part of the "moving dirty bitmaps to user space"

[Avi Kivity <avi@redhat.com>]

On 04/09/2010 12:38 PM, Takuya Yoshikawa wrote:
> By this patch, bitmap allocation is replaced with do_mmap() and
> bitmap manipulation is replaced with *_user() functions.
>
> Note that this does not change the APIs between kernel and user space.
> To get more advantage from this hack, we need to add a new interface
> for triggering the bitmap swith and getting the bitmap addresses: the
> addresses is in user space and we can export them to qemu.
>
>    

I would like to merge the new API together with the patchset, since 
without it, the improvement is insufficient.

> TODO:
> 1. We want to use copy_in_user() for 32bit case too.
>    

Definitely.  Why doesn't it work now?

>     Note that this is only for the compatibility issue: in the future,
>     we hope, qemu will not need to use this ioctl.
> 2. We have to implement test_bit_user() to avoid extra set_bit.
>    

This was important in the days of shadow paging.  I'm not so sure about 
it with nested paging, since we'll typically only fault a page once per 
iteration.  Since we're very likely to actually write these days, the 
extra access is wasteful.

>
> +int kvm_arch_create_dirty_bitmap(struct kvm_memory_slot *memslot)
> +{
> +	unsigned long user_addr1;
> +	unsigned long user_addr2;
> +	int dirty_bytes = kvm_dirty_bitmap_bytes(memslot);
> +
> +	down_write(&current->mm->mmap_sem);
> +	user_addr1 = do_mmap(NULL, 0, dirty_bytes,
> +			     PROT_READ | PROT_WRITE,
> +			     MAP_PRIVATE | MAP_ANONYMOUS, 0);
> +	if (IS_ERR((void *)user_addr1)) {
> +		up_write(&current->mm->mmap_sem);
> +		return PTR_ERR((void *)user_addr1);
> +	}
> +	user_addr2 = do_mmap(NULL, 0, dirty_bytes,
> +			     PROT_READ | PROT_WRITE,
> +			     MAP_PRIVATE | MAP_ANONYMOUS, 0);
> +	if (IS_ERR((void *)user_addr2)) {
> +		do_munmap(current->mm, user_addr1, dirty_bytes);
> +		up_write(&current->mm->mmap_sem);
> +		return PTR_ERR((void *)user_addr2);
> +	}
> +	up_write(&current->mm->mmap_sem);
>    

I think a single do_mmap() call for both bitmaps is simpler in terms of 
error handling.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.