| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <475F1E86.8000503@qumranet.com>
Date: Wed, 12 Dec 2007 01:34:30 +0200
From: Dor Laor <dor.laor@...il.com>
To: Christian Borntraeger <borntraeger@...ibm.com>
CC: Rusty Russell <rusty@...tcorp.com.au>,
kvm-devel <kvm-devel@...ts.sourceforge.net>,
netdev@...r.kernel.org, virtualization@...ts.linux-foundation.org
Subject: Re: [kvm-devel] [PATCH resent] virtio_net: Fix stalled inbound trafficon
early packets
Christian Borntraeger wrote:
> Am Dienstag, 11. Dezember 2007 schrieb Christian Borntraeger:
>
>>> The way other physical NICs doing it is by dis/en/abling interrupt
>>> using registers (look at e1000).
>>> I suggest we can export add_status and use the original code but
>>> before enabling napi add a call to add_status(dev,
>>> VIRTIO_CONFIG_DEV_OPEN).
>>> The host won't trigger an irq until it sees the above.
>>>
>> That would also work. We already have VRING_AVAIL_F_NO_INTERRUPT in
>> virtio_ring.c - maybe we can use that. Its hidden in callback and
>> restart handling, what about adding an explicit startup?
>>
>
> Ok, just to give an example what I thought about:
> ---
> drivers/block/virtio_blk.c | 3 ++-
> drivers/net/virtio_net.c | 2 ++
> drivers/virtio/virtio_ring.c | 16 +++++++++++++---
> include/linux/virtio.h | 5 +++++
> 4 files changed, 22 insertions(+), 4 deletions(-)
>
> Index: kvm/drivers/virtio/virtio_ring.c
> ===================================================================
> --- kvm.orig/drivers/virtio/virtio_ring.c
> +++ kvm/drivers/virtio/virtio_ring.c
> @@ -241,6 +241,16 @@ static bool vring_restart(struct virtque
> return true;
> }
>
> +static void vring_startup(struct virtqueue *_vq)
> +{
> + struct vring_virtqueue *vq = to_vvq(_vq);
> + START_USE(vq);
> + if (vq->vq.callback)
> + vq->vring.avail->flags &= ~VRING_AVAIL_F_NO_INTERRUPT;
> + END_USE(vq);
> +}
> +
> +
> irqreturn_t vring_interrupt(int irq, void *_vq)
> {
> struct vring_virtqueue *vq = to_vvq(_vq);
> @@ -265,6 +275,7 @@ static struct virtqueue_ops vring_vq_ops
> .get_buf = vring_get_buf,
> .kick = vring_kick,
> .restart = vring_restart,
> + .startup = vring_startup,
> .shutdown = vring_shutdown,
> };
>
> @@ -299,9 +310,8 @@ struct virtqueue *vring_new_virtqueue(un
> vq->in_use = false;
> #endif
>
> - /* No callback? Tell other side not to bother us. */
> - if (!callback)
> - vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
> + /* disable interrupts until we enable them */
> + vq->vring.avail->flags |= VRING_AVAIL_F_NO_INTERRUPT;
>
> /* Put everything in free lists. */
> vq->num_free = num;
> Index: kvm/include/linux/virtio.h
> ===================================================================
> --- kvm.orig/include/linux/virtio.h
> +++ kvm/include/linux/virtio.h
> @@ -45,6 +45,9 @@ struct virtqueue
> * vq: the struct virtqueue we're talking about.
> * This returns "false" (and doesn't re-enable) if there are pending
> * buffers in the queue, to avoid a race.
> + * @startup: enable callbacks
> + * vq: the struct virtqueue we're talking abount
> + * Returns 0 or an error
> * @shutdown: "unadd" all buffers.
> * vq: the struct virtqueue we're talking about.
> * Remove everything from the queue.
> @@ -67,6 +70,8 @@ struct virtqueue_ops {
>
> bool (*restart)(struct virtqueue *vq);
>
> + void (*startup) (struct virtqueue *vq);
> +
> void (*shutdown)(struct virtqueue *vq);
> };
>
> Index: kvm/drivers/net/virtio_net.c
> ===================================================================
> --- kvm.orig/drivers/net/virtio_net.c
> +++ kvm/drivers/net/virtio_net.c
> @@ -292,6 +292,8 @@ static int virtnet_open(struct net_devic
> return -ENOMEM;
>
> napi_enable(&vi->napi);
> +
> + vi->rvq->vq_ops->startup(vi->rvq);
> return 0;
> }
>
> Index: kvm/drivers/block/virtio_blk.c
> ===================================================================
> --- kvm.orig/drivers/block/virtio_blk.c
> +++ kvm/drivers/block/virtio_blk.c
> @@ -183,7 +183,8 @@ static int virtblk_probe(struct virtio_d
> err = PTR_ERR(vblk->vq);
> goto out_free_vblk;
> }
> -
> + /* enable interrupts */
> + vblk->vq->vq_ops->startup(vblk->vq);
> vblk->pool = mempool_create_kmalloc_pool(1,sizeof(struct virtblk_req));
> if (!vblk->pool) {
> err = -ENOMEM;
>
>
>
> There is still one small problem: what if the host fills up all
> host-to-guest buffers before we call startup? So I start to think that your
> solution is better, given that the host is not only not sending interrupts
>
This is why initially I suggested another status code in order to split
the ring logic with driver status.
> but also not filling any buffers as long as VIRTIO_CONFIG_DEV_OPEN is not
> set. I will have a look but I think that add_status needs to be called
>
It can fill the buffers even without dev_open, when the dev is finally
opened the host will issue an interrupt
if there are pending buffers. (I'm not sure it's worth solving, maybe
just drop them like you suggested).
> after napi_enable, otherwise we have the same race.
>
>
You're right, my mistake.
> Christian
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists