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

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

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!