Re: [PATCH 1/2] KVM: MMU: allow more page become unsync at gfn mapping time

[Avi Kivity <avi@redhat.com>]

On 05/23/2010 03:14 PM, Xiao Guangrong wrote:
> In current code, shadow page can become asynchronous only if one
> shadow page for a gfn, this rule is too strict, in fact, we can
> let all last mapping page(i.e, it's the pte page) become unsync,
> and sync them at invlpg or flush tlb time.
>
> This patch allow more page become asynchronous at gfn mapping time
>
>
> +
> +static void kvm_unsync_pages(struct kvm_vcpu *vcpu,  gfn_t gfn)
>   {
> -	unsigned index;
>   	struct hlist_head *bucket;
>   	struct kvm_mmu_page *s;
>   	struct hlist_node *node, *n;
> +	unsigned index;
>
> -	index = kvm_page_table_hashfn(sp->gfn);
> +	index = kvm_page_table_hashfn(gfn);
>   	bucket =&vcpu->kvm->arch.mmu_page_hash[index];
> -	/* don't unsync if pagetable is shadowed with multiple roles */
> +
>   	hlist_for_each_entry_safe(s, node, n, bucket, hash_link) {
> -		if (s->gfn != sp->gfn || s->role.direct)
> +		if (s->gfn != gfn || s->role.direct || s->unsync)
>   			continue;
>    

role.invalid?

> -		if (s->role.word != sp->role.word)
> -			return 1;
> +		WARN_ON(s->role.level != PT_PAGE_TABLE_LEVEL);
> +		__kvm_unsync_page(vcpu, s);
>   	}
> -	trace_kvm_mmu_unsync_page(sp);
> -	++vcpu->kvm->stat.mmu_unsync;
> -	sp->unsync = 1;
> -
> -	kvm_mmu_mark_parents_unsync(sp);
> -
> -	mmu_convert_notrap(sp);
> -	return 0;
>   }
>
>   static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
>   				  bool can_unsync)
>   {
> -	struct kvm_mmu_page *shadow;
> +	unsigned index;
> +	struct hlist_head *bucket;
> +	struct kvm_mmu_page *s;
> +	struct hlist_node *node, *n;
> +	bool need_unsync = false;
> +
> +	index = kvm_page_table_hashfn(gfn);
> +	bucket =&vcpu->kvm->arch.mmu_page_hash[index];
> +	hlist_for_each_entry_safe(s, node, n, bucket, hash_link) {
> +		if (s->gfn != gfn || s->role.direct)
> +			continue;
>
> -	shadow = kvm_mmu_lookup_page(vcpu->kvm, gfn);
> -	if (shadow) {
> -		if (shadow->role.level != PT_PAGE_TABLE_LEVEL)
> +		if (s->role.level != PT_PAGE_TABLE_LEVEL)
>   			return 1;
>    

role.invalid?

> -		if (shadow->unsync)
> -			return 0;
> -		if (can_unsync&&  oos_shadow)
> -			return kvm_unsync_page(vcpu, shadow);
> -		return 1;
> +
> +		if (!need_unsync&&  !s->unsync) {
> +			if (!can_unsync || !oos_shadow)
> +				return 1;
> +			need_unsync = true;
> +		}
>   	}
> +	if (need_unsync)
> +		kvm_unsync_pages(vcpu, gfn);
>   	return 0;
>   }
>
>    

Looks good, I'm just uncertain about role.invalid handling.  What's the 
reasoning here?

-- 
error compiling committee.c: too many arguments to function