[PATCH 0/9] Simplify memory registration

[PATCH 0/9] Simplify memory registration

[Glauber Costa]

Hi guys

This patchset simplifies memory registration even further. It's a respin
of the one I've already sent, but now, without the kernel memory case, it's
in definitive form.

There are still some issues to be solved regarding the whole kvm memory thing
(as aliases and logging, for example), but the current form after the patchset
already allows us to remove all the calls from kvm_cpu_register_phys_mem() from
all around the place, in favor of a single hook.

Important: This is _not_ tested in architectures other than x86. So, if people
with access to hardware covered by the other ports can test and ack it, it would
be really awesome.

For those waiting on QemuAccel updates, this is the memory patchset I was talking about.
We should have news on that front after we settle here, so please review.

thanks!

[PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

Actually, all registrations are the same. If IO_MEM_ROM is set, we only
need to take care of not passing its value as the phys_offset.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/qemu-kvm.c |   31 +++++++++++--------------------
 1 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index c522a28..58a6d4a 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -776,26 +776,17 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long phys_offset)
 {
     int r = 0;
-    if (!(phys_offset & ~TARGET_PAGE_MASK)) {
-        r = kvm_is_allocated_mem(kvm_context, start_addr, size);
-        if (r)
-            return;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
-    if (phys_offset & IO_MEM_ROM) {
-        phys_offset &= ~IO_MEM_ROM;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
+
+    phys_offset &= ~IO_MEM_ROM;
+    r = kvm_is_allocated_mem(kvm_context, start_addr, size);
+    if (r)
+        return;
+    r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    if (r)
+        kvm_create_mem_hole(kvm_context, start_addr, size);
+    r = kvm_register_phys_mem(kvm_context, start_addr,
+                              phys_ram_base + phys_offset,
+                              size, 0);
     if (r < 0) {
         printf("kvm_cpu_register_physical_memory: failed\n");
         exit(1);
-- 
1.5.5.1

[PATCH 2/9] do not use mem_hole anymore.

[Glauber Costa]

memory holes are totally evil. Right now they work for some basic tests,
but had never been stressed enough. Using memory holes leaves open questions like:

* what happens if a area being registered span two slots?
* what happens if there is already data in the slots?

also, the code behaves badly if the piece to be removed lies in the boundaries of the
current slot. Luckily, we don't really need it. Remove it, and make sure we never hit it.


Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   69 +-----------------------------------------------------
 qemu/qemu-kvm.c |   13 +++++----
 2 files changed, 9 insertions(+), 73 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index 63fbcba..e768e44 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -436,74 +436,9 @@ int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 	return 0;
 }
 
-int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start,
-			unsigned long len)
-{
-	int slot;
-	int r;
-	struct kvm_userspace_memory_region rmslot;
-	struct kvm_userspace_memory_region newslot1;
-	struct kvm_userspace_memory_region newslot2;
-
-	len = (len + PAGE_SIZE - 1) & PAGE_MASK;
-
-	slot = get_intersecting_slot(phys_start);
-	/* no need to create hole, as there is already hole */
-	if (slot == -1)
-		return 0;
-
-	memset(&rmslot, 0, sizeof(struct kvm_userspace_memory_region));
-	memset(&newslot1, 0, sizeof(struct kvm_userspace_memory_region));
-	memset(&newslot2, 0, sizeof(struct kvm_userspace_memory_region));
-
-	rmslot.guest_phys_addr = slots[slot].phys_addr;
-	rmslot.slot = slot;
-
-	newslot1.guest_phys_addr = slots[slot].phys_addr;
-	newslot1.memory_size = phys_start - slots[slot].phys_addr;
-	newslot1.slot = slot;
-	newslot1.userspace_addr = slots[slot].userspace_addr;
-	newslot1.flags = slots[slot].flags;
-
-	newslot2.guest_phys_addr = newslot1.guest_phys_addr +
-				   newslot1.memory_size + len;
-	newslot2.memory_size = slots[slot].phys_addr +
-			       slots[slot].len - newslot2.guest_phys_addr;
-	newslot2.userspace_addr = newslot1.userspace_addr +
-				  newslot1.memory_size;
-	newslot2.slot = get_free_slot(kvm);
-	newslot2.flags = newslot1.flags;
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &rmslot);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	free_slot(slot);
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &newslot1);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	register_slot(newslot1.slot, newslot1.guest_phys_addr,
-		      newslot1.memory_size, newslot1.userspace_addr,
-		      newslot1.flags);
-
-	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &newslot2);
-	if (r == -1) {
-		fprintf(stderr, "kvm_create_mem_hole: %s\n", strerror(errno));
-		return -1;
-	}
-	register_slot(newslot2.slot, newslot2.guest_phys_addr,
-		      newslot2.memory_size, newslot2.userspace_addr,
-		      newslot2.flags);
-	return 0;
-}
-
 int kvm_register_phys_mem(kvm_context_t kvm,
-			unsigned long phys_start, void *userspace_addr,
-			unsigned long len, int log)
+			  unsigned long phys_start, void *userspace_addr,
+			  unsigned long len, int log)
 {
 
 	struct kvm_userspace_memory_region memory = {
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 58a6d4a..cff04c5 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -781,12 +781,13 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
     r = kvm_is_allocated_mem(kvm_context, start_addr, size);
     if (r)
         return;
-    r = kvm_is_intersecting_mem(kvm_context, start_addr);
-    if (r)
-        kvm_create_mem_hole(kvm_context, start_addr, size);
-    r = kvm_register_phys_mem(kvm_context, start_addr,
-                              phys_ram_base + phys_offset,
-                              size, 0);
+        r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    if (r) {
+        printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
+    } else
+        r = kvm_register_phys_mem(kvm_context, start_addr,
+                                  phys_ram_base + phys_offset,
+                                  size, 0);
     if (r < 0) {
         printf("kvm_cpu_register_physical_memory: failed\n");
         exit(1);
-- 
1.5.5.1

[PATCH 3/9] allow intersecting region to be on the boundary.

[Glauber Costa]

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index e768e44..fa65c30 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -130,8 +130,8 @@ int get_intersecting_slot(unsigned long phys_addr)
 	int i;
 
 	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i)
-		if (slots[i].len && slots[i].phys_addr < phys_addr &&
-		    (slots[i].phys_addr + slots[i].len) > phys_addr)
+		if (slots[i].len && slots[i].phys_addr <= phys_addr &&
+		    (slots[i].phys_addr + slots[i].len) >= phys_addr)
 			return i;
 	return -1;
 }
-- 
1.5.5.1

[PATCH 4/9] substitute is_allocated_mem with more general is_containing_region

[Glauber Costa]

is_allocated_mem is a function that checks if every relevant aspect of the memory slot
match (start and size). Replace it with a more generic function that checks if a memory
region is totally contained into another. The former case is also covered.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   34 +++++++++++++++++++++-------------
 libkvm/libkvm.h |    2 +-
 qemu/qemu-kvm.c |    4 ++--
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index fa65c30..a5e20bb 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -136,6 +136,27 @@ int get_intersecting_slot(unsigned long phys_addr)
 	return -1;
 }
 
+/* Returns -1 if this slot is not totally contained on any other,
+ * and the number of the slot otherwise */
+int get_container_slot(uint64_t phys_addr, unsigned long size)
+{
+	int i;
+
+	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i)
+		if (slots[i].len && slots[i].phys_addr <= phys_addr &&
+		    (slots[i].phys_addr + slots[i].len) >= phys_addr + size)
+			return i;
+	return -1;
+}
+
+int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size)
+{
+	int slot = get_container_slot(phys_addr, size);
+	if (slot == -1)
+		return 0;
+	return 1;
+}
+
 /* 
  * dirty pages logging control 
  */
@@ -423,19 +444,6 @@ int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start)
 	return get_intersecting_slot(phys_start) != -1;
 }
 
-int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
-			 unsigned long len)
-{
-	int slot;
-
-	slot = get_slot(phys_start);
-	if (slot == -1)
-		return 0;
-	if (slots[slot].len == len)
-		return 1;
-	return 0;
-}
-
 int kvm_register_phys_mem(kvm_context_t kvm,
 			  unsigned long phys_start, void *userspace_addr,
 			  unsigned long len, int log)
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 79dd769..1e89993 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -454,7 +454,7 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start,
 			  unsigned long len, int log, int writable);
 void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, 
 			  unsigned long len);
-int kvm_is_intersecting_mem(kvm_context_t kvm, unsigned long phys_start);
+int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size);
 int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 			 unsigned long len);
 int kvm_create_mem_hole(kvm_context_t kvm, unsigned long phys_start,
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index cff04c5..e0b114a 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -778,10 +778,10 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
     int r = 0;
 
     phys_offset &= ~IO_MEM_ROM;
-    r = kvm_is_allocated_mem(kvm_context, start_addr, size);
+    r = kvm_is_containing_region(kvm_context, start_addr, size);
     if (r)
         return;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    r = kvm_is_intersecting_mem(kvm_context, start_addr);
     if (r) {
         printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
     } else
-- 
1.5.5.1

[PATCH 5/9] move kvm_cpu_register_memory_area into qemu's

[Glauber Costa]

Turn the explicit calls to kvm_cpu_register_memoy_area()
an empty function. Provide a __kvm_cpu_register_memory_area()
that is called from within cpu_register_memory_area().
To avoid registering mmio regions to the hypervisor, since we depend on
them faulting, we keep track of what regions are mmio regions too.

This is to be bisection friendly. Direct calls are to be removed
in a later commit.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   84 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 libkvm/libkvm.h |    6 ++++
 qemu/exec.c     |    3 ++
 qemu/qemu-kvm.c |   28 +++++++++++++++---
 4 files changed, 112 insertions(+), 9 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index a5e20bb..5df201e 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -62,14 +62,22 @@ struct slot_info {
 	unsigned flags;
 };
 
+struct mmio_slot_info {
+    uint64_t phys_addr;
+    unsigned int len;
+};
+
 struct slot_info slots[KVM_MAX_NUM_MEM_REGIONS];
+struct mmio_slot_info mmio_slots[KVM_MAX_NUM_MEM_REGIONS];
 
 void init_slots(void)
 {
 	int i;
 
-	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
+	for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i) {
 		slots[i].len = 0;
+		mmio_slots[i].len = 0;
+	}
 }
 
 int get_free_slot(kvm_context_t kvm)
@@ -99,6 +107,16 @@ int get_free_slot(kvm_context_t kvm)
 	return -1;
 }
 
+int get_free_mmio_slot(kvm_context_t kvm)
+{
+
+       unsigned int i;
+       for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS; ++i)
+		if (!mmio_slots[i].len)
+			return i;
+       return -1;
+}
+
 void register_slot(int slot, unsigned long phys_addr, unsigned long len,
 		   unsigned long userspace_addr, unsigned flags)
 {
@@ -149,14 +167,47 @@ int get_container_slot(uint64_t phys_addr, unsigned long size)
 	return -1;
 }
 
+int get_container_mmio_slot(kvm_context_t kvm, uint64_t phys_addr, unsigned long size)
+{
+       int i;
+
+       for (i = 0; i < KVM_MAX_NUM_MEM_REGIONS ; ++i)
+               if (mmio_slots[i].len && mmio_slots[i].phys_addr <= phys_addr &&
+                   (mmio_slots[i].phys_addr + mmio_slots[i].len) >= phys_addr + size)
+                       return i;
+       return -1;
+}
+
+int kvm_register_mmio_slot(kvm_context_t kvm, uint64_t phys_addr, unsigned int size)
+{
+       int slot = get_free_mmio_slot(kvm);
+
+       if (slot == -1)
+               goto out;
+
+#ifdef DEBUG_MEMREG
+	printf("Registering mmio region %llx (%lx)\n", phys_addr, size);
+#endif
+       mmio_slots[slot].phys_addr = phys_addr;
+       mmio_slots[slot].len = size;
+out:
+       return slot;
+}
+
 int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_addr, unsigned long size)
 {
 	int slot = get_container_slot(phys_addr, size);
-	if (slot == -1)
-		return 0;
-	return 1;
+
+	if (slot != -1)
+		return 1;
+	slot = get_container_mmio_slot(kvm, phys_addr, size);
+	if (slot != -1)
+		return 1;
+
+	return 0;
 }
 
+
 /* 
  * dirty pages logging control 
  */
@@ -509,6 +560,31 @@ void kvm_destroy_phys_mem(kvm_context_t kvm, unsigned long phys_start,
 	free_slot(memory.slot);
 }
 
+void kvm_unregister_memory_area(kvm_context_t kvm, uint64_t phys_addr, unsigned long size)
+{
+
+       int slot = get_container_slot(phys_addr, size);
+
+       if (slot != -1) {
+#ifdef DEBUG_MEMREG
+               printf("Unregistering memory region %llx (%lx)\n", phys_addr, size);
+#endif
+               kvm_destroy_phys_mem(kvm, phys_addr, size);
+               return;
+       }
+
+       slot = get_container_mmio_slot(kvm, phys_addr, size);
+       if (slot != -1) {
+#ifdef DEBUG_MEMREG
+               printf("Unregistering mmio region %llx (%lx)\n", phys_addr, size);
+#endif
+               kvm_unregister_coalesced_mmio(kvm, phys_addr, size);
+               mmio_slots[slot].len = 0;
+       }
+
+       return;
+}
+
 static int kvm_get_map(kvm_context_t kvm, int ioctl_num, int slot, void *buf)
 {
 	int r;
diff --git a/libkvm/libkvm.h b/libkvm/libkvm.h
index 1e89993..f9cdb9c 100644
--- a/libkvm/libkvm.h
+++ b/libkvm/libkvm.h
@@ -454,6 +454,10 @@ void *kvm_create_phys_mem(kvm_context_t, unsigned long phys_start,
 			  unsigned long len, int log, int writable);
 void kvm_destroy_phys_mem(kvm_context_t, unsigned long phys_start, 
 			  unsigned long len);
+
+void kvm_unregister_memory_area(kvm_context_t, uint64_t phys_start,
+				 unsigned long len);
+
 int kvm_is_containing_region(kvm_context_t kvm, unsigned long phys_start, unsigned long size);
 int kvm_is_allocated_mem(kvm_context_t kvm, unsigned long phys_start,
 			 unsigned long len);
@@ -467,6 +471,8 @@ int kvm_get_dirty_pages_range(kvm_context_t kvm, unsigned long phys_addr,
 			      unsigned long end_addr, void *buf, void*opaque,
 			      int (*cb)(unsigned long start, unsigned long len,
 					void*bitmap, void *opaque));
+int kvm_register_mmio_slot(kvm_context_t kvm,
+				uint64_t addr, uint32_t size);
 int kvm_register_coalesced_mmio(kvm_context_t kvm,
 				uint64_t addr, uint32_t size);
 int kvm_unregister_coalesced_mmio(kvm_context_t kvm,
diff --git a/qemu/exec.c b/qemu/exec.c
index bf037f0..f0e84c8 100644
--- a/qemu/exec.c
+++ b/qemu/exec.c
@@ -2203,6 +2203,9 @@ void cpu_register_physical_memory(target_phys_addr_t start_addr,
         kqemu_set_phys_mem(start_addr, size, phys_offset);
     }
 #endif
+
+    __kvm_cpu_register_physical_memory(start_addr, size, phys_offset);
+
     size = (size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
     end_addr = start_addr + (target_phys_addr_t)size;
     for(addr = start_addr; addr != end_addr; addr += TARGET_PAGE_SIZE) {
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index e0b114a..444f79e 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -775,12 +775,34 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
                                       unsigned long phys_offset)
 {
-    int r = 0;
+}
 
+void __kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
+                                      unsigned long size,
+                                      unsigned long phys_offset)
+{
+    int r = 0;
+    unsigned long area_flags = phys_offset & ~TARGET_PAGE_MASK;
     phys_offset &= ~IO_MEM_ROM;
+
+
+    if (area_flags == IO_MEM_UNASSIGNED) {
+        kvm_unregister_memory_area(kvm_context, start_addr, size);
+        return;
+    }
+
     r = kvm_is_containing_region(kvm_context, start_addr, size);
     if (r)
         return;
+
+    if (area_flags >= TLB_MMIO) {
+        r = kvm_register_mmio_slot(kvm_context, start_addr, size);
+        if (r < 0) {
+            printf("No free mmio slots\n");
+            exit(1);
+        }
+        return;
+    }
     r = kvm_is_intersecting_mem(kvm_context, start_addr);
     if (r) {
         printf("Ignoring intersecting memory %llx (%lx)\n", start_addr, size);
@@ -788,10 +810,6 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
         r = kvm_register_phys_mem(kvm_context, start_addr,
                                   phys_ram_base + phys_offset,
                                   size, 0);
-    if (r < 0) {
-        printf("kvm_cpu_register_physical_memory: failed\n");
-        exit(1);
-    }
     return;
 }
 
-- 
1.5.5.1

[PATCH 6/9] cleanup kvm memory registration

[Glauber Costa]

Remove all references to kvm_cpu_register_physical_memory(),
since it is called from within cpu_register_physical_memory().

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/exec.c             |    2 +-
 qemu/hw/ipf.c           |    8 --------
 qemu/hw/pc.c            |   23 ++---------------------
 qemu/hw/ppc440_bamboo.c |    2 --
 qemu/qemu-kvm.c         |    6 ------
 5 files changed, 3 insertions(+), 38 deletions(-)

diff --git a/qemu/exec.c b/qemu/exec.c
index f0e84c8..c5e369e 100644
--- a/qemu/exec.c
+++ b/qemu/exec.c
@@ -2204,7 +2204,7 @@ void cpu_register_physical_memory(target_phys_addr_t start_addr,
     }
 #endif
 
-    __kvm_cpu_register_physical_memory(start_addr, size, phys_offset);
+    kvm_cpu_register_physical_memory(start_addr, size, phys_offset);
 
     size = (size + TARGET_PAGE_SIZE - 1) & TARGET_PAGE_MASK;
     end_addr = start_addr + (target_phys_addr_t)size;
diff --git a/qemu/hw/ipf.c b/qemu/hw/ipf.c
index d70af90..5227385 100644
--- a/qemu/hw/ipf.c
+++ b/qemu/hw/ipf.c
@@ -420,18 +420,14 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
     if (kvm_enabled()) {
 		ram_addr = qemu_ram_alloc(0xa0000);
 		cpu_register_physical_memory(0, 0xa0000, ram_addr);
-		kvm_cpu_register_physical_memory(0, 0xa0000, ram_addr);
 
 		ram_addr = qemu_ram_alloc(0x20000); // Workaround 0xa0000-0xc0000
 
 		ram_addr = qemu_ram_alloc(0x40000);
 		cpu_register_physical_memory(0xc0000, 0x40000, ram_addr);
-		kvm_cpu_register_physical_memory(0xc0000, 0x40000, ram_addr);
 
 		ram_addr = qemu_ram_alloc(ram_size - 0x100000);
 		cpu_register_physical_memory(0x100000, ram_size - 0x100000, ram_addr);
-		kvm_cpu_register_physical_memory(0x100000, ram_size - 0x100000,
-				ram_addr);
 	} else
     {
         ram_addr = qemu_ram_alloc(ram_size);
@@ -444,9 +440,6 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
 	if (above_4g_mem_size > 0) {
 		ram_addr = qemu_ram_alloc(above_4g_mem_size);
 		cpu_register_physical_memory(0x100000000, above_4g_mem_size, ram_addr);
-	if (kvm_enabled())
-		kvm_cpu_register_physical_memory(0x100000000, above_4g_mem_size,
-					ram_addr);
 	}
 
 	/*Load firware to its proper position.*/
@@ -468,7 +461,6 @@ static void ipf_init1(ram_addr_t ram_size, int vga_ram_size,
 		fw_image_start = fw_start + GFW_SIZE - image_size;
 
         cpu_register_physical_memory(GFW_START, GFW_SIZE, fw_offset);
-        kvm_cpu_register_physical_memory(GFW_START,GFW_SIZE, fw_offset);
         memcpy(fw_image_start, image, image_size);
 
 		free(image);
diff --git a/qemu/hw/pc.c b/qemu/hw/pc.c
index 8a50096..bc4585c 100644
--- a/qemu/hw/pc.c
+++ b/qemu/hw/pc.c
@@ -769,9 +769,7 @@ static int load_option_rom(const char *filename, int offset, int type)
     cpu_register_physical_memory(0xd0000 + offset,
 				 size, option_rom_offset | type);
     option_rom_setup_reset(0xd0000 + offset, size);
-    if (kvm_enabled())
-	    kvm_cpu_register_physical_memory(0xd0000 + offset,
-                                             size, option_rom_offset | type);
+
     return size;
 }
 
@@ -845,16 +843,13 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     if (kvm_enabled()) {
         ram_addr = qemu_ram_alloc(0xa0000);
         cpu_register_physical_memory(0, 0xa0000, ram_addr);
-        kvm_cpu_register_physical_memory(0, 0xa0000, ram_addr);
 
         ram_addr = qemu_ram_alloc(0x100000 - 0xa0000);   // hole
         ram_addr = qemu_ram_alloc(below_4g_mem_size - 0x100000);
         cpu_register_physical_memory(0x100000,
 				     below_4g_mem_size - 0x100000,
 				     ram_addr);
-        kvm_cpu_register_physical_memory(0x100000,
-					 below_4g_mem_size - 0x100000,
-                                         ram_addr);
+
         /* above 4giga memory allocation */
         if (above_4g_mem_size > 0) {
             ram_addr = qemu_ram_alloc(above_4g_mem_size);
@@ -870,9 +865,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
             cpu_register_physical_memory(0x100000000ULL,
                                          above_4g_mem_size,
                                          ram_addr);
-            kvm_cpu_register_physical_memory(0x100000000ULL,
-					     above_4g_mem_size,
-                                             ram_addr);
         }
     } else
     {
@@ -926,9 +918,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     /* setup basic memory access */
     cpu_register_physical_memory(0xc0000, 0x10000,
                                  vga_bios_offset | IO_MEM_ROM);
-    if (kvm_enabled())
-        kvm_cpu_register_physical_memory(0xc0000, 0x10000,
-                                         vga_bios_offset | IO_MEM_ROM);
 
     /* map the last 128KB of the BIOS in ISA space */
     isa_bios_size = bios_size;
@@ -940,10 +929,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     cpu_register_physical_memory(0x100000 - isa_bios_size,
                                  isa_bios_size,
                                  (bios_offset + bios_size - isa_bios_size) /* | IO_MEM_ROM */);
-    if (kvm_enabled())
-        kvm_cpu_register_physical_memory(0x100000 - isa_bios_size,
-                                         isa_bios_size,
-                                         (bios_offset + bios_size - isa_bios_size) | IO_MEM_ROM);
 
     /* XXX: for DDIM support, "ROM space" should be writable during
        initialization, and (optionally) marked readonly by the BIOS
@@ -962,10 +947,6 @@ static void pc_init1(ram_addr_t ram_size, int vga_ram_size,
     /* map all the bios at the top of memory */
     cpu_register_physical_memory((uint32_t)(-bios_size),
                                  bios_size, bios_offset | IO_MEM_ROM);
-    if (kvm_enabled()) {
-        kvm_cpu_register_physical_memory((uint32_t)(-bios_size),
-                                           bios_size, bios_offset | IO_MEM_ROM);
-    }
 
     bochs_bios_init();
 
diff --git a/qemu/hw/ppc440_bamboo.c b/qemu/hw/ppc440_bamboo.c
index 9ff6f7d..deafc5f 100644
--- a/qemu/hw/ppc440_bamboo.c
+++ b/qemu/hw/ppc440_bamboo.c
@@ -93,8 +93,6 @@ void bamboo_init(ram_addr_t ram_size, int vga_ram_size,
 
 	/* Register mem */
 	cpu_register_physical_memory(0, ram_size, 0);
-	if (kvm_enabled())
-	  kvm_cpu_register_physical_memory(0, ram_size, 0);
 
 	/* load kernel with uboot loader */
 	printf("%s: load kernel\n", __func__);
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 444f79e..949a35b 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -775,12 +775,6 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long size,
                                       unsigned long phys_offset)
 {
-}
-
-void __kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
-                                      unsigned long size,
-                                      unsigned long phys_offset)
-{
     int r = 0;
     unsigned long area_flags = phys_offset & ~TARGET_PAGE_MASK;
     phys_offset &= ~IO_MEM_ROM;
-- 
1.5.5.1

[PATCH 7/9] add debuging facilities to memory registration at libkvm

[Glauber Costa]

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 libkvm/libkvm.c |   16 +++++++++++++---
 1 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/libkvm/libkvm.c b/libkvm/libkvm.c
index 5df201e..8764982 100644
--- a/libkvm/libkvm.c
+++ b/libkvm/libkvm.c
@@ -52,6 +52,8 @@
 #include "kvm-s390.h"
 #endif
 
+//#define DEBUG_MEMREG
+
 int kvm_abi = EXPECTED_KVM_API_VERSION;
 int kvm_page_size;
 
@@ -186,7 +188,7 @@ int kvm_register_mmio_slot(kvm_context_t kvm, uint64_t phys_addr, unsigned int s
                goto out;
 
 #ifdef DEBUG_MEMREG
-	printf("Registering mmio region %llx (%lx)\n", phys_addr, size);
+	fprintf(stderr, "Registering mmio region %llx (%lx)\n", phys_addr, size);
 #endif
        mmio_slots[slot].phys_addr = phys_addr;
        mmio_slots[slot].len = size;
@@ -509,6 +511,11 @@ int kvm_register_phys_mem(kvm_context_t kvm,
 	int r;
 
 	memory.slot = get_free_slot(kvm);
+#ifdef DEBUG_MEMREG
+	fprintf(stderr, "%s, memory: gpa: %llx, size: %llx, uaddr: %llx, slot: %x, flags: %lx\n",
+		__func__, memory.guest_phys_addr, memory.memory_size,
+		memory.userspace_addr, memory.slot, memory.flags);
+#endif
 	r = ioctl(kvm->vm_fd, KVM_SET_USER_MEMORY_REGION, &memory);
 	if (r == -1) {
 		fprintf(stderr, "create_userspace_phys_mem: %s\n", strerror(errno));
@@ -567,7 +574,7 @@ void kvm_unregister_memory_area(kvm_context_t kvm, uint64_t phys_addr, unsigned
 
        if (slot != -1) {
 #ifdef DEBUG_MEMREG
-               printf("Unregistering memory region %llx (%lx)\n", phys_addr, size);
+		fprintf(stderr, "Unregistering memory region %llx (%lx)\n", phys_addr, size);
 #endif
                kvm_destroy_phys_mem(kvm, phys_addr, size);
                return;
@@ -576,7 +583,7 @@ void kvm_unregister_memory_area(kvm_context_t kvm, uint64_t phys_addr, unsigned
        slot = get_container_mmio_slot(kvm, phys_addr, size);
        if (slot != -1) {
 #ifdef DEBUG_MEMREG
-               printf("Unregistering mmio region %llx (%lx)\n", phys_addr, size);
+		fprintf(stderr, "Unregistering mmio region %llx (%lx)\n", phys_addr, size);
 #endif
                kvm_unregister_coalesced_mmio(kvm, phys_addr, size);
                mmio_slots[slot].len = 0;
@@ -1072,6 +1079,9 @@ int kvm_unregister_coalesced_mmio(kvm_context_t kvm, uint64_t addr, uint32_t siz
 			perror("kvm_unregister_coalesced_mmio_zone");
 			return -errno;
 		}
+#ifdef DEBUG_MEMREG
+		fprintf(stderr, "Unregistered coalesced mmio region for %llx (%lx)\n", addr, size);
+#endif
 		return 0;
 	}
 #endif
-- 
1.5.5.1

[PATCH 8/9] coalesce mmio regions without an explicit call

[Glauber Costa]

Try to coalesce mmio regions inside kvm_cpu_register_physical_memory().
Coalescing is done if area has TLB_MMIO flags set, or anything greater than that.

The original explicit function turns into an empty function. This is to be
bisection friendly. Direct calls are to be removed in a later commit.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/qemu-kvm.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 949a35b..106a3ee 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -795,6 +795,7 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
             printf("No free mmio slots\n");
             exit(1);
         }
+        kvm_register_coalesced_mmio(kvm_context, start_addr, size);
         return;
     }
     r = kvm_is_intersecting_mem(kvm_context, start_addr);
@@ -1037,11 +1038,9 @@ void kvm_mutex_lock(void)
 
 int qemu_kvm_register_coalesced_mmio(target_phys_addr_t addr, unsigned int size)
 {
-    return kvm_register_coalesced_mmio(kvm_context, addr, size);
 }
 
 int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
 				       unsigned int size)
 {
-    return kvm_unregister_coalesced_mmio(kvm_context, addr, size);
 }
-- 
1.5.5.1

[PATCH 9/9] remove explicit calls to kvm_qemu_register_coalesced_mmio

[Glauber Costa]

It is now done automatically for IO regions inside
kvm_cpu_register_physical_memory().
---
 qemu/hw/cirrus_vga.c |    2 --
 qemu/hw/e1000.c      |   12 ------------
 qemu/hw/pci.c        |    3 ---
 qemu/hw/vga.c        |    4 ----
 4 files changed, 0 insertions(+), 21 deletions(-)

diff --git a/qemu/hw/cirrus_vga.c b/qemu/hw/cirrus_vga.c
index 0cf5b24..5919732 100644
--- a/qemu/hw/cirrus_vga.c
+++ b/qemu/hw/cirrus_vga.c
@@ -3291,8 +3291,6 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci)
                                            cirrus_vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
                                  vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(isa_mem_base + 0x000a0000, 0x20000);
 
     s->sr[0x06] = 0x0f;
     if (device_id == CIRRUS_ID_CLGD5446) {
diff --git a/qemu/hw/e1000.c b/qemu/hw/e1000.c
index 5ae3960..2d97b34 100644
--- a/qemu/hw/e1000.c
+++ b/qemu/hw/e1000.c
@@ -942,18 +942,6 @@ e1000_mmio_map(PCIDevice *pci_dev, int region_num,
 
     d->mmio_base = addr;
     cpu_register_physical_memory(addr, PNPMMIO_SIZE, d->mmio_index);
-
-    if (kvm_enabled()) {
-	int i;
-        uint32_t excluded_regs[] = {
-            E1000_MDIC, E1000_ICR, E1000_ICS, E1000_IMS,
-            E1000_IMC, E1000_TCTL, E1000_TDT, PNPMMIO_SIZE
-        };
-        qemu_kvm_register_coalesced_mmio(addr, excluded_regs[0]);
-        for (i = 0; excluded_regs[i] != PNPMMIO_SIZE; i++)
-            qemu_kvm_register_coalesced_mmio(addr + excluded_regs[i] + 4,
-                             excluded_regs[i + 1] - excluded_regs[i] - 4);
-    }
 }
 
 static int
diff --git a/qemu/hw/pci.c b/qemu/hw/pci.c
index 07d37a8..2e4ec92 100644
--- a/qemu/hw/pci.c
+++ b/qemu/hw/pci.c
@@ -324,9 +324,6 @@ static void pci_update_mappings(PCIDevice *d)
                         cpu_register_physical_memory(pci_to_cpu_addr(r->addr),
                                                      r->size,
                                                      IO_MEM_UNASSIGNED);
-                        if (kvm_enabled())
-                                qemu_kvm_unregister_coalesced_mmio(r->addr,
-                                                                 r->size);
                     }
                 }
                 r->addr = new_addr;
diff --git a/qemu/hw/vga.c b/qemu/hw/vga.c
index 3a5dcbc..ba0dec4 100644
--- a/qemu/hw/vga.c
+++ b/qemu/hw/vga.c
@@ -2259,8 +2259,6 @@ void vga_init(VGAState *s)
     vga_io_memory = cpu_register_io_memory(0, vga_mem_read, vga_mem_write, s);
     cpu_register_physical_memory(isa_mem_base + 0x000a0000, 0x20000,
                                  vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(isa_mem_base + 0x000a0000, 0x20000);
 }
 
 /* Memory mapped interface */
@@ -2336,8 +2334,6 @@ static void vga_mm_init(VGAState *s, target_phys_addr_t vram_base,
     cpu_register_physical_memory(ctrl_base, 0x100000, s_ioport_ctrl);
     s->bank_offset = 0;
     cpu_register_physical_memory(vram_base + 0x000a0000, 0x20000, vga_io_memory);
-    if (kvm_enabled())
-        qemu_kvm_register_coalesced_mmio(vram_base + 0x000a0000, 0x20000);
 }
 
 int isa_vga_init(DisplayState *ds, uint8_t *vga_ram_base,
-- 
1.5.5.1

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Jan Kiszka]

Glauber Costa wrote:
> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
> need to take care of not passing its value as the phys_offset.

As you are turning things upside down already: :->

Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
how to handle memory remappings during runtime (like
i440fx_update_memory_mappings does)?

I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
note that - at least so far - cpu_register_physical_memory is sometimes
misused to change the protection or the origin of some memory region.
That should be taken into account. Or the qemu interface should be
refactored first so that kvm (or qemuaccel) can cleanly hook into
dedicated remapping/protection changing services.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > Actually, all registrations are the same. If IO_MEM_ROM is set, we only
> > need to take care of not passing its value as the phys_offset.
> 
> As you are turning things upside down already: :->
> 
> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
> how to handle memory remappings during runtime (like
> i440fx_update_memory_mappings does)?
> 
> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
> note that - at least so far - cpu_register_physical_memory is sometimes
> misused to change the protection or the origin of some memory region.
> That should be taken into account. Or the qemu interface should be
> refactored first so that kvm (or qemuaccel) can cleanly hook into
> dedicated remapping/protection changing services.

Right now, KVM does not seem to bother.
The registering of memory does not account for any kind of protection, and the
only flag we have is regarding logging being enabled or disabled (for that one,
I do see the problem you describe, but haven't dig deeply yet).

Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
the calling of qemu's version. So for what qemu itself is concerned, the protection
changes still happen: only kvm takes no action about it.

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Jan Kiszka]

Glauber Costa wrote:
> On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
>>> need to take care of not passing its value as the phys_offset.
>> As you are turning things upside down already: :->
>>
>> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
>> how to handle memory remappings during runtime (like
>> i440fx_update_memory_mappings does)?
>>
>> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
>> note that - at least so far - cpu_register_physical_memory is sometimes
>> misused to change the protection or the origin of some memory region.
>> That should be taken into account. Or the qemu interface should be
>> refactored first so that kvm (or qemuaccel) can cleanly hook into
>> dedicated remapping/protection changing services.
> 
> Right now, KVM does not seem to bother.
> The registering of memory does not account for any kind of protection, and the
> only flag we have is regarding logging being enabled or disabled (for that one,
> I do see the problem you describe, but haven't dig deeply yet).
> 
> Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
> the calling of qemu's version. So for what qemu itself is concerned, the protection
> changes still happen: only kvm takes no action about it.

Yes, lacking protection may not harm that much, more problematic can be
the inconsistencies memory remappings leave behind.

Jan

-- 
Siemens AG, Corporate Technology, CT SE 2
Corporate Competence Center Embedded Linux

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

On Fri, Sep 12, 2008 at 06:26:37PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
> >>> need to take care of not passing its value as the phys_offset.
> >> As you are turning things upside down already: :->
> >>
> >> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
> >> how to handle memory remappings during runtime (like
> >> i440fx_update_memory_mappings does)?
> >>
> >> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
> >> note that - at least so far - cpu_register_physical_memory is sometimes
> >> misused to change the protection or the origin of some memory region.
> >> That should be taken into account. Or the qemu interface should be
> >> refactored first so that kvm (or qemuaccel) can cleanly hook into
> >> dedicated remapping/protection changing services.
> > 
> > Right now, KVM does not seem to bother.
> > The registering of memory does not account for any kind of protection, and the
> > only flag we have is regarding logging being enabled or disabled (for that one,
> > I do see the problem you describe, but haven't dig deeply yet).
> > 
> > Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
> > the calling of qemu's version. So for what qemu itself is concerned, the protection
> > changes still happen: only kvm takes no action about it.
> 
> Yes, lacking protection may not harm that much, more problematic can be
> the inconsistencies memory remappings leave behind.
Which inconsistencies? Since all memory as viewed as the same by KVM, I fail to see
how they can become inconsistent.

> 
> Jan
> 
> -- 
> Siemens AG, Corporate Technology, CT SE 2
> Corporate Competence Center Embedded Linux

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2386 bytes --]

Glauber Costa wrote:
> On Fri, Sep 12, 2008 at 06:26:37PM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
>>>> Glauber Costa wrote:
>>>>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
>>>>> need to take care of not passing its value as the phys_offset.
>>>> As you are turning things upside down already: :->
>>>>
>>>> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
>>>> how to handle memory remappings during runtime (like
>>>> i440fx_update_memory_mappings does)?
>>>>
>>>> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
>>>> note that - at least so far - cpu_register_physical_memory is sometimes
>>>> misused to change the protection or the origin of some memory region.
>>>> That should be taken into account. Or the qemu interface should be
>>>> refactored first so that kvm (or qemuaccel) can cleanly hook into
>>>> dedicated remapping/protection changing services.
>>> Right now, KVM does not seem to bother.
>>> The registering of memory does not account for any kind of protection, and the
>>> only flag we have is regarding logging being enabled or disabled (for that one,
>>> I do see the problem you describe, but haven't dig deeply yet).
>>>
>>> Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
>>> the calling of qemu's version. So for what qemu itself is concerned, the protection
>>> changes still happen: only kvm takes no action about it.
>> Yes, lacking protection may not harm that much, more problematic can be
>> the inconsistencies memory remappings leave behind.
> Which inconsistencies? Since all memory as viewed as the same by KVM, I fail to see
> how they can become inconsistent.

I'm currently not aware of a practical use case where this bites, but if
the guest maps some memory from A to B, it may expect to find the
content of A under B as well. That is not the case so far as B remains B
from KVM's POV. At the same time, all QEMU memory access functions see B
as A (that caused trouble for debugging and memory sniffing monitor
services).

IMHO, this inconsistency is waiting to cause troubles in the future
again and should be resolved cleanly when hooking into QEMU's memory
management, even if we lived without it until now.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

On Sat, Sep 13, 2008 at 08:26:06AM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Fri, Sep 12, 2008 at 06:26:37PM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
> >>>>> need to take care of not passing its value as the phys_offset.
> >>>> As you are turning things upside down already: :->
> >>>>
> >>>> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
> >>>> how to handle memory remappings during runtime (like
> >>>> i440fx_update_memory_mappings does)?
> >>>>
> >>>> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
> >>>> note that - at least so far - cpu_register_physical_memory is sometimes
> >>>> misused to change the protection or the origin of some memory region.
> >>>> That should be taken into account. Or the qemu interface should be
> >>>> refactored first so that kvm (or qemuaccel) can cleanly hook into
> >>>> dedicated remapping/protection changing services.
> >>> Right now, KVM does not seem to bother.
> >>> The registering of memory does not account for any kind of protection, and the
> >>> only flag we have is regarding logging being enabled or disabled (for that one,
> >>> I do see the problem you describe, but haven't dig deeply yet).
> >>>
> >>> Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
> >>> the calling of qemu's version. So for what qemu itself is concerned, the protection
> >>> changes still happen: only kvm takes no action about it.
> >> Yes, lacking protection may not harm that much, more problematic can be
> >> the inconsistencies memory remappings leave behind.
> > Which inconsistencies? Since all memory as viewed as the same by KVM, I fail to see
> > how they can become inconsistent.
> 
> I'm currently not aware of a practical use case where this bites, but if
> the guest maps some memory from A to B, it may expect to find the
> content of A under B as well. That is not the case so far as B remains B
> from KVM's POV. At the same time, all QEMU memory access functions see B
> as A (that caused trouble for debugging and memory sniffing monitor
> services).
It looks like KVM aliasing support, that (up to now), seemed completely orthogonal.
I'm looking at ways to integrate aliasing now, so if you can provide me with some use
cases of what you described above, (that seem to have happened in your debugging patches),
it would surely help.

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 3458 bytes --]

Glauber Costa wrote:
> On Sat, Sep 13, 2008 at 08:26:06AM +0200, Jan Kiszka wrote:
>> Glauber Costa wrote:
>>> On Fri, Sep 12, 2008 at 06:26:37PM +0200, Jan Kiszka wrote:
>>>> Glauber Costa wrote:
>>>>> On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
>>>>>> Glauber Costa wrote:
>>>>>>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
>>>>>>> need to take care of not passing its value as the phys_offset.
>>>>>> As you are turning things upside down already: :->
>>>>>>
>>>>>> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
>>>>>> how to handle memory remappings during runtime (like
>>>>>> i440fx_update_memory_mappings does)?
>>>>>>
>>>>>> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
>>>>>> note that - at least so far - cpu_register_physical_memory is sometimes
>>>>>> misused to change the protection or the origin of some memory region.
>>>>>> That should be taken into account. Or the qemu interface should be
>>>>>> refactored first so that kvm (or qemuaccel) can cleanly hook into
>>>>>> dedicated remapping/protection changing services.
>>>>> Right now, KVM does not seem to bother.
>>>>> The registering of memory does not account for any kind of protection, and the
>>>>> only flag we have is regarding logging being enabled or disabled (for that one,
>>>>> I do see the problem you describe, but haven't dig deeply yet).
>>>>>
>>>>> Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
>>>>> the calling of qemu's version. So for what qemu itself is concerned, the protection
>>>>> changes still happen: only kvm takes no action about it.
>>>> Yes, lacking protection may not harm that much, more problematic can be
>>>> the inconsistencies memory remappings leave behind.
>>> Which inconsistencies? Since all memory as viewed as the same by KVM, I fail to see
>>> how they can become inconsistent.
>> I'm currently not aware of a practical use case where this bites, but if
>> the guest maps some memory from A to B, it may expect to find the
>> content of A under B as well. That is not the case so far as B remains B
>> from KVM's POV. At the same time, all QEMU memory access functions see B
>> as A (that caused trouble for debugging and memory sniffing monitor
>> services).
> It looks like KVM aliasing support, that (up to now), seemed completely orthogonal.
> I'm looking at ways to integrate aliasing now, so if you can provide me with some use
> cases of what you described above, (that seem to have happened in your debugging patches),
> it would surely help.

The use case that currently exists is independent of my series:
rombios32.c remaps the BIOS code at 0x0f0000..0x100000 to RAM and plays
with its access permissions (bios_shadow_init, bios_lock_shadow_ram) via
the i440fx chipset (qemu/hw/piix_pci.c: i440fx_update_memory_mappings).
Due to my workaround [1] the emulation ignores this request for now.

Before that kvm continued to use the BIOS region after the remapping
while qemu started to look at the RAM. You were able to see this e.g.
via the monitor command 'xp /8xb 0xffff0'. This service and also the
gdbstub (that's how I found it) look at the memory via
cpu_physical_memory_rw, ie. with the help of qemu, while kvm-hosted code
uses kvm's mappings, of course.

Jan

[1] http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/21560



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

On Mon, Sep 15, 2008 at 03:08:01PM +0200, Jan Kiszka wrote:
> Glauber Costa wrote:
> > On Sat, Sep 13, 2008 at 08:26:06AM +0200, Jan Kiszka wrote:
> >> Glauber Costa wrote:
> >>> On Fri, Sep 12, 2008 at 06:26:37PM +0200, Jan Kiszka wrote:
> >>>> Glauber Costa wrote:
> >>>>> On Fri, Sep 12, 2008 at 05:47:40PM +0200, Jan Kiszka wrote:
> >>>>>> Glauber Costa wrote:
> >>>>>>> Actually, all registrations are the same. If IO_MEM_ROM is set, we only
> >>>>>>> need to take care of not passing its value as the phys_offset.
> >>>>>> As you are turning things upside down already: :->
> >>>>>>
> >>>>>> Any idea how to deal with that "real-only" property of IO_MEM_ROM? And
> >>>>>> how to handle memory remappings during runtime (like
> >>>>>> i440fx_update_memory_mappings does)?
> >>>>>>
> >>>>>> I like the hook-approach for kvm_cpu_register_physical_memory a lot. But
> >>>>>> note that - at least so far - cpu_register_physical_memory is sometimes
> >>>>>> misused to change the protection or the origin of some memory region.
> >>>>>> That should be taken into account. Or the qemu interface should be
> >>>>>> refactored first so that kvm (or qemuaccel) can cleanly hook into
> >>>>>> dedicated remapping/protection changing services.
> >>>>> Right now, KVM does not seem to bother.
> >>>>> The registering of memory does not account for any kind of protection, and the
> >>>>> only flag we have is regarding logging being enabled or disabled (for that one,
> >>>>> I do see the problem you describe, but haven't dig deeply yet).
> >>>>>
> >>>>> Calling of kvm_cpu_register_physical_what_a_big_name_memory() does not exclude
> >>>>> the calling of qemu's version. So for what qemu itself is concerned, the protection
> >>>>> changes still happen: only kvm takes no action about it.
> >>>> Yes, lacking protection may not harm that much, more problematic can be
> >>>> the inconsistencies memory remappings leave behind.
> >>> Which inconsistencies? Since all memory as viewed as the same by KVM, I fail to see
> >>> how they can become inconsistent.
> >> I'm currently not aware of a practical use case where this bites, but if
> >> the guest maps some memory from A to B, it may expect to find the
> >> content of A under B as well. That is not the case so far as B remains B
> >> from KVM's POV. At the same time, all QEMU memory access functions see B
> >> as A (that caused trouble for debugging and memory sniffing monitor
> >> services).
> > It looks like KVM aliasing support, that (up to now), seemed completely orthogonal.
> > I'm looking at ways to integrate aliasing now, so if you can provide me with some use
> > cases of what you described above, (that seem to have happened in your debugging patches),
> > it would surely help.
> 
> The use case that currently exists is independent of my series:
> rombios32.c remaps the BIOS code at 0x0f0000..0x100000 to RAM and plays
> with its access permissions (bios_shadow_init, bios_lock_shadow_ram) via
> the i440fx chipset (qemu/hw/piix_pci.c: i440fx_update_memory_mappings).
> Due to my workaround [1] the emulation ignores this request for now.
> 
> Before that kvm continued to use the BIOS region after the remapping
> while qemu started to look at the RAM. You were able to see this e.g.
> via the monitor command 'xp /8xb 0xffff0'. This service and also the
> gdbstub (that's how I found it) look at the memory via
> cpu_physical_memory_rw, ie. with the help of qemu, while kvm-hosted code
> uses kvm's mappings, of course.
Awesome

Will take a look at it today.

[PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Glauber Costa]

Actually, all registrations are the same. If IO_MEM_ROM is set, we only
need to take care of not passing its value as the phys_offset.

Signed-off-by: Glauber Costa <glommer@redhat.com>
---
 qemu/qemu-kvm.c |   31 +++++++++++--------------------
 1 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index c522a28..58a6d4a 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -776,26 +776,17 @@ void kvm_cpu_register_physical_memory(target_phys_addr_t start_addr,
                                       unsigned long phys_offset)
 {
     int r = 0;
-    if (!(phys_offset & ~TARGET_PAGE_MASK)) {
-        r = kvm_is_allocated_mem(kvm_context, start_addr, size);
-        if (r)
-            return;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
-    if (phys_offset & IO_MEM_ROM) {
-        phys_offset &= ~IO_MEM_ROM;
-        r = kvm_is_intersecting_mem(kvm_context, start_addr);
-        if (r)
-            kvm_create_mem_hole(kvm_context, start_addr, size);
-        r = kvm_register_phys_mem(kvm_context, start_addr,
-                                            phys_ram_base + phys_offset,
-                                            size, 0);
-    }
+
+    phys_offset &= ~IO_MEM_ROM;
+    r = kvm_is_allocated_mem(kvm_context, start_addr, size);
+    if (r)
+        return;
+    r = kvm_is_intersecting_mem(kvm_context, start_addr);
+    if (r)
+        kvm_create_mem_hole(kvm_context, start_addr, size);
+    r = kvm_register_phys_mem(kvm_context, start_addr,
+                              phys_ram_base + phys_offset,
+                              size, 0);
     if (r < 0) {
         printf("kvm_cpu_register_physical_memory: failed\n");
         exit(1);
-- 
1.5.5.1

Re: [PATCH 1/9] Don't separate registrations with IO_MEM_ROM set

[Avi Kivity]

Glauber Costa wrote:
>>
>> I'm currently not aware of a practical use case where this bites, but if
>> the guest maps some memory from A to B, it may expect to find the
>> content of A under B as well. That is not the case so far as B remains B
>> from KVM's POV. At the same time, all QEMU memory access functions see B
>> as A (that caused trouble for debugging and memory sniffing monitor
>> services).
>>     
> It looks like KVM aliasing support, that (up to now), seemed completely orthogonal.
> I'm looking at ways to integrate aliasing now, so if you can provide me with some use
> cases of what you described above, (that seem to have happened in your debugging patches),
> it would surely help.
>
>   

Aliasing/remapping can now be implemented using memory slots.  Simply 
map the same hva to different gpas.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.