From mboxrd@z Thu Jan 1 00:00:00 1970 From: Xiao Guangrong Subject: Re: [PATCH 04/12] KVM: MMU: log dirty page after marking spte writable Date: Wed, 20 Nov 2013 22:20:09 +0800 Message-ID: References: <1375189330-24066-1-git-send-email-xiaoguangrong@linux.vnet.ibm.com> <1375189330-24066-5-git-send-email-xiaoguangrong@linux.vnet.ibm.com> <20130807014828.GA4781@amt.cnet> <5201C7D9.20004@linux.vnet.ibm.com> <20131120002920.GA14230@amt.cnet> Mime-Version: 1.0 (Mac OS X Mail 7.0 \(1822\)) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Gleb Natapov , avi.kivity@gmail.com, "pbonzini@redhat.com Bonzini" , linux-kernel@vger.kernel.org, kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mail-pa0-f46.google.com ([209.85.220.46]:55517 "EHLO mail-pa0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751364Ab3KTOUX convert rfc822-to-8bit (ORCPT ); Wed, 20 Nov 2013 09:20:23 -0500 In-Reply-To: <20131120002920.GA14230@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On Nov 20, 2013, at 8:29 AM, Marcelo Tosatti wrot= e: > On Wed, Aug 07, 2013 at 12:06:49PM +0800, Xiao Guangrong wrote: >> On 08/07/2013 09:48 AM, Marcelo Tosatti wrote: >>> On Tue, Jul 30, 2013 at 09:02:02PM +0800, Xiao Guangrong wrote: >>>> Make sure we can see the writable spte before the dirt bitmap is v= isible >>>>=20 >>>> We do this is for kvm_vm_ioctl_get_dirty_log() write-protects the = spte based >>>> on the dirty bitmap, we should ensure the writable spte can be fou= nd in rmap >>>> before the dirty bitmap is visible. Otherwise, we cleared the dirt= y bitmap and >>>> failed to write-protect the page >>>>=20 >>>> Signed-off-by: Xiao Guangrong >>>> --- >>>> arch/x86/kvm/mmu.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>=20 >>> Can you explain why this is safe, with regard to the rule=20 >>> at edde99ce05290e50 ? >>=20 >> BTW, this log fixed this case: >>=20 >> VCPU 0 KVM migration control >>=20 >> write-protects all pages >> #Pf happen then the page >> become writable, set dirty >> bit on the bitmap >>=20 >> swap the bitmap, current bitmap is empty >>=20 >> write the page (no dirty log) >>=20 >> stop the guest and push >> the remaining dirty pages >> Stopped >> See current bitmap is empty that means >> no page is dirty. >>>=20 >>> "The rule is that all pages are either dirty in the current bitmap, >>> or write-protected, which is violated here." >>=20 >> Actually, this rule is not complete true, there's the 3th case: >> the window between write guest page and set dirty bitmap is valid. >> In that window, page is write-free and not dirty logged. >>=20 >> This case is based on the fact that at the final step of live migrat= ion, >> kvm should stop the guest and push the remaining dirty pages to the >> destination. >>=20 >> They're some examples in the current code: >> example 1, in fast_pf_fix_direct_spte(): >> if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) =3D=3D spte) >> /* The window in here... */ >> mark_page_dirty(vcpu->kvm, gfn); >>=20 >> example 2, in kvm_write_guest_page(): >> r =3D __copy_to_user((void __user *)addr + offset, data, len); >> if (r) >> return -EFAULT; >> /* >> * The window is here, the page is dirty but not logged in >> * The bitmap. >> */ >> mark_page_dirty(kvm, gfn); >> return 0; >=20 Hi Marcelo, > Why is this valid ? That is, the obviously correct rule is >=20 > "that all pages are either dirty in the current bitmap, > or write-protected, which is violated here." >=20 > With the window above, GET_DIRTY_LOG can be called 100 times while th= e=20 > page is dirty, but the corresponding bit not set in the dirty bitmap. >=20 > It violates the documentation: >=20 > /* for KVM_GET_DIRTY_LOG */ > struct kvm_dirty_log { > __u32 slot; > __u32 padding; > union { > void __user *dirty_bitmap; /* one bit per page */ > __u64 padding; > }; > }; >=20 > Given a memory slot, return a bitmap containing any pages dirtied > since the last call to this ioctl. Bit 0 is the first page in the > memory slot. Ensure the entire structure is cleared to avoid padding > issues. >=20 > The point about migration, is that GET_DIRTY_LOG is strictly correct > because it stops vcpus. >=20 > But what guarantee does userspace require, from GET_DIRTY_LOG, while = vcpus are > executing?=20 Aha. Single calling GET_DIRTY_LOG is useless since new dirty page can b= e generated when GET_DIRTY_LOG is being returned. If user wants to get exact dirty = pages the vcpus should be stopped.=20 >=20 > With fast page fault: >=20 > if (cmpxchg64(sptep, spte, spte | PT_WRITABLE_MASK) =3D=3D spte) > /* The window in here... */ > mark_page_dirty(vcpu->kvm, gfn); >=20 > And the $SUBJECT set_spte reordering, the rule becomes >=20 > A call to GET_DIRTY_LOG guarantees to return correct information abou= t=20 > dirty pages before invocation of the previous GET_DIRTY_LOG call. >=20 > (see example 1: the next GET_DIRTY_LOG will return the dirty informat= ion > there). >=20 It seems no. The first GET_DIRTY_LOG can happen before fast-page-fault=EF=BC=8C the second GET_DIRTY_LOG happens in the window between cmpxchg() and mark_page_dirty(), for the second one, the information is still =E2= =80=9Cincorrect=E2=80=9D. > The rule for sptes that is, because kvm_write_guest does not match th= e > documentation at all. You mean the case of =E2=80=9Ckvm_write_guest=E2=80=9D is valid (I do n= ot know why it is)? Or anything else? >=20 > So before example 1 and this patch, the rule (well for sptes at least= ) was >=20 > "Given a memory slot, return a bitmap containing any pages dirtied > since the last call to this ioctl. Bit 0 is the first page in the > memory slot. Ensure the entire structure is cleared to avoid padding > issues." >=20 > Can you explain why it is OK to relax this rule? It=E2=80=99s because: 1) it doesn=E2=80=99t break current use cases, i.e. Live migration and = =46B-flushing. 2) the current code, like kvm_write_guest has already broken the docum= entation (the guest page has been written but missed in the dirty bitmap). 3) it=E2=80=99s needless to implement a exact get-dirty-pages since the= dirty pages can no be exactly got except stopping vcpus.=20 So i think we'd document this case instead. No?