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

[Cole Robinson]

On 02/07/2013 07:31 AM, Alon Levy wrote:
>> On 02/06/2013 05:52 PM, Cole Robinson wrote:
>>> On 01/22/2013 05:09 AM, Gerd Hoffmann wrote:
>>>> From: Alon Levy <address@hidden>
>>>>
>>>> This is a simpler solution to 869981, where migration breaks since
>>>> qxl's
>>>> rom bar size has changed. Instead of ignoring fields in QXLRom,
>>>> which is what has
>>>> actually changed, we remove some of the modes, a mechanism already
>>>> accounted for by the guest. The modes left allow for portrait and
>>>> landscape only modes, corresponding to orientations 0 and 1.
>>>> Orientations 2 and 3 are dropped.
>>>>
>>>> Added assert so that rom size will fit the future QXLRom increases
>>>> via
>>>> spice-protocol changes.
>>>>
>>>> This patch has been tested with 6.1.0.10015. With the newer
>>>> 6.1.0.10016
>>>> there are problems with both "(flipped)" modes prior to the patch,
>>>> and
>>>> the patch loses the ability to set "Portrait" modes. But this is a
>>>> separate bug to be fixed in the driver, and besides the patch
>>>> doesn't
>>>> affect the new arbitrary mode setting functionality.
>>>>
>>>> Signed-off-by: Alon Levy <address@hidden>
>>>> Signed-off-by: Gerd Hoffmann <address@hidden>
>>>> ---
>>>>  hw/qxl.c |   13 +++++++------
>>>>  1 file changed, 7 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/hw/qxl.c b/hw/qxl.c
>>>> index 0d81816..a125e29 100644
>>>> --- a/hw/qxl.c
>>>> +++ b/hw/qxl.c
>>>> @@ -80,9 +80,7 @@
>>>>  
>>>>  #define QXL_MODE_EX(x_res, y_res)                 \
>>>>      QXL_MODE_16_32(x_res, y_res, 0),              \
>>>> -    QXL_MODE_16_32(y_res, x_res, 1),              \
>>>> -    QXL_MODE_16_32(x_res, y_res, 2),              \
>>>> -    QXL_MODE_16_32(y_res, x_res, 3)
>>>> +    QXL_MODE_16_32(x_res, y_res, 1)
>>>>  
>>>>  static QXLMode qxl_modes[] = {
>>>>      QXL_MODE_EX(640, 480),
>>>> @@ -306,10 +304,13 @@ static inline uint32_t msb_mask(uint32_t
>>>> val)
>>>>  
>>>>  static ram_addr_t qxl_rom_size(void)
>>>>  {
>>>> -    uint32_t rom_size = sizeof(QXLRom) + sizeof(QXLModes) +
>>>> sizeof(qxl_modes);
>>>> +    uint32_t required_rom_size = sizeof(QXLRom) +
>>>> sizeof(QXLModes) +
>>>> +                                 sizeof(qxl_modes);
>>>> +    uint32_t rom_size = 8192; /* two pages */
>>>>  
>>>> -    rom_size = MAX(rom_size, TARGET_PAGE_SIZE);
>>>> -    rom_size = msb_mask(rom_size * 2 - 1);
>>>> +    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;
>>>>  }
>>>>  
>>>>
>>>
>>> This breaks migration for me with -M pc-1.0 at least.
>>>
>>> Before this commit:
>>>
>>> ./x86_64-softmmu/qemu-system-x86_64 -M pc-1.0 -vga qxl -spice
>>> port=5905
>>> -monitor stdio
>>>> migrate "exec: cat > foo.img"
>>>> quit
>>>
>>> After this commit:
>>>
>>> ./x86_64-softmmu/qemu-system-x86_64 -M pc-1.0 -vga qxl -spice
>>> port=5905
>>> -monitor stdio -incoming "exec: cat foo.img"
>>> qemu: warning: error while loading state for instance 0x0 of device
>>> 'ram'
>>> cat: write error: Broken pipe
>>> load of migration failed
>>>
>>
>> Sorry, I realize that may seem a bit artificial, so just to clarify a
>> bit. If
>> I start on v1.3.1, then cherry-pick this commit, it breaks migration
>> using the
>> commands above. I also tried v1.3.1 to git head, reverting the
>> seabios rebase
>> that caused other migration issues, and got the same error (is there
>> any way
>> to get better error reporting out of migration failures?).
> 
> Migration error reporting sucks. Did you create foo.img with pre-patched qemu 
> and then try to read it with post patched, or both are using the same version?

I tried:

foo.img generated with stock qemu 1.3.1, migration fails when cherry picking
the patch.

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)

- Cole