From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH RFC 5/5] KVM: This is the main part of the "moving dirty bitmaps to user space" Date: Sun, 11 Apr 2010 20:21:31 +0300 Message-ID: <4BC2051B.9090101@redhat.com> References: <20100409182732.857de4db.yoshikawa.takuya@oss.ntt.co.jp> <20100409183808.b72fc9a3.yoshikawa.takuya@oss.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: mtosatti@redhat.com, kvm@vger.kernel.org, fernando@oss.ntt.co.jp To: Takuya Yoshikawa Return-path: Received: from mx1.redhat.com ([209.132.183.28]:13037 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752336Ab0DKRVi (ORCPT ); Sun, 11 Apr 2010 13:21:38 -0400 In-Reply-To: <20100409183808.b72fc9a3.yoshikawa.takuya@oss.ntt.co.jp> Sender: kvm-owner@vger.kernel.org List-ID: 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(¤t->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(¤t->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(¤t->mm->mmap_sem); > + return PTR_ERR((void *)user_addr2); > + } > + up_write(¤t->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.