| Message ID | 20151106162501.e97c5a05063625ed1a266553@lab.ntt.co.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/11/2015 08:25, Takuya Yoshikawa wrote: > At some call sites of rmap_get_first() and rmap_get_next(), BUG_ON is > placed right after the call to detect unrelated sptes which should not > be found in the reverse-mapping list. > > Move this check in rmap_get_first/next() so that all call sites, not > just the users of the for_each_rmap_spte() macro, will be checked the > same way. In addition, change the BUG_ON to WARN_ON since killing the > whole host is the last thing that KVM should try. > > One thing to keep in mind is that kvm_mmu_unlink_parents() also uses > rmap_get_first() to handle parent sptes. The change will not break it > because parent sptes are present, at least until drop_parent_pte() > actually unlinks them, and not mmio-sptes. Can you also change kvm_mmu_mark_parents_unsync to use for_each_rmap_spte instead of pte_list_walk? It is the last use of pte_list_walk, and it's nice if we have two uses of for_each_rmap_spte with parent_ptes as the argument. BTW, on my todo list is to change the rmap items to a struct (with a single u64 inside) for type safety. Since you are touching this code, perhaps you can give it a shot? Paolo > Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> > --- > Documentation/virtual/kvm/mmu.txt | 4 ++-- > arch/x86/kvm/mmu.c | 31 ++++++++++++++++++++++--------- > 2 files changed, 24 insertions(+), 11 deletions(-) > > diff --git a/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt > index 3a4d681..daf9c0f 100644 > --- a/Documentation/virtual/kvm/mmu.txt > +++ b/Documentation/virtual/kvm/mmu.txt > @@ -203,10 +203,10 @@ Shadow pages contain the following information: > page cannot be destroyed. See role.invalid. > parent_ptes: > The reverse mapping for the pte/ptes pointing at this page's spt. If > - parent_ptes bit 0 is zero, only one spte points at this pages and > + parent_ptes bit 0 is zero, only one spte points at this page and > parent_ptes points at this single spte, otherwise, there exists multiple > sptes pointing at this page and (parent_ptes & ~0x1) points at a data > - structure with a list of parent_ptes. > + structure with a list of parent sptes. > unsync: > If true, then the translations in this page may not match the guest's > translation. This is equivalent to the state of the tlb when a pte is > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index c5e2363..353d752 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -1099,17 +1099,28 @@ struct rmap_iterator { > */ > static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter) > { > + u64 *sptep; > + > if (!rmap) > return NULL; > > if (!(rmap & 1)) { > iter->desc = NULL; > - return (u64 *)rmap; > + sptep = (u64 *)rmap; > + goto out; > } > > iter->desc = (struct pte_list_desc *)(rmap & ~1ul); > iter->pos = 0; > - return iter->desc->sptes[iter->pos]; > + sptep = iter->desc->sptes[iter->pos]; > +out: > + /* > + * Parent sptes found in sp->parent_ptes lists are also checked here > + * since kvm_mmu_unlink_parents() uses this function. If the condition > + * needs to be changed for them, make another wrapper function. > + */ > + WARN_ON(!is_shadow_present_pte(*sptep)); > + return sptep; > } > > /* > @@ -1119,14 +1130,14 @@ static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter) > */ > static u64 *rmap_get_next(struct rmap_iterator *iter) > { > + u64 *sptep; > + > if (iter->desc) { > if (iter->pos < PTE_LIST_EXT - 1) { > - u64 *sptep; > - > ++iter->pos; > sptep = iter->desc->sptes[iter->pos]; > if (sptep) > - return sptep; > + goto out; > } > > iter->desc = iter->desc->more; > @@ -1134,17 +1145,20 @@ static u64 *rmap_get_next(struct rmap_iterator *iter) > if (iter->desc) { > iter->pos = 0; > /* desc->sptes[0] cannot be NULL */ > - return iter->desc->sptes[iter->pos]; > + sptep = iter->desc->sptes[iter->pos]; > + goto out; > } > } > > return NULL; > +out: > + WARN_ON(!is_shadow_present_pte(*sptep)); > + return sptep; > } > > #define for_each_rmap_spte(_rmap_, _iter_, _spte_) \ > for (_spte_ = rmap_get_first(*_rmap_, _iter_); \ > - _spte_ && ({BUG_ON(!is_shadow_present_pte(*_spte_)); 1;}); \ > - _spte_ = rmap_get_next(_iter_)) > + _spte_; _spte_ = rmap_get_next(_iter_)) > > static void drop_spte(struct kvm *kvm, u64 *sptep) > { > @@ -1358,7 +1372,6 @@ static bool kvm_zap_rmapp(struct kvm *kvm, unsigned long *rmapp) > bool flush = false; > > while ((sptep = rmap_get_first(*rmapp, &iter))) { > - BUG_ON(!(*sptep & PT_PRESENT_MASK)); > rmap_printk("%s: spte %p %llx.\n", __func__, sptep, *sptep); > > drop_spte(kvm, sptep); > -- 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/09 19:14, Paolo Bonzini wrote: > Can you also change kvm_mmu_mark_parents_unsync to use > for_each_rmap_spte instead of pte_list_walk? It is the last use of > pte_list_walk, and it's nice if we have two uses of for_each_rmap_spte > with parent_ptes as the argument. No problem, I will do. Since parent_ptes is also explained as the "reverse mapping" list of parent sptes (in mmu.txt and kvm_host.h), using rmap helpers will not look so strange. > BTW, on my todo list is to change the rmap items to a struct (with a > single u64 inside) for type safety. Since you are touching this code, > perhaps you can give it a shot? Yes, almost done here (assuming that you mean 'unsigned long'). But I have some candidates for its name in mind: 1. struct kvm_rmap { unsigned long val; }; 2. struct kvm_rmap_head { unsigned long val; }; 3. struct kvm_rmap_list_head { unsigned long val; }; 4. struct kvm_spte_list_head { unsigned long val; }; Since this is the head of the reverse mapping list of sptes, I thought name 3 might be the best and first made a patch with it, but it was a bit longer than I had hoped it to be. I have changed it to name 2, and it looks a bit nicer now, but even shorter, e.g. name 1, may be good as well. Do you have any preference? 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 10/11/2015 10:05, Takuya Yoshikawa wrote: > > >> BTW, on my todo list is to change the rmap items to a struct (with a >> single u64 inside) for type safety. Since you are touching this code, >> perhaps you can give it a shot? > > Yes, almost done here (assuming that you mean 'unsigned long'). Yes, thanks! > But I have some candidates for its name in mind: > > 1. struct kvm_rmap { unsigned long val; }; > 2. struct kvm_rmap_head { unsigned long val; }; > 3. struct kvm_rmap_list_head { unsigned long val; }; > 4. struct kvm_spte_list_head { unsigned long val; }; > > Since this is the head of the reverse mapping list of sptes, I thought > name 3 might be the best and first made a patch with it, but it was > a bit longer than I had hoped it to be. > > I have changed it to name 2, and it looks a bit nicer now, but even > shorter, e.g. name 1, may be good as well. > > Do you have any preference? I like kvm_rmap_head. Paolo -- 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/Documentation/virtual/kvm/mmu.txt b/Documentation/virtual/kvm/mmu.txt index 3a4d681..daf9c0f 100644 --- a/Documentation/virtual/kvm/mmu.txt +++ b/Documentation/virtual/kvm/mmu.txt @@ -203,10 +203,10 @@ Shadow pages contain the following information: page cannot be destroyed. See role.invalid. parent_ptes: The reverse mapping for the pte/ptes pointing at this page's spt. If - parent_ptes bit 0 is zero, only one spte points at this pages and + parent_ptes bit 0 is zero, only one spte points at this page and parent_ptes points at this single spte, otherwise, there exists multiple sptes pointing at this page and (parent_ptes & ~0x1) points at a data - structure with a list of parent_ptes. + structure with a list of parent sptes. unsync: If true, then the translations in this page may not match the guest's translation. This is equivalent to the state of the tlb when a pte is diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index c5e2363..353d752 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -1099,17 +1099,28 @@ struct rmap_iterator { */ static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter) { + u64 *sptep; + if (!rmap) return NULL; if (!(rmap & 1)) { iter->desc = NULL; - return (u64 *)rmap; + sptep = (u64 *)rmap; + goto out; } iter->desc = (struct pte_list_desc *)(rmap & ~1ul); iter->pos = 0; - return iter->desc->sptes[iter->pos]; + sptep = iter->desc->sptes[iter->pos]; +out: + /* + * Parent sptes found in sp->parent_ptes lists are also checked here + * since kvm_mmu_unlink_parents() uses this function. If the condition + * needs to be changed for them, make another wrapper function. + */ + WARN_ON(!is_shadow_present_pte(*sptep)); + return sptep; } /* @@ -1119,14 +1130,14 @@ static u64 *rmap_get_first(unsigned long rmap, struct rmap_iterator *iter) */ static u64 *rmap_get_next(struct rmap_iterator *iter) { + u64 *sptep; + if (iter->desc) { if (iter->pos < PTE_LIST_EXT - 1) { - u64 *sptep; - ++iter->pos; sptep = iter->desc->sptes[iter->pos]; if (sptep) - return sptep; + goto out; } iter->desc = iter->desc->more; @@ -1134,17 +1145,20 @@ static u64 *rmap_get_next(struct rmap_iterator *iter) if (iter->desc) { iter->pos = 0; /* desc->sptes[0] cannot be NULL */ - return iter->desc->sptes[iter->pos]; + sptep = iter->desc->sptes[iter->pos]; + goto out; } } return NULL; +out: + WARN_ON(!is_shadow_present_pte(*sptep)); + return sptep; } #define for_each_rmap_spte(_rmap_, _iter_, _spte_) \ for (_spte_ = rmap_get_first(*_rmap_, _iter_); \ - _spte_ && ({BUG_ON(!is_shadow_present_pte(*_spte_)); 1;}); \ - _spte_ = rmap_get_next(_iter_)) + _spte_; _spte_ = rmap_get_next(_iter_)) static void drop_spte(struct kvm *kvm, u64 *sptep) { @@ -1358,7 +1372,6 @@ static bool kvm_zap_rmapp(struct kvm *kvm, unsigned long *rmapp) bool flush = false; while ((sptep = rmap_get_first(*rmapp, &iter))) { - BUG_ON(!(*sptep & PT_PRESENT_MASK)); rmap_printk("%s: spte %p %llx.\n", __func__, sptep, *sptep); drop_spte(kvm, sptep);
At some call sites of rmap_get_first() and rmap_get_next(), BUG_ON is placed right after the call to detect unrelated sptes which should not be found in the reverse-mapping list. Move this check in rmap_get_first/next() so that all call sites, not just the users of the for_each_rmap_spte() macro, will be checked the same way. In addition, change the BUG_ON to WARN_ON since killing the whole host is the last thing that KVM should try. One thing to keep in mind is that kvm_mmu_unlink_parents() also uses rmap_get_first() to handle parent sptes. The change will not break it because parent sptes are present, at least until drop_parent_pte() actually unlinks them, and not mmio-sptes. Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp> --- Documentation/virtual/kvm/mmu.txt | 4 ++-- arch/x86/kvm/mmu.c | 31 ++++++++++++++++++++++--------- 2 files changed, 24 insertions(+), 11 deletions(-)