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

[Alon Levy]

> 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)

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?

> 
> - Cole
> 
> 
>