[Qemu-devel] [PATCH 0/7] kvm: fix system reset & rework slot management

[Qemu-devel] [PATCH 0/7] kvm: fix system reset & rework slot management

[Jan Kiszka]

This is the promised series to improve KVM's memory slot management.
Things turned out to be more complicated and ugly than planned due to an
unfortunate bug in KVM's kernel code, see [1] and patch 5 of this
series.

The most important impact of this series is that it finally enables
support for resetting qemu guests in KVM mode. Path 1 and 2 lay the
groundwork for this, and the slot management rework fixes the remaining
issues around memory remapping via PAM.

Find the patches also at git://git.kiszka.org/qemu.git queues/kvm

Jan Kiszka (7):
      kvm: Sync CPU state on reset
      kvm: Apply SMM-already-initialized workaround on reset
      kvm: Cleanup unmap condition in kvm_set_phys_mem
      kvm: Add sanity checks to slot management
      kvm: improve handling of overlapping slots
      vga: Fix inconsistent tracking of map_addr
      vga: Cleanup dirty logging

 hw/acpi.c       |   17 +++--
 hw/cirrus_vga.c |   17 ----
 hw/vga.c        |   27 +------
 hw/vga_int.h    |    1 -
 kvm-all.c       |  219 +++++++++++++++++++++++++++++++++++++++----------------
 kvm.h           |    7 +-
 vl.c            |    2 +
 7 files changed, 177 insertions(+), 113 deletions(-)

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

[Qemu-devel] [PATCH 3/7] kvm: Cleanup unmap condition in kvm_set_phys_mem

[Jan Kiszka]

Testing for TLB_MMIO on unmap makes no sense as A) that flag belongs to
CPUTLBEntry and not to io_memory slots or physical addresses and B) we
already use a different condition before mapping. So make this test
consistent.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 kvm-all.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 7198b32..e5c0d36 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -550,7 +550,7 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
 
     mem = kvm_lookup_slot(s, start_addr);
     if (mem) {
-        if ((flags == IO_MEM_UNASSIGNED) || (flags >= TLB_MMIO)) {
+        if (flags >= IO_MEM_UNASSIGNED) {
             mem->memory_size = 0;
             mem->start_addr = start_addr;
             mem->phys_offset = 0;

[Qemu-devel] [PATCH 7/7] vga: Cleanup dirty logging

[Jan Kiszka]

In theory, there are no more quirks in the KVM slot management that
requires dirty log start/stop all over the place. We just have to start
the logging each time the mapping may have changed. This patch drops
vga_dirty_log_stop for both standard and cirrus VGA. It also reverts
#6851 as it was obviously a tribute to the old slot system.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/cirrus_vga.c |   17 -----------------
 hw/vga.c        |   17 -----------------
 hw/vga_int.h    |    1 -
 3 files changed, 0 insertions(+), 35 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 2200ae0..114681e 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2618,8 +2618,6 @@ static CPUWriteMemoryFunc *cirrus_linear_bitblt_write[3] = {
 
 static void map_linear_vram(CirrusVGAState *s)
 {
-    vga_dirty_log_stop((VGAState *)s);
-
     if (!s->map_addr && s->lfb_addr && s->lfb_end) {
         s->map_addr = s->lfb_addr;
         s->map_end = s->lfb_end;
@@ -2631,16 +2629,11 @@ static void map_linear_vram(CirrusVGAState *s)
 
     s->lfb_vram_mapped = 0;
 
-    cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
-                                (s->vram_offset + s->cirrus_bank_base[0]) | IO_MEM_UNASSIGNED);
-    cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
-                                (s->vram_offset + s->cirrus_bank_base[1]) | IO_MEM_UNASSIGNED);
     if (!(s->cirrus_srcptr != s->cirrus_srcptr_end)
         && !((s->sr[0x07] & 0x01) == 0)
         && !((s->gr[0x0B] & 0x14) == 0x14)
         && !(s->gr[0x0B] & 0x02)) {
 
-        vga_dirty_log_stop((VGAState *)s);
         cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x8000,
                                     (s->vram_offset + s->cirrus_bank_base[0]) | IO_MEM_RAM);
         cpu_register_physical_memory(isa_mem_base + 0xa8000, 0x8000,
@@ -2658,15 +2651,11 @@ static void map_linear_vram(CirrusVGAState *s)
 
 static void unmap_linear_vram(CirrusVGAState *s)
 {
-    vga_dirty_log_stop((VGAState *)s);
-
     if (s->map_addr && s->lfb_addr && s->lfb_end)
         s->map_addr = s->map_end = 0;
 
     cpu_register_physical_memory(isa_mem_base + 0xa0000, 0x20000,
                                  s->vga_io_memory);
-
-    vga_dirty_log_start((VGAState *)s);
 }
 
 /* Compute the memory access functions */
@@ -3313,8 +3302,6 @@ static void cirrus_pci_lfb_map(PCIDevice *d, int region_num,
 {
     CirrusVGAState *s = &((PCICirrusVGAState *)d)->cirrus_vga;
 
-    vga_dirty_log_stop((VGAState *)s);
-
     /* XXX: add byte swapping apertures */
     cpu_register_physical_memory(addr, s->vram_size,
 				 s->cirrus_linear_io_addr);
@@ -3346,14 +3333,10 @@ static void pci_cirrus_write_config(PCIDevice *d,
     PCICirrusVGAState *pvs = container_of(d, PCICirrusVGAState, dev);
     CirrusVGAState *s = &pvs->cirrus_vga;
 
-    vga_dirty_log_stop((VGAState *)s);
-
     pci_default_write_config(d, address, val, len);
     if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
         s->map_addr = 0;
     cirrus_update_memory_access(s);
-
-    vga_dirty_log_start((VGAState *)s);
 }
 
 void pci_cirrus_vga_init(PCIBus *bus, int vga_ram_size)
diff --git a/hw/vga.c b/hw/vga.c
index a483168..2f122ee 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -1279,8 +1279,6 @@ static void vga_draw_text(VGAState *s, int full_update)
     vga_draw_glyph8_func *vga_draw_glyph8;
     vga_draw_glyph9_func *vga_draw_glyph9;
 
-    vga_dirty_log_stop(s);
-
     /* compute font data address (in plane 2) */
     v = s->sr[3];
     offset = (((v >> 4) & 1) | ((v << 1) & 6)) * 8192 * 4 + 2;
@@ -1579,7 +1577,6 @@ static void vga_sync_dirty_bitmap(VGAState *s)
         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa0000, 0xa8000);
         cpu_physical_sync_dirty_bitmap(isa_mem_base + 0xa8000, 0xb0000);
     }
-    vga_dirty_log_start(s);
 }
 
 /*
@@ -1810,7 +1807,6 @@ static void vga_draw_blank(VGAState *s, int full_update)
         return;
     if (s->last_scr_width <= 0 || s->last_scr_height <= 0)
         return;
-    vga_dirty_log_stop(s);
 
     s->rgb_to_pixel =
         rgb_to_pixel_dup_table[get_depth_index(s->ds)];
@@ -2238,17 +2234,6 @@ void vga_dirty_log_start(VGAState *s)
     }
 }
 
-void vga_dirty_log_stop(VGAState *s)
-{
-    if (kvm_enabled() && s->map_addr)
-        kvm_log_stop(s->map_addr, s->map_end - s->map_addr);
-
-    if (kvm_enabled() && s->lfb_vram_mapped) {
-        kvm_log_stop(isa_mem_base + 0xa0000, 0x8000);
-        kvm_log_stop(isa_mem_base + 0xa8000, 0x8000);
-    }
-}
-
 static void vga_map(PCIDevice *pci_dev, int region_num,
                     uint32_t addr, uint32_t size, int type)
 {
@@ -2489,11 +2474,9 @@ static void pci_vga_write_config(PCIDevice *d,
     PCIVGAState *pvs = container_of(d, PCIVGAState, dev);
     VGAState *s = &pvs->vga_state;
 
-    vga_dirty_log_stop(s);
     pci_default_write_config(d, address, val, len);
     if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
         s->map_addr = 0;
-    vga_dirty_log_start(s);
 }
 
 int pci_vga_init(PCIBus *bus, int vga_ram_size,
diff --git a/hw/vga_int.h b/hw/vga_int.h
index 5d66bd2..1971aa6 100644
--- a/hw/vga_int.h
+++ b/hw/vga_int.h
@@ -196,7 +196,6 @@ void vga_init(VGAState *s);
 void vga_reset(void *s);
 
 void vga_dirty_log_start(VGAState *s);
-void vga_dirty_log_stop(VGAState *s);
 
 uint32_t vga_mem_readb(void *opaque, target_phys_addr_t addr);
 void vga_mem_writeb(void *opaque, target_phys_addr_t addr, uint32_t val);

[Qemu-devel] [PATCH 6/7] vga: Fix inconsistent tracking of map_addr

[Jan Kiszka]

Only track video RAM mapping in map_addr and use the correct RAM size.
Furthermore, make sure the reset the address in case unmapping took
place via PCI reconfiguration.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/vga.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/hw/vga.c b/hw/vga.c
index b53b743..a483168 100644
--- a/hw/vga.c
+++ b/hw/vga.c
@@ -2258,12 +2258,10 @@ static void vga_map(PCIDevice *pci_dev, int region_num,
         cpu_register_physical_memory(addr, s->bios_size, s->bios_offset);
     } else {
         cpu_register_physical_memory(addr, s->vram_size, s->vram_offset);
+        s->map_addr = addr;
+        s->map_end = addr + s->vram_size;
+        vga_dirty_log_start(s);
     }
-
-    s->map_addr = addr;
-    s->map_end = addr + VGA_RAM_SIZE;
-
-    vga_dirty_log_start(s);
 }
 
 void vga_common_init(VGAState *s, int vga_ram_size)
@@ -2493,6 +2491,8 @@ static void pci_vga_write_config(PCIDevice *d,
 
     vga_dirty_log_stop(s);
     pci_default_write_config(d, address, val, len);
+    if (s->map_addr && pvs->dev.io_regions[0].addr == -1)
+        s->map_addr = 0;
     vga_dirty_log_start(s);
 }
 

[Qemu-devel] [PATCH 5/7] kvm: improve handling of overlapping slots

[Jan Kiszka]

This reworks the slot management to handle more patterns of
cpu_register_physical_memory*, finally allowing to reset KVM guests (so
far address remapping on reset broke the slot management).

We could actually handle all possible ones without failing, but a KVM
kernel bug in older versions would force us to track all previous
fragmentations and maintain them (as that bug prevents registering
larger slots that overlap also deleted ones). To remain backward
compatible but avoid overly complicated workarounds, we apply a simpler
workaround that covers all currently used patterns.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 kvm-all.c |  166 ++++++++++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 114 insertions(+), 52 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 3e4e421..32cd636 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -98,19 +98,31 @@ static KVMSlot *kvm_lookup_matching_slot(KVMState *s,
     return NULL;
 }
 
-static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
+/*
+ * Find overlapping slot with lowest start address
+ */
+static KVMSlot *kvm_lookup_overlapping_slot(KVMState *s,
+                                            target_phys_addr_t start_addr,
+                                            target_phys_addr_t end_addr)
 {
+    KVMSlot *found = NULL;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         KVMSlot *mem = &s->slots[i];
 
-        if (start_addr >= mem->start_addr &&
-            start_addr < (mem->start_addr + mem->memory_size))
-            return mem;
+        if (mem->memory_size == 0 ||
+            (found && found->start_addr < mem->start_addr)) {
+            continue;
+        }
+
+        if (end_addr > mem->start_addr &&
+            start_addr < mem->start_addr + mem->memory_size) {
+            found = mem;
+        }
     }
 
-    return NULL;
+    return found;
 }
 
 static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
@@ -567,7 +579,8 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
 {
     KVMState *s = kvm_state;
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
-    KVMSlot *mem;
+    KVMSlot *mem, old;
+    int err;
 
     if (start_addr & ~TARGET_PAGE_MASK) {
         fprintf(stderr, "Only page-aligned memory slots supported\n");
@@ -577,55 +590,100 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     /* KVM does not support read-only slots */
     phys_offset &= ~IO_MEM_ROM;
 
-    mem = kvm_lookup_slot(s, start_addr);
-    if (mem) {
-        if (flags >= IO_MEM_UNASSIGNED) {
-            mem->memory_size = 0;
-            mem->start_addr = start_addr;
-            mem->phys_offset = 0;
-            mem->flags = 0;
-
-            kvm_set_user_memory_region(s, mem);
-        } else if (start_addr >= mem->start_addr &&
-                   (start_addr + size) <= (mem->start_addr +
-                                           mem->memory_size)) {
-            KVMSlot slot;
-            target_phys_addr_t mem_start;
-            ram_addr_t mem_size, mem_offset;
-
-            /* Not splitting */
-            if ((phys_offset - (start_addr - mem->start_addr)) == 
-                mem->phys_offset)
-                return;
-
-            /* unregister whole slot */
-            memcpy(&slot, mem, sizeof(slot));
-            mem->memory_size = 0;
-            kvm_set_user_memory_region(s, mem);
-
-            /* register prefix slot */
-            mem_start = slot.start_addr;
-            mem_size = start_addr - slot.start_addr;
-            mem_offset = slot.phys_offset;
-            if (mem_size)
-                kvm_set_phys_mem(mem_start, mem_size, mem_offset);
-
-            /* register new slot */
-            kvm_set_phys_mem(start_addr, size, phys_offset);
-
-            /* register suffix slot */
-            mem_start = start_addr + size;
-            mem_offset += mem_size + size;
-            mem_size = slot.memory_size - mem_size - size;
-            if (mem_size)
-                kvm_set_phys_mem(mem_start, mem_size, mem_offset);
+    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;
-        } else {
-            printf("Registering overlapping slot\n");
+        }
+
+        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 (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;
@@ -636,8 +694,12 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     mem->phys_offset = phys_offset;
     mem->flags = 0;
 
-    kvm_set_user_memory_region(s, mem);
-    /* FIXME deal with errors */
+    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, ...)

[Qemu-devel] [PATCH 5/7 v2] kvm: improve handling of overlapping slots

[Jan Kiszka]

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

This reworks the slot management to handle all patterns of
cpu_register_physical_memory*, finally allowing to reset KVM guests (so
far address remapping on reset broke the slot management).

Nevertheless, KVM kernel bug in older versions forces us to track
previousfragmentations and maintain them (as that bug prevents
registering larger slots that overlap also deleted ones). When affected
KVM versions are detected, we apply a workaround that covers all
currently used patterns.

Changes in v2:
 - detect KVM kernel bug and apply the workaround only when needed

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 kvm-all.c |  176 +++++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 124 insertions(+), 52 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 3e4e421..9e9b462 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -57,6 +57,7 @@ struct KVMState
     int fd;
     int vmfd;
     int coalesced_mmio;
+    int broken_set_mem_region;
 #ifdef KVM_CAP_SET_GUEST_DEBUG
     struct kvm_sw_breakpoint_head kvm_sw_breakpoints;
 #endif
@@ -98,19 +99,31 @@ static KVMSlot *kvm_lookup_matching_slot(KVMState *s,
     return NULL;
 }
 
-static KVMSlot *kvm_lookup_slot(KVMState *s, target_phys_addr_t start_addr)
+/*
+ * Find overlapping slot with lowest start address
+ */
+static KVMSlot *kvm_lookup_overlapping_slot(KVMState *s,
+                                            target_phys_addr_t start_addr,
+                                            target_phys_addr_t end_addr)
 {
+    KVMSlot *found = NULL;
     int i;
 
     for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
         KVMSlot *mem = &s->slots[i];
 
-        if (start_addr >= mem->start_addr &&
-            start_addr < (mem->start_addr + mem->memory_size))
-            return mem;
+        if (mem->memory_size == 0 ||
+            (found && found->start_addr < mem->start_addr)) {
+            continue;
+        }
+
+        if (end_addr > mem->start_addr &&
+            start_addr < mem->start_addr + mem->memory_size) {
+            found = mem;
+        }
     }
 
-    return NULL;
+    return found;
 }
 
 static int kvm_set_user_memory_region(KVMState *s, KVMSlot *slot)
@@ -386,6 +399,14 @@ int kvm_init(int smp_cpus)
         s->coalesced_mmio = ret;
 #endif
 
+    s->broken_set_mem_region = 1;
+#ifdef KVM_CAP_JOIN_MEMORY_REGIONS_WORKS
+    ret = kvm_ioctl(s, KVM_CHECK_EXTENSION, KVM_CAP_JOIN_MEMORY_REGIONS_WORKS);
+    if (ret > 0) {
+        s->broken_set_mem_region = 0;
+    }
+#endif
+
     ret = kvm_arch_init(s, smp_cpus);
     if (ret < 0)
         goto err;
@@ -567,7 +588,8 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
 {
     KVMState *s = kvm_state;
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
-    KVMSlot *mem;
+    KVMSlot *mem, old;
+    int err;
 
     if (start_addr & ~TARGET_PAGE_MASK) {
         fprintf(stderr, "Only page-aligned memory slots supported\n");
@@ -577,55 +599,101 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     /* KVM does not support read-only slots */
     phys_offset &= ~IO_MEM_ROM;
 
-    mem = kvm_lookup_slot(s, start_addr);
-    if (mem) {
-        if (flags >= IO_MEM_UNASSIGNED) {
-            mem->memory_size = 0;
-            mem->start_addr = start_addr;
-            mem->phys_offset = 0;
-            mem->flags = 0;
-
-            kvm_set_user_memory_region(s, mem);
-        } else if (start_addr >= mem->start_addr &&
-                   (start_addr + size) <= (mem->start_addr +
-                                           mem->memory_size)) {
-            KVMSlot slot;
-            target_phys_addr_t mem_start;
-            ram_addr_t mem_size, mem_offset;
-
-            /* Not splitting */
-            if ((phys_offset - (start_addr - mem->start_addr)) == 
-                mem->phys_offset)
-                return;
-
-            /* unregister whole slot */
-            memcpy(&slot, mem, sizeof(slot));
-            mem->memory_size = 0;
-            kvm_set_user_memory_region(s, mem);
-
-            /* register prefix slot */
-            mem_start = slot.start_addr;
-            mem_size = start_addr - slot.start_addr;
-            mem_offset = slot.phys_offset;
-            if (mem_size)
-                kvm_set_phys_mem(mem_start, mem_size, mem_offset);
-
-            /* register new slot */
-            kvm_set_phys_mem(start_addr, size, phys_offset);
-
-            /* register suffix slot */
-            mem_start = start_addr + size;
-            mem_offset += mem_size + size;
-            mem_size = slot.memory_size - mem_size - size;
-            if (mem_size)
-                kvm_set_phys_mem(mem_start, mem_size, mem_offset);
+    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;
-        } else {
-            printf("Registering overlapping slot\n");
+        }
+
+        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;
@@ -636,8 +704,12 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     mem->phys_offset = phys_offset;
     mem->flags = 0;
 
-    kvm_set_user_memory_region(s, mem);
-    /* FIXME deal with errors */
+    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, ...)


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

Re: [Qemu-devel] [PATCH 5/7 v2] kvm: improve handling of overlapping slots

[Anthony Liguori]

Jan Kiszka wrote:
> This reworks the slot management to handle all patterns of
> cpu_register_physical_memory*, finally allowing to reset KVM guests (so
> far address remapping on reset broke the slot management).
>
> Nevertheless, KVM kernel bug in older versions forces us to track
> previousfragmentations and maintain them (as that bug prevents
> registering larger slots that overlap also deleted ones). When affected
> KVM versions are detected, we apply a workaround that covers all
> currently used patterns.
>
> Changes in v2:
>  - detect KVM kernel bug and apply the workaround only when needed
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>   

Please resubmit this as a patch against v1.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 2/7] kvm: Apply SMM-already-initialized workaround on reset

[Jan Kiszka]

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 hw/acpi.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/hw/acpi.c b/hw/acpi.c
index 52f50a0..53c1fec 100644
--- a/hw/acpi.c
+++ b/hw/acpi.c
@@ -483,13 +483,18 @@ static int pm_load(QEMUFile* f,void* opaque,int version_id)
 
 static void piix4_reset(void *opaque)
 {
-	PIIX4PMState *s = opaque;
-	uint8_t *pci_conf = s->dev.config;
+    PIIX4PMState *s = opaque;
+    uint8_t *pci_conf = s->dev.config;
+
+    pci_conf[0x58] = 0;
+    pci_conf[0x59] = 0;
+    pci_conf[0x5a] = 0;
+    pci_conf[0x5b] = 0;
 
-	pci_conf[0x58] = 0;
-	pci_conf[0x59] = 0;
-	pci_conf[0x5a] = 0;
-	pci_conf[0x5b] = 0;
+    if (kvm_enabled()) {
+        /* Mark SMM as already inited (until KVM supports SMM). */
+        pci_conf[0x5B] = 0x02;
+    }
 }
 
 i2c_bus *piix4_pm_init(PCIBus *bus, int devfn, uint32_t smb_io_base,

[Qemu-devel] [PATCH 1/7] kvm: Sync CPU state on reset

[Jan Kiszka]

Make sure KVM gets informed about the reset CPU state.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 vl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index cd974be..7417644 100644
--- a/vl.c
+++ b/vl.c
@@ -3623,6 +3623,8 @@ void qemu_system_reset(void)
     for(re = first_reset_entry; re != NULL; re = re->next) {
         re->func(re->opaque);
     }
+    if (kvm_enabled())
+        kvm_sync_vcpus();
 }
 
 void qemu_system_reset_request(void)

Re: [Qemu-devel] [PATCH 1/7] kvm: Sync CPU state on reset

[Anthony Liguori]

Jan Kiszka wrote:
> Make sure KVM gets informed about the reset CPU state.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>
>  vl.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/vl.c b/vl.c
> index cd974be..7417644 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -3623,6 +3623,8 @@ void qemu_system_reset(void)
>      for(re = first_reset_entry; re != NULL; re = re->next) {
>          re->func(re->opaque);
>      }
> +    if (kvm_enabled())
> +        kvm_sync_vcpus();
>  }
>   

Perhaps it would be better to register a reset handler in kvm_init?

If it has to run last, then we can add priority to the reset handlers.

Regards,

Anthony Liguori

>  void qemu_system_reset_request(void)
>
>
>
>
>
>   


-- 
Regards,

Anthony Liguori

[Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Fail loudly if we run out of memory slot.

Make sure that dirty log start/stop works with consistent memory regions
by reporting invalid parameters. This reveals several inconsistencies in
the vga code, patch to fix them follows later in this series.

And, for simplicity reasons, also catch and report unaligned memory
regions passed to kvm_set_phys_mem (KVM works on page basis).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

 kvm-all.c |   55 ++++++++++++++++++++++++++++++++++++++++++-------------
 kvm.h     |    7 ++++---
 2 files changed, 46 insertions(+), 16 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index e5c0d36..3e4e421 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -76,6 +76,25 @@ static KVMSlot *kvm_alloc_slot(KVMState *s)
             return &s->slots[i];
     }
 
+    fprintf(stderr, "%s: no free slot available\n", __func__);
+    abort();
+}
+
+static KVMSlot *kvm_lookup_matching_slot(KVMState *s,
+                                         target_phys_addr_t start_addr,
+                                         target_phys_addr_t end_addr)
+{
+    int i;
+
+    for (i = 0; i < ARRAY_SIZE(s->slots); i++) {
+        KVMSlot *mem = &s->slots[i];
+
+        if (start_addr == mem->start_addr &&
+            end_addr == mem->start_addr + mem->memory_size) {
+            return mem;
+        }
+    }
+
     return NULL;
 }
 
@@ -163,14 +182,16 @@ int kvm_sync_vcpus(void)
 /*
  * dirty pages logging control
  */
-static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr, target_phys_addr_t end_addr,
-                                      unsigned flags,
+static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr,
+                                      ram_addr_t size, unsigned flags,
                                       unsigned mask)
 {
     KVMState *s = kvm_state;
-    KVMSlot *mem = kvm_lookup_slot(s, phys_addr);
+    KVMSlot *mem = kvm_lookup_matching_slot(s, phys_addr, phys_addr + size);
     if (mem == NULL)  {
-            dprintf("invalid parameters %llx-%llx\n", phys_addr, end_addr);
+            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
+                    TARGET_FMT_plx "\n", __func__, phys_addr,
+                    phys_addr + size - 1);
             return -EINVAL;
     }
 
@@ -184,16 +205,16 @@ static int kvm_dirty_pages_log_change(target_phys_addr_t phys_addr, target_phys_
     return kvm_set_user_memory_region(s, mem);
 }
 
-int kvm_log_start(target_phys_addr_t phys_addr, target_phys_addr_t end_addr)
+int kvm_log_start(target_phys_addr_t phys_addr, ram_addr_t size)
 {
-        return kvm_dirty_pages_log_change(phys_addr, end_addr,
+        return kvm_dirty_pages_log_change(phys_addr, size,
                                           KVM_MEM_LOG_DIRTY_PAGES,
                                           KVM_MEM_LOG_DIRTY_PAGES);
 }
 
-int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t end_addr)
+int kvm_log_stop(target_phys_addr_t phys_addr, ram_addr_t size)
 {
-        return kvm_dirty_pages_log_change(phys_addr, end_addr,
+        return kvm_dirty_pages_log_change(phys_addr, size,
                                           0,
                                           KVM_MEM_LOG_DIRTY_PAGES);
 }
@@ -203,21 +224,24 @@ int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t end_addr)
  * This function updates qemu's dirty bitmap using cpu_physical_memory_set_dirty().
  * This means all bits are set to dirty.
  *
- * @start_add: start of logged region. This is what we use to search the memslot
+ * @start_add: start of logged region.
  * @end_addr: end of logged region.
  */
-void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr)
+void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr,
+                                    target_phys_addr_t end_addr)
 {
     KVMState *s = kvm_state;
     KVMDirtyLog d;
-    KVMSlot *mem = kvm_lookup_slot(s, start_addr);
+    KVMSlot *mem = kvm_lookup_matching_slot(s, start_addr, end_addr);
     unsigned long alloc_size;
     ram_addr_t addr;
     target_phys_addr_t phys_addr = start_addr;
 
-    dprintf("sync addr: %llx into %lx\n", start_addr, mem->phys_offset);
+    dprintf("sync addr: " TARGET_FMT_lx " into %lx\n", start_addr,
+            mem->phys_offset);
     if (mem == NULL) {
-            fprintf(stderr, "BUG: %s: invalid parameters\n", __func__);
+            fprintf(stderr, "BUG: %s: invalid parameters " TARGET_FMT_plx "-"
+                    TARGET_FMT_plx "\n", __func__, phys_addr, end_addr - 1);
             return;
     }
 
@@ -545,6 +569,11 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     ram_addr_t flags = phys_offset & ~TARGET_PAGE_MASK;
     KVMSlot *mem;
 
+    if (start_addr & ~TARGET_PAGE_MASK) {
+        fprintf(stderr, "Only page-aligned memory slots supported\n");
+        abort();
+    }
+
     /* KVM does not support read-only slots */
     phys_offset &= ~IO_MEM_ROM;
 
diff --git a/kvm.h b/kvm.h
index 0d6bf7e..803a874 100644
--- a/kvm.h
+++ b/kvm.h
@@ -40,10 +40,11 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
                       ram_addr_t size,
                       ram_addr_t phys_offset);
 
-void kvm_physical_sync_dirty_bitmap(target_phys_addr_t start_addr, target_phys_addr_t end_addr);
+void 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, target_phys_addr_t len);
-int kvm_log_stop(target_phys_addr_t phys_addr, target_phys_addr_t len);
+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_has_sync_mmu(void);
 

RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

> -----Original Message-----
> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> On Behalf Of Jan Kiszka
> Sent: Sunday, April 12, 2009 1:20 AM
> To: qemu-devel@nongnu.org
> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
> slot management
> 
> Fail loudly if we run out of memory slot.
> 
> Make sure that dirty log start/stop works with consistent 
> memory regions
> by reporting invalid parameters. This reveals several 
> inconsistencies in
> the vga code, patch to fix them follows later in this series.
> 
> And, for simplicity reasons, also catch and report unaligned memory
> regions passed to kvm_set_phys_mem (KVM works on page basis).
> 

Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Liu Yu-B13201 wrote:
> 
>> -----Original Message-----
>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>> On Behalf Of Jan Kiszka
>> Sent: Sunday, April 12, 2009 1:20 AM
>> To: qemu-devel@nongnu.org
>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
>> slot management
>>
>> Fail loudly if we run out of memory slot.
>>
>> Make sure that dirty log start/stop works with consistent 
>> memory regions
>> by reporting invalid parameters. This reveals several 
>> inconsistencies in
>> the vga code, patch to fix them follows later in this series.
>>
>> And, for simplicity reasons, also catch and report unaligned memory
>> regions passed to kvm_set_phys_mem (KVM works on page basis).
>>
> 
> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.

What is the alignment of those regions then? None? And do regions of
different types overlap even on the same page? Maybe the check reveals
some deeper conflict /wrt KVM. Can you point me to the involved code files?

Jan

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

RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
> Sent: Wednesday, April 29, 2009 6:38 PM
> To: Liu Yu-B13201
> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
> to slot management
> 
> Liu Yu-B13201 wrote:
> > 
> >> -----Original Message-----
> >> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >> On Behalf Of Jan Kiszka
> >> Sent: Sunday, April 12, 2009 1:20 AM
> >> To: qemu-devel@nongnu.org
> >> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
> >> slot management
> >>
> >> Fail loudly if we run out of memory slot.
> >>
> >> Make sure that dirty log start/stop works with consistent 
> >> memory regions
> >> by reporting invalid parameters. This reveals several 
> >> inconsistencies in
> >> the vga code, patch to fix them follows later in this series.
> >>
> >> And, for simplicity reasons, also catch and report unaligned memory
> >> regions passed to kvm_set_phys_mem (KVM works on page basis).
> >>
> > 
> > Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
> > The alignment check in kvm_set_phys_mem prevents pci 
> controller and mpic initializing mmio regions.
> 
> What is the alignment of those regions then? None? 
> And do regions of
> different types overlap even on the same page?
I think so.

> Maybe the check reveals
> some deeper conflict /wrt KVM. Can you point me to the 
> involved code files?
> 

hw/ppc4xx_pci.c	ppc4xx_pci_init()
hw/ppce500_pci.c	ppce500_pci_init()
hw/openpic.c		mpic_init()
hw/ppce500_mpc8544ds.c    serial_mm_init()

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Liu Yu-B13201 wrote:
>> -----Original Message-----
>> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
>> Sent: Wednesday, April 29, 2009 6:38 PM
>> To: Liu Yu-B13201
>> Cc: qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
>> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
>> to slot management
>>
>> Liu Yu-B13201 wrote:
>>>> -----Original Message-----
>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>> On Behalf Of Jan Kiszka
>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>> To: qemu-devel@nongnu.org
>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
>>>> slot management
>>>>
>>>> Fail loudly if we run out of memory slot.
>>>>
>>>> Make sure that dirty log start/stop works with consistent 
>>>> memory regions
>>>> by reporting invalid parameters. This reveals several 
>>>> inconsistencies in
>>>> the vga code, patch to fix them follows later in this series.
>>>>
>>>> And, for simplicity reasons, also catch and report unaligned memory
>>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
>>>>
>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
>>> The alignment check in kvm_set_phys_mem prevents pci 
>> controller and mpic initializing mmio regions.
>>
>> What is the alignment of those regions then? None? 
>> And do regions of
>> different types overlap even on the same page?
> I think so.
> 
>> Maybe the check reveals
>> some deeper conflict /wrt KVM. Can you point me to the 
>> involved code files?
>>
> 
> hw/ppc4xx_pci.c	ppc4xx_pci_init()
> hw/ppce500_pci.c	ppce500_pci_init()
> hw/openpic.c		mpic_init()
> hw/ppce500_mpc8544ds.c    serial_mm_init()
> 

Hmm, too bad that I have no platform at hand to test this. Could you
instrument kvm_set_phys_mem (on an older, working version), dumping its
arguments and post this trace?

TIA,
Jan

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

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Hollis Blanchard]

On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> Liu Yu-B13201 wrote:
> > 
> >> -----Original Message-----
> >> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >> On Behalf Of Jan Kiszka
> >> Sent: Sunday, April 12, 2009 1:20 AM
> >> To: qemu-devel@nongnu.org
> >> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
> >> slot management
> >>
> >> Fail loudly if we run out of memory slot.
> >>
> >> Make sure that dirty log start/stop works with consistent 
> >> memory regions
> >> by reporting invalid parameters. This reveals several 
> >> inconsistencies in
> >> the vga code, patch to fix them follows later in this series.
> >>
> >> And, for simplicity reasons, also catch and report unaligned memory
> >> regions passed to kvm_set_phys_mem (KVM works on page basis).
> >>
> > 
> > Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
> > The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.
> 
> What is the alignment of those regions then? None? And do regions of
> different types overlap even on the same page? Maybe the check reveals
> some deeper conflict /wrt KVM. Can you point me to the involved code files?

These PCI controllers make separate calls to
cpu_register_physical_memory() for separate callbacks. Reading
ppce500_pci_init(), for example:
0xe0008000 -> CFGADDR (4 bytes)
0xe0008004 -> CFGDATA (4 bytes)
0xe0008c00 -> other registers

The loop in cpu_register_physical_memory_offset() handles "subpage"
registration. However, kvm_set_phys_mem() is called outside that loop,
so it gets the non-page-aligned addresses.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>> Liu Yu-B13201 wrote:
>>>> -----Original Message-----
>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>> On Behalf Of Jan Kiszka
>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>> To: qemu-devel@nongnu.org
>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
>>>> slot management
>>>>
>>>> Fail loudly if we run out of memory slot.
>>>>
>>>> Make sure that dirty log start/stop works with consistent 
>>>> memory regions
>>>> by reporting invalid parameters. This reveals several 
>>>> inconsistencies in
>>>> the vga code, patch to fix them follows later in this series.
>>>>
>>>> And, for simplicity reasons, also catch and report unaligned memory
>>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
>>>>
>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
>>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.
>> What is the alignment of those regions then? None? And do regions of
>> different types overlap even on the same page? Maybe the check reveals
>> some deeper conflict /wrt KVM. Can you point me to the involved code files?
> 
> These PCI controllers make separate calls to
> cpu_register_physical_memory() for separate callbacks. Reading
> ppce500_pci_init(), for example:
> 0xe0008000 -> CFGADDR (4 bytes)
> 0xe0008004 -> CFGDATA (4 bytes)
> 0xe0008c00 -> other registers
> 
> The loop in cpu_register_physical_memory_offset() handles "subpage"
> registration. However, kvm_set_phys_mem() is called outside that loop,
> so it gets the non-page-aligned addresses.
> 

Half-blind shot:

diff --git a/kvm-all.c b/kvm-all.c
index 32cd636..c2c760e 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
     int err;

     if (start_addr & ~TARGET_PAGE_MASK) {
+        if (flags >= IO_MEM_UNASSIGNED) {
+            return;
+        }
         fprintf(stderr, "Only page-aligned memory slots supported\n");
         abort();
     }

If it works, it likely needs a cleaner approach to handle all cases.

Jan

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

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Hollis Blanchard]

On Wed, 2009-04-29 at 19:30 +0200, Jan Kiszka wrote:
> Hollis Blanchard wrote:
> > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> >> Liu Yu-B13201 wrote:
> >>>> -----Original Message-----
> >>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >>>> On Behalf Of Jan Kiszka
> >>>> Sent: Sunday, April 12, 2009 1:20 AM
> >>>> To: qemu-devel@nongnu.org
> >>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
> >>>> slot management
> >>>>
> >>>> Fail loudly if we run out of memory slot.
> >>>>
> >>>> Make sure that dirty log start/stop works with consistent 
> >>>> memory regions
> >>>> by reporting invalid parameters. This reveals several 
> >>>> inconsistencies in
> >>>> the vga code, patch to fix them follows later in this series.
> >>>>
> >>>> And, for simplicity reasons, also catch and report unaligned memory
> >>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
> >>>>
> >>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
> >>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.
> >> What is the alignment of those regions then? None? And do regions of
> >> different types overlap even on the same page? Maybe the check reveals
> >> some deeper conflict /wrt KVM. Can you point me to the involved code files?
> > 
> > These PCI controllers make separate calls to
> > cpu_register_physical_memory() for separate callbacks. Reading
> > ppce500_pci_init(), for example:
> > 0xe0008000 -> CFGADDR (4 bytes)
> > 0xe0008004 -> CFGDATA (4 bytes)
> > 0xe0008c00 -> other registers
> > 
> > The loop in cpu_register_physical_memory_offset() handles "subpage"
> > registration. However, kvm_set_phys_mem() is called outside that loop,
> > so it gets the non-page-aligned addresses.
> > 
> 
> Half-blind shot:
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 32cd636..c2c760e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
>      int err;
> 
>      if (start_addr & ~TARGET_PAGE_MASK) {
> +        if (flags >= IO_MEM_UNASSIGNED) {
> +            return;
> +        }
>          fprintf(stderr, "Only page-aligned memory slots supported\n");
>          abort();
>      }
> 
> If it works, it likely needs a cleaner approach to handle all cases.

I don't understand the point. kvm_set_phys_mem() already works without
this new abort() check.

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Jan Kiszka]

Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 19:30 +0200, Jan Kiszka wrote:
>> Hollis Blanchard wrote:
>>> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>>>> Liu Yu-B13201 wrote:
>>>>>> -----Original Message-----
>>>>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
>>>>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
>>>>>> On Behalf Of Jan Kiszka
>>>>>> Sent: Sunday, April 12, 2009 1:20 AM
>>>>>> To: qemu-devel@nongnu.org
>>>>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
>>>>>> slot management
>>>>>>
>>>>>> Fail loudly if we run out of memory slot.
>>>>>>
>>>>>> Make sure that dirty log start/stop works with consistent 
>>>>>> memory regions
>>>>>> by reporting invalid parameters. This reveals several 
>>>>>> inconsistencies in
>>>>>> the vga code, patch to fix them follows later in this series.
>>>>>>
>>>>>> And, for simplicity reasons, also catch and report unaligned memory
>>>>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
>>>>>>
>>>>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
>>>>> The alignment check in kvm_set_phys_mem prevents pci controller and mpic initializing mmio regions.
>>>> What is the alignment of those regions then? None? And do regions of
>>>> different types overlap even on the same page? Maybe the check reveals
>>>> some deeper conflict /wrt KVM. Can you point me to the involved code files?
>>> These PCI controllers make separate calls to
>>> cpu_register_physical_memory() for separate callbacks. Reading
>>> ppce500_pci_init(), for example:
>>> 0xe0008000 -> CFGADDR (4 bytes)
>>> 0xe0008004 -> CFGDATA (4 bytes)
>>> 0xe0008c00 -> other registers
>>>
>>> The loop in cpu_register_physical_memory_offset() handles "subpage"
>>> registration. However, kvm_set_phys_mem() is called outside that loop,
>>> so it gets the non-page-aligned addresses.
>>>
>> Half-blind shot:
>>
>> diff --git a/kvm-all.c b/kvm-all.c
>> index 32cd636..c2c760e 100644
>> --- a/kvm-all.c
>> +++ b/kvm-all.c
>> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t start_addr,
>>      int err;
>>
>>      if (start_addr & ~TARGET_PAGE_MASK) {
>> +        if (flags >= IO_MEM_UNASSIGNED) {
>> +            return;
>> +        }
>>          fprintf(stderr, "Only page-aligned memory slots supported\n");
>>          abort();
>>      }
>>
>> If it works, it likely needs a cleaner approach to handle all cases.
> 
> I don't understand the point. kvm_set_phys_mem() already works without
> this new abort() check.

This new check is there to catch those cases where someone tries to
register regions that are actually incompatible with KVM. IO-MEM regions
do not belong into this category (unless they would split existing KVM
slots in a non-align way), and so the test likely overshoots here.

Jan

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

RE: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Liu Yu-B13201]

 

> -----Original Message-----
> From: Jan Kiszka [mailto:jan.kiszka@siemens.com] 
> Sent: Thursday, April 30, 2009 1:31 AM
> To: Hollis Blanchard
> Cc: Liu Yu-B13201; qemu-devel@nongnu.org; kvm-ppc@vger.kernel.org
> Subject: Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks 
> to slot management
> 
> Hollis Blanchard wrote:
> > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> >> Liu Yu-B13201 wrote:
> >>>> -----Original Message-----
> >>>> From: qemu-devel-bounces+yu.liu=freescale.com@nongnu.org 
> >>>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@nongnu.org] 
> >>>> On Behalf Of Jan Kiszka
> >>>> Sent: Sunday, April 12, 2009 1:20 AM
> >>>> To: qemu-devel@nongnu.org
> >>>> Subject: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to 
> >>>> slot management
> >>>>
> >>>> Fail loudly if we run out of memory slot.
> >>>>
> >>>> Make sure that dirty log start/stop works with consistent 
> >>>> memory regions
> >>>> by reporting invalid parameters. This reveals several 
> >>>> inconsistencies in
> >>>> the vga code, patch to fix them follows later in this series.
> >>>>
> >>>> And, for simplicity reasons, also catch and report 
> unaligned memory
> >>>> regions passed to kvm_set_phys_mem (KVM works on page basis).
> >>>>
> >>> Commit d3f8d37fe2d0c24ec8bac9c94d5b0e2dc09c0d2a hurts kvm/powerpc
> >>> The alignment check in kvm_set_phys_mem prevents pci 
> controller and mpic initializing mmio regions.
> >> What is the alignment of those regions then? None? And do 
> regions of
> >> different types overlap even on the same page? Maybe the 
> check reveals
> >> some deeper conflict /wrt KVM. Can you point me to the 
> involved code files?
> > 
> > These PCI controllers make separate calls to
> > cpu_register_physical_memory() for separate callbacks. Reading
> > ppce500_pci_init(), for example:
> > 0xe0008000 -> CFGADDR (4 bytes)
> > 0xe0008004 -> CFGDATA (4 bytes)
> > 0xe0008c00 -> other registers
> > 
> > The loop in cpu_register_physical_memory_offset() handles "subpage"
> > registration. However, kvm_set_phys_mem() is called outside 
> that loop,
> > so it gets the non-page-aligned addresses.
> > 
> 
> Half-blind shot:
> 
> diff --git a/kvm-all.c b/kvm-all.c
> index 32cd636..c2c760e 100644
> --- a/kvm-all.c
> +++ b/kvm-all.c
> @@ -583,6 +583,9 @@ void kvm_set_phys_mem(target_phys_addr_t 
> start_addr,
>      int err;
> 
>      if (start_addr & ~TARGET_PAGE_MASK) {
> +        if (flags >= IO_MEM_UNASSIGNED) {
> +            return;
> +        }
>          fprintf(stderr, "Only page-aligned memory slots 
> supported\n");
>          abort();
>      }
> 
> If it works, it likely needs a cleaner approach to handle all cases.
> 

It works for me.

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Anthony Liguori]

Hollis Blanchard wrote:
> On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>   
>> What is the alignment of those regions then? None? And do regions of
>> different types overlap even on the same page? Maybe the check reveals
>> some deeper conflict /wrt KVM. Can you point me to the involved code files?
>>     
>
> These PCI controllers make separate calls to
> cpu_register_physical_memory() for separate callbacks. Reading
> ppce500_pci_init(), for example:
> 0xe0008000 -> CFGADDR (4 bytes)
> 0xe0008004 -> CFGDATA (4 bytes)
> 0xe0008c00 -> other registers
>   

That's goofy.  If the single device owns the entire region, it should 
region the entire region instead of relying on subpage functionality.

If just requires a switch() on the address to dispatch to the 
appropriate functions.  It should be easy enough to fix.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Hollis Blanchard]

On Wed, 2009-04-29 at 12:38 -0500, Anthony Liguori wrote:
> Hollis Blanchard wrote:
> > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
> >   
> >> What is the alignment of those regions then? None? And do regions of
> >> different types overlap even on the same page? Maybe the check reveals
> >> some deeper conflict /wrt KVM. Can you point me to the involved code files?
> >>     
> >
> > These PCI controllers make separate calls to
> > cpu_register_physical_memory() for separate callbacks. Reading
> > ppce500_pci_init(), for example:
> > 0xe0008000 -> CFGADDR (4 bytes)
> > 0xe0008004 -> CFGDATA (4 bytes)
> > 0xe0008c00 -> other registers
> >   
> 
> That's goofy.  If the single device owns the entire region, it should 
> region the entire region instead of relying on subpage functionality.
> 
> If just requires a switch() on the address to dispatch to the 
> appropriate functions.  It should be easy enough to fix.

There are two cases that share this code path:
1) same driver registers multiple regions in the same page
2) different drivers register regions in the same page

This is case 1, and as you say, we could add a switch statement to
handle it. I did not look closely to see how many other callers fall
into this category.

However, are you suggesting that case 2 is also "goofy" and will never
work with KVM? It works in qemu today. As long as case 2 works, case 1
will work too, so why change anything?

-- 
Hollis Blanchard
IBM Linux Technology Center

Re: [Qemu-devel] [PATCH 4/7] kvm: Add sanity checks to slot management

[Blue Swirl]

On 4/29/09, Hollis Blanchard <hollisb@us.ibm.com> wrote:
> On Wed, 2009-04-29 at 12:38 -0500, Anthony Liguori wrote:
>  > Hollis Blanchard wrote:
>  > > On Wed, 2009-04-29 at 12:38 +0200, Jan Kiszka wrote:
>  > >
>  > >> What is the alignment of those regions then? None? And do regions of
>  > >> different types overlap even on the same page? Maybe the check reveals
>  > >> some deeper conflict /wrt KVM. Can you point me to the involved code files?
>  > >>
>  > >
>  > > These PCI controllers make separate calls to
>  > > cpu_register_physical_memory() for separate callbacks. Reading
>  > > ppce500_pci_init(), for example:
>  > > 0xe0008000 -> CFGADDR (4 bytes)
>  > > 0xe0008004 -> CFGDATA (4 bytes)
>  > > 0xe0008c00 -> other registers
>  > >
>  >
>  > That's goofy.  If the single device owns the entire region, it should
>  > region the entire region instead of relying on subpage functionality.
>  >
>  > If just requires a switch() on the address to dispatch to the
>  > appropriate functions.  It should be easy enough to fix.
>
>  There are two cases that share this code path:
>  1) same driver registers multiple regions in the same page
>  2) different drivers register regions in the same page
>
>  This is case 1, and as you say, we could add a switch statement to
>  handle it. I did not look closely to see how many other callers fall
>  into this category.
>
>  However, are you suggesting that case 2 is also "goofy" and will never
>  work with KVM? It works in qemu today. As long as case 2 works, case 1
>  will work too, so why change anything?

I don't see why it would be wrong to register multiple regions within
the same page. It means that you can catch accesses to unassigned
addresses between the regions.

There are two instances of Sparc32 DMA controller, one to serve ESP
and the other for Lance. These are at addresses dma_base and dma_base
+ 16. Before subpage, this was handled with a switch, but now we rely
on the subpage mechanism instead.

Re: [Qemu-devel] [PATCH 0/7] kvm: fix system reset & rework slot management

[Anthony Liguori]

Jan Kiszka wrote:
> This is the promised series to improve KVM's memory slot management.
> Things turned out to be more complicated and ugly than planned due to an
> unfortunate bug in KVM's kernel code, see [1] and patch 5 of this
> series.
>
> The most important impact of this series is that it finally enables
> support for resetting qemu guests in KVM mode. Path 1 and 2 lay the
> groundwork for this, and the slot management rework fixes the remaining
> issues around memory remapping via PAM.
>
> Find the patches also at git://git.kiszka.org/qemu.git queues/kvm
>
> Jan Kiszka (7):
>       kvm: Sync CPU state on reset
>       kvm: Apply SMM-already-initialized workaround on reset
>       kvm: Cleanup unmap condition in kvm_set_phys_mem
>       kvm: Add sanity checks to slot management
>       kvm: improve handling of overlapping slots
>       vga: Fix inconsistent tracking of map_addr
>       vga: Cleanup dirty logging
>
>  hw/acpi.c       |   17 +++--
>  hw/cirrus_vga.c |   17 ----
>  hw/vga.c        |   27 +------
>  hw/vga_int.h    |    1 -
>  kvm-all.c       |  219 +++++++++++++++++++++++++++++++++++++++----------------
>  kvm.h           |    7 +-
>  vl.c            |    2 +
>  7 files changed, 177 insertions(+), 113 deletions(-)
>
> [1] http://permalink.gmane.org/gmane.comp.emulators.kvm.devel/30680
>
>   

I've applied this series but please submit a cleanup to #1.

I decided to just apply this to trunk.  It's a lot of churn for stable 
and I consider KVM experimental for 0.10.x.  If you think it should go 
in stable though, I'm willing to reconsider.

Thanks,

Regards,

Anthony Liguori