[Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Alon Levy]

RHBZ 869981

Before this patch revision < 4 (4 is the default) would result in a wrong
qxl_rom size of 16384 instead of 8192 when building with
spice-protocol-0.12, due to the addition of fields in
the rom for client capabilities and monitors config that were added
between spice-protocol 0.10 and 0.12.

The solution is a bit involved, since I decided not to change QXLRom
which is defined externally in spice-protocol. Instead for revision < 4
we allocate 72 bytes for the QXLRom on the qxl_rom bar (bytes [0,71])
and make sure no fields out of that range are accessed, via checking of
the revision and nop-ing.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 37 ++++++++++++++++++++++++++++++++-----
 hw/qxl.h     |  2 ++
 trace-events |  2 ++
 3 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 3f835b8..4794f13 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -314,10 +314,26 @@ static inline uint32_t msb_mask(uint32_t val)
     return mask;
 }
 
-static ram_addr_t qxl_rom_size(void)
+static ram_addr_t init_qxl_rom_size(PCIQXLDevice *qxl)
 {
-    uint32_t rom_size = sizeof(QXLRom) + sizeof(QXLModes) + sizeof(qxl_modes);
+    uint32_t rom_size;
 
+    switch (qxl->revision) {
+    case 1:
+    case 2:
+    case 3:
+        /* rom_size ends up in [4096, 8192), so it fits all revisions <= 3 */
+        qxl->qxl_rom_size = 72;
+        break;
+    case 4:
+        /* rom_size ends up >= 8192 for spice-protocol >= 12.1 because of added
+         * client capabilities */
+        qxl->qxl_rom_size = sizeof(QXLRom);
+        break;
+    default:
+        abort();
+    }
+    rom_size = qxl->qxl_rom_size + sizeof(QXLModes) + sizeof(qxl_modes);
     rom_size = MAX(rom_size, TARGET_PAGE_SIZE);
     rom_size = msb_mask(rom_size * 2 - 1);
     return rom_size;
@@ -326,7 +342,7 @@ static ram_addr_t qxl_rom_size(void)
 static void init_qxl_rom(PCIQXLDevice *d)
 {
     QXLRom *rom = memory_region_get_ram_ptr(&d->rom_bar);
-    QXLModes *modes = (QXLModes *)(rom + 1);
+    QXLModes *modes = (QXLModes *)((void *)rom + d->qxl_rom_size);
     uint32_t ram_header_size;
     uint32_t surface0_area_size;
     uint32_t num_pages;
@@ -338,7 +354,7 @@ static void init_qxl_rom(PCIQXLDevice *d)
     rom->magic         = cpu_to_le32(QXL_ROM_MAGIC);
     rom->id            = cpu_to_le32(d->id);
     rom->log_level     = cpu_to_le32(d->guestdebug);
-    rom->modes_offset  = cpu_to_le32(sizeof(QXLRom));
+    rom->modes_offset  = cpu_to_le32(d->qxl_rom_size);
 
     rom->slot_gen_bits = MEMSLOT_GENERATION_BITS;
     rom->slot_id_bits  = MEMSLOT_SLOT_BITS;
@@ -981,6 +997,12 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
+    if (qxl->revision < 4) {
+        trace_qxl_set_client_capabilities_unsupported_by_revision(qxl->id,
+                                                                  qxl->revision);
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE) ||
         runstate_check(RUN_STATE_POSTMIGRATE)) {
         return;
@@ -1013,6 +1035,11 @@ static int interface_client_monitors_config(QXLInstance *sin,
     QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
     int i;
 
+    if (qxl->revision < 4) {
+        trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
+                                                               qxl->revision);
+        return 0;
+    }
     /*
      * Older windows drivers set int_mask to 0 when their ISR is called,
      * then later set it to ~0. So it doesn't relate to the actual interrupts
@@ -2031,7 +2058,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();
+    qxl->rom_size = init_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);
diff --git a/hw/qxl.h b/hw/qxl.h
index b3564fb..c9dee70 100644
--- a/hw/qxl.h
+++ b/hw/qxl.h
@@ -92,6 +92,8 @@ typedef struct PCIQXLDevice {
     QXLRom             shadow_rom;
     QXLRom             *rom;
     QXLModes           *modes;
+    uint32_t           qxl_rom_size; /* size allocated for QXLRom,
+                                        <= sizeof(QXLRom) */
     uint32_t           rom_size;
     MemoryRegion       rom_bar;
 
diff --git a/trace-events b/trace-events
index 6c6cbf1..7d9d62d 100644
--- a/trace-events
+++ b/trace-events
@@ -1006,8 +1006,10 @@ qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
 qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
 qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Gerd Hoffmann]

On 12/06/12 16:41, Alon Levy wrote:
> RHBZ 869981
> 
> Before this patch revision < 4 (4 is the default) would result in a wrong
> qxl_rom size of 16384 instead of 8192 when building with
> spice-protocol-0.12, due to the addition of fields in
> the rom for client capabilities and monitors config that were added
> between spice-protocol 0.10 and 0.12.
> 
> The solution is a bit involved, since I decided not to change QXLRom
> which is defined externally in spice-protocol. Instead for revision < 4
> we allocate 72 bytes for the QXLRom on the qxl_rom bar (bytes [0,71])
> and make sure no fields out of that range are accessed, via checking of
> the revision and nop-ing.

Ok, I see we tackle two issues here.

Number one is qxl accessing the new fields with revision being < 4.
That needs fixing indeed.  But separate patch please.

Number two is breaking migration due to the rom size change.  Can't we
just get the rom below 8k again instead?  I think we can throw away a
whole bunch of modes.  Each mode is four times in the list, for
orientation = { 0, 1, 2, 3 }.  orientation is never ever used anywhere,
looks like historic leftover or something planned which was never
actually implemented.

So keeping orientation = 0 only and kick out everything else should give
us plenty of room ...

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Alon Levy]

> On 12/06/12 16:41, Alon Levy wrote:
> > RHBZ 869981
> > 
> > Before this patch revision < 4 (4 is the default) would result in a
> > wrong
> > qxl_rom size of 16384 instead of 8192 when building with
> > spice-protocol-0.12, due to the addition of fields in
> > the rom for client capabilities and monitors config that were added
> > between spice-protocol 0.10 and 0.12.
> > 
> > The solution is a bit involved, since I decided not to change
> > QXLRom
> > which is defined externally in spice-protocol. Instead for revision
> > < 4
> > we allocate 72 bytes for the QXLRom on the qxl_rom bar (bytes
> > [0,71])
> > and make sure no fields out of that range are accessed, via
> > checking of
> > the revision and nop-ing.
> 
> Ok, I see we tackle two issues here.
> 
> Number one is qxl accessing the new fields with revision being < 4.
> That needs fixing indeed.  But separate patch please.

Will do.

> 
> Number two is breaking migration due to the rom size change.  Can't
> we
> just get the rom below 8k again instead?  I think we can throw away a
> whole bunch of modes.  Each mode is four times in the list, for
> orientation = { 0, 1, 2, 3 }.  orientation is never ever used
> anywhere,
> looks like historic leftover or something planned which was never
> actually implemented.
> 
> So keeping orientation = 0 only and kick out everything else should
> give
> us plenty of room ...

That is indeed a better solution, but it does change functionality. I think it is correct but I'd like to get some other opinions - Uri, Arnon, Yonit, Soren - any problems with dropping these?

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Yonit Halperin]

On 12/13/2012 05:40 AM, Alon Levy wrote:
>> On 12/06/12 16:41, Alon Levy wrote:
>>> RHBZ 869981
>>>
>>> Before this patch revision < 4 (4 is the default) would result in a
>>> wrong
>>> qxl_rom size of 16384 instead of 8192 when building with
>>> spice-protocol-0.12, due to the addition of fields in
>>> the rom for client capabilities and monitors config that were added
>>> between spice-protocol 0.10 and 0.12.
>>>
>>> The solution is a bit involved, since I decided not to change
>>> QXLRom
>>> which is defined externally in spice-protocol. Instead for revision
>>> < 4
>>> we allocate 72 bytes for the QXLRom on the qxl_rom bar (bytes
>>> [0,71])
>>> and make sure no fields out of that range are accessed, via
>>> checking of
>>> the revision and nop-ing.
>>
>> Ok, I see we tackle two issues here.
>>
>> Number one is qxl accessing the new fields with revision being < 4.
>> That needs fixing indeed.  But separate patch please.
>
> Will do.
>
>>
>> Number two is breaking migration due to the rom size change.  Can't
>> we
>> just get the rom below 8k again instead?  I think we can throw away a
>> whole bunch of modes.  Each mode is four times in the list, for
>> orientation = { 0, 1, 2, 3 }.  orientation is never ever used
>> anywhere,
>> looks like historic leftover or something planned which was never
>> actually implemented.
>>
>> So keeping orientation = 0 only and kick out everything else should
>> give
>> us plenty of room ...
>
> That is indeed a better solution, but it does change functionality. I think it is correct but I'd like to get some other opinions - Uri, Arnon, Yonit, Soren - any problems with dropping these?
>
Orientation is used in the Windows Display driver. It is used to set
dmDisplayOrientation in DEVMODEW structure 
(http://msdn.microsoft.com/en-us/library/windows/hardware/ff552837(v=vs.85).aspx). 
So, I'm not sure we can just drop it. Moreover, we need at least 2 of 
the orientations, one for AxB resolution and the other for BxA.
>>
>> cheers,
>>    Gerd
>>
>>
>

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Gerd Hoffmann]

  Hi,

>> That is indeed a better solution, but it does change functionality. I
>> think it is correct but I'd like to get some other opinions - Uri,
>> Arnon, Yonit, Soren - any problems with dropping these?
>>
> Orientation is used in the Windows Display driver. It is used to set
> dmDisplayOrientation in DEVMODEW structure
> (http://msdn.microsoft.com/en-us/library/windows/hardware/ff552837(v=vs.85).aspx).

What is supposed to happen in case the guest picks a mode with
orientation != 0?

> So, I'm not sure we can just drop it. Moreover, we need at least 2 of
> the orientations, one for AxB resolution and the other for BxA.

How do I switch a windows guest into 600x800?

cheers,
  Gerd

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Alon Levy]

On Thu, Dec 13, 2012 at 03:43:46PM +0100, Gerd Hoffmann wrote:
>   Hi,
> 
> >> That is indeed a better solution, but it does change functionality. I
> >> think it is correct but I'd like to get some other opinions - Uri,
> >> Arnon, Yonit, Soren - any problems with dropping these?
> >>
> > Orientation is used in the Windows Display driver. It is used to set
> > dmDisplayOrientation in DEVMODEW structure
> > (http://msdn.microsoft.com/en-us/library/windows/hardware/ff552837(v=vs.85).aspx).
> 
> What is supposed to happen in case the guest picks a mode with
> orientation != 0?
> 
> > So, I'm not sure we can just drop it. Moreover, we need at least 2 of
> > the orientations, one for AxB resolution and the other for BxA.
> 
> How do I switch a windows guest into 600x800?

AFAIR it is just another mode that appears in the mode list, i.e. when
enumerating the modes via the windows API or via the GUI.

> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH] qxl: use correct rom size for revision < 4

[Gerd Hoffmann]

On 12/23/12 21:33, Alon Levy wrote:
> On Thu, Dec 13, 2012 at 03:43:46PM +0100, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> That is indeed a better solution, but it does change functionality. I
>>>> think it is correct but I'd like to get some other opinions - Uri,
>>>> Arnon, Yonit, Soren - any problems with dropping these?
>>>>
>>> Orientation is used in the Windows Display driver. It is used to set
>>> dmDisplayOrientation in DEVMODEW structure
>>> (http://msdn.microsoft.com/en-us/library/windows/hardware/ff552837(v=vs.85).aspx).
>>
>> What is supposed to happen in case the guest picks a mode with
>> orientation != 0?
>>
>>> So, I'm not sure we can just drop it. Moreover, we need at least 2 of
>>> the orientations, one for AxB resolution and the other for BxA.
>>
>> How do I switch a windows guest into 600x800?
> 
> AFAIR it is just another mode that appears in the mode list, i.e. when
> enumerating the modes via the windows API or via the GUI.

That doesn't answer the questions

How do I switch winxp (or any other guest) into 600x800, with
orientation == 1 (or 3)?  Display Properties offer 800x600 only.

What is supposed to happen with orientation != 0?  orientation isn't
used anywhere in qxl.  I'm pretty sure spice client doesn't know the
windows guest uses a orientation != 0 and thus will not display the
screen correctly.

cheers,
  Gerd

[Qemu-devel] [PATCH 1/2] qxl: stop using non revision 4 rom fields for revision < 4

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 11 +++++++++++
 trace-events |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 3f835b8..8611ee9 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -981,6 +981,12 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
+    if (qxl->revision < 4) {
+        trace_qxl_set_client_capabilities_unsupported_by_revision(qxl->id,
+                                                              qxl->revision);
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE) ||
         runstate_check(RUN_STATE_POSTMIGRATE)) {
         return;
@@ -1013,6 +1019,11 @@ static int interface_client_monitors_config(QXLInstance *sin,
     QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
     int i;
 
+    if (qxl->revision < 4) {
+        trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
+                                                               qxl->revision);
+        return 0;
+    }
     /*
      * Older windows drivers set int_mask to 0 when their ISR is called,
      * then later set it to ~0. So it doesn't relate to the actual interrupts
diff --git a/trace-events b/trace-events
index 6c6cbf1..7d9d62d 100644
--- a/trace-events
+++ b/trace-events
@@ -1006,8 +1006,10 @@ qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
 qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
 qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.8.0.1

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

[Alon Levy]

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

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 8611ee9..99b354a 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -88,9 +88,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),
-- 
1.8.0.1

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

[Gerd Hoffmann]

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.

cheers,
  Gerd

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

[Alon Levy]

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
> 

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

[Alon Levy]

On Sun, Dec 23, 2012 at 10:31:38PM +0200, Alon Levy wrote:
> 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?

Ping?

> 
> > 
> > cheers,
> >   Gerd
> > 
> 

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

[Gerd Hoffmann]

  Hi,

>>>>  #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?
> 
> Ping?

I think I've answered this one already.

It is still not clear to me how this orientation thing is supposed to
work and what exactly we loose when we drop those modes from the list.

I havn't found a way to activate a portrait mode (orientation == 1,3) in
the windows  guest.

Orientation isn't used anywhere in qxl (other than rom initialization),
so in case the guest activates a mode with orientation != 0 spice server
(and thus spice client) isn't notified about that.  So spice client
wouldn't rotate the display according to the guests orientation.  Hmm?

And finally only prehistorical 0.4.x spice guests (which use SET_MODE)
can pick modes with orientation != 0.  The QXLSurfaceCreate struct has
no orientation field ...

cheers,
  Gerd

[Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Alon Levy]

Regarding orientation setting in windows 7 64 guest:
Desktop, right click->Screen resolution
 - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
 - You can choose Resolution
 - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)

There are two changes after applying the "change rom size to 8192" patch:
 - there is no longer an Orientation option
 - the modes listed under "List All Modes" reduce as expected

Changes to the second patch:
 - no orientations except the normal
 - hard code 8192 bytes rom size
 - assert if the required size is larger

Alon Levy (2):
  qxl: stop using non revision 4 rom fields for revision < 4
  qxl: change rom size to 8192

 hw/qxl.c     | 25 ++++++++++++++++++-------
 trace-events |  2 ++
 2 files changed, 20 insertions(+), 7 deletions(-)

-- 
1.8.0.1

[Qemu-devel] [PATCH v2 1/2] qxl: stop using non revision 4 rom fields for revision < 4

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 11 +++++++++++
 trace-events |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 9dc44b9..0d81816 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -945,6 +945,12 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
+    if (qxl->revision < 4) {
+        trace_qxl_set_client_capabilities_unsupported_by_revision(qxl->id,
+                                                              qxl->revision);
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE) ||
         runstate_check(RUN_STATE_POSTMIGRATE)) {
         return;
@@ -979,6 +985,11 @@ static int interface_client_monitors_config(QXLInstance *sin,
     QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
     int i;
 
+    if (qxl->revision < 4) {
+        trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
+                                                               qxl->revision);
+        return 0;
+    }
     /*
      * Older windows drivers set int_mask to 0 when their ISR is called,
      * then later set it to ~0. So it doesn't relate to the actual interrupts
diff --git a/trace-events b/trace-events
index 6eabbac..bf7c5b7 100644
--- a/trace-events
+++ b/trace-events
@@ -1022,8 +1022,10 @@ qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
 qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
 qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.8.0.1

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

[Alon Levy]

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.

Added assert so that rom size will fit the future QXLRom increases via
spice-protocol changes.

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index 0d81816..0cd854a 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -79,10 +79,7 @@
     QXL_MODE(x_res, y_res, 32, orientation)
 
 #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, 0)
 
 static QXLMode qxl_modes[] = {
     QXL_MODE_EX(640, 480),
@@ -306,10 +303,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;
 }
 
-- 
1.8.0.1

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Gerd Hoffmann]

On 01/16/13 18:59, Alon Levy wrote:
> Regarding orientation setting in windows 7 64 guest:
> Desktop, right click->Screen resolution
>  - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
>  - You can choose Resolution
>  - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)

Ah, ok.  The driver seems to handle portrait and swap x+y when creating
a displaysurface.  At least I get a 600x800 display upright.

I can't see a difference between Landscape + Landscape (flipped).
Likewise Portrait + Portrait (flipped).  Is there any?

> There are two changes after applying the "change rom size to 8192" patch:
>  - there is no longer an Orientation option
>  - the modes listed under "List All Modes" reduce as expected

Ok, so we loose the Portrait mode.

> Changes to the second patch:
>  - no orientations except the normal

Keeping orientation 0+1 (and dropping the flipped 2+3 versions) should
make the mode list small enougth that it fits while maintaining support
for the portrait mode.

I think it would also be good to fix the driver to ignore everything with or

How about that?

>  - hard code 8192 bytes rom size
>  - assert if the required size is larger

Good.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Alon Levy]

----- Original Message -----
> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape
> >  (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the
> >  bottom, you get all the modes (i.e. four of each resolution, one
> >  for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when
> creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?
> 
> > There are two changes after applying the "change rom size to 8192"
> > patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions)
> should
> make the mode list small enougth that it fits while maintaining
> support
> for the portrait mode.

I'll test if this changes anything for a windows guest & linux guest.

> 
> I think it would also be good to fix the driver to ignore everything
> with or

... what was the end of that sentence?

> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
> 
> 

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Gerd Hoffmann]

  Hi,

>> I think it would also be good to fix the driver to ignore everything
>> with or

> ... what was the end of that sentence?

.. orientation != 0, then registers every mode with the orientations it
wants, so orientation becomes unused with newer drivers (and we keep
orientation=0,1 for old driver compatibility).

But maybe this isn't worth the trouble.

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Alon Levy]

On Thu, Jan 17, 2013 at 02:02:26PM +0100, Gerd Hoffmann wrote:
> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the bottom, you get all the modes (i.e. four of each resolution, one for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?

I can't actually get the "(flipped)" modes (both portrait and landscape)
to work, I get an error message "Unable to save display settings". How
did you manage to get them to work? which driver, qemu command line,
qemu version did you use?

> 
> > There are two changes after applying the "change rom size to 8192" patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions) should
> make the mode list small enougth that it fits while maintaining support
> for the portrait mode.

That's what I'm going to send.

> 
> I think it would also be good to fix the driver to ignore everything with or
> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
> 

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Gerd Hoffmann]

  Hi,

>> I can't see a difference between Landscape + Landscape (flipped).
>> Likewise Portrait + Portrait (flipped).  Is there any?
> 
> I can't actually get the "(flipped)" modes (both portrait and landscape)
> to work, I get an error message "Unable to save display settings". How
> did you manage to get them to work? which driver, qemu command line,
> qemu version did you use?

upstream qemu, qxl.rev set to 3, qxl driver 4.5.something (i.e. not the
latest 5.x).

cheers,
  Gerd

Re: [Qemu-devel] [PATCH v2 0/2] fix two revision related errors

[Alon Levy]

> On 01/16/13 18:59, Alon Levy wrote:
> > Regarding orientation setting in windows 7 64 guest:
> > Desktop, right click->Screen resolution
> >  - You can choose Orientation: Landscape, Portrait, Landscape
> >  (flipped), Portrait (flipped)
> >  - You can choose Resolution
> >  - You can click "Advanced Settings", then "List All Modes" at the
> >  bottom, you get all the modes (i.e. four of each resolution, one
> >  for each orientation)
> 
> Ah, ok.  The driver seems to handle portrait and swap x+y when
> creating
> a displaysurface.  At least I get a 600x800 display upright.
> 
> I can't see a difference between Landscape + Landscape (flipped).
> Likewise Portrait + Portrait (flipped).  Is there any?

I couldn't see any visible change when using 6.1.0.1015, so I still have no idea. I'm sure it was supposed to flip upside down. Perhaps the driver doesn't support it and the gui doesn't acknowledge it.

> 
> > There are two changes after applying the "change rom size to 8192"
> > patch:
> >  - there is no longer an Orientation option
> >  - the modes listed under "List All Modes" reduce as expected
> 
> Ok, so we loose the Portrait mode.
> 
> > Changes to the second patch:
> >  - no orientations except the normal
> 
> Keeping orientation 0+1 (and dropping the flipped 2+3 versions)
> should
> make the mode list small enougth that it fits while maintaining
> support
> for the portrait mode.

Sending a patch with this change.

> 
> I think it would also be good to fix the driver to ignore everything
> with or
> 
> How about that?
> 
> >  - hard code 8192 bytes rom size
> >  - assert if the required size is larger
> 
> Good.
> 
> cheers,
>   Gerd
> 
> 
> 

[Qemu-devel] [PATCH v3 1/2] qxl: stop using non revision 4 rom fields for revision < 4

[Alon Levy]

Signed-off-by: Alon Levy <alevy@redhat.com>
---
 hw/qxl.c     | 11 +++++++++++
 trace-events |  2 ++
 2 files changed, 13 insertions(+)

diff --git a/hw/qxl.c b/hw/qxl.c
index 9dc44b9..0d81816 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -945,6 +945,12 @@ static void interface_set_client_capabilities(QXLInstance *sin,
 {
     PCIQXLDevice *qxl = container_of(sin, PCIQXLDevice, ssd.qxl);
 
+    if (qxl->revision < 4) {
+        trace_qxl_set_client_capabilities_unsupported_by_revision(qxl->id,
+                                                              qxl->revision);
+        return;
+    }
+
     if (runstate_check(RUN_STATE_INMIGRATE) ||
         runstate_check(RUN_STATE_POSTMIGRATE)) {
         return;
@@ -979,6 +985,11 @@ static int interface_client_monitors_config(QXLInstance *sin,
     QXLRom *rom = memory_region_get_ram_ptr(&qxl->rom_bar);
     int i;
 
+    if (qxl->revision < 4) {
+        trace_qxl_client_monitors_config_unsupported_by_device(qxl->id,
+                                                               qxl->revision);
+        return 0;
+    }
     /*
      * Older windows drivers set int_mask to 0 when their ISR is called,
      * then later set it to ~0. So it doesn't relate to the actual interrupts
diff --git a/trace-events b/trace-events
index 7de9106..09091e6 100644
--- a/trace-events
+++ b/trace-events
@@ -1029,8 +1029,10 @@ qxl_send_events_vm_stopped(int qid, uint32_t events) "%d %d"
 qxl_set_guest_bug(int qid) "%d"
 qxl_interrupt_client_monitors_config(int qid, int num_heads, void *heads) "%d %d %p"
 qxl_client_monitors_config_unsupported_by_guest(int qid, uint32_t int_mask, void *client_monitors_config) "%d %X %p"
+qxl_client_monitors_config_unsupported_by_device(int qid, int revision) "%d revision=%d"
 qxl_client_monitors_config_capped(int qid, int requested, int limit) "%d %d %d"
 qxl_client_monitors_config_crc(int qid, unsigned size, uint32_t crc32) "%d %u %u"
+qxl_set_client_capabilities_unsupported_by_revision(int qid, int revision) "%d revision=%d"
 
 # hw/qxl-render.c
 qxl_render_blit_guest_primary_initialized(void) ""
-- 
1.8.0.1

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

[Alon Levy]

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 <alevy@redhat.com>
---
 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;
 }
 
-- 
1.8.0.1