| Message ID | 1478539385-11249-1-git-send-email-felipe@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, > On 7 Nov 2016, at 18:23, Felipe Franciosi <felipe@nutanix.com> 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 <felipe@nutanix.com> > > [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; I'm not sure if it's safe to do this from vhost_dev_disable/enable_notifiers() directly. If you'd rather this is fixed there, please let me know and I'll send a v2. This e-mail is basically to flag a problem which I recently came across when working on vhost-user-scsi. Basically my code stopped working and I didn't know why, so I rolled back to test a traditional vhost-scsi on master and found this. I have also not tested vhost-sock, but I imagine it might suffer from the same issue (even if it doesn't manifest itself this easily). Any other cases you can think of? Thanks, Felipe > > 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, > -- > 1.9.4 >
On 07/11/2016 18:26, Felipe Franciosi wrote: > > @@ -95,6 +95,7 @@ static int vhost_scsi_start(VHostSCSI *s) > > if (ret < 0) { > > return ret; > > } > > + VIRTIO_BUS(qbus)->ioeventfd_started = true; > > I'm not sure if it's safe to do this from > vhost_dev_disable/enable_notifiers() directly. If you'd rather this > is fixed there, please let me know and I'll send a v2. This e-mail is > basically to flag a problem which I recently came across when working > on vhost-user-scsi. Basically my code stopped working and I didn't > know why, so I rolled back to test a traditional vhost-scsi on master > and found this. > > I have also not tested vhost-sock, but I imagine it might suffer from > the same issue (even if it doesn't manifest itself this easily). Any > other cases you can think of? Hi Felipe, can you try overriding start_ioeventfd and stop_ioeventfd (like ad07cd69ecaffbaa015459a46975ab32e50df805 for regular virtio-scsi), so that they point to vhost_scsi_start and vhost_scsi_stop? You should not even need vhost_scsi_set_status anymore. I'm not sure however why vhost-vsock checks vdev->vm_running, but otherwise the same should apply to vhost-vsock as well. Thanks, Paolo
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 <felipe@nutanix.com> > > [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). 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. I'll send the vm_running fix separately, since that one is a real bugfix. Paolo
> On 8 Nov 2016, at 18:18, Paolo Bonzini <pbonzini@redhat.com> 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 <felipe@nutanix.com> >> >> [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
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,
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 <felipe@nutanix.com> [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(+)