diff mbox

Live migrate, inconsistent machine types - new machine type to fix?

Message ID 5AB9C3BF-1A4F-4942-A433-1724BBFC9D23@alex.org.uk
State New
Headers show

Commit Message

Alex Bligh July 22, 2014, 11:38 a.m. UTC
On 22 Jul 2014, at 11:54, Amit Shah <amit.shah@redhat.com> wrote:
>> 
>> This one, together with the PIIX4 one (which for some reason doesn't
>> show up) where the two I hit, after manually fixing the rom sizes
>> stuff on the command line.
>> 
>> Apparently "flags" and "channels" are pseudonyms.
> 
> No, they're not; flags is an extra 4-byte param in qemu-kvm-1.0, which
> doesn't exist in qemu-1.0.
> 
> In qemu-2.1 -M 1.0, "flags" got renamed to "channels[0].irq_disabled",
> which is also flagged.

I was basing that on the comment in Cole Robinson's patch here:
 http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20


I think what this means is that flags is equivalent to channels[0].irq_disabled,
yes?

>> The piix4_m thing
>> is that qemu-kvm-1.0 is actually using version 3 but says it's
>> using version 2, which is perhaps why it doesn't show up in your
>> test.
> 
> You're looking at the wrong output.  You should see one message back,
> where I compare qemu-1.0 with qemu-2.1 -M pc-1.0.  That has the
> piix4_pm version mismatch flagged.

Don't think I am

a) because I'm using qemu-kvm-1.0, not qemu-1.0. You looked at qemu-1.0
   in the previous message; that's not directly relevant for what I'm
   looking at as my source is qemu-kvm-1.0 (qemu vs qemu-kvm).

b) because I wasn't actually using your Json, but testing what made the
   thing load in real life!

Comments

Amit Shah July 22, 2014, 11:54 a.m. UTC | #1
On (Tue) 22 Jul 2014 [12:38:14], Alex Bligh wrote:
> 
> On 22 Jul 2014, at 11:54, Amit Shah <amit.shah@redhat.com> wrote:
> >> 
> >> This one, together with the PIIX4 one (which for some reason doesn't
> >> show up) where the two I hit, after manually fixing the rom sizes
> >> stuff on the command line.
> >> 
> >> Apparently "flags" and "channels" are pseudonyms.
> > 
> > No, they're not; flags is an extra 4-byte param in qemu-kvm-1.0, which
> > doesn't exist in qemu-1.0.
> > 
> > In qemu-2.1 -M 1.0, "flags" got renamed to "channels[0].irq_disabled",
> > which is also flagged.
> 
> I was basing that on the comment in Cole Robinson's patch here:
>  http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0001-Fix-migration-from-qemu-kvm.patch?h=f20
> 
> --- a/hw/timer/i8254_common.c
> +++ b/hw/timer/i8254_common.c
> @@ -267,7 +267,12 @@ static const VMStateDescription vmstate_pit_common = {
>      .pre_save = pit_dispatch_pre_save,
>      .post_load = pit_dispatch_post_load,
>      .fields = (VMStateField[]) {
> -        VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
> +        /* qemu-kvm version_id=2 had 'flags' here which is equivalent
> +         * This fixes incoming migration from qemu-kvm 1.0, but breaks
> +         * incoming migration from qemu < 1.1
> +         */
> +        //VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
> +        VMSTATE_UINT32(channels[0].irq_disabled, PITCommonState),
>          VMSTATE_STRUCT_ARRAY(channels, PITCommonState, 3, 2,
>                               vmstate_pit_channel, PITChannelState),
>          VMSTATE_INT64(channels[0].next_transition_time,
> 
> I think what this means is that flags is equivalent to channels[0].irq_disabled,
> yes?

Yes, that's what I said above.

> >> The piix4_m thing
> >> is that qemu-kvm-1.0 is actually using version 3 but says it's
> >> using version 2, which is perhaps why it doesn't show up in your
> >> test.
> > 
> > You're looking at the wrong output.  You should see one message back,
> > where I compare qemu-1.0 with qemu-2.1 -M pc-1.0.  That has the
> > piix4_pm version mismatch flagged.
> 
> Don't think I am
> 
> a) because I'm using qemu-kvm-1.0, not qemu-1.0. You looked at qemu-1.0
>    in the previous message; that's not directly relevant for what I'm
>    looking at as my source is qemu-kvm-1.0 (qemu vs qemu-kvm).

Sigh; please read both my replies.

The only difference between qemu-kvm-1.0 and qemu-1.0 is the presence
of the pci-assign section in qemu-kvm-1.0.  All other output from
qemu-1.0 -> qemu-2.1 is equally applicable to qemu-kvm-1.0 ->
qemu-2.1.

> b) because I wasn't actually using your Json, but testing what made the
>    thing load in real life!

I know that; but you brought up the comparison of your experiments
with the output of the checker.  I just pointed out the checker
flagged both those things you saw (and a few others too).


		Amit
Paolo Bonzini July 22, 2014, 12:12 p.m. UTC | #2
Il 22/07/2014 13:54, Amit Shah ha scritto:
>> > a) because I'm using qemu-kvm-1.0, not qemu-1.0. You looked at qemu-1.0
>> >    in the previous message; that's not directly relevant for what I'm
>> >    looking at as my source is qemu-kvm-1.0 (qemu vs qemu-kvm).
> Sigh; please read both my replies.
> 
> The only difference between qemu-kvm-1.0 and qemu-1.0 is the presence
> of the pci-assign section in qemu-kvm-1.0.  All other output from
> qemu-1.0 -> qemu-2.1 is equally applicable to qemu-kvm-1.0 ->
> qemu-2.1.

Not really true.

qemu 1.0 didn't have neither channels[0].irq_disabled nor flags and is
version 2.

qemu-2.1 has channels[0].irq_disabled and is version 3.

qemu-kvm-1.0 had flags, which is now called channels[0].irq_disabled,
and is version 2.

qemu-2.1 loads version 2 as something that doesn't have
channels[0].irq_disabled.  So it breaks if you feed it with qemu-kvm-1.0
data.

There's something similar going on with PIIX4_PM but I don't remember
the details.

Paolo
Alex Bligh July 22, 2014, 12:15 p.m. UTC | #3
On 22 Jul 2014, at 12:54, Amit Shah <amit.shah@redhat.com> wrote:

> Sigh; please read both my replies.
> 
> The only difference between qemu-kvm-1.0 and qemu-1.0 is the presence
> of the pci-assign section in qemu-kvm-1.0.  All other output from
> qemu-1.0 -> qemu-2.1 is equally applicable to qemu-kvm-1.0 ->
> qemu-2.1.
...
> I know that; but you brought up the comparison of your experiments
> with the output of the checker.  I just pointed out the checker
> flagged both those things you saw (and a few others too).

Apologies for my naivety here as it's the first time I've looked
at this stuff. So sorry if I'm getting the wrong end of the stick.

However, am I correct in assuming your JSON checker is only
checking for the field structure, and not for things like
changed defaults? For instance the cirrus vga video ram size
appears to be different between qemu-kvm-1.0 and qemu-1.0,
but this isn't something brought up in your email highlighting
the difference between a 2.1->qemu-1.0 comparison and a
2.1->qemu-kvm-1.0 comparison. I know you haven't directly
compared qemu-1.0 and qemu-1.0-kvm, but if this difference
does exist (which I think it does) I'd expect to see it
in the list of differences (i.e. your second email), if your
tool looks at that too.
Alex Bligh July 22, 2014, 12:19 p.m. UTC | #4
On 22 Jul 2014, at 13:12, Paolo Bonzini <pbonzini@redhat.com> wrote:

> There's something similar going on with PIIX4_PM but I don't remember
> the details.

From memory:

* qemu-1.0 uses the v2 format
* qemu-kvm-1.0 uses the v3 format but advertises itself as v2
* qemu-2.1 uses the v3 format

I don't quite understand whether Amit's checker is designed to pick
up all three situations as different, or whether it (quite reasonably)
expects people to play ball with version numbers.
Amit Shah July 22, 2014, 12:40 p.m. UTC | #5
On (Tue) 22 Jul 2014 [14:12:00], Paolo Bonzini wrote:
> Il 22/07/2014 13:54, Amit Shah ha scritto:
> >> > a) because I'm using qemu-kvm-1.0, not qemu-1.0. You looked at qemu-1.0
> >> >    in the previous message; that's not directly relevant for what I'm
> >> >    looking at as my source is qemu-kvm-1.0 (qemu vs qemu-kvm).
> > Sigh; please read both my replies.
> > 
> > The only difference between qemu-kvm-1.0 and qemu-1.0 is the presence
> > of the pci-assign section in qemu-kvm-1.0.  All other output from
> > qemu-1.0 -> qemu-2.1 is equally applicable to qemu-kvm-1.0 ->
> > qemu-2.1.
> 
> Not really true.

Here's the complete output:

Section "xio3130-downstream" Description "xio3130-express-downstream-port": version error: 2 > 0
Section "usb-host" Section "usb-host" Description "usb-host": minimum version error: 0 < 1
Section "ich9-usb-ehci1" Section "ich9-usb-ehci1" Description "ehci": minimum version error: 0 < 1
Section "x3130-upstream" Description "xio3130-express-upstream-port": version error: 2 > 0
Section "usb-ehci" Section "usb-ehci" Description "ehci": minimum version error: 0 < 1
Section "ioh3420" Description "ioh-3240-express-root-port": version error: 2 > 0
Section "pci-assign" does not exist in dest
Section "PIIX4_PM" Section "PIIX4_PM" Description "piix4_pm": minimum version error: 2 < 3

> qemu 1.0 didn't have neither channels[0].irq_disabled nor flags and is
> version 2.
>
> qemu-2.1 has channels[0].irq_disabled and is version 3.
> 
> qemu-kvm-1.0 had flags, which is now called channels[0].irq_disabled,
> and is version 2.

Right - 'flags' here is renamed, and doesn't appear in that output;
but the src version was 0 according to the json output.

> qemu-2.1 loads version 2 as something that doesn't have
> channels[0].irq_disabled.  So it breaks if you feed it with qemu-kvm-1.0
> data.

OK - that's not something the static checker can flag, though..

> There's something similar going on with PIIX4_PM but I don't remember
> the details.

According to the output above, minimum version that qemu-2.1 accepts
changed to 3, and qemu-kvm-1.0 sends version 2.


		Amit
Amit Shah July 22, 2014, 12:44 p.m. UTC | #6
On (Tue) 22 Jul 2014 [13:15:41], Alex Bligh wrote:
> 
> On 22 Jul 2014, at 12:54, Amit Shah <amit.shah@redhat.com> wrote:
> 
> > Sigh; please read both my replies.
> > 
> > The only difference between qemu-kvm-1.0 and qemu-1.0 is the presence
> > of the pci-assign section in qemu-kvm-1.0.  All other output from
> > qemu-1.0 -> qemu-2.1 is equally applicable to qemu-kvm-1.0 ->
> > qemu-2.1.
> ...
> > I know that; but you brought up the comparison of your experiments
> > with the output of the checker.  I just pointed out the checker
> > flagged both those things you saw (and a few others too).
> 
> Apologies for my naivety here as it's the first time I've looked
> at this stuff. So sorry if I'm getting the wrong end of the stick.
> 
> However, am I correct in assuming your JSON checker is only
> checking for the field structure, and not for things like
> changed defaults?

It only looks at the vmstate structs.

> For instance the cirrus vga video ram size
> appears to be different between qemu-kvm-1.0 and qemu-1.0,

Right, this won't get flagged.

> but this isn't something brought up in your email highlighting
> the difference between a 2.1->qemu-1.0 comparison and a
> 2.1->qemu-kvm-1.0 comparison. I know you haven't directly
> compared qemu-1.0 and qemu-1.0-kvm, but if this difference
> does exist (which I think it does) I'd expect to see it
> in the list of differences (i.e. your second email), if your
> tool looks at that too.

I did compare qemu-1.0 and qemu-kvm-1.0, but this difference won't pop
up.

The other differences that the checker spotted may not be relevant to
you, as your VMs don't have those devices enabled.

		Amit
Amit Shah July 22, 2014, 12:47 p.m. UTC | #7
On (Tue) 22 Jul 2014 [13:19:43], Alex Bligh wrote:
> 
> On 22 Jul 2014, at 13:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> 
> > There's something similar going on with PIIX4_PM but I don't remember
> > the details.
> 
> From memory:
> 
> * qemu-1.0 uses the v2 format
> * qemu-kvm-1.0 uses the v3 format but advertises itself as v2
> * qemu-2.1 uses the v3 format

Yea; I vaguely recall that happening.  Quite unfortunate.

> I don't quite understand whether Amit's checker is designed to pick
> up all three situations as different, or whether it (quite reasonably)
> expects people to play ball with version numbers.

This is what it says:

Section "PIIX4_PM" Section "PIIX4_PM" Description "piix4_pm": minimum version error: 2 < 3


Which means qemu-2.1 expects a minimum version of 3, but qemu-kvm-1.0
is sending it version 2.  Now even though they may be compatible,
version-number-wise they're not, as the dest qemu will also complain
about.

		Amit
diff mbox

Patch

--- a/hw/timer/i8254_common.c
+++ b/hw/timer/i8254_common.c
@@ -267,7 +267,12 @@  static const VMStateDescription vmstate_pit_common = {
     .pre_save = pit_dispatch_pre_save,
     .post_load = pit_dispatch_post_load,
     .fields = (VMStateField[]) {
-        VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
+        /* qemu-kvm version_id=2 had 'flags' here which is equivalent
+         * This fixes incoming migration from qemu-kvm 1.0, but breaks
+         * incoming migration from qemu < 1.1
+         */
+        //VMSTATE_UINT32_V(channels[0].irq_disabled, PITCommonState, 3),
+        VMSTATE_UINT32(channels[0].irq_disabled, PITCommonState),
         VMSTATE_STRUCT_ARRAY(channels, PITCommonState, 3, 2,
                              vmstate_pit_channel, PITChannelState),
         VMSTATE_INT64(channels[0].next_transition_time,