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

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

Posted by Li Feng 3 years, 2 months ago

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 <fengli@smartx.com>
---
 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;
     }
 
-- 
2.31.1

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Gerd Hoffmann 3 years, 2 months ago

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:
> 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.

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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Li Feng 3 years, 2 months ago

Thanks,
Feng Li

Gerd Hoffmann <kraxel@redhat.com> 于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:
> > 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.
>
> 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
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Gerd Hoffmann 3 years, 2 months ago

  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.html#x1-930001

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

_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org

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

Posted by Li Feng 3 years, 2 months ago

Thanks,
Feng Li

Gerd Hoffmann <kraxel@redhat.com> 于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.html#x1-930001
>
> 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
>
_______________________________________________
SeaBIOS mailing list -- seabios@seabios.org
To unsubscribe send an email to seabios-leave@seabios.org