Rusty Russell wrote:

On Mon, 2007-09-24 at 15:44 +0200, Dor Laor wrote:
> Rusty Russell wrote:
> > +irqreturn_t vring_interrupt(int irq, void *_vq)
> > +{
> > +       struct vring_virtqueue *vq = to_vvq(_vq);
> > +
> > +       pr_debug("virtqueue interrupt for %p\n", vq);
> > +
> > +       if (unlikely(vq->broken))
> > +               return IRQ_HANDLED;
> > +
> > +       if (more_used(vq)) {
> > +               pr_debug("virtqueue callback for %p (%p)\n",
> > +                        vq, vq->vq.callback);
> > +               if (!vq->vq.callback)
> > +                       return IRQ_NONE;
> > +               if (!vq->vq.callback(&vq->vq))
> > +                       vq->vring.avail->flags |=
> > VRING_AVAIL_F_NO_INTERRUPT;
> > +       } else
> > +               pr_debug("virtqueue %p no more used\n", vq);
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> >
> Seems like there is a problem with shared irq line, the interrupt
> handler always returns IRQ_HANDLED (except for the trivial case
> were the callback is null).

To reply for a second time, this time with thought.  Sorry.

Yes, this code is wrong.  It should be:

irqreturn_t vring_interrupt(int irq, void *_vq)
{
        struct vring_virtqueue *vq = to_vvq(_vq);

        if (!more_used(vq)) {
                pr_debug("virtqueue interrupt with no work for %p\n", vq);
                return IRQ_NONE;
        }

        if (unlikely(vq->broken))
                return IRQ_HANDLED;

        pr_debug("virtqueue callback for %p (%p)\n", vq, vq->vq.callback);
        if (vq->vq.callback && !vq->vq.callback(&vq->vq))
                vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;

        return IRQ_HANDLED;
}

Cheers,
Rusty.

At the moment it's not good enough, there is a potential race were the guest optimistically turn off the VRING_AVAIL_F_NO_INTERRUPT in the vring_restart and afterwards finds there are more_used so
it consume them in the poll function.
If in the middle, packets arrive the host will see the flag is off and send irq.
In that case the above irq handler will report IRQ_NONE.

It's also not trivial to cancel the optimistic approach in the restart function since then there might be another race
that will result in missing irq reaching the guest.
I'll try to think of something better than a hypercall for switching irq on/off.
Cheers mate,
Dor.
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel

Reply via email to