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

Avi Kivity
2010-06-29T08:49:25Z

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