Avi Kivity wrote:
> On 06/29/2010 10:35 AM, Xiao Guangrong wrote:
>>
>>> We have now
>>>
>>> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
>>> continue;
>>>
>>> So we need to add a check, if sp->role.access doesn't match pt_access&
>>> pte_access, we need to get a new sp with the correct access (can only
>>> change read->write).
>>>
>>>
>> Umm, we should update the spte at the gw->level, so we need get the child
>> sp, and compare its access at this point, just like this:
>>
>> if (level == gw->level&& is_shadow_present_pte(*sptep)) {
>> child_sp = page_header(__pa(*sptep& PT64_BASE_ADDR_MASK));
>>
>> if (child_sp->access != pt_access& pte_access& (diry ? 1 :
>> ~ACC_WRITE_MASK )) {
>> /* Zap sptep */
>> ......
>> }
>>
>> }
>>
>> So, why not use the new spte flag (SPTE_NO_DIRTY in my patch) to mark
>> this spte then we can see
>> this spte whether need updated directly? i think it more simpler ;-)
>>
>
> It's new state, and new state means more maintenance of that state and
> the need to consider the state in all relevant code paths.
>
> In terms of maintainability, changing walk_addr() is best, since it
> maintains the tight invariant that PT_PAGE_DIRECTORY_LEVEL sptes are
> always consistent with their sptes. Updating fetch() to allow for a
> relaxed invariant (spte may be read-only while gpte is write-dirty) is
> more complicated, but performs better. This is also consistent with
> what we do with PT_PAGE_TABLE_LEVEL gptes/sptes and with unsync pages.
>
Maybe you are right, i just think is more quickly by using SPTE_NO_DIRTY flag
to judge whether need updated. I'll modify this patch as your suggestion.
> btw, how can the patch work?
>
>>
>> + if (level == gw->level&& !dirty&&
>> + access& gw->pte_access& ACC_WRITE_MASK)
>> + spte |= SPTE_NO_DIRTY;
>> +
>> spte = __pa(sp->spt)
>> | PT_PRESENT_MASK | PT_ACCESSED_MASK
>> | PT_WRITABLE_MASK | PT_USER_MASK;
>>
>
> spte is immediately overwritten by the following assignment.
>
Ah, sorry, i miss it, spte |= SPTE_NO_DIRTY should behind of following assignment.
> However, the other half of the patch can be adapted:
>
>>
>> + if (*sptep& SPTE_NO_DIRTY) {
>> + struct kvm_mmu_page *child;
>> +
>> + WARN_ON(level != gw->level);
>> + WARN_ON(!is_shadow_present_pte(*sptep));
>> + if (dirty) {
>> + child = page_header(*sptep&
>> + PT64_BASE_ADDR_MASK);
>> + mmu_page_remove_parent_pte(child, sptep);
>> + __set_spte(sptep, shadow_trap_nonpresent_pte);
>> + kvm_flush_remote_tlbs(vcpu->kvm);
>> + }
>> + }
>> +
>> if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))
>> continue;
>>
>
> Simply replace (*spte & SPTE_NO_DIRTY) with a condition that checks
> whether sp->access is consistent with gw->pt(e)_access.
>
If the guest mapping is writable and it !dirty, we mark SPTE_NO_DIRTY flag in
the spte, when the next #PF occurs, we just need check this flag and see whether
gpte's D bit is set, if it's true, we zap this spte and map to the correct sp.
> Can you write a test case for qemu-kvm.git/kvm/test that demonstrates
> the problem and the fix? It will help ensure we don't regress in this
> area.
>
OK, but allow me do it later :-)
--
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
