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

Avi Kivity
2010-04-11T17:21:38Z

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.