From mboxrd@z Thu Jan  1 00:00:00 1970
From: Avi Kivity <avi@redhat.com>
Subject: Re: cr3 OOS optimisation breaks 32-bit GNU/kFreeBSD guest
Date: Sat, 04 Apr 2009 13:37:39 +0300
Message-ID: <49D73873.2090302@redhat.com>
References: <20090223003305.GW12976@hall.aurel32.net> <20090320231405.GA26415@amt.cnet> <49C60644.2090904@redhat.com> <20090323172725.GA28775@amt.cnet> <49C8AC35.3030803@redhat.com> <20090403214548.GA5394@amt.cnet>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Aurelien Jarno <aurelien@aurel32.net>, kvm@vger.kernel.org
To: Marcelo Tosatti <mtosatti@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mx2.redhat.com ([66.187.237.31]:38528 "EHLO mx2.redhat.com"
	rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
	id S1751042AbZDDKhP (ORCPT <rfc822;kvm@vger.kernel.org>);
	Sat, 4 Apr 2009 06:37:15 -0400
In-Reply-To: <20090403214548.GA5394@amt.cnet>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Marcelo Tosatti wrote:
> On Tue, Mar 24, 2009 at 11:47:33AM +0200, Avi Kivity wrote:
>   
>>> index 2ea8262..48169d7 100644
>>> --- a/arch/x86/kvm/x86.c
>>> +++ b/arch/x86/kvm/x86.c
>>> @@ -3109,6 +3109,8 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run)
>>>  			kvm_write_guest_time(vcpu);
>>>  		if (test_and_clear_bit(KVM_REQ_MMU_SYNC, &vcpu->requests))
>>>  			kvm_mmu_sync_roots(vcpu);
>>> +		if (test_and_clear_bit(KVM_REQ_MMU_GLOBAL_SYNC, &vcpu->requests))
>>> +			kvm_mmu_sync_global(vcpu);
>>>  		if (test_and_clear_bit(KVM_REQ_TLB_FLUSH, &vcpu->requests))
>>>  			kvm_x86_ops->tlb_flush(vcpu);
>>>  		if (test_and_clear_bit(KVM_REQ_REPORT_TPR_ACCESS
>>>       
>> Windows will (I think) write a PDE on every context switch, so this  
>> effectively disables global unsync for that guest.
>>
>> What about recursively syncing the newly linked page in FNAME(fetch)()?  
>> If the page isn't global, this becomes a no-op, so no new overhead.  The  
>> only question is the expense when linking a populated top-level page,  
>> especially in long mode.
>>     
>
> How about this?
>
> KVM: MMU: sync global pages on fetch()
>
> If an unsync global page becomes unreachable via the shadow tree, which
> can happen if one its parent pages is zapped, invlpg will fail to
> invalidate translations for gvas contained in such unreachable pages.
>
> So sync global pages in fetch().
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 09782a9..728be72 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -308,8 +308,14 @@ static u64 *FNAME(fetch)(struct kvm_vcpu *vcpu, gva_t addr,
>  			break;
>  		}
>  
> -		if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep))
> +		if (is_shadow_present_pte(*sptep) && !is_large_pte(*sptep)) {
> +			if (level-1 == PT_PAGE_TABLE_LEVEL) {
> +				shadow_page = page_header(__pa(sptep));
> +				if (shadow_page->unsync && shadow_page->global)
> +					kvm_sync_page(vcpu, shadow_page);
> +			}
>  			continue;
> +		}
>  
>  		if (is_large_pte(*sptep)) {
>  			rmap_remove(vcpu->kvm, sptep);
>   

But here the shadow page is already linked?  Isn't the root cause that 
an invlpg was called when the page wasn't linked, so it wasn't seen by 
invlpg?

So I thought the best place would be in fetch(), after 
kvm_mmu_get_page().  If we're linking a page which contains global ptes, 
they might be unsynced due to invlpgs that we've missed.

Or am I missing something about the root cause?

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