From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: KVM: MMU: always invalidate and flush on spte page size change Date: Mon, 31 May 2010 14:49:13 +0300 Message-ID: <4C03A239.9090306@redhat.com> References: <20100528124459.GA3734@amt.cnet> <4C023DC3.1090406@redhat.com> <20100530151917.GA3495@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm , Andrea Arcangeli To: Marcelo Tosatti Return-path: Received: from mx1.redhat.com ([209.132.183.28]:44554 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751691Ab0EaLtR (ORCPT ); Mon, 31 May 2010 07:49:17 -0400 Received: from int-mx08.intmail.prod.int.phx2.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.21]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o4VBnHNN017070 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 31 May 2010 07:49:17 -0400 In-Reply-To: <20100530151917.GA3495@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 05/30/2010 06:19 PM, Marcelo Tosatti wrote: > On Sun, May 30, 2010 at 01:28:19PM +0300, Avi Kivity wrote: > >> On 05/28/2010 03:44 PM, Marcelo Tosatti wrote: >> >>> Always invalidate spte and flush TLBs when changing page size, to make >>> sure different sized translations for the same address are never cached >>> in a CPU's TLB. >>> >>> The first case where this occurs is when a non-leaf spte pointer is >>> overwritten by a leaf, large spte entry. This can happen after dirty >>> logging is disabled on a memslot, for example. >>> >>> The second case is a leaf, large spte entry is overwritten with a >>> non-leaf spte pointer, in __direct_map. Note this cannot happen now >>> because the only potential source of such overwrite is dirty logging >>> being enabled, which zaps all MMU pages. But this might change >>> in the future, so better be robust against it. >>> >>> Noticed by Andrea. >>> >>> KVM-Stable-Tag >>> Signed-off-by: Marcelo Tosatti >>> >>> Index: kvm/arch/x86/kvm/mmu.c >>> =================================================================== >>> --- kvm.orig/arch/x86/kvm/mmu.c >>> +++ kvm/arch/x86/kvm/mmu.c >>> @@ -1952,6 +1952,8 @@ static void mmu_set_spte(struct kvm_vcpu >>> >>> child = page_header(pte& PT64_BASE_ADDR_MASK); >>> mmu_page_remove_parent_pte(child, sptep); >>> + __set_spte(sptep, shadow_trap_nonpresent_pte); >>> + kvm_flush_remote_tlbs(vcpu->kvm); >>> } else if (pfn != spte_to_pfn(*sptep)) { >>> pgprintk("hfn old %lx new %lx\n", >>> spte_to_pfn(*sptep), pfn); >>> >> Applied this bit. >> >> >>> @@ -2015,6 +2017,16 @@ static int __direct_map(struct kvm_vcpu >>> break; >>> } >>> >>> + if (is_shadow_present_pte(*iterator.sptep)&& >>> + !is_large_pte(*iterator.sptep)) >>> + continue; >>> + >>> + if (is_large_pte(*iterator.sptep)) { >>> + rmap_remove(vcpu->kvm, iterator.sptep); >>> + __set_spte(iterator.sptep, shadow_trap_nonpresent_pte); >>> + kvm_flush_remote_tlbs(vcpu->kvm); >>> + } >>> + >>> >> Don't we have exactly the same issue in FNAME(fetch)()? >> > Yes and its already handled there: > > if (is_shadow_present_pte(*sptep)&& !is_large_pte(*sptep)) > continue; > > if (is_large_pte(*sptep)) { > rmap_remove(vcpu->kvm, sptep); > __set_spte(sptep, shadow_trap_nonpresent_pte); > kvm_flush_remote_tlbs(vcpu->kvm); > } > > Well that bit of code wants deduplication under a good name. -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain.