[Qemu-devel] [PATCH 00/10] x86: migrate more data

[Qemu-devel] [PATCH 00/10] x86: migrate more data

[Paolo Bonzini]

Juan, David, Amit, here are Pavel's fixes for x86 migration.
Please help applying them, or ack them so that I can merge
them through the KVM tree.

Thanks,

Paolo

Paolo Bonzini (1):
  vl: use QLIST_FOREACH_SAFE to visit change state handlers

Pavel Dovgalyuk (9):
  apic_common: vapic_paddr synchronization fix
  cpu: init vmstate for ticks and clock offset
  pcspk: adding vmstate for save/restore
  fdc: adding vmstate for save/restore
  parallel: adding vmstate for save/restore
  serial: fixing vmstate for save/restore
  pckbd: adding new fields to vmstate
  piix: do not raise irq while loading vmstate
  mc146818rtc: add missed field to vmstate

 cpus.c                 |   8 +-
 hw/audio/pcspk.c       |  16 ++-
 hw/block/fdc.c         |  74 ++++++++++++++
 hw/char/parallel.c     |  18 ++++
 hw/char/serial.c       | 265 ++++++++++++++++++++++++++++++++++++++++---------
 hw/i386/kvmvapic.c     |  37 +++++--
 hw/input/pckbd.c       |  51 ++++++++++
 hw/pci-host/piix.c     |  26 ++++-
 hw/timer/mc146818rtc.c |  25 +++++
 include/qemu-common.h  |   2 +
 vl.c                   |   5 +-
 12 files changed, 463 insertions(+), 64 deletions(-)

-- 
2.1.0

[Qemu-devel] [PATCH 01/10] vl: use QLIST_FOREACH_SAFE to visit change state handlers

[Paolo Bonzini]

This lets a handler delete itself.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 vl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/vl.c b/vl.c
index 9c9acf5..15aea95 100644
--- a/vl.c
+++ b/vl.c
@@ -1721,11 +1721,11 @@ void qemu_del_vm_change_state_handler(VMChangeStateEntry *e)
 
 void vm_state_notify(int running, RunState state)
 {
-    VMChangeStateEntry *e;
+    VMChangeStateEntry *e, *next;
 
     trace_vm_state_notify(running, state);
 
-    for (e = vm_change_state_head.lh_first; e; e = e->entries.le_next) {
+    QLIST_FOREACH_SAFE(e, &vm_change_state_head, entries, next) {
         e->cb(e->opaque, running, state);
     }
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 01/10] vl: use QLIST_FOREACH_SAFE to visit change state handlers

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> This lets a handler delete itself.
>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch postpones vapic_paddr initialization, which is performed
during migration. When vapic_paddr is synchronized within the migration
process, apic_common functions could operate with incorrect apic state,
if it hadn't loaded yet. This patch postpones the synchronization until
the virtual machine is started, ensuring that the whole virtual machine
state has been loaded.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/i386/kvmvapic.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index ee95963..2bcc249 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -59,6 +59,7 @@ typedef struct VAPICROMState {
     GuestROMState rom_state;
     size_t rom_size;
     bool rom_mapped_writable;
+    VMChangeStateEntry *vmsentry;
 } VAPICROMState;
 
 #define TYPE_VAPIC "kvmvapic"
@@ -734,11 +735,34 @@ static void do_vapic_enable(void *data)
     vapic_enable(s, cpu);
 }
 
-static int vapic_post_load(void *opaque, int version_id)
+static void kvmvapic_vm_state_change(void *opaque, int running,
+				     RunState state)
 {
     VAPICROMState *s = opaque;
     uint8_t *zero;
 
+    if (!running) {
+        return;
+    }
+
+    if (s->state == VAPIC_ACTIVE) {
+        if (smp_cpus == 1) {
+            run_on_cpu(first_cpu, do_vapic_enable, s);
+        } else {
+            zero = g_malloc0(s->rom_state.vapic_size);
+            cpu_physical_memory_write(s->vapic_paddr, zero,
+                                      s->rom_state.vapic_size);
+            g_free(zero);
+        }
+    }
+
+    qemu_del_vm_change_state_handler(s->vmsentry);
+}
+
+static int vapic_post_load(void *opaque, int version_id)
+{
+    VAPICROMState *s = opaque;
+
     /*
      * The old implementation of qemu-kvm did not provide the state
      * VAPIC_STANDBY. Reconstruct it.
@@ -752,17 +776,8 @@ static int vapic_post_load(void *opaque, int version_id)
             return -1;
         }
     }
-    if (s->state == VAPIC_ACTIVE) {
-        if (smp_cpus == 1) {
-            run_on_cpu(first_cpu, do_vapic_enable, s);
-        } else {
-            zero = g_malloc0(s->rom_state.vapic_size);
-            cpu_physical_memory_write(s->vapic_paddr, zero,
-                                      s->rom_state.vapic_size);
-            g_free(zero);
-        }
-    }
 
+    s->vmsentry = qemu_add_vm_change_state_handler(kvmvapic_vm_state_change, s);
     return 0;
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> This patch postpones vapic_paddr initialization, which is performed
> during migration. When vapic_paddr is synchronized within the migration
> process, apic_common functions could operate with incorrect apic state,
> if it hadn't loaded yet. This patch postpones the synchronization until
> the virtual machine is started, ensuring that the whole virtual machine
> state has been loaded.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 02:30:01PM +0200, Paolo Bonzini wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch postpones vapic_paddr initialization, which is performed
> during migration. When vapic_paddr is synchronized within the migration
> process, apic_common functions could operate with incorrect apic state,
> if it hadn't loaded yet. This patch postpones the synchronization until
> the virtual machine is started, ensuring that the whole virtual machine
> state has been loaded.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/kvmvapic.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index ee95963..2bcc249 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -59,6 +59,7 @@ typedef struct VAPICROMState {
>      GuestROMState rom_state;
>      size_t rom_size;
>      bool rom_mapped_writable;
> +    VMChangeStateEntry *vmsentry;
>  } VAPICROMState;
>  
>  #define TYPE_VAPIC "kvmvapic"
> @@ -734,11 +735,34 @@ static void do_vapic_enable(void *data)
>      vapic_enable(s, cpu);
>  }
>  
> -static int vapic_post_load(void *opaque, int version_id)
> +static void kvmvapic_vm_state_change(void *opaque, int running,
> +				     RunState state)
>  {
>      VAPICROMState *s = opaque;
>      uint8_t *zero;
>  
> +    if (!running) {
> +        return;
> +    }
> +
> +    if (s->state == VAPIC_ACTIVE) {
> +        if (smp_cpus == 1) {

maybe add a comment explaining why we need
to special case smp_cpus?

> +            run_on_cpu(first_cpu, do_vapic_enable, s);

Is this safe to do in vmstate handler? cpu isn't running yet is it?

> +        } else {
> +            zero = g_malloc0(s->rom_state.vapic_size);
> +            cpu_physical_memory_write(s->vapic_paddr, zero,
> +                                      s->rom_state.vapic_size);
> +            g_free(zero);
> +        }
> +    }
> +
> +    qemu_del_vm_change_state_handler(s->vmsentry);
> +}
> +
> +static int vapic_post_load(void *opaque, int version_id)
> +{
> +    VAPICROMState *s = opaque;
> +
>      /*
>       * The old implementation of qemu-kvm did not provide the state
>       * VAPIC_STANDBY. Reconstruct it.
> @@ -752,17 +776,8 @@ static int vapic_post_load(void *opaque, int version_id)
>              return -1;
>          }
>      }
> -    if (s->state == VAPIC_ACTIVE) {
> -        if (smp_cpus == 1) {
> -            run_on_cpu(first_cpu, do_vapic_enable, s);
> -        } else {
> -            zero = g_malloc0(s->rom_state.vapic_size);
> -            cpu_physical_memory_write(s->vapic_paddr, zero,
> -                                      s->rom_state.vapic_size);
> -            g_free(zero);
> -        }
> -    }
>  
> +    s->vmsentry = qemu_add_vm_change_state_handler(kvmvapic_vm_state_change, s);
>      return 0;
>  }
>  
> -- 
> 2.1.0
> 
> 

Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Paolo Bonzini]

Il 09/09/2014 16:00, Michael S. Tsirkin ha scritto:
> On Tue, Sep 09, 2014 at 02:30:01PM +0200, Paolo Bonzini wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> This patch postpones vapic_paddr initialization, which is performed
>> during migration. When vapic_paddr is synchronized within the migration
>> process, apic_common functions could operate with incorrect apic state,
>> if it hadn't loaded yet. This patch postpones the synchronization until
>> the virtual machine is started, ensuring that the whole virtual machine
>> state has been loaded.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/i386/kvmvapic.c | 37 ++++++++++++++++++++++++++-----------
>>  1 file changed, 26 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
>> index ee95963..2bcc249 100644
>> --- a/hw/i386/kvmvapic.c
>> +++ b/hw/i386/kvmvapic.c
>> @@ -59,6 +59,7 @@ typedef struct VAPICROMState {
>>      GuestROMState rom_state;
>>      size_t rom_size;
>>      bool rom_mapped_writable;
>> +    VMChangeStateEntry *vmsentry;
>>  } VAPICROMState;
>>  
>>  #define TYPE_VAPIC "kvmvapic"
>> @@ -734,11 +735,34 @@ static void do_vapic_enable(void *data)
>>      vapic_enable(s, cpu);
>>  }
>>  
>> -static int vapic_post_load(void *opaque, int version_id)
>> +static void kvmvapic_vm_state_change(void *opaque, int running,
>> +				     RunState state)
>>  {
>>      VAPICROMState *s = opaque;
>>      uint8_t *zero;
>>  
>> +    if (!running) {
>> +        return;
>> +    }
>> +
>> +    if (s->state == VAPIC_ACTIVE) {
>> +        if (smp_cpus == 1) {
> 
> maybe add a comment explaining why we need
> to special case smp_cpus?

I don't know honestly.  I'm just moving code around.

>> +            run_on_cpu(first_cpu, do_vapic_enable, s);
> 
> Is this safe to do in vmstate handler? cpu isn't running yet is it?

Yes, it isn't running and run_on_cpu is synchronous.

Paolo

>> +        } else {
>> +            zero = g_malloc0(s->rom_state.vapic_size);
>> +            cpu_physical_memory_write(s->vapic_paddr, zero,
>> +                                      s->rom_state.vapic_size);
>> +            g_free(zero);
>> +        }
>> +    }
>> +
>> +    qemu_del_vm_change_state_handler(s->vmsentry);
>> +}
>> +
>> +static int vapic_post_load(void *opaque, int version_id)
>> +{
>> +    VAPICROMState *s = opaque;
>> +
>>      /*
>>       * The old implementation of qemu-kvm did not provide the state
>>       * VAPIC_STANDBY. Reconstruct it.
>> @@ -752,17 +776,8 @@ static int vapic_post_load(void *opaque, int version_id)
>>              return -1;
>>          }
>>      }
>> -    if (s->state == VAPIC_ACTIVE) {
>> -        if (smp_cpus == 1) {
>> -            run_on_cpu(first_cpu, do_vapic_enable, s);
>> -        } else {
>> -            zero = g_malloc0(s->rom_state.vapic_size);
>> -            cpu_physical_memory_write(s->vapic_paddr, zero,
>> -                                      s->rom_state.vapic_size);
>> -            g_free(zero);
>> -        }
>> -    }
>>  
>> +    s->vmsentry = qemu_add_vm_change_state_handler(kvmvapic_vm_state_change, s);
>>      return 0;
>>  }
>>  
>> -- 
>> 2.1.0
>>
>>

Re: [Qemu-devel] [PATCH 02/10] apic_common: vapic_paddr synchronization fix

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:paolo.bonzini@gmail.com] On Behalf Of Paolo Bonzini
> Sent: Tuesday, September 09, 2014 4:30 PM
> To: qemu-devel@nongnu.org
> Cc: quintela@redhat.com; amit.shah@redhat.com; dgilbert@redhat.com; Pavel.Dovgaluk@ispras.ru
> Subject: [PATCH 02/10] apic_common: vapic_paddr synchronization fix
> 
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch postpones vapic_paddr initialization, which is performed
> during migration. When vapic_paddr is synchronized within the migration
> process, apic_common functions could operate with incorrect apic state,
> if it hadn't loaded yet. This patch postpones the synchronization until
> the virtual machine is started, ensuring that the whole virtual machine
> state has been loaded.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/i386/kvmvapic.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index ee95963..2bcc249 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -59,6 +59,7 @@ typedef struct VAPICROMState {
>      GuestROMState rom_state;
>      size_t rom_size;
>      bool rom_mapped_writable;
> +    VMChangeStateEntry *vmsentry;
>  } VAPICROMState;
> 
>  #define TYPE_VAPIC "kvmvapic"
> @@ -734,11 +735,34 @@ static void do_vapic_enable(void *data)
>      vapic_enable(s, cpu);
>  }
> 
> -static int vapic_post_load(void *opaque, int version_id)
> +static void kvmvapic_vm_state_change(void *opaque, int running,
> +				     RunState state)

Thank you for apic solution.
I've tested this one with replay, and it worked fine.

Pavel Dovgalyuk

[Qemu-devel] [PATCH 03/10] cpu: init vmstate for ticks and clock offset

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

Ticks and clock offset used by CPU timers have to be saved in vmstate.
But vmstate for these fields registered only in icount mode.
Missing registration leads to breaking the continuity when vmstate is loaded.
This patch introduces new initialization function which fixes this.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c                | 8 ++++++--
 include/qemu-common.h | 2 ++
 vl.c                  | 1 +
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/cpus.c b/cpus.c
index 0f7d0ea..2a0e133 100644
--- a/cpus.c
+++ b/cpus.c
@@ -493,13 +493,17 @@ static const VMStateDescription vmstate_timers = {
     }
 };
 
+void cpu_ticks_init(void)
+{
+    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
+    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
+}
+
 void configure_icount(QemuOpts *opts, Error **errp)
 {
     const char *option;
     char *rem_str = NULL;
 
-    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
-    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
     option = qemu_opt_get(opts, "shift");
     if (!option) {
         if (qemu_opt_get(opts, "align") != NULL) {
diff --git a/include/qemu-common.h b/include/qemu-common.h
index bcf7a6a..dcb57ab 100644
--- a/include/qemu-common.h
+++ b/include/qemu-common.h
@@ -105,6 +105,8 @@ static inline char *realpath(const char *path, char *resolved_path)
 }
 #endif
 
+void cpu_ticks_init(void);
+
 /* icount */
 void configure_icount(QemuOpts *opts, Error **errp);
 extern int use_icount;
diff --git a/vl.c b/vl.c
index 15aea95..5db0d08 100644
--- a/vl.c
+++ b/vl.c
@@ -4334,6 +4334,7 @@ int main(int argc, char **argv, char **envp)
     qemu_spice_init();
 #endif
 
+    cpu_ticks_init();
     if (icount_opts) {
         if (kvm_enabled() || xen_enabled()) {
             fprintf(stderr, "-icount is not allowed with kvm or xen\n");
-- 
2.1.0

Re: [Qemu-devel] [PATCH 03/10] cpu: init vmstate for ticks and clock offset

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> Ticks and clock offset used by CPU timers have to be saved in vmstate.
> But vmstate for these fields registered only in icount mode.
> Missing registration leads to breaking the continuity when vmstate is loaded.
> This patch introduces new initialization function which fixes this.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Nack.

This break migration when using with older machine types.

Several questions before I propose an alternative:
- are we this fields always needed (then why migration has worked until
  now)
- after reding cpus.c the question is .... why it ever worked?


After reading cpus.c, it appears that we only really need this
functionality for

qemu_clock_get_ns()

when called with QEMU_CLOCK_VIRTUAL with icount disabled, right?

further grep shows that it is used for acpi_pm_tmr_get_clock, whatever
that is.  When is this used, thanks?

Later, Juan.

> ---
>  cpus.c                | 8 ++++++--
>  include/qemu-common.h | 2 ++
>  vl.c                  | 1 +
>  3 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/cpus.c b/cpus.c
> index 0f7d0ea..2a0e133 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -493,13 +493,17 @@ static const VMStateDescription vmstate_timers = {
>      }
>  };
>  
> +void cpu_ticks_init(void)
> +{
> +    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
> +    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
> +}
> +
>  void configure_icount(QemuOpts *opts, Error **errp)
>  {
>      const char *option;
>      char *rem_str = NULL;
>  
> -    seqlock_init(&timers_state.vm_clock_seqlock, NULL);
> -    vmstate_register(NULL, 0, &vmstate_timers, &timers_state);
>      option = qemu_opt_get(opts, "shift");
>      if (!option) {
>          if (qemu_opt_get(opts, "align") != NULL) {
> diff --git a/include/qemu-common.h b/include/qemu-common.h
> index bcf7a6a..dcb57ab 100644
> --- a/include/qemu-common.h
> +++ b/include/qemu-common.h
> @@ -105,6 +105,8 @@ static inline char *realpath(const char *path, char *resolved_path)
>  }
>  #endif
>  
> +void cpu_ticks_init(void);
> +
>  /* icount */
>  void configure_icount(QemuOpts *opts, Error **errp);
>  extern int use_icount;
> diff --git a/vl.c b/vl.c
> index 15aea95..5db0d08 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4334,6 +4334,7 @@ int main(int argc, char **argv, char **envp)
>      qemu_spice_init();
>  #endif
>  
> +    cpu_ticks_init();
>      if (icount_opts) {
>          if (kvm_enabled() || xen_enabled()) {
>              fprintf(stderr, "-icount is not allowed with kvm or xen\n");

Re: [Qemu-devel] [PATCH 03/10] cpu: init vmstate for ticks and clock offset

[Paolo Bonzini]

Il 09/09/2014 15:25, Juan Quintela ha scritto:
> Nack.
> 
> This break migration when using with older machine types.

The other way round---this was broken recently, the patch fixes it.

Paolo

> Several questions before I propose an alternative:
> - are we this fields always needed (then why migration has worked until
>   now)
> - after reding cpus.c the question is .... why it ever worked?
> 
> 
> After reading cpus.c, it appears that we only really need this
> functionality for
> 
> qemu_clock_get_ns()
> 
> when called with QEMU_CLOCK_VIRTUAL with icount disabled, right?
> 
> further grep shows that it is used for acpi_pm_tmr_get_clock, whatever
> that is.  When is this used, thanks?

[Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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

This breaks migration to 2.1 and earlier.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/audio/pcspk.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/hw/audio/pcspk.c b/hw/audio/pcspk.c
index 1d81bbe..62dfabe 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,17 @@ static const MemoryRegionOps pcspk_io_ops = {
     },
 };
 
+static const VMStateDescription vmstate_spk = {
+    .name = "pcspk",
+    .version_id = 1,
+    .minimum_version_id = 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);
@@ -192,6 +203,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
 
     dc->realize = pcspk_realizefn;
     set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
+    dc->vmsd = &vmstate_spk;
     dc->props = pcspk_properties;
     /* Reason: pointer property "pit", realize sets global pcspk_state */
     dc->cannot_instantiate_with_device_add_yet = true;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> VMState added by this patch preserves correct
> loading of the PC speaker device state.
>
> This breaks migration to 2.1 and earlier.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This should only be added for new machine types, right?

Later, Juan.

> @@ -192,6 +203,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
>  
>      dc->realize = pcspk_realizefn;
>      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> +    dc->vmsd = &vmstate_spk;
>      dc->props = pcspk_properties;
>      /* Reason: pointer property "pit", realize sets global pcspk_state */
>      dc->cannot_instantiate_with_device_add_yet = true;

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Paolo Bonzini]

Il 09/09/2014 15:26, Juan Quintela ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> VMState added by this patch preserves correct
>> loading of the PC speaker device state.
>>
>> This breaks migration to 2.1 and earlier.
>>
>> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> This should only be added for new machine types, right?

We never did that in the past.  Maybe downstream.

Paolo

> Later, Juan.
> 
>> @@ -192,6 +203,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
>>  
>>      dc->realize = pcspk_realizefn;
>>      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
>> +    dc->vmsd = &vmstate_spk;
>>      dc->props = pcspk_properties;
>>      /* Reason: pointer property "pit", realize sets global pcspk_state */
>>      dc->cannot_instantiate_with_device_add_yet = true;

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 03:28:43PM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 15:26, Juan Quintela ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >>
> >> VMState added by this patch preserves correct
> >> loading of the PC speaker device state.
> >>
> >> This breaks migration to 2.1 and earlier.
> >>
> >> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > 
> > This should only be added for new machine types, right?
> 
> We never did that in the past.  Maybe downstream.
> 
> Paolo

That's not too hard: we need two types that only
differ in the vmstate.
Use the correct one to create the device depending
on the pc type.

Put in that light, we definitely did create new
devices while keeping old ones around for
compatibility.


> > Later, Juan.
> > 
> >> @@ -192,6 +203,7 @@ static void pcspk_class_initfn(ObjectClass *klass, void *data)
> >>  
> >>      dc->realize = pcspk_realizefn;
> >>      set_bit(DEVICE_CATEGORY_SOUND, dc->categories);
> >> +    dc->vmsd = &vmstate_spk;
> >>      dc->props = pcspk_properties;
> >>      /* Reason: pointer property "pit", realize sets global pcspk_state */
> >>      dc->cannot_instantiate_with_device_add_yet = true;
> 

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Paolo Bonzini]

Il 10/09/2014 14:02, Michael S. Tsirkin ha scritto:
> That's not too hard: we need two types that only
> differ in the vmstate.
> Use the correct one to create the device depending
> on the pc type.
> 
> Put in that light, we definitely did create new
> devices while keeping old ones around for
> compatibility.

Do you have an example?  I can think of one that we did in RHEL (hda)
but it was never upstream.

Paolo

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 12:59:35PM +0200, Paolo Bonzini wrote:
> Il 10/09/2014 14:02, Michael S. Tsirkin ha scritto:
> > That's not too hard: we need two types that only
> > differ in the vmstate.
> > Use the correct one to create the device depending
> > on the pc type.
> > 
> > Put in that light, we definitely did create new
> > devices while keeping old ones around for
> > compatibility.
> 
> Do you have an example?  I can think of one that we did in RHEL (hda)
> but it was never upstream.
> 
> Paolo

Well the whole PC machine is exactly like this, isn't it?

Also, xen plans to do something similar for igd passthrough,
and I plan to do something similar for virtio 1.0.
stdvga is somewhat similar.

-- 
MST

Re: [Qemu-devel] [PATCH 04/10] pcspk: adding vmstate for save/restore

[Paolo Bonzini]

Il 10/09/2014 14:20, Michael S. Tsirkin ha scritto:
> > Do you have an example?  I can think of one that we did in RHEL (hda)
> > but it was never upstream.
> 
> Well the whole PC machine is exactly like this, isn't it?

Sorry, I cannot parse.

You proposed creating two versions of pcspk that only differ in the
vmstate.  We never did that in upstream QEMU, and all precedents are in
the other direction.

Furthermore, for a downstream that cares about backwards migration the
simplest thing to do would be to just revert this patch.  It would
really be overkill.

> Also, xen plans to do something similar for igd passthrough,
> and I plan to do something similar for virtio 1.0.
> stdvga is somewhat similar.

This has nothing to do with migration though.  I understand the
technicalities of how it would be done.  I just think that upstream
policy so far hasn't been to do this.

Paolo

[Qemu-devel] [PATCH 05/10] fdc: adding vmstate for save/restore

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/fdc.c | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/hw/block/fdc.c b/hw/block/fdc.c
index 490d127..6c86a6b 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 */
         }
     }
@@ -734,6 +761,40 @@ 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,
@@ -770,6 +831,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 */
+        }
     }
 };
 
@@ -844,6 +916,8 @@ static void fdctrl_reset(FDCtrl *fdctrl, int do_irq)
     fdctrl->dor = FD_DOR_nRESET;
     fdctrl->dor |= (fdctrl->dma_chann != -1) ? FD_DOR_DMAEN : 0;
     fdctrl->msr = FD_MSR_RQM;
+    fdctrl->reset_sensei = 0;
+    timer_del(fdctrl->result_timer);
     /* FIFO state */
     fdctrl->data_pos = 0;
     fdctrl->data_len = 0;
-- 
2.1.0

Re: [Qemu-devel] [PATCH 05/10] fdc: adding vmstate for save/restore

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> VMState added by this patch preserves correct
> loading of the FDC device state.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

[Qemu-devel] [PATCH 06/10] parallel: adding vmstate for save/restore

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/parallel.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/hw/char/parallel.c b/hw/char/parallel.c
index 7ac90a5..c2b553f 100644
--- a/hw/char/parallel.c
+++ b/hw/char/parallel.c
@@ -477,6 +477,23 @@ 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,
+    .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;
@@ -606,6 +623,7 @@ static void parallel_isa_class_initfn(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
 
     dc->realize = parallel_isa_realizefn;
+    dc->vmsd = &vmstate_parallel_isa;
     dc->props = parallel_isa_properties;
     set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
 }
-- 
2.1.0

Re: [Qemu-devel] [PATCH 06/10] parallel: adding vmstate for save/restore

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> VMState added by this patch preserves correct
> loading of the parallel port controller state.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

This breaks migration with old machine type, but as far as I can see,
parallel is only added when used, and if we are using it, we need this,
right?

If my understanding is correct:

Acked-by: Juan Quintela <quintela@redhat.com>

> ---
>  hw/char/parallel.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/hw/char/parallel.c b/hw/char/parallel.c
> index 7ac90a5..c2b553f 100644
> --- a/hw/char/parallel.c
> +++ b/hw/char/parallel.c
> @@ -477,6 +477,23 @@ 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,
> +    .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;
> @@ -606,6 +623,7 @@ static void parallel_isa_class_initfn(ObjectClass *klass, void *data)
>      DeviceClass *dc = DEVICE_CLASS(klass);
>  
>      dc->realize = parallel_isa_realizefn;
> +    dc->vmsd = &vmstate_parallel_isa;
>      dc->props = parallel_isa_properties;
>      set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
>  }

Re: [Qemu-devel] [PATCH 06/10] parallel: adding vmstate for save/restore

[Paolo Bonzini]

Il 09/09/2014 15:32, Juan Quintela ha scritto:
> This breaks migration with old machine type, but as far as I can see,
> parallel is only added when used, and if we are using it, we need this,
> right?

Yes.  But again, it only breaks backwards migration, which is not
supported upstream.

Paolo

> If my understanding is correct:
> 
> Acked-by: Juan Quintela <quintela@redhat.com>
> 

Re: [Qemu-devel] [PATCH 06/10] parallel: adding vmstate for save/restore

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 03:40:07PM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 15:32, Juan Quintela ha scritto:
> > This breaks migration with old machine type, but as far as I can see,
> > parallel is only added when used, and if we are using it, we need this,
> > right?
> 
> Yes.


Presumably, your next statement means that guests can typically recover
if parallel state is discarded in migration?

>  But again, it only breaks backwards migration, which is not
> supported upstream.
> 
> Paolo

Upstream doesn't provide support for old versions period.
We do our best to make it easy for downstreams to support
cross version migration and they care about both direction,
so I don't see why make an exception here.

So if guests can recover, it seems worth it to
stay bug for bug compatible for old machine types.


> > If my understanding is correct:
> > 
> > Acked-by: Juan Quintela <quintela@redhat.com>
> > 
> 

Re: [Qemu-devel] [PATCH 06/10] parallel: adding vmstate for save/restore

[Paolo Bonzini]

Il 10/09/2014 14:09, Michael S. Tsirkin ha scritto:
>> > Yes.
> 
> Presumably, your next statement means that guests can typically recover
> if parallel state is discarded in migration?

No, it means that I would not care just like for pcspk.

If they are using parallel at the time of migration, they would not recover.

Paolo

>> >  But again, it only breaks backwards migration, which is not
>> > supported upstream.

[Qemu-devel] [PATCH 07/10] serial: fixing vmstate for save/restore

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/char/serial.c | 265 +++++++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 220 insertions(+), 45 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 764e184..2b04927 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,14 @@ 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;
+    s->poll_msl = -1;
+    return 0;
+}
+
 static int serial_post_load(void *opaque, int version_id)
 {
     SerialState *s = opaque;
@@ -597,17 +620,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 +766,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 */
+        }
     }
 };
 
@@ -642,6 +813,10 @@ static void serial_reset(void *opaque)
     s->char_transmit_time = (get_ticks_per_sec() / 9600) * 10;
     s->poll_msl = 0;
 
+    s->timeout_ipending = 0;
+    timer_del(s->fifo_timeout_timer);
+    timer_del(s->modem_status_poll);
+
     fifo8_reset(&s->recv_fifo);
     fifo8_reset(&s->xmit_fifo);
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH 07/10] serial: fixing vmstate for save/restore

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 265 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 220 insertions(+), 45 deletions(-)
>
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 764e184..2b04927 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;

Both users call it with SerialState *s, not a void *.

is_load can be a bool.

> +static int serial_pre_load(void *opaque)
> +{
> +    SerialState *s = (SerialState *)opaque;

Unneeded cast (and not used in the rest of the file)


> +    s->thr_ipending = -1;
> +    s->poll_msl = -1;

We set both to -1.


> +    return 0;
> +}
> +
>  static int serial_post_load(void *opaque, int version_id)
>  {
>      SerialState *s = opaque;
> @@ -597,17 +620,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);
> +    }

If it is still -1 at this point, we "calculate" a value for it.
(I assume it is right, no knowledge of serial port)

But poll_msl is "more" interesting, because we are not "reseting it".

So, we have that if we are migrating from an old version, we would have
poll_msl == -1, and we used to have it to poll_msl == 0.

Should we change it?

I think that putting:

s->poll_msl = 0;

in preload, and 

static bool serial_poll_needed(void *opaque)
{
    SerialState *s = opaque;
    return s->poll_msl != 0;
}

would give exactly the same behaviour for new qemus, and behave better
for older ones?

what do you think?

Later, Juan.



> +    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 +766,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 */
> +        }
>      }
>  };
>  
> @@ -642,6 +813,10 @@ static void serial_reset(void *opaque)
>      s->char_transmit_time = (get_ticks_per_sec() / 9600) * 10;
>      s->poll_msl = 0;
>  
> +    s->timeout_ipending = 0;
> +    timer_del(s->fifo_timeout_timer);
> +    timer_del(s->modem_status_poll);
> +
>      fifo8_reset(&s->recv_fifo);
>      fifo8_reset(&s->xmit_fifo);

Re: [Qemu-devel] [PATCH 07/10] serial: fixing vmstate for save/restore

[Paolo Bonzini]

Il 09/09/2014 15:59, Juan Quintela ha scritto:
> If it is still -1 at this point, we "calculate" a value for it.
> (I assume it is right, no knowledge of serial port)

Not necessarily right, or we wouldn't need the subsection, but at least
a decent approximation.

> But poll_msl is "more" interesting, because we are not "reseting it".
> 
> So, we have that if we are migrating from an old version, we would have
> poll_msl == -1, and we used to have it to poll_msl == 0.
> 
> Should we change it?
> 
> I think that putting:
> 
> s->poll_msl = 0;
> 
> in preload, and 
> 
> static bool serial_poll_needed(void *opaque)
> {
>     SerialState *s = opaque;
>     return s->poll_msl != 0;
> }
> 
> would give exactly the same behaviour for new qemus, and behave better
> for older ones?

poll_msl is usually -1, so the "needed" function must be like Pavel wrote.

poll_msl is only used for serial port passthrough, which I guess we can
say "just doesn't work" for migration on < 2.1.  In fact, putting
migration + passthrough together is probably not a great idea. :)  We
probably could drop ust the poll_msl/modem_status_poll subsection, but I
assume Pavel had some kind of test case.

Paolo

Re: [Qemu-devel] [PATCH 07/10] serial: fixing vmstate for save/restore

[Pavel Dovgaluk]

> From: Paolo Bonzini [mailto:pbonzini@redhat.com]
> Il 09/09/2014 15:59, Juan Quintela ha scritto:
> > But poll_msl is "more" interesting, because we are not "reseting it".
> >
> > So, we have that if we are migrating from an old version, we would have
> > poll_msl == -1, and we used to have it to poll_msl == 0.
> >
> > Should we change it?
> >
> > I think that putting:
> >
> > s->poll_msl = 0;
> >
> > in preload, and
> >
> > static bool serial_poll_needed(void *opaque)
> > {
> >     SerialState *s = opaque;
> >     return s->poll_msl != 0;
> > }
> >
> > would give exactly the same behaviour for new qemus, and behave better
> > for older ones?
> 
> poll_msl is usually -1, so the "needed" function must be like Pavel wrote.
> 
> poll_msl is only used for serial port passthrough, which I guess we can
> say "just doesn't work" for migration on < 2.1.  In fact, putting
> migration + passthrough together is probably not a great idea. :)  We
> probably could drop ust the poll_msl/modem_status_poll subsection, but I
> assume Pavel had some kind of test case.

I have an example for record/replay.
Record can work with passthrough enabled. And it usually saves some vmstates.
In replay mode we emulate passthrough by sending data read from the log
as they were received from the real device. That's why we need correct 
state recovery even for these fields.

Pavel Dovgalyuk

Re: [Qemu-devel] [PATCH 07/10] serial: fixing vmstate for save/restore

[Paolo Bonzini]

Il 09/09/2014 14:30, Paolo Bonzini ha scritto:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> 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.

This is actually not entirely correct because...

> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/char/serial.c | 265 +++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 220 insertions(+), 45 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 764e184..2b04927 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;
> +    }

... if the value of the FE bit changes, this will nullify the change you
made to send/restore FIFOs.  The handling of RFR/XFR must remain in
serial_ioport_write, and serial_write_fcr must receive the final value
(masked by 0xc9).  I can fix this up.

Paolo

> +    /* 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,14 @@ 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;
> +    s->poll_msl = -1;
> +    return 0;
> +}
> +
>  static int serial_post_load(void *opaque, int version_id)
>  {
>      SerialState *s = opaque;
> @@ -597,17 +620,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 +766,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 */
> +        }
>      }
>  };
>  
> @@ -642,6 +813,10 @@ static void serial_reset(void *opaque)
>      s->char_transmit_time = (get_ticks_per_sec() / 9600) * 10;
>      s->poll_msl = 0;
>  
> +    s->timeout_ipending = 0;
> +    timer_del(s->fifo_timeout_timer);
> +    timer_del(s->modem_status_poll);
> +
>      fifo8_reset(&s->recv_fifo);
>      fifo8_reset(&s->xmit_fifo);
>  
> 

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

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/input/pckbd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
index 2ab8c87..2b0cd3d 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()
     }
 };
 
-- 
2.1.0

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

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>
> ---
>  hw/input/pckbd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
>
> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
> index 2ab8c87..2b0cd3d 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;

I don't like this one, but ....


>      /* 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;

Only solution that I thought of is putting here:


     s->outport |=
                | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
                | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);


But I am not sure if that bits can be off if status bits are on.

Thinking about it, why it is that it is not necessary to have on
postload something like that?

PD: no, I don't claim to understand how the pc keyboard work ...

Later, Juan.

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

[Paolo Bonzini]

Il 09/09/2014 15:07, Juan Quintela ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> wrote:
>> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>>
>> 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>
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Acked-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  hw/input/pckbd.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>>
>> diff --git a/hw/input/pckbd.c b/hw/input/pckbd.c
>> index 2ab8c87..2b0cd3d 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;
> 
> I don't like this one, but ....
> 
> 
>>      /* 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;
> 
> Only solution that I thought of is putting here:
> 
> 
>      s->outport |=
>                 | (s->status & KBD_STAT_OBF ? KBD_OUT_OBF : 0)
>                 | (s->status & KBD_STAT_MOUSE_OBF ? KBD_OUT_MOUSE_OBF : 0);
> 
> 
> But I am not sure if that bits can be off if status bits are on.

Yes, they can---the outport can be written by the guest (see
outport_write).  That was my first thought as well. :)

Paolo

> Thinking about it, why it is that it is not necessary to have on
> postload something like that?
> 
> PD: no, I don't claim to understand how the pc keyboard work ...
> 
> Later, Juan.
> 
> 

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

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

This patch disables raising an irq while loading the state of PCI bridge.
The aim of this patch is preserving the same behavior while saving and
restoring the VM state. IRQ is not raised while saving the state of the bridge.
That's why the behavior of the restored system will differ from
the original one. This patch eliminates raising an irq and just restores
the calculated state fields in post_load function.

Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/pci-host/piix.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
index e0e0946..cd50435 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,18 @@ 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;
+
+    /* Update irq levels without raising an interrupt which could
+     * happen in piix3_update_irq_levels.  Raising an IRQ will
+     * bring the system to a different state than the saved one.
+     * Interrupt state is serialized separately through the i8259.
+     */
+    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;
 }
 
-- 
2.1.0

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

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> This patch disables raising an irq while loading the state of PCI bridge.
> The aim of this patch is preserving the same behavior while saving and
> restoring the VM state. IRQ is not raised while saving the state of the bridge.
> That's why the behavior of the restored system will differ from
> the original one. This patch eliminates raising an irq and just restores
> the calculated state fields in post_load function.
>
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

No problem from migration point of view.
I assume that the pci bits are ok, so

Acked-by: Juan Quintela <quintela@redhat.com>

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

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 02:30:08PM +0200, Paolo Bonzini wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> 
> This patch disables raising an irq while loading the state of PCI bridge.
> The aim of this patch is preserving the same behavior while saving and
> restoring the VM state. IRQ is not raised while saving the state of the bridge.
> That's why the behavior of the restored system will differ from
> the original one.


Hmm I don't understand.
You are removing call to piix3_update_irq_levels
this call is supposed to just sync up bus to
irq level.

How can this change system state? Saved state
is supposed to already be in sync.


> This patch eliminates raising an irq and just restores
> the calculated state fields in post_load function.
> 
> Signed-off-by: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/pci-host/piix.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> index e0e0946..cd50435 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,18 @@ 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;
> +
> +    /* Update irq levels without raising an interrupt which could
> +     * happen in piix3_update_irq_levels.  Raising an IRQ will
> +     * bring the system to a different state than the saved one.

I don't like comments like this: don't discuss what could
have been if code was different. Explain why code
is like it is.

> +     * Interrupt state is serialized separately through the i8259.
> +     */
> +    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;
>  }
>  
> -- 
> 2.1.0
> 
> 

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

[Paolo Bonzini]

Il 09/09/2014 15:54, Michael S. Tsirkin ha scritto:
> 
> Hmm I don't understand.
> You are removing call to piix3_update_irq_levels
> this call is supposed to just sync up bus to
> irq level.
> 
> How can this change system state? Saved state
> is supposed to already be in sync.

i440FX/PIIX3 state is loaded before i8259, so the interrupt will never
be in the i8259 ISR.  I am not sure why it is a problem for
record/replay, but I think it's plausible to consider this a bug.  i8259
state should not be affected by the load of PIIX3 state, since i8259 is
migrated separately.

Paolo

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

[Michael S. Tsirkin]

On Tue, Sep 09, 2014 at 07:16:19PM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 15:54, Michael S. Tsirkin ha scritto:
> > 
> > Hmm I don't understand.
> > You are removing call to piix3_update_irq_levels
> > this call is supposed to just sync up bus to
> > irq level.
> > 
> > How can this change system state? Saved state
> > is supposed to already be in sync.
> 
> i440FX/PIIX3 state is loaded before i8259, so the interrupt will never
> be in the i8259 ISR.  I am not sure why it is a problem for
> record/replay, but I think it's plausible to consider this a bug.  i8259
> state should not be affected by the load of PIIX3 state, since i8259 is
> migrated separately.
> 
> Paolo

Sorry I still don't understand. Why do stuff from vmstate callback then?
How is it different?

I'd like to see a description of a scenario where this patch makes
a difference.

-- 
MST

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

[Paolo Bonzini]

Il 09/09/2014 22:51, Michael S. Tsirkin ha scritto:
> > i440FX/PIIX3 state is loaded before i8259, so the interrupt will never
> > be in the i8259 ISR.  I am not sure why it is a problem for
> > record/replay, but I think it's plausible to consider this a bug.  i8259
> > state should not be affected by the load of PIIX3 state, since i8259 is
> > migrated separately.
> 
> Sorry I still don't understand. Why do stuff from vmstate callback then?
> How is it different?

Reconstructing internal state from post_load is okay.

What is not okay (and I think it should be a rule) is to touch other
devices from post_load, unless you know that they are deserialized
first.  For example it's okay for a PCI device to talk to the parent
bridge in its post_load function.

In the case of PIIX3 vs. i8259, however, you know that i8259 is
deserialized _last_ because i8259 is an ISA device and PIIX3 provides
the ISA bus.  So it's incorrect, even though it's currently harmless, to
touch the i8259 before it's deserialized.

> I'd like to see a description of a scenario where this patch makes
> a difference.

Of course it would be nice to have testcases for this, but I guess one
case could be:

- LAPIC configured in ExtINT mode

- interrupts are masked in the i8259, but the i8259 doesn't know that
yet because it's not been loaded yet

- the PIIX3 loads the state and the interrupt is set.  pic_set_irq is
called, calls pic_update_irq

- pic_update_irq calls pic_get_irq, which uses IMR=0 and thus raises LINT0

- the APIC has been loaded already, so LINT0 is injected incorrectly


Another case could be:

- i8259 is processing IRQ0.  The lower-priority interrupt from PIIX3 is
in IRR.  Machine is migrated.

- the PIIX3 loads the state and sets the interrupt in the i8259.
pic_set_irq is called, calls pic_update_irq, calls pic_get_irq

- because i8259 has not been loaded yet, pic_get_irq sees ISR=0 and the
interrupt is injected even though IRQ0 (higher priority) is being serviced.


In both cases, the saved i8259 state will have the PIIX3 interrupt in
IRR, so the interrupt is not lost, just held (as it would have been on
the source machine).

Paolo

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

[Peter Maydell]

On 10 September 2014 09:38, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 09/09/2014 22:51, Michael S. Tsirkin ha scritto:
>> Sorry I still don't understand. Why do stuff from vmstate callback then?
>> How is it different?
>
> Reconstructing internal state from post_load is okay.
>
> What is not okay (and I think it should be a rule) is to touch other
> devices from post_load, unless you know that they are deserialized
> first.  For example it's okay for a PCI device to talk to the parent
> bridge in its post_load function.

I don't think it's right to talk to another device even if you do
know it's deserialized first. Talking to it might make it change
its state, which would be wrong (since its correct state is
the state it's just deserialized). I would suggest the rule should
be "never do something that can change the state of another
device in post-load".

(We have similar issues with reset, except worse in that we
don't have a coherent rule to cause everything to come out
of reset in the right state.)

thanks
-- PMM

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

[Paolo Bonzini]

Il 10/09/2014 10:51, Peter Maydell ha scritto:
> > What is not okay (and I think it should be a rule) is to touch other
> > devices from post_load, unless you know that they are deserialized
> > first.  For example it's okay for a PCI device to talk to the parent
> > bridge in its post_load function.
> 
> I don't think it's right to talk to another device even if you do
> know it's deserialized first. Talking to it might make it change
> its state, which would be wrong (since its correct state is
> the state it's just deserialized). I would suggest the rule should
> be "never do something that can change the state of another
> device in post-load".

That's harder to do, but if it is possible to do it, it would be great
as well.

It would not surprise me to find a case where the parent device actually
_expects_ the children's post_load to inform it about something, instead
of serializing that part of state on its own.

Paolo

> (We have similar issues with reset, except worse in that we
> don't have a coherent rule to cause everything to come out
> of reset in the right state.)

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

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 11:05:39AM +0200, Paolo Bonzini wrote:
> Il 10/09/2014 10:51, Peter Maydell ha scritto:
> > > What is not okay (and I think it should be a rule) is to touch other
> > > devices from post_load, unless you know that they are deserialized
> > > first.  For example it's okay for a PCI device to talk to the parent
> > > bridge in its post_load function.
> > 
> > I don't think it's right to talk to another device even if you do
> > know it's deserialized first. Talking to it might make it change
> > its state, which would be wrong (since its correct state is
> > the state it's just deserialized). I would suggest the rule should
> > be "never do something that can change the state of another
> > device in post-load".
> 
> That's harder to do, but if it is possible to do it, it would be great
> as well.
> 
> It would not surprise me to find a case where the parent device actually
> _expects_ the children's post_load to inform it about something, instead
> of serializing that part of state on its own.
> 
> Paolo

Absolutely, I don't think we can require that.

For example, at the moment, for PCI bridges, we serialize the
state of all interrupt lines, but that's just a function
of all devices connected to each line.
So we are transmitting redundant information, and I have plans
to discard that and recompute parent state based on child state.



> > (We have similar issues with reset, except worse in that we
> > don't have a coherent rule to cause everything to come out
> > of reset in the right state.)

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

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 10:38:46AM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 22:51, Michael S. Tsirkin ha scritto:
> > > i440FX/PIIX3 state is loaded before i8259, so the interrupt will never
> > > be in the i8259 ISR.  I am not sure why it is a problem for
> > > record/replay, but I think it's plausible to consider this a bug.  i8259
> > > state should not be affected by the load of PIIX3 state, since i8259 is
> > > migrated separately.
> > 
> > Sorry I still don't understand. Why do stuff from vmstate callback then?
> > How is it different?
> 
> Reconstructing internal state from post_load is okay.
> 
> What is not okay (and I think it should be a rule) is to touch other
> devices from post_load, unless you know that they are deserialized
> first.  For example it's okay for a PCI device to talk to the parent
> bridge in its post_load function.
> 
> In the case of PIIX3 vs. i8259, however, you know that i8259 is
> deserialized _last_ because i8259 is an ISA device and PIIX3 provides
> the ISA bus.  So it's incorrect, even though it's currently harmless, to
> touch the i8259 before it's deserialized.

OK, got this, thanks for the explanation!
So the reason i8259 might be out of sync is
because it's not yet deserialized.

I think it's a good idea to put (at least the
last part) in the commit log.
Also it's updating irq state, not just raising irq,
that might be problematic, right?

So also, something like this for the comment:
+    /* We update irq levels in PIIX3 but don't set IRQ, since
+     *	IRQ state is serialized separately through the i8259,
+     * which is not deserialized yet, at this point.
+     */





> > I'd like to see a description of a scenario where this patch makes
> > a difference.
> 
> Of course it would be nice to have testcases for this, but I guess one
> case could be:
> 
> - LAPIC configured in ExtINT mode
> 
> - interrupts are masked in the i8259, but the i8259 doesn't know that
> yet because it's not been loaded yet
> 
> - the PIIX3 loads the state and the interrupt is set.  pic_set_irq is
> called, calls pic_update_irq
> 
> - pic_update_irq calls pic_get_irq, which uses IMR=0 and thus raises LINT0
> 
> - the APIC has been loaded already, so LINT0 is injected incorrectly
> 
> 
> Another case could be:
> 
> - i8259 is processing IRQ0.  The lower-priority interrupt from PIIX3 is
> in IRR.  Machine is migrated.
> 
> - the PIIX3 loads the state and sets the interrupt in the i8259.
> pic_set_irq is called, calls pic_update_irq, calls pic_get_irq
> 
> - because i8259 has not been loaded yet, pic_get_irq sees ISR=0 and the
> interrupt is injected even though IRQ0 (higher priority) is being serviced.
> 
> 
> In both cases, the saved i8259 state will have the PIIX3 interrupt in
> IRR, so the interrupt is not lost, just held (as it would have been on
> the source machine).
> 
> Paolo

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

[Paolo Bonzini]

Il 10/09/2014 12:50, Michael S. Tsirkin ha scritto:
> OK, got this, thanks for the explanation!
> So the reason i8259 might be out of sync is
> because it's not yet deserialized.

Yes, especially the IMR/IRR/ISR fields.

> I think it's a good idea to put (at least the
> last part) in the commit log.

Like this:

    This patch disables raising an irq while loading the state of PCI bridge.
    Because the i8259 has not been deserialized yet, raising an interrupt
    could bring the system out-of-sync with the migration source.  For example,
    the migration source could have masked the interrupt in the i8259. On the
    destination, the i8259 device model would not know that yet and would
    trigger an interrupt in the CPU.
    
    This patch eliminates raising an irq and just restores the calculated
    state fields in post_load function.  Interrupt state will be deserialized
    separately through the IRR field of the i8259.

> Also it's updating irq state, not just raising irq,
> that might be problematic, right?

Well, the i8259 is in the reset state so ISR=IRR=0, aka all IRQ lines 
are known to be low.  But yes, in general it's the update that is 
problematic.

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

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 11:58:34AM +0200, Paolo Bonzini wrote:
> Il 10/09/2014 12:50, Michael S. Tsirkin ha scritto:
> > OK, got this, thanks for the explanation!
> > So the reason i8259 might be out of sync is
> > because it's not yet deserialized.
> 
> Yes, especially the IMR/IRR/ISR fields.
> 
> > I think it's a good idea to put (at least the
> > last part) in the commit log.
> 
> Like this:
> 
>     This patch disables raising an irq while loading the state of PCI bridge.
>     Because the i8259 has not been deserialized yet, raising an interrupt
>     could bring the system out-of-sync with the migration source.  For example,
>     the migration source could have masked the interrupt in the i8259. On the
>     destination, the i8259 device model would not know that yet and would
>     trigger an interrupt in the CPU.
>     
>     This patch eliminates raising an irq and just restores the calculated
>     state fields in post_load function.  Interrupt state will be deserialized
>     separately through the IRR field of the i8259.

Yes, thanks!
Except imho it's a bit better to s/raising/setting/ in the last paragraph.

> > Also it's updating irq state, not just raising irq,
> > that might be problematic, right?
> 
> Well, the i8259 is in the reset state so ISR=IRR=0, aka all IRQ lines 
> are known to be low.

By luck, yes.

>  But yes, in general it's the update that is 
> problematic.

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

[Paolo Bonzini]

Il 10/09/2014 13:04, Michael S. Tsirkin ha scritto:
>> >     This patch disables raising an irq while loading the state of PCI bridge.
>> >     Because the i8259 has not been deserialized yet, raising an interrupt
>> >     could bring the system out-of-sync with the migration source.  For example,
>> >     the migration source could have masked the interrupt in the i8259. On the
>> >     destination, the i8259 device model would not know that yet and would
>> >     trigger an interrupt in the CPU.
>> >     
>> >     This patch eliminates raising an irq and just restores the calculated
>> >     state fields in post_load function.  Interrupt state will be deserialized
>> >     separately through the IRR field of the i8259.
> Yes, thanks!
> Except imho it's a bit better to s/raising/setting/ in the last paragraph.

And pretty much everywhere else, not just in the last paragraph.

Thanks!

Paolo

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

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 12:07:22PM +0200, Paolo Bonzini wrote:
> Il 10/09/2014 13:04, Michael S. Tsirkin ha scritto:
> >> >     This patch disables raising an irq while loading the state of PCI bridge.
> >> >     Because the i8259 has not been deserialized yet, raising an interrupt
> >> >     could bring the system out-of-sync with the migration source.  For example,
> >> >     the migration source could have masked the interrupt in the i8259. On the
> >> >     destination, the i8259 device model would not know that yet and would
> >> >     trigger an interrupt in the CPU.
> >> >     
> >> >     This patch eliminates raising an irq and just restores the calculated
> >> >     state fields in post_load function.  Interrupt state will be deserialized
> >> >     separately through the IRR field of the i8259.
> > Yes, thanks!
> > Except imho it's a bit better to s/raising/setting/ in the last paragraph.
> 
> And pretty much everywhere else, not just in the last paragraph.
> 
> Thanks!
> 
> Paolo

Right. With these minor changes, you can attach

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>

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

[Paolo Bonzini]

From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>

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>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/timer/mc146818rtc.c | 24 +++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
index 17912b8..2c4b650 100644
--- a/hw/timer/mc146818rtc.c
+++ b/hw/timer/mc146818rtc.c
@@ -733,6 +733,22 @@ 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,
@@ -753,6 +769,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 */
+        }
     }
 };
 
-- 
2.1.0

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

[Juan Quintela]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> From: Pavel Dovgalyuk <Pavel.Dovgaluk@ispras.ru>
>
> 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>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Acked-by: Juan Quintela <quintela@redhat.com>

> ---
>  hw/timer/mc146818rtc.c | 24 +++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/hw/timer/mc146818rtc.c b/hw/timer/mc146818rtc.c
> index 17912b8..2c4b650 100644
> --- a/hw/timer/mc146818rtc.c
> +++ b/hw/timer/mc146818rtc.c
> @@ -733,6 +733,22 @@ 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;

If you have to resend for any reason, please remove this unneeded cast.


> +    return s->irq_reinject_on_ack_count != 0;
> +}
> +
>  static const VMStateDescription vmstate_rtc = {
>      .name = "mc146818rtc",
>      .version_id = 3,
> @@ -753,6 +769,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 */
> +        }
>      }
>  };

Re: [Qemu-devel] [PATCH 00/10] x86: migrate more data

[Paolo Bonzini]

Il 09/09/2014 14:29, Paolo Bonzini ha scritto:
> Juan, David, Amit, here are Pavel's fixes for x86 migration.
> Please help applying them, or ack them so that I can merge
> them through the KVM tree.
> 
> Thanks,
> 
> Paolo
> 
> Paolo Bonzini (1):
>   vl: use QLIST_FOREACH_SAFE to visit change state handlers
> 
> Pavel Dovgalyuk (9):
>   apic_common: vapic_paddr synchronization fix
>   cpu: init vmstate for ticks and clock offset
>   pcspk: adding vmstate for save/restore
>   fdc: adding vmstate for save/restore
>   parallel: adding vmstate for save/restore
>   serial: fixing vmstate for save/restore
>   pckbd: adding new fields to vmstate
>   piix: do not raise irq while loading vmstate
>   mc146818rtc: add missed field to vmstate
> 
>  cpus.c                 |   8 +-
>  hw/audio/pcspk.c       |  16 ++-
>  hw/block/fdc.c         |  74 ++++++++++++++
>  hw/char/parallel.c     |  18 ++++
>  hw/char/serial.c       | 265 ++++++++++++++++++++++++++++++++++++++++---------
>  hw/i386/kvmvapic.c     |  37 +++++--
>  hw/input/pckbd.c       |  51 ++++++++++
>  hw/pci-host/piix.c     |  26 ++++-
>  hw/timer/mc146818rtc.c |  25 +++++
>  include/qemu-common.h  |   2 +
>  vl.c                   |   5 +-
>  12 files changed, 463 insertions(+), 64 deletions(-)
> 

I've applied all patches except 4 to the uq/master branch.  Patch 4
deserves more discussion to see what to do about older machine types
(spoiler: my idea is "nothing" :)).

Paolo

Re: [Qemu-devel] [PATCH 00/10] x86: migrate more data

[Michael S. Tsirkin]

On Wed, Sep 10, 2014 at 12:50:35PM +0200, Paolo Bonzini wrote:
> Il 09/09/2014 14:29, Paolo Bonzini ha scritto:
> > Juan, David, Amit, here are Pavel's fixes for x86 migration.
> > Please help applying them, or ack them so that I can merge
> > them through the KVM tree.
> > 
> > Thanks,
> > 
> > Paolo
> > 
> > Paolo Bonzini (1):
> >   vl: use QLIST_FOREACH_SAFE to visit change state handlers
> > 
> > Pavel Dovgalyuk (9):
> >   apic_common: vapic_paddr synchronization fix
> >   cpu: init vmstate for ticks and clock offset
> >   pcspk: adding vmstate for save/restore
> >   fdc: adding vmstate for save/restore
> >   parallel: adding vmstate for save/restore
> >   serial: fixing vmstate for save/restore
> >   pckbd: adding new fields to vmstate
> >   piix: do not raise irq while loading vmstate
> >   mc146818rtc: add missed field to vmstate
> > 
> >  cpus.c                 |   8 +-
> >  hw/audio/pcspk.c       |  16 ++-
> >  hw/block/fdc.c         |  74 ++++++++++++++
> >  hw/char/parallel.c     |  18 ++++
> >  hw/char/serial.c       | 265 ++++++++++++++++++++++++++++++++++++++++---------
> >  hw/i386/kvmvapic.c     |  37 +++++--
> >  hw/input/pckbd.c       |  51 ++++++++++
> >  hw/pci-host/piix.c     |  26 ++++-
> >  hw/timer/mc146818rtc.c |  25 +++++
> >  include/qemu-common.h  |   2 +
> >  vl.c                   |   5 +-
> >  12 files changed, 463 insertions(+), 64 deletions(-)
> > 
> 
> I've applied all patches except 4 to the uq/master branch.  Patch 4
> deserves more discussion to see what to do about older machine types
> (spoiler: my idea is "nothing" :)).
> 
> Paolo

9/10 with a tweak to commit log/comments?

Re: [Qemu-devel] [PATCH 00/10] x86: migrate more data

[Paolo Bonzini]

Il 10/09/2014 13:56, Michael S. Tsirkin ha scritto:
>> > I've applied all patches except 4 to the uq/master branch.  Patch 4
>> > deserves more discussion to see what to do about older machine types
>> > (spoiler: my idea is "nothing" :)).
>> > 
>> > Paolo
> 9/10 with a tweak to commit log/comments?

Yes, of course.

https://git.kernel.org/cgit/virt/kvm/qemu-kvm.git/commit/?h=uq/master&id=b34ceddce16c32b33c59d50e77653cbac7d701c9

Paolo