Re: [PATCH v2 3/10] KVM: MMU: fix direct sp's access corruptted

From: Avi Kivity
Date: Tue Jun 29 2010 - 04:49:30 EST

On 06/29/2010 10:35 AM, Xiao Guangrong wrote:

We have now

if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep))

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)

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&
+ 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))

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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at