Re: [Qemu-devel] [PATCH 1/3] virtio: interface to enable/disable host notifiers

[Cornelia Huck <cornelia.huck@de.ibm.com>]

On Tue, 7 Apr 2015 12:45:56 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> generalize and move these from vhost to virtio.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/hw/virtio/virtio.h |  3 +++
>  hw/virtio/vhost.c          | 45 ++--------------------------------------
>  hw/virtio/virtio.c         | 51 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 56 insertions(+), 43 deletions(-)
> 

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 17c1260..e7ee069 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1283,6 +1283,57 @@ void virtio_device_set_child_bus_name(VirtIODevice *vdev, char *bus_name)
>      vdev->bus_name = g_strdup(bus_name);
>  }
> 
> +int virtio_enable_host_notifiers(VirtIODevice *vdev, int startvq, int nvqs)

I realize that you just move the function, but would it make sense to
verify that startvq + nvqs is actually still a valid queue?

> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> +    int i, r;
> +    if (!k->set_host_notifier) {
> +        fprintf(stderr, "binding does not support host notifiers\n");
> +        r = -ENOSYS;
> +        goto fail;
> +    }
> +
> +    for (i = 0; i < nvqs; ++i) {
> +        r = k->set_host_notifier(qbus->parent, startvq + i, true);
> +        if (r < 0) {
> +            fprintf(stderr, "vhost VQ %d notifier binding failed: %d\n", i, -r);

I think you'll want to get rid of the vhost in the message here. And
the number of the failing virtqueue is startvq+i, no?

Also: would it make sense to convert this to the error reporting
infrastructure?

> +            goto fail_vq;
> +        }
> +    }
> +
> +    return 0;
> +fail_vq:
> +    while (--i >= 0) {
> +        r = k->set_host_notifier(qbus->parent, startvq + i, false);
> +        if (r < 0) {
> +            fprintf(stderr, "vhost VQ %d notifier cleanup error: %d\n", i, -r);
> +            fflush(stderr);
> +        }
> +        assert (r >= 0);
> +    }
> +fail:
> +    return r;
> +}
> +
> +void virtio_disable_host_notifiers(VirtIODevice *vdev, int startvq, int nvqs)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> +    VirtioBusState *vbus = VIRTIO_BUS(qbus);
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(vbus);
> +    int i, r;
> +
> +    for (i = 0; i < nvqs; ++i) {
> +        r = k->set_host_notifier(qbus->parent, startvq + i, false);

Should a generic function maybe check for ->set_host_notifier and bail
out early if it doesn't exist? Probably depends on how the callers want
to handle non-existence of host notifiers.

> +        if (r < 0) {
> +            fprintf(stderr, "vhost VQ %d notifier cleanup failed: %d\n", i, -r);
> +            fflush(stderr);
> +        }
> +        assert (r >= 0);
> +    }
> +}
> +
>  static void virtio_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);