Messages in this thread | | | Date | Tue, 29 Jun 2010 11:49:20 +0300 | From | Avi Kivity <> | Subject | Re: [PATCH v2 3/10] KVM: MMU: fix direct sp's access corruptted |
| |
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
| |