On Thu, Nov 04, 2021, Ben Gardon wrote:

quoted

@@ -1597,6 +1596,26 @@ static int kvm_set_memslot(struct kvm *kvm,
                kvm_copy_memslots(slots, __kvm_memslots(kvm, as_id));
        }

+       /*
+        * 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.  This needs to
+        * happen *after* (re)acquiring slots_arch_lock.
+        */
+       slot = id_to_memslot(slots, new->id);
+       if (slot) {
+               old = *slot;
+       } else {
+               WARN_ON_ONCE(change != KVM_MR_CREATE);
+               memset(&old, 0, sizeof(old));
+               old.id = new->id;
+               old.as_id = as_id;
+       }
+
+       /* Copy the arch-specific data, again after (re)acquiring slots_arch_lock. */
+       memcpy(&new->arch, &old.arch, sizeof(old.arch));
+

Is new->arch not initialized before this function is called? Does this
need to be here, or could it be moved above into the first branch of
the if statement?
Oh I see you removed the memset below and replaced it with this. I
think this is fine, but it might be easier to reason about if we left
the memset and moved the memcopy into the if.
No point in doing a memcpy of zeros here.

Hmm, good point.  I wrote it like this so that the "arch" part is more identifiable
since that's what needs to be protected by the lock, but I completely agree that
it's odd when viewed without that lens.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel