Re: [Qemu-devel] [PATCH v2 1/3] virtio-mmio: introduce set_host_notifier()

[Cornelia Huck]

On Thu, 07 May 2015 13:48:20 +0300
Pavel Fedin <address@hidden> wrote:

> set_host_notifier() is introduced into virtio-mmio now. Most of codes came
> from virtio-pci.
> 
> Signed-off-by: Ying-Shiuan Pan <address@hidden>

Meta-comment #1:

This is almost certainly not the correct email address of the original
author :) I'd recommend to check another archive (e.g. marc.info) that
does not mangle the email addresses in the patch body. The original
author also should go into the author field in git.

> Signed-off-by: Pavel Fedin <address@hidden>
> ---
>  hw/virtio/virtio-mmio.c | 72
> +++++++++++++++++++++++++++++++++++++++++++++++++

Meta-comment #2:

Your patches are word-wrapped, which not only makes them harder to read
but impossible to feed to git am.

>  1 file changed, 72 insertions(+)
> 
(...)

> +static void virtio_mmio_stop_ioeventfd(VirtIOMMIOProxy *proxy)
> +{
> +    int r;
> +    int n;
> +    VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus);
> +
> +    if (!proxy->ioeventfd_started) {
> +        return;
> +    }
> +
> +    for (n = 0; n < VIRTIO_PCI_QUEUE_MAX; n++) {
> +        if (!virtio_queue_get_num(vdev, n)) {
> +            continue;
> +        }
> +
> +        r = virtio_mmio_set_host_notifier_internal(proxy, n, false, false);
> +        assert(r >= 0);
> +    }
> +    proxy->ioeventfd_started = false;
> +}
> +
>  static uint64_t virtio_mmio_read(void *opaque, hwaddr offset, unsigned
> size)
>  {
>      VirtIOMMIOProxy *proxy = (VirtIOMMIOProxy *)opaque;
> @@ -342,6 +394,25 @@ static void virtio_mmio_reset(DeviceState *d)
>      proxy->guest_page_shift = 0;
>  }
> 
> +static int virtio_mmio_set_host_notifier(DeviceState *opaque, int n,
> +                                         bool assign)
> +{
> +    VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque);
> +
> +    /* Stop using ioeventfd for virtqueue kick if the device starts using
> host
> +     * notifiers.  This makes it easy to avoid stepping on each others'
> toes.
> +     */
> +    proxy->ioeventfd_disabled = assign;
> +    if (assign) {
> +        virtio_mmio_stop_ioeventfd(proxy);
> +    }
> +    /* We don't need to start here: it's not needed because backend
> +     * currently only stops on status change away from ok,
> +     * reset, vmstop and such. If we do add code to start here,
> +     * need to check vmstate, device state etc. */
> +    return virtio_mmio_set_host_notifier_internal(proxy, n, assign, false);
> +}
> +

Much of this looks basically the same across any transport supporting
ioeventfds (pci, ccw and now mmio) - only actually setting up the
notifiers is transport-specific. A case for common helper functions?