On Thu, 24 Mar 2016 13:20:34 +0200
"Michael S. Tsirkin" <m...@redhat.com> 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 <cornelia.h...@de.ibm.com>
>
> 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
>
