Re: [Qemu-devel] [PULL 12/47] virtio: remove ioeventfd_disabled altogether

[Paolo Bonzini]

On 10/11/2016 15:35, Christian Borntraeger wrote:
> On 10/30/2016 10:23 PM, Michael S. Tsirkin wrote:
>> From: Paolo Bonzini <address@hidden>
>>
>> Now that there is not anymore a switch from the generic ioeventfd handler
>> to the dataplane handler, virtio_bus_set_host_notifier(assign=true) is
>> always called with !bus->ioeventfd_started, hence virtio_bus_stop_ioeventfd
>> does nothing in this case.  Move the invocation to vhost.c, which is the
>> only place that needs it.
>>
>> Reviewed-by: Cornelia Huck <address@hidden>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> Reviewed-by: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>> ---
>>  include/hw/virtio/virtio-bus.h |  6 ------
>>  hw/virtio/vhost.c              |  3 +++
>>  hw/virtio/virtio-bus.c         | 23 ++++++++---------------
>>  3 files changed, 11 insertions(+), 21 deletions(-)
> 
> This breaks vhost-net for s390/kvm after rebooting the guest. (ping fails and
> ifconfig shows no packets is TXed)
> 
> Any idea?

Patch from Felipe:
[PATCH v2] vhost: Update 'ioeventfd_started' with host notifiers

Paolo

>>
>> diff --git a/include/hw/virtio/virtio-bus.h b/include/hw/virtio/virtio-bus.h
>> index af6b5c4..cbdf745 100644
>> --- a/include/hw/virtio/virtio-bus.h
>> +++ b/include/hw/virtio/virtio-bus.h
>> @@ -94,12 +94,6 @@ struct VirtioBusState {
>>      BusState parent_obj;
>>
>>      /*
>> -     * Set if the default ioeventfd handlers are disabled by vhost
>> -     * or dataplane.
>> -     */
>> -    bool ioeventfd_disabled;
>> -
>> -    /*
>>       * Set if ioeventfd has been started.
>>       */
>>      bool ioeventfd_started;
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 501a5f4..131f164 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -1196,6 +1196,7 @@ int vhost_dev_enable_notifiers(struct vhost_dev *hdev, 
>> VirtIODevice *vdev)
>>          goto fail;
>>      }
>>
>> +    virtio_device_stop_ioeventfd(vdev);
>>      for (i = 0; i < hdev->nvqs; ++i) {
>>          r = virtio_bus_set_host_notifier(VIRTIO_BUS(qbus), hdev->vq_index + 
>> i,
>>                                           true);
>> @@ -1215,6 +1216,7 @@ fail_vq:
>>          }
>>          assert (e >= 0);
>>      }
>> +    virtio_device_start_ioeventfd(vdev);
>>  fail:
>>      return r;
>>  }
>> @@ -1237,6 +1239,7 @@ void vhost_dev_disable_notifiers(struct vhost_dev 
>> *hdev, VirtIODevice *vdev)
>>          }
>>          assert (r >= 0);
>>      }
>> +    virtio_device_start_ioeventfd(vdev);
>>  }
>>
>>  /* Test and clear event pending status.
>> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
>> index a619445..b0e4544 100644
>> --- a/hw/virtio/virtio-bus.c
>> +++ b/hw/virtio/virtio-bus.c
>> @@ -190,7 +190,7 @@ int virtio_bus_start_ioeventfd(VirtioBusState *bus)
>>      if (!k->ioeventfd_assign || !k->ioeventfd_enabled(proxy)) {
>>          return -ENOSYS;
>>      }
>> -    if (bus->ioeventfd_started || bus->ioeventfd_disabled) {
>> +    if (bus->ioeventfd_started) {
>>          return 0;
>>      }
>>      r = vdc->start_ioeventfd(vdev);
>> @@ -226,8 +226,8 @@ bool virtio_bus_ioeventfd_enabled(VirtioBusState *bus)
>>  }
>>
>>  /*
>> - * This function switches from/to the generic ioeventfd handler.
>> - * assign==false means 'use generic ioeventfd handler'.
>> + * This function switches ioeventfd on/off in the device.
>> + * The caller must set or clear the handlers for the EventNotifier.
>>   */
>>  int virtio_bus_set_host_notifier(VirtioBusState *bus, int n, bool assign)
>>  {
>> @@ -237,19 +237,12 @@ int virtio_bus_set_host_notifier(VirtioBusState *bus, 
>> int n, bool assign)
>>      if (!k->ioeventfd_assign) {
>>          return -ENOSYS;
>>      }
>> -    bus->ioeventfd_disabled = assign;
>>      if (assign) {
>> -        /*
>> -         * Stop using the generic ioeventfd, we are doing eventfd handling
>> -         * ourselves below
>> -         *
>> -         * FIXME: We should just switch the handler and not 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_bus_stop_ioeventfd(bus);
>> +        assert(!bus->ioeventfd_started);
>> +    } else {
>> +        if (!bus->ioeventfd_started) {
>> +            return 0;
>> +        }
>>      }
>>      return set_host_notifier_internal(proxy, bus, n, assign);
>>  }
>>
>