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

[Avi Kivity <avi@redhat.com>]

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