[Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Felipe Franciosi]

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,
-- 
1.9.4

Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Felipe Franciosi]

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
> 

Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH] vhost-scsi: Update 'ioeventfd_started' with host notifiers

[Felipe Franciosi]

> 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