| Message ID | 20200121223157.15263-13-sean.j.christopherson@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: Dynamically size memslot arrays | expand |
On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote: > Move memslot deletion into its own routine so that the success path for > other memslot updates does not need to use kvm_free_memslot(), i.e. can > explicitly destroy the dirty bitmap when necessary. This paves the way > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass > NULL for @dont. > > Add a comment above the code to make a copy of the existing memslot > prior to deletion, it is not at all obvious that the pointer will become > stale during sorting and/or installation of new memslots. Could you help explain a bit on this explicit comment? I can follow up with the patch itself which looks all correct to me, but I failed to catch what this extra comment wants to emphasize... Thanks, > > Note, kvm_arch_commit_memory_region() allows an architecture to free > resources when moving a memslot or changing its flags, e.g. x86 frees > its arch specific memslot metadata during commit_memory_region().
On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote: > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote: > > Move memslot deletion into its own routine so that the success path for > > other memslot updates does not need to use kvm_free_memslot(), i.e. can > > explicitly destroy the dirty bitmap when necessary. This paves the way > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass > > NULL for @dont. > > > > Add a comment above the code to make a copy of the existing memslot > > prior to deletion, it is not at all obvious that the pointer will become > > stale during sorting and/or installation of new memslots. > > Could you help explain a bit on this explicit comment? I can follow > up with the patch itself which looks all correct to me, but I failed > to catch what this extra comment wants to emphasize... It's tempting to write the code like this (I know, because I did it): if (!mem->memory_size) return kvm_delete_memslot(kvm, mem, slot, as_id); new = *slot; Where @slot is a pointer to the memslot to be deleted. At first, second, and third glances, this seems perfectly sane. The issue is that slot was pulled from struct kvm_memslots.memslots, e.g. slot = &slots->memslots[index]; Note that slots->memslots holds actual "struct kvm_memory_slot" objects, not pointers to slots. When update_memslots() sorts the slots, it swaps the actual slot objects, not pointers. I.e. after update_memslots(), even though @slot points at the same address, it's could be pointing at a different slot. As a result kvm_free_memslot() in kvm_delete_memslot() will free the dirty page info and arch-specific points for some random slot, not the intended slot, and will set npages=0 for that random slot.
On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote: > On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote: > > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote: > > > Move memslot deletion into its own routine so that the success path for > > > other memslot updates does not need to use kvm_free_memslot(), i.e. can > > > explicitly destroy the dirty bitmap when necessary. This paves the way > > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass > > > NULL for @dont. > > > > > > Add a comment above the code to make a copy of the existing memslot > > > prior to deletion, it is not at all obvious that the pointer will become > > > stale during sorting and/or installation of new memslots. > > > > Could you help explain a bit on this explicit comment? I can follow > > up with the patch itself which looks all correct to me, but I failed > > to catch what this extra comment wants to emphasize... > > It's tempting to write the code like this (I know, because I did it): > > if (!mem->memory_size) > return kvm_delete_memslot(kvm, mem, slot, as_id); > > new = *slot; > > Where @slot is a pointer to the memslot to be deleted. At first, second, > and third glances, this seems perfectly sane. > > The issue is that slot was pulled from struct kvm_memslots.memslots, e.g. > > slot = &slots->memslots[index]; > > Note that slots->memslots holds actual "struct kvm_memory_slot" objects, > not pointers to slots. When update_memslots() sorts the slots, it swaps > the actual slot objects, not pointers. I.e. after update_memslots(), even > though @slot points at the same address, it's could be pointing at a > different slot. As a result kvm_free_memslot() in kvm_delete_memslot() > will free the dirty page info and arch-specific points for some random > slot, not the intended slot, and will set npages=0 for that random slot. Ah I see, thanks. Another alternative is we move the "old = *slot" copy into kvm_delete_memslot(), which could be even clearer imo. However I'm not sure whether it's a good idea to drop the test-by for this. Considering that comment change should not affect it, would you mind enrich the comment into something like this (or anything better)? /* * Make a full copy of the old memslot, the pointer will become stale * when the memslots are re-sorted by update_memslots() in * kvm_delete_memslot(), while to make the kvm_free_memslot() work as * expected later on, we still need the cached memory slot. */ In all cases: Reviewed-by: Peter Xu <peterx@redhat.com>
On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote: > On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote: > > On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote: > > > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote: > > > > Move memslot deletion into its own routine so that the success path for > > > > other memslot updates does not need to use kvm_free_memslot(), i.e. can > > > > explicitly destroy the dirty bitmap when necessary. This paves the way > > > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass > > > > NULL for @dont. > > > > > > > > Add a comment above the code to make a copy of the existing memslot > > > > prior to deletion, it is not at all obvious that the pointer will become > > > > stale during sorting and/or installation of new memslots. > > > > > > Could you help explain a bit on this explicit comment? I can follow > > > up with the patch itself which looks all correct to me, but I failed > > > to catch what this extra comment wants to emphasize... > > > > It's tempting to write the code like this (I know, because I did it): > > > > if (!mem->memory_size) > > return kvm_delete_memslot(kvm, mem, slot, as_id); > > > > new = *slot; > > > > Where @slot is a pointer to the memslot to be deleted. At first, second, > > and third glances, this seems perfectly sane. > > > > The issue is that slot was pulled from struct kvm_memslots.memslots, e.g. > > > > slot = &slots->memslots[index]; > > > > Note that slots->memslots holds actual "struct kvm_memory_slot" objects, > > not pointers to slots. When update_memslots() sorts the slots, it swaps > > the actual slot objects, not pointers. I.e. after update_memslots(), even > > though @slot points at the same address, it's could be pointing at a > > different slot. As a result kvm_free_memslot() in kvm_delete_memslot() > > will free the dirty page info and arch-specific points for some random > > slot, not the intended slot, and will set npages=0 for that random slot. > > Ah I see, thanks. Another alternative is we move the "old = *slot" > copy into kvm_delete_memslot(), which could be even clearer imo. The copy is also needed in __kvm_set_memory_region() for the MOVE case. > However I'm not sure whether it's a good idea to drop the test-by for > this. Considering that comment change should not affect it, would you > mind enrich the comment into something like this (or anything better)? > > /* > * Make a full copy of the old memslot, the pointer will become stale > * when the memslots are re-sorted by update_memslots() in > * kvm_delete_memslot(), while to make the kvm_free_memslot() work as > * expected later on, we still need the cached memory slot. > */ As above, it's more subtle than just the kvm_delete_memslot() case. /* * Make a full copy of the old memslot, the pointer will become stale * when the memslots are re-sorted by update_memslots() when deleting * or moving a memslot, and additional modifications to the old memslot * need to be made after calling update_memslots(). */
On Fri, Feb 07, 2020 at 09:59:12AM -0800, Sean Christopherson wrote: > On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote: > > /* > > * Make a full copy of the old memslot, the pointer will become stale > > * when the memslots are re-sorted by update_memslots() in > > * kvm_delete_memslot(), while to make the kvm_free_memslot() work as > > * expected later on, we still need the cached memory slot. > > */ > > As above, it's more subtle than just the kvm_delete_memslot() case. > > /* > * Make a full copy of the old memslot, the pointer will become stale > * when the memslots are re-sorted by update_memslots() when deleting > * or moving a memslot, and additional modifications to the old memslot > * need to be made after calling update_memslots(). > */ Actually, that's not quite correct, as the same is true for all memslot updates, and we still query @old after update_memslots() for CREATE and FLAGS. This is better. /* * Make a full copy of the old memslot, the pointer will become stale * when the memslots are re-sorted by update_memslots(), and the old * memslot needs to be referenced after calling update_memslots(), e.g. * to free its resources and for arch specific behavior. */
On Fri, Feb 07, 2020 at 09:59:12AM -0800, Sean Christopherson wrote: > On Thu, Feb 06, 2020 at 11:51:16AM -0500, Peter Xu wrote: > > On Thu, Feb 06, 2020 at 08:28:18AM -0800, Sean Christopherson wrote: > > > On Thu, Feb 06, 2020 at 11:14:15AM -0500, Peter Xu wrote: > > > > On Tue, Jan 21, 2020 at 02:31:50PM -0800, Sean Christopherson wrote: > > > > > Move memslot deletion into its own routine so that the success path for > > > > > other memslot updates does not need to use kvm_free_memslot(), i.e. can > > > > > explicitly destroy the dirty bitmap when necessary. This paves the way > > > > > for dropping @dont from kvm_free_memslot(), i.e. all callers now pass > > > > > NULL for @dont. > > > > > > > > > > Add a comment above the code to make a copy of the existing memslot > > > > > prior to deletion, it is not at all obvious that the pointer will become > > > > > stale during sorting and/or installation of new memslots. > > > > > > > > Could you help explain a bit on this explicit comment? I can follow > > > > up with the patch itself which looks all correct to me, but I failed > > > > to catch what this extra comment wants to emphasize... > > > > > > It's tempting to write the code like this (I know, because I did it): > > > > > > if (!mem->memory_size) > > > return kvm_delete_memslot(kvm, mem, slot, as_id); > > > > > > new = *slot; > > > > > > Where @slot is a pointer to the memslot to be deleted. At first, second, > > > and third glances, this seems perfectly sane. > > > > > > The issue is that slot was pulled from struct kvm_memslots.memslots, e.g. > > > > > > slot = &slots->memslots[index]; > > > > > > Note that slots->memslots holds actual "struct kvm_memory_slot" objects, > > > not pointers to slots. When update_memslots() sorts the slots, it swaps > > > the actual slot objects, not pointers. I.e. after update_memslots(), even > > > though @slot points at the same address, it's could be pointing at a > > > different slot. As a result kvm_free_memslot() in kvm_delete_memslot() > > > will free the dirty page info and arch-specific points for some random > > > slot, not the intended slot, and will set npages=0 for that random slot. > > > > Ah I see, thanks. Another alternative is we move the "old = *slot" > > copy into kvm_delete_memslot(), which could be even clearer imo. > > The copy is also needed in __kvm_set_memory_region() for the MOVE case. Right. I actually meant to do all "old = *slot" in any function we need to cache the data explicitly, with that we also need another one after calling kvm_delete_memslot() for move. But with the comment as you suggested below it looks good to me too. Thanks, > > > However I'm not sure whether it's a good idea to drop the test-by for > > this. Considering that comment change should not affect it, would you > > mind enrich the comment into something like this (or anything better)? > > > > /* > > * Make a full copy of the old memslot, the pointer will become stale > > * when the memslots are re-sorted by update_memslots() in > > * kvm_delete_memslot(), while to make the kvm_free_memslot() work as > > * expected later on, we still need the cached memory slot. > > */ > > As above, it's more subtle than just the kvm_delete_memslot() case. > > /* > * Make a full copy of the old memslot, the pointer will become stale > * when the memslots are re-sorted by update_memslots() when deleting > * or moving a memslot, and additional modifications to the old memslot > * need to be made after calling update_memslots(). > */ >
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 69d6158cb405..5ae2dd6d9993 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -1042,6 +1042,27 @@ static int kvm_set_memslot(struct kvm *kvm, return r; } +static int kvm_delete_memslot(struct kvm *kvm, + const struct kvm_userspace_memory_region *mem, + struct kvm_memory_slot *old, int as_id) +{ + struct kvm_memory_slot new; + int r; + + if (!old->npages) + return -EINVAL; + + memset(&new, 0, sizeof(new)); + new.id = old->id; + + r = kvm_set_memslot(kvm, mem, old, &new, as_id, KVM_MR_DELETE); + if (r) + return r; + + kvm_free_memslot(kvm, old, NULL); + return 0; +} + /* * Allocate some memory and give it an address in the guest physical address * space. @@ -1091,7 +1112,15 @@ int __kvm_set_memory_region(struct kvm *kvm, if (npages > KVM_MEM_MAX_NR_PAGES) return -EINVAL; - new = old = *slot; + /* + * Make a full copy of the old memslot, the pointer will become stale + * when the memslots are re-sorted by update_memslots(). + */ + old = *slot; + if (!mem->memory_size) + return kvm_delete_memslot(kvm, mem, &old, as_id); + + new = old; new.id = id; new.base_gfn = base_gfn; @@ -1099,29 +1128,20 @@ int __kvm_set_memory_region(struct kvm *kvm, new.flags = mem->flags; new.userspace_addr = mem->userspace_addr; - if (npages) { - if (!old.npages) - change = KVM_MR_CREATE; - else { /* Modify an existing slot. */ - if ((new.userspace_addr != old.userspace_addr) || - (npages != old.npages) || - ((new.flags ^ old.flags) & KVM_MEM_READONLY)) - return -EINVAL; - - if (base_gfn != old.base_gfn) - change = KVM_MR_MOVE; - else if (new.flags != old.flags) - change = KVM_MR_FLAGS_ONLY; - else /* Nothing to change. */ - return 0; - } - } else { - if (!old.npages) + if (!old.npages) { + change = KVM_MR_CREATE; + } else { /* Modify an existing slot. */ + if ((new.userspace_addr != old.userspace_addr) || + (npages != old.npages) || + ((new.flags ^ old.flags) & KVM_MEM_READONLY)) return -EINVAL; - change = KVM_MR_DELETE; - new.base_gfn = 0; - new.flags = 0; + if (base_gfn != old.base_gfn) + change = KVM_MR_MOVE; + else if (new.flags != old.flags) + change = KVM_MR_FLAGS_ONLY; + else /* Nothing to change. */ + return 0; } if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) { @@ -1144,17 +1164,12 @@ int __kvm_set_memory_region(struct kvm *kvm, return r; } - /* actual memory is freed via old in kvm_free_memslot below */ - if (change == KVM_MR_DELETE) { - new.dirty_bitmap = NULL; - memset(&new.arch, 0, sizeof(new.arch)); - } - r = kvm_set_memslot(kvm, mem, &old, &new, as_id, change); if (r) goto out_bitmap; - kvm_free_memslot(kvm, &old, &new); + if (old.dirty_bitmap && !new.dirty_bitmap) + kvm_destroy_dirty_bitmap(&old); return 0; out_bitmap: