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: Mon, 22 Sep 2008 23:41:14 +0300
Message-ID: <48D802EA.9070807@redhat.com>
References: <20080918212749.800177179@localhost.localdomain> <20080918213337.148804603@localhost.localdomain> <48D4506C.5070804@redhat.com> <20080921004515.GC10120@dmt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii; format=flowed
Content-Transfer-Encoding: 7bit
Cc: 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]:34672 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751353AbYIVUlk (ORCPT <rfc822;kvm@vger.kernel.org>);
	Mon, 22 Sep 2008 16:41:40 -0400
In-Reply-To: <20080921004515.GC10120@dmt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Marcelo Tosatti wrote:
>>> +	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);
>>>       
>> What does this do?
>>     
>
> Walks all children of given page with no efficiency. Its replaced later
> by the bitmap version.
>
>   

I don't understand how the variables sp, child, and parent interact. You 
either need recursion or an explicit stack?

>>> +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?
>>     
>
> The shadow page will go unsync on pte touch and resynced as soon as its
> visible (after return to paging).
>
> Or, while still in real mode, it might be zapped by
> kvm_mmu_get_page->kvm_sync_page.
>
> Am I missing something?
>
>   

I guess I was. Ok.

>
>>>  static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
>>> -	return 0;
>>> +	return ret;
>>>  }
>>>   
>>>       
>> Why does the caller care if zap also zapped some other random pages?  To  
>> restart walking the list?
>>     
>
> Yes. The next element for_each_entry_safe saved could have been zapped.
>
>   

Ouch. Ouch.

I hate doing this. Can see no alternative though.

>>> +	/* 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.  
>>     
>
> Yes. 
>
>   
>> So to allow nonpae oos, check the level instead.
>>     
>
> Windows 2008 64-bit has all sorts of sharing a pagetable at multiple
> levels too.
>
>   

We still want to allow oos for the two quadrants of a nonpae shadow page.

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