From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Date: Wed, 21 Apr 2010 11:26:28 +0000 Subject: Re: [PATCH RFC v2 5/6] KVM: moving dirty bitmaps to user space Message-Id: <4BCEE0E4.6060707@redhat.com> List-Id: References: <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> In-Reply-To: <20100420200225.efca602f.yoshikawa.takuya@oss.ntt.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kvm-ia64@vger.kernel.org On 04/20/2010 02:02 PM, Takuya Yoshikawa wrote: > We move dirty bitmaps to user space. > > - Allocation and destruction: we use do_mmap() and do_munmap(). > The new bitmap space is twice longer than the original one and we > use the additional space for double buffering: this makes it > possible to update the active bitmap while letting the user space > read the other one safely. > > - Bitmap manipulations: we replace all functions which access dirty > bitmaps to *_user() functions. Note that some of these should be > optimized later. > > - For ia64: moving the dirty bitmaps of memory slots does not effect > much to ia64 because it's using a different space to store bitmaps > which is directly updated: all we have to change are sync and get > of dirty log, so we don't need set_bit_user like function for ia64. > > Signed-off-by: Takuya Yoshikawa > Signed-off-by: Fernando Luis Vazquez Cao > --- > arch/ia64/kvm/kvm-ia64.c | 12 ++++- > arch/powerpc/kvm/book3s.c | 2 +- > arch/x86/kvm/x86.c | 24 +++++----- > include/linux/kvm_host.h | 5 +- > virt/kvm/kvm_main.c | 101 ++++++++++++++++++++++++++++++++++++++++----- > 5 files changed, 116 insertions(+), 28 deletions(-) > > diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c > index d60dafe..c3f0b70 100644 > --- a/arch/ia64/kvm/kvm-ia64.c > +++ b/arch/ia64/kvm/kvm-ia64.c > @@ -1823,11 +1823,19 @@ static int kvm_ia64_sync_dirty_log(struct kvm *kvm, > n = kvm_dirty_bitmap_bytes(memslot); > base = memslot->base_gfn / BITS_PER_LONG; > > + r = -EFAULT; > + if (!access_ok(VERIFY_WRITE, memslot->dirty_bitmap, n)) > + goto out; > + > for (i = 0; i< n/sizeof(long); ++i) { > if (dirty_bitmap[base + i]) > memslot->is_dirty = true; > > - memslot->dirty_bitmap[i] = dirty_bitmap[base + i]; > + if (__put_user(dirty_bitmap[base + i], > + &memslot->dirty_bitmap[i])) { > + r = -EFAULT; > + goto out; > + } > dirty_bitmap[base + i] = 0; > } > r = 0; > @@ -1858,7 +1866,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, > if (memslot->is_dirty) { > kvm_flush_remote_tlbs(kvm); > n = kvm_dirty_bitmap_bytes(memslot); > - memset(memslot->dirty_bitmap, 0, n); > + clear_user(memslot->dirty_bitmap, n); > memslot->is_dirty = false; > Does this need an error check? > @@ -468,8 +480,12 @@ void kvm_free_physmem(struct kvm *kvm) > int i; > struct kvm_memslots *slots = kvm->memslots; > > - for (i = 0; i< slots->nmemslots; ++i) > + for (i = 0; i< slots->nmemslots; ++i) { > + /* We don't munmap dirty bitmaps by ourselves. */ > Why not? If we allocated them, we have to free them. > + slots->memslots[i].dirty_bitmap = NULL; > + slots->memslots[i].dirty_bitmap_old = NULL; > kvm_free_physmem_slot(&slots->memslots[i], NULL); > + } > > kfree(kvm->memslots); > } > @@ -523,13 +539,22 @@ static int kvm_vm_release(struct inode *inode, struct file *filp) > > static int kvm_create_dirty_bitmap(struct kvm_memory_slot *memslot) > { > - unsigned long dirty_bytes = kvm_dirty_bitmap_bytes(memslot); > + unsigned long user_addr; > + unsigned long n = kvm_dirty_bitmap_bytes(memslot); > > - memslot->dirty_bitmap = vmalloc(dirty_bytes); > - if (!memslot->dirty_bitmap) > - return -ENOMEM; > + down_write(¤t->mm->mmap_sem); > + user_addr = do_mmap(NULL, 0, 2 * n, > + PROT_READ | PROT_WRITE, > + MAP_PRIVATE | MAP_ANONYMOUS, 0); > + up_write(¤t->mm->mmap_sem); > + > + if (IS_ERR((void *)user_addr)) > + return PTR_ERR((void *)user_addr); > + > + memslot->dirty_bitmap = (unsigned long __user *)user_addr; > + memslot->dirty_bitmap_old = (unsigned long __user *)(user_addr + n); > + clear_user(memslot->dirty_bitmap, 2 * n); > Error check. > > - memset(memslot->dirty_bitmap, 0, dirty_bytes); > return 0; > } > > @@ -778,13 +803,45 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, > } > > int kvm_copy_dirty_bitmap(unsigned long __user *to, > - const unsigned long *from, > + const unsigned long __user *from, > unsigned long bytes) > { > - if (copy_to_user(to, from, bytes)) > +#if defined(CONFIG_X86_64) || defined(CONFIG_PPC64) || defined(CONFIG_IA64) > + if (copy_in_user(to, from, bytes)) { > + printk(KERN_WARNING "%s: copy_in_user failed.\n", __func__); > return -EFAULT; > + } > + return 0; > +#else > + int num, bufbytes; > + unsigned long buf[32]; > > + if (!access_ok(VERIFY_READ, from, bytes) || > + !access_ok(VERIFY_WRITE, to, bytes)) { > + goto out_fault; > + } > + > + bufbytes = sizeof(buf); > + num = bufbytes / sizeof(buf[0]); > + > + for (; bytes> bufbytes; bytes -= bufbytes, to += num, from += num) { > + if (__copy_from_user(buf, from, bufbytes)) > + goto out_fault; > + if (__copy_to_user(to, buf, bufbytes)) > + goto out_fault; > + } > + if (bytes> 0) { > + if (__copy_from_user(buf, from, bytes)) > + goto out_fault; > + if (__copy_to_user(to, buf, bytes)) > + goto out_fault; > + } > return 0; > + > +out_fault: > + printk(KERN_WARNING "%s: copy to(from) user failed.\n", __func__); > + return -EFAULT; > +#endif > } > This really wants to be hidden in lib/. > > int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log) > @@ -1194,13 +1251,35 @@ int kvm_clear_guest(struct kvm *kvm, gpa_t gpa, unsigned long len) > } > EXPORT_SYMBOL_GPL(kvm_clear_guest); > > +/* > + * Please use generic *_user bitops once they become available. > + * Be careful setting the bit won't be done atomically. > + */ > Please introduce the user bitops as part of this patchset. -- error compiling committee.c: too many arguments to function