From mboxrd@z Thu Jan 1 00:00:00 1970 From: Carsten Otte Subject: Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch Date: Mon, 19 Nov 2007 15:55:58 +0100 Message-ID: <4741A3FE.70908@de.ibm.com> References: <42DFA526FC41B1429CE7279EF83C6BDC9A0A7D@pdsmsx415.ccr.corp.intel.com> <4741A011.2080405@de.ibm.com> <4741A322.8040201@qumranet.com> Reply-To: carsteno-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org, carsteno-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org, Christian Ehrhardt , Hollis Blanchard , "Zhang, Xiantao" To: Avi Kivity Return-path: In-Reply-To: <4741A322.8040201-atKUWr5tajBWk0Htik3J/w@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Avi Kivity wrote: > Carsten Otte wrote: >> Zhang, Xiantao wrote: >>> Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface >>> in common. >> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c >> index 45b18e1..3400265 100644 >> --- a/drivers/kvm/kvm_main.c >> +++ b/drivers/kvm/kvm_main.c >> @@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm, >> return kvm_set_memory_region(kvm, mem, user_alloc); >> } >> >> -/* >> - * Get (and clear) the dirty memory log for a memory slot. >> - */ >> -static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, >> - struct kvm_dirty_log *log) >> +int kvm_get_dirty_log(struct kvm *kvm, >> + struct kvm_dirty_log *log, int *is_dirty) >> >> Any reason to remove that comment? It looks helpful to me. >> >> >> { >> struct kvm_memory_slot *memslot; >> int r, i; >> int n; >> unsigned long any = 0; >> >> - mutex_lock(&kvm->lock); >> - >> r = -EINVAL; >> if (log->slot >= KVM_MEMORY_SLOTS) >> goto out; >> @@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm >> *kvm, >> if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n)) >> goto out; >> >> - /* If nothing is dirty, don't bother messing with page tables. */ >> - if (any) { >> - kvm_mmu_slot_remove_write_access(kvm, log->slot); >> - kvm_flush_remote_tlbs(kvm); >> - memset(memslot->dirty_bitmap, 0, n); >> - } >> + if (any) >> + *is_dirty = 1; >> >> r = 0; >> - >> out: >> - mutex_unlock(&kvm->lock); >> return r; >> } >> >> This split won't work on s390. I think kvm_get_dirty_log should be >> arch dependent alltogether: we're not going to have a dirty bitmap on >> s390, we rather rely on our hardware support to update our storage >> keys accordingly without guest/host intervention required. We'd want >> to use that, and thus this implementation of kvm_get_dirty_log makes >> no sense for us. On the other hand, I'd really want to keep the ioctl >> common, so that guest migration code in userland can be common for all >> archs. > > On the other hand, it will work for all others IIUC. Duplicating it in > all other archs is not a good idea. > > We can special case it using #ifdef ARCH_HAS_KVM_GUEST_DIRTY_BITMAP or > something. > > (hides from the #ifdef police) Yea, I agree that it would make sense in case it works for power too. We could have an #ifdef CONFIG_ARCH_S390 return cool_big_iron_get_dirty_log(args); #endif at the beginning of that function. It would'nt hurt readability too much. Hollis? Christian? How about ppc? ------------------------------------------------------------------------- This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2005. http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/