Re: [Qemu-devel] [PATCH 2/2] qxl: change rom size to 8192

[Cole Robinson]

On 02/19/2013 05:15 PM, Cole Robinson wrote:
> On 02/08/2013 04:36 AM, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> foo.img generated with stock qemu 1.3.1, migration fails with git
>>>> head, still
>>>> fails after reverting the seabios 1.7.2 update which supposedly
>>>> causes other
>>>> migrate issues (commit 3588185b8396eb97fd9efd41c2b97775465f67c4)
>>>
>>> So this is by design - that patch changes the rom size for revision 4 
>>> (which is the default for qemu 1.3.1) from 16384 bytes to 8192. This breaks 
>>> migration from previously created revision 4 images... But it unbreaks 
>>> migration from other revisions. Different things that could be done: do a 
>>> special migration hook to copy from src 16384 byte rom bar to dst 8192 byte 
>>> rom bar. Gerd, what do you think?
>>
>> So the change making the rpm 16k was before 1.3, the fix thereafter, and
>> now we have migration 1.2 -> 1.3 broken (maybe masked by -M 1.2
>> defaulting to qxl rev 3), 1.3 -> 1.4 broken and 1.2 -> 1.4 working, correct?
>>
>> So a compat property changing the rom size to 16k for pc-1.3 should do
>> the trick I think.
>>
> 
> I took a stab at this, unfortunately the situation isn't that simple. qxl
> rom_size is actually dependent on the spice version we build against:
> 
> stock qemu 1.2 built on F17 against spice-0.10.1:
> sizeof(QXLRom)=72 rom_size=8192
> 
> stock qemu 1.2 built on F18 against spice-0.12.2:
> sizeof(QXLRom)=1160 rom_size=16834
> 
> Maybe a more complex solution like Alon proposed up thread is needed.
> 

For reference, here's the patch I'm carrying in Fedora. It likely isn't
suitable for upstream given what I mentioned above, but is good enough
to handle rom sizes of Fedora packages.



>From 95a59bc743f27d7d3fdcc1b0ff131f240e01e839 Mon Sep 17 00:00:00 2001
From: Cole Robinson <address@hidden>
Date: Tue, 19 Feb 2013 16:19:02 -0500
Subject: [PATCH] qxl: Add rom_size compat property, fix migration from 1.2

Commit 038c1879a00153b14bce113315b693e8c2944fa9 changed the qxl rom
size to 8192, which fixes incoming migration from qemu 1.0. However
from qemu 1.2 and 1.3 had rom size 16384, so incoming migration
from those versions is now broken.

Add a rom_size compat property. 1.2 and 1.3 get 16384, everything
else is 8192.

This isn't actually fool proof, since rom_size can be dependent on
the version of spice qemu is built against:

https://lists.gnu.org/archive/html/qemu-devel/2013-02/msg03154.html

However these sizes match what native Fedora packages get, so it's
good enough for now.

Signed-off-by: Cole Robinson <address@hidden>
---
 hw/pc_piix.c | 16 ++++++++++++++++
 hw/qxl.c     |  9 ++++-----
 2 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/hw/pc_piix.c b/hw/pc_piix.c
index e3f8e96..a1a6794 100644
--- a/hw/pc_piix.c
+++ b/hw/pc_piix.c
@@ -317,6 +317,14 @@ static QEMUMachine pc_i440fx_machine_v1_4 = {
             .driver   = "virtio-net-pci", \
             .property = "mq", \
             .value    = "off", \
+        },{ \
+            .driver   = "qxl", \
+            .property = "rom_size", \
+            .value    = stringify(16384), \
+        },{\
+            .driver   = "qxl-vga", \
+            .property = "rom_size", \
+            .value    = stringify(16384), \
         }

 static QEMUMachine pc_machine_v1_3 = {
@@ -413,6 +421,14 @@ static QEMUMachine pc_machine_v1_2 = {
             .driver   = "virtio-blk-pci",\
             .property = "config-wce",\
             .value    = "off",\
+        },{ \
+            .driver   = "qxl", \
+            .property = "rom_size", \
+            .value    = stringify(8192), \
+        },{\
+            .driver   = "qxl-vga", \
+            .property = "rom_size", \
+            .value    = stringify(8192), \
         }

 static QEMUMachine pc_machine_v1_1 = {
diff --git a/hw/qxl.c b/hw/qxl.c
index 2e1c5e2..436e375 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -302,16 +302,14 @@ static inline uint32_t msb_mask(uint32_t val)
     return mask;
 }

-static ram_addr_t qxl_rom_size(void)
+static void check_qxl_rom_size(PCIQXLDevice *d)
 {
     uint32_t required_rom_size = sizeof(QXLRom) + sizeof(QXLModes) +
                                  sizeof(qxl_modes);
-    uint32_t rom_size = 8192; /* two pages */

     required_rom_size = MAX(required_rom_size, TARGET_PAGE_SIZE);
     required_rom_size = msb_mask(required_rom_size * 2 - 1);
-    assert(required_rom_size <= rom_size);
-    return rom_size;
+    assert(required_rom_size <= d->rom_size);
 }

 static void init_qxl_rom(PCIQXLDevice *d)
@@ -1979,7 +1977,7 @@ static int qxl_init_common(PCIQXLDevice *qxl)
     pci_set_byte(&config[PCI_REVISION_ID], pci_device_rev);
     pci_set_byte(&config[PCI_INTERRUPT_PIN], 1);

-    qxl->rom_size = qxl_rom_size();
+    check_qxl_rom_size(qxl);
     memory_region_init_ram(&qxl->rom_bar, "qxl.vrom", qxl->rom_size);
     vmstate_register_ram(&qxl->rom_bar, &qxl->pci.qdev);
     init_qxl_rom(qxl);
@@ -2296,6 +2294,7 @@ static Property qxl_properties[] = {
         DEFINE_PROP_UINT32("vram64_size_mb", PCIQXLDevice, vram_size_mb, -1),
         DEFINE_PROP_UINT32("vgamem_mb", PCIQXLDevice, vgamem_size_mb, 16),
         DEFINE_PROP_INT32("surfaces", PCIQXLDevice, ssd.num_surfaces, 1024),
+        DEFINE_PROP_UINT32("rom_size", PCIQXLDevice, rom_size, 8192),
         DEFINE_PROP_END_OF_LIST(),
 };