> On 8 Nov 2016, at 18:18, Paolo Bonzini <[email protected]> wrote: > > > > On 07/11/2016 18:23, Felipe Franciosi wrote: >> Following the recent refactor of virtio notfiers [1], more specifically >> the patch that uses virtio_bus_set_host_notifier [2] by default, core >> virtio code requires 'ioeventfd_started' to be set to true/false when >> the host notifiers are configured. Since vhost-scsi uses the legacy >> interface, this value is not updated. >> >> When booting a guest with a vhost-scsi backend controller, SeaBIOS will >> initially configure the device which sets all notifiers. The guest will >> continue to boot fine until the kernel virtio-scsi module reinitialises >> the device causing a stop followed by another start. Since >> ioeventfd_started was never set to true, the 'stop' operation triggered >> by virtio_bus_set_host_notifier() will not result in a call to >> virtio_pci_ioeventfd_assign(assign=false). This leaves the memory >> regions with stale notifiers and results on the next start triggering >> the following assertion: >> >> kvm_mem_ioeventfd_add: error adding ioeventfd: File exists >> Aborted >> >> This patch updates ioeventfd_started whenever the notifiers are set or >> cleared, fixing this issue. >> >> Signed-off-by: Felipe Franciosi <[email protected]> >> >> [1] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07748.html >> [2] http://lists.nongnu.org/archive/html/qemu-devel/2016-10/msg07760.html >> --- >> hw/scsi/vhost-scsi.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/hw/scsi/vhost-scsi.c b/hw/scsi/vhost-scsi.c >> index 5b26946..1c6e6d4 100644 >> --- a/hw/scsi/vhost-scsi.c >> +++ b/hw/scsi/vhost-scsi.c >> @@ -95,6 +95,7 @@ static int vhost_scsi_start(VHostSCSI *s) >> if (ret < 0) { >> return ret; >> } >> + VIRTIO_BUS(qbus)->ioeventfd_started = true; >> >> s->dev.acked_features = vdev->guest_features; >> ret = vhost_dev_start(&s->dev, vdev); >> @@ -152,6 +153,7 @@ static void vhost_scsi_stop(VHostSCSI *s) >> vhost_scsi_clear_endpoint(s); >> vhost_dev_stop(&s->dev, vdev); >> vhost_dev_disable_notifiers(&s->dev, vdev); >> + VIRTIO_BUS(qbus)->ioeventfd_started = false; >> } >> >> static uint64_t vhost_scsi_get_features(VirtIODevice *vdev, >> > > While a bit hacky, at least for 2.8 the idea should be fine. Only it > has to be done in vhost_dev_enable_notifiers and > vhost_dev_disable_notifiers, close to the calls to > virtio_device_stop_ioeventfd (i.e., set bus->ioeventfd_started after the > call) and virtio_device_start_ioeventfd (clear it before the call).
Ok. I'll send a v2 tomorrow which does this from vhost_dev_enable_notifiers(). I won't have time to test it now. > > I've now worked through a fix that uses start_ioeventfd/stop_ioeventfd, > and it does comes out nice (29 lines added, 65 removed :)), but it isn't > bisectable (so it has to be squashed into a single patch) and does not > cover vhost-net yet. So let's go with your patch for now. Perfect. Yeah as I told you on IRC I had a quick go at using the new start/stop, but it didn't seem straightforward and I ran out of time to do it properly. > > I'll send the vm_running fix separately, since that one is a real bugfix. Ok. Thanks for taking on that one. > > Paolo Cheers, Felipe
