Patchwork qxl: use correct rom size for revision < 4

login
register
mail settings
Submitter Alon Levy
Date Dec. 6, 2012, 3:41 p.m.
Message ID <1354808473-7634-1-git-send-email-alevy@redhat.com>
Download mbox | patch
Permalink /patch/204283/
State New
Headers show

Comments

Alon Levy - Dec. 6, 2012, 3:41 p.m.
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(-)
Gerd Hoffmann - Dec. 12, 2012, 11:46 a.m.
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
Alon Levy - Dec. 13, 2012, 10:40 a.m.
> 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
> 
>
Yonit Halperin - Dec. 13, 2012, 2:30 p.m.
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
>>
>>
>
Gerd Hoffmann - Dec. 13, 2012, 2:43 p.m.
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
Alon Levy - Dec. 23, 2012, 8:33 p.m.
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
> 
>
Gerd Hoffmann - Jan. 3, 2013, 7:55 a.m.
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

Patch

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