Re: [Qemu-devel] [PATCH 2/2] qxl: add QXL_IO_UPDATE_MEM for guest S3&S4 support

[yhalperi <yhalperi@redhat.com>]

On 06/27/2011 11:16 AM, Alon Levy wrote:
> On Mon, Jun 27, 2011 at 09:28:45AM +0300, yhalperi wrote:
>> On 06/26/2011 08:47 PM, Alon Levy wrote:
>>> On Sun, Jun 26, 2011 at 12:59:06PM -0400, Yonit Halperin wrote:
>>>> Sorry for the late response, wasn't available.
>>>> I'm afraid  that (1) and (2) will indeed wakeup the worker, but will not assure emptying the command ring, as it depends on the client pipe size.
>>>>
>>>
>>> I actually can't figure out what wakeup does (that's what both NOTIFY and
>>> NOTIFY_CURSOR do, see hw/qxl.c).
>> It turns on an event the worker is waiting for on epoll_wait.
> Ah, ok. So it will only read up to the pipe size like you said.
>
>>
>> What we did in prepare_to_sleep before was
>>> call flush_all_qxl_commands, which reads the command and cursor rings until
>>> they are empty (calling flush_cursor_commands and flush_display_commands), waiting
>>> whenever the pipe is too large. (avoiding this wait still needs to be done, but
>>> after ensuring suspend is correct).
>>>
>>> More to the point this flush is done from handle_dev_destroy_surfaces, but
>>> this is not good enough since this also destroys the surfaces, before we have
>>> a chance to actually render the surfaces.
>>>
>>> Perhaps RED_WORKER_MESSAGE_STOP should do a flush_all_qxl_commands?
>>>
>> We can do it as long as it doesn't affect migration - does STOP
>> happens before or after savevm? If it happens after - we can't touch
>> the command ring, i.e., we can't call flush. And even if it happens
>> before - do we really want to call flush during migration and
>> presumably slow it down?
> But if we don't then the client connected before migration doesn't get some of
> the messages it was supposed to get.
I think it will receive them after migration, since the command ring 
was stored.
>
> stop is called before hw/qxl.c:qxl_pre_save, from
> ui/spice-display.c:qemu_spice_vm_change_state_handler
>
>
>>
>> Cheers,
>> Yonit.
>>
>>>>
>>>> ----- Original Message -----
>>>> From: "Alon Levy"<alevy@redhat.com>
>>>> To: "Gerd Hoffmann"<kraxel@redhat.com>
>>>> Cc: qemu-devel@nongnu.org, yhalperi@redhat.com
>>>> Sent: Wednesday, June 22, 2011 11:57:54 AM
>>>> Subject: Re: [Qemu-devel] [PATCH 2/2] qxl: add QXL_IO_UPDATE_MEM for guest S3&S4 support
>>>>
>>>> On Wed, Jun 22, 2011 at 11:13:19AM +0200, Gerd Hoffmann wrote:
>>>>>    Hi,
>>>>>
>>>>>>> worker call.  We can add a I/O command to ask qxl to push the
>>>>>>> release queue head to the release ring.
>>>>>>
>>>>>> So you suggest to replace QXL_IO_UPDATE_MEM with what, two io commands instead
>>>>>> of using the val parameter?
>>>>>
>>>>> I'd like to (a) avoid updating the libspice-server API if possible
>>>>> and (b) have one I/O command for each logical step.  So going into
>>>>> S3 could look like this:
>>>>>
>>>>>    (0) stop putting new commands into the rings
>>>>>    (1) QXL_IO_NOTIFY_CMD
>>>>>    (2) QXL_IO_NOTIFY_CURSOR
>>>>>        qxl calls notify(), to make the worker thread empty the command
>>>>>        rings before it processes the next dispatcher request.
>>>>>    (3) QXl_IO_FLUSH_SURFACES (to be implemented)
>>>>>        qxl calls stop()+start(), spice-server renders all surfaces,
>>>>>        thereby flushing state to device memory.
>>>>>    (4) QXL_IO_DESTROY_ALL_SURFACES
>>>>>        zap surfaces
>>>>>    (5) QXL_IO_FLUSH_RELEASE (to be implemented)
>>>>>        push release queue head into the release ring, so the guest
>>>>>        will see it and can release everything.
>>>>>
>>>>> (1)+(2)+(4) exist already.
>>>>> (3)+(5) can be done without libspice-server changes.
>>>>>
>>>>> Looks workable?
>>>>
>>>> yeah. Yonit?
>>>>
>>>>>
>>>>> cheers,
>>>>>    Gerd
>>>>>
>>>>>
>>>>
>>
>>