On 11/21/2012 07:51 AM, Marcelo Tosatti wrote: > 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. Okay, i will not change the position of page_header_update_slot in the next version. Thank you, Marcelo! -- 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
