Skip to contentSkip to conversation

qemu: memory notifiers

PATCH Superseded Michael S. Tsirkin 2 participants 13 messages Jan 5, 2010 – Jan 18, 2010 1 message not in the archive

lore
Share & tools
lore
#1 lore raw

PATCH v2 1/3 qemu: memory notifiers

In reply to a message not in the archive

This adds notifiers for phys memory changes: a set of callbacks that vhost can register and update kernel accordingly. Down the road, kvm code can be switched to use these as well, instead of calling kvm code directly from exec.c as is done now.

---
 cpu-common.h |   19 ++++++++++
 exec.c       |  111 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 127 insertions(+), 3 deletions(-)
cpu-common.h +19 -0
index 6302372..0ec9b72 100644
@@ -8,6 +8,7 @@
 #endif
 
 #include "bswap.h"
+#include "qemu-queue.h"
 
 /* address in the RAM (different from a physical address) */
 typedef unsigned long ram_addr_t;
@@ -61,6 +62,24 @@ void cpu_physical_memory_unmap(void *buffer, target_phys_addr_t len,
 void *cpu_register_map_client(void *opaque, void (*callback)(void *opaque));
 void cpu_unregister_map_client(void *cookie);
 
+struct CPUPhysMemoryClient;
+typedef struct CPUPhysMemoryClient CPUPhysMemoryClient;
+struct CPUPhysMemoryClient {
+    void (*set_memory)(struct CPUPhysMemoryClient *client,
+                       target_phys_addr_t start_addr,
+                       ram_addr_t size,
+                       ram_addr_t phys_offset);
+    int (*sync_dirty_bitmap)(struct CPUPhysMemoryClient *client,
+                             target_phys_addr_t start_addr,
+                             target_phys_addr_t end_addr);
+    int (*migration_log)(struct CPUPhysMemoryClient *client,
+                         int enable);
+    QLIST_ENTRY(CPUPhysMemoryClient) list;
+};
+
+void cpu_register_phys_memory_client(CPUPhysMemoryClient *);
+void cpu_unregister_phys_memory_client(CPUPhysMemoryClient *);
+
 uint32_t ldub_phys(target_phys_addr_t addr);
 uint32_t lduw_phys(target_phys_addr_t addr);
 uint32_t ldl_phys(target_phys_addr_t addr);
exec.c +108 -3

139 lines not on this page · raw message

Signature · 1 line
1.6.6.rc1.43.gf55cc
#13 lore raw

PATCH v2 3/3 kvm: move kvm to use memory notifiers

In reply to a message not in the archive

remove direct kvm calls from exec.c, make
kvm use memory notifiers framework instead.

---
 exec.c    |   17 +----------------
 kvm-all.c |   40 ++++++++++++++++++++++++++++++++++------
 kvm.h     |    8 --------
 3 files changed, 35 insertions(+), 30 deletions(-)
exec.c +1 -16
index 56c347b..8a1c08e 100644
@@ -1977,12 +1977,6 @@ int cpu_physical_memory_set_dirty_tracking(int enable)
 {
     int ret = 0;
     in_migration = enable;
-    if (kvm_enabled()) {
-        ret = kvm_set_migration_log(enable);
-    }
-    if (ret < 0) {
-        return ret;
-    }
     ret = cpu_notify_migration_log(!!enable);
     return ret;
 }
@@ -1995,14 +1989,8 @@ int cpu_physical_memory_get_dirty_tracking(void)
 int cpu_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
                                    target_phys_addr_t end_addr)
 {
-    int ret = 0;
+    int ret;
 
-    if (kvm_enabled()) {
-        ret = kvm_physical_sync_dirty_bitmap(start_addr, end_addr);
-    }
-    if (ret < 0) {
-        return ret;
-    }
     ret = cpu_notify_sync_dirty_bitmap(start_addr, end_addr);
     return ret;
 }
@@ -2415,9 +2403,6 @@ void cpu_register_physical_memory_offset(target_phys_addr_t start_addr,
     ram_addr_t orig_size = size;
     void *subpage;
 
-    if (kvm_enabled())
-        kvm_set_phys_mem(start_addr, size, phys_offset);
-
     cpu_notify_set_memory(start_addr, size, phys_offset);
 
     if (phys_offset == IO_MEM_UNASSIGNED) {
kvm-all.c +34 -6
index 4efb653..a312654 100644
@@ -257,7 +257,7 @@ int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
                                           KVM_MEM_LOG_DIRTY_PAGES);
 }
 
-int kvm_set_migration_log(int enable)
+static int kvm_set_migration_log(int enable)
 {
     KVMState *s = kvm_state;
     KVMSlot *mem;
@@ -292,8 +292,8 @@ static int test_le_bit(unsigned long nr, unsigned char *addr)
  * @start_add: start of logged region.
  * @end_addr: end of logged region.
  */
-int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
-                                   target_phys_addr_t end_addr)
+static int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
+					  target_phys_addr_t end_addr)
 {
     KVMState *s = kvm_state;
     unsigned long size, allocated_size = 0;
@@ -394,9 +394,9 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
     return ret;
 }
 
-void kvm_set_phys_mem(target_phys_addr_t start_addr,
-		       ram_addr_t size,
-		       ram_addr_t phys_offset)
+static void kvm_set_phys_mem(target_phys_addr_t start_addr,
+			     ram_addr_t size,
+			     ram_addr_t phys_offset)
 {
     KVMState *s = kvm_state;
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
@@ -532,6 +532,33 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     }
 }
 
+static void kvm_client_set_memory(struct CPUPhysMemoryClient *client,
+				  target_phys_addr_t start_addr,
+				  ram_addr_t size,
+				  ram_addr_t phys_offset)
+{
+	kvm_set_phys_mem(start_addr, size, phys_offset);
+}
+
+static int kvm_client_sync_dirty_bitmap(struct CPUPhysMemoryClient *client,
+					target_phys_addr_t start_addr,
+					target_phys_addr_t end_addr)
+{
+	return kvm_physical_sync_dirty_bitmap(start_addr, end_addr);
+}
+
+static int kvm_client_migration_log(struct CPUPhysMemoryClient *client,
+				    int enable)
+{
+	return kvm_set_migration_log(enable);
+}
+
+static CPUPhysMemoryClient kvm_cpu_phys_memory_client = {
+	.set_memory = kvm_client_set_memory,
+	.sync_dirty_bitmap = kvm_client_sync_dirty_bitmap,
+	.migration_log = kvm_client_migration_log,
+};
+
 int kvm_init(int smp_cpus)
 {
     static const char upgrade_note[] =
@@ -628,6 +655,7 @@ int kvm_init(int smp_cpus)
         goto err;
 
     kvm_state = s;
+    cpu_register_phys_memory_client(&kvm_cpu_phys_memory_client);
 
     return 0;
 
kvm.h +0 -8
index 1c93ac5..672d511 100644
@@ -35,16 +35,8 @@ int kvm_init_vcpu(CPUState *env);
 
 int kvm_cpu_exec(CPUState *env);
 
-void kvm_set_phys_mem(target_phys_addr_t start_addr,
-                      ram_addr_t size,
-                      ram_addr_t phys_offset);
-
-int kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
-                                   target_phys_addr_t end_addr);
-
 int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size);
 int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size);
-int kvm_set_migration_log(int enable);
 
 int kvm_has_sync_mmu(void);
 int kvm_has_vcpu_events(void);
Signature · 1 line
1.6.6.rc1.43.gf55cc
#12 lore raw

PATCH v2 2/3 kvm: move kvm_set_phys_mem around

In reply to a message not in the archive

move kvm_set_phys_mem so that it will
be later available earlier in the file.
needed for next patch using memory notifiers.

---
 kvm-all.c |  276 ++++++++++++++++++++++++++++++------------------------------
 1 files changed, 138 insertions(+), 138 deletions(-)
kvm-all.c +138 -138
index 15ec38e..4efb653 100644
@@ -394,6 +394,144 @@ int kvm_check_extension(KVMState *s, unsigned int extension)
     return ret;
 }
 
+void kvm_set_phys_mem(target_phys_addr_t start_addr,
+		       ram_addr_t size,
+		       ram_addr_t phys_offset)
+{
+    KVMState *s = kvm_state;
+    ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
+    KVMSlot *mem, old;
+    int err;
+
+    if (start_addr & ~TARGET_PAGE_MASK) {
+        if (flags >= IO_MEM_UNASSIGNED) {
+            if (!kvm_lookup_overlapping_slot(s, start_addr,
+                                             start_addr + size)) {
+                return;
+            }
+            fprintf(stderr, "Unaligned split of a KVM memory slot\n");
+        } else {
+            fprintf(stderr, "Only page-aligned memory slots supported\n");
+        }
+        abort();
+    }
+
+    /* KVM does not support read-only slots */
+    phys_offset &= ~IO_MEM_ROM;
+
+    while (1) {
+        mem = kvm_lookup_overlapping_slot(s, start_addr, start_addr + size);
+        if (!mem) {
+            break;
+        }
+
+        if (flags < IO_MEM_UNASSIGNED && start_addr >= mem->start_addr &&
+            (start_addr + size <= mem->start_addr + mem->memory_size) &&
+            (phys_offset - start_addr == mem->phys_offset - mem->start_addr)) {
+            /* The new slot fits into the existing one and comes with
+             * identical parameters - nothing to be done. */
+            return;
+        }
+
+        old = *mem;
+
+        /* unregister the overlapping slot */
+        mem->memory_size = 0;
+        err = kvm_set_user_memory_region(s, mem);
+        if (err) {
+            fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",
+                    __func__, strerror(-err));
+            abort();
+        }
+
+        /* Workaround for older KVM versions: we can't join slots, even not by
+         * unregistering the previous ones and then registering the larger
+         * slot. We have to maintain the existing fragmentation. Sigh.
+         *
+         * This workaround assumes that the new slot starts at the same
+         * address as the first existing one. If not or if some overlapping
+         * slot comes around later, we will fail (not seen in practice so far)
+         * - and actually require a recent KVM version. */
+        if (s->broken_set_mem_region &&
+            old.start_addr == start_addr && old.memory_size < size &&
+            flags < IO_MEM_UNASSIGNED) {
+            mem = kvm_alloc_slot(s);
+            mem->memory_size = old.memory_size;
+            mem->start_addr = old.start_addr;
+            mem->phys_offset = old.phys_offset;
+            mem->flags = 0;
+
+            err = kvm_set_user_memory_region(s, mem);
+            if (err) {
+                fprintf(stderr, "%s: error updating slot: %s\n", __func__,
+                        strerror(-err));
+                abort();
+            }
+
+            start_addr += old.memory_size;
+            phys_offset += old.memory_size;
+            size -= old.memory_size;
+            continue;
+        }
+
+        /* register prefix slot */
+        if (old.start_addr < start_addr) {
+            mem = kvm_alloc_slot(s);
+            mem->memory_size = start_addr - old.start_addr;
+            mem->start_addr = old.start_addr;
+            mem->phys_offset = old.phys_offset;
+            mem->flags = 0;
+
+            err = kvm_set_user_memory_region(s, mem);
+            if (err) {
+                fprintf(stderr, "%s: error registering prefix slot: %s\n",
+                        __func__, strerror(-err));
+                abort();
+            }
+        }
+
+        /* register suffix slot */
+        if (old.start_addr + old.memory_size > start_addr + size) {
+            ram_addr_t size_delta;
+
+            mem = kvm_alloc_slot(s);
+            mem->start_addr = start_addr + size;
+            size_delta = mem->start_addr - old.start_addr;
+            mem->memory_size = old.memory_size - size_delta;
+            mem->phys_offset = old.phys_offset + size_delta;
+            mem->flags = 0;
+
+            err = kvm_set_user_memory_region(s, mem);
+            if (err) {
+                fprintf(stderr, "%s: error registering suffix slot: %s\n",
+                        __func__, strerror(-err));
+                abort();
+            }
+        }
+    }
+
+    /* in case the KVM bug workaround already "consumed" the new slot */
+    if (!size)
+        return;
+
+    /* KVM does not need to know about this memory */
+    if (flags >= IO_MEM_UNASSIGNED)
+        return;
+
+    mem = kvm_alloc_slot(s);
+    mem->memory_size = size;
+    mem->start_addr = start_addr;
+    mem->phys_offset = phys_offset;
+    mem->flags = 0;
+
+    err = kvm_set_user_memory_region(s, mem);
+    if (err) {
+        fprintf(stderr, "%s: error registering slot: %s\n", __func__,
+                strerror(-err));
+        abort();
+    }
+}
+
 int kvm_init(int smp_cpus)
 {
     static const char upgrade_note[] =
@@ -674,144 +812,6 @@ int kvm_cpu_exec(CPUState *env)
     return ret;
 }
 
-void kvm_set_phys_mem(target_phys_addr_t start_addr,
-                      ram_addr_t size,
-                      ram_addr_t phys_offset)
-{
-    KVMState *s = kvm_state;
-    ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
-    KVMSlot *mem, old;
-    int err;
-
-    if (start_addr & ~TARGET_PAGE_MASK) {
-        if (flags >= IO_MEM_UNASSIGNED) {
-            if (!kvm_lookup_overlapping_slot(s, start_addr,
-                                             start_addr + size)) {
-                return;
-            }
-            fprintf(stderr, "Unaligned split of a KVM memory slot\n");
-        } else {
-            fprintf(stderr, "Only page-aligned memory slots supported\n");
-        }
-        abort();
-    }
-
-    /* KVM does not support read-only slots */
-    phys_offset &= ~IO_MEM_ROM;
-
-    while (1) {
-        mem = kvm_lookup_overlapping_slot(s, start_addr, start_addr + size);
-        if (!mem) {
-            break;
-        }
-
-        if (flags < IO_MEM_UNASSIGNED && start_addr >= mem->start_addr &&
-            (start_addr + size <= mem->start_addr + mem->memory_size) &&
-            (phys_offset - start_addr == mem->phys_offset - mem->start_addr)) {
-            /* The new slot fits into the existing one and comes with
-             * identical parameters - nothing to be done. */
-            return;
-        }
-
-        old = *mem;
-
-        /* unregister the overlapping slot */
-        mem->memory_size = 0;
-        err = kvm_set_user_memory_region(s, mem);
-        if (err) {
-            fprintf(stderr, "%s: error unregistering overlapping slot: %s\n",
-                    __func__, strerror(-err));
-            abort();
-        }
-
-        /* Workaround for older KVM versions: we can't join slots, even not by
-         * unregistering the previous ones and then registering the larger
-         * slot. We have to maintain the existing fragmentation. Sigh.
-         *
-         * This workaround assumes that the new slot starts at the same
-         * address as the first existing one. If not or if some overlapping
-         * slot comes around later, we will fail (not seen in practice so far)
-         * - and actually require a recent KVM version. */
-        if (s->broken_set_mem_region &&
-            old.start_addr == start_addr && old.memory_size < size &&
-            flags < IO_MEM_UNASSIGNED) {
-            mem = kvm_alloc_slot(s);
-            mem->memory_size = old.memory_size;
-            mem->start_addr = old.start_addr;
-            mem->phys_offset = old.phys_offset;
-            mem->flags = 0;
-
-            err = kvm_set_user_memory_region(s, mem);
-            if (err) {
-                fprintf(stderr, "%s: error updating slot: %s\n", __func__,
-                        strerror(-err));
-                abort();
-            }
-
-            start_addr += old.memory_size;
-            phys_offset += old.memory_size;
-            size -= old.memory_size;
-            continue;
-        }
-
-        /* register prefix slot */
-        if (old.start_addr < start_addr) {
-            mem = kvm_alloc_slot(s);
-            mem->memory_size = start_addr - old.start_addr;
-            mem->start_addr = old.start_addr;
-            mem->phys_offset = old.phys_offset;
-            mem->flags = 0;
-
-            err = kvm_set_user_memory_region(s, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering prefix slot: %s\n",
-                        __func__, strerror(-err));
-                abort();
-            }
-        }
-
-        /* register suffix slot */
-        if (old.start_addr + old.memory_size > start_addr + size) {
-            ram_addr_t size_delta;
-
-            mem = kvm_alloc_slot(s);
-            mem->start_addr = start_addr + size;
-            size_delta = mem->start_addr - old.start_addr;
-            mem->memory_size = old.memory_size - size_delta;
-            mem->phys_offset = old.phys_offset + size_delta;
-            mem->flags = 0;
-
-            err = kvm_set_user_memory_region(s, mem);
-            if (err) {
-                fprintf(stderr, "%s: error registering suffix slot: %s\n",
-                        __func__, strerror(-err));
-                abort();
-            }
-        }
-    }
-
-    /* in case the KVM bug workaround already "consumed" the new slot */
-    if (!size)
-        return;
-
-    /* KVM does not need to know about this memory */
-    if (flags >= IO_MEM_UNASSIGNED)
-        return;
-
-    mem = kvm_alloc_slot(s);
-    mem->memory_size = size;
-    mem->start_addr = start_addr;
-    mem->phys_offset = phys_offset;
-    mem->flags = 0;
-
-    err = kvm_set_user_memory_region(s, mem);
-    if (err) {
-        fprintf(stderr, "%s: error registering slot: %s\n", __func__,
-                strerror(-err));
-        abort();
-    }
-}
-
 int kvm_ioctl(KVMState *s, int type, ...)
 {
     int ret;
Signature · 1 line
1.6.6.rc1.43.gf55cc
#2 lore raw
In reply to Michael S. Tsirkin
Quoted 43 lines · Michael S. Tsirkin

This adds notifiers for phys memory changes: a set of callbacks that vhost can register and update kernel accordingly. Down the road, kvm code can be switched to use these as well, instead of calling kvm code directly from exec.c as is done now.

exec.c +36 -0
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+                                         CPUPhysMemoryClient *client)
+{
+    PhysPageDesc *pd;
+    int l1, l2;
+
+    for (l1 = 0; l1<  L1_SIZE; ++l1) {
+        pd = phys_map[l1];
+        if (!pd) {
+            continue;
+        }
+        for (l2 = 0; l2<  L2_SIZE; ++l2) {
+            if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+                continue;
+            }
+            client->set_memory(client, pd[l2].region_offset,
+                               TARGET_PAGE_SIZE, pd[l2].phys_offset);
+        }
+    }
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS>  32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS>  (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+    void **phys_map = (void **)l1_phys_map;
+    int l1;
+    if (!l1_phys_map) {
+        return;
+    }
+    for (l1 = 0; l1<  L1_SIZE; ++l1) {
+        if (phys_map[l1]) {

exec.c const CPULogItem cpu_log_items[] = { · line 1706

Quoted 10 lines
exec.c +10 -0
+            phys_page_for_each_in_l1_map(phys_map[l1], client);
+        }
+    }
+#else
+    if (!l1_phys_map) {
+        return;
+    }
+    phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}

This looks pretty frightening. What is it needed for?

I think we should stick with range operations, but maybe I misunderstood something here.

Otherwise, I like this patchset.

Signature · 1 line
error compiling committee.c: too many arguments to function
#3 lore raw
Quoted 48 lines · Avi Kivity
Quoted 47 lines · Michael S. Tsirkin

This adds notifiers for phys memory changes: a set of callbacks that vhost can register and update kernel accordingly. Down the road, kvm code can be switched to use these as well, instead of calling kvm code directly from exec.c as is done now.

exec.c +41 -0
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+                                         CPUPhysMemoryClient *client)
+{
+    PhysPageDesc *pd;
+    int l1, l2;
+
+    for (l1 = 0; l1<  L1_SIZE; ++l1) {
+        pd = phys_map[l1];
+        if (!pd) {
+            continue;
+        }
+        for (l2 = 0; l2<  L2_SIZE; ++l2) {
+            if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+                continue;
+            }
+            client->set_memory(client, pd[l2].region_offset,
+                               TARGET_PAGE_SIZE, pd[l2].phys_offset);
+        }
+    }
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS>  32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS>  (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+    void **phys_map = (void **)l1_phys_map;
+    int l1;
+    if (!l1_phys_map) {
+        return;
+    }
+    for (l1 = 0; l1<  L1_SIZE; ++l1) {
+        if (phys_map[l1]) {
+            phys_page_for_each_in_l1_map(phys_map[l1], client);
+        }
+    }
+#else
+    if (!l1_phys_map) {
Quoted 8 lines

exec.c const CPULogItem cpu_log_items[] = { · line 1706

Quoted 6 lines
exec.c +5 -0
+        return;
+    }
+    phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}

This looks pretty frightening. What is it needed for?

The point is that clients can be registered at any point.

A client that registered when memory is present needs to be notified about it.

Quoted 7 lines

I think we should stick with range operations, but maybe I misunderstood something here.
Otherwise, I like this patchset.

Signature · 1 line
error compiling committee.c: too many arguments to function
#4 lore raw
Quoted 60 lines · Michael S. Tsirkin
Quoted 49 lines · Avi Kivity
Quoted 47 lines · Michael S. Tsirkin

This adds notifiers for phys memory changes: a set of callbacks that vhost can register and update kernel accordingly. Down the road, kvm code can be switched to use these as well, instead of calling kvm code directly from exec.c as is done now.

exec.c +41 -0
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+                                         CPUPhysMemoryClient *client)
+{
+    PhysPageDesc *pd;
+    int l1, l2;
+
+    for (l1 = 0; l1<   L1_SIZE; ++l1) {
+        pd = phys_map[l1];
+        if (!pd) {
+            continue;
+        }
+        for (l2 = 0; l2<   L2_SIZE; ++l2) {
+            if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+                continue;
+            }
+            client->set_memory(client, pd[l2].region_offset,
+                               TARGET_PAGE_SIZE, pd[l2].phys_offset);
+        }
+    }
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS>   32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS>   (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+    void **phys_map = (void **)l1_phys_map;
+    int l1;
+    if (!l1_phys_map) {
+        return;
+    }
+    for (l1 = 0; l1<   L1_SIZE; ++l1) {
+        if (phys_map[l1]) {
+            phys_page_for_each_in_l1_map(phys_map[l1], client);
+        }
+    }
+#else
+    if (!l1_phys_map) {
Quoted 9 lines

exec.c const CPULogItem cpu_log_items[] = { · line 1706

Quoted 7 lines
exec.c +5 -0
+        return;
+    }
+    phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}

This looks pretty frightening. What is it needed for?

Quoted 5 lines

The point is that clients can be registered at any point.

A client that registered when memory is present needs to be notified about it.

It looks very expensive. Maybe we mandate clients be registered at init-time?

Long term we need to move to a range based memory description.

Signature · 1 line
error compiling committee.c: too many arguments to function
#5 lore raw
Quoted 61 lines · Avi Kivity
Quoted 60 lines · Michael S. Tsirkin
Quoted 58 lines · Avi Kivity
Quoted 44 lines · Michael S. Tsirkin

This adds notifiers for phys memory changes: a set of callbacks that vhost can register and update kernel accordingly. Down the road, kvm code can be switched to use these as well, instead of calling kvm code directly from exec.c as is done now.

exec.c +36 -0
+
+static void phys_page_for_each_in_l1_map(PhysPageDesc **phys_map,
+                                         CPUPhysMemoryClient *client)
+{
+    PhysPageDesc *pd;
+    int l1, l2;
+
+    for (l1 = 0; l1<   L1_SIZE; ++l1) {
+        pd = phys_map[l1];
+        if (!pd) {
+            continue;
+        }
+        for (l2 = 0; l2<   L2_SIZE; ++l2) {
+            if (pd[l2].phys_offset == IO_MEM_UNASSIGNED) {
+                continue;
+            }
+            client->set_memory(client, pd[l2].region_offset,
+                               TARGET_PAGE_SIZE, pd[l2].phys_offset);
+        }
+    }
+}
+
+static void phys_page_for_each(CPUPhysMemoryClient *client)
+{
+#if TARGET_PHYS_ADDR_SPACE_BITS>   32
+
+#if TARGET_PHYS_ADDR_SPACE_BITS>   (32 + L1_BITS)
+#error unsupported TARGET_PHYS_ADDR_SPACE_BITS
+#endif
+    void **phys_map = (void **)l1_phys_map;
+    int l1;
+    if (!l1_phys_map) {
+        return;
+    }
+    for (l1 = 0; l1<   L1_SIZE; ++l1) {
+        if (phys_map[l1]) {

exec.c const CPULogItem cpu_log_items[] = { · line 1706

Quoted 10 lines
exec.c +10 -0
+            phys_page_for_each_in_l1_map(phys_map[l1], client);
+        }
+    }
+#else
+    if (!l1_phys_map) {
+        return;
+    }
+    phys_page_for_each_in_l1_map(l1_phys_map, client);
+#endif
+}

This looks pretty frightening. What is it needed for?

Quoted 7 lines
Quoted 5 lines

The point is that clients can be registered at any point.

A client that registered when memory is present needs to be notified about it.

It looks very expensive.

Shouldn't be hard to optimize ...

Quoted 1 line

Maybe we mandate clients be registered at init-time?

This might be tricky - vhost currently only registers when the first device is hot-added.

Quoted 4 lines

Long term we need to move to a range based memory description.

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
Quoted 4 lines · Michael S. Tsirkin
Quoted 4 lines
Quoted 4 lines

The point is that clients can be registered at any point.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

A client that registered when memory is present needs to be notified about it.

It looks very expensive.

Shouldn't be hard to optimize ...

It's O(memory size), which can be very big.

Quoted 6 lines
Quoted 2 lines

Maybe we mandate clients be registered at init-time?

This might be tricky - vhost currently only registers when the first device is hot-added.

I see.

Maybe coalesce adjacent pages and call the callback with the ranges?

Signature · 1 line
error compiling committee.c: too many arguments to function
#7 lore raw
Quoted 18 lines · Avi Kivity
Quoted 4 lines · Michael S. Tsirkin
Quoted 4 lines
Quoted 4 lines

The point is that clients can be registered at any point.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

A client that registered when memory is present needs to be notified about it.

It looks very expensive.

Shouldn't be hard to optimize ...

It's O(memory size), which can be very big.

Quoted 3 lines
Quoted 2 lines

Maybe we mandate clients be registered at init-time?

Quoted 7 lines
Quoted 3 lines

This might be tricky - vhost currently only registers when the first device is hot-added.

I see.

Maybe coalesce adjacent pages and call the callback with the ranges?

Yes, I'll do that.

#8 lore raw
In reply to Avi Kivity
Quoted 9 lines · Avi Kivity
Quoted 8 lines · Michael S. Tsirkin
Quoted 8 lines
Quoted 8 lines

The point is that clients can be registered at any point.

A client that registered when memory is present needs to be notified about it.

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines

It looks very expensive.

Shouldn't be hard to optimize ...

It's O(memory size), which can be very big.

cpu_register_physical_memory_offset already is O(memory size) btw.

Quoted 10 lines
Quoted 6 lines
Quoted 2 lines

Maybe we mandate clients be registered at init-time?

This might be tricky - vhost currently only registers when the first device is hot-added.

I see.

Maybe coalesce adjacent pages and call the callback with the ranges?

Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ...
Can't we just rely on callback to do the coalescing?

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#9 lore raw
Quoted 3 lines · Michael S. Tsirkin

cpu_register_physical_memory_offset already is O(memory size) btw.

Right, but we'd like to replace it with a range API.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines

Maybe we mandate clients be registered at init-time?

This might be tricky - vhost currently only registers when the first device is hot-added.

Quoted 8 lines
Quoted 4 lines

I see.

Maybe coalesce adjacent pages and call the callback with the ranges?

Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ...
Can't we just rely on callback to do the coalescing?

If the callback can do the coalescing, surely the caller can as well?

This way we don't introduce a new per-page API.

Signature · 1 line
error compiling committee.c: too many arguments to function
#10 lore raw
Quoted 6 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin

cpu_register_physical_memory_offset already is O(memory size) btw.

Right, but we'd like to replace it with a range API.

So, when we do the implementation of notifiers can follow?

Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 7 lines
Quoted 3 lines

Maybe we mandate clients be registered at init-time?

This might be tricky - vhost currently only registers when the first device is hot-added.

I see.

Quoted 8 lines
Quoted 6 lines
Quoted 2 lines

Maybe coalesce adjacent pages and call the callback with the ranges?

Hmm, it turns out to be tricky: it seems whether we can do this really depends on what get_ram_ptr returns ...
Can't we just rely on callback to do the coalescing?

If the callback can do the coalescing, surely the caller can as well?

The callback calls qemu_ram_ptr and coalesces when the virtual memory pointers are matching. caller can do this as well but it looks ugly: we don't know this is what caller does ...

Quoted 1 line

This way we don't introduce a new per-page API.

What do you mean by new per-page API?
The API is range-based, implementation
currently scans all pages.

Quoted 2 lines
Signature · 1 line
error compiling committee.c: too many arguments to function
#11 lore raw
Quoted 11 lines · Michael S. Tsirkin
Quoted 7 lines · Avi Kivity
Quoted 3 lines · Michael S. Tsirkin

cpu_register_physical_memory_offset already is O(memory size) btw.

Right, but we'd like to replace it with a range API.

So, when we do the implementation of notifiers can follow?

Okay.

Signature · 1 line
error compiling committee.c: too many arguments to function