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

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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