[Qemu-devel] [PATCH v2 1/2] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Qemu-devel] [PATCH v2 1/2] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Yonit Halperin]

if qxl_send_events was called from spice server context, and then
migration had completed before a call to pipe_read, the target
guest qxl driver didn't get the interrupt. In addition,
qxl_send_events ignored further interrupts of the same kind, since
ram->int_pending was set. As a result, the guest driver was stacked
or very slow (when the waiting for the interrupt was with timeout).
---
 hw/qxl.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index b34bccf..c7edc60 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -1362,7 +1362,6 @@ static void pipe_read(void *opaque)
     qxl_set_irq(d);
 }
 
-/* called from spice server thread context only */
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
 {
     uint32_t old_pending;
@@ -1463,7 +1462,13 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
     PCIQXLDevice *qxl = opaque;
     qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
 
-    if (!running && qxl->mode == QXL_MODE_NATIVE) {
+    if (running) {
+        /*
+         * if qxl_send_events was called from spice server context before
+         * migration ended, qxl_set_irq for these events might not have been called
+         */
+         qxl_set_irq(qxl);
+    } else if (qxl->mode == QXL_MODE_NATIVE) {
         /* dirty all vram (which holds surfaces) and devram (primary surface)
          * to make sure they are saved */
         /* FIXME #1: should go out during "live" stage */
-- 
1.7.4.4

[Qemu-devel] [PATCH v2 2/2] qxl: s/qxl_set_irq/qxl_update_irq/

[Yonit Halperin]

---
 hw/qxl.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/hw/qxl.c b/hw/qxl.c
index c7edc60..3ddca88 100644
--- a/hw/qxl.c
+++ b/hw/qxl.c
@@ -808,7 +808,7 @@ static void qxl_exit_vga_mode(PCIQXLDevice *d)
     qxl_destroy_primary(d, QXL_SYNC);
 }
 
-static void qxl_set_irq(PCIQXLDevice *d)
+static void qxl_update_irq(PCIQXLDevice *d)
 {
     uint32_t pending = le32_to_cpu(d->ram->int_pending);
     uint32_t mask    = le32_to_cpu(d->ram->int_mask);
@@ -1209,7 +1209,7 @@ async_common:
         qemu_spice_wakeup(&d->ssd);
         break;
     case QXL_IO_UPDATE_IRQ:
-        qxl_set_irq(d);
+        qxl_update_irq(d);
         break;
     case QXL_IO_NOTIFY_OOM:
         if (!SPICE_RING_IS_EMPTY(&d->ram->release_ring)) {
@@ -1359,7 +1359,7 @@ static void pipe_read(void *opaque)
     do {
         len = read(d->pipe[0], &dummy, sizeof(dummy));
     } while (len == sizeof(dummy));
-    qxl_set_irq(d);
+    qxl_update_irq(d);
 }
 
 static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
@@ -1373,7 +1373,7 @@ static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
         return;
     }
     if (pthread_self() == d->main) {
-        qxl_set_irq(d);
+        qxl_update_irq(d);
     } else {
         if (write(d->pipe[1], d, 1) != 1) {
             dprint(d, 1, "%s: write to pipe failed\n", __FUNCTION__);
@@ -1465,9 +1465,9 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
     if (running) {
         /*
          * if qxl_send_events was called from spice server context before
-         * migration ended, qxl_set_irq for these events might not have been called
+         * migration ended, qxl_update_irq for these events might not have been called
          */
-         qxl_set_irq(qxl);
+         qxl_update_irq(qxl);
     } else if (qxl->mode == QXL_MODE_NATIVE) {
         /* dirty all vram (which holds surfaces) and devram (primary surface)
          * to make sure they are saved */
-- 
1.7.4.4

Re: [Qemu-devel] [PATCH v2 1/2] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Gerd Hoffmann]

On 08/31/11 14:37, Yonit Halperin wrote:
> if qxl_send_events was called from spice server context, and then
> migration had completed before a call to pipe_read, the target
> guest qxl driver didn't get the interrupt. In addition,
> qxl_send_events ignored further interrupts of the same kind, since
> ram->int_pending was set. As a result, the guest driver was stacked
> or very slow (when the waiting for the interrupt was with timeout).

Looks fine except for this:

=== checkpatch complains ===
WARNING: line over 80 characters
#22: FILE: hw/qxl.c:1468:
+         * migration ended, qxl_set_irq for these events might not have 
been called

cheers,
   Gerd

PS: /me suggests to check out
http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html

Re: [Qemu-devel] [PATCH v2 1/2] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Michael S. Tsirkin]

On Wed, Aug 31, 2011 at 03:37:33PM +0300, Yonit Halperin wrote:
> if qxl_send_events was called from spice server context, and then
> migration had completed before a call to pipe_read, the target
> guest qxl driver didn't get the interrupt.

This is a general issue with interrupt migration, and PCI core has code
to handle this, migrating interrupts.  So rather than work around this
in qxl I'd like us to first understand whether there really exists such
a problem, since if yes it would affect other devices.

Could you help with that please?

> In addition,
> qxl_send_events ignored further interrupts of the same kind, since
> ram->int_pending was set.

Maybe this is the only issue?
A way to check would be to call
    uint32_t pending = le32_to_cpu(d->ram->int_pending);
    uint32_t mask    = le32_to_cpu(d->ram->int_mask);
    int level = !!(pending & mask);
    qxl_ring_set_dirty(d);

instead of qxl_set_irq, and see if that is enough.

Note: I don't object to reusing qxl_set_irq in
production, just let us make sure we don't hide bugs.

> As a result, the guest driver was stacked
> or very slow (when the waiting for the interrupt was with timeout).

You need to sign off :)

> ---
>  hw/qxl.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/qxl.c b/hw/qxl.c
> index b34bccf..c7edc60 100644
> --- a/hw/qxl.c
> +++ b/hw/qxl.c
> @@ -1362,7 +1362,6 @@ static void pipe_read(void *opaque)
>      qxl_set_irq(d);
>  }
>  
> -/* called from spice server thread context only */
>  static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>  {
>      uint32_t old_pending;
> @@ -1463,7 +1462,13 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
>      PCIQXLDevice *qxl = opaque;
>      qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
>  
> -    if (!running && qxl->mode == QXL_MODE_NATIVE) {
> +    if (running) {
> +        /*
> +         * if qxl_send_events was called from spice server context before
> +         * migration ended, qxl_set_irq for these events might not have been called
> +         */
> +         qxl_set_irq(qxl);
> +    } else if (qxl->mode == QXL_MODE_NATIVE) {
>          /* dirty all vram (which holds surfaces) and devram (primary surface)
>           * to make sure they are saved */
>          /* FIXME #1: should go out during "live" stage */
> -- 
> 1.7.4.4
> 

Re: [Qemu-devel] [PATCH v2 1/2] qxl: send interrupt after migration in case ram->int_pending != 0, RHBZ #732949

[Yonit Halperin]

On 09/01/2011 10:36 PM, Michael S. Tsirkin wrote:
> On Wed, Aug 31, 2011 at 03:37:33PM +0300, Yonit Halperin wrote:
>> if qxl_send_events was called from spice server context, and then
>> migration had completed before a call to pipe_read, the target
>> guest qxl driver didn't get the interrupt.
>
> This is a general issue with interrupt migration, and PCI core has code
> to handle this, migrating interrupts.  So rather than work around this
> in qxl I'd like us to first understand whether there really exists such
> a problem, since if yes it would affect other devices.
>
> Could you help with that please?
>
I think this issue is spice-specific: the problem is that when a 
spice_server thread issues a request for interrupt, the request is 
passed to the qemu thread through a pipe. This pipe status is not saved 
during migration. Thus, any pending interrupt request are purged when 
migration completes.
>> In addition,
>> qxl_send_events ignored further interrupts of the same kind, since
>> ram->int_pending was set.
>
> Maybe this is the only issue?
> A way to check would be to call
>      uint32_t pending = le32_to_cpu(d->ram->int_pending);
>      uint32_t mask    = le32_to_cpu(d->ram->int_mask);
>      int level = !!(pending&  mask);
>      qxl_ring_set_dirty(d);
>
> instead of qxl_set_irq, and see if that is enough.
>
I was talking about the check in qxl_send_events
> Note: I don't object to reusing qxl_set_irq in
> production, just let us make sure we don't hide bugs.
>
>> As a result, the guest driver was stacked
>> or very slow (when the waiting for the interrupt was with timeout).
>
> You need to sign off :)
>
>> ---
>>   hw/qxl.c |    9 +++++++--
>>   1 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/qxl.c b/hw/qxl.c
>> index b34bccf..c7edc60 100644
>> --- a/hw/qxl.c
>> +++ b/hw/qxl.c
>> @@ -1362,7 +1362,6 @@ static void pipe_read(void *opaque)
>>       qxl_set_irq(d);
>>   }
>>
>> -/* called from spice server thread context only */
>>   static void qxl_send_events(PCIQXLDevice *d, uint32_t events)
>>   {
>>       uint32_t old_pending;
>> @@ -1463,7 +1462,13 @@ static void qxl_vm_change_state_handler(void *opaque, int running, int reason)
>>       PCIQXLDevice *qxl = opaque;
>>       qemu_spice_vm_change_state_handler(&qxl->ssd, running, reason);
>>
>> -    if (!running&&  qxl->mode == QXL_MODE_NATIVE) {
>> +    if (running) {
>> +        /*
>> +         * if qxl_send_events was called from spice server context before
>> +         * migration ended, qxl_set_irq for these events might not have been called
>> +         */
>> +         qxl_set_irq(qxl);
>> +    } else if (qxl->mode == QXL_MODE_NATIVE) {
>>           /* dirty all vram (which holds surfaces) and devram (primary surface)
>>            * to make sure they are saved */
>>           /* FIXME #1: should go out during "live" stage */
>> --
>> 1.7.4.4
>>