[PATCH v2 00/10] kvm: memory slot cleanups, fix, and increase

[PATCH v2 00/10] kvm: memory slot cleanups, fix, and increase

[Alex Williamson]

v2: Update 02/10 to not check userspace_addr when slot is removed.
    Yoshikawa-san withdrew objection to increase slot_bitmap prior
    to his series to remove slot_bitmap.

This series does away with any kind of complicated resizing of the
slot array and simply does a one time increase.  I do compact struct
kvm_memory_slot a bit to take better advantage of the space we are
using.  This reduces each slot from 64 bytes (x86_64) to 56 bytes.
By enforcing the API around valid operations for an unused slot and
fields that can be modified runtime, I found and was able to fix a
bug in iommu mapping for slots.  The renames enabled me to find the
previously posted bug fix for catching slot overlaps.

As mentioned in the series, the primary motivation for increasing
memory slots is assigned devices.  With this, I've been able to
assign 30 devices to a single VM and could have gone further, but
ran out of SRIOV VFs.  Typical devices use anywhere from 2-4 slots
and max out at 8 slots.  125 user slots (3 private slots) allows
us to support between 28 and 56 typical devices per VM.

Tested on x86_64, compiled on ia64, powerpc, and s390.

Thanks,
Alex

---

Alex Williamson (10):
      kvm: Restrict non-existing slot state transitions
      kvm: Check userspace_addr when modifying a memory slot
      kvm: Fix iommu map/unmap to handle memory slot moves
      kvm: Minor memory slot optimization
      kvm: Rename KVM_MEMORY_SLOTS -> KVM_USER_MEM_SLOTS
      kvm: Make KVM_PRIVATE_MEM_SLOTS optional
      kvm: struct kvm_memory_slot.user_alloc -> bool
      kvm: struct kvm_memory_slot.flags -> u32
      kvm: struct kvm_memory_slot.id -> short
      kvm: Increase user memory slots on x86 to 125


 arch/ia64/include/asm/kvm_host.h    |    4 --
 arch/ia64/kvm/kvm-ia64.c            |    8 ++--
 arch/powerpc/include/asm/kvm_host.h |    6 +--
 arch/powerpc/kvm/book3s_hv.c        |    2 -
 arch/powerpc/kvm/powerpc.c          |    4 +-
 arch/s390/include/asm/kvm_host.h    |    4 --
 arch/s390/kvm/kvm-s390.c            |    4 +-
 arch/x86/include/asm/kvm_host.h     |    8 ++--
 arch/x86/include/asm/vmx.h          |    6 +--
 arch/x86/kvm/vmx.c                  |    6 +--
 arch/x86/kvm/x86.c                  |   10 ++---
 include/linux/kvm_host.h            |   24 +++++++-----
 virt/kvm/kvm_main.c                 |   72 +++++++++++++++++++++++------------
 13 files changed, 90 insertions(+), 68 deletions(-)

[PATCH v2 01/10] kvm: Restrict non-existing slot state transitions

[Alex Williamson]

The API documentation states:

	When changing an existing slot, it may be moved in the guest
	physical memory space, or its flags may be modified.

An "existing slot" requires a non-zero npages (memory_size).  The only
transition we should therefore allow for a non-existing slot should be
to create the slot, which includes setting a non-zero memory_size.  We
currently allow calls to modify non-existing slots, which is pointless,
confusing, and possibly wrong.

With this we know that the invalidation path of __kvm_set_memory_region
is always for a delete or move and never for adding a zero size slot.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 virt/kvm/kvm_main.c |    9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6e8fa7e..e426704 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -753,10 +753,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	new.npages = npages;
 	new.flags = mem->flags;
 
-	/* Disallow changing a memory slot's size. */
+	/*
+	 * 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_free;
+	if (!npages && !old.npages)
+		goto out_free;
 
 	/* Check for overlaps */
 	r = -EEXIST;
@@ -775,7 +780,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	r = -ENOMEM;
 
 	/* Allocate if a slot is being created */
-	if (npages && !old.npages) {
+	if (!old.npages) {
 		new.user_alloc = user_alloc;
 		new.userspace_addr = mem->userspace_addr;
 

[PATCH v2 02/10] kvm: Check userspace_addr when modifying a memory slot

[Alex Williamson]

The API documents that only flags and guest physical memory space can
be modified on an existing slot, but we don't enforce that the
userspace address cannot be modified.  Instead we just ignore it.
This means that a user may think they've successfully moved both the
guest and user addresses, when in fact only the guest address changed.
Check and error instead.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 virt/kvm/kvm_main.c |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e426704..6e9d3f9 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -779,13 +779,19 @@ int __kvm_set_memory_region(struct kvm *kvm,
 
 	r = -ENOMEM;
 
-	/* Allocate if a slot is being created */
+	/*
+	 * Allocate if a slot is being created.  If modifying a slot,
+	 * the userspace_addr cannot change.
+	 */
 	if (!old.npages) {
 		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 */

[PATCH v2 03/10] kvm: Fix iommu map/unmap to handle memory slot moves

[Alex Williamson]

The iommu integration into memory slots expects memory slots to be
added or removed and doesn't handle the move case.  We can unmap
slots from the iommu after we mark them invalid and map them before
installing the final memslot array.  Also re-order the kmemdup vs
map so we don't leave iommu mappings if we get ENOMEM.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 virt/kvm/kvm_main.c |   19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6e9d3f9..0f66a0e 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -817,6 +817,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		old_memslots = kvm->memslots;
 		rcu_assign_pointer(kvm->memslots, slots);
 		synchronize_srcu_expedited(&kvm->srcu);
+		/* slot was deleted or moved, clear iommu mapping */
+		kvm_iommu_unmap_pages(kvm, &old);
 		/* From this point no new shadow pages pointing to a deleted,
 		 * or moved, memslot will be created.
 		 *
@@ -832,20 +834,19 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	if (r)
 		goto out_free;
 
-	/* map/unmap the pages in iommu page table */
-	if (npages) {
-		r = kvm_iommu_map_pages(kvm, &new);
-		if (r)
-			goto out_free;
-	} else
-		kvm_iommu_unmap_pages(kvm, &old);
-
 	r = -ENOMEM;
 	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
 			GFP_KERNEL);
 	if (!slots)
 		goto out_free;
 
+	/* map new memory slot into the iommu */
+	if (npages) {
+		r = kvm_iommu_map_pages(kvm, &new);
+		if (r)
+			goto out_slots;
+	}
+
 	/* actual memory is freed via old in kvm_free_physmem_slot below */
 	if (!npages) {
 		new.dirty_bitmap = NULL;
@@ -864,6 +865,8 @@ int __kvm_set_memory_region(struct kvm *kvm,
 
 	return 0;
 
+out_slots:
+	kfree(slots);
 out_free:
 	kvm_free_physmem_slot(&new, &old);
 out:

[PATCH v2 04/10] kvm: Minor memory slot optimization

[Alex Williamson]

If a slot is removed or moved in the guest physical address space, we
first allocate and install a new slot array with the invalidated
entry.  The old array is then freed.  We then proceed to allocate yet
another slot array to install the permanent replacement.  Re-use the
original array when this occurs and avoid the extra kfree/kmalloc.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 virt/kvm/kvm_main.c |   21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 0f66a0e..2a80dac 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -711,7 +711,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	unsigned long npages;
 	struct kvm_memory_slot *memslot, *slot;
 	struct kvm_memory_slot old, new;
-	struct kvm_memslots *slots, *old_memslots;
+	struct kvm_memslots *slots = NULL, *old_memslots;
 
 	r = check_memory_region_flags(mem);
 	if (r)
@@ -827,18 +827,25 @@ int __kvm_set_memory_region(struct kvm *kvm,
 		 * 	- kvm_is_visible_gfn (mmu_check_roots)
 		 */
 		kvm_arch_flush_shadow_memslot(kvm, slot);
-		kfree(old_memslots);
+		slots = old_memslots;
 	}
 
 	r = kvm_arch_prepare_memory_region(kvm, &new, old, mem, user_alloc);
 	if (r)
-		goto out_free;
+		goto out_slots;
 
 	r = -ENOMEM;
-	slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
-			GFP_KERNEL);
-	if (!slots)
-		goto out_free;
+	/*
+	 * We can re-use the old_memslots from above, the only difference
+	 * from the currently installed memslots is the invalid flag.  This
+	 * will get overwritten by update_memslots anyway.
+	 */
+	if (!slots) {
+		slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots),
+				GFP_KERNEL);
+		if (!slots)
+			goto out_free;
+	}
 
 	/* map new memory slot into the iommu */
 	if (npages) {

[PATCH v2 05/10] kvm: Rename KVM_MEMORY_SLOTS -> KVM_USER_MEM_SLOTS

[Alex Williamson]

It's easy to confuse KVM_MEMORY_SLOTS and KVM_MEM_SLOTS_NUM.  One is
the user accessible slots and the other is user + private.  Make this
more obvious.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 arch/ia64/include/asm/kvm_host.h    |    2 +-
 arch/ia64/kvm/kvm-ia64.c            |    2 +-
 arch/powerpc/include/asm/kvm_host.h |    4 ++--
 arch/powerpc/kvm/book3s_hv.c        |    2 +-
 arch/s390/include/asm/kvm_host.h    |    2 +-
 arch/x86/include/asm/kvm_host.h     |    4 ++--
 arch/x86/include/asm/vmx.h          |    6 +++---
 arch/x86/kvm/x86.c                  |    6 +++---
 include/linux/kvm_host.h            |    2 +-
 virt/kvm/kvm_main.c                 |    8 ++++----
 10 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index 6d6a5ac..48d7b0e 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -23,7 +23,7 @@
 #ifndef __ASM_KVM_HOST_H
 #define __ASM_KVM_HOST_H
 
-#define KVM_MEMORY_SLOTS 32
+#define KVM_USER_MEM_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
 
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index 8b3a9c0..f1a46bd 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -1831,7 +1831,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 	mutex_lock(&kvm->slots_lock);
 
 	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
+	if (log->slot >= KVM_USER_MEM_SLOTS)
 		goto out;
 
 	memslot = id_to_memslot(kvm->memslots, log->slot);
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 28e8f5e..5eb1dd8 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -37,10 +37,10 @@
 
 #define KVM_MAX_VCPUS		NR_CPUS
 #define KVM_MAX_VCORES		NR_CPUS
-#define KVM_MEMORY_SLOTS 32
+#define KVM_USER_MEM_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
-#define KVM_MEM_SLOTS_NUM (KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS)
+#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 
 #ifdef CONFIG_KVM_MMIO
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
diff --git a/arch/powerpc/kvm/book3s_hv.c b/arch/powerpc/kvm/book3s_hv.c
index 721d460..75ce80e 100644
--- a/arch/powerpc/kvm/book3s_hv.c
+++ b/arch/powerpc/kvm/book3s_hv.c
@@ -1262,7 +1262,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 	mutex_lock(&kvm->slots_lock);
 
 	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
+	if (log->slot >= KVM_USER_MEM_SLOTS)
 		goto out;
 
 	memslot = id_to_memslot(kvm->memslots, log->slot);
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index b784154..ac33432 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -20,7 +20,7 @@
 #include <asm/cpu.h>
 
 #define KVM_MAX_VCPUS 64
-#define KVM_MEMORY_SLOTS 32
+#define KVM_USER_MEM_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
 
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b2e11f4..e619519 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -31,10 +31,10 @@
 
 #define KVM_MAX_VCPUS 254
 #define KVM_SOFT_MAX_VCPUS 160
-#define KVM_MEMORY_SLOTS 32
+#define KVM_USER_MEM_SLOTS 32
 /* memory slots that does not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 4
-#define KVM_MEM_SLOTS_NUM (KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS)
+#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 
 #define KVM_MMIO_SIZE 16
 
diff --git a/arch/x86/include/asm/vmx.h b/arch/x86/include/asm/vmx.h
index 36ec21c..72932d2 100644
--- a/arch/x86/include/asm/vmx.h
+++ b/arch/x86/include/asm/vmx.h
@@ -427,9 +427,9 @@ enum vmcs_field {
 
 #define AR_RESERVD_MASK 0xfffe0f00
 
-#define TSS_PRIVATE_MEMSLOT			(KVM_MEMORY_SLOTS + 0)
-#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 1)
-#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_MEMORY_SLOTS + 2)
+#define TSS_PRIVATE_MEMSLOT			(KVM_USER_MEM_SLOTS + 0)
+#define APIC_ACCESS_PAGE_PRIVATE_MEMSLOT	(KVM_USER_MEM_SLOTS + 1)
+#define IDENTITY_PAGETABLE_PRIVATE_MEMSLOT	(KVM_USER_MEM_SLOTS + 2)
 
 #define VMX_NR_VPIDS				(1 << 16)
 #define VMX_VPID_EXTENT_SINGLE_CONTEXT		1
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 4f76417..1aa3fae 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2204,7 +2204,7 @@ int kvm_dev_ioctl_check_extension(long ext)
 		r = KVM_MAX_VCPUS;
 		break;
 	case KVM_CAP_NR_MEMSLOTS:
-		r = KVM_MEMORY_SLOTS;
+		r = KVM_USER_MEM_SLOTS;
 		break;
 	case KVM_CAP_PV_MMU:	/* obsolete */
 		r = 0;
@@ -3135,7 +3135,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log)
 	mutex_lock(&kvm->slots_lock);
 
 	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
+	if (log->slot >= KVM_USER_MEM_SLOTS)
 		goto out;
 
 	memslot = id_to_memslot(kvm->memslots, log->slot);
@@ -6449,7 +6449,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 	int map_flags = MAP_PRIVATE | MAP_ANONYMOUS;
 
 	/* Prevent internal slot pages from being moved by fork()/COW. */
-	if (memslot->id >= KVM_MEMORY_SLOTS)
+	if (memslot->id >= KVM_USER_MEM_SLOTS)
 		map_flags = MAP_SHARED | MAP_ANONYMOUS;
 
 	/*To keep backward compatibility with older userspace,
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index ecc5543..fb9354d 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -299,7 +299,7 @@ struct kvm_irq_routing_table {};
 #endif
 
 #ifndef KVM_MEM_SLOTS_NUM
-#define KVM_MEM_SLOTS_NUM (KVM_MEMORY_SLOTS + KVM_PRIVATE_MEM_SLOTS)
+#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 #endif
 
 /*
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2a80dac..e429cd6 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -766,7 +766,7 @@ int __kvm_set_memory_region(struct kvm *kvm,
 	/* Check for overlaps */
 	r = -EEXIST;
 	kvm_for_each_memslot(slot, kvm->memslots) {
-		if (slot->id >= KVM_MEMORY_SLOTS || slot == memslot)
+		if (slot->id >= KVM_USER_MEM_SLOTS || slot == memslot)
 			continue;
 		if (!((base_gfn + npages <= slot->base_gfn) ||
 		      (base_gfn >= slot->base_gfn + slot->npages)))
@@ -900,7 +900,7 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   kvm_userspace_memory_region *mem,
 				   int user_alloc)
 {
-	if (mem->slot >= KVM_MEMORY_SLOTS)
+	if (mem->slot >= KVM_USER_MEM_SLOTS)
 		return -EINVAL;
 	return kvm_set_memory_region(kvm, mem, user_alloc);
 }
@@ -914,7 +914,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
 	unsigned long any = 0;
 
 	r = -EINVAL;
-	if (log->slot >= KVM_MEMORY_SLOTS)
+	if (log->slot >= KVM_USER_MEM_SLOTS)
 		goto out;
 
 	memslot = id_to_memslot(kvm->memslots, log->slot);
@@ -960,7 +960,7 @@ int kvm_is_visible_gfn(struct kvm *kvm, gfn_t gfn)
 {
 	struct kvm_memory_slot *memslot = gfn_to_memslot(kvm, gfn);
 
-	if (!memslot || memslot->id >= KVM_MEMORY_SLOTS ||
+	if (!memslot || memslot->id >= KVM_USER_MEM_SLOTS ||
 	      memslot->flags & KVM_MEMSLOT_INVALID)
 		return 0;
 

[PATCH v2 06/10] kvm: Make KVM_PRIVATE_MEM_SLOTS optional

[Alex Williamson]

Seems like everyone copied x86 and defined 4 private memory slots
that never actually get used.  Even x86 only uses 3 of the 4.  These
aren't exposed so there's no need to add padding.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 arch/ia64/include/asm/kvm_host.h    |    2 --
 arch/powerpc/include/asm/kvm_host.h |    4 +---
 arch/s390/include/asm/kvm_host.h    |    2 --
 arch/x86/include/asm/kvm_host.h     |    4 ++--
 include/linux/kvm_host.h            |    4 ++++
 5 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index 48d7b0e..cfa7498 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -24,8 +24,6 @@
 #define __ASM_KVM_HOST_H
 
 #define KVM_USER_MEM_SLOTS 32
-/* memory slots that does not exposed to userspace */
-#define KVM_PRIVATE_MEM_SLOTS 4
 
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
 
diff --git a/arch/powerpc/include/asm/kvm_host.h b/arch/powerpc/include/asm/kvm_host.h
index 5eb1dd8..23ca70d 100644
--- a/arch/powerpc/include/asm/kvm_host.h
+++ b/arch/powerpc/include/asm/kvm_host.h
@@ -38,9 +38,7 @@
 #define KVM_MAX_VCPUS		NR_CPUS
 #define KVM_MAX_VCORES		NR_CPUS
 #define KVM_USER_MEM_SLOTS 32
-/* memory slots that does not exposed to userspace */
-#define KVM_PRIVATE_MEM_SLOTS 4
-#define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
+#define KVM_MEM_SLOTS_NUM KVM_USER_MEM_SLOTS
 
 #ifdef CONFIG_KVM_MMIO
 #define KVM_COALESCED_MMIO_PAGE_OFFSET 1
diff --git a/arch/s390/include/asm/kvm_host.h b/arch/s390/include/asm/kvm_host.h
index ac33432..711c5ab 100644
--- a/arch/s390/include/asm/kvm_host.h
+++ b/arch/s390/include/asm/kvm_host.h
@@ -21,8 +21,6 @@
 
 #define KVM_MAX_VCPUS 64
 #define KVM_USER_MEM_SLOTS 32
-/* memory slots that does not exposed to userspace */
-#define KVM_PRIVATE_MEM_SLOTS 4
 
 struct sca_entry {
 	atomic_t scn;
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e619519..ce8b037 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -32,8 +32,8 @@
 #define KVM_MAX_VCPUS 254
 #define KVM_SOFT_MAX_VCPUS 160
 #define KVM_USER_MEM_SLOTS 32
-/* memory slots that does not exposed to userspace */
-#define KVM_PRIVATE_MEM_SLOTS 4
+/* memory slots that are not exposed to userspace */
+#define KVM_PRIVATE_MEM_SLOTS 3
 #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 
 #define KVM_MMIO_SIZE 16
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index fb9354d..bf8380f 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -298,6 +298,10 @@ struct kvm_irq_routing_table {};
 
 #endif
 
+#ifndef KVM_PRIVATE_MEM_SLOTS
+#define KVM_PRIVATE_MEM_SLOTS 0
+#endif
+
 #ifndef KVM_MEM_SLOTS_NUM
 #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)
 #endif

[PATCH v2 07/10] kvm: struct kvm_memory_slot.user_alloc -> bool

[Alex Williamson]

There's no need for this to be an int, it holds a boolean.
Move to the end of the struct for alignment.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 arch/ia64/kvm/kvm-ia64.c   |    6 +++---
 arch/powerpc/kvm/powerpc.c |    4 ++--
 arch/s390/kvm/kvm-s390.c   |    4 ++--
 arch/x86/kvm/vmx.c         |    6 +++---
 arch/x86/kvm/x86.c         |    4 ++--
 include/linux/kvm_host.h   |   12 ++++++------
 virt/kvm/kvm_main.c        |    9 +++++----
 7 files changed, 23 insertions(+), 22 deletions(-)

diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f1a46bd..a8b4022 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -955,7 +955,7 @@ long kvm_arch_vm_ioctl(struct file *filp,
 					kvm_mem.guest_phys_addr;
 		kvm_userspace_mem.memory_size = kvm_mem.memory_size;
 		r = kvm_vm_ioctl_set_memory_region(kvm,
-					&kvm_userspace_mem, 0);
+					&kvm_userspace_mem, false);
 		if (r)
 			goto out;
 		break;
@@ -1577,7 +1577,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 		struct kvm_memory_slot *memslot,
 		struct kvm_memory_slot old,
 		struct kvm_userspace_memory_region *mem,
-		int user_alloc)
+		bool user_alloc)
 {
 	unsigned long i;
 	unsigned long pfn;
@@ -1608,7 +1608,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 		struct kvm_userspace_memory_region *mem,
 		struct kvm_memory_slot old,
-		int user_alloc)
+		bool user_alloc)
 {
 	return;
 }
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 4d213b8..da606a9 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -321,7 +321,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
                                    struct kvm_memory_slot *memslot,
                                    struct kvm_memory_slot old,
                                    struct kvm_userspace_memory_region *mem,
-                                   int user_alloc)
+                                   bool user_alloc)
 {
 	return kvmppc_core_prepare_memory_region(kvm, mem);
 }
@@ -329,7 +329,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 void kvm_arch_commit_memory_region(struct kvm *kvm,
                struct kvm_userspace_memory_region *mem,
                struct kvm_memory_slot old,
-               int user_alloc)
+               bool user_alloc)
 {
 	kvmppc_core_commit_memory_region(kvm, mem);
 }
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index ecced9d..37646cb 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -927,7 +927,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				   struct kvm_memory_slot *memslot,
 				   struct kvm_memory_slot old,
 				   struct kvm_userspace_memory_region *mem,
-				   int user_alloc)
+				   bool user_alloc)
 {
 	/* A few sanity checks. We can have exactly one memory slot which has
 	   to start at guest virtual zero and which has to be located at a
@@ -957,7 +957,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 				struct kvm_userspace_memory_region *mem,
 				struct kvm_memory_slot old,
-				int user_alloc)
+				bool user_alloc)
 {
 	int rc;
 
diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index f858159..108becc 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -3597,7 +3597,7 @@ static int alloc_apic_access_page(struct kvm *kvm)
 	kvm_userspace_mem.flags = 0;
 	kvm_userspace_mem.guest_phys_addr = 0xfee00000ULL;
 	kvm_userspace_mem.memory_size = PAGE_SIZE;
-	r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, 0);
+	r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false);
 	if (r)
 		goto out;
 
@@ -3627,7 +3627,7 @@ static int alloc_identity_pagetable(struct kvm *kvm)
 	kvm_userspace_mem.guest_phys_addr =
 		kvm->arch.ept_identity_map_addr;
 	kvm_userspace_mem.memory_size = PAGE_SIZE;
-	r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, 0);
+	r = __kvm_set_memory_region(kvm, &kvm_userspace_mem, false);
 	if (r)
 		goto out;
 
@@ -4191,7 +4191,7 @@ static int vmx_set_tss_addr(struct kvm *kvm, unsigned int addr)
 		.flags = 0,
 	};
 
-	ret = kvm_set_memory_region(kvm, &tss_mem, 0);
+	ret = kvm_set_memory_region(kvm, &tss_mem, false);
 	if (ret)
 		return ret;
 	kvm->arch.tss_addr = addr;
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 1aa3fae..85dc733 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -6443,7 +6443,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *memslot,
 				struct kvm_memory_slot old,
 				struct kvm_userspace_memory_region *mem,
-				int user_alloc)
+				bool user_alloc)
 {
 	int npages = memslot->npages;
 	int map_flags = MAP_PRIVATE | MAP_ANONYMOUS;
@@ -6479,7 +6479,7 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 				struct kvm_userspace_memory_region *mem,
 				struct kvm_memory_slot old,
-				int user_alloc)
+				bool user_alloc)
 {
 
 	int nr_mmu_pages = 0, npages = mem->memory_size >> PAGE_SHIFT;
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bf8380f..9ff30f2 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -255,8 +255,8 @@ struct kvm_memory_slot {
 	unsigned long *dirty_bitmap;
 	struct kvm_arch_memory_slot arch;
 	unsigned long userspace_addr;
-	int user_alloc;
 	int id;
+	bool user_alloc;
 };
 
 static inline unsigned long kvm_dirty_bitmap_bytes(struct kvm_memory_slot *memslot)
@@ -436,10 +436,10 @@ id_to_memslot(struct kvm_memslots *slots, int id)
 
 int kvm_set_memory_region(struct kvm *kvm,
 			  struct kvm_userspace_memory_region *mem,
-			  int user_alloc);
+			  bool user_alloc);
 int __kvm_set_memory_region(struct kvm *kvm,
 			    struct kvm_userspace_memory_region *mem,
-			    int user_alloc);
+			    bool user_alloc);
 void kvm_arch_free_memslot(struct kvm_memory_slot *free,
 			   struct kvm_memory_slot *dont);
 int kvm_arch_create_memslot(struct kvm_memory_slot *slot, unsigned long npages);
@@ -447,11 +447,11 @@ int kvm_arch_prepare_memory_region(struct kvm *kvm,
 				struct kvm_memory_slot *memslot,
 				struct kvm_memory_slot old,
 				struct kvm_userspace_memory_region *mem,
-				int user_alloc);
+				bool user_alloc);
 void kvm_arch_commit_memory_region(struct kvm *kvm,
 				struct kvm_userspace_memory_region *mem,
 				struct kvm_memory_slot old,
-				int user_alloc);
+				bool user_alloc);
 bool kvm_largepages_enabled(void);
 void kvm_disable_largepages(void);
 /* flush all memory translations */
@@ -537,7 +537,7 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct
 				   kvm_userspace_memory_region *mem,
-				   int user_alloc);
+				   bool user_alloc);
 int kvm_vm_ioctl_irq_line(struct kvm *kvm, struct kvm_irq_level *irq_level);
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index e429cd6..8c70391 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -704,7 +704,7 @@ static int check_memory_region_flags(struct kvm_userspace_memory_region *mem)
  */
 int __kvm_set_memory_region(struct kvm *kvm,
 			    struct kvm_userspace_memory_region *mem,
-			    int user_alloc)
+			    bool user_alloc)
 {
 	int r;
 	gfn_t base_gfn;
@@ -884,7 +884,7 @@ EXPORT_SYMBOL_GPL(__kvm_set_memory_region);
 
 int kvm_set_memory_region(struct kvm *kvm,
 			  struct kvm_userspace_memory_region *mem,
-			  int user_alloc)
+			  bool user_alloc)
 {
 	int r;
 
@@ -898,7 +898,7 @@ EXPORT_SYMBOL_GPL(kvm_set_memory_region);
 int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
 				   struct
 				   kvm_userspace_memory_region *mem,
-				   int user_alloc)
+				   bool user_alloc)
 {
 	if (mem->slot >= KVM_USER_MEM_SLOTS)
 		return -EINVAL;
@@ -2158,7 +2158,8 @@ static long kvm_vm_ioctl(struct file *filp,
 						sizeof kvm_userspace_mem))
 			goto out;
 
-		r = kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem, 1);
+		r = kvm_vm_ioctl_set_memory_region(kvm,
+						   &kvm_userspace_mem, true);
 		if (r)
 			goto out;
 		break;

[PATCH v2 08/10] kvm: struct kvm_memory_slot.flags -> u32

[Alex Williamson]

struct kvm_userspace_memory_region.flags is a u32 with a comment that
bits 0 ~ 15 are visible to userspace and the other bits are reserved
for kvm internal use.  KVM_MEMSLOT_INVALID is the only internal use
flag and it has a comment that bits 16 ~ 31 are internally used and
the other bits are visible to userspace.

Therefore, let's define this as a u32 so we don't waste bytes on LP64
systems.  Move to the end of the struct for alignment.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 include/linux/kvm_host.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 9ff30f2..641f5fb 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -251,10 +251,10 @@ static inline int kvm_vcpu_exiting_guest_mode(struct kvm_vcpu *vcpu)
 struct kvm_memory_slot {
 	gfn_t base_gfn;
 	unsigned long npages;
-	unsigned long flags;
 	unsigned long *dirty_bitmap;
 	struct kvm_arch_memory_slot arch;
 	unsigned long userspace_addr;
+	u32 flags;
 	int id;
 	bool user_alloc;
 };

[PATCH v2 09/10] kvm: struct kvm_memory_slot.id -> short

[Alex Williamson]

We're currently offering a whopping 32 memory slots to user space, an
int is a bit excessive for storing this.  We would like to increase
our memslots, but SHRT_MAX should be more than enough.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 include/linux/kvm_host.h |    4 ++--
 virt/kvm/kvm_main.c      |    2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 641f5fb..87089dd 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -255,7 +255,7 @@ struct kvm_memory_slot {
 	struct kvm_arch_memory_slot arch;
 	unsigned long userspace_addr;
 	u32 flags;
-	int id;
+	short id;
 	bool user_alloc;
 };
 
@@ -315,7 +315,7 @@ struct kvm_memslots {
 	u64 generation;
 	struct kvm_memory_slot memslots[KVM_MEM_SLOTS_NUM];
 	/* The mapping table from slot id to the index in memslots[]. */
-	int id_to_index[KVM_MEM_SLOTS_NUM];
+	short id_to_index[KVM_MEM_SLOTS_NUM];
 };
 
 struct kvm {
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8c70391..c4c8ec1 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -469,6 +469,8 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	INIT_HLIST_HEAD(&kvm->irq_ack_notifier_list);
 #endif
 
+	BUILD_BUG_ON(KVM_MEM_SLOTS_NUM > SHRT_MAX);
+
 	r = -ENOMEM;
 	kvm->memslots = kzalloc(sizeof(struct kvm_memslots), GFP_KERNEL);
 	if (!kvm->memslots)

[PATCH v2 10/10] kvm: Increase user memory slots on x86 to 125

[Alex Williamson]

With the 3 private slots, this gives us a nice round 128 slots total.
The primary motivation for this is to support more assigned devices.
Each assigned device can theoretically use up to 8 slots (6 MMIO BARs,
1 ROM BAR, 1 spare for a split MSI-X table mapping) though it's far
more typical for a device to use 3-4 slots.  If we assume a typical VM
uses a dozen slots for non-assigned devices purposes, we should always
be able to support 14 worst case assigned devices or 28 to 37 typical
devices.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---
 arch/x86/include/asm/kvm_host.h |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index ce8b037..9558a1e 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -31,7 +31,7 @@
 
 #define KVM_MAX_VCPUS 254
 #define KVM_SOFT_MAX_VCPUS 160
-#define KVM_USER_MEM_SLOTS 32
+#define KVM_USER_MEM_SLOTS 125
 /* memory slots that are not exposed to userspace */
 #define KVM_PRIVATE_MEM_SLOTS 3
 #define KVM_MEM_SLOTS_NUM (KVM_USER_MEM_SLOTS + KVM_PRIVATE_MEM_SLOTS)

Re: [PATCH v2 01/10] kvm: Restrict non-existing slot state transitions

[Marcelo Tosatti]

On Mon, Dec 10, 2012 at 10:32:45AM -0700, Alex Williamson wrote:
> The API documentation states:
> 
> 	When changing an existing slot, it may be moved in the guest
> 	physical memory space, or its flags may be modified.
> 
> An "existing slot" requires a non-zero npages (memory_size).  The only
> transition we should therefore allow for a non-existing slot should be
> to create the slot, which includes setting a non-zero memory_size.  We
> currently allow calls to modify non-existing slots, which is pointless,
> confusing, and possibly wrong.
> 
> With this we know that the invalidation path of __kvm_set_memory_region
> is always for a delete or move and never for adding a zero size slot.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
>  virt/kvm/kvm_main.c |    9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 6e8fa7e..e426704 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -753,10 +753,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	new.npages = npages;
>  	new.flags = mem->flags;
>  
> -	/* Disallow changing a memory slot's size. */
> +	/*
> +	 * 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_free;
> +	if (!npages && !old.npages)
> +		goto out_free;

        /* General sanity checks */
        if (mem->memory_size & (PAGE_SIZE - 1))
                goto out;
        if (mem->guest_phys_addr & (PAGE_SIZE - 1))
                goto out;

        if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
                goto out;

If mem->memory_size is zero, then check above fails. 
If mem->memory_size > 0, it must be at least PAGE_SIZE, in which case 
npages == 1.

Which means the code does not allow changes to non-existing slots.
Yes?

Re: [PATCH v2 01/10] kvm: Restrict non-existing slot state transitions

[Marcelo Tosatti]

On Tue, Dec 11, 2012 at 11:29:09PM -0200, Marcelo Tosatti wrote:
> On Mon, Dec 10, 2012 at 10:32:45AM -0700, Alex Williamson wrote:
> > The API documentation states:
> > 
> > 	When changing an existing slot, it may be moved in the guest
> > 	physical memory space, or its flags may be modified.
> > 
> > An "existing slot" requires a non-zero npages (memory_size).  The only
> > transition we should therefore allow for a non-existing slot should be
> > to create the slot, which includes setting a non-zero memory_size.  We
> > currently allow calls to modify non-existing slots, which is pointless,
> > confusing, and possibly wrong.
> > 
> > With this we know that the invalidation path of __kvm_set_memory_region
> > is always for a delete or move and never for adding a zero size slot.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> >  virt/kvm/kvm_main.c |    9 +++++++--
> >  1 file changed, 7 insertions(+), 2 deletions(-)
> > 
> > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > index 6e8fa7e..e426704 100644
> > --- a/virt/kvm/kvm_main.c
> > +++ b/virt/kvm/kvm_main.c
> > @@ -753,10 +753,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
> >  	new.npages = npages;
> >  	new.flags = mem->flags;
> >  
> > -	/* Disallow changing a memory slot's size. */
> > +	/*
> > +	 * 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_free;
> > +	if (!npages && !old.npages)
> > +		goto out_free;
> 
>         /* General sanity checks */
>         if (mem->memory_size & (PAGE_SIZE - 1))
>                 goto out;
>         if (mem->guest_phys_addr & (PAGE_SIZE - 1))
>                 goto out;
> 
>         if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
>                 goto out;
> 
> If mem->memory_size is zero, then check above fails. 

Err, no read a "<=" while writing that.

> If mem->memory_size > 0, it must be at least PAGE_SIZE, in which case 
> npages == 1.
> 
> Which means the code does not allow changes to non-existing slots.
> Yes?
> 

Re: [PATCH v2 00/10] kvm: memory slot cleanups, fix, and increase

[Marcelo Tosatti]

On Mon, Dec 10, 2012 at 10:32:39AM -0700, Alex Williamson wrote:
> v2: Update 02/10 to not check userspace_addr when slot is removed.
>     Yoshikawa-san withdrew objection to increase slot_bitmap prior
>     to his series to remove slot_bitmap.
> 
> This series does away with any kind of complicated resizing of the
> slot array and simply does a one time increase.  I do compact struct
> kvm_memory_slot a bit to take better advantage of the space we are
> using.  This reduces each slot from 64 bytes (x86_64) to 56 bytes.
> By enforcing the API around valid operations for an unused slot and
> fields that can be modified runtime, I found and was able to fix a
> bug in iommu mapping for slots.  The renames enabled me to find the
> previously posted bug fix for catching slot overlaps.
> 
> As mentioned in the series, the primary motivation for increasing
> memory slots is assigned devices.  With this, I've been able to
> assign 30 devices to a single VM and could have gone further, but
> ran out of SRIOV VFs.  Typical devices use anywhere from 2-4 slots
> and max out at 8 slots.  125 user slots (3 private slots) allows
> us to support between 28 and 56 typical devices per VM.
> 
> Tested on x86_64, compiled on ia64, powerpc, and s390.
> 
> Thanks,
> Alex

Reviewed-by: Marcelo Tosatti <mtosatti@redhat.com>

Re: [PATCH v2 01/10] kvm: Restrict non-existing slot state transitions

[Alex Williamson]

On Tue, 2012-12-11 at 23:39 -0200, Marcelo Tosatti wrote:
> On Tue, Dec 11, 2012 at 11:29:09PM -0200, Marcelo Tosatti wrote:
> > On Mon, Dec 10, 2012 at 10:32:45AM -0700, Alex Williamson wrote:
> > > The API documentation states:
> > > 
> > > 	When changing an existing slot, it may be moved in the guest
> > > 	physical memory space, or its flags may be modified.
> > > 
> > > An "existing slot" requires a non-zero npages (memory_size).  The only
> > > transition we should therefore allow for a non-existing slot should be
> > > to create the slot, which includes setting a non-zero memory_size.  We
> > > currently allow calls to modify non-existing slots, which is pointless,
> > > confusing, and possibly wrong.
> > > 
> > > With this we know that the invalidation path of __kvm_set_memory_region
> > > is always for a delete or move and never for adding a zero size slot.
> > > 
> > > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > > ---
> > >  virt/kvm/kvm_main.c |    9 +++++++--
> > >  1 file changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> > > index 6e8fa7e..e426704 100644
> > > --- a/virt/kvm/kvm_main.c
> > > +++ b/virt/kvm/kvm_main.c
> > > @@ -753,10 +753,15 @@ int __kvm_set_memory_region(struct kvm *kvm,
> > >  	new.npages = npages;
> > >  	new.flags = mem->flags;
> > >  
> > > -	/* Disallow changing a memory slot's size. */
> > > +	/*
> > > +	 * 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_free;
> > > +	if (!npages && !old.npages)
> > > +		goto out_free;
> > 
> >         /* General sanity checks */
> >         if (mem->memory_size & (PAGE_SIZE - 1))
> >                 goto out;
> >         if (mem->guest_phys_addr & (PAGE_SIZE - 1))
> >                 goto out;
> > 
> >         if (mem->guest_phys_addr + mem->memory_size < mem->guest_phys_addr)
> >                 goto out;
> > 
> > If mem->memory_size is zero, then check above fails. 
> 
> Err, no read a "<=" while writing that.

;)  Thanks for the double check,

Alex

> > If mem->memory_size > 0, it must be at least PAGE_SIZE, in which case 
> > npages == 1.
> > 
> > Which means the code does not allow changes to non-existing slots.
> > Yes?
> > 

Re: [PATCH v2 00/10] kvm: memory slot cleanups, fix, and increase

[Gleb Natapov]

On Mon, Dec 10, 2012 at 10:32:39AM -0700, Alex Williamson wrote:
> v2: Update 02/10 to not check userspace_addr when slot is removed.
>     Yoshikawa-san withdrew objection to increase slot_bitmap prior
>     to his series to remove slot_bitmap.
> 
> This series does away with any kind of complicated resizing of the
> slot array and simply does a one time increase.  I do compact struct
> kvm_memory_slot a bit to take better advantage of the space we are
> using.  This reduces each slot from 64 bytes (x86_64) to 56 bytes.
> By enforcing the API around valid operations for an unused slot and
> fields that can be modified runtime, I found and was able to fix a
> bug in iommu mapping for slots.  The renames enabled me to find the
> previously posted bug fix for catching slot overlaps.
> 
> As mentioned in the series, the primary motivation for increasing
> memory slots is assigned devices.  With this, I've been able to
> assign 30 devices to a single VM and could have gone further, but
> ran out of SRIOV VFs.  Typical devices use anywhere from 2-4 slots
> and max out at 8 slots.  125 user slots (3 private slots) allows
> us to support between 28 and 56 typical devices per VM.
> 
> Tested on x86_64, compiled on ia64, powerpc, and s390.
> 
> Thanks,
> Alex
> 
Reviewed-by: Gleb Natapov <gleb@redhat.com>

> ---
> 
> Alex Williamson (10):
>       kvm: Restrict non-existing slot state transitions
>       kvm: Check userspace_addr when modifying a memory slot
>       kvm: Fix iommu map/unmap to handle memory slot moves
>       kvm: Minor memory slot optimization
>       kvm: Rename KVM_MEMORY_SLOTS -> KVM_USER_MEM_SLOTS
>       kvm: Make KVM_PRIVATE_MEM_SLOTS optional
>       kvm: struct kvm_memory_slot.user_alloc -> bool
>       kvm: struct kvm_memory_slot.flags -> u32
>       kvm: struct kvm_memory_slot.id -> short
>       kvm: Increase user memory slots on x86 to 125
> 
> 
>  arch/ia64/include/asm/kvm_host.h    |    4 --
>  arch/ia64/kvm/kvm-ia64.c            |    8 ++--
>  arch/powerpc/include/asm/kvm_host.h |    6 +--
>  arch/powerpc/kvm/book3s_hv.c        |    2 -
>  arch/powerpc/kvm/powerpc.c          |    4 +-
>  arch/s390/include/asm/kvm_host.h    |    4 --
>  arch/s390/kvm/kvm-s390.c            |    4 +-
>  arch/x86/include/asm/kvm_host.h     |    8 ++--
>  arch/x86/include/asm/vmx.h          |    6 +--
>  arch/x86/kvm/vmx.c                  |    6 +--
>  arch/x86/kvm/x86.c                  |   10 ++---
>  include/linux/kvm_host.h            |   24 +++++++-----
>  virt/kvm/kvm_main.c                 |   72 +++++++++++++++++++++++------------
>  13 files changed, 90 insertions(+), 68 deletions(-)

--
			Gleb.

Re: [PATCH v2 00/10] kvm: memory slot cleanups, fix, and increase

[Marcelo Tosatti]

On Mon, Dec 10, 2012 at 10:32:39AM -0700, Alex Williamson wrote:
> v2: Update 02/10 to not check userspace_addr when slot is removed.
>     Yoshikawa-san withdrew objection to increase slot_bitmap prior
>     to his series to remove slot_bitmap.
> 
> This series does away with any kind of complicated resizing of the
> slot array and simply does a one time increase.  I do compact struct
> kvm_memory_slot a bit to take better advantage of the space we are
> using.  This reduces each slot from 64 bytes (x86_64) to 56 bytes.
> By enforcing the API around valid operations for an unused slot and
> fields that can be modified runtime, I found and was able to fix a
> bug in iommu mapping for slots.  The renames enabled me to find the
> previously posted bug fix for catching slot overlaps.
> 
> As mentioned in the series, the primary motivation for increasing
> memory slots is assigned devices.  With this, I've been able to
> assign 30 devices to a single VM and could have gone further, but
> ran out of SRIOV VFs.  Typical devices use anywhere from 2-4 slots
> and max out at 8 slots.  125 user slots (3 private slots) allows
> us to support between 28 and 56 typical devices per VM.
> 
> Tested on x86_64, compiled on ia64, powerpc, and s390.
> 
> Thanks,
> Alex

Applied, thanks.