Re: [Qemu-devel] [PATCH 2/2] qxl: change rom so that 4096 < size < 8192

[Alon Levy <alevy@redhat.com>]

On Thu, Dec 13, 2012 at 01:05:48PM +0100, Gerd Hoffmann wrote:
> On 12/13/12 12:36, Alon Levy wrote:
> > 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. To reach exactly two pages and not one, we
> > remove two out of four orientations, the remaining are normal and right
> > turn (chosen arbitrarily). Leaving only normal would result in a single
> > page which would also break migration.
> > 
> > Added assertions to ensure changes in spice-protocol in the future
> > causing increase or decrease of page size will result in failure at
> > startup (could do this compile time also but not sure how).
> 
> The assertions are not in the patch.
> 
> >  #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)
> 
> Why do you leave orientation = 1 in?  Just to keep the size above 4K?
> Shouldn't we just hardcode the rom size to 8k instead?  Then assert that
> everything fits into 8k?  Or even better add a compile time check?
> 
> While being at it it might be a good idea to move the mode table to a
> fixed, large enougth offset (say 0x4096), so it doesn't move around
> again in case we extend QXLRom one more time.

This solution is breaking backward compatibility like Yonit pointed
out. The fact that I can't produce a user that would break because of
this doesn't prove there is no such user. I suggest we go back to the
original patch I posted, breaking it into two like you requested. What
do you say?

> 
> cheers,
>   Gerd
>