On 29/03/2016 16:17, Cornelia Huck wrote: > The ->set_host_notifier() callback is invoked whenever we want to > switch from or to the generic ioeventfd handler. Currently, all > transports deregister the ioeventfd backing and then re-register > it. This opens a race window where we are without ioeventfd > backing for a time period: In the virtio-blk dataplane case, we > observed notifications coming in from both the vcpu thread and > the iothread. > > Let's change pci, mmio and ccw to keep the ioeventfd during > ->set_host_notifier() and only switch the ioeventfd handler. > > Signed-off-by: Cornelia Huck <cornelia.h...@de.ibm.com> > --- > hw/s390x/virtio-ccw.c | 22 +++++++++++++++++----- > hw/virtio/virtio-mmio.c | 27 +++++++++++++++++---------- > hw/virtio/virtio-pci.c | 28 ++++++++++++++++++---------- > include/hw/virtio/virtio-bus.h | 4 ++++ > 4 files changed, 56 insertions(+), 25 deletions(-) > > diff --git a/hw/s390x/virtio-ccw.c b/hw/s390x/virtio-ccw.c > index cb887ba..7b1088e 100644 > --- a/hw/s390x/virtio-ccw.c > +++ b/hw/s390x/virtio-ccw.c > @@ -1196,14 +1196,26 @@ static bool > virtio_ccw_query_guest_notifiers(DeviceState *d) > static int virtio_ccw_set_host_notifier(DeviceState *d, int n, bool assign) > { > VirtioCcwDevice *dev = VIRTIO_CCW_DEVICE(d); > + VirtIODevice *vdev = virtio_bus_get_device(&dev->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > > - /* Stop using the generic ioeventfd, we are doing eventfd handling > - * ourselves below */ > - dev->ioeventfd_disabled = assign; > if (assign) { > - virtio_ccw_stop_ioeventfd(dev); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + dev->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + dev->ioeventfd_disabled = false; > } > - return virtio_ccw_set_guest2host_notifier(dev, n, assign, false); > + return 0; > } > > static int virtio_ccw_get_mappings(VirtioCcwDevice *dev) > diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c > index d4cd91f..aafebdf 100644 > --- a/hw/virtio/virtio-mmio.c > +++ b/hw/virtio/virtio-mmio.c > @@ -502,19 +502,26 @@ static int virtio_mmio_set_host_notifier(DeviceState > *opaque, int n, > bool assign) > { > VirtIOMMIOProxy *proxy = VIRTIO_MMIO(opaque); > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > > - /* 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); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + proxy->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + proxy->ioeventfd_disabled = false; > } > - /* 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); > + return 0; > } > > /* virtio-mmio device */ > diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c > index 0dadb66..a91c1e8 100644 > --- a/hw/virtio/virtio-pci.c > +++ b/hw/virtio/virtio-pci.c > @@ -1115,18 +1115,26 @@ static int virtio_pci_set_host_notifier(DeviceState > *d, int n, bool assign) > { > VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d); > > - /* 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; > + VirtIODevice *vdev = virtio_bus_get_device(&proxy->bus); > + VirtQueue *vq = virtio_get_queue(vdev, n); > + > if (assign) { > - virtio_pci_stop_ioeventfd(proxy); > + /* Stop using the generic ioeventfd, we are doing eventfd handling > + * ourselves below */ > + proxy->ioeventfd_disabled = true; > + }; > + /* > + * Just switch the handler, don't deassign the ioeventfd. > + * Otherwise, there's a window where we don't have an > + * ioeventfd and we may end up with a notification where > + * we don't expect one. > + */ > + virtio_queue_set_host_notifier_fd_handler(vq, assign, !assign); > + if (!assign) { > + /* Use generic ioeventfd handler again. */ > + proxy->ioeventfd_disabled = false; > } > - /* 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_pci_set_host_notifier_internal(proxy, n, assign, false); > + return 0; > } > > static void virtio_pci_vmstate_change(DeviceState *d, bool running) > diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h > index 3f2c136..98c660a 100644 > --- a/include/hw/virtio/virtio-bus.h > +++ b/include/hw/virtio/virtio-bus.h > @@ -52,6 +52,10 @@ typedef struct VirtioBusClass { > bool (*has_extra_state)(DeviceState *d); > bool (*query_guest_notifiers)(DeviceState *d); > int (*set_guest_notifiers)(DeviceState *d, int nvqs, bool assign); > + /* > + * Switch from/to the generic ioeventfd handler. > + * assigned==false means 'use generic ioeventfd handler'. > + */ > int (*set_host_notifier)(DeviceState *d, int n, bool assigned); > void (*vmstate_change)(DeviceState *d, bool running); > /* >
Tested-by: Paolo Bonzini <pbonz...@redhat.com> (survives about 15 minutes of reboot loops) Paolo