On Wed, Nov 21, 2012 at 07:23:26AM +0800, Xiao Guangrong wrote: > On 11/21/2012 06:18 AM, Marcelo Tosatti wrote: > > >>>> - child = page_header(pte & PT64_BASE_ADDR_MASK); > >>>> - drop_parent_pte(child, sptep); > >>>> - kvm_flush_remote_tlbs(vcpu->kvm); > >>> > >>> How come its safe to drop this case? > >> > >> We use "if (pfn != spte_to_pfn(*sptep))" to simplify the thing. > >> There are two cases: > >> 1) the sptep is not the last mapping. > >> under this case, sptep must point to a shadow page table, that means > >> spte_to_pfn(*sptep)) is used by KVM module, and 'pfn' is used by userspace. > >> so, 'if' condition must be satisfied, the sptep will be dropped. > >> > >> Actually, This is the origin case: > >> | if (level > PT_PAGE_TABLE_LEVEL && > >> | !is_large_pte(*sptep))" > >> > >> 2) the sptep is the last mapping. > >> under this case, the level of spte (sp.level) must equal the 'level' which > >> we pass to mmu_set_spte. If they point to the same pfn, it is 'remap', otherwise > >> we drop it. > >> > >> I think this is safe. :) > > > > mmu_page_zap_pte takes care of it, OK. > > > > What if was_rmapped=true but gfn is different? Say if the spte comes > > from an unsync shadow page, the guest modifies that shadow page (but > > does not invalidate it with invlpg), then faults. gfn can still point > > to the same gfn (but in that case, with your patch, > > page_header_update_slot is not called. > > Marcelo, > > Page fault path and other sync/prefetch paths will reread guest page table, > then it get a different target pfn. > > The scenario is like this: > > gfn1 = pfn1, gfn2 = pfn2 > gpte = pfn1, spte is shadowed by gpte and it is a unsync spte > > Guest Host > spte = (gfn1, pfn1) > > modify gpte to let it point to gfn2 > spte = (gfn1, pfn1) > page-fault on gpte > intercept the page-fault, then > want to update spte to (gfn2, pfn2) > > in mmu_set_spte, we can detect > pfn2 != pfn1, then drop it. > > Hmm, the interesting thing is what if different gfns map to the same pfn. > For example, spte1 is shadowed by gfn1 and spte2 is shadowed by pfn2, both > gfn1 and gfn2 map to pfn, the code (including the current code) will set > spte1 to the gfn2's rmap and spte2 to the gfn1's rmap. But i think it is ok. Current code updates gfn properly in set_spte by page_header_update_slot. Better keep state properly. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
