[Qemu-devel] [PATCH 1/2] qxl: create slots on post_load in any state

[Qemu-devel] [PATCH 1/2] qxl: create slots on post_load in any state

[Alon Levy]

If we migrate when the device is not in a native state the guest
still believes the slots are created, and will cause operations
that reference the slots, causing a "panic: virtual address out of range"
on the first of them. Easy to see by migrating in vga mode (with
a driver loaded, for instance windows cmd window in full screen mode)
and then exiting vga mode back to native mode will cause said panic.

Fixed by doing the slot recreation unconditionally at post_load, and
using the delta value. Next patch makes sure we actually migrate that
delta.

RHBZ: 740547

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   14 ++++++++------
 1 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 03848ed..4e9f39f 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1684,6 +1684,14 @@ static int qxl_post_load(void *opaque, int version)
         qxl_mode_to_string(d->mode));
     newmode = d->mode;
     d->mode = QXL_MODE_UNDEFINED;
+    for (i = 0; i < NUM_MEMSLOTS; i++) {
+        if (!d->guest_slots[i].active) {
+            continue;
+        }
+        dprint(d, 1, "%s: restoring guest slot %d delta %"PRIu64"\n",
+               __func__, i, d->guest_slots[i].delta);
+        qxl_add_memslot(d, i, d->guest_slots[i].delta, QXL_SYNC);
+    }
     switch (newmode) {
     case QXL_MODE_UNDEFINED:
         break;
@@ -1691,12 +1699,6 @@ static int qxl_post_load(void *opaque, int version)
         qxl_enter_vga_mode(d);
         break;
     case QXL_MODE_NATIVE:
-        for (i = 0; i < NUM_MEMSLOTS; i++) {
-            if (!d->guest_slots[i].active) {
-                continue;
-            }
-            qxl_add_memslot(d, i, 0, QXL_SYNC);
-        }
         qxl_create_guest_primary(d, 1, QXL_SYNC);
 
         /* replay surface-create and cursor-set commands */
-- 
1.7.6.4

[Qemu-devel] [PATCH 2/2] qxl: migrate delta in memslot

[Alon Levy]

Adds a subsection to do the migration. Doesn't update the version of
vmstate. Delta is required to recreate the mem slots if delta!=0, which
happens for a driver using QXL_IO_SET_MODE.

RHBZ: 740547

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c |   30 ++++++++++++++++++++++++++++--
 1 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 4e9f39f..e9ba731 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -911,9 +911,9 @@ static void qxl_add_memslot(PCIQXLDevice *d, uint32_t slot_id, uint64_t delta,
     guest_start = le64_to_cpu(d->guest_slots[slot_id].slot.mem_start);
     guest_end   = le64_to_cpu(d->guest_slots[slot_id].slot.mem_end);
 
-    dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 "\n",
+    dprint(d, 1, "%s: slot %d: guest phys 0x%" PRIx64 " - 0x%" PRIx64 " delta 0x%"PRIx64"\n",
            __FUNCTION__, slot_id,
-           guest_start, guest_end);
+           guest_start, guest_end, delta);
 
     PANIC_ON(slot_id >= NUM_MEMSLOTS);
     PANIC_ON(guest_start > guest_end);
@@ -1731,6 +1731,24 @@ static int qxl_post_load(void *opaque, int version)
 
 #define QXL_SAVE_VERSION 21
 
+static bool qxl_memslot_delta_needed(void *opaque)
+{
+    struct guest_slots *s = opaque;
+
+    return s->delta != 0; /* 0 is the default state */
+}
+
+static VMStateDescription qxl_memslot_delta = {
+    .name               = "qxl-memslot-delta",
+    .version_id         = QXL_SAVE_VERSION,
+    .minimum_version_id = QXL_SAVE_VERSION,
+    .minimum_version_id_old = QXL_SAVE_VERSION,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(delta,          struct guest_slots),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static VMStateDescription qxl_memslot = {
     .name               = "qxl-memslot",
     .version_id         = QXL_SAVE_VERSION,
@@ -1740,6 +1758,14 @@ static VMStateDescription qxl_memslot = {
         VMSTATE_UINT64(slot.mem_end,   struct guest_slots),
         VMSTATE_UINT32(active,         struct guest_slots),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection []) {
+        {
+            .vmsd = &qxl_memslot_delta,
+            .needed = qxl_memslot_delta_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 
-- 
1.7.6.4

Re: [Qemu-devel] [PATCH 1/2] qxl: create slots on post_load in any state

[Gerd Hoffmann]

On 10/18/11 17:26, Alon Levy wrote:
> If we migrate when the device is not in a native state the guest
> still believes the slots are created, and will cause operations
> that reference the slots, causing a "panic: virtual address out of range"
> on the first of them. Easy to see by migrating in vga mode (with
> a driver loaded, for instance windows cmd window in full screen mode)
> and then exiting vga mode back to native mode will cause said panic.
>
> Fixed by doing the slot recreation unconditionally at post_load, and
> using the delta value. Next patch makes sure we actually migrate that
> delta.

I don't think we have to save the delta value.  The delta is used only 
in compat mode.  In compat mode the guest doesn't create memory slots, 
instead qxl does that (in qxl_set_mode).

I think you can just skip the slot re-creation in compat mode and let 
qxl_set_mode (which is called later in post_load) handle it.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/2] qxl: create slots on post_load in any state

[Alon Levy]

On Wed, Oct 19, 2011 at 10:28:57AM +0200, Gerd Hoffmann wrote:
> On 10/18/11 17:26, Alon Levy wrote:
> >If we migrate when the device is not in a native state the guest
> >still believes the slots are created, and will cause operations
> >that reference the slots, causing a "panic: virtual address out of range"
> >on the first of them. Easy to see by migrating in vga mode (with
> >a driver loaded, for instance windows cmd window in full screen mode)
> >and then exiting vga mode back to native mode will cause said panic.
> >
> >Fixed by doing the slot recreation unconditionally at post_load, and
> >using the delta value. Next patch makes sure we actually migrate that
> >delta.
> 
> I don't think we have to save the delta value.  The delta is used
> only in compat mode.  In compat mode the guest doesn't create memory
> slots, instead qxl does that (in qxl_set_mode).
> 
> I think you can just skip the slot re-creation in compat mode and
> let qxl_set_mode (which is called later in post_load) handle it.
> 

My bad, missed that. Will resend.

> cheers,
>   Gerd
>