From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chen, Tiejun" Subject: Re: [PATCH] kvm: fix to update memslots properly Date: Tue, 10 Mar 2015 14:17:10 +0800 Message-ID: <54FE8C66.8020001@intel.com> References: <1419569710-8127-1-git-send-email-tiejun.chen@intel.com> <549F1989.5050305@redhat.com> <20150309205438.GA3204@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: KVM list , Andy Lutomirski , jamie@audible.transient.net, Igor Mammedov To: Marcelo Tosatti , Paolo Bonzini Return-path: Received: from mga14.intel.com ([192.55.52.115]:9315 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751016AbbCJGRN (ORCPT ); Tue, 10 Mar 2015 02:17:13 -0400 In-Reply-To: <20150309205438.GA3204@amt.cnet> Sender: kvm-owner@vger.kernel.org List-ID: On 2015/3/10 4:54, Marcelo Tosatti wrote: > 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? > But this is already applied long time ago... Tiejun