From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753416Ab2KTXXe (ORCPT ); Tue, 20 Nov 2012 18:23:34 -0500 Received: from e28smtp06.in.ibm.com ([122.248.162.6]:47718 "EHLO e28smtp06.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751754Ab2KTXXc (ORCPT ); Tue, 20 Nov 2012 18:23:32 -0500 Message-ID: <50AC10EE.8000008@linux.vnet.ibm.com> Date: Wed, 21 Nov 2012 07:23:26 +0800 From: Xiao Guangrong User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120911 Thunderbird/15.0.1 MIME-Version: 1.0 To: Marcelo Tosatti CC: Avi Kivity , LKML , KVM Subject: Re: [PATCH 2/5] KVM: MMU: simplify mmu_set_spte References: <5097AC70.1080904@linux.vnet.ibm.com> <5097ACA0.7080408@linux.vnet.ibm.com> <20121112231223.GC5798@amt.cnet> <50A20750.8050808@linux.vnet.ibm.com> <20121120221853.GA31427@amt.cnet> In-Reply-To: <20121120221853.GA31427@amt.cnet> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit x-cbid: 12112023-9574-0000-0000-0000056B23B6 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/21/2012 06:18 AM, Marcelo Tosatti wrote: >>>> - child = page_header(pte & PT64_BASE_ADDR_MASK); >>>> - drop_parent_pte(child, sptep); >>>> - kvm_flush_remote_tlbs(vcpu->kvm); >>> >>> How come its safe to drop this case? >> >> We use "if (pfn != spte_to_pfn(*sptep))" to simplify the thing. >> There are two cases: >> 1) the sptep is not the last mapping. >> under this case, sptep must point to a shadow page table, that means >> spte_to_pfn(*sptep)) is used by KVM module, and 'pfn' is used by userspace. >> so, 'if' condition must be satisfied, the sptep will be dropped. >> >> Actually, This is the origin case: >> | if (level > PT_PAGE_TABLE_LEVEL && >> | !is_large_pte(*sptep))" >> >> 2) the sptep is the last mapping. >> under this case, the level of spte (sp.level) must equal the 'level' which >> we pass to mmu_set_spte. If they point to the same pfn, it is 'remap', otherwise >> we drop it. >> >> I think this is safe. :) > > mmu_page_zap_pte takes care of it, OK. > > What if was_rmapped=true but gfn is different? Say if the spte comes > from an unsync shadow page, the guest modifies that shadow page (but > does not invalidate it with invlpg), then faults. gfn can still point > to the same gfn (but in that case, with your patch, > page_header_update_slot is not called. Marcelo, Page fault path and other sync/prefetch paths will reread guest page table, then it get a different target pfn. The scenario is like this: gfn1 = pfn1, gfn2 = pfn2 gpte = pfn1, spte is shadowed by gpte and it is a unsync spte Guest Host spte = (gfn1, pfn1) modify gpte to let it point to gfn2 spte = (gfn1, pfn1) page-fault on gpte intercept the page-fault, then want to update spte to (gfn2, pfn2) in mmu_set_spte, we can detect pfn2 != pfn1, then drop it. Hmm, the interesting thing is what if different gfns map to the same pfn. For example, spte1 is shadowed by gfn1 and spte2 is shadowed by pfn2, both gfn1 and gfn2 map to pfn, the code (including the current code) will set spte1 to the gfn2's rmap and spte2 to the gfn1's rmap. But i think it is ok.