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