[Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Marcelo Tosatti]

To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
of RAM.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 19e342a..ead4b6b 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -347,6 +347,26 @@ static QEMUMachine pc_machine_v1_2 = {
     },
 };
 
+#define QEMUKVMPC_COMPAT_1_2 \
+        {\
+            .driver   = "cirrus-vga",\
+            .property = "vgamem_mb",\
+            .value    = "16",\
+        }
+
+static QEMUMachine qemukvmpc_machine_v1_2 = {
+    .name = "qemukvm-pc-1.2",
+    .alias = "pc",
+    .desc = "Standard PC",
+    .init = pc_init_pci,
+    .max_cpus = 255,
+    .compat_props = (GlobalProperty[]) {
+        QEMUKVMPC_COMPAT_1_2,
+        PC_COMPAT_1_2,
+        { /* end of list */ }
+    },
+};
+
 #define PC_COMPAT_1_1 \
         PC_COMPAT_1_2,\
         {\
@@ -645,6 +665,7 @@ static QEMUMachine xenfv_machine = {
 
 static void pc_machine_init(void)
 {
+    qemu_register_machine(&qemukvmpc_machine_v1_2);
     qemu_register_machine(&pc_machine_v1_4);
     qemu_register_machine(&pc_machine_v1_3);
     qemu_register_machine(&pc_machine_v1_2);

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

Il 12/12/2012 23:39, Marcelo Tosatti ha scritto:
> 
> To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> of RAM.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

A similar patch would be needed for all machine types though, no?

I think distros that used to ship qemu-kvm should just change the
default just like for the acpi_piix4.c change.

Paolo

> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 19e342a..ead4b6b 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -347,6 +347,26 @@ static QEMUMachine pc_machine_v1_2 = {
>      },
>  };
>  
> +#define QEMUKVMPC_COMPAT_1_2 \
> +        {\
> +            .driver   = "cirrus-vga",\
> +            .property = "vgamem_mb",\
> +            .value    = "16",\
> +        }
> +
> +static QEMUMachine qemukvmpc_machine_v1_2 = {
> +    .name = "qemukvm-pc-1.2",
> +    .alias = "pc",
> +    .desc = "Standard PC",
> +    .init = pc_init_pci,
> +    .max_cpus = 255,
> +    .compat_props = (GlobalProperty[]) {
> +        QEMUKVMPC_COMPAT_1_2,
> +        PC_COMPAT_1_2,
> +        { /* end of list */ }
> +    },
> +};
> +
>  #define PC_COMPAT_1_1 \
>          PC_COMPAT_1_2,\
>          {\
> @@ -645,6 +665,7 @@ static QEMUMachine xenfv_machine = {
>  
>  static void pc_machine_init(void)
>  {
> +    qemu_register_machine(&qemukvmpc_machine_v1_2);
>      qemu_register_machine(&pc_machine_v1_4);
>      qemu_register_machine(&pc_machine_v1_3);
>      qemu_register_machine(&pc_machine_v1_2);
> 

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Marcelo Tosatti]

On Thu, Dec 13, 2012 at 09:35:13AM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 23:39, Marcelo Tosatti ha scritto:
> > 
> > To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> > of RAM.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> A similar patch would be needed for all machine types though, no?

Why ? That would break incoming migration from QEMU.

> I think distros that used to ship qemu-kvm should just change the
> default just like for the acpi_piix4.c change.
> 
> Paolo

Works for me.

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Eduardo Habkost]

On Thu, Dec 13, 2012 at 09:35:13AM +0100, Paolo Bonzini wrote:
> Il 12/12/2012 23:39, Marcelo Tosatti ha scritto:
> > 
> > To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> > of RAM.
> > 
> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> A similar patch would be needed for all machine types though, no?
> 
> I think distros that used to ship qemu-kvm should just change the
> default just like for the acpi_piix4.c change.

Maybe we could provide a --with-qemu-kvm-compat configure flag to them?

-- 
Eduardo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Anthony Liguori]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Thu, Dec 13, 2012 at 09:35:13AM +0100, Paolo Bonzini wrote:
>> Il 12/12/2012 23:39, Marcelo Tosatti ha scritto:
>> > 
>> > To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
>> > of RAM.
>> > 
>> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>> 
>> A similar patch would be needed for all machine types though, no?
>> 
>> I think distros that used to ship qemu-kvm should just change the
>> default just like for the acpi_piix4.c change.
>
> Maybe we could provide a --with-qemu-kvm-compat configure flag to
> them?

I think that defeats the purpose of a single binary.

I think it would be better for the distros to have a qemu-kvm script
that was:

/usr/libexec/qemu-kvm:

#!/bin/sh

qemu-system-x86_64 -enable-qemu-kvm-compat "$@"

Regards,

Anthony Liguori

>
> -- 
> Eduardo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Eduardo Habkost]

On Thu, Dec 13, 2012 at 03:35:03PM -0600, Anthony Liguori wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Thu, Dec 13, 2012 at 09:35:13AM +0100, Paolo Bonzini wrote:
> >> Il 12/12/2012 23:39, Marcelo Tosatti ha scritto:
> >> > 
> >> > To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> >> > of RAM.
> >> > 
> >> > Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >> 
> >> A similar patch would be needed for all machine types though, no?
> >> 
> >> I think distros that used to ship qemu-kvm should just change the
> >> default just like for the acpi_piix4.c change.
> >
> > Maybe we could provide a --with-qemu-kvm-compat configure flag to
> > them?
> 
> I think that defeats the purpose of a single binary.
> 
> I think it would be better for the distros to have a qemu-kvm script
> that was:
> 
> /usr/libexec/qemu-kvm:
> 
> #!/bin/sh
> 
> qemu-system-x86_64 -enable-qemu-kvm-compat "$@"

That would be even better. I proposed a configure flag because I
understood (maybe incorrectly) that Paolo proposed a build-time default
change.

-- 
Eduardo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

> > > > I think distros that used to ship qemu-kvm should just change
> > > > the default just like for the acpi_piix4.c change.
> > >
> > > Maybe we could provide a --with-qemu-kvm-compat configure flag to
> > > them?

I like this.

> > I think that defeats the purpose of a single binary.
> > 
> > I think it would be better for the distros to have a qemu-kvm
> > script that was:
> > 
> > /usr/libexec/qemu-kvm:
> > 
> > #!/bin/sh
> > 
> > qemu-system-x86_64 -enable-qemu-kvm-compat "$@"
> 
> That would be even better. I proposed a configure flag because I
> understood (maybe incorrectly) that Paolo proposed a build-time
> default change.

Yes, that's what I was thinking.  The problem is that Fedora did ship a
qemu-system-x86_64 binary that disabled the qemu-kvm options (including
using TCG by default), but it still had a qemu-kvm-compatible migration
format.

Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

>> > > > I think distros that used to ship qemu-kvm should just change
>> > > > the default just like for the acpi_piix4.c change.
>> > >
>> > > Maybe we could provide a --with-qemu-kvm-compat configure flag to
>> > > them?
>
> I like this.
>
>> > I think that defeats the purpose of a single binary.
>> > 
>> > I think it would be better for the distros to have a qemu-kvm
>> > script that was:
>> > 
>> > /usr/libexec/qemu-kvm:
>> > 
>> > #!/bin/sh
>> > 
>> > qemu-system-x86_64 -enable-qemu-kvm-compat "$@"
>> 
>> That would be even better. I proposed a configure flag because I
>> understood (maybe incorrectly) that Paolo proposed a build-time
>> default change.
>
> Yes, that's what I was thinking.  The problem is that Fedora did ship a
> qemu-system-x86_64 binary that disabled the qemu-kvm options (including
> using TCG by default), but it still had a qemu-kvm-compatible migration
> format.

Can you be more specific?  What's different in the migration format?

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

Il 14/12/2012 14:29, Anthony Liguori ha scritto:
>> >
>> > Yes, that's what I was thinking.  The problem is that Fedora did ship a
>> > qemu-system-x86_64 binary that disabled the qemu-kvm options (including
>> > using TCG by default), but it still had a qemu-kvm-compatible migration
>> > format.
> Can you be more specific?  What's different in the migration format?

The amount of VRAM for Cirrus is 16 MB, and the ACPI migration is
incompatible with qemu 1.2 (what Marcelo changed for 1.3).

Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Anthony Liguori]

Marcelo Tosatti <mtosatti@redhat.com> writes:

> To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> of RAM.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 19e342a..ead4b6b 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -347,6 +347,26 @@ static QEMUMachine pc_machine_v1_2 = {
>      },
>  };
>  
> +#define QEMUKVMPC_COMPAT_1_2 \
> +        {\
> +            .driver   = "cirrus-vga",\
> +            .property = "vgamem_mb",\
> +            .value    = "16",\
> +        }
> +
> +static QEMUMachine qemukvmpc_machine_v1_2 = {
> +    .name = "qemukvm-pc-1.2",
> +    .alias = "pc",

This shouldn't alias pc...  I think what you need mean to do is alias
pc-1.2 but in order to do that, we need some sort of way to indicate
that 'pc-1.2' should behave like qemu-kvm vs. qemu-system-x86_64.

Regards,

Anthony Liguori

> +    .desc = "Standard PC",
> +    .init = pc_init_pci,
> +    .max_cpus = 255,
> +    .compat_props = (GlobalProperty[]) {
> +        QEMUKVMPC_COMPAT_1_2,
> +        PC_COMPAT_1_2,
> +        { /* end of list */ }
> +    },
> +};
> +
>  #define PC_COMPAT_1_1 \
>          PC_COMPAT_1_2,\
>          {\
> @@ -645,6 +665,7 @@ static QEMUMachine xenfv_machine = {
>  
>  static void pc_machine_init(void)
>  {
> +    qemu_register_machine(&qemukvmpc_machine_v1_2);
>      qemu_register_machine(&pc_machine_v1_4);
>      qemu_register_machine(&pc_machine_v1_3);
>      qemu_register_machine(&pc_machine_v1_2);

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Cole Robinson]

On 12/12/2012 05:39 PM, Marcelo Tosatti wrote:
> 
> To allow migration from qemu-kvm-1.2, where cirrus device has 16 megabytes
> of RAM.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 

Hi Marcelo, I'm trying to sort through this again.

> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 19e342a..ead4b6b 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -347,6 +347,26 @@ static QEMUMachine pc_machine_v1_2 = {
>      },
>  };
>  
> +#define QEMUKVMPC_COMPAT_1_2 \
> +        {\
> +            .driver   = "cirrus-vga",\
> +            .property = "vgamem_mb",\
> +            .value    = "16",\
> +        }
> +
> +static QEMUMachine qemukvmpc_machine_v1_2 = {
> +    .name = "qemukvm-pc-1.2",
> +    .alias = "pc",
> +    .desc = "Standard PC",
> +    .init = pc_init_pci,
> +    .max_cpus = 255,
> +    .compat_props = (GlobalProperty[]) {
> +        QEMUKVMPC_COMPAT_1_2,
> +        PC_COMPAT_1_2,
> +        { /* end of list */ }
> +    },
> +};
> +
>  #define PC_COMPAT_1_1 \
>          PC_COMPAT_1_2,\
>          {\
> @@ -645,6 +665,7 @@ static QEMUMachine xenfv_machine = {
>  
>  static void pc_machine_init(void)
>  {
> +    qemu_register_machine(&qemukvmpc_machine_v1_2);
>      qemu_register_machine(&pc_machine_v1_4);
>      qemu_register_machine(&pc_machine_v1_3);
>      qemu_register_machine(&pc_machine_v1_2);
> 

As previously mentioned in the thread, a back compat machine type doesn't help
us out in Fedora. For the past 3 years all qemu usage in fedora has been using
qemu-kvm.git, so the only incoming migration we care about has to deal with
the qemu-kvm migration incompatibilities.

Libvirt always specifies an explicit machine type and carries it for the life
of the VM. What we want is for 'qemu-kvm-1.2 -M pc-1.2' to migrate seamlessly
into 'qemu-1.3+ -M pc-1.2' without the user or libvirt having to change
machine type.

So what we want to carry in Fedora is:


diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index aa3e7f4..b8f5f8a 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -315,6 +315,10 @@ static QEMUMachine pc_machine_v1_3 = {
             .driver   = "VGA",\
             .property = "mmio",\
             .value    = "off",\
+        },{\
+            .driver   = "cirrus-vga",\
+            .property = "vgamem_mb",\
+            .value    = stringify(8),\
         }

 static QEMUMachine pc_machine_v1_2 = {


That sticks the cirrus compat block on the end of PC_COMPAT_1_2, which should
fix cirrus migration from qemu-kvm for all machine types <= pc-1.2. Obviously
this isn't suitable for upstream but we can hide it behind something like
./configure --qemu-kvm-migrate-compat

The other migrate bit is the comment you added to hw/acpi_piix4.c:

/* qemu-kvm 1.2 uses version 3 but advertised as 2
 * To support incoming qemu-kvm 1.2 migration, change version_id
 * and minimum_version_id to 2 below (which breaks migration from
 * qemu 1.2).
 *
 */

That's a bit more problematic. Upstream has minimum_version_id=3, so if we
carry version_id=2 in Fedora, our packages will never be able to migrate to an
upstream qemu version, even a VM using pc-1.3 that was added after qemu-kvm
was deprecated.

Is it safe to just set minimum_version_id=2 in qemu.git and be done with it?
It at least doesn't blow up[1] going from qemu-kvm-1.2 -> patched qemu.git,
nor qemu-1.2.2 -> patched qemu.git. But admittedly I don't understand the
subtleties involved in migration compat.


[1] Well, doesn't blow up any more than normal. Migrating stock qemu 1.2.2 ->
qemu.git is crashing for me:

#0  0x00007ffff24b6e50 in __memcmp_sse4_1 () from /lib64/libc.so.6
#1  0x000055555578c532 in patch_hypercalls (s=0x555556513f10)
    at /home/crobinso/qemu-clean/hw/i386/../kvmvapic.c:544
#2  vapic_prepare (s=s@entry=0x555556513f10)
    at /home/crobinso/qemu-clean/hw/i386/../kvmvapic.c:609
#3  0x000055555578c612 in vapic_post_load (opaque=0x555556513f10,
    version_id=<optimized out>)
    at /home/crobinso/qemu-clean/hw/i386/../kvmvapic.c:726
#4  0x00005555557ca0b0 in vmstate_load_state (f=f@entry=0x555556636510, vmsd=
    0x555555c0a840 <vmstate_vapic>, opaque=0x555556513f10, version_id=1)
    at /home/crobinso/qemu-clean/savevm.c:1451
#5  0x00005555557cac53 in vmstate_load (version_id=<optimized out>, se=
    0x555556520da0, f=0x555556636510)
    at /home/crobinso/qemu-clean/savevm.c:1514
#6  qemu_loadvm_state (f=f@entry=0x555556636510)
    at /home/crobinso/qemu-clean/savevm.c:1977
#7  0x00005555556fa15e in process_incoming_migration_co (opaque=0x555556636510)
    at migration.c:97
#8  0x000055555561f82a in coroutine_trampoline (i0=<optimized out>,
    i1=<optimized out>) at coroutine-ucontext.c:138
#9  0x00007ffff23a16c0 in ?? () from /lib64/libc.so.6
#10 0x00007fffffffd270 in ?? ()
#11 0x0000000000000000 in ?? ()

Using:
qemu-system-x86_64 -M pc-1.2 -sdl -net none -vga cirrus -enable-kvm -m 2048
-incoming 'exec:cat migrate-to-file.img'


Thanks,
Cole

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

Il 15/01/2013 04:45, Cole Robinson ha scritto:
> Libvirt always specifies an explicit machine type and carries it for the life
> of the VM. What we want is for 'qemu-kvm-1.2 -M pc-1.2' to migrate seamlessly
> into 'qemu-1.3+ -M pc-1.2' without the user or libvirt having to change
> machine type.
> 
> So what we want to carry in Fedora is:
> 
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index aa3e7f4..b8f5f8a 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -315,6 +315,10 @@ static QEMUMachine pc_machine_v1_3 = {
>              .driver   = "VGA",\
>              .property = "mmio",\
>              .value    = "off",\
> +        },{\
> +            .driver   = "cirrus-vga",\
> +            .property = "vgamem_mb",\
> +            .value    = stringify(8),\
>          }
> 
>  static QEMUMachine pc_machine_v1_2 = {
> 
> 
> That sticks the cirrus compat block on the end of PC_COMPAT_1_2, which should
> fix cirrus migration from qemu-kvm for all machine types <= pc-1.2. Obviously
> this isn't suitable for upstream but we can hide it behind something like
> ./configure --qemu-kvm-migrate-compat

Let's look at the source to reconstruct the changes.

In qemu-kvm-0.15.1 (Fedora 16), file hw/vga_int.h, we had

    #define VGA_RAM_SIZE (16 * 1024 * 1024)

In qemu-0.15.1, same file, we had

    #define VGA_RAM_SIZE (8192 * 1024)

The same holds all the way back to 0.9, which had them in hw/pc.h.

hw/vga-pci.c in qemu-kvm-1.2 and qemu-1.2 had both this line:

    DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),

but hw/cirrus_vga.c had respectively:

    #define VGA_RAM_SIZE (16384 * 1024)
    #define VGA_RAM_SIZE (8192 * 1024)

So the right patch for downstreams that used qemu-kvm is this:

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index 85529b2..c29ea0d 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -410,22 +410,6 @@ static QEMUMachine pc_machine_v1_2 = {
             .property = "param_change",\
             .value    = "off",\
         },{\
-            .driver   = "VGA",\
-            .property = "vgamem_mb",\
-            .value    = stringify(8),\
-        },{\
-            .driver   = "vmware-svga",\
-            .property = "vgamem_mb",\
-            .value    = stringify(8),\
-        },{\
-            .driver   = "qxl-vga",\
-            .property = "vgamem_mb",\
-            .value    = stringify(8),\
-        },{\
-            .driver   = "qxl",\
-            .property = "vgamem_mb",\
-            .value    = stringify(8),\
-        },{\
             .driver   = "virtio-blk-pci",\
             .property = "config-wce",\
             .value    = "off",\
diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index 9bef96e..8c94428 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -2975,7 +2975,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)

 static Property pci_vga_cirrus_properties[] = {
     DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
-                       cirrus_vga.vga.vram_size_mb, 8),
+                       cirrus_vga.vga.vram_size_mb, 16),
     DEFINE_PROP_END_OF_LIST(),
 };


i.e. use 16 MB for all machine types and all cards.  The latter should
probably be pushed into 1.4 with compat properties for 1.3.  At this
point, you need to remove the compat property as in the pc_piix.c above.

Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Cole Robinson]

On 01/15/2013 11:29 AM, Paolo Bonzini wrote:
> Il 15/01/2013 04:45, Cole Robinson ha scritto:
>> Libvirt always specifies an explicit machine type and carries it for the life
>> of the VM. What we want is for 'qemu-kvm-1.2 -M pc-1.2' to migrate seamlessly
>> into 'qemu-1.3+ -M pc-1.2' without the user or libvirt having to change
>> machine type.
>>
>> So what we want to carry in Fedora is:
>>
>>
>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>> index aa3e7f4..b8f5f8a 100644
>> --- a/hw/pc_piix.c
>> +++ b/hw/pc_piix.c
>> @@ -315,6 +315,10 @@ static QEMUMachine pc_machine_v1_3 = {
>>              .driver   = "VGA",\
>>              .property = "mmio",\
>>              .value    = "off",\
>> +        },{\
>> +            .driver   = "cirrus-vga",\
>> +            .property = "vgamem_mb",\
>> +            .value    = stringify(8),\
>>          }
>>
>>  static QEMUMachine pc_machine_v1_2 = {
>>
>>
>> That sticks the cirrus compat block on the end of PC_COMPAT_1_2, which should
>> fix cirrus migration from qemu-kvm for all machine types <= pc-1.2. Obviously
>> this isn't suitable for upstream but we can hide it behind something like
>> ./configure --qemu-kvm-migrate-compat
> 
> Let's look at the source to reconstruct the changes.
> 
> In qemu-kvm-0.15.1 (Fedora 16), file hw/vga_int.h, we had
> 
>     #define VGA_RAM_SIZE (16 * 1024 * 1024)
> 
> In qemu-0.15.1, same file, we had
> 
>     #define VGA_RAM_SIZE (8192 * 1024)
> 
> The same holds all the way back to 0.9, which had them in hw/pc.h.
> 
> hw/vga-pci.c in qemu-kvm-1.2 and qemu-1.2 had both this line:
> 
>     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> 
> but hw/cirrus_vga.c had respectively:
> 
>     #define VGA_RAM_SIZE (16384 * 1024)
>     #define VGA_RAM_SIZE (8192 * 1024)
> 
> So the right patch for downstreams that used qemu-kvm is this:
> 
> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> index 85529b2..c29ea0d 100644
> --- a/hw/pc_piix.c
> +++ b/hw/pc_piix.c
> @@ -410,22 +410,6 @@ static QEMUMachine pc_machine_v1_2 = {
>              .property = "param_change",\
>              .value    = "off",\
>          },{\
> -            .driver   = "VGA",\
> -            .property = "vgamem_mb",\
> -            .value    = stringify(8),\
> -        },{\
> -            .driver   = "vmware-svga",\
> -            .property = "vgamem_mb",\
> -            .value    = stringify(8),\
> -        },{\
> -            .driver   = "qxl-vga",\
> -            .property = "vgamem_mb",\
> -            .value    = stringify(8),\
> -        },{\
> -            .driver   = "qxl",\
> -            .property = "vgamem_mb",\
> -            .value    = stringify(8),\
> -        },{\
>              .driver   = "virtio-blk-pci",\
>              .property = "config-wce",\
>              .value    = "off",\
> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> index 9bef96e..8c94428 100644
> --- a/hw/cirrus_vga.c
> +++ b/hw/cirrus_vga.c
> @@ -2975,7 +2975,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> 
>  static Property pci_vga_cirrus_properties[] = {
>      DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
> -                       cirrus_vga.vga.vram_size_mb, 8),
> +                       cirrus_vga.vga.vram_size_mb, 16),
>      DEFINE_PROP_END_OF_LIST(),
>  };
> 
> 
> i.e. use 16 MB for all machine types and all cards.  The latter should
> probably be pushed into 1.4 with compat properties for 1.3.  At this
> point, you need to remove the compat property as in the pc_piix.c above.
> 

Thanks for the analysis Paolo, here's the final patch I used:

http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0004-Fix-migration-compat-with-qemu-kvm.patch

Thanks,
Cole

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Marcelo Tosatti]

On Wed, Jan 16, 2013 at 06:17:03PM -0500, Cole Robinson wrote:
> On 01/15/2013 11:29 AM, Paolo Bonzini wrote:
> > Il 15/01/2013 04:45, Cole Robinson ha scritto:
> >> Libvirt always specifies an explicit machine type and carries it for the life
> >> of the VM. What we want is for 'qemu-kvm-1.2 -M pc-1.2' to migrate seamlessly
> >> into 'qemu-1.3+ -M pc-1.2' without the user or libvirt having to change
> >> machine type.
> >>
> >> So what we want to carry in Fedora is:
> >>
> >>
> >> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> >> index aa3e7f4..b8f5f8a 100644
> >> --- a/hw/pc_piix.c
> >> +++ b/hw/pc_piix.c
> >> @@ -315,6 +315,10 @@ static QEMUMachine pc_machine_v1_3 = {
> >>              .driver   = "VGA",\
> >>              .property = "mmio",\
> >>              .value    = "off",\
> >> +        },{\
> >> +            .driver   = "cirrus-vga",\
> >> +            .property = "vgamem_mb",\
> >> +            .value    = stringify(8),\
> >>          }
> >>
> >>  static QEMUMachine pc_machine_v1_2 = {
> >>
> >>
> >> That sticks the cirrus compat block on the end of PC_COMPAT_1_2, which should
> >> fix cirrus migration from qemu-kvm for all machine types <= pc-1.2. Obviously
> >> this isn't suitable for upstream but we can hide it behind something like
> >> ./configure --qemu-kvm-migrate-compat
> > 
> > Let's look at the source to reconstruct the changes.
> > 
> > In qemu-kvm-0.15.1 (Fedora 16), file hw/vga_int.h, we had
> > 
> >     #define VGA_RAM_SIZE (16 * 1024 * 1024)
> > 
> > In qemu-0.15.1, same file, we had
> > 
> >     #define VGA_RAM_SIZE (8192 * 1024)
> > 
> > The same holds all the way back to 0.9, which had them in hw/pc.h.
> > 
> > hw/vga-pci.c in qemu-kvm-1.2 and qemu-1.2 had both this line:
> > 
> >     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
> > 
> > but hw/cirrus_vga.c had respectively:
> > 
> >     #define VGA_RAM_SIZE (16384 * 1024)
> >     #define VGA_RAM_SIZE (8192 * 1024)
> > 
> > So the right patch for downstreams that used qemu-kvm is this:
> > 
> > diff --git a/hw/pc_piix.c b/hw/pc_piix.c
> > index 85529b2..c29ea0d 100644
> > --- a/hw/pc_piix.c
> > +++ b/hw/pc_piix.c
> > @@ -410,22 +410,6 @@ static QEMUMachine pc_machine_v1_2 = {
> >              .property = "param_change",\
> >              .value    = "off",\
> >          },{\
> > -            .driver   = "VGA",\
> > -            .property = "vgamem_mb",\
> > -            .value    = stringify(8),\
> > -        },{\
> > -            .driver   = "vmware-svga",\
> > -            .property = "vgamem_mb",\
> > -            .value    = stringify(8),\
> > -        },{\
> > -            .driver   = "qxl-vga",\
> > -            .property = "vgamem_mb",\
> > -            .value    = stringify(8),\
> > -        },{\
> > -            .driver   = "qxl",\
> > -            .property = "vgamem_mb",\
> > -            .value    = stringify(8),\
> > -        },{\
> >              .driver   = "virtio-blk-pci",\
> >              .property = "config-wce",\
> >              .value    = "off",\
> > diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
> > index 9bef96e..8c94428 100644
> > --- a/hw/cirrus_vga.c
> > +++ b/hw/cirrus_vga.c
> > @@ -2975,7 +2975,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
> > 
> >  static Property pci_vga_cirrus_properties[] = {
> >      DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
> > -                       cirrus_vga.vga.vram_size_mb, 8),
> > +                       cirrus_vga.vga.vram_size_mb, 16),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> > 
> > 
> > i.e. use 16 MB for all machine types and all cards.  The latter should
> > probably be pushed into 1.4 with compat properties for 1.3.  At this
> > point, you need to remove the compat property as in the pc_piix.c above.
> > 
> 
> Thanks for the analysis Paolo, here's the final patch I used:
> 
> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0004-Fix-migration-compat-with-qemu-kvm.patch
> 
> Thanks,
> Cole

Cole,

version_id and minimum_version_id should be changed to 2, as the comment
indicates.

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Cole Robinson]

On 01/18/2013 06:12 AM, Marcelo Tosatti wrote:
> On Wed, Jan 16, 2013 at 06:17:03PM -0500, Cole Robinson wrote:
>> On 01/15/2013 11:29 AM, Paolo Bonzini wrote:
>>> Il 15/01/2013 04:45, Cole Robinson ha scritto:
>>>> Libvirt always specifies an explicit machine type and carries it for the life
>>>> of the VM. What we want is for 'qemu-kvm-1.2 -M pc-1.2' to migrate seamlessly
>>>> into 'qemu-1.3+ -M pc-1.2' without the user or libvirt having to change
>>>> machine type.
>>>>
>>>> So what we want to carry in Fedora is:
>>>>
>>>>
>>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>>> index aa3e7f4..b8f5f8a 100644
>>>> --- a/hw/pc_piix.c
>>>> +++ b/hw/pc_piix.c
>>>> @@ -315,6 +315,10 @@ static QEMUMachine pc_machine_v1_3 = {
>>>>              .driver   = "VGA",\
>>>>              .property = "mmio",\
>>>>              .value    = "off",\
>>>> +        },{\
>>>> +            .driver   = "cirrus-vga",\
>>>> +            .property = "vgamem_mb",\
>>>> +            .value    = stringify(8),\
>>>>          }
>>>>
>>>>  static QEMUMachine pc_machine_v1_2 = {
>>>>
>>>>
>>>> That sticks the cirrus compat block on the end of PC_COMPAT_1_2, which should
>>>> fix cirrus migration from qemu-kvm for all machine types <= pc-1.2. Obviously
>>>> this isn't suitable for upstream but we can hide it behind something like
>>>> ./configure --qemu-kvm-migrate-compat
>>>
>>> Let's look at the source to reconstruct the changes.
>>>
>>> In qemu-kvm-0.15.1 (Fedora 16), file hw/vga_int.h, we had
>>>
>>>     #define VGA_RAM_SIZE (16 * 1024 * 1024)
>>>
>>> In qemu-0.15.1, same file, we had
>>>
>>>     #define VGA_RAM_SIZE (8192 * 1024)
>>>
>>> The same holds all the way back to 0.9, which had them in hw/pc.h.
>>>
>>> hw/vga-pci.c in qemu-kvm-1.2 and qemu-1.2 had both this line:
>>>
>>>     DEFINE_PROP_UINT32("vgamem_mb", PCIVGAState, vga.vram_size_mb, 16),
>>>
>>> but hw/cirrus_vga.c had respectively:
>>>
>>>     #define VGA_RAM_SIZE (16384 * 1024)
>>>     #define VGA_RAM_SIZE (8192 * 1024)
>>>
>>> So the right patch for downstreams that used qemu-kvm is this:
>>>
>>> diff --git a/hw/pc_piix.c b/hw/pc_piix.c
>>> index 85529b2..c29ea0d 100644
>>> --- a/hw/pc_piix.c
>>> +++ b/hw/pc_piix.c
>>> @@ -410,22 +410,6 @@ static QEMUMachine pc_machine_v1_2 = {
>>>              .property = "param_change",\
>>>              .value    = "off",\
>>>          },{\
>>> -            .driver   = "VGA",\
>>> -            .property = "vgamem_mb",\
>>> -            .value    = stringify(8),\
>>> -        },{\
>>> -            .driver   = "vmware-svga",\
>>> -            .property = "vgamem_mb",\
>>> -            .value    = stringify(8),\
>>> -        },{\
>>> -            .driver   = "qxl-vga",\
>>> -            .property = "vgamem_mb",\
>>> -            .value    = stringify(8),\
>>> -        },{\
>>> -            .driver   = "qxl",\
>>> -            .property = "vgamem_mb",\
>>> -            .value    = stringify(8),\
>>> -        },{\
>>>              .driver   = "virtio-blk-pci",\
>>>              .property = "config-wce",\
>>>              .value    = "off",\
>>> diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
>>> index 9bef96e..8c94428 100644
>>> --- a/hw/cirrus_vga.c
>>> +++ b/hw/cirrus_vga.c
>>> @@ -2975,7 +2975,7 @@ static int pci_cirrus_vga_initfn(PCIDevice *dev)
>>>
>>>  static Property pci_vga_cirrus_properties[] = {
>>>      DEFINE_PROP_UINT32("vgamem_mb", struct PCICirrusVGAState,
>>> -                       cirrus_vga.vga.vram_size_mb, 8),
>>> +                       cirrus_vga.vga.vram_size_mb, 16),
>>>      DEFINE_PROP_END_OF_LIST(),
>>>  };
>>>
>>>
>>> i.e. use 16 MB for all machine types and all cards.  The latter should
>>> probably be pushed into 1.4 with compat properties for 1.3.  At this
>>> point, you need to remove the compat property as in the pc_piix.c above.
>>>
>>
>> Thanks for the analysis Paolo, here's the final patch I used:
>>
>> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0004-Fix-migration-compat-with-qemu-kvm.patch
>>
>> Thanks,
>> Cole
> 
> Cole,
> 
> version_id and minimum_version_id should be changed to 2, as the comment
> indicates.
> 
> 
> 

But won't that mean we have to carry that patch forever, and while we carry
that patch we can never migrate from Fedora qemu to an upstream qemu instance?
I'd like to avoid carrying any incompatibility forward, if possible.

- Cole

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

> > version_id and minimum_version_id should be changed to 2, as the
> > comment indicates.
> 
> But won't that mean we have to carry that patch forever, and while we carry
> that patch we can never migrate from Fedora qemu to an upstream qemu instance?
> I'd like to avoid carrying any incompatibility forward, if possible.

To some extent you have to choose between backwards- and forwards-
compatibility.  But I think that you can achieve what you want
by leaving version_id to 3, while setting minimum_version_id to 2.

Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Marcelo Tosatti]

On Fri, Jan 18, 2013 at 12:34:57PM -0500, Paolo Bonzini wrote:
> > > version_id and minimum_version_id should be changed to 2, as the
> > > comment indicates.
> > 
> > But won't that mean we have to carry that patch forever, and while we carry
> > that patch we can never migrate from Fedora qemu to an upstream qemu instance?
> > I'd like to avoid carrying any incompatibility forward, if possible.
> 
> To some extent you have to choose between backwards- and forwards-
> compatibility.  But I think that you can achieve what you want
> by leaving version_id to 3, while setting minimum_version_id to 2.
> 
> Paolo

What about

"Problem is it uses acpi_load_old, when reading from qemu-kvm 1.2 (which
advertises format as V2), which reads 4*16 bits (instead of 16 bits)
for en/sts fields. So it can corrupt incoming migration data."

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Paolo Bonzini]

> On Fri, Jan 18, 2013 at 12:34:57PM -0500, Paolo Bonzini wrote:
> > > > version_id and minimum_version_id should be changed to 2, as the
> > > > comment indicates.
> > > 
> > > But won't that mean we have to carry that patch forever, and
> > > while we carry
> > > that patch we can never migrate from Fedora qemu to an upstream
> > > qemu instance?
> > > I'd like to avoid carrying any incompatibility forward, if
> > > possible.
> > 
> > To some extent you have to choose between backwards- and forwards-
> > compatibility.  But I think that you can achieve what you want
> > by leaving version_id to 3, while setting minimum_version_id to 2.
> 
> What about
> 
> "Problem is it uses acpi_load_old, when reading from qemu-kvm 1.2 (which
> advertises format as V2), which reads 4*16 bits (instead of 16 bits)
> for en/sts fields. So it can corrupt incoming migration data."

That's if minimum_version_id==3.

But if you set minimum_version_id==2, you fix incoming migration from
qemu-kvm 1.2 (and break it from upstream QEMU 1.2).  acpi_load_old will
only be called for version 1.

At the same time, because version_id==3 you will still have working
migration to upstream QEMU 1.3 and future releases (and break backwards
migration to qemu-kvm 1.2, but that's not a problem).

Paolo

Re: [Qemu-devel] [PATCH] define qemukvm-1.2 machine type

[Marcelo Tosatti]

On Fri, Jan 18, 2013 at 11:04:04AM -0600, Cole Robinson wrote:
> >> Thanks for the analysis Paolo, here's the final patch I used:
> >>
> >> http://pkgs.fedoraproject.org/cgit/qemu.git/tree/0004-Fix-migration-compat-with-qemu-kvm.patch
> >>
> >> Thanks,
> >> Cole
> > 
> > Cole,
> > 
> > version_id and minimum_version_id should be changed to 2, as the comment
> > indicates.

Problem is it uses acpi_load_old, when reading from qemu-kvm 1.2 (which
advertises format as V2), which reads 4*16 bits (instead of 16 bits)
for en/sts fields. So it can corrupt incoming migration data.

> But won't that mean we have to carry that patch forever, and while we carry
> that patch we can never migrate from Fedora qemu to an upstream qemu instance?
> I'd like to avoid carrying any incompatibility forward, if possible.

No useful ideas :(