[Qemu-devel] [PATCH 00/12] Fixing hardware migration issues

[Qemu-devel] [PATCH 00/12] Fixing hardware migration issues

[Pavel Dovgalyuk]

This set of patches is related to migration issues in hardware devices.
Some of the devices had fields in their states that didn't saved and restored.
These patches add missed fields to the new subsections of the vmstates.
For several devices (like integratorcp) the patches add new vmstates, that
didn't exist at all.

---

Pavel Dovgalyuk (12):
      integratorcp: adding vmstate for save/restore
      pcspk: adding vmstate for save/restore
      fdc: adding vmstate for save/restore
      parallel: adding vmstate for save/restore
      serial: fixing vmstate for save/restore
      kvmvapic: fixing loading vmstate
      hpet: fixing saving and loading process
      pckbd: adding new fields to vmstate
      rtl8139: adding new fields to vmstate
      piix: do not raise irq while loading vmstate
      mc146818rtc: add missed field to vmstate
      pl031: add missed field to vmstate


 hw/arm/integratorcp.c           |   38 +++++-
 hw/audio/pcspk.c                |   18 ++-
 hw/block/fdc.c                  |   81 ++++++++++++
 hw/char/parallel.c              |   20 +++
 hw/char/serial.c                |  264 ++++++++++++++++++++++++++++++++-------
 hw/i386/kvmvapic.c              |   22 +++
 hw/input/pckbd.c                |   51 ++++++++
 hw/intc/apic_common.c           |   44 +++++++
 hw/net/rtl8139.c                |   50 +++++++
 hw/pci-host/piix.c              |   22 +++
 hw/timer/hpet.c                 |   15 --
 hw/timer/mc146818rtc.c          |   32 +++++
 hw/timer/pl031.c                |    3 
 include/hw/i386/apic_internal.h |    2 
 14 files changed, 594 insertions(+), 68 deletions(-)

-- 
Pavel Dovgalyuk

[Qemu-devel] [PATCH 01/12] integratorcp: adding vmstate for save/restore

[Pavel Dovgalyuk]

VMState added by this patch preserves correct
loading of the integratorcp device state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/arm/integratorcp.c |   38 +++++++++++++++++++++++++++++++++++++-
 1 files changed, 37 insertions(+), 1 deletions(-)

diff --git a/hw/arm/integratorcp.c b/hw/arm/integratorcp.c
index 0e476c3..e5d5751 100644
--- a/hw/arm/integratorcp.c
+++ b/hw/arm/integratorcp.c
@@ -42,6 +42,27 @@ typedef struct IntegratorCMState {
     uint32_t fiq_enabled;
 } IntegratorCMState;
 
+static const VMStateDescription vmstate_integratorcm = {
+    .name = "integratorcm",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(cm_osc, IntegratorCMState),
+        VMSTATE_UINT32(cm_ctrl, IntegratorCMState),
+        VMSTATE_UINT32(cm_lock, IntegratorCMState),
+        VMSTATE_UINT32(cm_auxosc, IntegratorCMState),
+        VMSTATE_UINT32(cm_sdram, IntegratorCMState),
+        VMSTATE_UINT32(cm_init, IntegratorCMState),
+        VMSTATE_UINT32(cm_flags, IntegratorCMState),
+        VMSTATE_UINT32(cm_nvflags, IntegratorCMState),
+        VMSTATE_UINT32(int_level, IntegratorCMState),
+        VMSTATE_UINT32(irq_enabled, IntegratorCMState),
+        VMSTATE_UINT32(fiq_enabled, IntegratorCMState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static uint8_t integrator_spd[128] = {
    128, 8, 4, 11, 9, 1, 64, 0,  2, 0xa0, 0xa0, 0, 0, 8, 0, 1,
    0xe, 4, 0x1c, 1, 2, 0x20, 0xc0, 0, 0, 0, 0, 0x30, 0x28, 0x30, 0x28, 0x40
@@ -272,7 +293,7 @@ static int integratorcm_init(SysBusDevice *dev)
     sysbus_init_mmio(dev, &s->iomem);
 
     integratorcm_do_remap(s);
-    /* ??? Save/restore.  */
+    vmstate_register(NULL, -1, &vmstate_integratorcm, s);
     return 0;
 }
 
@@ -296,6 +317,20 @@ typedef struct icp_pic_state {
     qemu_irq parent_fiq;
 } icp_pic_state;
 
+
+static const VMStateDescription vmstate_icp_pic = {
+    .name = "icp_pic_state",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT32(level, icp_pic_state),
+        VMSTATE_UINT32(irq_enabled, icp_pic_state),
+        VMSTATE_UINT32(fiq_enabled, icp_pic_state),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void icp_pic_update(icp_pic_state *s)
 {
     uint32_t flags;
@@ -399,6 +434,7 @@ static int icp_pic_init(SysBusDevice *sbd)
     memory_region_init_io(&s->iomem, OBJECT(s), &icp_pic_ops, s,
                           "icp-pic", 0x00800000);
     sysbus_init_mmio(sbd, &s->iomem);
+    vmstate_register(NULL, -1, &vmstate_icp_pic, s);
     return 0;
 }
 

[Qemu-devel] [PATCH 02/12] pcspk: adding vmstate for save/restore

[Pavel Dovgalyuk]

VMState added by this patch preserves correct
loading of the PC speaker device state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/audio/pcspk.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 1d81bbe..8b22dbf 100644
--- a/hw/audio/pcspk.c
+++ b/hw/audio/pcspk.c
@@ -50,8 +50,8 @@ typedef struct {
     unsigned int pit_count;
     unsigned int samples;
     unsigned int play_pos;
-    int data_on;
-    int dummy_refresh_clock;
+    uint8_t data_on;
+    uint8_t dummy_refresh_clock;
 } PCSpkState;
 
 static const char *s_spk = "pcspk";
@@ -163,6 +163,18 @@ static const MemoryRegionOps pcspk_io_ops = {
     },
 };
 
+static const VMStateDescription vmstate_spk = {
+    .name = "pcspk",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(data_on, PCSpkState),
+        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void pcspk_initfn(Object *obj)
 {
     PCSpkState *s = PC_SPEAKER(obj);
@@ -175,6 +187,8 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp)
     ISADevice *isadev = ISA_DEVICE(dev);
     PCSpkState *s = PC_SPEAKER(dev);
 
+    vmstate_register(NULL, 0, &vmstate_spk, s);
+
     isa_register_ioport(isadev, &s->ioport, s->iobase);
 
     pcspk_state = s;

[Qemu-devel] [PATCH 03/12] fdc: adding vmstate for save/restore

[Pavel Dovgalyuk]

VMState added by this patch preserves correct
loading of the FDC device state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/block/fdc.c |   81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 81 insertions(+), 0 deletions(-)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 490d127..a10c458 100644
--- a/hw/block/fdc.c
+++ b/hw/block/fdc.c
@@ -695,10 +695,34 @@ static const VMStateDescription vmstate_fdrive_media_rate = {
     }
 };
 
+static bool fdrive_perpendicular_needed(void *opaque)
+{
+    FDrive *drive = opaque;
+
+    return drive->perpendicular != 0;
+}
+
+static const VMStateDescription vmstate_fdrive_perpendicular = {
+    .name = "fdrive/perpendicular",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(perpendicular, FDrive),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static int fdrive_post_load(void *opaque, int version_id)
+{
+    fd_revalidate(opaque);
+    return 0;
+}
+
 static const VMStateDescription vmstate_fdrive = {
     .name = "fdrive",
     .version_id = 1,
     .minimum_version_id = 1,
+    .post_load = fdrive_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(head, FDrive),
         VMSTATE_UINT8(track, FDrive),
@@ -713,6 +737,9 @@ static const VMStateDescription vmstate_fdrive = {
             .vmsd = &vmstate_fdrive_media_rate,
             .needed = &fdrive_media_rate_needed,
         } , {
+            .vmsd = &vmstate_fdrive_perpendicular,
+            .needed = &fdrive_perpendicular_needed,
+        } , {
             /* empty */
         }
     }
@@ -725,6 +752,14 @@ static void fdc_pre_save(void *opaque)
     s->dor_vmstate = s->dor | GET_CUR_DRV(s);
 }
 
+static int fdc_pre_load(void *opaque)
+{
+    FDCtrl *s = opaque;
+    s->reset_sensei = 0;
+    timer_del(s->result_timer);
+    return 0;
+}
+
 static int fdc_post_load(void *opaque, int version_id)
 {
     FDCtrl *s = opaque;
@@ -734,11 +769,46 @@ static int fdc_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool fdc_reset_sensei_needed(void *opaque)
+{
+    FDCtrl *s = opaque;
+
+    return s->reset_sensei != 0;
+}
+
+static const VMStateDescription vmstate_fdc_reset_sensei = {
+    .name = "fdc/reset_sensei",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(reset_sensei, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool fdc_result_timer_needed(void *opaque)
+{
+    FDCtrl *s = opaque;
+
+    return timer_pending(s->result_timer);
+}
+
+static const VMStateDescription vmstate_fdc_result_timer = {
+    .name = "fdc/result_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER(result_timer, FDCtrl),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_fdc = {
     .name = "fdc",
     .version_id = 2,
     .minimum_version_id = 2,
     .pre_save = fdc_pre_save,
+    .pre_load = fdc_pre_load,
     .post_load = fdc_post_load,
     .fields = (VMStateField[]) {
         /* Controller State */
@@ -770,6 +840,17 @@ static const VMStateDescription vmstate_fdc = {
         VMSTATE_STRUCT_ARRAY(drives, FDCtrl, MAX_FD, 1,
                              vmstate_fdrive, FDrive),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_fdc_reset_sensei,
+            .needed = fdc_reset_sensei_needed,
+        } , {
+            .vmsd = &vmstate_fdc_result_timer,
+            .needed = fdc_result_timer_needed,
+        } , {
+            /* empty */
+        }
     }
 };
 

[Qemu-devel] [PATCH 04/12] parallel: adding vmstate for save/restore

[Pavel Dovgalyuk]

VMState added by this patch preserves correct
loading of the parallel port controller state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/char/parallel.c |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 7ac90a5..26c03d7 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -477,6 +477,24 @@ static const MemoryRegionPortio isa_parallel_portio_sw_list[] = {
     PORTIO_END_OF_LIST(),
 };
 
+
+static const VMStateDescription vmstate_parallel_isa = {
+    .name = "parallel_isa",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .minimum_version_id_old = 1,
+    .fields      = (VMStateField[]) {
+        VMSTATE_UINT8(state.dataw, ISAParallelState),
+        VMSTATE_UINT8(state.datar, ISAParallelState),
+        VMSTATE_UINT8(state.status, ISAParallelState),
+        VMSTATE_UINT8(state.control, ISAParallelState),
+        VMSTATE_INT32(state.irq_pending, ISAParallelState),
+        VMSTATE_INT32(state.epp_timeout, ISAParallelState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+
 static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
 {
     static int index;
@@ -518,6 +536,8 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
                               ? &isa_parallel_portio_hw_list[0]
                               : &isa_parallel_portio_sw_list[0]),
                              s, "parallel");
+
+    vmstate_register(NULL, -1, &vmstate_parallel_isa, isa);
 }
 
 /* Memory mapped interface */

[Qemu-devel] [PATCH 05/12] serial: fixing vmstate for save/restore

[Pavel Dovgalyuk]

Some fields were added to VMState by this patch to preserve correct
loading of the serial port controller state.
Updating FCR value while loading was also modified to disable generating
an interrupt by loadvm.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/char/serial.c |  264 +++++++++++++++++++++++++++++++++++++++++++++---------
 1 files changed, 219 insertions(+), 45 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 764e184..192f9a7 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -272,6 +272,64 @@ static gboolean serial_xmit(GIOChannel *chan, GIOCondition cond, void *opaque)
 }
 
 
+/* Setter for FCR.
+   is_load flag means, that value is set while loading VM state
+   and interrupt should not be invoked */
+static void serial_write_fcr(void *opaque, uint32_t val, int is_load)
+{
+    SerialState *s = opaque;
+    val = val & 0xFF;
+
+    if (s->fcr == val) {
+        return;
+    }
+
+    /* Did the enable/disable flag change? If so, make sure FIFOs get flushed */
+    if ((val ^ s->fcr) & UART_FCR_FE) {
+        val |= UART_FCR_XFR | UART_FCR_RFR;
+    }
+
+    /* FIFO clear */
+
+    if (val & UART_FCR_RFR) {
+        timer_del(s->fifo_timeout_timer);
+        s->timeout_ipending = 0;
+        fifo8_reset(&s->recv_fifo);
+    }
+
+    if (val & UART_FCR_XFR) {
+        fifo8_reset(&s->xmit_fifo);
+    }
+
+    if (val & UART_FCR_FE) {
+        s->iir |= UART_IIR_FE;
+        /* Set recv_fifo trigger Level */
+        switch (val & 0xC0) {
+        case UART_FCR_ITL_1:
+            s->recv_fifo_itl = 1;
+            break;
+        case UART_FCR_ITL_2:
+            s->recv_fifo_itl = 4;
+            break;
+        case UART_FCR_ITL_3:
+            s->recv_fifo_itl = 8;
+            break;
+        case UART_FCR_ITL_4:
+            s->recv_fifo_itl = 14;
+            break;
+        }
+    } else {
+        s->iir &= ~UART_IIR_FE;
+    }
+
+    /* Set fcr - or at least the bits in it that are supposed to "stick" */
+    s->fcr = val & 0xC9;
+
+    if (!is_load) {
+        serial_update_irq(s);
+    }
+}
+
 static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
                                 unsigned size)
 {
@@ -327,50 +385,7 @@ static void serial_ioport_write(void *opaque, hwaddr addr, uint64_t val,
         }
         break;
     case 2:
-        val = val & 0xFF;
-
-        if (s->fcr == val)
-            break;
-
-        /* Did the enable/disable flag change? If so, make sure FIFOs get flushed */
-        if ((val ^ s->fcr) & UART_FCR_FE)
-            val |= UART_FCR_XFR | UART_FCR_RFR;
-
-        /* FIFO clear */
-
-        if (val & UART_FCR_RFR) {
-            timer_del(s->fifo_timeout_timer);
-            s->timeout_ipending=0;
-            fifo8_reset(&s->recv_fifo);
-        }
-
-        if (val & UART_FCR_XFR) {
-            fifo8_reset(&s->xmit_fifo);
-        }
-
-        if (val & UART_FCR_FE) {
-            s->iir |= UART_IIR_FE;
-            /* Set recv_fifo trigger Level */
-            switch (val & 0xC0) {
-            case UART_FCR_ITL_1:
-                s->recv_fifo_itl = 1;
-                break;
-            case UART_FCR_ITL_2:
-                s->recv_fifo_itl = 4;
-                break;
-            case UART_FCR_ITL_3:
-                s->recv_fifo_itl = 8;
-                break;
-            case UART_FCR_ITL_4:
-                s->recv_fifo_itl = 14;
-                break;
-            }
-        } else
-            s->iir &= ~UART_IIR_FE;
-
-        /* Set fcr - or at least the bits in it that are supposed to "stick" */
-        s->fcr = val & 0xC9;
-        serial_update_irq(s);
+        serial_write_fcr(s, val, 0);
         break;
     case 3:
         {
@@ -590,6 +605,17 @@ static void serial_pre_save(void *opaque)
     s->fcr_vmstate = s->fcr;
 }
 
+static int serial_pre_load(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    s->thr_ipending = -1;
+    timer_del(s->fifo_timeout_timer);
+    s->timeout_ipending = 0;
+    s->poll_msl = -1;
+    timer_del(s->modem_status_poll);
+    return 0;
+}
+
 static int serial_post_load(void *opaque, int version_id)
 {
     SerialState *s = opaque;
@@ -597,17 +623,139 @@ static int serial_post_load(void *opaque, int version_id)
     if (version_id < 3) {
         s->fcr_vmstate = 0;
     }
+    if (s->thr_ipending == -1) {
+        s->thr_ipending = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
+    }
+    s->last_break_enable = (s->lcr >> 6) & 1;
     /* Initialize fcr via setter to perform essential side-effects */
-    serial_ioport_write(s, 0x02, s->fcr_vmstate, 1);
+    serial_write_fcr(s, s->fcr_vmstate, 1);
     serial_update_parameters(s);
     return 0;
 }
 
+static bool serial_thr_ipending_needed(void *opaque)
+{
+    SerialState *s = opaque;
+    bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
+    return s->thr_ipending != expected_value;
+}
+
+const VMStateDescription vmstate_serial_thr_ipending = {
+    .name = "serial/thr_ipending",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(thr_ipending, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_tsr_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return s->tsr_retry != 0;
+}
+
+const VMStateDescription vmstate_serial_tsr = {
+    .name = "serial/tsr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(tsr_retry, SerialState),
+        VMSTATE_UINT8(thr, SerialState),
+        VMSTATE_UINT8(tsr, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_recv_fifo_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return !fifo8_is_empty(&s->recv_fifo);
+
+}
+
+const VMStateDescription vmstate_serial_recv_fifo = {
+    .name = "serial/recv_fifo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(recv_fifo, SerialState, 1, vmstate_fifo8, Fifo8),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_xmit_fifo_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return !fifo8_is_empty(&s->xmit_fifo);
+}
+
+const VMStateDescription vmstate_serial_xmit_fifo = {
+    .name = "serial/xmit_fifo",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_STRUCT(xmit_fifo, SerialState, 1, vmstate_fifo8, Fifo8),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_fifo_timeout_timer_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return timer_pending(s->fifo_timeout_timer);
+}
+
+const VMStateDescription vmstate_serial_fifo_timeout_timer = {
+    .name = "serial/fifo_timeout_timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER(fifo_timeout_timer, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_timeout_ipending_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return s->timeout_ipending != 0;
+}
+
+const VMStateDescription vmstate_serial_timeout_ipending = {
+    .name = "serial/timeout_ipending",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(timeout_ipending, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool serial_poll_needed(void *opaque)
+{
+    SerialState *s = (SerialState *)opaque;
+    return s->poll_msl >= 0;
+}
+
+const VMStateDescription vmstate_serial_poll = {
+    .name = "serial/poll",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(poll_msl, SerialState),
+        VMSTATE_TIMER(modem_status_poll, SerialState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 const VMStateDescription vmstate_serial = {
     .name = "serial",
     .version_id = 3,
     .minimum_version_id = 2,
     .pre_save = serial_pre_save,
+    .pre_load = serial_pre_load,
     .post_load = serial_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT16_V(divider, SerialState, 2),
@@ -621,6 +769,32 @@ const VMStateDescription vmstate_serial = {
         VMSTATE_UINT8(scr, SerialState),
         VMSTATE_UINT8_V(fcr_vmstate, SerialState, 3),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_serial_thr_ipending,
+            .needed = &serial_thr_ipending_needed,
+        } , {
+            .vmsd = &vmstate_serial_tsr,
+            .needed = &serial_tsr_needed,
+        } , {
+            .vmsd = &vmstate_serial_recv_fifo,
+            .needed = &serial_recv_fifo_needed,
+        } , {
+            .vmsd = &vmstate_serial_xmit_fifo,
+            .needed = &serial_xmit_fifo_needed,
+        } , {
+            .vmsd = &vmstate_serial_fifo_timeout_timer,
+            .needed = &serial_fifo_timeout_timer_needed,
+        } , {
+            .vmsd = &vmstate_serial_timeout_ipending,
+            .needed = &serial_timeout_ipending_needed,
+        } , {
+            .vmsd = &vmstate_serial_poll,
+            .needed = &serial_poll_needed,
+        } , {
+            /* empty */
+        }
     }
 };
 

[Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Pavel Dovgalyuk]

vapic state should not be synchronized with APIC while loading,
because APIC state could be not loaded yet at that moment.
We just save vapic_paddr in APIC VMState instead of synchronization.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/i386/kvmvapic.c              |   22 +++++++++++++++++++-
 hw/intc/apic_common.c           |   44 +++++++++++++++++++++++++++++++++++++++
 include/hw/i386/apic_internal.h |    2 +-
 3 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index ee95963..3c403c5 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -351,6 +351,24 @@ static int get_kpcr_number(X86CPU *cpu)
     return kpcr.number;
 }
 
+static int vapic_enable_post_load(VAPICROMState *s, X86CPU *cpu)
+{
+    int cpu_number = get_kpcr_number(cpu);
+    hwaddr vapic_paddr;
+    static const uint8_t enabled = 1;
+
+    if (cpu_number < 0) {
+        return -1;
+    }
+    vapic_paddr = s->vapic_paddr +
+        (((hwaddr)cpu_number) << VAPIC_CPU_SHIFT);
+    cpu_physical_memory_rw(vapic_paddr + offsetof(VAPICState, enabled),
+                           (void *)&enabled, sizeof(enabled), 1);
+    s->state = VAPIC_ACTIVE;
+
+    return 0;
+}
+
 static int vapic_enable(VAPICROMState *s, X86CPU *cpu)
 {
     int cpu_number = get_kpcr_number(cpu);
@@ -731,7 +749,9 @@ static void do_vapic_enable(void *data)
     VAPICROMState *s = data;
     X86CPU *cpu = X86_CPU(first_cpu);
 
-    vapic_enable(s, cpu);
+    /* Do not synchronize with APIC, because it was not loaded yet.
+       Just call the enable function which does not have synchronization. */
+    vapic_enable_post_load(s, cpu);
 }
 
 static int vapic_post_load(void *opaque, int version_id)
diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index ce3d903..8d672bd 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -345,6 +345,39 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static bool apic_common_sipi_needed(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    return s->wait_for_sipi != 0;
+}
+
+static const VMStateDescription vmstate_apic_common_sipi = {
+    .name = "apic_sipi",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT32(sipi_vector, APICCommonState),
+        VMSTATE_INT32(wait_for_sipi, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool apic_common_vapic_paddr_needed(void *opaque)
+{
+    APICCommonState *s = APIC_COMMON(opaque);
+    return s->vapic_paddr != 0;
+}
+
+static const VMStateDescription vmstate_apic_common_vapic_paddr = {
+    .name = "apic_vapic_paddr",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT64(vapic_paddr, APICCommonState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static const VMStateDescription vmstate_apic_common = {
     .name = "apic",
     .version_id = 3,
@@ -375,6 +408,17 @@ static const VMStateDescription vmstate_apic_common = {
         VMSTATE_INT64(timer_expiry,
                       APICCommonState), /* open-coded timer state */
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_apic_common_sipi,
+            .needed = apic_common_sipi_needed,
+        },
+        {
+            .vmsd = &vmstate_apic_common_vapic_paddr,
+            .needed = apic_common_vapic_paddr_needed,
+        },
+        VMSTATE_END_OF_LIST()
     }
 };
 
diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
index 83e2a42..df4381c 100644
--- a/include/hw/i386/apic_internal.h
+++ b/include/hw/i386/apic_internal.h
@@ -124,7 +124,7 @@ struct APICCommonState {
 
     uint32_t vapic_control;
     DeviceState *vapic;
-    hwaddr vapic_paddr; /* note: persistence via kvmvapic */
+    hwaddr vapic_paddr;
 };
 
 typedef struct VAPICState {

[Qemu-devel] [PATCH 07/12] hpet: fixing saving and loading process

[Pavel Dovgalyuk]

VM clock does not run while saving, so there is no need for saving the ticks
in HPET. Also added saving of hpet_offset field.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/timer/hpet.c |   15 ++-------------
 1 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/hw/timer/hpet.c b/hw/timer/hpet.c
index e160e8f..4cdda64 100644
--- a/hw/timer/hpet.c
+++ b/hw/timer/hpet.c
@@ -222,14 +222,6 @@ static void update_irq(struct HPETTimer *timer, int set)
     }
 }
 
-static void hpet_pre_save(void *opaque)
-{
-    HPETState *s = opaque;
-
-    /* save current counter value */
-    s->hpet_counter = hpet_get_ticks(s);
-}
-
 static int hpet_pre_load(void *opaque)
 {
     HPETState *s = opaque;
@@ -255,9 +247,6 @@ static int hpet_post_load(void *opaque, int version_id)
 {
     HPETState *s = opaque;
 
-    /* Recalculate the offset between the main counter and guest time */
-    s->hpet_offset = ticks_to_ns(s->hpet_counter) - qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
-
     /* Push number of timers into capability returned via HPET_ID */
     s->capability &= ~HPET_ID_NUM_TIM_MASK;
     s->capability |= (s->num_timers - 1) << HPET_ID_NUM_TIM_SHIFT;
@@ -306,15 +295,15 @@ static const VMStateDescription vmstate_hpet_timer = {
 
 static const VMStateDescription vmstate_hpet = {
     .name = "hpet",
-    .version_id = 2,
+    .version_id = 3,
     .minimum_version_id = 1,
-    .pre_save = hpet_pre_save,
     .pre_load = hpet_pre_load,
     .post_load = hpet_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT64(config, HPETState),
         VMSTATE_UINT64(isr, HPETState),
         VMSTATE_UINT64(hpet_counter, HPETState),
+        VMSTATE_UINT64_V(hpet_offset, HPETState, 3),
         VMSTATE_UINT8_V(num_timers, HPETState, 2),
         VMSTATE_VALIDATE("num_timers in range", hpet_validate_num_timers),
         VMSTATE_STRUCT_VARRAY_UINT8(timer, HPETState, num_timers, 0,

[Qemu-devel] [PATCH 08/12] pckbd: adding new fields to vmstate

[Pavel Dovgalyuk]

This patch adds outport to VMState to allow correct saving and restoring
the state of PC keyboard controller.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/input/pckbd.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 51 insertions(+), 0 deletions(-)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index ca1cffc..95d8767 100644
--- a/hw/input/pckbd.c
+++ b/hw/input/pckbd.c
@@ -131,6 +131,7 @@ typedef struct KBDState {
     uint8_t status;
     uint8_t mode;
     uint8_t outport;
+    bool outport_present;
     /* Bitmask of devices with data available.  */
     uint8_t pending;
     void *kbd;
@@ -367,18 +368,68 @@ static void kbd_reset(void *opaque)
     s->mode = KBD_MODE_KBD_INT | KBD_MODE_MOUSE_INT;
     s->status = KBD_STAT_CMD | KBD_STAT_UNLOCKED;
     s->outport = KBD_OUT_RESET | KBD_OUT_A20;
+    s->outport_present = false;
+}
+
+static uint8_t kbd_outport_default(KBDState *s)
+{
+    return KBD_OUT_RESET | KBD_OUT_A20
+           | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
+           | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);
+}
+
+static int kbd_outport_post_load(void *opaque, int version_id)
+{
+    KBDState *s = opaque;
+    s->outport_present = true;
+    return 0;
+}
+
+static const VMStateDescription vmstate_kbd_outport = {
+    .name = "pckbd_outport",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .post_load = kbd_outport_post_load,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT8(outport, KBDState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool kbd_outport_needed(void *opaque)
+{
+    KBDState *s = opaque;
+    return s->outport != kbd_outport_default(s);
+}
+
+static int kbd_post_load(void *opaque, int version_id)
+{
+    KBDState *s = opaque;
+    if (!s->outport_present) {
+        s->outport = kbd_outport_default(s);
+    }
+    s->outport_present = false;
+    return 0;
 }
 
 static const VMStateDescription vmstate_kbd = {
     .name = "pckbd",
     .version_id = 3,
     .minimum_version_id = 3,
+    .post_load = kbd_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(write_cmd, KBDState),
         VMSTATE_UINT8(status, KBDState),
         VMSTATE_UINT8(mode, KBDState),
         VMSTATE_UINT8(pending, KBDState),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_kbd_outport,
+            .needed = kbd_outport_needed,
+        },
+        VMSTATE_END_OF_LIST()
     }
 };
 

[Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Pavel Dovgalyuk]

This patch adds virtual clock-dependent timers to VMState to allow correct
saving and restoring the state of RTL8139 network controller.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/net/rtl8139.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 48 insertions(+), 2 deletions(-)

diff --git a/hw/net/rtl8139.c b/hw/net/rtl8139.c
index 6e59f38..cc2f705 100644
--- a/hw/net/rtl8139.c
+++ b/hw/net/rtl8139.c
@@ -3246,10 +3246,17 @@ static uint32_t rtl8139_mmio_readl(void *opaque, hwaddr addr)
     return val;
 }
 
+static int rtl8139_pre_load(void *opaque)
+{
+    RTL8139State *s = opaque;
+    s->TimerExpire = 0;
+    timer_del(s->timer);
+    return 0;
+}
+
 static int rtl8139_post_load(void *opaque, int version_id)
 {
     RTL8139State* s = opaque;
-    rtl8139_set_next_tctr_time(s, qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL));
     if (version_id < 4) {
         s->cplus_enabled = s->CpCmd != 0;
     }
@@ -3275,6 +3282,38 @@ static const VMStateDescription vmstate_rtl8139_hotplug_ready ={
     }
 };
 
+static bool rtl8139_TimerExpire_needed(void *opaque)
+{
+    RTL8139State *s = (RTL8139State *)opaque;
+    return s->TimerExpire != 0;
+}
+
+static const VMStateDescription vmstate_rtl8139_TimerExpire = {
+    .name = "rtl8139/TimerExpire",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_INT64(TimerExpire, RTL8139State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool rtl8139_timer_needed(void *opaque)
+{
+    RTL8139State *s = (RTL8139State *)opaque;
+    return timer_pending(s->timer);
+}
+
+static const VMStateDescription vmstate_rtl8139_timer = {
+    .name = "rtl8139/timer",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_TIMER(timer, RTL8139State),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
 static void rtl8139_pre_save(void *opaque)
 {
     RTL8139State* s = opaque;
@@ -3289,8 +3328,9 @@ static void rtl8139_pre_save(void *opaque)
 
 static const VMStateDescription vmstate_rtl8139 = {
     .name = "rtl8139",
-    .version_id = 4,
+    .version_id = 5,
     .minimum_version_id = 3,
+    .pre_load = rtl8139_pre_load,
     .post_load = rtl8139_post_load,
     .pre_save  = rtl8139_pre_save,
     .fields = (VMStateField[]) {
@@ -3371,6 +3411,12 @@ static const VMStateDescription vmstate_rtl8139 = {
             .vmsd = &vmstate_rtl8139_hotplug_ready,
             .needed = rtl8139_hotplug_ready_needed,
         }, {
+            .vmsd = &vmstate_rtl8139_TimerExpire,
+            .needed = rtl8139_TimerExpire_needed,
+        }, {
+            .vmsd = &vmstate_rtl8139_timer,
+            .needed = rtl8139_timer_needed,
+        }, {
             /* empty */
         }
     }

[Qemu-devel] [PATCH 10/12] piix: do not raise irq while loading vmstate

[Pavel Dovgalyuk]

This patch disables raising an irq while loading the state of PCI bridge.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/pci-host/piix.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..86d6d20 100644
--- a/hw/pci-host/piix.c
+++ b/hw/pci-host/piix.c
@@ -409,7 +409,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
                      (pic_irq * PIIX_NUM_PIRQS))));
 }
 
-static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
+static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level)
 {
     int pic_irq;
     uint64_t mask;
@@ -422,6 +422,18 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
     mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
     piix3->pic_levels &= ~mask;
     piix3->pic_levels |= mask * !!level;
+}
+
+static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
+{
+    int pic_irq;
+
+    pic_irq = piix3->dev.config[PIIX_PIRQC + pirq];
+    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
+        return;
+    }
+
+    piix3_set_irq_level_internal(piix3, pirq, level);
 
     piix3_set_irq_pic(piix3, pic_irq);
 }
@@ -527,7 +539,13 @@ static void piix3_reset(void *opaque)
 static int piix3_post_load(void *opaque, int version_id)
 {
     PIIX3State *piix3 = opaque;
-    piix3_update_irq_levels(piix3);
+    int pirq;
+
+    piix3->pic_levels = 0;
+    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
+        piix3_set_irq_level_internal(piix3, pirq,
+                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
+    }
     return 0;
 }
 

[Qemu-devel] [PATCH 11/12] mc146818rtc: add missed field to vmstate

[Pavel Dovgalyuk]

This patch adds irq_reinject_on_ack_count field to VMState to allow correct
saving/loading the state of MC146818 RTC.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/timer/mc146818rtc.c |   32 ++++++++++++++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 233fc70..6ba360a 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -698,6 +698,13 @@ int rtc_get_memory(ISADevice *dev, int addr)
     return s->cmos_data[addr];
 }
 
+static int rtc_pre_load(void *opaque)
+{
+    RTCState *s = (RTCState *)opaque;
+    s->irq_reinject_on_ack_count = 0;
+    return 0;
+}
+
 static void rtc_set_date_from_host(ISADevice *dev)
 {
     RTCState *s = MC146818_RTC(dev);
@@ -733,10 +740,27 @@ static int rtc_post_load(void *opaque, int version_id)
     return 0;
 }
 
+static const VMStateDescription vmstate_rtc_irq_reinject_on_ack_count = {
+    .name = "irq_reinject_on_ack_count",
+    .version_id = 1,
+    .minimum_version_id = 1,
+    .fields = (VMStateField[]) {
+        VMSTATE_UINT16(irq_reinject_on_ack_count, RTCState),
+        VMSTATE_END_OF_LIST()
+    }
+};
+
+static bool rtc_irq_reinject_on_ack_count_needed(void *opaque)
+{
+    RTCState *s = (RTCState *)opaque;
+    return s->irq_reinject_on_ack_count != 0;
+}
+
 static const VMStateDescription vmstate_rtc = {
     .name = "mc146818rtc",
     .version_id = 3,
     .minimum_version_id = 1,
+    .pre_load = rtc_pre_load,
     .post_load = rtc_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_BUFFER(cmos_data, RTCState),
@@ -753,6 +777,14 @@ static const VMStateDescription vmstate_rtc = {
         VMSTATE_TIMER_V(update_timer, RTCState, 3),
         VMSTATE_UINT64_V(next_alarm_time, RTCState, 3),
         VMSTATE_END_OF_LIST()
+    },
+    .subsections = (VMStateSubsection[]) {
+        {
+            .vmsd = &vmstate_rtc_irq_reinject_on_ack_count,
+            .needed = rtc_irq_reinject_on_ack_count_needed,
+        }, {
+            /* empty */
+        }
     }
 };
 

[Qemu-devel] [PATCH 12/12] pl031: add missed field to vmstate

[Pavel Dovgalyuk]

This patch adds timer which uses virtual clock to the VMState.
Such timers are required for saving because virtual clock is the part
of the virtual machine state.

Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
---
 hw/timer/pl031.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/hw/timer/pl031.c b/hw/timer/pl031.c
index 34d9b44..f8e5abc 100644
--- a/hw/timer/pl031.c
+++ b/hw/timer/pl031.c
@@ -230,12 +230,13 @@ static int pl031_post_load(void *opaque, int version_id)
 
 static const VMStateDescription vmstate_pl031 = {
     .name = "pl031",
-    .version_id = 1,
+    .version_id = 2,
     .minimum_version_id = 1,
     .pre_save = pl031_pre_save,
     .post_load = pl031_post_load,
     .fields = (VMStateField[]) {
         VMSTATE_UINT32(tick_offset_vmstate, PL031State),
+        VMSTATE_TIMER_V(timer, PL031State, 2),
         VMSTATE_UINT32(mr, PL031State),
         VMSTATE_UINT32(lr, PL031State),
         VMSTATE_UINT32(cr, PL031State),

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Paolo Bonzini]

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> This patch adds virtual clock-dependent timers to VMState to allow correct
> saving and restoring the state of RTL8139 network controller.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/net/rtl8139.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 files changed, 48 insertions(+), 2 deletions(-)

Again, this is only needed in your record/replay system (and you haven't
yet quite explained why the design has this limitation), so it should
not be a part of this series.

I'll review the other patches separately, no need to resubmit.

Paolo

Re: [Qemu-devel] [PATCH 11/12] mc146818rtc: add missed field to vmstate

[Paolo Bonzini]

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> +static int rtc_pre_load(void *opaque)
> +{
> +    RTCState *s = (RTCState *)opaque;
> +    s->irq_reinject_on_ack_count = 0;
> +    return 0;
> +}
> +

You found a real bug here, in that the field has to be cleared at reset
time.  But reinitializing stuff in pre-load hooks is another quirk of
your record/replay patches that does not belong in these patches (and
that you have not justified yet, either).

Paolo

Re: [Qemu-devel] [PATCH 03/12] fdc: adding vmstate for save/restore

[Paolo Bonzini]

Il 26/08/2014 09:14, Pavel Dovgalyuk ha scritto:
> +static int fdc_pre_load(void *opaque)
> +{
> +    FDCtrl *s = opaque;
> +    s->reset_sensei = 0;
> +    timer_del(s->result_timer);
> +    return 0;
> +}

This should be in fdctrl_reset.

Paolo

Re: [Qemu-devel] [PATCH 04/12] parallel: adding vmstate for save/restore

[Paolo Bonzini]

Il 26/08/2014 09:14, Pavel Dovgalyuk ha scritto:
> VMState added by this patch preserves correct
> loading of the parallel port controller state.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/char/parallel.c |   20 ++++++++++++++++++++
>  1 files changed, 20 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index 7ac90a5..26c03d7 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -477,6 +477,24 @@ static const MemoryRegionPortio isa_parallel_portio_sw_list[] = {
>      PORTIO_END_OF_LIST(),
>  };
>  
> +
> +static const VMStateDescription vmstate_parallel_isa = {
> +    .name = "parallel_isa",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8(state.dataw, ISAParallelState),
> +        VMSTATE_UINT8(state.datar, ISAParallelState),
> +        VMSTATE_UINT8(state.status, ISAParallelState),
> +        VMSTATE_UINT8(state.control, ISAParallelState),
> +        VMSTATE_INT32(state.irq_pending, ISAParallelState),
> +        VMSTATE_INT32(state.epp_timeout, ISAParallelState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +
>  static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
>  {
>      static int index;
> @@ -518,6 +536,8 @@ static void parallel_isa_realizefn(DeviceState *dev, Error **errp)
>                                ? &isa_parallel_portio_hw_list[0]
>                                : &isa_parallel_portio_sw_list[0]),
>                               s, "parallel");
> +
> +    vmstate_register(NULL, -1, &vmstate_parallel_isa, isa);
>  }
>  
>  /* Memory mapped interface */

I think you should use dc->vmsd.

Paolo

Re: [Qemu-devel] [PATCH 02/12] pcspk: adding vmstate for save/restore

[Paolo Bonzini]

Il 26/08/2014 09:14, Pavel Dovgalyuk ha scritto:
> VMState added by this patch preserves correct
> loading of the PC speaker device state.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/audio/pcspk.c |   18 ++++++++++++++++--
>  1 files changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
> index 1d81bbe..8b22dbf 100644
> --- a/hw/audio/pcspk.c
> +++ b/hw/audio/pcspk.c
> @@ -50,8 +50,8 @@ typedef struct {
>      unsigned int pit_count;
>      unsigned int samples;
>      unsigned int play_pos;
> -    int data_on;
> -    int dummy_refresh_clock;
> +    uint8_t data_on;
> +    uint8_t dummy_refresh_clock;
>  } PCSpkState;
>  
>  static const char *s_spk = "pcspk";
> @@ -163,6 +163,18 @@ static const MemoryRegionOps pcspk_io_ops = {
>      },
>  };
>  
> +static const VMStateDescription vmstate_spk = {
> +    .name = "pcspk",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .minimum_version_id_old = 1,
> +    .fields      = (VMStateField[]) {
> +        VMSTATE_UINT8(data_on, PCSpkState),
> +        VMSTATE_UINT8(dummy_refresh_clock, PCSpkState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static void pcspk_initfn(Object *obj)
>  {
>      PCSpkState *s = PC_SPEAKER(obj);
> @@ -175,6 +187,8 @@ static void pcspk_realizefn(DeviceState *dev, Error **errp)
>      ISADevice *isadev = ISA_DEVICE(dev);
>      PCSpkState *s = PC_SPEAKER(dev);
>  
> +    vmstate_register(NULL, 0, &vmstate_spk, s);
> +
>      isa_register_ioport(isadev, &s->ioport, s->iobase);
>  
>      pcspk_state = s;
> 
> 
> 

I think you should use dc->vmsd.

Paolo

Re: [Qemu-devel] [PATCH 08/12] pckbd: adding new fields to vmstate

[Paolo Bonzini]

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> This patch adds outport to VMState to allow correct saving and restoring
> the state of PC keyboard controller.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/input/pckbd.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 51 insertions(+), 0 deletions(-)
> 
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index ca1cffc..95d8767 100644
> --- a/hw/input/pckbd.c
> +++ b/hw/input/pckbd.c
> @@ -131,6 +131,7 @@ typedef struct KBDState {
>      uint8_t status;
>      uint8_t mode;
>      uint8_t outport;
> +    bool outport_present;
>      /* Bitmask of devices with data available.  */
>      uint8_t pending;
>      void *kbd;
> @@ -367,18 +368,68 @@ static void kbd_reset(void *opaque)
>      s->mode = KBD_MODE_KBD_INT | KBD_MODE_MOUSE_INT;
>      s->status = KBD_STAT_CMD | KBD_STAT_UNLOCKED;
>      s->outport = KBD_OUT_RESET | KBD_OUT_A20;
> +    s->outport_present = false;
> +}
> +
> +static uint8_t kbd_outport_default(KBDState *s)
> +{
> +    return KBD_OUT_RESET | KBD_OUT_A20
> +           | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
> +           | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);
> +}
> +
> +static int kbd_outport_post_load(void *opaque, int version_id)
> +{
> +    KBDState *s = opaque;
> +    s->outport_present = true;
> +    return 0;
> +}
> +
> +static const VMStateDescription vmstate_kbd_outport = {
> +    .name = "pckbd_outport",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .post_load = kbd_outport_post_load,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT8(outport, KBDState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool kbd_outport_needed(void *opaque)
> +{
> +    KBDState *s = opaque;
> +    return s->outport != kbd_outport_default(s);
> +}
> +
> +static int kbd_post_load(void *opaque, int version_id)
> +{
> +    KBDState *s = opaque;
> +    if (!s->outport_present) {
> +        s->outport = kbd_outport_default(s);
> +    }
> +    s->outport_present = false;
> +    return 0;
>  }
>  
>  static const VMStateDescription vmstate_kbd = {
>      .name = "pckbd",
>      .version_id = 3,
>      .minimum_version_id = 3,
> +    .post_load = kbd_post_load,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(write_cmd, KBDState),
>          VMSTATE_UINT8(status, KBDState),
>          VMSTATE_UINT8(mode, KBDState),
>          VMSTATE_UINT8(pending, KBDState),
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            .vmsd = &vmstate_kbd_outport,
> +            .needed = kbd_outport_needed,
> +        },
> +        VMSTATE_END_OF_LIST()
>      }
>  };
>  
> 
> 
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

Re: [Qemu-devel] [PATCH 10/12] piix: do not raise irq while loading vmstate

[Paolo Bonzini]

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> This patch disables raising an irq while loading the state of PCI bridge.
> 
> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/pci-host/piix.c |   22 ++++++++++++++++++++--
>  1 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..86d6d20 100644
> --- a/hw/pci-host/piix.c
> +++ b/hw/pci-host/piix.c
> @@ -409,7 +409,7 @@ static void piix3_set_irq_pic(PIIX3State *piix3, int pic_irq)
>                       (pic_irq * PIIX_NUM_PIRQS))));
>  }
>  
> -static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
> +static void piix3_set_irq_level_internal(PIIX3State *piix3, int pirq, int level)
>  {
>      int pic_irq;
>      uint64_t mask;
> @@ -422,6 +422,18 @@ static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
>      mask = 1ULL << ((pic_irq * PIIX_NUM_PIRQS) + pirq);
>      piix3->pic_levels &= ~mask;
>      piix3->pic_levels |= mask * !!level;
> +}
> +
> +static void piix3_set_irq_level(PIIX3State *piix3, int pirq, int level)
> +{
> +    int pic_irq;
> +
> +    pic_irq = piix3->dev.config[PIIX_PIRQC + pirq];
> +    if (pic_irq >= PIIX_NUM_PIC_IRQS) {
> +        return;
> +    }
> +
> +    piix3_set_irq_level_internal(piix3, pirq, level);
>  
>      piix3_set_irq_pic(piix3, pic_irq);
>  }
> @@ -527,7 +539,13 @@ static void piix3_reset(void *opaque)
>  static int piix3_post_load(void *opaque, int version_id)
>  {
>      PIIX3State *piix3 = opaque;
> -    piix3_update_irq_levels(piix3);
> +    int pirq;
> +
> +    piix3->pic_levels = 0;
> +    for (pirq = 0; pirq < PIIX_NUM_PIRQS; pirq++) {
> +        piix3_set_irq_level_internal(piix3, pirq,
> +                            pci_bus_get_irq_level(piix3->dev.bus, pirq));
> +    }
>      return 0;
>  }
>  
> 
> 
> 

The commit message or (probably better) a comment in the code should
explain why the PIC state must not be updated, that is which side effect
is undesirable.

This would clarify whether the change is useful for migration too, or
just cosmetic outside record/replay.  Unlike other patches, however, I
think this could be acceptable even without record/replay
infrastructure, because it is not going against "accepted" patterns for
migration.

Paolo

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Paolo Bonzini]

Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> vapic state should not be synchronized with APIC while loading,
> because APIC state could be not loaded yet at that moment.
> We just save vapic_paddr in APIC VMState instead of synchronization.

Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
with expiration time in the past (e.g. at time zero) to run the sync
code as soon as possible?  Then you can preserve the current migration
format and avoid using the invalid APIC state.

Paolo

> Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> ---
>  hw/i386/kvmvapic.c              |   22 +++++++++++++++++++-
>  hw/intc/apic_common.c           |   44 +++++++++++++++++++++++++++++++++++++++
>  include/hw/i386/apic_internal.h |    2 +-
>  3 files changed, 66 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index ee95963..3c403c5 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -351,6 +351,24 @@ static int get_kpcr_number(X86CPU *cpu)
>      return kpcr.number;
>  }
>  
> +static int vapic_enable_post_load(VAPICROMState *s, X86CPU *cpu)
> +{
> +    int cpu_number = get_kpcr_number(cpu);
> +    hwaddr vapic_paddr;
> +    static const uint8_t enabled = 1;
> +
> +    if (cpu_number < 0) {
> +        return -1;
> +    }
> +    vapic_paddr = s->vapic_paddr +
> +        (((hwaddr)cpu_number) << VAPIC_CPU_SHIFT);
> +    cpu_physical_memory_rw(vapic_paddr + offsetof(VAPICState, enabled),
> +                           (void *)&enabled, sizeof(enabled), 1);
> +    s->state = VAPIC_ACTIVE;
> +
> +    return 0;
> +}
> +
>  static int vapic_enable(VAPICROMState *s, X86CPU *cpu)
>  {
>      int cpu_number = get_kpcr_number(cpu);
> @@ -731,7 +749,9 @@ static void do_vapic_enable(void *data)
>      VAPICROMState *s = data;
>      X86CPU *cpu = X86_CPU(first_cpu);
>  
> -    vapic_enable(s, cpu);
> +    /* Do not synchronize with APIC, because it was not loaded yet.
> +       Just call the enable function which does not have synchronization. */
> +    vapic_enable_post_load(s, cpu);
>  }
>  
>  static int vapic_post_load(void *opaque, int version_id)
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index ce3d903..8d672bd 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -345,6 +345,39 @@ static int apic_dispatch_post_load(void *opaque, int version_id)
>      return 0;
>  }
>  
> +static bool apic_common_sipi_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->wait_for_sipi != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_sipi = {
> +    .name = "apic_sipi",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_INT32(sipi_vector, APICCommonState),
> +        VMSTATE_INT32(wait_for_sipi, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
> +static bool apic_common_vapic_paddr_needed(void *opaque)
> +{
> +    APICCommonState *s = APIC_COMMON(opaque);
> +    return s->vapic_paddr != 0;
> +}
> +
> +static const VMStateDescription vmstate_apic_common_vapic_paddr = {
> +    .name = "apic_vapic_paddr",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .fields = (VMStateField[]) {
> +        VMSTATE_UINT64(vapic_paddr, APICCommonState),
> +        VMSTATE_END_OF_LIST()
> +    }
> +};
> +
>  static const VMStateDescription vmstate_apic_common = {
>      .name = "apic",
>      .version_id = 3,
> @@ -375,6 +408,17 @@ static const VMStateDescription vmstate_apic_common = {
>          VMSTATE_INT64(timer_expiry,
>                        APICCommonState), /* open-coded timer state */
>          VMSTATE_END_OF_LIST()
> +    },
> +    .subsections = (VMStateSubsection[]) {
> +        {
> +            .vmsd = &vmstate_apic_common_sipi,
> +            .needed = apic_common_sipi_needed,
> +        },
> +        {
> +            .vmsd = &vmstate_apic_common_vapic_paddr,
> +            .needed = apic_common_vapic_paddr_needed,
> +        },
> +        VMSTATE_END_OF_LIST()
>      }
>  };
>  
> diff --git a/include/hw/i386/apic_internal.h b/include/hw/i386/apic_internal.h
> index 83e2a42..df4381c 100644
> --- a/include/hw/i386/apic_internal.h
> +++ b/include/hw/i386/apic_internal.h
> @@ -124,7 +124,7 @@ struct APICCommonState {
>  
>      uint32_t vapic_control;
>      DeviceState *vapic;
> -    hwaddr vapic_paddr; /* note: persistence via kvmvapic */
> +    hwaddr vapic_paddr;
>  };
>  
>  typedef struct VAPICState {
> 
> 
> 

Re: [Qemu-devel] [PATCH 05/12] serial: fixing vmstate for save/restore

[Paolo Bonzini]

Il 26/08/2014 09:14, Pavel Dovgalyuk ha scritto:
> +static int serial_pre_load(void *opaque)
> +{
> +    SerialState *s = (SerialState *)opaque;
> +    s->thr_ipending = -1;
> +    timer_del(s->fifo_timeout_timer);
> +    s->timeout_ipending = 0;
> +    s->poll_msl = -1;
> +    timer_del(s->modem_status_poll);
> +    return 0;
> +}

The two timer_dels and s->timeout_ipending = 0 should be in the reset
function rather than here.  The two assignments of -1 are correct here.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> > This patch adds virtual clock-dependent timers to VMState to allow correct
> > saving and restoring the state of RTL8139 network controller.
> >
> > Signed-off-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>
> > ---
> >  hw/net/rtl8139.c |   50 ++++++++++++++++++++++++++++++++++++++++++++++++--
> >  1 files changed, 48 insertions(+), 2 deletions(-)
> 
> Again, this is only needed in your record/replay system (and you haven't
> yet quite explained why the design has this limitation), so it should
> not be a part of this series.

  I see. Updating s->timer and s->TimerExpire in post_load will be enough?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Paolo Bonzini]

Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
>> > Again, this is only needed in your record/replay system (and you haven't
>> > yet quite explained why the design has this limitation), so it should
>> > not be a part of this series.
>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?

In theory it should be done already, I guess it's not deterministic
enough though.  Have you tried my patch to rewrite the timer stuff?

Paolo

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
> >> > Again, this is only needed in your record/replay system (and you haven't
> >> > yet quite explained why the design has this limitation), so it should
> >> > not be a part of this series.
> >   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
> 
> In theory it should be done already, I guess it's not deterministic
> enough though.

I split existing function into the two parts: one sets irq (and calls another).
And the second part sets only timer and TimerExpire fields. The second function
is also called from post_load.

> Have you tried my patch to rewrite the timer stuff?

What patch do you mean?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Paolo Bonzini]

Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
>>>>> Again, this is only needed in your record/replay system (and you haven't
>>>>> yet quite explained why the design has this limitation), so it should
>>>>> not be a part of this series.
>>>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
>>
>> In theory it should be done already, I guess it's not deterministic
>> enough though.
> 
> I split existing function into the two parts: one sets irq (and calls another).
> And the second part sets only timer and TimerExpire fields. The second function
> is also called from post_load.
> 
>> Have you tried my patch to rewrite the timer stuff?
> 
> What patch do you mean?

This one:

http://article.gmane.org/gmane.comp.emulators.qemu/288521

Paolo

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
> >>>>> Again, this is only needed in your record/replay system (and you haven't
> >>>>> yet quite explained why the design has this limitation), so it should
> >>>>> not be a part of this series.
> >>>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
> >>
> >> In theory it should be done already, I guess it's not deterministic
> >> enough though.
> >
> > I split existing function into the two parts: one sets irq (and calls another).
> > And the second part sets only timer and TimerExpire fields. The second function
> > is also called from post_load.
> >
> >> Have you tried my patch to rewrite the timer stuff?
> >
> > What patch do you mean?
> 
> This one:
> 
> http://article.gmane.org/gmane.comp.emulators.qemu/288521

It will solve the problem, because it removes raising an irq from 
rtl8139_set_next_tctr_time function.

Ok then, I'll remove my rtl8139 patch from the series.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 26/08/2014 09:15, Pavel Dovgalyuk ha scritto:
> > vapic state should not be synchronized with APIC while loading,
> > because APIC state could be not loaded yet at that moment.
> > We just save vapic_paddr in APIC VMState instead of synchronization.
> 
> Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
> with expiration time in the past (e.g. at time zero) to run the sync
> code as soon as possible?  Then you can preserve the current migration
> format and avoid using the invalid APIC state.

Does this method guarantee, that nobody (like other timers) will access
APIC between loading the vmstate and invocation of the timer?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Paolo Bonzini]

Il 27/08/2014 14:16, Pavel Dovgaluk ha scritto:
>> > Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
>> > with expiration time in the past (e.g. at time zero) to run the sync
>> > code as soon as possible?  Then you can preserve the current migration
>> > format and avoid using the invalid APIC state.
> Does this method guarantee, that nobody (like other timers) will access
> APIC between loading the vmstate and invocation of the timer?

Hmm, probably not.  The bug would not be other timers accessing the
APIC, because that would also call apic_sync_vapic and the only effect
would be an extra useless synchronization.  The bug would happen if the
APIC is accessed by the CPU before the timer has the occasion to run.

However, a vm_change_state_handler should work.  It runs before VCPUs
are started.

Paolo

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 14:16, Pavel Dovgaluk ha scritto:
> >> > Can you use a vm_change_state_handler, or a QEMU_CLOCK_VIRTUAL timer
> >> > with expiration time in the past (e.g. at time zero) to run the sync
> >> > code as soon as possible?  Then you can preserve the current migration
> >> > format and avoid using the invalid APIC state.
> > Does this method guarantee, that nobody (like other timers) will access
> > APIC between loading the vmstate and invocation of the timer?
> 
> Hmm, probably not.  The bug would not be other timers accessing the
> APIC, because that would also call apic_sync_vapic and the only effect
> would be an extra useless synchronization.  The bug would happen if the
> APIC is accessed by the CPU before the timer has the occasion to run.

Sorry, but I don't understand which problem we will solve with apic_sync_vapic.
All fields that were added to vmstate are not affected by this function.
Could you explain your idea?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Paolo Bonzini]

Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
>> > Hmm, probably not.  The bug would not be other timers accessing the
>> > APIC, because that would also call apic_sync_vapic and the only effect
>> > would be an extra useless synchronization.  The bug would happen if the
>> > APIC is accessed by the CPU before the timer has the occasion to run.
> Sorry, but I don't understand which problem we will solve with apic_sync_vapic.

In KVM mode, it is not a problem to call apic_enable_vapic before APIC
state is loaded; all vapic processing is delayed anyway to after the
VCPUs are started.

In TCG mode, apic_enable_vapic calls apic_sync_vapic.

Taking inspiration from what KVM does, the fix could be even simpler
than a change state handler.  run_on_cpu functions do not run while the
VM is stopped, so the following should work:

diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
index ce3d903..81d1ad7 100644
--- a/hw/intc/apic_common.c
+++ b/hw/intc/apic_common.c
@@ -91,13 +91,20 @@ void apic_enable_tpr_access_reporting(DeviceState
*dev, bool enable)
     }
 }

+static void do_apic_enable_vapic(void *data)
+{
+    APICCommonState *s = APIC_COMMON(data);
+    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
+
+    info->vapic_base_update(s);
+}
+
 void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
 {
     APICCommonState *s = APIC_COMMON(dev);
-    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);

     s->vapic_paddr = paddr;
-    info->vapic_base_update(s);
+    run_on_cpu(CPU(s->cpu), do_apic_enable_vapic, s);
 }

 void apic_handle_tpr_access_report(DeviceState *dev, target_ulong ip,

> All fields that were added to vmstate are not affected by this function.

The sipi_vector and wait_for_sipi parts of this patch are okay.  You
should split those in a separate patch.

Paolo

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Paolo Bonzini]

Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
>>>>>>> Again, this is only needed in your record/replay system (and you haven't
>>>>>>> yet quite explained why the design has this limitation), so it should
>>>>>>> not be a part of this series.
>>>>>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
>>>>
>>>> In theory it should be done already, I guess it's not deterministic
>>>> enough though.
>>>
>>> I split existing function into the two parts: one sets irq (and calls another).
>>> And the second part sets only timer and TimerExpire fields. The second function
>>> is also called from post_load.
>>>
>>>> Have you tried my patch to rewrite the timer stuff?
>>>
>>> What patch do you mean?
>>
>> This one:
>>
>> http://article.gmane.org/gmane.comp.emulators.qemu/288521
> 
> It will solve the problem, because it removes raising an irq from 
> rtl8139_set_next_tctr_time function.
> 
> Ok then, I'll remove my rtl8139 patch from the series.

Please include that patch in your record/replay work though.  I'm not
sure if I'll have time to test it and submit it.

Paolo

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto:
> >> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto:
> >>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> >>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
> >>>>>>> Again, this is only needed in your record/replay system (and you haven't
> >>>>>>> yet quite explained why the design has this limitation), so it should
> >>>>>>> not be a part of this series.
> >>>>>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
> >>>>
> >>>> In theory it should be done already, I guess it's not deterministic
> >>>> enough though.
> >>>
> >>> I split existing function into the two parts: one sets irq (and calls another).
> >>> And the second part sets only timer and TimerExpire fields. The second function
> >>> is also called from post_load.
> >>>
> >>>> Have you tried my patch to rewrite the timer stuff?
> >>>
> >>> What patch do you mean?
> >>
> >> This one:
> >>
> >> http://article.gmane.org/gmane.comp.emulators.qemu/288521
> >
> > It will solve the problem, because it removes raising an irq from
> > rtl8139_set_next_tctr_time function.
> >
> > Ok then, I'll remove my rtl8139 patch from the series.
> 
> Please include that patch in your record/replay work though.  I'm not
> sure if I'll have time to test it and submit it.

Ok. Now I've fixed all issues and ready to submit the second version.
Will you review any of the other patches from the series?

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 09/12] rtl8139: adding new fields to vmstate

[Paolo Bonzini]

Il 28/08/2014 10:31, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
>> Il 27/08/2014 12:48, Pavel Dovgaluk ha scritto:
>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>> Il 27/08/2014 12:30, Pavel Dovgaluk ha scritto:
>>>>>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>>>>>> Il 27/08/2014 12:15, Pavel Dovgaluk ha scritto:
>>>>>>>>> Again, this is only needed in your record/replay system (and you haven't
>>>>>>>>> yet quite explained why the design has this limitation), so it should
>>>>>>>>> not be a part of this series.
>>>>>>>   I see. Updating s->timer and s->TimerExpire in post_load will be enough?
>>>>>>
>>>>>> In theory it should be done already, I guess it's not deterministic
>>>>>> enough though.
>>>>>
>>>>> I split existing function into the two parts: one sets irq (and calls another).
>>>>> And the second part sets only timer and TimerExpire fields. The second function
>>>>> is also called from post_load.
>>>>>
>>>>>> Have you tried my patch to rewrite the timer stuff?
>>>>>
>>>>> What patch do you mean?
>>>>
>>>> This one:
>>>>
>>>> http://article.gmane.org/gmane.comp.emulators.qemu/288521
>>>
>>> It will solve the problem, because it removes raising an irq from
>>> rtl8139_set_next_tctr_time function.
>>>
>>> Ok then, I'll remove my rtl8139 patch from the series.
>>
>> Please include that patch in your record/replay work though.  I'm not
>> sure if I'll have time to test it and submit it.
> 
> Ok. Now I've fixed all issues and ready to submit the second version.
> Will you review any of the other patches from the series?

I reviewed all x86 patches.  The ARM patches seem okay at first glance.

I also noticed a couple serial port issues not related to migration, but
we can handle them separately.

Paolo

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
> >> > Hmm, probably not.  The bug would not be other timers accessing the
> >> > APIC, because that would also call apic_sync_vapic and the only effect
> >> > would be an extra useless synchronization.  The bug would happen if the
> >> > APIC is accessed by the CPU before the timer has the occasion to run.
> > Sorry, but I don't understand which problem we will solve with apic_sync_vapic.
> 
> Taking inspiration from what KVM does, the fix could be even simpler
> than a change state handler.  run_on_cpu functions do not run while the
> VM is stopped, so the following should work:
> 
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index ce3d903..81d1ad7 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -91,13 +91,20 @@ void apic_enable_tpr_access_reporting(DeviceState
> *dev, bool enable)
>      }
>  }
> 
> +static void do_apic_enable_vapic(void *data)
> +{
> +    APICCommonState *s = APIC_COMMON(data);
> +    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> +
> +    info->vapic_base_update(s);
> +}
> +
>  void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
>  {
>      APICCommonState *s = APIC_COMMON(dev);
> -    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
> 
>      s->vapic_paddr = paddr;
> -    info->vapic_base_update(s);
> +    run_on_cpu(CPU(s->cpu), do_apic_enable_vapic, s);
>  }

I've tried this one and it doesn't work.
do_apic_enable_vapic runs on cpu, at the same time the VM state is loaded.
APIC state still remains broken because of this.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 06/12] kvmvapic: fixing loading vmstate

[Paolo Bonzini]

Il 09/09/2014 12:30, Pavel Dovgaluk ha scritto:
>> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
>> Il 27/08/2014 15:03, Pavel Dovgaluk ha scritto:
>>>>> Hmm, probably not.  The bug would not be other timers accessing the
>>>>> APIC, because that would also call apic_sync_vapic and the only effect
>>>>> would be an extra useless synchronization.  The bug would happen if the
>>>>> APIC is accessed by the CPU before the timer has the occasion to run.
>>> Sorry, but I don't understand which problem we will solve with apic_sync_vapic.
>>
>> Taking inspiration from what KVM does, the fix could be even simpler
>> than a change state handler.  run_on_cpu functions do not run while the
>> VM is stopped, so the following should work:
>>
>> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
>> index ce3d903..81d1ad7 100644
>> --- a/hw/intc/apic_common.c
>> +++ b/hw/intc/apic_common.c
>> @@ -91,13 +91,20 @@ void apic_enable_tpr_access_reporting(DeviceState
>> *dev, bool enable)
>>      }
>>  }
>>
>> +static void do_apic_enable_vapic(void *data)
>> +{
>> +    APICCommonState *s = APIC_COMMON(data);
>> +    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>> +
>> +    info->vapic_base_update(s);
>> +}
>> +
>>  void apic_enable_vapic(DeviceState *dev, hwaddr paddr)
>>  {
>>      APICCommonState *s = APIC_COMMON(dev);
>> -    APICCommonClass *info = APIC_COMMON_GET_CLASS(s);
>>
>>      s->vapic_paddr = paddr;
>> -    info->vapic_base_update(s);
>> +    run_on_cpu(CPU(s->cpu), do_apic_enable_vapic, s);
>>  }
> 
> I've tried this one and it doesn't work.
> do_apic_enable_vapic runs on cpu, at the same time the VM state is loaded.
> APIC state still remains broken because of this.

You're right (in fact run_on_cpu is synchronous so the alternative would
have been deadlock).  A change state handler can work.  I'll submit your
patches to the migration maintainers, including an alternative fix for
this VAPIC problem.

Paolo