[PATCH] virtio-scsi: initialize the ctrl and event vq

[Li Feng]

Currently, virtio-scsi doesn't support any control or event message, this patch
add the basic initialization.

Some backends need this feature, like dpdk.

Signed-off-by: Li Feng 
---
 src/hw/virtio-scsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
index 369c981..e16d4b0 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -203,8 +203,18 @@ init_virtio_scsi(void *data)
         }
     }

+    if (vp_find_vq(vp, 0, &vq) < 0 ) {
+        dprintf(1, "fail to find ctrl vq for virtio-scsi %pP\n", pci);
+        goto fail;
+    }
+
+    if (vp_find_vq(vp, 1, &vq) < 0 ) {
+        dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
+        goto fail;
+    }
+
     if (vp_find_vq(vp, 2, &vq) < 0 ) {
-        dprintf(1, "fail to find vq for virtio-scsi %pP\n", pci);
+        dprintf(1, "fail to find req vq for virtio-scsi %pP\n", pci);
         goto fail;
     }

[Paul Menzel]

Dear Li,

Thank you for your contribution.

Am 31.08.21 um 12:49 schrieb Li Feng:
> Currently, virtio-scsi doesn't support any control or event message, this patch
> add the basic initialization.

add*s* or imperative mood: …, so add the basic initialization.
> Some backends need this feature, like dpdk.

It’d be great, if you added more details how to test it.
> Signed-off-by: Li Feng
>
> src/hw/virtio-scsi.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
> index 369c981..e16d4b0 100644
> --- a/src/hw/virtio-scsi.c
> +++ b/src/hw/virtio-scsi.c
> @@ -203,8 +203,18 @@ init_virtio_scsi(void *data)
>           }
>       }
> if (vp_find_vq(vp, 0, &vq) < 0 ) {
>    dprintf(1, "fail to find ctrl vq for virtio-scsi %pP\n", pci);
>    goto fail;
> }
> if (vp_find_vq(vp, 1, &vq) < 0 ) {
>    dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
>    goto fail;
> }
> if (vp_find_vq(vp, 2, &vq) < 0 ) {
>    dprintf(1, "fail to find vq for virtio-scsi %pP\n", pci);
>    dprintf(1, "fail to find req vq for virtio-scsi %pP\n", pci);
>     goto fail;
> }

Kind regards,

Paul

[Li Feng]

Thanks,
Feng Li

Paul Menzel  于2021年8月31日周二 下午6:57写道：
> Dear Li,
>
> Thank you for your contribution.
>
> Am 31.08.21 um 12:49 schrieb Li Feng:
> > Currently, virtio-scsi doesn't support any control or event message, this patch
> > add the basic initialization.
>
> add*s* or imperative mood: …, so add the basic initialization.
> > Some backends need this feature, like dpdk.
>
> It’d be great, if you added more details how to test it.

Thanks for your fast reply, I will send the v2 patch with a better
commit message.
> > Signed-off-by: Li Feng
> >
> > src/hw/virtio-scsi.c | 12 +++++++++++-
> >   1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
> > index 369c981..e16d4b0 100644
> > --- a/src/hw/virtio-scsi.c
> > +++ b/src/hw/virtio-scsi.c
> > @@ -203,8 +203,18 @@ init_virtio_scsi(void *data)
> >           }
> >       }
> > if (vp_find_vq(vp, 0, &vq) < 0 ) {
> >    dprintf(1, "fail to find ctrl vq for virtio-scsi %pP\n", pci);
> >    goto fail;
> > }
> > if (vp_find_vq(vp, 1, &vq) < 0 ) {
> >    dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
> >    goto fail;
> > }
> > if (vp_find_vq(vp, 2, &vq) < 0 ) {
> >    dprintf(1, "fail to find vq for virtio-scsi %pP\n", pci);
> >    dprintf(1, "fail to find req vq for virtio-scsi %pP\n", pci);
> >     goto fail;
> > }
>
> Kind regards,
>
> Paul

[PATCH v2] virtio-scsi: initialize the ctrl and event vq

[Li Feng]

Currently, virtio-scsi doesn't support any control or event message, this patch
adds the basic initialization.

Some backends need this feature, like dpdk/spdk vhost-user backend.

Reproduce:
1. Start spdk vhost-user-scsi backend;
2. Create a vm with a SCSI vhost-user-scsi disk and start the vm;
3. Mount an iso to the vm without an OS;
3. Stop the vhost-user-scsi backend;
5. Vhost-user-scsi backend will crash before exiting when cleaning the resources.

The reason is that the seabios virtio-scsi only initializes the req vq,
then the spdk/dpdk treats it a not good session, it's hard to handle this tricky
issue from dpdk architecture.

Signed-off-by: Li Feng 
---
 src/hw/virtio-scsi.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/hw/virtio-scsi.c b/src/hw/virtio-scsi.c
index 369c981..e16d4b0 100644
--- a/src/hw/virtio-scsi.c
+++ b/src/hw/virtio-scsi.c
@@ -203,8 +203,18 @@ init_virtio_scsi(void *data)
         }
     }

+    if (vp_find_vq(vp, 0, &vq) < 0 ) {
+        dprintf(1, "fail to find ctrl vq for virtio-scsi %pP\n", pci);
+        goto fail;
+    }
+
+    if (vp_find_vq(vp, 1, &vq) < 0 ) {
+        dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
+        goto fail;
+    }
+
     if (vp_find_vq(vp, 2, &vq) < 0 ) {
-        dprintf(1, "fail to find vq for virtio-scsi %pP\n", pci);
+        dprintf(1, "fail to find req vq for virtio-scsi %pP\n", pci);
         goto fail;
     }

[PATCH v2] virtio-scsi: initialize the ctrl and event vq

[Gerd Hoffmann]

On Tue, Aug 31, 2021 at 08:23:39PM +0800, Li Feng wrote:
> Currently, virtio-scsi doesn't support any control or event message, this patch
> adds the basic initialization.
>
> Some backends need this feature, like dpdk/spdk vhost-user backend.
>
> Reproduce:
> Start spdk vhost-user-scsi backend;
> Create a vm with a SCSI vhost-user-scsi disk and start the vm;
> Mount an iso to the vm without an OS;
> Stop the vhost-user-scsi backend;
> Vhost-user-scsi backend will crash before exiting when cleaning the resources.

IMHO this must be fixed in vhost-user-scsi no matter what.  Host
processes crashing in case the guest doesn't behave as expected
is a security problem.
> The reason is that the seabios virtio-scsi only initializes the req vq,
> then the spdk/dpdk treats it a not good session, it's hard to handle this tricky
> issue from dpdk architecture.

What is the exact problem here?  vhost-user-scsi trying to send a
hotplug event (when adding the iso, probably as virtual cdrom) and
seabios not listening?
> if (vp_find_vq(vp, 1, &vq) < 0 ) {
>    dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
>    goto fail;
> }

That alone doesn't allow receiving events (no recv buffers added).  Also
note that seabios does not support hotplugging devices, so the only
thing we could do with those events is to throw them away.

take care,
  Gerd

[PATCH v2] virtio-scsi: initialize the ctrl and event vq

[Li Feng]

Thanks,
Feng Li

Gerd Hoffmann  于2021年8月31日周二 下午8:59写道：
> On Tue, Aug 31, 2021 at 08:23:39PM +0800, Li Feng wrote:
> > Currently, virtio-scsi doesn't support any control or event message, this patch
> > adds the basic initialization.
> >
> > Some backends need this feature, like dpdk/spdk vhost-user backend.
> >
> > Reproduce:
> > Start spdk vhost-user-scsi backend;
> > Create a vm with a SCSI vhost-user-scsi disk and start the vm;
> > Mount an iso to the vm without an OS;
> > Stop the vhost-user-scsi backend;
> > Vhost-user-scsi backend will crash before exiting when cleaning the resources.
>
> IMHO this must be fixed in vhost-user-scsi no matter what.  Host
> processes crashing in case the guest doesn't behave as expected
> is a security problem.

Yes, the backend should be fixed.
However, we may as well initialize these two vqs, because other virtio
frontend drivers have implemented these
as I know. It's good for compatibility.
> > The reason is that the seabios virtio-scsi only initializes the req vq,
> > then the spdk/dpdk treats it a not good session, it's hard to handle this tricky
> > issue from dpdk architecture.
>
> What is the exact problem here?  vhost-user-scsi trying to send a
> hotplug event (when adding the iso, probably as virtual cdrom) and
> seabios not listening?

No, hotplug is another question I should handle.
This crash is that some vring(controlq, eventq) aren't initialized
done from vhost-user aspect.
 And spdk/dpdk handles incoming events from theses queues, but the
vhost-user device isn't RUNNING
 status, becase some vqs are not ready, so the destruction is bad.
> > if (vp_find_vq(vp, 1, &vq) < 0 ) {
> >    dprintf(1, "fail to find event vq for virtio-scsi %pP\n", pci);
> >    goto fail;
> > }
>
> That alone doesn't allow receiving events (no recv buffers added).  Also
> note that seabios does not support hotplugging devices, so the only
> thing we could do with those events is to throw them away.

Yes, from the spec, I should put some recv buffers here, so events
could be written here.
Here just initialized it, then qemu vhost-user device will send the
VHOST_USER_KICK to drive the vhost-user backend.

BTW, do you accept a future patch that implements the virtio SCSI hotplug?

Thanks.
> take care,
>   Gerd

[PATCH v2] virtio-scsi: initialize the ctrl and event vq

[Gerd Hoffmann]

Hi,
> > IMHO this must be fixed in vhost-user-scsi no matter what.  Host
> > processes crashing in case the guest doesn't behave as expected
> > is a security problem.
>
> Yes, the backend should be fixed.
> However, we may as well initialize these two vqs, because other virtio
> frontend drivers have implemented these
> as I know. It's good for compatibility.

Is this required by the virtio spec?  I don't think so.
> This crash is that some vring(controlq, eventq) aren't initialized
> done from vhost-user aspect.
>  And spdk/dpdk handles incoming events from theses queues, but the
> vhost-user device isn't RUNNING
>  status, becase some vqs are not ready, so the destruction is bad.

The device should look at the status bits.

https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.h...

When the driver sets DRIVER_OK it is done setting up virtqueues.
When some of them are not ready the driver apparently doesn't want
use them.
> BTW, do you accept a future patch that implements the virtio SCSI hotplug?

Well, the problem with hotplug is that the BIOS interfaces have been
created in the 1980-ies and are not designed with hotplug in mind.  So
retrofitting hotplug support into that is a rather hard problem and the
benefits are questionable given that the BIOS typically runs only for a
very short time, before the real operating system takes over control.

take care,
  Gerd

[PATCH v2] virtio-scsi: initialize the ctrl and event vq

[Li Feng]

Thanks,
Feng Li

Gerd Hoffmann  于2021年9月1日周三 下午4:16写道：
> Hi,
> > > IMHO this must be fixed in vhost-user-scsi no matter what.  Host
> > > processes crashing in case the guest doesn't behave as expected
> > > is a security problem.
> >
> > Yes, the backend should be fixed.
> > However, we may as well initialize these two vqs, because other virtio
> > frontend drivers have implemented these
> > as I know. It's good for compatibility.
>
> Is this required by the virtio spec?  I don't think so.

Yes, the spec don't require this.
> > This crash is that some vring(controlq, eventq) aren't initialized
> > done from vhost-user aspect.
> >  And spdk/dpdk handles incoming events from theses queues, but the
> > vhost-user device isn't RUNNING
> >  status, becase some vqs are not ready, so the destruction is bad.
>
> The device should look at the status bits.
>
> https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.h...
>
> When the driver sets DRIVER_OK it is done setting up virtqueues.
> When some of them are not ready the driver apparently doesn't want
> use them.
> > BTW, do you accept a future patch that implements the virtio SCSI hotplug?
>
> Well, the problem with hotplug is that the BIOS interfaces have been
> created in the 1980-ies and are not designed with hotplug in mind.  So
> retrofitting hotplug support into that is a rather hard problem and the
> benefits are questionable given that the BIOS typically runs only for a
> very short time, before the real operating system takes over control.

Thanks for you to give me some background of this.
> take care,
>   Gerd