From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] KVM: MMU: Replace role.glevels with role.cr4_pae Date: Thu, 15 Apr 2010 12:02:32 +0300 Message-ID: <4BC6D628.9090306@redhat.com> References: <1271262003-29973-1-git-send-email-avi@redhat.com> <4BC5EE0C.60705@redhat.com> <20100414182946.GA8353@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42210 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752500Ab0DOJCi (ORCPT ); Thu, 15 Apr 2010 05:02:38 -0400 Received: from int-mx05.intmail.prod.int.phx2.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.18]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o3F92bn8022451 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 15 Apr 2010 05:02:37 -0400 In-Reply-To: <20100414182946.GA8353@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 04/14/2010 09:29 PM, Marcelo Tosatti wrote: > On Wed, Apr 14, 2010 at 07:32:12PM +0300, Avi Kivity wrote: > >> On 04/14/2010 07:20 PM, Avi Kivity wrote: >> >>> There is no real distinction between glevels=3 and glevels=4; both have >>> exactly the same format and the code is treated exactly the same way. Drop >>> role.glevels and replace is with role.cr4_pae (which is meaningful). This >>> simplifies the code a bit. >>> >>> As a side effect, it allows sharing shadow page tables between pae and >>> longmode guest page tables at the same guest page. >>> >> >>> static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp) >>> { >>> - if (sp->role.glevels != vcpu->arch.mmu.root_level) { >>> + if (sp->role.cr4_pae != !!is_pae(vcpu)) { >>> kvm_mmu_zap_page(vcpu->kvm, sp); >>> return 1; >>> } >>> >> This bit confuses me a little. Why is it needed? It will never hit >> from mmu_sync_children(), and as for kvm_mmu_get_page(), it will >> simply zap unrelated pages? >> > kvm_mmu_get_page is write protecting a gfn. Took me a while to figure out why. > If there's shadow for a > differ ent role, and its unsync, it needs to be synchronized. > > We could leave it unsync and write protected, though that destroys an invariant (sync==protected, unsync==unprotected), and all the calls to rmap_write_protect() become confused. > Perhaps it could call the appropriate _sync_page version instead > of zapping, similar to mmu_pte_write_new_pte. > Probably better for nonpae. >> Is it related to the restriction that we can only unsync if we have >> just one shadow page for a gfn? That's somewhat artificial (and >> hurts nonpae guests, and guests with linear page tables). >> > If gfn is shadowed at PMD or higher level, you can't unsync the PTE > shadow. > Yes. Even if we could, invlpg is defined to drop all PDE caches (except large page PDEs), so we would have to resync all those pages on invlpg. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.