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; PaoloPaolo, Can you include a proper changelog for this patch?
But this is already applied long time ago... Tiejun -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
