[Qemu-devel] [PATCH] Migration compatibility for serial

[Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert (git)]

From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>

Older QEMUs dont understand the new (sub)sections that
may be generated in the serial device.   Limit their generation
to newer machine types.

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hw/char/serial.c         | 19 +++++++++++++------
 hw/i386/pc_piix.c        |  2 ++
 hw/i386/pc_q35.c         |  2 ++
 hw/ppc/spapr.c           |  2 ++
 include/hw/char/serial.h |  3 +++
 5 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 513d73c..ef31df3 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
 do {} while (0)
 #endif
 
+/* Force migration compatibility of pre-2.2 machine types */
+bool serial_migrate_pre_2_2;
+
 static void serial_receive1(void *opaque, const uint8_t *buf, int size);
 
 static inline void recv_fifo_put(SerialState *s, uint8_t chr)
@@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
 {
     SerialState *s = opaque;
 
+    if (serial_migrate_pre_2_2) {
+        return false;
+    }
+
     if (s->ier & UART_IER_THRI) {
         bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
         return s->thr_ipending != expected_value;
@@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
 static bool serial_tsr_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return s->tsr_retry != 0;
+    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
 }
 
 static const VMStateDescription vmstate_serial_tsr = {
@@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
 static bool serial_recv_fifo_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return !fifo8_is_empty(&s->recv_fifo);
+    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
 
 }
 
@@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
 static bool serial_xmit_fifo_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return !fifo8_is_empty(&s->xmit_fifo);
+    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
 }
 
 static const VMStateDescription vmstate_serial_xmit_fifo = {
@@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
 static bool serial_fifo_timeout_timer_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return timer_pending(s->fifo_timeout_timer);
+    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
 }
 
 static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
@@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
 static bool serial_timeout_ipending_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return s->timeout_ipending != 0;
+    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
 }
 
 static const VMStateDescription vmstate_serial_timeout_ipending = {
@@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
 static bool serial_poll_needed(void *opaque)
 {
     SerialState *s = (SerialState *)opaque;
-    return s->poll_msl >= 0;
+    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
 }
 
 static const VMStateDescription vmstate_serial_poll = {
diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
index e142f75..d6d596c 100644
--- a/hw/i386/pc_piix.c
+++ b/hw/i386/pc_piix.c
@@ -39,6 +39,7 @@
 #include "hw/kvm/clock.h"
 #include "sysemu/sysemu.h"
 #include "hw/sysbus.h"
+#include "hw/char/serial.h"
 #include "hw/cpu/icc_bus.h"
 #include "sysemu/arch_init.h"
 #include "sysemu/block-backend.h"
@@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
     x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
     pcms->enforce_aligned_dimm = false;
+    serial_migrate_pre_2_2 = true;
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
index b68263d..a211f21 100644
--- a/hw/i386/pc_q35.c
+++ b/hw/i386/pc_q35.c
@@ -32,6 +32,7 @@
 #include "sysemu/arch_init.h"
 #include "hw/i2c/smbus.h"
 #include "hw/boards.h"
+#include "hw/char/serial.h"
 #include "hw/timer/mc146818rtc.h"
 #include "hw/xen/xen.h"
 #include "sysemu/kvm.h"
@@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
     x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
     x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
+    serial_migrate_pre_2_2 = true;
 }
 
 static void pc_compat_2_0(MachineState *machine)
diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
index 01f8da8..f2673da 100644
--- a/hw/ppc/spapr.c
+++ b/hw/ppc/spapr.c
@@ -39,6 +39,7 @@
 #include "qom/cpu.h"
 
 #include "hw/boards.h"
+#include "hw/char/serial.h"
 #include "hw/ppc/ppc.h"
 #include "hw/loader.h"
 
@@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
 static void spapr_compat_2_1(Object *obj)
 {
     spapr_compat_2_2(obj);
+    serial_migrate_pre_2_2 = true;
 }
 
 static void spapr_machine_2_3_instance_init(Object *obj)
diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index 15beb6b..527d728 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
 #define TYPE_ISA_SERIAL "isa-serial"
 void serial_hds_isa_init(ISABus *bus, int n);
 
+/* Force migration compatibility of pre-2.2 machine types */
+extern bool serial_migrate_pre_2_2;
+
 #endif
-- 
2.4.3

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Tue, Jun 16, 2015 at 07:54:09PM +0100, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Older QEMUs dont understand the new (sub)sections that
> may be generated in the serial device.   Limit their generation
> to newer machine types.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

I don't see a problem with this, but the specific
interface is unfortunate: this spreads versioning
info around which we don't normally do.
Instead, please add specific flags, e.g.
	serial_has_recv_fifo

Also, is it really enough to just skip a bunch of
stuff on load? For example, I notice that before
	7385b275d9ae8bdf3c012bc4e2ae9779fcea6312
we used to generate interrupts on loadvm -
isn't that needed in this compat mode?



> ---
>  hw/char/serial.c         | 19 +++++++++++++------
>  hw/i386/pc_piix.c        |  2 ++
>  hw/i386/pc_q35.c         |  2 ++
>  hw/ppc/spapr.c           |  2 ++
>  include/hw/char/serial.h |  3 +++
>  5 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 513d73c..ef31df3 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
>  do {} while (0)
>  #endif
>  
> +/* Force migration compatibility of pre-2.2 machine types */
> +bool serial_migrate_pre_2_2;
> +
>  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
>  
>  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> @@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
>  {
>      SerialState *s = opaque;
>  
> +    if (serial_migrate_pre_2_2) {
> +        return false;
> +    }
> +
>      if (s->ier & UART_IER_THRI) {
>          bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
>          return s->thr_ipending != expected_value;
> @@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
>  static bool serial_tsr_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->tsr_retry != 0;
> +    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_tsr = {
> @@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
>  static bool serial_recv_fifo_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return !fifo8_is_empty(&s->recv_fifo);
> +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
>  
>  }
>  
> @@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
>  static bool serial_xmit_fifo_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return !fifo8_is_empty(&s->xmit_fifo);
> +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
>  }
>  
>  static const VMStateDescription vmstate_serial_xmit_fifo = {
> @@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
>  static bool serial_fifo_timeout_timer_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return timer_pending(s->fifo_timeout_timer);
> +    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
>  }
>  
>  static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> @@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
>  static bool serial_timeout_ipending_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->timeout_ipending != 0;
> +    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_timeout_ipending = {
> @@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
>  static bool serial_poll_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->poll_msl >= 0;
> +    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_poll = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e142f75..d6d596c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -39,6 +39,7 @@
>  #include "hw/kvm/clock.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/sysbus.h"
> +#include "hw/char/serial.h"
>  #include "hw/cpu/icc_bus.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/block-backend.h"
> @@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
>      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
>      pcms->enforce_aligned_dimm = false;
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b68263d..a211f21 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/arch_init.h"
>  #include "hw/i2c/smbus.h"
>  #include "hw/boards.h"
> +#include "hw/char/serial.h"
>  #include "hw/timer/mc146818rtc.h"
>  #include "hw/xen/xen.h"
>  #include "sysemu/kvm.h"
> @@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
>      x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01f8da8..f2673da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -39,6 +39,7 @@
>  #include "qom/cpu.h"
>  
>  #include "hw/boards.h"
> +#include "hw/char/serial.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/loader.h"
>  
> @@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
>  static void spapr_compat_2_1(Object *obj)
>  {
>      spapr_compat_2_2(obj);
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void spapr_machine_2_3_instance_init(Object *obj)
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 15beb6b..527d728 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>  #define TYPE_ISA_SERIAL "isa-serial"
>  void serial_hds_isa_init(ISABus *bus, int n);
>  
> +/* Force migration compatibility of pre-2.2 machine types */
> +extern bool serial_migrate_pre_2_2;
> +
>  #endif
> -- 
> 2.4.3

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Markus Armbruster]

"Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:

> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> Older QEMUs dont understand the new (sub)sections that
> may be generated in the serial device.   Limit their generation
> to newer machine types.

Please explain briefly what state migration can lose with old machine
types, and how this could impact the guest.  The commits adding the
subsections should have the information[*].  Pointers to them might
suffice.


[*] Should != do; I didn't check.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 08:52:42AM +0200, Markus Armbruster wrote:
> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> 
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >
> > Older QEMUs dont understand the new (sub)sections that
> > may be generated in the serial device.   Limit their generation
> > to newer machine types.
> 
> Please explain briefly what state migration can lose with old machine
> types, and how this could impact the guest.  The commits adding the
> subsections should have the information[*].  Pointers to them might
> suffice.
> 
> 
> [*] Should != do; I didn't check.

OTOH if we have flags named after specific portions we skip,
and not after qemu versions, this becomes evident from code.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Markus Armbruster]

"Michael S. Tsirkin" <mst@redhat.com> writes:

> On Wed, Jun 17, 2015 at 08:52:42AM +0200, Markus Armbruster wrote:
>> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
>> 
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> >
>> > Older QEMUs dont understand the new (sub)sections that
>> > may be generated in the serial device.   Limit their generation
>> > to newer machine types.
>> 
>> Please explain briefly what state migration can lose with old machine
>> types, and how this could impact the guest.  The commits adding the
>> subsections should have the information[*].  Pointers to them might
>> suffice.
>> 
>> 
>> [*] Should != do; I didn't check.
>
> OTOH if we have flags named after specific portions we skip,
> and not after qemu versions, this becomes evident from code.

The commit message needs to spell things out regardless.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 09:11:56AM +0200, Markus Armbruster wrote:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
> > On Wed, Jun 17, 2015 at 08:52:42AM +0200, Markus Armbruster wrote:
> >> "Dr. David Alan Gilbert (git)" <dgilbert@redhat.com> writes:
> >> 
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> >
> >> > Older QEMUs dont understand the new (sub)sections that
> >> > may be generated in the serial device.   Limit their generation
> >> > to newer machine types.
> >> 
> >> Please explain briefly what state migration can lose with old machine
> >> types, and how this could impact the guest.  The commits adding the
> >> subsections should have the information[*].  Pointers to them might
> >> suffice.
> >> 
> >> 
> >> [*] Should != do; I didn't check.
> >
> > OTOH if we have flags named after specific portions we skip,
> > and not after qemu versions, this becomes evident from code.
> 
> The commit message needs to spell things out regardless.

Sure, thanks for pointing this out.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> 
> Older QEMUs dont understand the new (sub)sections that
> may be generated in the serial device.   Limit their generation
> to newer machine types.
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

No, please.  Upstream QEMU doesn't want to get into judgement about when
migration quality might be "good enough" that you can drop subsections.
 It's one thing to perfect the .needed functions to make the appearance
of subsections as unlikely as possible, but adding flags is not
something we've done so far---and not something at least *I* want to do.

Paolo


> ---
>  hw/char/serial.c         | 19 +++++++++++++------
>  hw/i386/pc_piix.c        |  2 ++
>  hw/i386/pc_q35.c         |  2 ++
>  hw/ppc/spapr.c           |  2 ++
>  include/hw/char/serial.h |  3 +++
>  5 files changed, 22 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 513d73c..ef31df3 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
>  do {} while (0)
>  #endif
>  
> +/* Force migration compatibility of pre-2.2 machine types */
> +bool serial_migrate_pre_2_2;
> +
>  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
>  
>  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> @@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
>  {
>      SerialState *s = opaque;
>  
> +    if (serial_migrate_pre_2_2) {
> +        return false;
> +    }
> +
>      if (s->ier & UART_IER_THRI) {
>          bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
>          return s->thr_ipending != expected_value;
> @@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
>  static bool serial_tsr_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->tsr_retry != 0;
> +    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_tsr = {
> @@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
>  static bool serial_recv_fifo_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return !fifo8_is_empty(&s->recv_fifo);
> +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
>  
>  }
>  
> @@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
>  static bool serial_xmit_fifo_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return !fifo8_is_empty(&s->xmit_fifo);
> +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
>  }
>  
>  static const VMStateDescription vmstate_serial_xmit_fifo = {
> @@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
>  static bool serial_fifo_timeout_timer_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return timer_pending(s->fifo_timeout_timer);
> +    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
>  }
>  
>  static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> @@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
>  static bool serial_timeout_ipending_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->timeout_ipending != 0;
> +    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_timeout_ipending = {
> @@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
>  static bool serial_poll_needed(void *opaque)
>  {
>      SerialState *s = (SerialState *)opaque;
> -    return s->poll_msl >= 0;
> +    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
>  }
>  
>  static const VMStateDescription vmstate_serial_poll = {
> diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> index e142f75..d6d596c 100644
> --- a/hw/i386/pc_piix.c
> +++ b/hw/i386/pc_piix.c
> @@ -39,6 +39,7 @@
>  #include "hw/kvm/clock.h"
>  #include "sysemu/sysemu.h"
>  #include "hw/sysbus.h"
> +#include "hw/char/serial.h"
>  #include "hw/cpu/icc_bus.h"
>  #include "sysemu/arch_init.h"
>  #include "sysemu/block-backend.h"
> @@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
>      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
>      pcms->enforce_aligned_dimm = false;
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> index b68263d..a211f21 100644
> --- a/hw/i386/pc_q35.c
> +++ b/hw/i386/pc_q35.c
> @@ -32,6 +32,7 @@
>  #include "sysemu/arch_init.h"
>  #include "hw/i2c/smbus.h"
>  #include "hw/boards.h"
> +#include "hw/char/serial.h"
>  #include "hw/timer/mc146818rtc.h"
>  #include "hw/xen/xen.h"
>  #include "sysemu/kvm.h"
> @@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
>      x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
>      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void pc_compat_2_0(MachineState *machine)
> diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> index 01f8da8..f2673da 100644
> --- a/hw/ppc/spapr.c
> +++ b/hw/ppc/spapr.c
> @@ -39,6 +39,7 @@
>  #include "qom/cpu.h"
>  
>  #include "hw/boards.h"
> +#include "hw/char/serial.h"
>  #include "hw/ppc/ppc.h"
>  #include "hw/loader.h"
>  
> @@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
>  static void spapr_compat_2_1(Object *obj)
>  {
>      spapr_compat_2_2(obj);
> +    serial_migrate_pre_2_2 = true;
>  }
>  
>  static void spapr_machine_2_3_instance_init(Object *obj)
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index 15beb6b..527d728 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>  #define TYPE_ISA_SERIAL "isa-serial"
>  void serial_hds_isa_init(ISABus *bus, int n);
>  
> +/* Force migration compatibility of pre-2.2 machine types */
> +extern bool serial_migrate_pre_2_2;
> +
>  #endif
> 

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 09:41:49AM +0200, Paolo Bonzini wrote:
> 
> 
> On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Older QEMUs dont understand the new (sub)sections that
> > may be generated in the serial device.   Limit their generation
> > to newer machine types.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> No, please.  Upstream QEMU doesn't want to get into judgement about when
> migration quality might be "good enough" that you can drop subsections.
>  It's one thing to perfect the .needed functions to make the appearance
> of subsections as unlikely as possible, but adding flags is not
> something we've done so far---and not something at least *I* want to do.
> 
> Paolo

Not like this, sure.  But e.g. patches that force specific fields to
behave in a way consistent with QEMU 2.2, with appropriate
doducmentation would be ok I think.

> 
> > ---
> >  hw/char/serial.c         | 19 +++++++++++++------
> >  hw/i386/pc_piix.c        |  2 ++
> >  hw/i386/pc_q35.c         |  2 ++
> >  hw/ppc/spapr.c           |  2 ++
> >  include/hw/char/serial.h |  3 +++
> >  5 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 513d73c..ef31df3 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
> >  do {} while (0)
> >  #endif
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +bool serial_migrate_pre_2_2;
> > +
> >  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> >  
> >  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> > @@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = opaque;
> >  
> > +    if (serial_migrate_pre_2_2) {
> > +        return false;
> > +    }
> > +
> >      if (s->ier & UART_IER_THRI) {
> >          bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> >          return s->thr_ipending != expected_value;
> > @@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
> >  static bool serial_tsr_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->tsr_retry != 0;
> > +    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_tsr = {
> > @@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
> >  static bool serial_recv_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->recv_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
> >  
> >  }
> >  
> > @@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
> >  static bool serial_xmit_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->xmit_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_xmit_fifo = {
> > @@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
> >  static bool serial_fifo_timeout_timer_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return timer_pending(s->fifo_timeout_timer);
> > +    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> > @@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> >  static bool serial_timeout_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->timeout_ipending != 0;
> > +    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_timeout_ipending = {
> > @@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
> >  static bool serial_poll_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->poll_msl >= 0;
> > +    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_poll = {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index e142f75..d6d596c 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -39,6 +39,7 @@
> >  #include "hw/kvm/clock.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/cpu/icc_bus.h"
> >  #include "sysemu/arch_init.h"
> >  #include "sysemu/block-backend.h"
> > @@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> >      pcms->enforce_aligned_dimm = false;
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index b68263d..a211f21 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -32,6 +32,7 @@
> >  #include "sysemu/arch_init.h"
> >  #include "hw/i2c/smbus.h"
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/timer/mc146818rtc.h"
> >  #include "hw/xen/xen.h"
> >  #include "sysemu/kvm.h"
> > @@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 01f8da8..f2673da 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -39,6 +39,7 @@
> >  #include "qom/cpu.h"
> >  
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "hw/loader.h"
> >  
> > @@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
> >  static void spapr_compat_2_1(Object *obj)
> >  {
> >      spapr_compat_2_2(obj);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void spapr_machine_2_3_instance_init(Object *obj)
> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> > index 15beb6b..527d728 100644
> > --- a/include/hw/char/serial.h
> > +++ b/include/hw/char/serial.h
> > @@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
> >  #define TYPE_ISA_SERIAL "isa-serial"
> >  void serial_hds_isa_init(ISABus *bus, int n);
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +extern bool serial_migrate_pre_2_2;
> > +
> >  #endif
> > 

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > migration quality might be "good enough" that you can drop subsections.
> >  It's one thing to perfect the .needed functions to make the appearance
> > of subsections as unlikely as possible, but adding flags is not
> > something we've done so far---and not something at least *I* want to do.
> 
> Not like this, sure.  But e.g. patches that force specific fields to
> behave in a way consistent with QEMU 2.2, with appropriate
> doducmentation would be ok I think.

That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
as 2.2", not "bug-compatible with 2.2".

Refining the .needed functions (e.g. see commit bfa7362889) is just
that: describing when a subsection is needed.  Forcing specific fields
to behave in a way consistent with QEMU 2.2 is bug compatibility.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Older QEMUs dont understand the new (sub)sections that
> > may be generated in the serial device.   Limit their generation
> > to newer machine types.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> No, please.  Upstream QEMU doesn't want to get into judgement about when
> migration quality might be "good enough" that you can drop subsections.

Other people disagree with that statement.
Who upstream doesn't want it?

Dave


>  It's one thing to perfect the .needed functions to make the appearance
> of subsections as unlikely as possible, but adding flags is not
> something we've done so far---and not something at least *I* want to do.


> 
> Paolo
> 
> 
> > ---
> >  hw/char/serial.c         | 19 +++++++++++++------
> >  hw/i386/pc_piix.c        |  2 ++
> >  hw/i386/pc_q35.c         |  2 ++
> >  hw/ppc/spapr.c           |  2 ++
> >  include/hw/char/serial.h |  3 +++
> >  5 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 513d73c..ef31df3 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
> >  do {} while (0)
> >  #endif
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +bool serial_migrate_pre_2_2;
> > +
> >  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> >  
> >  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> > @@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = opaque;
> >  
> > +    if (serial_migrate_pre_2_2) {
> > +        return false;
> > +    }
> > +
> >      if (s->ier & UART_IER_THRI) {
> >          bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> >          return s->thr_ipending != expected_value;
> > @@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
> >  static bool serial_tsr_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->tsr_retry != 0;
> > +    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_tsr = {
> > @@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
> >  static bool serial_recv_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->recv_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
> >  
> >  }
> >  
> > @@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
> >  static bool serial_xmit_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->xmit_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_xmit_fifo = {
> > @@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
> >  static bool serial_fifo_timeout_timer_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return timer_pending(s->fifo_timeout_timer);
> > +    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> > @@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> >  static bool serial_timeout_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->timeout_ipending != 0;
> > +    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_timeout_ipending = {
> > @@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
> >  static bool serial_poll_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->poll_msl >= 0;
> > +    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_poll = {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index e142f75..d6d596c 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -39,6 +39,7 @@
> >  #include "hw/kvm/clock.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/cpu/icc_bus.h"
> >  #include "sysemu/arch_init.h"
> >  #include "sysemu/block-backend.h"
> > @@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> >      pcms->enforce_aligned_dimm = false;
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index b68263d..a211f21 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -32,6 +32,7 @@
> >  #include "sysemu/arch_init.h"
> >  #include "hw/i2c/smbus.h"
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/timer/mc146818rtc.h"
> >  #include "hw/xen/xen.h"
> >  #include "sysemu/kvm.h"
> > @@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 01f8da8..f2673da 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -39,6 +39,7 @@
> >  #include "qom/cpu.h"
> >  
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "hw/loader.h"
> >  
> > @@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
> >  static void spapr_compat_2_1(Object *obj)
> >  {
> >      spapr_compat_2_2(obj);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void spapr_machine_2_3_instance_init(Object *obj)
> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> > index 15beb6b..527d728 100644
> > --- a/include/hw/char/serial.h
> > +++ b/include/hw/char/serial.h
> > @@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
> >  #define TYPE_ISA_SERIAL "isa-serial"
> >  void serial_hds_isa_init(ISABus *bus, int n);
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +extern bool serial_migrate_pre_2_2;
> > +
> >  #endif
> > 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 10:37, Dr. David Alan Gilbert wrote:
>> > On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
>> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> > > 
>> > > Older QEMUs dont understand the new (sub)sections that
>> > > may be generated in the serial device.   Limit their generation
>> > > to newer machine types.
>> > > 
>> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > migration quality might be "good enough" that you can drop subsections.
> 
> Other people disagree with that statement.
> Who upstream doesn't want it?

That's always been the policy as far as I know.  Certainly I don't.
When we were working on RHEL7, there were quite a few discussions about
this; I remember Orit Wassermann also was a proponent of the "clean
slate" approach.

The problem is that if you want bug compatibility, you also want a point
(e.g. a major release) where you can start from a clean slate and drop
all compatibility hacks.  Upstream there is no such point.

It's already hard enough to ensure compatibility of versioned machine
types, which are static, up to QEMU 0.10 or so; imagine what it would be
like to guarantee the same for migration six or seven years down the
line, considering how extremely data-driven migration is.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Tue, Jun 16, 2015 at 07:54:09PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > 
> > Older QEMUs dont understand the new (sub)sections that
> > may be generated in the serial device.   Limit their generation
> > to newer machine types.
> > 
> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> 
> I don't see a problem with this, but the specific
> interface is unfortunate: this spreads versioning
> info around which we don't normally do.
> Instead, please add specific flags, e.g.
> 	serial_has_recv_fifo

Yes, I could do; although note that they are all added
at the same time, and the 'serial_migrate_pre_2_2' name does
make it clear it's a compatibility thing rather than
a functional one.

> Also, is it really enough to just skip a bunch of
> stuff on load? For example, I notice that before
> 	7385b275d9ae8bdf3c012bc4e2ae9779fcea6312
> we used to generate interrupts on loadvm -
> isn't that needed in this compat mode?

Can you point me at the change in there?  I can see that
afer 7385b27 it's better at regenerating thr_ipending
even in the case where the section wasn't present;
but that seems to be the opposite of what you're pointing
out.

Dave

> 
> 
> 
> > ---
> >  hw/char/serial.c         | 19 +++++++++++++------
> >  hw/i386/pc_piix.c        |  2 ++
> >  hw/i386/pc_q35.c         |  2 ++
> >  hw/ppc/spapr.c           |  2 ++
> >  include/hw/char/serial.h |  3 +++
> >  5 files changed, 22 insertions(+), 6 deletions(-)
> > 
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 513d73c..ef31df3 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -103,6 +103,9 @@ do { fprintf(stderr, "serial: " fmt , ## __VA_ARGS__); } while (0)
> >  do {} while (0)
> >  #endif
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +bool serial_migrate_pre_2_2;
> > +
> >  static void serial_receive1(void *opaque, const uint8_t *buf, int size);
> >  
> >  static inline void recv_fifo_put(SerialState *s, uint8_t chr)
> > @@ -646,6 +649,10 @@ static bool serial_thr_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = opaque;
> >  
> > +    if (serial_migrate_pre_2_2) {
> > +        return false;
> > +    }
> > +
> >      if (s->ier & UART_IER_THRI) {
> >          bool expected_value = ((s->iir & UART_IIR_ID) == UART_IIR_THRI);
> >          return s->thr_ipending != expected_value;
> > @@ -672,7 +679,7 @@ static const VMStateDescription vmstate_serial_thr_ipending = {
> >  static bool serial_tsr_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->tsr_retry != 0;
> > +    return !serial_migrate_pre_2_2 && s->tsr_retry != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_tsr = {
> > @@ -691,7 +698,7 @@ static const VMStateDescription vmstate_serial_tsr = {
> >  static bool serial_recv_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->recv_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->recv_fifo);
> >  
> >  }
> >  
> > @@ -709,7 +716,7 @@ static const VMStateDescription vmstate_serial_recv_fifo = {
> >  static bool serial_xmit_fifo_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return !fifo8_is_empty(&s->xmit_fifo);
> > +    return !serial_migrate_pre_2_2 && !fifo8_is_empty(&s->xmit_fifo);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_xmit_fifo = {
> > @@ -726,7 +733,7 @@ static const VMStateDescription vmstate_serial_xmit_fifo = {
> >  static bool serial_fifo_timeout_timer_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return timer_pending(s->fifo_timeout_timer);
> > +    return !serial_migrate_pre_2_2 && timer_pending(s->fifo_timeout_timer);
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> > @@ -743,7 +750,7 @@ static const VMStateDescription vmstate_serial_fifo_timeout_timer = {
> >  static bool serial_timeout_ipending_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->timeout_ipending != 0;
> > +    return !serial_migrate_pre_2_2 && s->timeout_ipending != 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_timeout_ipending = {
> > @@ -760,7 +767,7 @@ static const VMStateDescription vmstate_serial_timeout_ipending = {
> >  static bool serial_poll_needed(void *opaque)
> >  {
> >      SerialState *s = (SerialState *)opaque;
> > -    return s->poll_msl >= 0;
> > +    return !serial_migrate_pre_2_2 && s->poll_msl >= 0;
> >  }
> >  
> >  static const VMStateDescription vmstate_serial_poll = {
> > diff --git a/hw/i386/pc_piix.c b/hw/i386/pc_piix.c
> > index e142f75..d6d596c 100644
> > --- a/hw/i386/pc_piix.c
> > +++ b/hw/i386/pc_piix.c
> > @@ -39,6 +39,7 @@
> >  #include "hw/kvm/clock.h"
> >  #include "sysemu/sysemu.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/cpu/icc_bus.h"
> >  #include "sysemu/arch_init.h"
> >  #include "sysemu/block-backend.h"
> > @@ -344,6 +345,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> >      pcms->enforce_aligned_dimm = false;
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/i386/pc_q35.c b/hw/i386/pc_q35.c
> > index b68263d..a211f21 100644
> > --- a/hw/i386/pc_q35.c
> > +++ b/hw/i386/pc_q35.c
> > @@ -32,6 +32,7 @@
> >  #include "sysemu/arch_init.h"
> >  #include "hw/i2c/smbus.h"
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/timer/mc146818rtc.h"
> >  #include "hw/xen/xen.h"
> >  #include "sysemu/kvm.h"
> > @@ -328,6 +329,7 @@ static void pc_compat_2_1(MachineState *machine)
> >      x86_cpu_compat_set_features("coreduo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_set_features("core2duo", FEAT_1_ECX, CPUID_EXT_VMX, 0);
> >      x86_cpu_compat_kvm_no_autodisable(FEAT_8000_0001_ECX, CPUID_EXT3_SVM);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void pc_compat_2_0(MachineState *machine)
> > diff --git a/hw/ppc/spapr.c b/hw/ppc/spapr.c
> > index 01f8da8..f2673da 100644
> > --- a/hw/ppc/spapr.c
> > +++ b/hw/ppc/spapr.c
> > @@ -39,6 +39,7 @@
> >  #include "qom/cpu.h"
> >  
> >  #include "hw/boards.h"
> > +#include "hw/char/serial.h"
> >  #include "hw/ppc/ppc.h"
> >  #include "hw/loader.h"
> >  
> > @@ -1863,6 +1864,7 @@ static void spapr_compat_2_2(Object *obj)
> >  static void spapr_compat_2_1(Object *obj)
> >  {
> >      spapr_compat_2_2(obj);
> > +    serial_migrate_pre_2_2 = true;
> >  }
> >  
> >  static void spapr_machine_2_3_instance_init(Object *obj)
> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> > index 15beb6b..527d728 100644
> > --- a/include/hw/char/serial.h
> > +++ b/include/hw/char/serial.h
> > @@ -94,4 +94,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
> >  #define TYPE_ISA_SERIAL "isa-serial"
> >  void serial_hds_isa_init(ISABus *bus, int n);
> >  
> > +/* Force migration compatibility of pre-2.2 machine types */
> > +extern bool serial_migrate_pre_2_2;
> > +
> >  #endif
> > -- 
> > 2.4.3
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jun 17, 2015 at 09:41:49AM +0200, Paolo Bonzini wrote:
>> 
>> 
>> On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
>> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>> > 
>> > Older QEMUs dont understand the new (sub)sections that
>> > may be generated in the serial device.   Limit their generation
>> > to newer machine types.
>> > 
>> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> 
>> No, please.  Upstream QEMU doesn't want to get into judgement about when
>> migration quality might be "good enough" that you can drop subsections.
>>  It's one thing to perfect the .needed functions to make the appearance
>> of subsections as unlikely as possible, but adding flags is not
>> something we've done so far---and not something at least *I* want to do.
>> 
>> Paolo
>
> Not like this, sure.  But e.g. patches that force specific fields to
> behave in a way consistent with QEMU 2.2, with appropriate
> doducmentation would be ok I think.

It is not consistent.  It is that in 2.2 are broken.
The case of the "broken" thing is that some users don't matter.  Think
that you have a getty listening in a serial console.  Some users don't
care about breaking serial console during migration because they are not
using serial console, it just happens that for some reason, it has been
configured.

So, the problem we have here is:

- pre 2.3: serial sometimes didn't migrate correctly
- post 2.4: we migrate serial correctly (always)

We can get the old behaviour (that is what this series do), but that
just mean that we _know_ that we are breaking serial during migration.
Without this patch, we only send the serial information if we are using
serial.

Why is this case special?  It appears that it is normal to have
syslog/getty whatever on a serial, users didn't notice that they have it
enabled, and now migration is not working and it used to work.

On the other hand, there are the users that were using serial, and now
it works correctly for them.

Correct thing to do: NO to apply this series.

Easier and more userfriendly thing: apply the series.

I preffer (for upstream) not applying the series, as they are incorrect.
On the other hand, we have applied them on downstream (RHEL), so you can
see that they are kinda useful.

You can't have both things.

As said, I preffer for upstream the "correct" behaviour, even if that is
a bit less userfriendly.  But I can understand why (in this case mst)
disagree on this.

Later, Juan.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 17/06/2015 10:37, Dr. David Alan Gilbert wrote:
> >> > On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
> >> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> > > 
> >> > > Older QEMUs dont understand the new (sub)sections that
> >> > > may be generated in the serial device.   Limit their generation
> >> > > to newer machine types.
> >> > > 
> >> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > > 
> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > > migration quality might be "good enough" that you can drop subsections.
> > 
> > Other people disagree with that statement.
> > Who upstream doesn't want it?
> 
> That's always been the policy as far as I know.  Certainly I don't.
> When we were working on RHEL7, there were quite a few discussions about
> this; I remember Orit Wassermann also was a proponent of the "clean
> slate" approach.

> The problem is that if you want bug compatibility, you also want a point
> (e.g. a major release) where you can start from a clean slate and drop
> all compatibility hacks.  Upstream there is no such point.

It doesn't necessarily have to be a clean slate; the other way to do it is
to have a version cut off, so you don't have any bug-compatibility
prior to a particular version, and then roll that cut off point forward,
much in the same way we do for glib version dependencies.
My flag naming of 'serial_migrate_pre_2_2' at least made it clear
where the line was for that feature.

> It's already hard enough to ensure compatibility of versioned machine
> types, which are static, up to QEMU 0.10 or so; imagine what it would be
> like to guarantee the same for migration six or seven years down the
> line, considering how extremely data-driven migration is.

I agree it's not easy, and indeed this serial fix is just one of a whole
bunch of fixes that are needed.  But if you never try then you never
get any compatibility.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > > migration quality might be "good enough" that you can drop subsections.
> > >  It's one thing to perfect the .needed functions to make the appearance
> > > of subsections as unlikely as possible, but adding flags is not
> > > something we've done so far---and not something at least *I* want to do.
> > 
> > Not like this, sure.  But e.g. patches that force specific fields to
> > behave in a way consistent with QEMU 2.2, with appropriate
> > doducmentation would be ok I think.
> 
> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> as 2.2", not "bug-compatible with 2.2".
> 
> Refining the .needed functions (e.g. see commit bfa7362889) is just
> that: describing when a subsection is needed.  Forcing specific fields
> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> 
> Paolo

We do bug-compatible if it's not a big pain, too.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 11:26:07AM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Jun 17, 2015 at 09:41:49AM +0200, Paolo Bonzini wrote:
> >> 
> >> 
> >> On 16/06/2015 20:54, Dr. David Alan Gilbert (git) wrote:
> >> > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> >> > 
> >> > Older QEMUs dont understand the new (sub)sections that
> >> > may be generated in the serial device.   Limit their generation
> >> > to newer machine types.
> >> > 
> >> > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> 
> >> No, please.  Upstream QEMU doesn't want to get into judgement about when
> >> migration quality might be "good enough" that you can drop subsections.
> >>  It's one thing to perfect the .needed functions to make the appearance
> >> of subsections as unlikely as possible, but adding flags is not
> >> something we've done so far---and not something at least *I* want to do.
> >> 
> >> Paolo
> >
> > Not like this, sure.  But e.g. patches that force specific fields to
> > behave in a way consistent with QEMU 2.2, with appropriate
> > doducmentation would be ok I think.
> 
> It is not consistent.  It is that in 2.2 are broken.
> The case of the "broken" thing is that some users don't matter.  Think
> that you have a getty listening in a serial console.  Some users don't
> care about breaking serial console during migration because they are not
> using serial console, it just happens that for some reason, it has been
> configured.
> 
> So, the problem we have here is:
> 
> - pre 2.3: serial sometimes didn't migrate correctly
> - post 2.4: we migrate serial correctly (always)
> 
> We can get the old behaviour (that is what this series do), but that
> just mean that we _know_ that we are breaking serial during migration.
> Without this patch, we only send the serial information if we are using
> serial.
> 
> Why is this case special?  It appears that it is normal to have
> syslog/getty whatever on a serial, users didn't notice that they have it
> enabled, and now migration is not working and it used to work.
> 
> On the other hand, there are the users that were using serial, and now
> it works correctly for them.
>
> Correct thing to do: NO to apply this series.
> 
> Easier and more userfriendly thing: apply the series.
> 
> I preffer (for upstream) not applying the series, as they are incorrect.
> On the other hand, we have applied them on downstream (RHEL), so you can
> see that they are kinda useful.
> 
> You can't have both things.
> 
> As said, I preffer for upstream the "correct" behaviour, even if that is
> a bit less userfriendly.  But I can understand why (in this case mst)
> disagree on this.
> 
> Later, Juan.

I think we need to be more specific. I think that under typical use the
new sections do not appear, and migration just works.
Under stress, some info is needed that was missing in qemu 2.2.
What's the result under 2.2? I'm guessing either lost characters or
broken serial.  Our current code instead breaks migration to 2.2.

If we can make do with just losing characters, then I think
it's better than breaking migration.

OTOH if migration breaks serial completely, it is harder to decide.

Which is it?


-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
>> 
>> 
>> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
>> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
>> > > migration quality might be "good enough" that you can drop subsections.
>> > >  It's one thing to perfect the .needed functions to make the appearance
>> > > of subsections as unlikely as possible, but adding flags is not
>> > > something we've done so far---and not something at least *I* want to do.
>> > 
>> > Not like this, sure.  But e.g. patches that force specific fields to
>> > behave in a way consistent with QEMU 2.2, with appropriate
>> > doducmentation would be ok I think.
>> 
>> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
>> as 2.2", not "bug-compatible with 2.2".
>> 
>> Refining the .needed functions (e.g. see commit bfa7362889) is just
>> that: describing when a subsection is needed.  Forcing specific fields
>> to behave in a way consistent with QEMU 2.2 is bug compatibility.
>> 
>> Paolo
>
> We do bug-compatible if it's not a big pain, too.

In this case, there is disagreement about what is better:
- correct solution
- bug compatible

We can't have both in this case :-(

Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
something like:

Source: 2.2  Destination: 2.2 -> bug compatible 2.2
Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always


So the problem is 2.3 -> 2.2 when serial is being used (notice that just
opening it it is using).  That is what we are differing about what is
the right thing to do.  As Paolo says, in upstream, we have done in the
past the correct thing, in downstream, it depends.

Notice that adding this patch makes that the three cases are bug
compatible, i.e. there is no way to detect breakage neither a way to fix
the issue (fix without the patch is just upgrade both binaries.
)

Later, Juan.

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> >> 
> >> 
> >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> >> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> >> > > migration quality might be "good enough" that you can drop subsections.
> >> > >  It's one thing to perfect the .needed functions to make the appearance
> >> > > of subsections as unlikely as possible, but adding flags is not
> >> > > something we've done so far---and not something at least *I* want to do.
> >> > 
> >> > Not like this, sure.  But e.g. patches that force specific fields to
> >> > behave in a way consistent with QEMU 2.2, with appropriate
> >> > doducmentation would be ok I think.
> >> 
> >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> >> as 2.2", not "bug-compatible with 2.2".
> >> 
> >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> >> that: describing when a subsection is needed.  Forcing specific fields
> >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> >> 
> >> Paolo
> >
> > We do bug-compatible if it's not a big pain, too.
> 
> In this case, there is disagreement about what is better:
> - correct solution
> - bug compatible
> 
> We can't have both in this case :-(
> 
> Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
> pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
> something like:
> 
> Source: 2.2  Destination: 2.2 -> bug compatible 2.2
> Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
> Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always

To be fair the 2.3->2.2 is more subtle; opening it is unlikely
to generate the subsections; it needs a bit more than that (certainly on Linux)
figuring out exactly what triggers each subsection is trickier.

Dave

> 
> 
> So the problem is 2.3 -> 2.2 when serial is being used (notice that just
> opening it it is using).  That is what we are differing about what is
> the right thing to do.  As Paolo says, in upstream, we have done in the
> past the correct thing, in downstream, it depends.
> 
> Notice that adding this patch makes that the three cases are bug
> compatible, i.e. there is no way to detect breakage neither a way to fix
> the issue (fix without the patch is just upgrade both binaries.
> )
> 
> Later, Juan.
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 12:48:00PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> >> 
> >> 
> >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> >> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> >> > > migration quality might be "good enough" that you can drop subsections.
> >> > >  It's one thing to perfect the .needed functions to make the appearance
> >> > > of subsections as unlikely as possible, but adding flags is not
> >> > > something we've done so far---and not something at least *I* want to do.
> >> > 
> >> > Not like this, sure.  But e.g. patches that force specific fields to
> >> > behave in a way consistent with QEMU 2.2, with appropriate
> >> > doducmentation would be ok I think.
> >> 
> >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> >> as 2.2", not "bug-compatible with 2.2".
> >> 
> >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> >> that: describing when a subsection is needed.  Forcing specific fields
> >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> >> 
> >> Paolo
> >
> > We do bug-compatible if it's not a big pain, too.
> 
> In this case, there is disagreement about what is better:
> - correct solution
> - bug compatible
> 
> We can't have both in this case :-(

I think this depends on how major and/or common the bug is.

> Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
> pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
> something like:
> 
> Source: 2.2  Destination: 2.2 -> bug compatible 2.2
> Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
> Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always
> 
> 
> So the problem is 2.3 -> 2.2 when serial is being used (notice that just
> opening it it is using).  That is what we are differing about what is
> the right thing to do.  As Paolo says, in upstream, we have done in the
> past the correct thing, in downstream, it depends.
> 
> Notice that adding this patch makes that the three cases are bug
> compatible, i.e. there is no way to detect breakage neither a way to fix
> the issue (fix without the patch is just upgrade both binaries.
> )
> 
> Later, Juan.

I would say it's the "correct" thing. We don't really know for sure
it's correct, we think so.  We might in theory have bugs in 2.4 code
too.  Whether we are better off failing migration depends on how severe
the bugs are.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 11:51:57AM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> > >> 
> > >> 
> > >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> > >> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > >> > > migration quality might be "good enough" that you can drop subsections.
> > >> > >  It's one thing to perfect the .needed functions to make the appearance
> > >> > > of subsections as unlikely as possible, but adding flags is not
> > >> > > something we've done so far---and not something at least *I* want to do.
> > >> > 
> > >> > Not like this, sure.  But e.g. patches that force specific fields to
> > >> > behave in a way consistent with QEMU 2.2, with appropriate
> > >> > doducmentation would be ok I think.
> > >> 
> > >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> > >> as 2.2", not "bug-compatible with 2.2".
> > >> 
> > >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> > >> that: describing when a subsection is needed.  Forcing specific fields
> > >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> > >> 
> > >> Paolo
> > >
> > > We do bug-compatible if it's not a big pain, too.
> > 
> > In this case, there is disagreement about what is better:
> > - correct solution
> > - bug compatible
> > 
> > We can't have both in this case :-(
> > 
> > Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
> > pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
> > something like:
> > 
> > Source: 2.2  Destination: 2.2 -> bug compatible 2.2
> > Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
> > Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always
> 
> To be fair the 2.3->2.2 is more subtle; opening it is unlikely
> to generate the subsections; it needs a bit more than that (certainly on Linux)
> figuring out exactly what triggers each subsection is trickier.
> 
> Dave

And more importantly, what is the result of skipping them,
like you proposed. E.g. if guests crash that's no
better than failing migration.

> > 
> > 
> > So the problem is 2.3 -> 2.2 when serial is being used (notice that just
> > opening it it is using).  That is what we are differing about what is
> > the right thing to do.  As Paolo says, in upstream, we have done in the
> > past the correct thing, in downstream, it depends.
> > 
> > Notice that adding this patch makes that the three cases are bug
> > compatible, i.e. there is no way to detect breakage neither a way to fix
> > the issue (fix without the patch is just upgrade both binaries.
> > )
> > 
> > Later, Juan.
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Michael S. Tsirkin (mst@redhat.com) wrote:
> On Wed, Jun 17, 2015 at 11:51:57AM +0100, Dr. David Alan Gilbert wrote:
> > * Juan Quintela (quintela@redhat.com) wrote:
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> > > >> 
> > > >> 
> > > >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> > > >> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > > >> > > migration quality might be "good enough" that you can drop subsections.
> > > >> > >  It's one thing to perfect the .needed functions to make the appearance
> > > >> > > of subsections as unlikely as possible, but adding flags is not
> > > >> > > something we've done so far---and not something at least *I* want to do.
> > > >> > 
> > > >> > Not like this, sure.  But e.g. patches that force specific fields to
> > > >> > behave in a way consistent with QEMU 2.2, with appropriate
> > > >> > doducmentation would be ok I think.
> > > >> 
> > > >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> > > >> as 2.2", not "bug-compatible with 2.2".
> > > >> 
> > > >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> > > >> that: describing when a subsection is needed.  Forcing specific fields
> > > >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> > > >> 
> > > >> Paolo
> > > >
> > > > We do bug-compatible if it's not a big pain, too.
> > > 
> > > In this case, there is disagreement about what is better:
> > > - correct solution
> > > - bug compatible
> > > 
> > > We can't have both in this case :-(
> > > 
> > > Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
> > > pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
> > > something like:
> > > 
> > > Source: 2.2  Destination: 2.2 -> bug compatible 2.2
> > > Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
> > > Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always
> > 
> > To be fair the 2.3->2.2 is more subtle; opening it is unlikely
> > to generate the subsections; it needs a bit more than that (certainly on Linux)
> > figuring out exactly what triggers each subsection is trickier.
> > 
> > Dave
> 
> And more importantly, what is the result of skipping them,
> like you proposed. E.g. if guests crash that's no
> better than failing migration.

I believe it's the same behaviour as qemu serial migration has been
doing for many years when it never sent that data over, and I'm not
aware of that ever causing us problems.

Dave

> > > 
> > > 
> > > So the problem is 2.3 -> 2.2 when serial is being used (notice that just
> > > opening it it is using).  That is what we are differing about what is
> > > the right thing to do.  As Paolo says, in upstream, we have done in the
> > > past the correct thing, in downstream, it depends.
> > > 
> > > Notice that adding this patch makes that the three cases are bug
> > > compatible, i.e. there is no way to detect breakage neither a way to fix
> > > the issue (fix without the patch is just upgrade both binaries.
> > > )
> > > 
> > > Later, Juan.
> > --
> > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 12:13, Michael S. Tsirkin wrote:
> On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
>>
>>
>> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
>>>> No, please.  Upstream QEMU doesn't want to get into judgement about when
>>>> migration quality might be "good enough" that you can drop subsections.
>>>>  It's one thing to perfect the .needed functions to make the appearance
>>>> of subsections as unlikely as possible, but adding flags is not
>>>> something we've done so far---and not something at least *I* want to do.
>>>
>>> Not like this, sure.  But e.g. patches that force specific fields to
>>> behave in a way consistent with QEMU 2.2, with appropriate
>>> doducmentation would be ok I think.
>>
>> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
>> as 2.2", not "bug-compatible with 2.2".
>>
>> Refining the .needed functions (e.g. see commit bfa7362889) is just
>> that: describing when a subsection is needed.  Forcing specific fields
>> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> 
> We do bug-compatible if it's not a big pain, too.

Where, in the specific case of migration?

Like Juan, I see where you're coming from.  But it's a slippery slope,
and upstream chose not to go down it.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 11:59:05AM +0100, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Wed, Jun 17, 2015 at 11:51:57AM +0100, Dr. David Alan Gilbert wrote:
> > > * Juan Quintela (quintela@redhat.com) wrote:
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> > > > >> 
> > > > >> 
> > > > >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> > > > >> > > No, please.  Upstream QEMU doesn't want to get into judgement about when
> > > > >> > > migration quality might be "good enough" that you can drop subsections.
> > > > >> > >  It's one thing to perfect the .needed functions to make the appearance
> > > > >> > > of subsections as unlikely as possible, but adding flags is not
> > > > >> > > something we've done so far---and not something at least *I* want to do.
> > > > >> > 
> > > > >> > Not like this, sure.  But e.g. patches that force specific fields to
> > > > >> > behave in a way consistent with QEMU 2.2, with appropriate
> > > > >> > doducmentation would be ok I think.
> > > > >> 
> > > > >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> > > > >> as 2.2", not "bug-compatible with 2.2".
> > > > >> 
> > > > >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> > > > >> that: describing when a subsection is needed.  Forcing specific fields
> > > > >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> > > > >> 
> > > > >> Paolo
> > > > >
> > > > > We do bug-compatible if it's not a big pain, too.
> > > > 
> > > > In this case, there is disagreement about what is better:
> > > > - correct solution
> > > > - bug compatible
> > > > 
> > > > We can't have both in this case :-(
> > > > 
> > > > Notice that if "both" are 2.2 <improved>, i.e. 2.3 with -M
> > > > pc-i440fx-2.2, we also got the correct behaviour.  So the matrix is
> > > > something like:
> > > > 
> > > > Source: 2.2  Destination: 2.2 -> bug compatible 2.2
> > > > Source: 2.3  Destination: 2.2 -> breaks if serial is being used, works otherwise
> > > > Source: 2.3  Destination: 2.3 with -M pc-i440fx-2.2: works always
> > > 
> > > To be fair the 2.3->2.2 is more subtle; opening it is unlikely
> > > to generate the subsections; it needs a bit more than that (certainly on Linux)
> > > figuring out exactly what triggers each subsection is trickier.
> > > 
> > > Dave
> > 
> > And more importantly, what is the result of skipping them,
> > like you proposed. E.g. if guests crash that's no
> > better than failing migration.
> 
> I believe it's the same behaviour as qemu serial migration has been
> doing for many years when it never sent that data over, and I'm not
> aware of that ever causing us problems.
> 
> Dave

A bit more data on testing and/or code analysis would be helpful
I think.


> > > > 
> > > > 
> > > > So the problem is 2.3 -> 2.2 when serial is being used (notice that just
> > > > opening it it is using).  That is what we are differing about what is
> > > > the right thing to do.  As Paolo says, in upstream, we have done in the
> > > > past the correct thing, in downstream, it depends.
> > > > 
> > > > Notice that adding this patch makes that the three cases are bug
> > > > compatible, i.e. there is no way to detect breakage neither a way to fix
> > > > the issue (fix without the patch is just upgrade both binaries.
> > > > )
> > > > 
> > > > Later, Juan.
> > > --
> > > Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 17/06/2015 12:13, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> >>>> No, please.  Upstream QEMU doesn't want to get into judgement about when
> >>>> migration quality might be "good enough" that you can drop subsections.
> >>>>  It's one thing to perfect the .needed functions to make the appearance
> >>>> of subsections as unlikely as possible, but adding flags is not
> >>>> something we've done so far---and not something at least *I* want to do.
> >>>
> >>> Not like this, sure.  But e.g. patches that force specific fields to
> >>> behave in a way consistent with QEMU 2.2, with appropriate
> >>> doducmentation would be ok I think.
> >>
> >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> >> as 2.2", not "bug-compatible with 2.2".
> >>
> >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> >> that: describing when a subsection is needed.  Forcing specific fields
> >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> > 
> > We do bug-compatible if it's not a big pain, too.
> 
> Where, in the specific case of migration?
> 
> Like Juan, I see where you're coming from.  But it's a slippery slope,
> and upstream chose not to go down it.

Whatever choice upstream may have made, that was a long time ago
and doesn't mean it can't change.

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 13:40, Dr. David Alan Gilbert wrote:
> > Like Juan, I see where you're coming from.  But it's a slippery slope,
> > and upstream chose not to go down it.
> 
> Whatever choice upstream may have made, that was a long time ago
> and doesn't mean it can't change.

My question really is: what has changed for this choice to not make
sense anymore?

Backward migration is not supported upstream except between minor
releases.  It is really the same as in RHEL, except that a minor release
lasts a few years less. :)

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 01:33:26PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 12:13, Michael S. Tsirkin wrote:
> > On Wed, Jun 17, 2015 at 10:11:48AM +0200, Paolo Bonzini wrote:
> >>
> >>
> >> On 17/06/2015 09:52, Michael S. Tsirkin wrote:
> >>>> No, please.  Upstream QEMU doesn't want to get into judgement about when
> >>>> migration quality might be "good enough" that you can drop subsections.
> >>>>  It's one thing to perfect the .needed functions to make the appearance
> >>>> of subsections as unlikely as possible, but adding flags is not
> >>>> something we've done so far---and not something at least *I* want to do.
> >>>
> >>> Not like this, sure.  But e.g. patches that force specific fields to
> >>> behave in a way consistent with QEMU 2.2, with appropriate
> >>> doducmentation would be ok I think.
> >>
> >> That's not what 2.2 means in "pc-i440fx-2.2".  It means "same hardware
> >> as 2.2", not "bug-compatible with 2.2".
> >>
> >> Refining the .needed functions (e.g. see commit bfa7362889) is just
> >> that: describing when a subsection is needed.  Forcing specific fields
> >> to behave in a way consistent with QEMU 2.2 is bug compatibility.
> > 
> > We do bug-compatible if it's not a big pain, too.
> 
> Where, in the specific case of migration?

Just look at hour compat flags.

For example (intentionally using serial here):
        {\
            .driver   = "pci-serial",\
            .property = "prog_if",\
            .value    = stringify(0),\
        },\
        {\
            .driver   = "pci-serial-2x",\
            .property = "prog_if",\
            .value    = stringify(0),\
        },\
        {\
            .driver   = "pci-serial-4x",\
            .property = "prog_if",\
            .value    = stringify(0),\
        },\

apparently some clients check the specific prog if
value, completely breaking if the value was incorrect.
So we fixed it for new machine types but not old ones.

We have tons of other examples for other devices.

> Like Juan, I see where you're coming from.  But it's a slippery slope,
> and upstream chose not to go down it.
> 
> Paolo

We did very similar things in the past. A minor bug is not worth
failing migration for. Is this a minor bug? Still need to
figure this out, commit logs should help more in this respect.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 01:44:12PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 13:40, Dr. David Alan Gilbert wrote:
> > > Like Juan, I see where you're coming from.  But it's a slippery slope,
> > > and upstream chose not to go down it.
> > 
> > Whatever choice upstream may have made, that was a long time ago
> > and doesn't mean it can't change.
> 
> My question really is: what has changed for this choice to not make
> sense anymore?

We are getting more testing for migration is what changed.  And ping
pong migration - which requires backward migration - is very useful for
such testing.

> Backward migration is not supported upstream except between minor
> releases.  It is really the same as in RHEL, except that a minor release
> lasts a few years less. :)
> 
> Paolo

Not supported does not mean we can't add code to make it work if it's
helpful for developers.  All this assuming this is not too much code,
and not too ugly.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 13:45, Michael S. Tsirkin wrote:
>> > Where, in the specific case of migration?
> Just look at hour compat flags.
> 
> For example (intentionally using serial here):
>         {\
>             .driver   = "pci-serial",\
>             .property = "prog_if",\
>             .value    = stringify(0),\
>         },\
>         {\
>             .driver   = "pci-serial-2x",\
>             .property = "prog_if",\
>             .value    = stringify(0),\
>         },\
>         {\
>             .driver   = "pci-serial-4x",\
>             .property = "prog_if",\
>             .value    = stringify(0),\
>         },\
> 
> apparently some clients check the specific prog if
> value, completely breaking if the value was incorrect.
> So we fixed it for new machine types but not old ones.

That has nothing to do with migration.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 01:53:11PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 13:45, Michael S. Tsirkin wrote:
> >> > Where, in the specific case of migration?
> > Just look at hour compat flags.
> > 
> > For example (intentionally using serial here):
> >         {\
> >             .driver   = "pci-serial",\
> >             .property = "prog_if",\
> >             .value    = stringify(0),\
> >         },\
> >         {\
> >             .driver   = "pci-serial-2x",\
> >             .property = "prog_if",\
> >             .value    = stringify(0),\
> >         },\
> >         {\
> >             .driver   = "pci-serial-4x",\
> >             .property = "prog_if",\
> >             .value    = stringify(0),\
> >         },\
> > 
> > apparently some clients check the specific prog if
> > value, completely breaking if the value was incorrect.
> > So we fixed it for new machine types but not old ones.
> 
> That has nothing to do with migration.
> 
> Paolo

If you change prog_if migration fails.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 13:54, Michael S. Tsirkin wrote:
>>> > >> > Where, in the specific case of migration?
> > > Just look at hour compat flags.
> > > 
> > > For example (intentionally using serial here):
> > >         {\
> > >             .driver   = "pci-serial",\
> > >             .property = "prog_if",\
> > >             .value    = stringify(0),\
> > >         },\
> > >         {\
> > >             .driver   = "pci-serial-2x",\
> > >             .property = "prog_if",\
> > >             .value    = stringify(0),\
> > >         },\
> > >         {\
> > >             .driver   = "pci-serial-4x",\
> > >             .property = "prog_if",\
> > >             .value    = stringify(0),\
> > >         },\
> > > 
> > > apparently some clients check the specific prog if
> > > value, completely breaking if the value was incorrect.
> > > So we fixed it for new machine types but not old ones.
> > 
> > That has nothing to do with migration.
> 
> If you change prog_if migration fails.

No, it doesn't.  The guest misbehaves maybe, but the migration format is
not affected.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 01:55:37PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 13:54, Michael S. Tsirkin wrote:
> >>> > >> > Where, in the specific case of migration?
> > > > Just look at hour compat flags.
> > > > 
> > > > For example (intentionally using serial here):
> > > >         {\
> > > >             .driver   = "pci-serial",\
> > > >             .property = "prog_if",\
> > > >             .value    = stringify(0),\
> > > >         },\
> > > >         {\
> > > >             .driver   = "pci-serial-2x",\
> > > >             .property = "prog_if",\
> > > >             .value    = stringify(0),\
> > > >         },\
> > > >         {\
> > > >             .driver   = "pci-serial-4x",\
> > > >             .property = "prog_if",\
> > > >             .value    = stringify(0),\
> > > >         },\
> > > > 
> > > > apparently some clients check the specific prog if
> > > > value, completely breaking if the value was incorrect.
> > > > So we fixed it for new machine types but not old ones.
> > > 
> > > That has nothing to do with migration.
> > 
> > If you change prog_if migration fails.
> 
> No, it doesn't.  The guest misbehaves maybe, but the migration format is
> not affected.
> 
> Paolo

I just tried, set prog_if to different values, sure it failed.

Here's another one, at random:

Author: Michael S. Tsirkin <mst@redhat.com>
Date:   Thu Feb 14 19:11:27 2013 +0200

    e1000: unbreak the guest network migration to 1.3
    
    QEMU 1.3 does not emulate the link auto negotiation, so if migrate to a
    1.3 machine during link auto negotiation, the guest link will be set to down.
    Fix this by just disabling auto negotiation for 1.3 and older.
    
    Signed-off-by: Michael S. Tsirkin <mst@redhat.com>


-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Dr. David Alan Gilbert]

* Paolo Bonzini (pbonzini@redhat.com) wrote:
> 
> 
> On 17/06/2015 13:40, Dr. David Alan Gilbert wrote:
> > > Like Juan, I see where you're coming from.  But it's a slippery slope,
> > > and upstream chose not to go down it.
> > 
> > Whatever choice upstream may have made, that was a long time ago
> > and doesn't mean it can't change.
> 
> My question really is: what has changed for this choice to not make
> sense anymore?

At one time migration was used as an occasional facility to deal with 
a machine that was dieing; now you've got systems doing thousands of migrations
a day between hosts for load balancing and power saving.
At one time migrations were happening over GigE on small VMs, now we
have people using 10-40GbE networks with 100GB of RAM in the VMs that
take a significant time to migrate, even when mostly idle, spending
ages migrating for it to fail with an answer of 'oh, it's data dependent,
try again' is really bad.

> Backward migration is not supported upstream except between minor
> releases.  It is really the same as in RHEL, except that a minor release
> lasts a few years less. :)

Of course for us on RHEL our minor releases don't correspond to
QEMU minor releases, so we already support migrating from our
downstream 7.1 (QEMU 2.1) derivative to our 7.0 (1.5.3) version.  
And the reason for this patch series is to support something >2.2
migrating back to that 2.1 (or maybe even to that 1.5.3).

I don't believe we're alone in wanting to be able to do that type
of thing; so you can either worry about not burdening upstream
with compatibility patches like this, or think it's not fair
to leave them out if others upstream might want them.  How many
others? Well I'd say it's got to be more than some of the other
obscure features in QEMU!

Dave

> 
> Paolo
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 13:58, Michael S. Tsirkin wrote:
> > No, it doesn't.  The guest misbehaves maybe, but the migration format is
> > not affected.
> 
> I just tried, set prog_if to different values, sure it failed.

How so?  It's just a byte in config space.  But even then, fixing
migration is just a side effect of keeping config space consistent for a
given machine type (i.e. not changing hardware type under the guest's feet).

> Here's another one, at random:
> 
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date:   Thu Feb 14 19:11:27 2013 +0200
> 
>     e1000: unbreak the guest network migration to 1.3
>     
>     QEMU 1.3 does not emulate the link auto negotiation, so if migrate to a
>     1.3 machine during link auto negotiation, the guest link will be set to down.
>     Fix this by just disabling auto negotiation for 1.3 and older.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Okay, that's an interesting one, and there's a similar one for e1000
interrupt mitigation.

The interesting point is that in both cases the bug compatibility
extends to other behavior of the device, i.e. more than just migration.

I would even say that bug-compatibility of migration is just a side
effect, not the primary end.  For interrupt mitigation, it was not
enabled on older machine types in the first place, because it could
break guests.  Keeping backwards migration working was just a side
effect; simply, checking "s->compat_flags & E1000_FLAG_MIT" is the only
sensible way to write e1000_mit_state_needed.  Auto negotiation should
have been done the same way, which is what your patch did.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 14:07, Dr. David Alan Gilbert wrote:
> Of course for us on RHEL our minor releases don't correspond to
> QEMU minor releases, so we already support migrating from our
> downstream 7.1 (QEMU 2.1) derivative to our 7.0 (1.5.3) version.  
> And the reason for this patch series is to support something >2.2
> migrating back to that 2.1 (or maybe even to that 1.5.3).
> 
> I don't believe we're alone in wanting to be able to do that type
> of thing;

Others may prefer to have migration only work when it is absolutely sure
that it works.  It is much easier to add hacks on top of what upstream
QEMU does (e.g. using the static checker), than to remove the hacks.

If we really didn't care about others' support for bidirectional
migration, we would have kept the static checker internal to Red Hat.
Or we wouldn't have bothered to refine the .needed functions, and so on.

Paolo

> so you can either worry about not burdening upstream
> with compatibility patches like this, or think it's not fair
> to leave them out if others upstream might want them.  How many
> others? Well I'd say it's got to be more than some of the other
> obscure features in QEMU!

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 02:20:42PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 14:07, Dr. David Alan Gilbert wrote:
> > Of course for us on RHEL our minor releases don't correspond to
> > QEMU minor releases, so we already support migrating from our
> > downstream 7.1 (QEMU 2.1) derivative to our 7.0 (1.5.3) version.  
> > And the reason for this patch series is to support something >2.2
> > migrating back to that 2.1 (or maybe even to that 1.5.3).
> > 
> > I don't believe we're alone in wanting to be able to do that type
> > of thing;
> 
> Others may prefer to have migration only work when it is absolutely sure
> that it works.  It is much easier to add hacks on top of what upstream
> QEMU does (e.g. using the static checker), than to remove the hacks.
> 
> If we really didn't care about others' support for bidirectional
> migration, we would have kept the static checker internal to Red Hat.
> Or we wouldn't have bothered to refine the .needed functions, and so on.
> 
> Paolo

What we need to decide is how major is the breakage.
If it's minor - like some lost characters - then it's not
worth breaking migration for most users.
And I think this should be a property so people can
force strict mode if they really want to.

If it's a major breakage, it's harder to decide:
some people might be able to retry migration later.
Maybe a flag to enable this mode would make sense?
Also, maybe it would be better to fail migration on source
rather than send something destination can't handle?

But let's see what the symptoms are before we argue
about this option.

> > so you can either worry about not burdening upstream
> > with compatibility patches like this, or think it's not fair
> > to leave them out if others upstream might want them.  How many
> > others? Well I'd say it's got to be more than some of the other
> > obscure features in QEMU!

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 02:20:30PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 13:58, Michael S. Tsirkin wrote:
> > > No, it doesn't.  The guest misbehaves maybe, but the migration format is
> > > not affected.
> > 
> > I just tried, set prog_if to different values, sure it failed.
> 
> How so?  It's just a byte in config space.  But even then, fixing
> migration is just a side effect of keeping config space consistent for a
> given machine type (i.e. not changing hardware type under the guest's feet).

David's patches are also guest visible, are they not?
We are losing state guest can indirectly observe, right?

> > Here's another one, at random:
> > 
> > Author: Michael S. Tsirkin <mst@redhat.com>
> > Date:   Thu Feb 14 19:11:27 2013 +0200
> > 
> >     e1000: unbreak the guest network migration to 1.3
> >     
> >     QEMU 1.3 does not emulate the link auto negotiation, so if migrate to a
> >     1.3 machine during link auto negotiation, the guest link will be set to down.
> >     Fix this by just disabling auto negotiation for 1.3 and older.
> >     
> >     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> Okay, that's an interesting one, and there's a similar one for e1000
> interrupt mitigation.
> 
> The interesting point is that in both cases the bug compatibility
> extends to other behavior of the device, i.e. more than just migration.

But the guest registers are exactly the same. It is only guest-visible
indirectly, as timing of link up events.
So why not keep auto-negotiation running correctly?  Because
we can't keep it running across migration.

> I would even say that bug-compatibility of migration is just a side
> effect, not the primary end.  For interrupt mitigation, it was not
> enabled on older machine types in the first place, because it could
> break guests.  Keeping backwards migration working was just a side
> effect; simply, checking "s->compat_flags & E1000_FLAG_MIT" is the only
> sensible way to write e1000_mit_state_needed.  Auto negotiation should
> have been done the same way, which is what your patch did.
> 
> Paolo

True, David's patches only trigger if migration happens.
I am guessing there is no need to touch other paths,
but I am not very familiar with the hardware in question.

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 16:50, Michael S. Tsirkin wrote:
> > > I just tried, set prog_if to different values, sure it failed.
> > 
> > How so?  It's just a byte in config space.  But even then, fixing
> > migration is just a side effect of keeping config space consistent for a
> > given machine type (i.e. not changing hardware type under the guest's feet).
> 
> David's patches are also guest visible, are they not?
> We are losing state guest can indirectly observe, right?

One case only happens during migration, the other does not.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 06:16:19PM +0200, Paolo Bonzini wrote:
> 
> 
> On 17/06/2015 16:50, Michael S. Tsirkin wrote:
> > > > I just tried, set prog_if to different values, sure it failed.
> > > 
> > > How so?  It's just a byte in config space.

Why does it fail? Because pci core migrates it and validates it
on both sides.

> > > But even then, fixing
> > > migration is just a side effect of keeping config space consistent for a
> > > given machine type (i.e. not changing hardware type under the guest's feet).
> > 
> > David's patches are also guest visible, are they not?
> > We are losing state guest can indirectly observe, right?
> 
> One case only happens during migration, the other does not.
> 
> Paolo

True - apparently the bug was only in the migration stream,
not the functionality?

-- 
MST

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Juan Quintela]

"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jun 17, 2015 at 02:20:42PM +0200, Paolo Bonzini wrote:
>> 
>> 
>> On 17/06/2015 14:07, Dr. David Alan Gilbert wrote:
>> > Of course for us on RHEL our minor releases don't correspond to
>> > QEMU minor releases, so we already support migrating from our
>> > downstream 7.1 (QEMU 2.1) derivative to our 7.0 (1.5.3) version.  
>> > And the reason for this patch series is to support something >2.2
>> > migrating back to that 2.1 (or maybe even to that 1.5.3).
>> > 
>> > I don't believe we're alone in wanting to be able to do that type
>> > of thing;
>> 
>> Others may prefer to have migration only work when it is absolutely sure
>> that it works.  It is much easier to add hacks on top of what upstream
>> QEMU does (e.g. using the static checker), than to remove the hacks.
>> 
>> If we really didn't care about others' support for bidirectional
>> migration, we would have kept the static checker internal to Red Hat.
>> Or we wouldn't have bothered to refine the .needed functions, and so on.
>> 
>> Paolo
>
> What we need to decide is how major is the breakage.
> If it's minor - like some lost characters - then it's not
> worth breaking migration for most users.
> And I think this should be a property so people can
> force strict mode if they really want to.
>
> If it's a major breakage, it's harder to decide:
> some people might be able to retry migration later.
> Maybe a flag to enable this mode would make sense?
> Also, maybe it would be better to fail migration on source
> rather than send something destination can't handle?

Source don't know if destination understand it or not.  That is the
whole point of being optional.  Source sends it if it is needed.
Destination can handle it (or not).

there are (at least) two qemu pc-2.2:
qemu-2.2 -M pc-2.2
qemu-2.3 -M pc-2.2

Same machine type.  Second is able to receive it.  First one is not.
Source don't know what is on the other side.  If user is going to put a:

--dont_send_serial_because_I_don't_care

Then it can as well just disable the serial device and live with it.

Later, Juan.


>
> But let's see what the symptoms are before we argue
> about this option.
>
>> > so you can either worry about not burdening upstream
>> > with compatibility patches like this, or think it's not fair
>> > to leave them out if others upstream might want them.  How many
>> > others? Well I'd say it's got to be more than some of the other
>> > obscure features in QEMU!

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 18:34, Michael S. Tsirkin wrote:
> > > > I just tried, set prog_if to different values, sure it failed.
> > > 
> > > How so?  It's just a byte in config space.
>
> Why does it fail? Because pci core migrates it and validates it
> on both sides.

I cannot find it, can you show me the validation code?

> > > David's patches are also guest visible, are they not?
> > > We are losing state guest can indirectly observe, right?
> > 
> > One case only happens during migration, the other does not.
> > 
> True - apparently the bug was only in the migration stream,
> not the functionality?

Yes.

Paolo

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Michael S. Tsirkin]

On Wed, Jun 17, 2015 at 06:34:55PM +0200, Juan Quintela wrote:
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Wed, Jun 17, 2015 at 02:20:42PM +0200, Paolo Bonzini wrote:
> >> 
> >> 
> >> On 17/06/2015 14:07, Dr. David Alan Gilbert wrote:
> >> > Of course for us on RHEL our minor releases don't correspond to
> >> > QEMU minor releases, so we already support migrating from our
> >> > downstream 7.1 (QEMU 2.1) derivative to our 7.0 (1.5.3) version.  
> >> > And the reason for this patch series is to support something >2.2
> >> > migrating back to that 2.1 (or maybe even to that 1.5.3).
> >> > 
> >> > I don't believe we're alone in wanting to be able to do that type
> >> > of thing;
> >> 
> >> Others may prefer to have migration only work when it is absolutely sure
> >> that it works.  It is much easier to add hacks on top of what upstream
> >> QEMU does (e.g. using the static checker), than to remove the hacks.
> >> 
> >> If we really didn't care about others' support for bidirectional
> >> migration, we would have kept the static checker internal to Red Hat.
> >> Or we wouldn't have bothered to refine the .needed functions, and so on.
> >> 
> >> Paolo
> >
> > What we need to decide is how major is the breakage.
> > If it's minor - like some lost characters - then it's not
> > worth breaking migration for most users.
> > And I think this should be a property so people can
> > force strict mode if they really want to.
> >
> > If it's a major breakage, it's harder to decide:
> > some people might be able to retry migration later.
> > Maybe a flag to enable this mode would make sense?
> > Also, maybe it would be better to fail migration on source
> > rather than send something destination can't handle?
> 
> Source don't know if destination understand it or not.  That is the
> whole point of being optional.  Source sends it if it is needed.
> Destination can handle it (or not).
> 
> there are (at least) two qemu pc-2.2:
> qemu-2.2 -M pc-2.2
> qemu-2.3 -M pc-2.2
> 
> Same machine type.  Second is able to receive it.  First one is not.
> Source don't know what is on the other side.  If user is going to put a:
> 
> --dont_send_serial_because_I_don't_care
> 
> Then it can as well just disable the serial device and live with it.
> 
> Later, Juan.

Just losing worst-case a couple of characters is not the same as
losing serial functionality.

We could have a flag to tell us what's on the other side,
but that would need even more testing. So let's keep it
simple.


> 
> >
> > But let's see what the symptoms are before we argue
> > about this option.
> >
> >> > so you can either worry about not burdening upstream
> >> > with compatibility patches like this, or think it's not fair
> >> > to leave them out if others upstream might want them.  How many
> >> > others? Well I'd say it's got to be more than some of the other
> >> > obscure features in QEMU!

Re: [Qemu-devel] [PATCH] Migration compatibility for serial

[Paolo Bonzini]

On 17/06/2015 18:39, Michael S. Tsirkin wrote:
> > Same machine type.  Second is able to receive it.  First one is not.
> > Source don't know what is on the other side.  If user is going to put a:
> > 
> > --dont_send_serial_because_I_don't_care
> > 
> > Then it can as well just disable the serial device and live with it.
> 
> Just losing worst-case a couple of characters is not the same as
> losing serial functionality.

This is _not_ losing serial functionality.  Usually the FIFOs are not
migrated.  This is losing serial functionality _that wasn't being used
anyway_.

> We could have a flag to tell us what's on the other side,
> but that would need even more testing. So let's keep it
> simple.

Yes, let's keep it simple.  Let's keep it the same as 2.3 has been released.

Paolo