[Spice-devel] qxl-win important issues

Submitted by Yonit Halperin on Aug. 18, 2011, 7:16 p.m.

Details

Message ID 4E4D028A.5010001@redhat.com
State New, archived
Headers show

Not browsing as part of any series.

Commit Message

Yonit Halperin Aug. 18, 2011, 7:16 p.m.
Hi,
In the last days I encountered several BSODs in qxl-win, and I started 
investigating them. I have reached some conclusions, and learnt some new 
stuff that I think is important if you wish to make changes in the driver.

The first BSOD resulted when I worked with a dual monitor vm, and 
diabled and enabled the second monitor and changed its resolution at the 
same time. see RHBZ #722954 
(https://bugzilla.redhat.com/show_bug.cgi?id=722954). I attached a patch 
for this bug, and it includes a detailed explanation

The second BSOD (or sometimes a crash) is described in RHBZ #731644 
(https://bugzilla.redhat.com/show_bug.cgi?id=731644).
The key for this bug is understanding that while the miniport is loaded 
to the kernel system memory, the display driver is loades into session 
space. from http://www.codeproject.com/KB/system/driverdev6asp.aspx:

"Session space is the kernel equivalent of process isolation. In user 
mode processes have their own virtual memory address space and in the 
kernel sessions have their own virtual memory address space. System 
space is the kernel memory which is global to all sessions.

A session is an instance of a logged on user which contains its own 
Window Manager, Desktop(s), shell and applications. This is most notable 
in Windows XP "Fast User Switching" in which you can log multiple users 
onto a single machine. Each user is actually in a unique session with a 
unique range of kernel memory known as session space."

This means that the display driver can be loaded several time 
simultaneously, and that global variables are not shared between 
different sessions (!!!important!!!).

The problem is, qxl does use globals :(
Here is how the BSOD/crash occurred:
In rev2, objects (surfaces and other devram objects) stayed alive in the 
pci ram after DrvAssertMode(Disable) was called. Then, when another 
session started, the driver had newly initiated mspace, but an old 
release ring (with objects from the older session's mspace) => crash

In rev3 and rev2: sometimes DrvDisableDriver was called for the older 
driver session, and it performed Reset to all devices, while the other 
session has already started and was valid. Reset shouldn’t be performed 
there (actually, this code was added in order to solve the multiple 
session problem, but worked only with one monitor).

Assuming (need to check) that DrvAssertMode(disable) is called before 
moving to another session, I think that if we make sure we clear all 
memory also for rev2, this crash won't happen. We should also move the 
surfaces_info data structure to pdev (and also the caches).
The other problem left is all the other globals we use, mostly 
semaphores which according to 
http://msdn.microsoft.com/en-us/library/ff568281%28v=vs.85%29.aspx, we 
definitely don't implement correctly, and we also need to reconsider if 
all of them are really needed.

Cheers,
Yonit.

Patch hide | download patch | download mbox

From 6f5cf2dbcc876c82db5cd870ef21104b7b83c838 Mon Sep 17 00:00:00 2001
From: Yonit Halperin <yhalperi@redhat.com>
Date: Wed, 17 Aug 2011 14:48:39 +0300
Subject: [PATCH] miniport: map vram to virtual address only once, #722954

Drv<Enable/Disbale>Surface calls IOCTL_VIDEO_MAP/UNMAP_VIDEO_MEMORY respectively.
These calls make the miniport map/unmap the vram to/from system space.
The BSOD occurred since with qxl revision 2, the driver assumes that
as long as it is loaded the vram stays (and all the other mem slots) valid
and in the same place. However, the miniport's call to VideoPortMapMemory
(in IOCTL_VIDEO_MAP_VIDEO_MEMORY) is not guaranteed to map the vram to the same
virtual address. As a result, when the driver accessed to an mspace instance
that was initialized with a no longer valid virtual address, we got BSOD.

Since we don't change the size of the vram, I see no point in mapping and
unmapping it more then once.

For qxl revision 3 this problem is not relevant since between DrvAssertMode(disable)
to DrvAssertMode(enable) we clear the pci ram and initialize all
the data structure that are related to it (mspace, caches, etc.)

I didn't remove the calls to IOCTL_VIDEO_MAP/UNMAP_VIDEO_MEMORY
since I think they would be useful for supporting dynamically changing the
size of the memslots according to the current resolution. Which I think is the
real purpose of IOCTL_VIDEO_MAP/UNMAP_VIDEO_MEMORY. Today, the size
of the frame buffer (surface0) is static -> the maximal resolution we support.
---
 miniport/qxl.c |   75 ++++++++++++++++++++++---------------------------------
 1 files changed, 30 insertions(+), 45 deletions(-)

diff --git a/miniport/qxl.c b/miniport/qxl.c
index 40e8379..7006adf 100644
--- a/miniport/qxl.c
+++ b/miniport/qxl.c
@@ -324,6 +324,11 @@  VP_STATUS InitVRAM(QXLExtension *dev, PVIDEO_ACCESS_RANGE range);
 
 VP_STATUS InitVRAM(QXLExtension *dev, PVIDEO_ACCESS_RANGE range)
 {
+    UINT8 *vram_addr = NULL;
+    ULONG vram_mapped_size = range->RangeLength;
+    ULONG io_space = VIDEO_MEMORY_SPACE_MEMORY;
+    VP_STATUS error;
+
     PAGED_CODE();
     DEBUG_PRINT((dev, 0, "%s\n", __FUNCTION__));
 
@@ -332,10 +337,24 @@  VP_STATUS InitVRAM(QXLExtension *dev, PVIDEO_ACCESS_RANGE range)
         return ERROR_INVALID_DATA;
     }
 
+    if ((error = VideoPortMapMemory(dev, range->RangeStart,
+                                    &vram_mapped_size,
+                                    &io_space,
+                                    &vram_addr))) {
+        DEBUG_PRINT((dev, 0, "%s: map vram failed\n",  __FUNCTION__));
+        return error;
+    }
+
+    if (vram_mapped_size < range->RangeLength) {
+        DEBUG_PRINT((dev, 0, "%s: vram shrinked\n", __FUNCTION__));
+        VideoPortUnmapMemory(dev, vram_addr, NULL);
+        return ERROR_NOT_ENOUGH_MEMORY;
+    }
     dev->vram_physical = range->RangeStart;
+    dev->vram_start = vram_addr;
     dev->vram_size = range->RangeLength;
-    DEBUG_PRINT((dev, 0, "%s: OK, vram 0x%lx size %lu\n",
-                 __FUNCTION__, (ULONG)range->RangeStart.QuadPart, range->RangeLength));
+    DEBUG_PRINT((dev, 0, "%s: OK, vram 0x%lx size %lu vaddr 0x%lx\n", __FUNCTION__,
+                (ULONG)range->RangeStart.QuadPart, range->RangeLength, dev->vram_start));
     return NO_ERROR;
 }
 
@@ -577,6 +596,10 @@  void DevExternsionCleanup(QXLExtension *dev)
         VideoPortUnmapMemory(dev, dev->ram_start, NULL);
     }
 
+    if (dev->vram_start) {
+        VideoPortUnmapMemory(dev, dev->vram_start, NULL);
+    }
+
     if (dev->modes) {
         VideoPortFreePool(dev, dev->modes);
     }
@@ -966,7 +989,6 @@  BOOLEAN StartIO(PVOID dev_extension, PVIDEO_REQUEST_PACKET packet)
         break;
     case IOCTL_VIDEO_MAP_VIDEO_MEMORY: {
             PVIDEO_MEMORY_INFORMATION mem_info;
-            ULONG fb_io_space;
 
             DEBUG_PRINT((dev_ext, 0, "%s: IOCTL_VIDEO_MAP_VIDEO_MEMORY\n", __FUNCTION__));
 
@@ -976,36 +998,11 @@  BOOLEAN StartIO(PVOID dev_extension, PVIDEO_REQUEST_PACKET packet)
                 error = ERROR_INSUFFICIENT_BUFFER;
                 goto err;
             }
-            mem_info = packet->OutputBuffer;
 
-            mem_info->VideoRamBase =
-                                    ((PVIDEO_MEMORY)(packet->InputBuffer))->RequestedVirtualAddress;
-            ASSERT(mem_info->VideoRamBase == NULL);
-            mem_info->VideoRamLength = dev_ext->vram_size;
-            fb_io_space = VIDEO_MEMORY_SPACE_MEMORY;
-
-            if ((error = VideoPortMapMemory(dev_ext, dev_ext->vram_physical,
-                                            &mem_info->VideoRamLength,
-                                            &fb_io_space, &mem_info->VideoRamBase)) != NO_ERROR) {
-                DEBUG_PRINT((dev_ext, 0, "%s: map filed\n", __FUNCTION__));
-                goto err;
-            }
-            dev_ext->vram_start = mem_info->VideoRamBase;
-            DEBUG_PRINT((dev_ext, 0, "%s: vram size %lu ret size %lu fb vaddr 0x%lx\n",
-                         __FUNCTION__,
-                         dev_ext->vram_size,
-                         mem_info->VideoRamLength,
-                         mem_info->VideoRamBase));
-            if (mem_info->VideoRamLength < dev_ext->vram_size) {
-                DEBUG_PRINT((dev_ext, 0, "%s: fb shrink\n", __FUNCTION__));
-                VideoPortUnmapMemory(dev_ext, mem_info->VideoRamBase, NULL);
-                mem_info->VideoRamBase = NULL;
-                mem_info->VideoRamLength = 0;
-                error = ERROR_NOT_ENOUGH_MEMORY;
-                goto err;
-            }
-            mem_info->FrameBufferBase = mem_info->VideoRamBase;
-            mem_info->FrameBufferLength = mem_info->VideoRamLength;
+            ASSERT(((PVIDEO_MEMORY)(packet->InputBuffer))->RequestedVirtualAddress == NULL);
+            mem_info = packet->OutputBuffer;
+            mem_info->VideoRamBase = mem_info->FrameBufferBase = dev_ext->vram_start;
+            mem_info->VideoRamLength = mem_info->FrameBufferLength = dev_ext->vram_size;
 #if 0
 #ifdef DBG
             DEBUG_PRINT((dev, 0, "%s: zap\n", __FUNCTION__));
@@ -1016,19 +1013,7 @@  BOOLEAN StartIO(PVOID dev_extension, PVIDEO_REQUEST_PACKET packet)
         }
         break;
     case IOCTL_VIDEO_UNMAP_VIDEO_MEMORY: {
-            PVOID addr;
-
-            DEBUG_PRINT((dev_ext, 0, "%s: IOCTL_VIDEO_UNMAP_VIDEO_MEMORY\n", __FUNCTION__));
-
-            if (packet->InputBufferLength < sizeof(VIDEO_MEMORY)) {
-                error = ERROR_INSUFFICIENT_BUFFER;
-                goto err;
-            }
-            addr = ((PVIDEO_MEMORY)(packet->InputBuffer))->RequestedVirtualAddress;
-            if ((error = VideoPortUnmapMemory(dev_ext, addr, NULL)) != NO_ERROR) {
-                DEBUG_PRINT((dev_ext, 0, "%s: unmap failed\n", __FUNCTION__));
-            }
-            dev_ext->vram_start = NULL;
+            DEBUG_PRINT((dev_ext, 0, "%s: IOCTL_VIDEO_UNMAP_VIDEO_MEMORY (do nothing) \n", __FUNCTION__));
         }
         break;
     case IOCTL_VIDEO_RESET_DEVICE:
-- 
1.7.4.4