[PATCH 0/2] KVM: set_memory_region: Cleanup and new restriction

[PATCH 0/2] KVM: set_memory_region: Cleanup and new restriction

[Takuya Yoshikawa]

Patch 1: just rebased for this series.
Patch 2: an API change, so please let me know if you notice any problems.

Takuya Yoshikawa (2):
  KVM: set_memory_region: Identify the requested change explicitly
  KVM: set_memory_region: Disallow changing read-only attribute later

 Documentation/virtual/kvm/api.txt |   12 ++--
 virt/kvm/kvm_main.c               |   95 +++++++++++++++++++++----------------
 2 files changed, 60 insertions(+), 47 deletions(-)

-- 
1.7.5.4

[PATCH 1/2 -v3] KVM: set_memory_region: Identify the requested change explicitly

[Takuya Yoshikawa]

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

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

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index a83ca63..64c5dc3 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -719,6 +719,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.
  *
@@ -737,6 +755,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)
@@ -780,17 +799,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) {
@@ -808,20 +840,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 */
@@ -830,7 +854,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);
@@ -881,7 +905,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);
@@ -896,7 +920,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));
 	}
-- 
1.7.5.4

[PATCH 2/2] KVM: set_memory_region: Disallow changing read-only attribute later

[Takuya Yoshikawa]

As Xiao pointed out, there are a few problems with it:
 - kvm_arch_commit_memory_region() write protects the memory slot only
   for GET_DIRTY_LOG when modifying the flags.
 - FNAME(sync_page) uses the old spte value to set a new one without
   checking KVM_MEM_READONLY flag.

Since we flush all shadow pages when creating a new slot, the simplest
fix is to disallow such problematic flag changes: this is safe because
no one is doing such things.

Signed-off-by: Takuya Yoshikawa <yoshikawa_takuya_b1@lab.ntt.co.jp>
Cc: Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>
Cc: Alex Williamson <alex.williamson@redhat.com>
---
 Documentation/virtual/kvm/api.txt |   12 ++++++------
 virt/kvm/kvm_main.c               |   35 ++++++++++++-----------------------
 2 files changed, 18 insertions(+), 29 deletions(-)

diff --git a/Documentation/virtual/kvm/api.txt b/Documentation/virtual/kvm/api.txt
index 09905cb..0e03b19 100644
--- a/Documentation/virtual/kvm/api.txt
+++ b/Documentation/virtual/kvm/api.txt
@@ -874,12 +874,12 @@ It is recommended that the lower 21 bits of guest_phys_addr and userspace_addr
 be identical.  This allows large pages in the guest to be backed by large
 pages in the host.
 
-The flags field supports two flag, KVM_MEM_LOG_DIRTY_PAGES, which instructs
-kvm to keep track of writes to memory within the slot.  See KVM_GET_DIRTY_LOG
-ioctl.  The KVM_CAP_READONLY_MEM capability indicates the availability of the
-KVM_MEM_READONLY flag.  When this flag is set for a memory region, KVM only
-allows read accesses.  Writes will be posted to userspace as KVM_EXIT_MMIO
-exits.
+The flags field supports two flags: KVM_MEM_LOG_DIRTY_PAGES and
+KVM_MEM_READONLY.  The former can be set to instruct KVM to keep track of
+writes to memory within the slot.  See KVM_GET_DIRTY_LOG ioctl to know how to
+use it.  The latter can be set, if KVM_CAP_READONLY_MEM capability allows it,
+to make a new slot read-only.  In this case, writes to this memory will be
+posted to userspace as KVM_EXIT_MMIO exits.
 
 When the KVM_CAP_SYNC_MMU capability is available, changes in the backing of
 the memory region are automatically reflected into the guest.  For example, an
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 64c5dc3..2e93630 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -754,7 +754,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	struct kvm_memory_slot *slot;
 	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);
@@ -797,15 +796,14 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	new.npages = npages;
 	new.flags = mem->flags;
 
-	old_iommu_mapped = old.npages;
-
 	r = -EINVAL;
 	if (npages) {
 		if (!old.npages)
 			change = KVM_MR_CREATE;
 		else { /* Modify an existing slot. */
 			if ((mem->userspace_addr != old.userspace_addr) ||
-			    (npages != old.npages))
+			    (npages != old.npages) ||
+			    ((new.flags ^ old.flags) & KVM_MEM_READONLY))
 				goto out;
 
 			if (base_gfn != old.base_gfn)
@@ -867,7 +865,6 @@ int __kvm_set_memory_region(struct kvm *kvm,
 
 		/* slot was deleted or moved, clear iommu mapping */
 		kvm_iommu_unmap_pages(kvm, &old);
-		old_iommu_mapped = false;
 		/* From this point no new shadow pages pointing to a deleted,
 		 * or moved, memslot will be created.
 		 *
@@ -898,25 +895,17 @@ int __kvm_set_memory_region(struct kvm *kvm,
 
 	/*
 	 * IOMMU mapping:  New slots need to be mapped.  Old slots need to be
-	 * un-mapped and re-mapped if their base changes or if flags that the
-	 * iommu cares about change (read-only).  Base change unmapping is
-	 * handled above with slot deletion, so we only unmap incompatible
-	 * flags here.  Anything else the iommu might care about for existing
-	 * slots (size changes, userspace addr changes) is disallowed above,
-	 * so any other attribute changes getting here can be skipped.
+	 * un-mapped and re-mapped if their base changes.  Since base change
+	 * unmapping is handled above with slot deletion, mapping alone is
+	 * needed here.  Anything else the iommu might care about for existing
+	 * slots (size changes, userspace addr changes and read-only flag
+	 * changes) is disallowed above, so any other attribute changes getting
+	 * here can be skipped.
 	 */
-	if (change != KVM_MR_DELETE) {
-		if (old_iommu_mapped &&
-		    ((new.flags ^ old.flags) & KVM_MEM_READONLY)) {
-			kvm_iommu_unmap_pages(kvm, &old);
-			old_iommu_mapped = false;
-		}
-
-		if (!old_iommu_mapped) {
-			r = kvm_iommu_map_pages(kvm, &new);
-			if (r)
-				goto out_slots;
-		}
+	if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) {
+		r = kvm_iommu_map_pages(kvm, &new);
+		if (r)
+			goto out_slots;
 	}
 
 	/* actual memory is freed via old in kvm_free_physmem_slot below */
-- 
1.7.5.4

Re: [PATCH 0/2] KVM: set_memory_region: Cleanup and new restriction

[Gleb Natapov]

On Wed, Jan 30, 2013 at 07:38:37PM +0900, Takuya Yoshikawa wrote:
> Patch 1: just rebased for this series.
> Patch 2: an API change, so please let me know if you notice any problems.
> 
> Takuya Yoshikawa (2):
>   KVM: set_memory_region: Identify the requested change explicitly
>   KVM: set_memory_region: Disallow changing read-only attribute later
> 
>  Documentation/virtual/kvm/api.txt |   12 ++--
>  virt/kvm/kvm_main.c               |   95 +++++++++++++++++++++----------------
>  2 files changed, 60 insertions(+), 47 deletions(-)
> 
Reviewed-by: Gleb Natapov <gleb@redhat.com>

--
			Gleb.

Re: [PATCH 0/2] KVM: set_memory_region: Cleanup and new restriction

[Marcelo Tosatti]

On Thu, Jan 31, 2013 at 12:15:01PM +0200, Gleb Natapov wrote:
> On Wed, Jan 30, 2013 at 07:38:37PM +0900, Takuya Yoshikawa wrote:
> > Patch 1: just rebased for this series.
> > Patch 2: an API change, so please let me know if you notice any problems.
> > 
> > Takuya Yoshikawa (2):
> >   KVM: set_memory_region: Identify the requested change explicitly
> >   KVM: set_memory_region: Disallow changing read-only attribute later
> > 
> >  Documentation/virtual/kvm/api.txt |   12 ++--
> >  virt/kvm/kvm_main.c               |   95 +++++++++++++++++++++----------------
> >  2 files changed, 60 insertions(+), 47 deletions(-)
> > 
> Reviewed-by: Gleb Natapov <gleb@redhat.com>

Applied, thanks.