From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:60825) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj3TL-0000Fu-9q for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:30:43 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1aj3TF-0002Z7-Qx for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:30:39 -0400 Received: from e06smtp09.uk.ibm.com ([195.75.94.105]:55351) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1aj3TF-0002Yg-CZ for qemu-devel@nongnu.org; Thu, 24 Mar 2016 07:30:33 -0400 Received: from localhost by e06smtp09.uk.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 24 Mar 2016 11:30:32 -0000 Received: from b06cxnps3074.portsmouth.uk.ibm.com (d06relay09.portsmouth.uk.ibm.com [9.149.109.194]) by d06dlp01.portsmouth.uk.ibm.com (Postfix) with ESMTP id 8E1B317D805A for ; Thu, 24 Mar 2016 11:31:05 +0000 (GMT) Received: from d06av04.portsmouth.uk.ibm.com (d06av04.portsmouth.uk.ibm.com [9.149.37.216]) by b06cxnps3074.portsmouth.uk.ibm.com (8.14.9/8.14.9/NCO v10.0) with ESMTP id u2OBUUrH3277276 for ; Thu, 24 Mar 2016 11:30:30 GMT Received: from d06av04.portsmouth.uk.ibm.com (localhost [127.0.0.1]) by d06av04.portsmouth.uk.ibm.com (8.14.4/8.14.4/NCO v10.0 AVout) with ESMTP id u2OBUT2b028054 for ; Thu, 24 Mar 2016 05:30:29 -0600 Date: Thu, 24 Mar 2016 12:30:26 +0100 From: Cornelia Huck Message-ID: <20160324123026.1d4ae460.cornelia.huck@de.ibm.com> In-Reply-To: <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> <20160324131510-mutt-send-email-mst@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, famz@redhat.com, qemu-devel@nongnu.org, stefanha@redhat.com, borntraeger@de.ibm.com On Thu, 24 Mar 2016 13:20:34 +0200 "Michael S. Tsirkin" wrote: > 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. Yes, final version will have more description + comments. > > > > --- > > 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; > > +} Note that I'm currently trying to disentangle ioeventfd registration and handler assignment, so this will look a bit different in the final patch. > > + > > +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)) { And this one is actually the wrong way around, as mentioned in the other thread. Will be fixed. > > + 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. On my list. > > > +{ > > + 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. That's a part I'm currently reworking anyway, as we don't want to try to assign ioeventfds twice, but just keep them. It will be hopefully clearer then. > > > + 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); I've also added a short description for the callbacks in the meantime. > > + /* > > * 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 >