Message ID | 1434480849-23093-1-git-send-email-dgilbert@redhat.com |
---|---|
State | New |
Headers | show |
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
"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.
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.
"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.
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.
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 >
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 > >
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
* 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
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
* 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
"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.
* 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
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.
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?
"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.
* 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
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.
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
* 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
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
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
* 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
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
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.
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.
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
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.
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
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>
* 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
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
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!
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!
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.
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
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?
"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!
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
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!
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
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