From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH] kvm: fix to update memslots properly Date: Mon, 9 Mar 2015 17:54:38 -0300 Message-ID: <20150309205438.GA3204@amt.cnet> References: <1419569710-8127-1-git-send-email-tiejun.chen@intel.com> <549F1989.5050305@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Tiejun Chen , KVM list , Andy Lutomirski , jamie@audible.transient.net, Igor Mammedov To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:46226 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752718AbbCIUzP (ORCPT ); Mon, 9 Mar 2015 16:55:15 -0400 Content-Disposition: inline In-Reply-To: <549F1989.5050305@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sat, Dec 27, 2014 at 09:41:45PM +0100, Paolo Bonzini wrote: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index f528343..6e52f3f 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -672,6 +672,7 @@ static void update_memslots(struct kvm_memslots *slots, > > WARN_ON(mslots[i].id != id); > > if (!new->npages) { > > new->base_gfn = 0; > > + new->flags = 0; > > if (mslots[i].npages) > > slots->used_slots--; > > } else { > > This should not be necessary. The part of the mslots array that has > base_gfn == npages == 0 is entirely unused, and such a slot can never > be returned by search_memslots because this: > > if (gfn >= memslots[slot].base_gfn && > gfn < memslots[slot].base_gfn + memslots[slot].npages) > > can never be true. > > > @@ -688,7 +689,9 @@ static void update_memslots(struct kvm_memslots *slots, > > i++; > > } > > while (i > 0 && > > - new->base_gfn > mslots[i - 1].base_gfn) { > > + ((new->base_gfn > mslots[i - 1].base_gfn) || > > + (!new->base_gfn && > > + !mslots[i - 1].base_gfn && !mslots[i - 1].npages))) { > > mslots[i] = mslots[i - 1]; > > slots->id_to_index[mslots[i].id] = i; > > i--; > > > > You should have explained _why_ this fixes the bug, and what invariant > is not being respected, something like this: > > kvm: fix sorting of memslots with base_gfn == 0 > > Before commit 0e60b0799fed (kvm: change memslot sorting rule from size > to GFN, 2014-12-01), the memslots' sorting key was npages, meaning > that a valid memslot couldn't have its sorting key equal to zero. > On the other hand, a valid memslot can have base_gfn == 0, and invalid > memslots are identified by base_gfn == npages == 0. > > Because of this, commit 0e60b0799fed broke the invariant that invalid > memslots are at the end of the mslots array. When a memslot with > base_gfn == 0 was created, any invalid memslot before it were left > in place. > > This suggests another fix. We can change the insertion to use a ">=" > comparison, as in your first patch. Alone it is not correct, but we > only need to take some care and avoid breaking the case of deleting a > memslot. > > It's enough to wrap the second loop (that you patched) with > "if (new->npages)". In the new->npages == 0 case the first loop has > already set i to the right value, and moving i back would be wrong: > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > index f5283438ee05..050974c051b5 100644 > --- a/virt/kvm/kvm_main.c > +++ b/virt/kvm/kvm_main.c > @@ -687,11 +687,23 @@ static void update_memslots(struct kvm_memslots *slots, > slots->id_to_index[mslots[i].id] = i; > i++; > } > - while (i > 0 && > - new->base_gfn > mslots[i - 1].base_gfn) { > - mslots[i] = mslots[i - 1]; > - slots->id_to_index[mslots[i].id] = i; > - i--; > + > + /* > + * The ">=" is needed when creating a slot with base_gfn == 0, > + * so that it moves before all those with base_gfn == npages == 0. > + * > + * On the other hand, if new->npages is zero, the above loop has > + * already left i pointing to the beginning of the empty part of > + * mslots, and the ">=" would move the hole backwards in this > + * case---which is wrong. So skip the loop when deleting a slot. > + */ > + if (new->npages) { > + while (i > 0 && > + new->base_gfn >= mslots[i - 1].base_gfn) { > + mslots[i] = mslots[i - 1]; > + slots->id_to_index[mslots[i].id] = i; > + i--; > + } > } > > mslots[i] = *new; > > Paolo Paolo, Can you include a proper changelog for this patch?