From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 06/11] KVM: introduce kvm->srcu and convert kvm_set_memory_region to SRCU update Date: Wed, 23 Dec 2009 14:32:55 +0200 Message-ID: <4B320DF7.3070005@redhat.com> References: <20091223113833.742662117@redhat.com> <20091223114438.751996250@redhat.com> 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]:26570 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751197AbZLWMc5 (ORCPT ); Wed, 23 Dec 2009 07:32:57 -0500 Received: from int-mx04.intmail.prod.int.phx2.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id nBNCWvOM031721 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 23 Dec 2009 07:32:57 -0500 In-Reply-To: <20091223114438.751996250@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On 12/23/2009 01:38 PM, Marcelo Tosatti wrote: > Use two steps for memslot deletion: mark the slot invalid (which stops > instantiation of new shadow pages for that slot, but allows destruction), > then instantiate the new empty slot. > > Also simplifies kvm_handle_hva locking. > > > r = kvm_arch_prepare_memory_region(kvm,&new, old, user_alloc); > if (r) > goto out_free; > r == 0 > > - spin_lock(&kvm->mmu_lock); > - if (mem->slot>= kvm->memslots->nmemslots) > - kvm->memslots->nmemslots = mem->slot + 1; > +#ifdef CONFIG_DMAR > + /* map the pages in iommu page table */ > + if (npages) > + r = kvm_iommu_map_pages(kvm,&new); > + if (r) > + goto out_free; > Bad indentation. (still r == 0) > +#endif > > - *memslot = new; > - spin_unlock(&kvm->mmu_lock); > + slots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL); > + if (!slots) > + goto out_free; > goto out_free with r == 0. Best to assign r = -ENOMEM each time, to avoid these headaches. > > +int memslot_id(struct kvm *kvm, gfn_t gfn) > Should be either static or kvm_memslot_id(). But the source is already like that, so leave it. > @@ -807,23 +808,18 @@ static int kvm_handle_hva(struct kvm *kv > int (*handler)(struct kvm *kvm, unsigned long *rmapp, > unsigned long data)) > { > - int i, j; > + int i, j, idx; > int retval = 0; > - struct kvm_memslots *slots = kvm->memslots; > + struct kvm_memslots *slots; > + > + idx = srcu_read_lock(&kvm->srcu); > Maybe a better place is in the mmu notifiers, so the code is shared (once other archs start to use mmu notifiers). > @@ -3020,16 +3017,20 @@ nomem: > */ > unsigned int kvm_mmu_calculate_mmu_pages(struct kvm *kvm) > { > - int i; > + int i, idx; > unsigned int nr_mmu_pages; > unsigned int nr_pages = 0; > + struct kvm_memslots *slots; > > - for (i = 0; i< kvm->memslots->nmemslots; i++) > - nr_pages += kvm->memslots->memslots[i].npages; > + idx = srcu_read_lock(&kvm->srcu); > + slots = rcu_dereference(kvm->memslots); > + for (i = 0; i< slots->nmemslots; i++) > + nr_pages += slots->memslots[i].npages; > > nr_mmu_pages = nr_pages * KVM_PERMILLE_MMU_PAGES / 1000; > nr_mmu_pages = max(nr_mmu_pages, > (unsigned int) KVM_MIN_ALLOC_MMU_PAGES); > + srcu_read_unlock(&kvm->srcu, idx); > > Again, would like to move the srcu_locking to an outer scope. > @@ -1503,10 +1504,18 @@ static void enter_pmode(struct kvm_vcpu > static gva_t rmode_tss_base(struct kvm *kvm) > { > if (!kvm->arch.tss_addr) { > - gfn_t base_gfn = kvm->memslots->memslots[0].base_gfn + > - kvm->memslots->memslots[0].npages - 3; > + struct kvm_memslots *slots; > + gfn_t base_gfn; > + int idx; > + > + idx = srcu_read_lock(&kvm->srcu); > + slots = rcu_dereference(kvm->memslots); > + base_gfn = slots->memslots[0].base_gfn + > + slots->memslots[0].npages - 3; > + srcu_read_unlock(&kvm->srcu, idx); > return base_gfn<< PAGE_SHIFT; > } > + > return kvm->arch.tss_addr; > } > Shouldn't we already hold the srcu_lock as part of normal guest exit (similar to down_read() we do today)? -- error compiling committee.c: too many arguments to function