[Spice-devel,v2,08/12] qxl-wddm-dod: Prepare for failure to allocate memory

Submitted by Yuri Benditovich on April 1, 2017, 4:40 p.m.

Details

Message ID 1491064839-13612-9-git-send-email-yuri.benditovich@daynix.com
State New
Headers show
Series "Set of patches for further support of VSync" ( rev: 2 ) in Spice

Not browsing as part of any series.

Commit Message

Yuri Benditovich April 1, 2017, 4:40 p.m.
From: "yuri.benditovich@daynix.com" <yuri.benditovich@daynix.com>

Preparation for further scenarios when memory allocation failure
can happen and we'll need to use fallback when possible.
Memory allocation can fail in following cases:
- when non-forced memory allocation used and the attempt to allocate
  the memory must be as fast as possible, without waits
- when forced memory allocation used, but the driver already received
  stop command and waits for thread termination. Note that in case
  the VSync control is enabled stop command may happen even after the video
  subsystem executes switch to VGA mode. In such case QEMU will not return
  previously allocated objects (assuming the QXL driver already disabled
  and ignoring callbacks from Spice server in VGA mode).

In case of forced memory allocation the allocation routine waits
unpredictable time until the request is satisfied. In current commit
we do not acquire m_MemLock mutex for all this time, but release it
when entering long wait to allow another caller to try allocating memory.

Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
---
 qxldod/QxlDod.cpp | 86 +++++++++++++++++++++++++++++++++++++++++++++++++------
 qxldod/QxlDod.h   |  3 +-
 2 files changed, 79 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
index aa70f39..f8440d4 100755
--- a/qxldod/QxlDod.cpp
+++ b/qxldod/QxlDod.cpp
@@ -3064,6 +3064,7 @@  QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
     m_FreeOutputs = 0;
     m_Pending = 0;
     m_PresentThread = NULL;
+    m_bActive = FALSE;
 }
 
 QxlDevice::~QxlDevice(void)
@@ -3490,14 +3491,19 @@  NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION* pDispInfo)
     Status = AcquireDisplayInfo(*(pDispInfo));
     if (NT_SUCCESS(Status))
     {
+        m_bActive = TRUE;
         Status = StartPresentThread();
     }
+    if (!NT_SUCCESS(Status)) {
+        m_bActive = FALSE;
+    }
     return Status;
 }
 
 void QxlDevice::QxlClose()
 {
     PAGED_CODE();
+    m_bActive = FALSE;
     StopPresentThread();
     DestroyMemSlots();
 }
@@ -3945,14 +3951,18 @@  void QxlDevice::WaitForReleaseRing(void)
 {
     PAGED_CODE();
     int wait;
+    BOOLEAN locked;
 
     DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
 
+    locked = WaitForObject(&m_MemLock, NULL);
     for (;;) {
         LARGE_INTEGER timeout;
 
         if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
+            ReleaseMutex(&m_MemLock, locked);
             QXL_SLEEP(10);
+            locked = WaitForObject(&m_MemLock, NULL);
             if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
                 break;
             }
@@ -3960,17 +3970,20 @@  void QxlDevice::WaitForReleaseRing(void)
         }
         SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
 
-        if (!wait) {
+        if (!wait || !m_bActive) {
             break;
         }
 
+        ReleaseMutex(&m_MemLock, locked);
         timeout.QuadPart = -30 * 1000 * 10; //30ms
         WaitForObject(&m_DisplayEvent, &timeout);
+        locked = WaitForObject(&m_MemLock, NULL);
 
         if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
             SyncIo(QXL_IO_NOTIFY_OOM, 0);
         }
     }
+    ReleaseMutex(&m_MemLock, locked);
     DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
 }
 
@@ -4052,7 +4065,16 @@  void *QxlDevice::AllocMem(UINT32 mspace_type, size_t size, BOOL force)
      mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
 #endif
 
-    locked = WaitForObject(&m_MemLock, NULL);
+    if (force)
+        locked = WaitForObject(&m_MemLock, NULL);
+    else {
+        LARGE_INTEGER doNotWait;
+        doNotWait.QuadPart = 0;
+        locked = WaitForObject(&m_MemLock, &doNotWait);
+        if (!locked) {
+             return NULL;
+        }
+    }
 
     while (1) {
         /* Release lots of queued resources, before allocating, as we
@@ -4070,17 +4092,20 @@  void *QxlDevice::AllocMem(UINT32 mspace_type, size_t size, BOOL force)
             continue;
         }
 
-        if (force) {
+        if (force && m_bActive) {
             /* Ask spice to free some stuff */
+            ReleaseMutex(&m_MemLock, locked);
             WaitForReleaseRing();
+            locked = WaitForObject(&m_MemLock, NULL);
         } else {
             /* Fail */
             break;
         }
     }
+
     ReleaseMutex(&m_MemLock, locked);
 
-    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
+    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
                                       ptr < m_MSInfo[mspace_type].mspace_end));
     DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__, ptr));
     return ptr;
@@ -4113,6 +4138,9 @@  QXLDrawable *QxlDevice::GetDrawable()
     QXLOutput *output;
 
     output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) + sizeof(QXLDrawable), TRUE);
+    if (!output) {
+        return NULL;
+    }
     output->num_res = 0;
     RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
     ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
@@ -4128,6 +4156,9 @@  QXLCursorCmd *QxlDevice::CursorCmd()
 
     DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
     output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) + sizeof(QXLCursorCmd), TRUE);
+    if (!output) {
+        return NULL;
+    }
     output->num_res = 0;
     RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
     cursor_cmd = (QXLCursorCmd *)output->data;
@@ -4277,6 +4308,9 @@  QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT *area, CONST RECT *clip,
     DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
 
     drawable = GetDrawable();
+    if (!drawable) {
+        return NULL;
+    }
     drawable->surface_id = surface_id;
     drawable->type = type;
     drawable->effect = QXL_EFFECT_OPAQUE;
@@ -4420,6 +4454,11 @@  QXLDrawable *QxlDevice::PrepareBltBits (
     alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX % line_size;
     alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
     image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
+    if (!image_res) {
+        ReleaseOutput(drawable->release_info.id);
+        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for drawable\n"));
+        return NULL;
+    }
 
     image_res->refs = 1;
     image_res->free = FreeBitmapImageEx;
@@ -4450,8 +4489,12 @@  QXLDrawable *QxlDevice::PrepareBltBits (
     alloc_size = height * line_size;
 
     for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
-        PutBytesAlign(&chunk, &dest, &dest_end, src,
-                      line_size, alloc_size, line_size);
+        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
+            line_size, alloc_size, line_size)) {
+            ReleaseOutput(drawable->release_info.id);
+            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap for drawable\n"));
+            return NULL;
+        }
     }
 
     internal->image.bitmap.palette = 0;
@@ -4472,7 +4515,7 @@  QXLDrawable *QxlDevice::PrepareBltBits (
     return drawable;
 }
 
-VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
+BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
                             UINT8 **end_ptr, UINT8 *src, int size,
                             size_t alloc_size, uint32_t alignment)
 {
@@ -4490,6 +4533,9 @@  VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
             aligned_size -=  aligned_size % alignment;
 
             void *ptr = AllocMem(MSPACE_TYPE_VRAM, size + sizeof(QXLDataChunk), TRUE);
+            if (!ptr) {
+                return FALSE;
+            }
             chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
             ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
             chunk = (QXLDataChunk *)ptr;
@@ -4510,6 +4556,7 @@  VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
     *now_ptr = now;
     *end_ptr = end;
     DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
+    return TRUE;
 }
 
 VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
@@ -4573,6 +4620,11 @@  NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSetPoi
     int num_images = 1;
 
     cursor_cmd = CursorCmd();
+    if (!cursor_cmd) {
+        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor command\n", __FUNCTION__));
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
+
     cursor_cmd->type = QXL_CURSOR_SET;
 
     cursor_cmd->u.set.visible = TRUE;
@@ -4580,6 +4632,12 @@  NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSetPoi
     cursor_cmd->u.set.position.y = 0;
 
     res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
+    if (!res) {
+        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n", __FUNCTION__));
+        ReleaseOutput(cursor_cmd->release_info.id);
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
+
     res->refs = 1;
     res->free = FreeCursorEx;
     res->ptr = this;
@@ -4616,8 +4674,13 @@  NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST DXGKARG_SETPOINTERSHAPE* pSetPoi
     end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
     src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height * num_images);
     for (; src != src_end; src += pSetPointerShape->Pitch) {
-        PutBytesAlign(&chunk, &now, &end, src, line_size,
-                 PAGE_SIZE, 1);
+        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
+                 PAGE_SIZE, 1))
+        {
+            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor bitmap\n", __FUNCTION__));
+            ReleaseOutput(cursor_cmd->release_info.id);
+            return STATUS_INSUFFICIENT_RESOURCES;
+        }
     }
     CursorCmdAddRes(cursor_cmd, res);
     RELEASE_RES(res);
@@ -4638,6 +4701,11 @@  NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST DXGKARG_SETPOINTERPOSITION* pS
                                  pSetPointerPosition->X,
                                  pSetPointerPosition->Y));
     QXLCursorCmd *cursor_cmd = CursorCmd();
+    if (!cursor_cmd) {
+        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor command\n", __FUNCTION__));
+        return STATUS_INSUFFICIENT_RESOURCES;
+    }
+
     if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
         cursor_cmd->type = QXL_CURSOR_HIDE;
     } else {
diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
index a1e9634..cbcd11e 100755
--- a/qxldod/QxlDod.h
+++ b/qxldod/QxlDod.h
@@ -601,7 +601,7 @@  private:
     void PushCmd(void);
     void WaitForCursorRing(void);
     void PushCursor(void);
-    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
+    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
                             UINT8 **end_ptr, UINT8 *src, int size,
                             size_t alloc_size, uint32_t alignment);
     void AsyncIo(UCHAR  Port, UCHAR Value);
@@ -671,6 +671,7 @@  private:
 
     QXLPresentOnlyRing m_PresentRing[1];
     HANDLE m_PresentThread;
+    BOOLEAN m_bActive;
 };
 
 class QxlDod {

Comments

> 
> From: "yuri.benditovich@daynix.com" <yuri.benditovich@daynix.com>
> 
> Preparation for further scenarios when memory allocation failure
> can happen and we'll need to use fallback when possible.
> Memory allocation can fail in following cases:
> - when non-forced memory allocation used and the attempt to allocate
>   the memory must be as fast as possible, without waits
> - when forced memory allocation used, but the driver already received
>   stop command and waits for thread termination. Note that in case
>   the VSync control is enabled stop command may happen even after the video
>   subsystem executes switch to VGA mode. In such case QEMU will not return
>   previously allocated objects (assuming the QXL driver already disabled
>   and ignoring callbacks from Spice server in VGA mode).
> 

Just to confirm, by VGA mode you mean when Windows attempt to unload the
driver and use VESA/VGA modes, not VgaDevice, right?
In this case I assume the memory will be freed when the driver is
unloaded by Windows.
I would expect the number of commands sent in this case is small.
Also why is not possible to just ignore the commands sent by Windows
in this case?

> In case of forced memory allocation the allocation routine waits
> unpredictable time until the request is satisfied. In current commit
> we do not acquire m_MemLock mutex for all this time, but release it
> when entering long wait to allow another caller to try allocating memory.
> 

Looking at final code AllocMem can return NULL in all cases depending
on the state of the driver however not all paths check if this function
returns NULL. Specifically InitMonitorConfig and SetClip.
InitMonitorConfig is called just after initializing memory so is
unlikely to fail and SetClip is always called with clip == NULL however
I don't like to have unexploded bombs in the code.

> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  qxldod/QxlDod.cpp | 86
>  +++++++++++++++++++++++++++++++++++++++++++++++++------
>  qxldod/QxlDod.h   |  3 +-
>  2 files changed, 79 insertions(+), 10 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index aa70f39..f8440d4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>      m_FreeOutputs = 0;
>      m_Pending = 0;
>      m_PresentThread = NULL;
> +    m_bActive = FALSE;
>  }
>  
>  QxlDevice::~QxlDevice(void)
> @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> pDispInfo)
>      Status = AcquireDisplayInfo(*(pDispInfo));
>      if (NT_SUCCESS(Status))
>      {
> +        m_bActive = TRUE;
>          Status = StartPresentThread();
>      }
> +    if (!NT_SUCCESS(Status)) {
> +        m_bActive = FALSE;
> +    }
>      return Status;
>  }
>  
>  void QxlDevice::QxlClose()
>  {
>      PAGED_CODE();
> +    m_bActive = FALSE;
>      StopPresentThread();
>      DestroyMemSlots();
>  }
> @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
>  {
>      PAGED_CODE();
>      int wait;
> +    BOOLEAN locked;
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
>  
> +    locked = WaitForObject(&m_MemLock, NULL);
>      for (;;) {
>          LARGE_INTEGER timeout;
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> +            ReleaseMutex(&m_MemLock, locked);
>              QXL_SLEEP(10);
> +            locked = WaitForObject(&m_MemLock, NULL);
>              if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>                  break;
>              }
> @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
>          }
>          SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
>  
> -        if (!wait) {
> +        if (!wait || !m_bActive) {
>              break;
>          }
>  
> +        ReleaseMutex(&m_MemLock, locked);
>          timeout.QuadPart = -30 * 1000 * 10; //30ms
>          WaitForObject(&m_DisplayEvent, &timeout);
> +        locked = WaitForObject(&m_MemLock, NULL);
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>              SyncIo(QXL_IO_NOTIFY_OOM, 0);
>          }
>      }
> +    ReleaseMutex(&m_MemLock, locked);
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
>  }
>  
> @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>       mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
>  #endif
>  
> -    locked = WaitForObject(&m_MemLock, NULL);
> +    if (force)
> +        locked = WaitForObject(&m_MemLock, NULL);
> +    else {
> +        LARGE_INTEGER doNotWait;
> +        doNotWait.QuadPart = 0;
> +        locked = WaitForObject(&m_MemLock, &doNotWait);
> +        if (!locked) {
> +             return NULL;
> +        }
> +    }
>  
>      while (1) {
>          /* Release lots of queued resources, before allocating, as we
> @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>              continue;
>          }
>  
> -        if (force) {
> +        if (force && m_bActive) {
>              /* Ask spice to free some stuff */
> +            ReleaseMutex(&m_MemLock, locked);
>              WaitForReleaseRing();
> +            locked = WaitForObject(&m_MemLock, NULL);
>          } else {
>              /* Fail */
>              break;
>          }
>      }
> +
>      ReleaseMutex(&m_MemLock, locked);
>  
> -    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
> +    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> m_MSInfo[mspace_type].mspace_start &&
>                                        ptr <
>                                        m_MSInfo[mspace_type].mspace_end));
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
>      ptr));
>      return ptr;
> @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
>      QXLOutput *output;
>  
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLDrawable), TRUE);
> +    if (!output) {
> +        return NULL;
> +    }
>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
>      ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
> @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLCursorCmd), TRUE);
> +    if (!output) {
> +        return NULL;
> +    }
>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
>      cursor_cmd = (QXLCursorCmd *)output->data;
> @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT
> *area, CONST RECT *clip,
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>  
>      drawable = GetDrawable();
> +    if (!drawable) {
> +        return NULL;
> +    }
>      drawable->surface_id = surface_id;
>      drawable->type = type;
>      drawable->effect = QXL_EFFECT_OPAQUE;
> @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
>      line_size;
>      alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
>      image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
> +    if (!image_res) {
> +        ReleaseOutput(drawable->release_info.id);
> +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> drawable\n"));
> +        return NULL;
> +    }
>  
>      image_res->refs = 1;
>      image_res->free = FreeBitmapImageEx;
> @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      alloc_size = height * line_size;
>  
>      for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
> -        PutBytesAlign(&chunk, &dest, &dest_end, src,
> -                      line_size, alloc_size, line_size);
> +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> +            line_size, alloc_size, line_size)) {
> +            ReleaseOutput(drawable->release_info.id);
> +            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap
> for drawable\n"));
> +            return NULL;
> +        }
>      }
>  
>      internal->image.bitmap.palette = 0;
> @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      return drawable;
>  }
>  
> -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment)
>  {
> @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
>              aligned_size -=  aligned_size % alignment;
>  
>              void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
>              sizeof(QXLDataChunk), TRUE);
> +            if (!ptr) {
> +                return FALSE;
> +            }
>              chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
>              ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
>              chunk = (QXLDataChunk *)ptr;
> @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
>      *now_ptr = now;
>      *end_ptr = end;
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> +    return TRUE;
>  }
>  
>  VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> @@ -4573,6 +4620,11 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      int num_images = 1;
>  
>      cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      cursor_cmd->type = QXL_CURSOR_SET;
>  
>      cursor_cmd->u.set.visible = TRUE;
> @@ -4580,6 +4632,12 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      cursor_cmd->u.set.position.y = 0;
>  
>      res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
> +    if (!res) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",
> __FUNCTION__));
> +        ReleaseOutput(cursor_cmd->release_info.id);
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      res->refs = 1;
>      res->free = FreeCursorEx;
>      res->ptr = this;
> @@ -4616,8 +4674,13 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
>      src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *
>      num_images);
>      for (; src != src_end; src += pSetPointerShape->Pitch) {
> -        PutBytesAlign(&chunk, &now, &end, src, line_size,
> -                 PAGE_SIZE, 1);
> +        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> +                 PAGE_SIZE, 1))
> +        {
> +            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> bitmap\n", __FUNCTION__));
> +            ReleaseOutput(cursor_cmd->release_info.id);
> +            return STATUS_INSUFFICIENT_RESOURCES;
> +        }
>      }
>      CursorCmdAddRes(cursor_cmd, res);
>      RELEASE_RES(res);
> @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> DXGKARG_SETPOINTERPOSITION* pS
>                                   pSetPointerPosition->X,
>                                   pSetPointerPosition->Y));
>      QXLCursorCmd *cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
>          cursor_cmd->type = QXL_CURSOR_HIDE;
>      } else {
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index a1e9634..cbcd11e 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -601,7 +601,7 @@ private:
>      void PushCmd(void);
>      void WaitForCursorRing(void);
>      void PushCursor(void);
> -    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment);
>      void AsyncIo(UCHAR  Port, UCHAR Value);
> @@ -671,6 +671,7 @@ private:
>  
>      QXLPresentOnlyRing m_PresentRing[1];
>      HANDLE m_PresentThread;
> +    BOOLEAN m_bActive;
>  };
>  
>  class QxlDod {
On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio <fziglio@redhat.com> wrote:

> >
> > From: "yuri.benditovich@daynix.com" <yuri.benditovich@daynix.com>
> >
> > Preparation for further scenarios when memory allocation failure
> > can happen and we'll need to use fallback when possible.
> > Memory allocation can fail in following cases:
> > - when non-forced memory allocation used and the attempt to allocate
> >   the memory must be as fast as possible, without waits
> > - when forced memory allocation used, but the driver already received
> >   stop command and waits for thread termination. Note that in case
> >   the VSync control is enabled stop command may happen even after the
> video
> >   subsystem executes switch to VGA mode. In such case QEMU will not
> return
> >   previously allocated objects (assuming the QXL driver already disabled
> >   and ignoring callbacks from Spice server in VGA mode).
> >
>
> Just to confirm, by VGA mode you mean when Windows attempt to unload the
> driver and use VESA/VGA modes, not VgaDevice, right?
> In this case I assume the memory will be freed when the driver is
> unloaded by Windows.
> I would expect the number of commands sent in this case is small.
> Also why is not possible to just ignore the commands sent by Windows
> in this case?
>

When VSync is enabled and the system activates watchdog policy, there are 2
possible flows
related to driver stopping:
First one is regular PnP stop (for example, on disable), on stop the driver
waits until thread
finishes and the thread is able to complete all the operations, in the
worst case it will take
some time until thread allocates all the pending drawables and sends them
down.
Second one is forced stop due to long processing of presentation callback.
We do
our best to avoid this, but this is still possible, at least in theory. In
this case OS
may not not wait until the driver completes stop flow and can activate
fallback
mode and make switch to VGA mode. If the thread is processing outstanding
drawables, it needs to allocate memory (forced) and may pend until some
previously allocated
objects must be reported by QEMU/spice server to the guest. As the switch
to VGA
already happened, QEMU does not report them (assuming the guest does need
this,
which is completely correct in case of regular stop). At the moment of VGA
switch the
thread can be in the middle of allocation, so the driver shall know to
abort the allocation if
stop flow already started.

BTW, In earlier notes you mentioned possibility to use VGA memory as
alternate area for
objects allocation. I think that in stop flow due to watchdog the VGA area
may be used by
fallback video engine as frame buffer and this can corrupt the memory arena
managed by mspace code.


> > In case of forced memory allocation the allocation routine waits
> > unpredictable time until the request is satisfied. In current commit
> > we do not acquire m_MemLock mutex for all this time, but release it
> > when entering long wait to allow another caller to try allocating memory.
> >
>
> Looking at final code AllocMem can return NULL in all cases depending
> on the state of the driver however not all paths check if this function
> returns NULL. Specifically InitMonitorConfig and SetClip.
> InitMonitorConfig is called just after initializing memory so is
> unlikely to fail and SetClip is always called with clip == NULL however
> I don't like to have unexploded bombs in the code.
>
> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> > ---
> >  qxldod/QxlDod.cpp | 86
> >  +++++++++++++++++++++++++++++++++++++++++++++++++------
> >  qxldod/QxlDod.h   |  3 +-
> >  2 files changed, 79 insertions(+), 10 deletions(-)
> >
> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> > index aa70f39..f8440d4 100755
> > --- a/qxldod/QxlDod.cpp
> > +++ b/qxldod/QxlDod.cpp
> > @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
> >      m_FreeOutputs = 0;
> >      m_Pending = 0;
> >      m_PresentThread = NULL;
> > +    m_bActive = FALSE;
> >  }
> >
> >  QxlDevice::~QxlDevice(void)
> > @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLA
> Y_INFORMATION*
> > pDispInfo)
> >      Status = AcquireDisplayInfo(*(pDispInfo));
> >      if (NT_SUCCESS(Status))
> >      {
> > +        m_bActive = TRUE;
> >          Status = StartPresentThread();
> >      }
> > +    if (!NT_SUCCESS(Status)) {
> > +        m_bActive = FALSE;
> > +    }
> >      return Status;
> >  }
> >
> >  void QxlDevice::QxlClose()
> >  {
> >      PAGED_CODE();
> > +    m_bActive = FALSE;
> >      StopPresentThread();
> >      DestroyMemSlots();
> >  }
> > @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
> >  {
> >      PAGED_CODE();
> >      int wait;
> > +    BOOLEAN locked;
> >
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
> >
> > +    locked = WaitForObject(&m_MemLock, NULL);
> >      for (;;) {
> >          LARGE_INTEGER timeout;
> >
> >          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> > +            ReleaseMutex(&m_MemLock, locked);
> >              QXL_SLEEP(10);
> > +            locked = WaitForObject(&m_MemLock, NULL);
> >              if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> >                  break;
> >              }
> > @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
> >          }
> >          SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
> >
> > -        if (!wait) {
> > +        if (!wait || !m_bActive) {
> >              break;
> >          }
> >
> > +        ReleaseMutex(&m_MemLock, locked);
> >          timeout.QuadPart = -30 * 1000 * 10; //30ms
> >          WaitForObject(&m_DisplayEvent, &timeout);
> > +        locked = WaitForObject(&m_MemLock, NULL);
> >
> >          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> >              SyncIo(QXL_IO_NOTIFY_OOM, 0);
> >          }
> >      }
> > +    ReleaseMutex(&m_MemLock, locked);
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
> >  }
> >
> > @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
> size_t
> > size, BOOL force)
> >       mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
> >  #endif
> >
> > -    locked = WaitForObject(&m_MemLock, NULL);
> > +    if (force)
> > +        locked = WaitForObject(&m_MemLock, NULL);
> > +    else {
> > +        LARGE_INTEGER doNotWait;
> > +        doNotWait.QuadPart = 0;
> > +        locked = WaitForObject(&m_MemLock, &doNotWait);
> > +        if (!locked) {
> > +             return NULL;
> > +        }
> > +    }
> >
> >      while (1) {
> >          /* Release lots of queued resources, before allocating, as we
> > @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
> size_t
> > size, BOOL force)
> >              continue;
> >          }
> >
> > -        if (force) {
> > +        if (force && m_bActive) {
> >              /* Ask spice to free some stuff */
> > +            ReleaseMutex(&m_MemLock, locked);
> >              WaitForReleaseRing();
> > +            locked = WaitForObject(&m_MemLock, NULL);
> >          } else {
> >              /* Fail */
> >              break;
> >          }
> >      }
> > +
> >      ReleaseMutex(&m_MemLock, locked);
> >
> > -    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start
> &&
> > +    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> > m_MSInfo[mspace_type].mspace_start &&
> >                                        ptr <
> >                                        m_MSInfo[mspace_type].mspace_e
> nd));
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
> >      ptr));
> >      return ptr;
> > @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
> >      QXLOutput *output;
> >
> >      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> >      sizeof(QXLDrawable), TRUE);
> > +    if (!output) {
> > +        return NULL;
> > +    }
> >      output->num_res = 0;
> >      RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
> >      ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
> > @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
> >
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
> >      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> >      sizeof(QXLCursorCmd), TRUE);
> > +    if (!output) {
> > +        return NULL;
> > +    }
> >      output->num_res = 0;
> >      RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
> >      cursor_cmd = (QXLCursorCmd *)output->data;
> > @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST
> RECT
> > *area, CONST RECT *clip,
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
> >
> >      drawable = GetDrawable();
> > +    if (!drawable) {
> > +        return NULL;
> > +    }
> >      drawable->surface_id = surface_id;
> >      drawable->type = type;
> >      drawable->effect = QXL_EFFECT_OPAQUE;
> > @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> >      alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
> >      line_size;
> >      alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size,
> alloc_size);
> >      image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size,
> TRUE);
> > +    if (!image_res) {
> > +        ReleaseOutput(drawable->release_info.id);
> > +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> > drawable\n"));
> > +        return NULL;
> > +    }
> >
> >      image_res->refs = 1;
> >      image_res->free = FreeBitmapImageEx;
> > @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> >      alloc_size = height * line_size;
> >
> >      for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size)
> {
> > -        PutBytesAlign(&chunk, &dest, &dest_end, src,
> > -                      line_size, alloc_size, line_size);
> > +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> > +            line_size, alloc_size, line_size)) {
> > +            ReleaseOutput(drawable->release_info.id);
> > +            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional
> bitmap
> > for drawable\n"));
> > +            return NULL;
> > +        }
> >      }
> >
> >      internal->image.bitmap.palette = 0;
> > @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> >      return drawable;
> >  }
> >
> > -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8
> **now_ptr,
> > +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8
> **now_ptr,
> >                              UINT8 **end_ptr, UINT8 *src, int size,
> >                              size_t alloc_size, uint32_t alignment)
> >  {
> > @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
> **chunk_ptr,
> > UINT8 **now_ptr,
> >              aligned_size -=  aligned_size % alignment;
> >
> >              void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
> >              sizeof(QXLDataChunk), TRUE);
> > +            if (!ptr) {
> > +                return FALSE;
> > +            }
> >              chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
> >              ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk,
> m_SurfaceMemSlot);
> >              chunk = (QXLDataChunk *)ptr;
> > @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
> **chunk_ptr,
> > UINT8 **now_ptr,
> >      *now_ptr = now;
> >      *end_ptr = end;
> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> > +    return TRUE;
> >  }
> >
> >  VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> > @@ -4573,6 +4620,11 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> > DXGKARG_SETPOINTERSHAPE* pSetPoi
> >      int num_images = 1;
> >
> >      cursor_cmd = CursorCmd();
> > +    if (!cursor_cmd) {
> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> > command\n", __FUNCTION__));
> > +        return STATUS_INSUFFICIENT_RESOURCES;
> > +    }
> > +
> >      cursor_cmd->type = QXL_CURSOR_SET;
> >
> >      cursor_cmd->u.set.visible = TRUE;
> > @@ -4580,6 +4632,12 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> > DXGKARG_SETPOINTERSHAPE* pSetPoi
> >      cursor_cmd->u.set.position.y = 0;
> >
> >      res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE,
> TRUE);
> > +    if (!res) {
> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> data\n",
> > __FUNCTION__));
> > +        ReleaseOutput(cursor_cmd->release_info.id);
> > +        return STATUS_INSUFFICIENT_RESOURCES;
> > +    }
> > +
> >      res->refs = 1;
> >      res->free = FreeCursorEx;
> >      res->ptr = this;
> > @@ -4616,8 +4674,13 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> > DXGKARG_SETPOINTERSHAPE* pSetPoi
> >      end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
> >      src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height
> *
> >      num_images);
> >      for (; src != src_end; src += pSetPointerShape->Pitch) {
> > -        PutBytesAlign(&chunk, &now, &end, src, line_size,
> > -                 PAGE_SIZE, 1);
> > +        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> > +                 PAGE_SIZE, 1))
> > +        {
> > +            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> > bitmap\n", __FUNCTION__));
> > +            ReleaseOutput(cursor_cmd->release_info.id);
> > +            return STATUS_INSUFFICIENT_RESOURCES;
> > +        }
> >      }
> >      CursorCmdAddRes(cursor_cmd, res);
> >      RELEASE_RES(res);
> > @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> > DXGKARG_SETPOINTERPOSITION* pS
> >                                   pSetPointerPosition->X,
> >                                   pSetPointerPosition->Y));
> >      QXLCursorCmd *cursor_cmd = CursorCmd();
> > +    if (!cursor_cmd) {
> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> > command\n", __FUNCTION__));
> > +        return STATUS_INSUFFICIENT_RESOURCES;
> > +    }
> > +
> >      if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible)
> {
> >          cursor_cmd->type = QXL_CURSOR_HIDE;
> >      } else {
> > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> > index a1e9634..cbcd11e 100755
> > --- a/qxldod/QxlDod.h
> > +++ b/qxldod/QxlDod.h
> > @@ -601,7 +601,7 @@ private:
> >      void PushCmd(void);
> >      void WaitForCursorRing(void);
> >      void PushCursor(void);
> > -    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> > +    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> >                              UINT8 **end_ptr, UINT8 *src, int size,
> >                              size_t alloc_size, uint32_t alignment);
> >      void AsyncIo(UCHAR  Port, UCHAR Value);
> > @@ -671,6 +671,7 @@ private:
> >
> >      QXLPresentOnlyRing m_PresentRing[1];
> >      HANDLE m_PresentThread;
> > +    BOOLEAN m_bActive;
> >  };
> >
> >  class QxlDod {
>
> On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio < fziglio@redhat.com > wrote:

> > >
> 
> > > From: " yuri.benditovich@daynix.com " < yuri.benditovich@daynix.com >
> 
> > >
> 
> > > Preparation for further scenarios when memory allocation failure
> 
> > > can happen and we'll need to use fallback when possible.
> 
> > > Memory allocation can fail in following cases:
> 
> > > - when non-forced memory allocation used and the attempt to allocate
> 
> > > the memory must be as fast as possible, without waits
> 
> > > - when forced memory allocation used, but the driver already received
> 
> > > stop command and waits for thread termination. Note that in case
> 
> > > the VSync control is enabled stop command may happen even after the video
> 
> > > subsystem executes switch to VGA mode. In such case QEMU will not return
> 
> > > previously allocated objects (assuming the QXL driver already disabled
> 
> > > and ignoring callbacks from Spice server in VGA mode).
> 
> > >
> 

> > Just to confirm, by VGA mode you mean when Windows attempt to unload the
> 
> > driver and use VESA/VGA modes, not VgaDevice, right?
> 
> > In this case I assume the memory will be freed when the driver is
> 
> > unloaded by Windows.
> 
> > I would expect the number of commands sent in this case is small.
> 
> > Also why is not possible to just ignore the commands sent by Windows
> 
> > in this case?
> 

> When VSync is enabled and the system activates watchdog policy, there are 2
> possible flows
> related to driver stopping:
> First one is regular PnP stop (for example, on disable), on stop the driver
> waits until thread
> finishes and the thread is able to complete all the operations, in the worst
> case it will take
> some time until thread allocates all the pending drawables and sends them
> down.
> Second one is forced stop due to long processing of presentation callback. We
> do
> our best to avoid this, but this is still possible, at least in theory. In
> this case OS
> may not not wait until the driver completes stop flow and can activate
> fallback
> mode and make switch to VGA mode. If the thread is processing outstanding
> drawables, it needs to allocate memory (forced) and may pend until some
> previously allocated
> objects must be reported by QEMU/spice server to the guest. As the switch to
> VGA
> already happened, QEMU does not report them (assuming the guest does need
> this,
> which is completely correct in case of regular stop). At the moment of VGA
> switch the
> thread can be in the middle of allocation, so the driver shall know to abort
> the allocation if
> stop flow already started.

Wouldn't be helpful to have a flag set on Stop so the thread could start deleting drawables directly 
instead of processing them? This should make the close faster. 

> BTW, In earlier notes you mentioned possibility to use VGA memory as
> alternate area for
> objects allocation. I think that in stop flow due to watchdog the VGA area
> may be used by
> fallback video engine as frame buffer and this can corrupt the memory arena
> managed by mspace code.

This should not be an issue. The first part of Bar0 is reserved for the frame buffer and excluded 
mspace. VGA should use only this part. But better to test it. 

> > > In case of forced memory allocation the allocation routine waits
> 
> > > unpredictable time until the request is satisfied. In current commit
> 
> > > we do not acquire m_MemLock mutex for all this time, but release it
> 
> > > when entering long wait to allow another caller to try allocating memory.
> 
> > >
> 

> > Looking at final code AllocMem can return NULL in all cases depending
> 
> > on the state of the driver however not all paths check if this function
> 
> > returns NULL. Specifically InitMonitorConfig and SetClip.
> 
> > InitMonitorConfig is called just after initializing memory so is
> 
> > unlikely to fail and SetClip is always called with clip == NULL however
> 
> > I don't like to have unexploded bombs in the code.
> 

> > > Signed-off-by: Yuri Benditovich < yuri.benditovich@daynix.com >
> 
> > > ---
> 
> > > qxldod/QxlDod.cpp | 86
> 
> > > +++++++++++++++++++++++++++++++++++++++++++++++++------
> 
> > > qxldod/QxlDod.h | 3 +-
> 
> > > 2 files changed, 79 insertions(+), 10 deletions(-)
> 
> > >
> 
> > > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> 
> > > index aa70f39..f8440d4 100755
> 
> > > --- a/qxldod/QxlDod.cpp
> 
> > > +++ b/qxldod/QxlDod.cpp
> 
> > > @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
> 
> > > m_FreeOutputs = 0;
> 
> > > m_Pending = 0;
> 
> > > m_PresentThread = NULL;
> 
> > > + m_bActive = FALSE;
> 
> > > }
> 
> > >
> 
> > > QxlDevice::~QxlDevice(void)
> 
> > > @@ -3490,14 +3491,19 @@ NTSTATUS
> > > QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> 
> > > pDispInfo)
> 
> > > Status = AcquireDisplayInfo(*(pDispInfo));
> 
> > > if (NT_SUCCESS(Status))
> 
> > > {
> 
> > > + m_bActive = TRUE;
> 
> > > Status = StartPresentThread();
> 
> > > }
> 
> > > + if (!NT_SUCCESS(Status)) {
> 
> > > + m_bActive = FALSE;
> 
> > > + }
> 
> > > return Status;
> 
> > > }
> 
> > >
> 
> > > void QxlDevice::QxlClose()
> 
> > > {
> 
> > > PAGED_CODE();
> 
> > > + m_bActive = FALSE;
> 
> > > StopPresentThread();
> 
> > > DestroyMemSlots();
> 
> > > }
> 
> > > @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
> 
> > > {
> 
> > > PAGED_CODE();
> 
> > > int wait;
> 
> > > + BOOLEAN locked;
> 
> > >
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
> 
> > >
> 
> > > + locked = WaitForObject(&m_MemLock, NULL);
> 
> > > for (;;) {
> 
> > > LARGE_INTEGER timeout;
> 
> > >
> 
> > > if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> 
> > > + ReleaseMutex(&m_MemLock, locked);
> 
> > > QXL_SLEEP(10);
> 
> > > + locked = WaitForObject(&m_MemLock, NULL);
> 
> > > if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> 
> > > break;
> 
> > > }
> 
> > > @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
> 
> > > }
> 
> > > SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
> 
> > >
> 
> > > - if (!wait) {
> 
> > > + if (!wait || !m_bActive) {
> 
> > > break;
> 
> > > }
> 
> > >
> 
> > > + ReleaseMutex(&m_MemLock, locked);
> 
> > > timeout.QuadPart = -30 * 1000 * 10; //30ms
> 
> > > WaitForObject(&m_DisplayEvent, &timeout);
> 
> > > + locked = WaitForObject(&m_MemLock, NULL);
> 
> > >
> 
> > > if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> 
> > > SyncIo(QXL_IO_NOTIFY_OOM, 0);
> 
> > > }
> 
> > > }
> 
> > > + ReleaseMutex(&m_MemLock, locked);
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
> 
> > > }
> 
> > >
> 
> > > @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
> > > size_t
> 
> > > size, BOOL force)
> 
> > > mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
> 
> > > #endif
> 
> > >
> 
> > > - locked = WaitForObject(&m_MemLock, NULL);
> 
> > > + if (force)
> 
> > > + locked = WaitForObject(&m_MemLock, NULL);
> 
> > > + else {
> 
> > > + LARGE_INTEGER doNotWait;
> 
> > > + doNotWait.QuadPart = 0;
> 
> > > + locked = WaitForObject(&m_MemLock, &doNotWait);
> 
> > > + if (!locked) {
> 
> > > + return NULL;
> 
> > > + }
> 
> > > + }
> 
> > >
> 
> > > while (1) {
> 
> > > /* Release lots of queued resources, before allocating, as we
> 
> > > @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
> > > size_t
> 
> > > size, BOOL force)
> 
> > > continue;
> 
> > > }
> 
> > >
> 
> > > - if (force) {
> 
> > > + if (force && m_bActive) {
> 
> > > /* Ask spice to free some stuff */
> 
> > > + ReleaseMutex(&m_MemLock, locked);
> 
> > > WaitForReleaseRing();
> 
> > > + locked = WaitForObject(&m_MemLock, NULL);
> 
> > > } else {
> 
> > > /* Fail */
> 
> > > break;
> 
> > > }
> 
> > > }
> 
> > > +
> 
> > > ReleaseMutex(&m_MemLock, locked);
> 
> > >
> 
> > > - ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start
> > > &&
> 
> > > + ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> 
> > > m_MSInfo[mspace_type].mspace_start &&
> 
> > > ptr <
> 
> > > m_MSInfo[mspace_type].mspace_end));
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
> 
> > > ptr));
> 
> > > return ptr;
> 
> > > @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
> 
> > > QXLOutput *output;
> 
> > >
> 
> > > output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> 
> > > sizeof(QXLDrawable), TRUE);
> 
> > > + if (!output) {
> 
> > > + return NULL;
> 
> > > + }
> 
> > > output->num_res = 0;
> 
> > > RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
> 
> > > ((QXLDrawable *)output->data)-> release_info.id = (UINT64)output;
> 
> > > @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
> 
> > >
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
> 
> > > output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
> 
> > > sizeof(QXLCursorCmd), TRUE);
> 
> > > + if (!output) {
> 
> > > + return NULL;
> 
> > > + }
> 
> > > output->num_res = 0;
> 
> > > RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
> 
> > > cursor_cmd = (QXLCursorCmd *)output->data;
> 
> > > @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST
> > > RECT
> 
> > > *area, CONST RECT *clip,
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
> 
> > >
> 
> > > drawable = GetDrawable();
> 
> > > + if (!drawable) {
> 
> > > + return NULL;
> 
> > > + }
> 
> > > drawable->surface_id = surface_id;
> 
> > > drawable->type = type;
> 
> > > drawable->effect = QXL_EFFECT_OPAQUE;
> 
> > > @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> 
> > > alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
> 
> > > line_size;
> 
> > > alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
> 
> > > image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
> 
> > > + if (!image_res) {
> 
> > > + ReleaseOutput(drawable-> release_info.id );
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> 
> > > drawable\n"));
> 
> > > + return NULL;
> 
> > > + }
> 
> > >
> 
> > > image_res->refs = 1;
> 
> > > image_res->free = FreeBitmapImageEx;
> 
> > > @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> 
> > > alloc_size = height * line_size;
> 
> > >
> 
> > > for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
> 
> > > - PutBytesAlign(&chunk, &dest, &dest_end, src,
> 
> > > - line_size, alloc_size, line_size);
> 
> > > + if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> 
> > > + line_size, alloc_size, line_size)) {
> 
> > > + ReleaseOutput(drawable-> release_info.id );
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap
> 
> > > for drawable\n"));
> 
> > > + return NULL;
> 
> > > + }
> 
> > > }
> 
> > >
> 
> > > internal->image.bitmap.palette = 0;
> 
> > > @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
> 
> > > return drawable;
> 
> > > }
> 
> > >
> 
> > > -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> 
> > > +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8
> > > **now_ptr,
> 
> > > UINT8 **end_ptr, UINT8 *src, int size,
> 
> > > size_t alloc_size, uint32_t alignment)
> 
> > > {
> 
> > > @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
> > > **chunk_ptr,
> 
> > > UINT8 **now_ptr,
> 
> > > aligned_size -= aligned_size % alignment;
> 
> > >
> 
> > > void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
> 
> > > sizeof(QXLDataChunk), TRUE);
> 
> > > + if (!ptr) {
> 
> > > + return FALSE;
> 
> > > + }
> 
> > > chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
> 
> > > ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
> 
> > > chunk = (QXLDataChunk *)ptr;
> 
> > > @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
> > > **chunk_ptr,
> 
> > > UINT8 **now_ptr,
> 
> > > *now_ptr = now;
> 
> > > *end_ptr = end;
> 
> > > DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> 
> > > + return TRUE;
> 
> > > }
> 
> > >
> 
> > > VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> 
> > > @@ -4573,6 +4620,11 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> 
> > > DXGKARG_SETPOINTERSHAPE* pSetPoi
> 
> > > int num_images = 1;
> 
> > >
> 
> > > cursor_cmd = CursorCmd();
> 
> > > + if (!cursor_cmd) {
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> 
> > > command\n", __FUNCTION__));
> 
> > > + return STATUS_INSUFFICIENT_RESOURCES;
> 
> > > + }
> 
> > > +
> 
> > > cursor_cmd->type = QXL_CURSOR_SET;
> 
> > >
> 
> > > cursor_cmd->u.set.visible = TRUE;
> 
> > > @@ -4580,6 +4632,12 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> 
> > > DXGKARG_SETPOINTERSHAPE* pSetPoi
> 
> > > cursor_cmd->u.set.position.y = 0;
> 
> > >
> 
> > > res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
> 
> > > + if (!res) {
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",
> 
> > > __FUNCTION__));
> 
> > > + ReleaseOutput(cursor_cmd-> release_info.id );
> 
> > > + return STATUS_INSUFFICIENT_RESOURCES;
> 
> > > + }
> 
> > > +
> 
> > > res->refs = 1;
> 
> > > res->free = FreeCursorEx;
> 
> > > res->ptr = this;
> 
> > > @@ -4616,8 +4674,13 @@ NTSTATUS QxlDevice::SetPointerShape(_In_ CONST
> 
> > > DXGKARG_SETPOINTERSHAPE* pSetPoi
> 
> > > end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
> 
> > > src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *
> 
> > > num_images);
> 
> > > for (; src != src_end; src += pSetPointerShape->Pitch) {
> 
> > > - PutBytesAlign(&chunk, &now, &end, src, line_size,
> 
> > > - PAGE_SIZE, 1);
> 
> > > + if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> 
> > > + PAGE_SIZE, 1))
> 
> > > + {
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> 
> > > bitmap\n", __FUNCTION__));
> 
> > > + ReleaseOutput(cursor_cmd-> release_info.id );
> 
> > > + return STATUS_INSUFFICIENT_RESOURCES;
> 
> > > + }
> 
> > > }
> 
> > > CursorCmdAddRes(cursor_cmd, res);
> 
> > > RELEASE_RES(res);
> 
> > > @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> 
> > > DXGKARG_SETPOINTERPOSITION* pS
> 
> > > pSetPointerPosition->X,
> 
> > > pSetPointerPosition->Y));
> 
> > > QXLCursorCmd *cursor_cmd = CursorCmd();
> 
> > > + if (!cursor_cmd) {
> 
> > > + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> 
> > > command\n", __FUNCTION__));
> 
> > > + return STATUS_INSUFFICIENT_RESOURCES;
> 
> > > + }
> 
> > > +
> 
> > > if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
> 
> > > cursor_cmd->type = QXL_CURSOR_HIDE;
> 
> > > } else {
> 
> > > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> 
> > > index a1e9634..cbcd11e 100755
> 
> > > --- a/qxldod/QxlDod.h
> 
> > > +++ b/qxldod/QxlDod.h
> 
> > > @@ -601,7 +601,7 @@ private:
> 
> > > void PushCmd(void);
> 
> > > void WaitForCursorRing(void);
> 
> > > void PushCursor(void);
> 
> > > - void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> 
> > > + BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> 
> > > UINT8 **end_ptr, UINT8 *src, int size,
> 
> > > size_t alloc_size, uint32_t alignment);
> 
> > > void AsyncIo(UCHAR Port, UCHAR Value);
> 
> > > @@ -671,6 +671,7 @@ private:
> 
> > >
> 
> > > QXLPresentOnlyRing m_PresentRing[1];
> 
> > > HANDLE m_PresentThread;
> 
> > > + BOOLEAN m_bActive;
> 
> > > };
> 
> > >
> 
> > > class QxlDod {
>
On Mon, Apr 3, 2017 at 9:12 PM, Frediano Ziglio <fziglio@redhat.com> wrote:

>
> On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio <fziglio@redhat.com>
> wrote:
>
>> >
>> > From: "yuri.benditovich@daynix.com" <yuri.benditovich@daynix.com>
>> >
>> > Preparation for further scenarios when memory allocation failure
>> > can happen and we'll need to use fallback when possible.
>> > Memory allocation can fail in following cases:
>> > - when non-forced memory allocation used and the attempt to allocate
>> >   the memory must be as fast as possible, without waits
>> > - when forced memory allocation used, but the driver already received
>> >   stop command and waits for thread termination. Note that in case
>> >   the VSync control is enabled stop command may happen even after the
>> video
>> >   subsystem executes switch to VGA mode. In such case QEMU will not
>> return
>> >   previously allocated objects (assuming the QXL driver already disabled
>> >   and ignoring callbacks from Spice server in VGA mode).
>> >
>>
>> Just to confirm, by VGA mode you mean when Windows attempt to unload the
>> driver and use VESA/VGA modes, not VgaDevice, right?
>> In this case I assume the memory will be freed when the driver is
>> unloaded by Windows.
>> I would expect the number of commands sent in this case is small.
>> Also why is not possible to just ignore the commands sent by Windows
>> in this case?
>>
>
> When VSync is enabled and the system activates watchdog policy, there are
> 2 possible flows
> related to driver stopping:
> First one is regular PnP stop (for example, on disable), on stop the
> driver waits until thread
> finishes and the thread is able to complete all the operations, in the
> worst case it will take
> some time until thread allocates all the pending drawables and sends them
> down.
> Second one is forced stop due to long processing of presentation callback.
> We do
> our best to avoid this, but this is still possible, at least in theory. In
> this case OS
> may not not wait until the driver completes stop flow and can activate
> fallback
> mode and make switch to VGA mode. If the thread is processing outstanding
> drawables, it needs to allocate memory (forced) and may pend until some
> previously allocated
> objects must be reported by QEMU/spice server to the guest. As the switch
> to VGA
> already happened, QEMU does not report them (assuming the guest does need
> this,
> which is completely correct in case of regular stop). At the moment of VGA
> switch the
> thread can be in the middle of allocation, so the driver shall know to
> abort the allocation if
> stop flow already started.
>
> Wouldn't be helpful to have a flag set on Stop so the thread could start
> deleting drawables directly
> instead of processing them? This should make the close faster.
>
> Yes, this can make the stop faster. I'll prepare such patch over 12/12 as
the driver
anyway shall review the chain of chunks and free the memory it allocated
from OS, in any.

>
> BTW, In earlier notes you mentioned possibility to use VGA memory as
> alternate area for
> objects allocation. I think that in stop flow due to watchdog the VGA area
> may be used by
> fallback video engine as frame buffer and this can corrupt the memory
> arena managed by mspace code.
>
> This should not be an issue. The first part of Bar0 is reserved for the
> frame buffer and excluded
> mspace. VGA should use only this part. But better to test it.
>
>
>> > In case of forced memory allocation the allocation routine waits
>> > unpredictable time until the request is satisfied. In current commit
>> > we do not acquire m_MemLock mutex for all this time, but release it
>> > when entering long wait to allow another caller to try allocating
>> memory.
>> >
>>
>> Looking at final code AllocMem can return NULL in all cases depending
>> on the state of the driver however not all paths check if this function
>> returns NULL. Specifically InitMonitorConfig and SetClip.
>> InitMonitorConfig is called just after initializing memory so is
>> unlikely to fail and SetClip is always called with clip == NULL however
>> I don't like to have unexploded bombs in the code.
>>
>> > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
>> > ---
>> >  qxldod/QxlDod.cpp | 86
>> >  +++++++++++++++++++++++++++++++++++++++++++++++++------
>> >  qxldod/QxlDod.h   |  3 +-
>> >  2 files changed, 79 insertions(+), 10 deletions(-)
>> >
>> > diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
>> > index aa70f39..f8440d4 100755
>> > --- a/qxldod/QxlDod.cpp
>> > +++ b/qxldod/QxlDod.cpp
>> > @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>> >      m_FreeOutputs = 0;
>> >      m_Pending = 0;
>> >      m_PresentThread = NULL;
>> > +    m_bActive = FALSE;
>> >  }
>> >
>> >  QxlDevice::~QxlDevice(void)
>> > @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_
>> DISPLAY_INFORMATION*
>> > pDispInfo)
>> >      Status = AcquireDisplayInfo(*(pDispInfo));
>> >      if (NT_SUCCESS(Status))
>> >      {
>> > +        m_bActive = TRUE;
>> >          Status = StartPresentThread();
>> >      }
>> > +    if (!NT_SUCCESS(Status)) {
>> > +        m_bActive = FALSE;
>> > +    }
>> >      return Status;
>> >  }
>> >
>> >  void QxlDevice::QxlClose()
>> >  {
>> >      PAGED_CODE();
>> > +    m_bActive = FALSE;
>> >      StopPresentThread();
>> >      DestroyMemSlots();
>> >  }
>> > @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
>> >  {
>> >      PAGED_CODE();
>> >      int wait;
>> > +    BOOLEAN locked;
>> >
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
>> >
>> > +    locked = WaitForObject(&m_MemLock, NULL);
>> >      for (;;) {
>> >          LARGE_INTEGER timeout;
>> >
>> >          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>> > +            ReleaseMutex(&m_MemLock, locked);
>> >              QXL_SLEEP(10);
>> > +            locked = WaitForObject(&m_MemLock, NULL);
>> >              if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>> >                  break;
>> >              }
>> > @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
>> >          }
>> >          SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
>> >
>> > -        if (!wait) {
>> > +        if (!wait || !m_bActive) {
>> >              break;
>> >          }
>> >
>> > +        ReleaseMutex(&m_MemLock, locked);
>> >          timeout.QuadPart = -30 * 1000 * 10; //30ms
>> >          WaitForObject(&m_DisplayEvent, &timeout);
>> > +        locked = WaitForObject(&m_MemLock, NULL);
>> >
>> >          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>> >              SyncIo(QXL_IO_NOTIFY_OOM, 0);
>> >          }
>> >      }
>> > +    ReleaseMutex(&m_MemLock, locked);
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
>> >  }
>> >
>> > @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
>> size_t
>> > size, BOOL force)
>> >       mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
>> >  #endif
>> >
>> > -    locked = WaitForObject(&m_MemLock, NULL);
>> > +    if (force)
>> > +        locked = WaitForObject(&m_MemLock, NULL);
>> > +    else {
>> > +        LARGE_INTEGER doNotWait;
>> > +        doNotWait.QuadPart = 0;
>> > +        locked = WaitForObject(&m_MemLock, &doNotWait);
>> > +        if (!locked) {
>> > +             return NULL;
>> > +        }
>> > +    }
>> >
>> >      while (1) {
>> >          /* Release lots of queued resources, before allocating, as we
>> > @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type,
>> size_t
>> > size, BOOL force)
>> >              continue;
>> >          }
>> >
>> > -        if (force) {
>> > +        if (force && m_bActive) {
>> >              /* Ask spice to free some stuff */
>> > +            ReleaseMutex(&m_MemLock, locked);
>> >              WaitForReleaseRing();
>> > +            locked = WaitForObject(&m_MemLock, NULL);
>> >          } else {
>> >              /* Fail */
>> >              break;
>> >          }
>> >      }
>> > +
>> >      ReleaseMutex(&m_MemLock, locked);
>> >
>> > -    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start
>> &&
>> > +    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
>> > m_MSInfo[mspace_type].mspace_start &&
>> >                                        ptr <
>> >                                        m_MSInfo[mspace_type].mspace_
>> end));
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
>> >      ptr));
>> >      return ptr;
>> > @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
>> >      QXLOutput *output;
>> >
>> >      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput)
>> +
>> >      sizeof(QXLDrawable), TRUE);
>> > +    if (!output) {
>> > +        return NULL;
>> > +    }
>> >      output->num_res = 0;
>> >      RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
>> >      ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
>> > @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
>> >
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>> >      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput)
>> +
>> >      sizeof(QXLCursorCmd), TRUE);
>> > +    if (!output) {
>> > +        return NULL;
>> > +    }
>> >      output->num_res = 0;
>> >      RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
>> >      cursor_cmd = (QXLCursorCmd *)output->data;
>> > @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type,
>> CONST RECT
>> > *area, CONST RECT *clip,
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>> >
>> >      drawable = GetDrawable();
>> > +    if (!drawable) {
>> > +        return NULL;
>> > +    }
>> >      drawable->surface_id = surface_id;
>> >      drawable->type = type;
>> >      drawable->effect = QXL_EFFECT_OPAQUE;
>> > @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> >      alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
>> >      line_size;
>> >      alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size,
>> alloc_size);
>> >      image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size,
>> TRUE);
>> > +    if (!image_res) {
>> > +        ReleaseOutput(drawable->release_info.id);
>> > +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
>> > drawable\n"));
>> > +        return NULL;
>> > +    }
>> >
>> >      image_res->refs = 1;
>> >      image_res->free = FreeBitmapImageEx;
>> > @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> >      alloc_size = height * line_size;
>> >
>> >      for (; src != src_end; src -= pSrc->Pitch, alloc_size -=
>> line_size) {
>> > -        PutBytesAlign(&chunk, &dest, &dest_end, src,
>> > -                      line_size, alloc_size, line_size);
>> > +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
>> > +            line_size, alloc_size, line_size)) {
>> > +            ReleaseOutput(drawable->release_info.id);
>> > +            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional
>> bitmap
>> > for drawable\n"));
>> > +            return NULL;
>> > +        }
>> >      }
>> >
>> >      internal->image.bitmap.palette = 0;
>> > @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>> >      return drawable;
>> >  }
>> >
>> > -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8
>> **now_ptr,
>> > +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8
>> **now_ptr,
>> >                              UINT8 **end_ptr, UINT8 *src, int size,
>> >                              size_t alloc_size, uint32_t alignment)
>> >  {
>> > @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
>> **chunk_ptr,
>> > UINT8 **now_ptr,
>> >              aligned_size -=  aligned_size % alignment;
>> >
>> >              void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
>> >              sizeof(QXLDataChunk), TRUE);
>> > +            if (!ptr) {
>> > +                return FALSE;
>> > +            }
>> >              chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
>> >              ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk,
>> m_SurfaceMemSlot);
>> >              chunk = (QXLDataChunk *)ptr;
>> > @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk
>> **chunk_ptr,
>> > UINT8 **now_ptr,
>> >      *now_ptr = now;
>> >      *end_ptr = end;
>> >      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
>> > +    return TRUE;
>> >  }
>> >
>> >  VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
>> > @@ -4573,6 +4620,11 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
>> > DXGKARG_SETPOINTERSHAPE* pSetPoi
>> >      int num_images = 1;
>> >
>> >      cursor_cmd = CursorCmd();
>> > +    if (!cursor_cmd) {
>> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
>> > command\n", __FUNCTION__));
>> > +        return STATUS_INSUFFICIENT_RESOURCES;
>> > +    }
>> > +
>> >      cursor_cmd->type = QXL_CURSOR_SET;
>> >
>> >      cursor_cmd->u.set.visible = TRUE;
>> > @@ -4580,6 +4632,12 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
>> > DXGKARG_SETPOINTERSHAPE* pSetPoi
>> >      cursor_cmd->u.set.position.y = 0;
>> >
>> >      res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE,
>> TRUE);
>> > +    if (!res) {
>> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
>> data\n",
>> > __FUNCTION__));
>> > +        ReleaseOutput(cursor_cmd->release_info.id);
>> > +        return STATUS_INSUFFICIENT_RESOURCES;
>> > +    }
>> > +
>> >      res->refs = 1;
>> >      res->free = FreeCursorEx;
>> >      res->ptr = this;
>> > @@ -4616,8 +4674,13 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
>> > DXGKARG_SETPOINTERSHAPE* pSetPoi
>> >      end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
>> >      src_end = src + (pSetPointerShape->Pitch *
>> pSetPointerShape->Height *
>> >      num_images);
>> >      for (; src != src_end; src += pSetPointerShape->Pitch) {
>> > -        PutBytesAlign(&chunk, &now, &end, src, line_size,
>> > -                 PAGE_SIZE, 1);
>> > +        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
>> > +                 PAGE_SIZE, 1))
>> > +        {
>> > +            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
>> > bitmap\n", __FUNCTION__));
>> > +            ReleaseOutput(cursor_cmd->release_info.id);
>> > +            return STATUS_INSUFFICIENT_RESOURCES;
>> > +        }
>> >      }
>> >      CursorCmdAddRes(cursor_cmd, res);
>> >      RELEASE_RES(res);
>> > @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_
>> CONST
>> > DXGKARG_SETPOINTERPOSITION* pS
>> >                                   pSetPointerPosition->X,
>> >                                   pSetPointerPosition->Y));
>> >      QXLCursorCmd *cursor_cmd = CursorCmd();
>> > +    if (!cursor_cmd) {
>> > +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
>> > command\n", __FUNCTION__));
>> > +        return STATUS_INSUFFICIENT_RESOURCES;
>> > +    }
>> > +
>> >      if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible)
>> {
>> >          cursor_cmd->type = QXL_CURSOR_HIDE;
>> >      } else {
>> > diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
>> > index a1e9634..cbcd11e 100755
>> > --- a/qxldod/QxlDod.h
>> > +++ b/qxldod/QxlDod.h
>> > @@ -601,7 +601,7 @@ private:
>> >      void PushCmd(void);
>> >      void WaitForCursorRing(void);
>> >      void PushCursor(void);
>> > -    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>> > +    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>> >                              UINT8 **end_ptr, UINT8 *src, int size,
>> >                              size_t alloc_size, uint32_t alignment);
>> >      void AsyncIo(UCHAR  Port, UCHAR Value);
>> > @@ -671,6 +671,7 @@ private:
>> >
>> >      QXLPresentOnlyRing m_PresentRing[1];
>> >      HANDLE m_PresentThread;
>> > +    BOOLEAN m_bActive;
>> >  };
>> >
>> >  class QxlDod {
>>
>
>
>
Acked

Frediano

> 
> From: "yuri.benditovich@daynix.com" <yuri.benditovich@daynix.com>
> 
> Preparation for further scenarios when memory allocation failure
> can happen and we'll need to use fallback when possible.
> Memory allocation can fail in following cases:
> - when non-forced memory allocation used and the attempt to allocate
>   the memory must be as fast as possible, without waits
> - when forced memory allocation used, but the driver already received
>   stop command and waits for thread termination. Note that in case
>   the VSync control is enabled stop command may happen even after the video
>   subsystem executes switch to VGA mode. In such case QEMU will not return
>   previously allocated objects (assuming the QXL driver already disabled
>   and ignoring callbacks from Spice server in VGA mode).
> 
> In case of forced memory allocation the allocation routine waits
> unpredictable time until the request is satisfied. In current commit
> we do not acquire m_MemLock mutex for all this time, but release it
> when entering long wait to allow another caller to try allocating memory.
> 
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> ---
>  qxldod/QxlDod.cpp | 86
>  +++++++++++++++++++++++++++++++++++++++++++++++++------
>  qxldod/QxlDod.h   |  3 +-
>  2 files changed, 79 insertions(+), 10 deletions(-)
> 
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp
> index aa70f39..f8440d4 100755
> --- a/qxldod/QxlDod.cpp
> +++ b/qxldod/QxlDod.cpp
> @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)
>      m_FreeOutputs = 0;
>      m_Pending = 0;
>      m_PresentThread = NULL;
> +    m_bActive = FALSE;
>  }
>  
>  QxlDevice::~QxlDevice(void)
> @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_DISPLAY_INFORMATION*
> pDispInfo)
>      Status = AcquireDisplayInfo(*(pDispInfo));
>      if (NT_SUCCESS(Status))
>      {
> +        m_bActive = TRUE;
>          Status = StartPresentThread();
>      }
> +    if (!NT_SUCCESS(Status)) {
> +        m_bActive = FALSE;
> +    }
>      return Status;
>  }
>  
>  void QxlDevice::QxlClose()
>  {
>      PAGED_CODE();
> +    m_bActive = FALSE;
>      StopPresentThread();
>      DestroyMemSlots();
>  }
> @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(void)
>  {
>      PAGED_CODE();
>      int wait;
> +    BOOLEAN locked;
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));
>  
> +    locked = WaitForObject(&m_MemLock, NULL);
>      for (;;) {
>          LARGE_INTEGER timeout;
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
> +            ReleaseMutex(&m_MemLock, locked);
>              QXL_SLEEP(10);
> +            locked = WaitForObject(&m_MemLock, NULL);
>              if (!SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>                  break;
>              }
> @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(void)
>          }
>          SPICE_RING_CONS_WAIT(m_ReleaseRing, wait);
>  
> -        if (!wait) {
> +        if (!wait || !m_bActive) {
>              break;
>          }
>  
> +        ReleaseMutex(&m_MemLock, locked);
>          timeout.QuadPart = -30 * 1000 * 10; //30ms
>          WaitForObject(&m_DisplayEvent, &timeout);
> +        locked = WaitForObject(&m_MemLock, NULL);
>  
>          if (SPICE_RING_IS_EMPTY(m_ReleaseRing)) {
>              SyncIo(QXL_IO_NOTIFY_OOM, 0);
>          }
>      }
> +    ReleaseMutex(&m_MemLock, locked);
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));
>  }
>  
> @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>       mspace_malloc_stats(m_MSInfo[mspace_type]._mspace);
>  #endif
>  
> -    locked = WaitForObject(&m_MemLock, NULL);
> +    if (force)
> +        locked = WaitForObject(&m_MemLock, NULL);
> +    else {
> +        LARGE_INTEGER doNotWait;
> +        doNotWait.QuadPart = 0;
> +        locked = WaitForObject(&m_MemLock, &doNotWait);
> +        if (!locked) {
> +             return NULL;
> +        }
> +    }
>  
>      while (1) {
>          /* Release lots of queued resources, before allocating, as we
> @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t
> size, BOOL force)
>              continue;
>          }
>  
> -        if (force) {
> +        if (force && m_bActive) {
>              /* Ask spice to free some stuff */
> +            ReleaseMutex(&m_MemLock, locked);
>              WaitForReleaseRing();
> +            locked = WaitForObject(&m_MemLock, NULL);
>          } else {
>              /* Fail */
>              break;
>          }
>      }
> +
>      ReleaseMutex(&m_MemLock, locked);
>  
> -    ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_start &&
> +    ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=
> m_MSInfo[mspace_type].mspace_start &&
>                                        ptr <
>                                        m_MSInfo[mspace_type].mspace_end));
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,
>      ptr));
>      return ptr;
> @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()
>      QXLOutput *output;
>  
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLDrawable), TRUE);
> +    if (!output) {
> +        return NULL;
> +    }
>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);
>      ((QXLDrawable *)output->data)->release_info.id = (UINT64)output;
> @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()
>  
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>      output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +
>      sizeof(QXLCursorCmd), TRUE);
> +    if (!output) {
> +        return NULL;
> +    }
>      output->num_res = 0;
>      RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);
>      cursor_cmd = (QXLCursorCmd *)output->data;
> @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT
> *area, CONST RECT *clip,
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));
>  
>      drawable = GetDrawable();
> +    if (!drawable) {
> +        return NULL;
> +    }
>      drawable->surface_id = surface_id;
>      drawable->type = type;
>      drawable->effect = QXL_EFFECT_OPAQUE;
> @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %
>      line_size;
>      alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);
>      image_res = (Resource*)AllocMem(MSPACE_TYPE_VRAM, alloc_size, TRUE);
> +    if (!image_res) {
> +        ReleaseOutput(drawable->release_info.id);
> +        DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for
> drawable\n"));
> +        return NULL;
> +    }
>  
>      image_res->refs = 1;
>      image_res->free = FreeBitmapImageEx;
> @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      alloc_size = height * line_size;
>  
>      for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {
> -        PutBytesAlign(&chunk, &dest, &dest_end, src,
> -                      line_size, alloc_size, line_size);
> +        if (!PutBytesAlign(&chunk, &dest, &dest_end, src,
> +            line_size, alloc_size, line_size)) {
> +            ReleaseOutput(drawable->release_info.id);
> +            DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap
> for drawable\n"));
> +            return NULL;
> +        }
>      }
>  
>      internal->image.bitmap.palette = 0;
> @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (
>      return drawable;
>  }
>  
> -VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +BOOLEAN QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment)
>  {
> @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
>              aligned_size -=  aligned_size % alignment;
>  
>              void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +
>              sizeof(QXLDataChunk), TRUE);
> +            if (!ptr) {
> +                return FALSE;
> +            }
>              chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);
>              ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);
>              chunk = (QXLDataChunk *)ptr;
> @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(QXLDataChunk **chunk_ptr,
> UINT8 **now_ptr,
>      *now_ptr = now;
>      *end_ptr = end;
>      DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));
> +    return TRUE;
>  }
>  
>  VOID QxlDevice::BlackOutScreen(CURRENT_BDD_MODE* pCurrentBddMod)
> @@ -4573,6 +4620,11 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      int num_images = 1;
>  
>      cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      cursor_cmd->type = QXL_CURSOR_SET;
>  
>      cursor_cmd->u.set.visible = TRUE;
> @@ -4580,6 +4632,12 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      cursor_cmd->u.set.position.y = 0;
>  
>      res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);
> +    if (!res) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",
> __FUNCTION__));
> +        ReleaseOutput(cursor_cmd->release_info.id);
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      res->refs = 1;
>      res->free = FreeCursorEx;
>      res->ptr = this;
> @@ -4616,8 +4674,13 @@ NTSTATUS  QxlDevice::SetPointerShape(_In_ CONST
> DXGKARG_SETPOINTERSHAPE* pSetPoi
>      end = (UINT8 *)res + CURSOR_ALLOC_SIZE;
>      src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *
>      num_images);
>      for (; src != src_end; src += pSetPointerShape->Pitch) {
> -        PutBytesAlign(&chunk, &now, &end, src, line_size,
> -                 PAGE_SIZE, 1);
> +        if (!PutBytesAlign(&chunk, &now, &end, src, line_size,
> +                 PAGE_SIZE, 1))
> +        {
> +            DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> bitmap\n", __FUNCTION__));
> +            ReleaseOutput(cursor_cmd->release_info.id);
> +            return STATUS_INSUFFICIENT_RESOURCES;
> +        }
>      }
>      CursorCmdAddRes(cursor_cmd, res);
>      RELEASE_RES(res);
> @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(_In_ CONST
> DXGKARG_SETPOINTERPOSITION* pS
>                                   pSetPointerPosition->X,
>                                   pSetPointerPosition->Y));
>      QXLCursorCmd *cursor_cmd = CursorCmd();
> +    if (!cursor_cmd) {
> +        DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor
> command\n", __FUNCTION__));
> +        return STATUS_INSUFFICIENT_RESOURCES;
> +    }
> +
>      if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.Visible) {
>          cursor_cmd->type = QXL_CURSOR_HIDE;
>      } else {
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h
> index a1e9634..cbcd11e 100755
> --- a/qxldod/QxlDod.h
> +++ b/qxldod/QxlDod.h
> @@ -601,7 +601,7 @@ private:
>      void PushCmd(void);
>      void WaitForCursorRing(void);
>      void PushCursor(void);
> -    void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
> +    BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,
>                              UINT8 **end_ptr, UINT8 *src, int size,
>                              size_t alloc_size, uint32_t alignment);
>      void AsyncIo(UCHAR  Port, UCHAR Value);
> @@ -671,6 +671,7 @@ private:
>  
>      QXLPresentOnlyRing m_PresentRing[1];
>      HANDLE m_PresentThread;
> +    BOOLEAN m_bActive;
>  };
>  
>  class QxlDod {