From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: [patch 09/10] KVM: MMU: out of sync shadow core v2
Date: Fri, 19 Sep 2008 18:22:52 -0700
Message-ID: <48D4506C.5070804@redhat.com>
References: <20080918212749.800177179@localhost.localdomain> <20080918213337.148804603@localhost.localdomain>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Avi Kivity <avi@qumranet.com>, kvm@vger.kernel.org,
	"David S. Ahern" <daahern@cisco.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:55337 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1752138AbYITBWw (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 19 Sep 2008 21:22:52 -0400
In-Reply-To: <20080918213337.148804603@localhost.localdomain>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Marcelo Tosatti wrote:
>  static struct kmem_cache *rmap_desc_cache;
> @@ -942,6 +943,39 @@ static void nonpaging_invlpg(struct kvm_
>  {
>  }
>  
> +static int mmu_unsync_walk(struct kvm_mmu_page *parent, mmu_unsync_fn fn,
> +			   void *priv)
>   

Instead of private, have an object contain both callback and private 
data, and use container_of().  Reduces the chance of type errors.

> +{
> +	int i, ret;
> +	struct kvm_mmu_page *sp = parent;
> +
> +	while (parent->unsync_children) {
> +		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
> +			u64 ent = sp->spt[i];
> +
> +			if (is_shadow_present_pte(ent)) {
> +				struct kvm_mmu_page *child;
> +				child = page_header(ent & PT64_BASE_ADDR_MASK);
> +
> +				if (child->unsync_children) {
> +					sp = child;
> +					break;
> +				}
> +				if (child->unsync) {
> +					ret = fn(child, priv);
> +					if (ret)
> +						return ret;
> +				}
> +			}
> +		}
> +		if (i == PT64_ENT_PER_PAGE) {
> +			sp->unsync_children = 0;
> +			sp = parent;
> +		}
> +	}
> +	return 0;
> +}
>   

What does this do?

> +static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
> +{
> +	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
> +		kvm_mmu_zap_page(vcpu->kvm, sp);
> +		return 1;
> +	}
>   

Suppose we switch to real mode, touch a pte, switch back.  Is this handled?

> @@ -991,8 +1066,18 @@ static struct kvm_mmu_page *kvm_mmu_get_
>  		 gfn, role.word);
>  	index = kvm_page_table_hashfn(gfn);
>  	bucket = &vcpu->kvm->arch.mmu_page_hash[index];
> -	hlist_for_each_entry(sp, node, bucket, hash_link)
> -		if (sp->gfn == gfn && sp->role.word == role.word) {
> +	hlist_for_each_entry_safe(sp, node, tmp, bucket, hash_link)
> +		if (sp->gfn == gfn) {
> +			if (sp->unsync)
> +				if (kvm_sync_page(vcpu, sp))
> +					continue;
> +
> +			if (sp->role.word != role.word)
> +				continue;
> +
> +			if (sp->unsync_children)
> +				vcpu->arch.mmu.need_root_sync = 1;
>   

mmu_reload() maybe?

>  static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
>  {
> +	int ret;
>  	++kvm->stat.mmu_shadow_zapped;
> +	ret = mmu_zap_unsync_children(kvm, sp);
>  	kvm_mmu_page_unlink_children(kvm, sp);
>  	kvm_mmu_unlink_parents(kvm, sp);
>  	kvm_flush_remote_tlbs(kvm);
>  	if (!sp->role.invalid && !sp->role.metaphysical)
>  		unaccount_shadowed(kvm, sp->gfn);
> +	if (sp->unsync)
> +		kvm_unlink_unsync_page(kvm, sp);
>  	if (!sp->root_count) {
>  		hlist_del(&sp->hash_link);
>  		kvm_mmu_free_page(kvm, sp);
> @@ -1129,7 +1245,7 @@ static int kvm_mmu_zap_page(struct kvm *
>  		kvm_reload_remote_mmus(kvm);
>  	}
>  	kvm_mmu_reset_last_pte_updated(kvm);
> -	return 0;
> +	return ret;
>  }
>   

Why does the caller care if zap also zapped some other random pages?  To 
restart walking the list?

>  
> +
> +static int kvm_unsync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
> +{
> +	unsigned index;
> +	struct hlist_head *bucket;
> +	struct kvm_mmu_page *s;
> +	struct hlist_node *node, *n;
> +
> +	index = kvm_page_table_hashfn(sp->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.metaphysical)
> +			continue;
> +		if (s->role.word != sp->role.word)
> +			return 1;
> +	}
>   

This will happen for nonpae paging.  But why not allow it?  Zap all 
unsynced pages on mode switch.

Oh, if a page is both a page directory and page table, yes.  So to allow 
nonpae oos, check the level instead.


-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.