Patchwork define qemukvm-1.2 machine type

login
register
mail settings
Submitter Paolo Bonzini
Date Jan. 15, 2013, 4:29 p.m.
Message ID <50F583E6.1040907@redhat.com>
Download mbox | patch
Permalink /patch/212223/
State New
Headers show

Comments

Paolo Bonzini - Jan. 15, 2013, 4:29 p.m.
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:


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
Cole Robinson - Jan. 16, 2013, 11:17 p.m.
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
Marcelo Tosatti - Jan. 18, 2013, 12:12 p.m.
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.
Cole Robinson - Jan. 18, 2013, 5:04 p.m.
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
Paolo Bonzini - Jan. 18, 2013, 5:34 p.m.
> > 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
Marcelo Tosatti - Jan. 18, 2013, 7:50 p.m.
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 :(
Marcelo Tosatti - Jan. 18, 2013, 8:09 p.m.
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."
Paolo Bonzini - Jan. 19, 2013, 2:17 p.m.
> 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

Patch

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(),
 };