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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.