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

[Cole Robinson <crobinso@redhat.com>]

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