From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jamie Heilman Subject: Re: [PATCH] kvm: fix to update memslots properly Date: Sat, 27 Dec 2014 22:52:59 +0000 Message-ID: <20141227225259.GA4029@cucamonga.audible.transient.net> 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 , Igor Mammedov To: Paolo Bonzini Return-path: Received: from audible.transient.net ([24.143.126.66]:46559 "HELO audible.transient.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1750826AbaL0WxA (ORCPT ); Sat, 27 Dec 2014 17:53:00 -0500 Content-Disposition: inline In-Reply-To: <549F1989.5050305@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Paolo Bonzini wrote: > 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; I gave this a try, and it works just fine for me too. -- Jamie Heilman http://audible.transient.net/~jamie/