| Message ID | 1354808473-7634-1-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
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
> 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 > >
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 >> >> >
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
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 > >
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
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) ""
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(-)