<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Apr 3, 2017 at 9:12 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><div><div class="h5"><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><br><div class="gmail_extra"><div class="gmail_quote">On Mon, Apr 3, 2017 at 1:55 PM, Frediano Ziglio <span dir="ltr"><<a href="mailto:fziglio@redhat.com" target="_blank">fziglio@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span>><br>
> From: "<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>" <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>><br>
><br>
> Preparation for further scenarios when memory allocation failure<br>
> can happen and we'll need to use fallback when possible.<br>
> Memory allocation can fail in following cases:<br>
> - when non-forced memory allocation used and the attempt to allocate<br>
> the memory must be as fast as possible, without waits<br>
> - when forced memory allocation used, but the driver already received<br>
> stop command and waits for thread termination. Note that in case<br>
> the VSync control is enabled stop command may happen even after the video<br>
> subsystem executes switch to VGA mode. In such case QEMU will not return<br>
> previously allocated objects (assuming the QXL driver already disabled<br>
> and ignoring callbacks from Spice server in VGA mode).<br>
><br>
<br>
</span>Just to confirm, by VGA mode you mean when Windows attempt to unload the<br>
driver and use VESA/VGA modes, not VgaDevice, right?<br>
In this case I assume the memory will be freed when the driver is<br>
unloaded by Windows.<br>
I would expect the number of commands sent in this case is small.<br>
Also why is not possible to just ignore the commands sent by Windows<br>
in this case?<br></blockquote><div><br></div><div><div>When VSync is enabled and the system activates watchdog policy, there are 2 possible flows</div><div>related to driver stopping:</div><div>First one is regular PnP stop (for example, on disable), on stop the driver waits until thread</div><div>finishes and the thread is able to complete all the operations, in the worst case it will take</div><div>some time until thread allocates all the pending drawables and sends them down.</div><div>Second one is forced stop due to long processing of presentation callback. We do</div><div>our best to avoid this, but this is still possible, at least in theory. In this case OS</div><div>may not not wait until the driver completes stop flow and can activate fallback</div><div>mode and make switch to VGA mode. If the thread is processing outstanding</div><div>drawables, it needs to allocate memory (forced) and may pend until some previously allocated</div><div>objects must be reported by QEMU/spice server to the guest. As the switch to VGA</div><div>already happened, QEMU does not report them (assuming the guest does need this,</div><div>which is completely correct in case of regular stop). At the moment of VGA switch the</div><div>thread can be in the middle of allocation, so the driver shall know to abort the allocation if</div><div>stop flow already started.</div></div></div></div></div></blockquote></div></div><div>Wouldn't be helpful to have a flag set on Stop so the thread could start deleting drawables directly<br></div><div>instead of processing them? This should make the close faster.<br></div><span class=""><div><br></div></span></div></div></blockquote><div>Yes, this can make the stop faster. I'll prepare such patch over 12/12 as the driver</div><div>anyway shall review the chain of chunks and free the memory it allocated from OS, in any. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div><div style="font-family:times new roman,new york,times,serif;font-size:12pt;color:#000000"><span class=""><div></div><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><br></div><div>BTW, In earlier notes you mentioned possibility to use VGA memory as alternate area for </div><div>objects allocation. I think that in stop flow due to watchdog the VGA area may be used by</div><div>fallback video engine as frame buffer and this can corrupt the memory arena managed by mspace code.</div></div></div></div></blockquote></span><div>This should not be an issue. The first part of Bar0 is reserved for the frame buffer and excluded<br></div><div>mspace. VGA should use only this part. But better to test it.<br></div><div><div class="h5"><div><br></div><blockquote style="border-left:2px solid #1010ff;margin-left:5px;padding-left:5px;color:#000;font-weight:normal;font-style:normal;text-decoration:none;font-family:Helvetica,Arial,sans-serif;font-size:12pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span><br>
> In case of forced memory allocation the allocation routine waits<br>
> unpredictable time until the request is satisfied. In current commit<br>
> we do not acquire m_MemLock mutex for all this time, but release it<br>
> when entering long wait to allow another caller to try allocating memory.<br>
><br>
<br>
</span>Looking at final code AllocMem can return NULL in all cases depending<br>
on the state of the driver however not all paths check if this function<br>
returns NULL. Specifically InitMonitorConfig and SetClip.<br>
InitMonitorConfig is called just after initializing memory so is<br>
unlikely to fail and SetClip is always called with clip == NULL however<br>
I don't like to have unexploded bombs in the code.<br><div class="m_5849252187370418548gmail-m_-721699572120810773HOEnZb"><div class="m_5849252187370418548gmail-m_-721699572120810773h5"><br>
> Signed-off-by: Yuri Benditovich <<a href="mailto:yuri.benditovich@daynix.com" target="_blank">yuri.benditovich@daynix.com</a>><br>
> ---<br>
> qxldod/QxlDod.cpp | 86<br>
> ++++++++++++++++++++++++++++++<wbr>+++++++++++++++++++------<br>
> qxldod/QxlDod.h | 3 +-<br>
> 2 files changed, 79 insertions(+), 10 deletions(-)<br>
><br>
> diff --git a/qxldod/QxlDod.cpp b/qxldod/QxlDod.cpp<br>
> index aa70f39..f8440d4 100755<br>
> --- a/qxldod/QxlDod.cpp<br>
> +++ b/qxldod/QxlDod.cpp<br>
> @@ -3064,6 +3064,7 @@ QxlDevice::QxlDevice(_In_ QxlDod* pQxlDod)<br>
> m_FreeOutputs = 0;<br>
> m_Pending = 0;<br>
> m_PresentThread = NULL;<br>
> + m_bActive = FALSE;<br>
> }<br>
><br>
> QxlDevice::~QxlDevice(void)<br>
> @@ -3490,14 +3491,19 @@ NTSTATUS QxlDevice::QxlInit(DXGK_<wbr>DISPLAY_INFORMATION*<br>
> pDispInfo)<br>
> Status = AcquireDisplayInfo(*(<wbr>pDispInfo));<br>
> if (NT_SUCCESS(Status))<br>
> {<br>
> + m_bActive = TRUE;<br>
> Status = StartPresentThread();<br>
> }<br>
> + if (!NT_SUCCESS(Status)) {<br>
> + m_bActive = FALSE;<br>
> + }<br>
> return Status;<br>
> }<br>
><br>
> void QxlDevice::QxlClose()<br>
> {<br>
> PAGED_CODE();<br>
> + m_bActive = FALSE;<br>
> StopPresentThread();<br>
> DestroyMemSlots();<br>
> }<br>
> @@ -3945,14 +3951,18 @@ void QxlDevice::WaitForReleaseRing(<wbr>void)<br>
> {<br>
> PAGED_CODE();<br>
> int wait;<br>
> + BOOLEAN locked;<br>
><br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("--->%s\n", __FUNCTION__));<br>
><br>
> + locked = WaitForObject(&m_MemLock, NULL);<br>
> for (;;) {<br>
> LARGE_INTEGER timeout;<br>
><br>
> if (SPICE_RING_IS_EMPTY(m_<wbr>ReleaseRing)) {<br>
> + ReleaseMutex(&m_MemLock, locked);<br>
> QXL_SLEEP(10);<br>
> + locked = WaitForObject(&m_MemLock, NULL);<br>
> if (!SPICE_RING_IS_EMPTY(m_<wbr>ReleaseRing)) {<br>
> break;<br>
> }<br>
> @@ -3960,17 +3970,20 @@ void QxlDevice::WaitForReleaseRing(<wbr>void)<br>
> }<br>
> SPICE_RING_CONS_WAIT(m_<wbr>ReleaseRing, wait);<br>
><br>
> - if (!wait) {<br>
> + if (!wait || !m_bActive) {<br>
> break;<br>
> }<br>
><br>
> + ReleaseMutex(&m_MemLock, locked);<br>
> timeout.QuadPart = -30 * 1000 * 10; //30ms<br>
> WaitForObject(&m_DisplayEvent, &timeout);<br>
> + locked = WaitForObject(&m_MemLock, NULL);<br>
><br>
> if (SPICE_RING_IS_EMPTY(m_<wbr>ReleaseRing)) {<br>
> SyncIo(QXL_IO_NOTIFY_OOM, 0);<br>
> }<br>
> }<br>
> + ReleaseMutex(&m_MemLock, locked);<br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("%s: <---\n", __FUNCTION__));<br>
> }<br>
><br>
> @@ -4052,7 +4065,16 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t<br>
> size, BOOL force)<br>
> mspace_malloc_stats(m_MSInfo[<wbr>mspace_type]._mspace);<br>
> #endif<br>
><br>
> - locked = WaitForObject(&m_MemLock, NULL);<br>
> + if (force)<br>
> + locked = WaitForObject(&m_MemLock, NULL);<br>
> + else {<br>
> + LARGE_INTEGER doNotWait;<br>
> + doNotWait.QuadPart = 0;<br>
> + locked = WaitForObject(&m_MemLock, &doNotWait);<br>
> + if (!locked) {<br>
> + return NULL;<br>
> + }<br>
> + }<br>
><br>
> while (1) {<br>
> /* Release lots of queued resources, before allocating, as we<br>
> @@ -4070,17 +4092,20 @@ void *QxlDevice::AllocMem(UINT32 mspace_type, size_t<br>
> size, BOOL force)<br>
> continue;<br>
> }<br>
><br>
> - if (force) {<br>
> + if (force && m_bActive) {<br>
> /* Ask spice to free some stuff */<br>
> + ReleaseMutex(&m_MemLock, locked);<br>
> WaitForReleaseRing();<br>
> + locked = WaitForObject(&m_MemLock, NULL);<br>
> } else {<br>
> /* Fail */<br>
> break;<br>
> }<br>
> }<br>
> +<br>
> ReleaseMutex(&m_MemLock, locked);<br>
><br>
> - ASSERT((!ptr && !force) || (ptr >= m_MSInfo[mspace_type].mspace_<wbr>start &&<br>
> + ASSERT((!ptr && (!force || !m_bActive)) || (ptr >=<br>
> m_MSInfo[mspace_type].mspace_<wbr>start &&<br>
> ptr <<br>
> m_MSInfo[mspace_type].mspace_<wbr>end));<br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("<---%s: ptr 0x%x\n", __FUNCTION__,<br>
> ptr));<br>
> return ptr;<br>
> @@ -4113,6 +4138,9 @@ QXLDrawable *QxlDevice::GetDrawable()<br>
> QXLOutput *output;<br>
><br>
> output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +<br>
> sizeof(QXLDrawable), TRUE);<br>
> + if (!output) {<br>
> + return NULL;<br>
> + }<br>
> output->num_res = 0;<br>
> RESOURCE_TYPE(output, RESOURCE_TYPE_DRAWABLE);<br>
> ((QXLDrawable *)output->data)-><a href="http://release_info.id" rel="noreferrer" target="_blank">release_info.<wbr>id</a> = (UINT64)output;<br>
> @@ -4128,6 +4156,9 @@ QXLCursorCmd *QxlDevice::CursorCmd()<br>
><br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));<br>
> output = (QXLOutput *)AllocMem(MSPACE_TYPE_VRAM, sizeof(QXLOutput) +<br>
> sizeof(QXLCursorCmd), TRUE);<br>
> + if (!output) {<br>
> + return NULL;<br>
> + }<br>
> output->num_res = 0;<br>
> RESOURCE_TYPE(output, RESOURCE_TYPE_CURSOR);<br>
> cursor_cmd = (QXLCursorCmd *)output->data;<br>
> @@ -4277,6 +4308,9 @@ QXLDrawable *QxlDevice::Drawable(UINT8 type, CONST RECT<br>
> *area, CONST RECT *clip,<br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("---> %s\n", __FUNCTION__));<br>
><br>
> drawable = GetDrawable();<br>
> + if (!drawable) {<br>
> + return NULL;<br>
> + }<br>
> drawable->surface_id = surface_id;<br>
> drawable->type = type;<br>
> drawable->effect = QXL_EFFECT_OPAQUE;<br>
> @@ -4420,6 +4454,11 @@ QXLDrawable *QxlDevice::PrepareBltBits (<br>
> alloc_size = BITMAP_ALLOC_BASE + BITS_BUF_MAX - BITS_BUF_MAX %<br>
> line_size;<br>
> alloc_size = MIN(BITMAP_ALLOC_BASE + height * line_size, alloc_size);<br>
> image_res = (Resource*)AllocMem(MSPACE_<wbr>TYPE_VRAM, alloc_size, TRUE);<br>
> + if (!image_res) {<br>
> + ReleaseOutput(drawable-><a href="http://release_info.id" rel="noreferrer" target="_blank">releas<wbr>e_info.id</a>);<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate bitmap for<br>
> drawable\n"));<br>
> + return NULL;<br>
> + }<br>
><br>
> image_res->refs = 1;<br>
> image_res->free = FreeBitmapImageEx;<br>
> @@ -4450,8 +4489,12 @@ QXLDrawable *QxlDevice::PrepareBltBits (<br>
> alloc_size = height * line_size;<br>
><br>
> for (; src != src_end; src -= pSrc->Pitch, alloc_size -= line_size) {<br>
> - PutBytesAlign(&chunk, &dest, &dest_end, src,<br>
> - line_size, alloc_size, line_size);<br>
> + if (!PutBytesAlign(&chunk, &dest, &dest_end, src,<br>
> + line_size, alloc_size, line_size)) {<br>
> + ReleaseOutput(drawable-><a href="http://release_info.id" rel="noreferrer" target="_blank">releas<wbr>e_info.id</a>);<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("Cannot allocate additional bitmap<br>
> for drawable\n"));<br>
> + return NULL;<br>
> + }<br>
> }<br>
><br>
> internal->image.bitmap.palette = 0;<br>
> @@ -4472,7 +4515,7 @@ QXLDrawable *QxlDevice::PrepareBltBits (<br>
> return drawable;<br>
> }<br>
><br>
> -VOID QxlDevice::PutBytesAlign(<wbr>QXLDataChunk **chunk_ptr, UINT8 **now_ptr,<br>
> +BOOLEAN QxlDevice::PutBytesAlign(<wbr>QXLDataChunk **chunk_ptr, UINT8 **now_ptr,<br>
> UINT8 **end_ptr, UINT8 *src, int size,<br>
> size_t alloc_size, uint32_t alignment)<br>
> {<br>
> @@ -4490,6 +4533,9 @@ VOID QxlDevice::PutBytesAlign(<wbr>QXLDataChunk **chunk_ptr,<br>
> UINT8 **now_ptr,<br>
> aligned_size -= aligned_size % alignment;<br>
><br>
> void *ptr = AllocMem(MSPACE_TYPE_VRAM, size +<br>
> sizeof(QXLDataChunk), TRUE);<br>
> + if (!ptr) {<br>
> + return FALSE;<br>
> + }<br>
> chunk->next_chunk = PA(ptr, m_SurfaceMemSlot);<br>
> ((QXLDataChunk *)ptr)->prev_chunk = PA(chunk, m_SurfaceMemSlot);<br>
> chunk = (QXLDataChunk *)ptr;<br>
> @@ -4510,6 +4556,7 @@ VOID QxlDevice::PutBytesAlign(<wbr>QXLDataChunk **chunk_ptr,<br>
> UINT8 **now_ptr,<br>
> *now_ptr = now;<br>
> *end_ptr = end;<br>
> DbgPrint(TRACE_LEVEL_VERBOSE, ("<--- %s\n", __FUNCTION__));<br>
> + return TRUE;<br>
> }<br>
><br>
> VOID QxlDevice::BlackOutScreen(<wbr>CURRENT_BDD_MODE* pCurrentBddMod)<br>
> @@ -4573,6 +4620,11 @@ NTSTATUS QxlDevice::SetPointerShape(_<wbr>In_ CONST<br>
> DXGKARG_SETPOINTERSHAPE* pSetPoi<br>
> int num_images = 1;<br>
><br>
> cursor_cmd = CursorCmd();<br>
> + if (!cursor_cmd) {<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor<br>
> command\n", __FUNCTION__));<br>
> + return STATUS_INSUFFICIENT_RESOURCES;<br>
> + }<br>
> +<br>
> cursor_cmd->type = QXL_CURSOR_SET;<br>
><br>
> cursor_cmd->u.set.visible = TRUE;<br>
> @@ -4580,6 +4632,12 @@ NTSTATUS QxlDevice::SetPointerShape(_<wbr>In_ CONST<br>
> DXGKARG_SETPOINTERSHAPE* pSetPoi<br>
> cursor_cmd->u.set.position.y = 0;<br>
><br>
> res = (Resource *)AllocMem(MSPACE_TYPE_VRAM, CURSOR_ALLOC_SIZE, TRUE);<br>
> + if (!res) {<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor data\n",<br>
> __FUNCTION__));<br>
> + ReleaseOutput(cursor_cmd-><a href="http://release_info.id" rel="noreferrer" target="_blank">rele<wbr>ase_info.id</a>);<br>
> + return STATUS_INSUFFICIENT_RESOURCES;<br>
> + }<br>
> +<br>
> res->refs = 1;<br>
> res->free = FreeCursorEx;<br>
> res->ptr = this;<br>
> @@ -4616,8 +4674,13 @@ NTSTATUS QxlDevice::SetPointerShape(_<wbr>In_ CONST<br>
> DXGKARG_SETPOINTERSHAPE* pSetPoi<br>
> end = (UINT8 *)res + CURSOR_ALLOC_SIZE;<br>
> src_end = src + (pSetPointerShape->Pitch * pSetPointerShape->Height *<br>
> num_images);<br>
> for (; src != src_end; src += pSetPointerShape->Pitch) {<br>
> - PutBytesAlign(&chunk, &now, &end, src, line_size,<br>
> - PAGE_SIZE, 1);<br>
> + if (!PutBytesAlign(&chunk, &now, &end, src, line_size,<br>
> + PAGE_SIZE, 1))<br>
> + {<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor<br>
> bitmap\n", __FUNCTION__));<br>
> + ReleaseOutput(cursor_cmd-><a href="http://release_info.id" rel="noreferrer" target="_blank">rele<wbr>ase_info.id</a>);<br>
> + return STATUS_INSUFFICIENT_RESOURCES;<br>
> + }<br>
> }<br>
> CursorCmdAddRes(cursor_cmd, res);<br>
> RELEASE_RES(res);<br>
> @@ -4638,6 +4701,11 @@ NTSTATUS QxlDevice::SetPointerPosition(<wbr>_In_ CONST<br>
> DXGKARG_SETPOINTERPOSITION* pS<br>
> pSetPointerPosition->X,<br>
> pSetPointerPosition->Y));<br>
> QXLCursorCmd *cursor_cmd = CursorCmd();<br>
> + if (!cursor_cmd) {<br>
> + DbgPrint(TRACE_LEVEL_ERROR, ("%s: Failed to allocate cursor<br>
> command\n", __FUNCTION__));<br>
> + return STATUS_INSUFFICIENT_RESOURCES;<br>
> + }<br>
> +<br>
> if (pSetPointerPosition->X < 0 || !pSetPointerPosition->Flags.<wbr>Visible) {<br>
> cursor_cmd->type = QXL_CURSOR_HIDE;<br>
> } else {<br>
> diff --git a/qxldod/QxlDod.h b/qxldod/QxlDod.h<br>
> index a1e9634..cbcd11e 100755<br>
> --- a/qxldod/QxlDod.h<br>
> +++ b/qxldod/QxlDod.h<br>
> @@ -601,7 +601,7 @@ private:<br>
> void PushCmd(void);<br>
> void WaitForCursorRing(void);<br>
> void PushCursor(void);<br>
> - void PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,<br>
> + BOOLEAN PutBytesAlign(QXLDataChunk **chunk_ptr, UINT8 **now_ptr,<br>
> UINT8 **end_ptr, UINT8 *src, int size,<br>
> size_t alloc_size, uint32_t alignment);<br>
> void AsyncIo(UCHAR Port, UCHAR Value);<br>
> @@ -671,6 +671,7 @@ private:<br>
><br>
> QXLPresentOnlyRing m_PresentRing[1];<br>
> HANDLE m_PresentThread;<br>
> + BOOLEAN m_bActive;<br>
> };<br>
><br>
> class QxlDod {<br></div></div></blockquote></div><br></div></div></blockquote><div><br></div></div></div></div></div></blockquote></div><br></div></div>