| Message ID | 20151120174717.7b0c31caa267aa83027e8d8f@lab.ntt.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
You just ignored my comment on the previous version... On 11/20/2015 04:47 PM, Takuya Yoshikawa wrote: > kvm_mmu_mark_parents_unsync() alone uses pte_list_walk(), witch does > nearly the same as the for_each_rmap_spte macro. The only difference > is that is_shadow_present_pte() checks cannot be placed there because > kvm_mmu_mark_parents_unsync() can be called with a new parent pointer > whose entry is not set yet. > > By calling mark_unsync() separately for the parent and adding the parent > pointer to the parent_ptes chain later in kvm_mmu_get_page(), the macro > works with no problem. > > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> > --- > arch/x86/kvm/mmu.c | 36 +++++++++++++----------------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 7f46e3e..4e29d9a 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -1007,26 +1007,6 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) > } > } > > -typedef void (*pte_list_walk_fn) (u64 *spte); > -static void pte_list_walk(struct kvm_rmap_head *rmap_head, pte_list_walk_fn fn) > -{ > - struct pte_list_desc *desc; > - int i; > - > - if (!rmap_head->val) > - return; > - > - if (!(rmap_head->val & 1)) > - return fn((u64 *)rmap_head->val); > - > - desc = (struct pte_list_desc *)(rmap_head->val & ~1ul); > - while (desc) { > - for (i = 0; i < PTE_LIST_EXT && desc->sptes[i]; ++i) > - fn(desc->sptes[i]); > - desc = desc->more; > - } > -} > - > static struct kvm_rmap_head *__gfn_to_rmap(gfn_t gfn, int level, > struct kvm_memory_slot *slot) > { > @@ -1749,7 +1729,12 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct > static void mark_unsync(u64 *spte); > static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp) > { > - pte_list_walk(&sp->parent_ptes, mark_unsync); > + u64 *sptep; > + struct rmap_iterator iter; > + > + for_each_rmap_spte(&sp->parent_ptes, &iter, sptep) { > + mark_unsync(sptep); > + } > } > > static void mark_unsync(u64 *spte) > @@ -2119,12 +2104,17 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > if (sp->unsync && kvm_sync_page_transient(vcpu, sp)) > break; > > - mmu_page_add_parent_pte(vcpu, sp, parent_pte); > if (sp->unsync_children) { > kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > kvm_mmu_mark_parents_unsync(sp); > - } else if (sp->unsync) > + if (parent_pte) > + mark_unsync(parent_pte); > + } else if (sp->unsync) { > kvm_mmu_mark_parents_unsync(sp); > + if (parent_pte) > + mark_unsync(parent_pte); > + } > + mmu_page_add_parent_pte(vcpu, sp, parent_pte); > > __clear_sp_write_flooding_count(sp); > trace_kvm_mmu_get_page(sp, false); > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/11/20 17:46, Xiao Guangrong wrote: > > You just ignored my comment on the previous version... I'm sorry but please read the explanation in patch 00. I've read your comments and I'm not ignoring you. Since this patch set has become huge than expected, I'm sending this version so that patch 01-07 can be applied first. For patch 08-10, I think we need to check more because there seems to be some confusion between us. You can also read other discussions between Marcelo, Paolo and me. Anyway, since these three patches has been placed at the end of the series now, I hope we can concentrate on them easier than before. Thanks, Takuya -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 20/11/2015 09:47, Takuya Yoshikawa wrote: > kvm_mmu_mark_parents_unsync() alone uses pte_list_walk(), witch does > nearly the same as the for_each_rmap_spte macro. The only difference > is that is_shadow_present_pte() checks cannot be placed there because > kvm_mmu_mark_parents_unsync() can be called with a new parent pointer > whose entry is not set yet. > > By calling mark_unsync() separately for the parent and adding the parent > pointer to the parent_ptes chain later in kvm_mmu_get_page(), the macro > works with no problem. > > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> > --- > arch/x86/kvm/mmu.c | 36 +++++++++++++----------------------- > 1 file changed, 13 insertions(+), 23 deletions(-) > > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index 7f46e3e..4e29d9a 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -1007,26 +1007,6 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) > } > } > > -typedef void (*pte_list_walk_fn) (u64 *spte); > -static void pte_list_walk(struct kvm_rmap_head *rmap_head, pte_list_walk_fn fn) > -{ > - struct pte_list_desc *desc; > - int i; > - > - if (!rmap_head->val) > - return; > - > - if (!(rmap_head->val & 1)) > - return fn((u64 *)rmap_head->val); > - > - desc = (struct pte_list_desc *)(rmap_head->val & ~1ul); > - while (desc) { > - for (i = 0; i < PTE_LIST_EXT && desc->sptes[i]; ++i) > - fn(desc->sptes[i]); > - desc = desc->more; > - } > -} > - > static struct kvm_rmap_head *__gfn_to_rmap(gfn_t gfn, int level, > struct kvm_memory_slot *slot) > { > @@ -1749,7 +1729,12 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct > static void mark_unsync(u64 *spte); > static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp) > { > - pte_list_walk(&sp->parent_ptes, mark_unsync); > + u64 *sptep; > + struct rmap_iterator iter; > + > + for_each_rmap_spte(&sp->parent_ptes, &iter, sptep) { > + mark_unsync(sptep); > + } > } > > static void mark_unsync(u64 *spte) > @@ -2119,12 +2104,17 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, > if (sp->unsync && kvm_sync_page_transient(vcpu, sp)) > break; > > - mmu_page_add_parent_pte(vcpu, sp, parent_pte); > if (sp->unsync_children) { > kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); > kvm_mmu_mark_parents_unsync(sp); > - } else if (sp->unsync) > + if (parent_pte) > + mark_unsync(parent_pte); > + } else if (sp->unsync) { > kvm_mmu_mark_parents_unsync(sp); > + if (parent_pte) > + mark_unsync(parent_pte); > + } > + mmu_page_add_parent_pte(vcpu, sp, parent_pte); This patch is okay with Xiao's suggestion to remove the kvm_mmu_mark_parents_unsync call. Paolo > __clear_sp_write_flooding_count(sp); > trace_kvm_mmu_get_page(sp, false); > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index 7f46e3e..4e29d9a 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1007,26 +1007,6 @@ static void pte_list_remove(u64 *spte, struct kvm_rmap_head *rmap_head) } } -typedef void (*pte_list_walk_fn) (u64 *spte); -static void pte_list_walk(struct kvm_rmap_head *rmap_head, pte_list_walk_fn fn) -{ - struct pte_list_desc *desc; - int i; - - if (!rmap_head->val) - return; - - if (!(rmap_head->val & 1)) - return fn((u64 *)rmap_head->val); - - desc = (struct pte_list_desc *)(rmap_head->val & ~1ul); - while (desc) { - for (i = 0; i < PTE_LIST_EXT && desc->sptes[i]; ++i) - fn(desc->sptes[i]); - desc = desc->more; - } -} - static struct kvm_rmap_head *__gfn_to_rmap(gfn_t gfn, int level, struct kvm_memory_slot *slot) { @@ -1749,7 +1729,12 @@ static struct kvm_mmu_page *kvm_mmu_alloc_page(struct kvm_vcpu *vcpu, int direct static void mark_unsync(u64 *spte); static void kvm_mmu_mark_parents_unsync(struct kvm_mmu_page *sp) { - pte_list_walk(&sp->parent_ptes, mark_unsync); + u64 *sptep; + struct rmap_iterator iter; + + for_each_rmap_spte(&sp->parent_ptes, &iter, sptep) { + mark_unsync(sptep); + } } static void mark_unsync(u64 *spte) @@ -2119,12 +2104,17 @@ static struct kvm_mmu_page *kvm_mmu_get_page(struct kvm_vcpu *vcpu, if (sp->unsync && kvm_sync_page_transient(vcpu, sp)) break; - mmu_page_add_parent_pte(vcpu, sp, parent_pte); if (sp->unsync_children) { kvm_make_request(KVM_REQ_MMU_SYNC, vcpu); kvm_mmu_mark_parents_unsync(sp); - } else if (sp->unsync) + if (parent_pte) + mark_unsync(parent_pte); + } else if (sp->unsync) { kvm_mmu_mark_parents_unsync(sp); + if (parent_pte) + mark_unsync(parent_pte); + } + mmu_page_add_parent_pte(vcpu, sp, parent_pte); __clear_sp_write_flooding_count(sp); trace_kvm_mmu_get_page(sp, false);
kvm_mmu_mark_parents_unsync() alone uses pte_list_walk(), witch does nearly the same as the for_each_rmap_spte macro. The only difference is that is_shadow_present_pte() checks cannot be placed there because kvm_mmu_mark_parents_unsync() can be called with a new parent pointer whose entry is not set yet. By calling mark_unsync() separately for the parent and adding the parent pointer to the parent_ptes chain later in kvm_mmu_get_page(), the macro works with no problem. Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> --- arch/x86/kvm/mmu.c | 36 +++++++++++++----------------------- 1 file changed, 13 insertions(+), 23 deletions(-)