| Message ID | 50F583E6.1040907@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
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.
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
> > 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
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 :(
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."
> 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
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(), };