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.
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.
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.
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.
--
error compiling committee.c: too many arguments to function
--
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
