Re: [PATCH 2/5] KVM: MMU: simplify mmu_set_spte

[Marcelo Tosatti <mtosatti@redhat.com>]

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.