From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58535) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj3Jm-0003xl-CV for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:20:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aj3Jg-0000HA-8u for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:20:46 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50102) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj3Jf-0000H0-S7 for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:20:40 -0400 Date: Thu, 24 Mar 2016 13:20:34 +0200 From: "Michael S. Tsirkin" Message-ID: <20160324131510-mutt-send-email-mst@redhat.com> References: <1458208893-15949-1-git-send-email-cornelia.huck@de.ibm.com> <1458208893-15949-2-git-send-email-cornelia.huck@de.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1458208893-15949-2-git-send-email-cornelia.huck@de.ibm.com> Subject: Re: [Qemu-devel] [PATCH RFC 1/6] virtio-bus: common ioeventfd infrastructure List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Cornelia Huck Cc: pbonzini@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, borntraeger@de.ibm.com On Thu, Mar 17, 2016 at 11:01:28AM +0100, Cornelia Huck wrote: > Introduce a set of ioeventfd callbacks on the virtio-bus level > that can be implemented by the individual transports. At the > virtio-bus level, do common handling for host notifiers (which > is actually most of it). > > Two things of note: > - We always iterate over all possible virtio queues, even though > ccw (currently) has a lower limit. It does not really matter in > this place. > - We allow for the virtio-bus caller to pass an "assign" argument > down when stopping ioeventfd, which the old interface did not allow. > > Signed-off-by: Cornelia Huck So it sounds like this is supposed to fix races in current code, pls document how. > --- > hw/virtio/virtio-bus.c | 108 +++++++++++++++++++++++++++++++++++++++++ > include/hw/virtio/virtio-bus.h | 14 ++++++ > 2 files changed, 122 insertions(+) > > diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c > index 574f0e2..501300f 100644 > --- a/hw/virtio/virtio-bus.c > +++ b/hw/virtio/virtio-bus.c > @@ -146,6 +146,114 @@ void virtio_bus_set_vdev_config(VirtioBusState *bus, uint8_t *config) > } > } > > +static int set_host_notifier_internal(DeviceState *proxy, VirtioBusState *bus, > + int n, bool assign, bool set_handler) > +{ > + VirtIODevice *vdev = virtio_bus_get_device(bus); > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > + EventNotifier *notifier = virtio_queue_get_host_notifier(vq); > + int r = 0; > + > + if (assign) { > + r = event_notifier_init(notifier, 1); > + if (r < 0) { > + error_report("%s: unable to init event notifier: %d", __func__, r); > + return r; > + } > + virtio_queue_set_host_notifier_fd_handler(vq, true, set_handler); > + r = k->ioeventfd_assign(proxy, notifier, n, assign); > + if (r < 0) { > + error_report("%s: unable to assign ioeventfd: %d", __func__, r); > + virtio_queue_set_host_notifier_fd_handler(vq, false, false); > + event_notifier_cleanup(notifier); > + return r; > + } > + } else { > + virtio_queue_set_host_notifier_fd_handler(vq, false, false); > + k->ioeventfd_assign(proxy, notifier, n, assign); > + event_notifier_cleanup(notifier); > + } > + return r; > +} > + > +void virtio_bus_start_ioeventfd(VirtioBusState *bus) > +{ > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + VirtIODevice *vdev; > + int n, r; > + > + if (!k->ioeventfd_started || k->ioeventfd_started(proxy)) { > + return; > + } > + if (!k->ioeventfd_disabled(proxy)) { > + return; > + } > + vdev = virtio_bus_get_device(bus); > + for (n = 0; n < VIRTIO_QUEUE_MAX; n++) { > + if (!virtio_queue_get_num(vdev, n)) { > + continue; > + } > + r = set_host_notifier_internal(proxy, bus, n, true, true); > + if (r < 0) { > + goto assign_error; > + } > + } > + k->ioeventfd_set_started(proxy, true, false); > + return; > + > +assign_error: > + while (--n >= 0) { > + if (!virtio_queue_get_num(vdev, n)) { > + continue; > + } > + > + r = set_host_notifier_internal(proxy, bus, n, false, false); > + assert(r >= 0); > + } > + k->ioeventfd_set_started(proxy, false, true); > + error_report("%s: failed. Fallback to userspace (slower).", __func__); > +} > + > +void virtio_bus_stop_ioeventfd(VirtioBusState *bus, bool assign) > +{ > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + VirtIODevice *vdev; > + int n, r; > + > + if (!k->ioeventfd_started || !k->ioeventfd_started(proxy)) { > + return; > + } > + vdev = virtio_bus_get_device(bus); > + for (n = 0; n < VIRTIO_QUEUE_MAX; n++) { > + if (!virtio_queue_get_num(vdev, n)) { > + continue; > + } > + r = set_host_notifier_internal(proxy, bus, n, assign, false); > + assert(r >= 0); > + } > + k->ioeventfd_set_started(proxy, false, false); > +} > + > +int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign) Could you add documentation for what does "assign" mean here? It was there as an internal API, but now it's external so needs better docs. > +{ > + VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(bus); > + DeviceState *proxy = DEVICE(BUS(bus)->parent); > + > + if (!k->ioeventfd_started) { > + return -ENOSYS; > + } > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + k->ioeventfd_set_disabled(proxy, assign); > + if (assign) { > + virtio_bus_stop_ioeventfd(bus, assign); > + } Need comment to explain why don't we start on !assign. > + return set_host_notifier_internal(proxy, bus, n, assign, false); > +} > + > static char *virtio_bus_get_dev_path(DeviceState *dev) > { > BusState *bus = qdev_get_parent_bus(dev); > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index 3f2c136..0281cbf 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -71,6 +71,16 @@ typedef struct VirtioBusClass { > void (*device_unplugged)(DeviceState *d); > int (*query_nvectors)(DeviceState *d); > /* > + * ioeventfd handling: if the transport implements ioeventfd_started, > + * it must implement the other ioeventfd callbacks as well > + */ > + bool (*ioeventfd_started)(DeviceState *d); > + void (*ioeventfd_set_started)(DeviceState *d, bool started, bool err); > + bool (*ioeventfd_disabled)(DeviceState *d); > + void (*ioeventfd_set_disabled)(DeviceState *d, bool disabled); > + int (*ioeventfd_assign)(DeviceState *d, EventNotifier *notifier, > + int n, bool assign); > + /* > * Does the transport have variable vring alignment? > * (ie can it ever call virtio_queue_set_align()?) > * Note that changing this will break migration for this transport. > @@ -111,4 +121,8 @@ static inline VirtIODevice *virtio_bus_get_device(VirtioBusState *bus) > return (VirtIODevice *)qdev; > } > > +void virtio_bus_start_ioeventfd(VirtioBusState *bus); > +void virtio_bus_stop_ioeventfd(VirtioBusState *bus, bool assign); > +int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign); > + > #endif /* VIRTIO_BUS_H */ > -- > 2.6.5