On Thu, Apr 18, 2013 at 12:00:16PM +0800, Xiao Guangrong wrote: > > > > What is the justification for this? > > We want the rmap of being deleted memslot is removed-only that is > needed for unmapping rmap out of mmu-lock. > > ====== > 1) do not corrupt the rmap > 2) keep pte-list-descs available > 3) keep shadow page available > > Resolve 1): > we make the invalid rmap be remove-only that means we only delete and > clear spte from the rmap, no new sptes can be added to it. > This is reasonable since kvm can not do address translation on invalid rmap > (gfn_to_pfn is failed on invalid memslot) and all sptes on invalid rmap can > not be reused (they belong to invalid shadow page). > ====== > > clear_flush_young / test_young / change_pte of mmu-notify can rewrite > rmap with the present-spte (P bit is set), we should umap rmap in > these handlers. > > > > >> + > >> + /* > >> + * To ensure that all vcpus and mmu-notify are not clearing > >> + * spte and rmap entry. > >> + */ > >> + synchronize_srcu_expedited(&kvm->srcu); > >> +} > >> + > >> #ifdef MMU_DEBUG > >> static int is_empty_shadow_page(u64 *spt) > >> { > >> @@ -2219,6 +2283,11 @@ static void clear_sp_write_flooding_count(u64 *spte) > >> __clear_sp_write_flooding_count(sp); > >> } > >> > >> +static bool is_valid_sp(struct kvm *kvm, struct kvm_mmu_page *sp) > >> +{ > >> + return likely(sp->mmu_valid_gen == kvm->arch.mmu_valid_gen); > >> +} > >> + > >> static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > >> gfn_t gfn, > >> gva_t gaddr, > >> @@ -2245,6 +2314,9 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct > >> kvm_vcpu *vcpu, > >> role.quadrant = quadrant; > >> } > >> for_each_gfn_sp(vcpu->kvm, sp, gfn) { > >> + if (!is_valid_sp(vcpu->kvm, sp)) > >> + continue; > >> + > >> if (!need_sync && sp->unsync) > >> need_sync = true; > >> > >> @@ -2281,6 +2353,7 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct > >> kvm_vcpu *vcpu, > >> > >> account_shadowed(vcpu->kvm, gfn); > >> } > >> + sp->mmu_valid_gen = vcpu->kvm->arch.mmu_valid_gen; > >> init_shadow_page_table(sp); > >> trace_kvm_mmu_get_page(sp, true); > >> return sp; > >> @@ -2451,8 +2524,12 @@ static int kvm_mmu_prepare_zap_page(struct kvm > >> *kvm, struct kvm_mmu_page *sp, > >> ret = mmu_zap_unsync_children(kvm, sp, invalid_list); > >> kvm_mmu_page_unlink_children(kvm, sp); > > > > The rmaps[] arrays linked to !is_valid_sp() shadow pages should not be > > accessed (as they have been freed already). > > > > I suppose the is_valid_sp() conditional below should be moved earlier, > > before kvm_mmu_unlink_parents or any other rmap access. > > > > This is fine: the !is_valid_sp() shadow pages are only reachable > > by SLAB and the hypervisor itself. > > Unfortunately we can not do this. :( > > The sptes in shadow pape can linked to many slots, if the spte is linked > to the rmap of being deleted memslot, it is ok, otherwise, the rmap of > still used memslot is miss updated. > > For example, slot 0 is being deleted, sp->spte[0] is linked on slot[0].rmap, > sp->spte[1] is linked on slot[1].rmap. If we do not access rmap of this 'sp', > the already-freed spte[1] is still linked on slot[1].rmap. > > We can let kvm update the rmap for sp->spte[1] and do not unlink sp->spte[0]. > This is also not allowed since mmu-notify can access the invalid rmap before > the memslot is destroyed, then mmu-notify will get already-freed spte on > the rmap or page Access/Dirty is miss tracked (if let mmu-notify do not access > the invalid rmap).
Why not release all rmaps? Subject: [PATCH v2 3/7] KVM: x86: introduce kvm_clear_all_gfn_page_info This function is used to reset the rmaps and page info of all guest page which will be used in later patch Signed-off-by: Xiao Guangrong <xiaoguangr...@linux.vnet.ibm.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html