diff mbox

[v3] KVM: set_memory_region: Identify the requested change explicitly

Message ID 20130129110007.4e8ccf76.yoshikawa_takuya_b1@lab.ntt.co.jp (mailing list archive)
State New, archived
Headers show

Commit Message

Takuya Yoshikawa Jan. 29, 2013, 2 a.m. UTC
KVM_SET_USER_MEMORY_REGION forces __kvm_set_memory_region() to identify
what kind of change is being requested by checking the arguments.  The
current code does this checking at various points in code and each
condition being used there is not easy to understand at first glance.

This patch consolidates these checks and introduces an enum to name the
possible changes to clean up the code.

Although this does not introduce any functional changes, there is one
change which optimizes the code a bit: if we have nothing to change, the
new code returns 0 immediately.

Note that the return value for this case cannot be changed since QEMU
relies on it: we noticed this when we changed it to -EINVAL and got a
section mismatch error at the final stage of live migration.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
---
v2: updated iommu related parts
v3: converted !(A == B) to A != B
Note: based on this patch, disallowing RO change will become trivial:
    check such changes when we identify KVM_MR_FLAGS_ONLY.

 virt/kvm/kvm_main.c |   64 +++++++++++++++++++++++++++++++++++----------------
 1 files changed, 44 insertions(+), 20 deletions(-)

Comments

Marcelo Tosatti Feb. 5, 2013, 12:01 a.m. UTC | #1
On Tue, Jan 29, 2013 at 11:00:07AM +0900, Takuya Yoshikawa wrote:
> KVM_SET_USER_MEMORY_REGION forces __kvm_set_memory_region() to identify
> what kind of change is being requested by checking the arguments.  The
> current code does this checking at various points in code and each
> condition being used there is not easy to understand at first glance.
> 
> This patch consolidates these checks and introduces an enum to name the
> possible changes to clean up the code.
> 
> Although this does not introduce any functional changes, there is one
> change which optimizes the code a bit: if we have nothing to change, the
> new code returns 0 immediately.
> 
> Note that the return value for this case cannot be changed since QEMU
> relies on it: we noticed this when we changed it to -EINVAL and got a
> section mismatch error at the final stage of live migration.
> 
> Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
> ---
> v2: updated iommu related parts
> v3: converted !(A == B) to A != B
> Note: based on this patch, disallowing RO change will become trivial:
>     check such changes when we identify KVM_MR_FLAGS_ONLY.
> 
>  virt/kvm/kvm_main.c |   64 +++++++++++++++++++++++++++++++++++----------------
>  1 files changed, 44 insertions(+), 20 deletions(-)

Applied, thanks. It would be good to propagate the enum into
kvm_arch_prepare/commit memory region.

--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Takuya Yoshikawa Feb. 5, 2013, 1:35 a.m. UTC | #2
On Mon, 4 Feb 2013 22:01:33 -0200
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> Applied, thanks. It would be good to propagate the enum into
> kvm_arch_prepare/commit memory region.

Yes, that's what I want to do after ARM-KVM gets merged.

	Takuya
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 3fec2cd..c07be48 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -714,6 +714,24 @@  static struct kvm_memslots *install_new_memslots(struct kvm *kvm,
 }
 
 /*
+ * KVM_SET_USER_MEMORY_REGION ioctl allows the following operations:
+ * - create a new memory slot
+ * - delete an existing memory slot
+ * - modify an existing memory slot
+ *   -- move it in the guest physical memory space
+ *   -- just change its flags
+ *
+ * Since flags can be changed by some of these operations, the following
+ * differentiation is the best we can do for __kvm_set_memory_region():
+ */
+enum kvm_mr_change {
+	KVM_MR_CREATE,
+	KVM_MR_DELETE,
+	KVM_MR_MOVE,
+	KVM_MR_FLAGS_ONLY,
+};
+
+/*
  * Allocate some memory and give it an address in the guest physical address
  * space.
  *
@@ -732,6 +750,7 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	struct kvm_memory_slot old, new;
 	struct kvm_memslots *slots = NULL, *old_memslots;
 	bool old_iommu_mapped;
+	enum kvm_mr_change change;
 
 	r = check_memory_region_flags(mem);
 	if (r)
@@ -775,17 +794,30 @@  int __kvm_set_memory_region(struct kvm *kvm,
 
 	old_iommu_mapped = old.npages;
 
-	/*
-	 * Disallow changing a memory slot's size or changing anything about
-	 * zero sized slots that doesn't involve making them non-zero.
-	 */
 	r = -EINVAL;
-	if (npages && old.npages && npages != old.npages)
-		goto out;
-	if (!npages && !old.npages)
+	if (npages) {
+		if (!old.npages)
+			change = KVM_MR_CREATE;
+		else { /* Modify an existing slot. */
+			if ((mem->userspace_addr != old.userspace_addr) ||
+			    (npages != old.npages))
+				goto out;
+
+			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. */
+				r = 0;
+				goto out;
+			}
+		}
+	} else if (old.npages) {
+		change = KVM_MR_DELETE;
+	} else /* Modify a non-existent slot: disallowed. */
 		goto out;
 
-	if ((npages && !old.npages) || (base_gfn != old.base_gfn)) {
+	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
 		/* Check for overlaps */
 		r = -EEXIST;
 		kvm_for_each_memslot(slot, kvm->memslots) {
@@ -803,20 +835,12 @@  int __kvm_set_memory_region(struct kvm *kvm,
 		new.dirty_bitmap = NULL;
 
 	r = -ENOMEM;
-
-	/*
-	 * Allocate if a slot is being created.  If modifying a slot,
-	 * the userspace_addr cannot change.
-	 */
-	if (!old.npages) {
+	if (change == KVM_MR_CREATE) {
 		new.user_alloc = user_alloc;
 		new.userspace_addr = mem->userspace_addr;
 
 		if (kvm_arch_create_memslot(&new, npages))
 			goto out_free;
-	} else if (npages && mem->userspace_addr != old.userspace_addr) {
-		r = -EINVAL;
-		goto out_free;
 	}
 
 	/* Allocate page dirty bitmap if needed */
@@ -825,7 +849,7 @@  int __kvm_set_memory_region(struct kvm *kvm,
 			goto out_free;
 	}
 
-	if (!npages || base_gfn != old.base_gfn) {
+	if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) {
 		r = -ENOMEM;
 		slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
 				GFP_KERNEL);
@@ -876,7 +900,7 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	 * slots (size changes, userspace addr changes) is disallowed above,
 	 * so any other attribute changes getting here can be skipped.
 	 */
-	if (npages) {
+	if (change != KVM_MR_DELETE) {
 		if (old_iommu_mapped &&
 		    ((new.flags ^ old.flags) & KVM_MEM_READONLY)) {
 			kvm_iommu_unmap_pages(kvm, &old);
@@ -891,7 +915,7 @@  int __kvm_set_memory_region(struct kvm *kvm,
 	}
 
 	/* actual memory is freed via old in kvm_free_physmem_slot below */
-	if (!npages) {
+	if (change == KVM_MR_DELETE) {
 		new.dirty_bitmap = NULL;
 		memset(&new.arch, 0, sizeof(new.arch));
 	}