diff mbox

[RFC,v2,09/10] virtio-scsi-dataplane: Code to run virtio-scsi on iothread

Message ID 1407303308-4615-10-git-send-email-famz@redhat.com
State New
Headers show

Commit Message

Fam Zheng Aug. 6, 2014, 5:35 a.m. UTC
This implements the core part of dataplane feature of virtio-scsi.

A few fields are added in VirtIOSCSICommon to maintain the dataplane
status. These fields are managed by a new source file:
virtio-scsi-dataplane.c.

Most code in this file will run on an iothread, unless otherwise
commented as in a global mutex context, such as those functions to
start, stop and setting the iothread property.

Upon start, we set up guest/host event notifiers, in a same way as
virtio-blk does. The handlers then pop request from vring and call into
virtio-scsi.c functions to process it. So we need to make sure make all
those called functions work with iothread, too.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/scsi/Makefile.objs           |   2 +-
 hw/scsi/virtio-scsi-dataplane.c | 219 ++++++++++++++++++++++++++++++++++++++++
 include/hw/virtio/virtio-scsi.h |  19 ++++
 3 files changed, 239 insertions(+), 1 deletion(-)
 create mode 100644 hw/scsi/virtio-scsi-dataplane.c

Comments

Paolo Bonzini Aug. 6, 2014, 8:45 a.m. UTC | #1
Il 06/08/2014 07:35, Fam Zheng ha scritto:
>  ifeq ($(CONFIG_VIRTIO),y)
> -obj-y += virtio-scsi.o
> +obj-y += virtio-scsi.o virtio-scsi-dataplane.o
>  obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o
>  endif

I first thought that this must be conditional on 
CONFIG_VIRTIO_BLK_DATA_PLANE.  However, CONFIG_VIRTIO_BLK_DATA_PLANE is 
itself obsolete:

##########################################
# adjust virtio-blk-data-plane based on linux-aio

if test "$virtio_blk_data_plane" = "yes" -a \
        "$linux_aio" != "yes" ; then
  error_exit "virtio-blk-data-plane requires Linux AIO, please try --enable-linux-aio"
elif test -z "$virtio_blk_data_plane" ; then
  virtio_blk_data_plane=$linux_aio
fi

and there's no requirement to have Linux AIO anymore.  Can you prepare a
patch to drop CONFIG_VIRTIO_BLK_DATA_PLANE, and replace patch 1 with it?

We can leave --disable-virtio-blk-data-plane and --enable-virtio-blk-data-plane
in for a couple of releases.

Paolo
Fam Zheng Aug. 6, 2014, 9:07 a.m. UTC | #2
On Wed, 08/06 10:45, Paolo Bonzini wrote:
> Il 06/08/2014 07:35, Fam Zheng ha scritto:
> >  ifeq ($(CONFIG_VIRTIO),y)
> > -obj-y += virtio-scsi.o
> > +obj-y += virtio-scsi.o virtio-scsi-dataplane.o
> >  obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o
> >  endif
> 
> I first thought that this must be conditional on 
> CONFIG_VIRTIO_BLK_DATA_PLANE.  However, CONFIG_VIRTIO_BLK_DATA_PLANE is 
> itself obsolete:
> 
> ##########################################
> # adjust virtio-blk-data-plane based on linux-aio
> 
> if test "$virtio_blk_data_plane" = "yes" -a \
>         "$linux_aio" != "yes" ; then
>   error_exit "virtio-blk-data-plane requires Linux AIO, please try --enable-linux-aio"
> elif test -z "$virtio_blk_data_plane" ; then
>   virtio_blk_data_plane=$linux_aio
> fi
> 
> and there's no requirement to have Linux AIO anymore.  Can you prepare a
> patch to drop CONFIG_VIRTIO_BLK_DATA_PLANE, and replace patch 1 with it?
> 
> We can leave --disable-virtio-blk-data-plane and --enable-virtio-blk-data-plane
> in for a couple of releases.
> 

Yes. Sounds a good idea.

Fam
Paolo Bonzini Sept. 19, 2014, 9:29 a.m. UTC | #3
Il 06/08/2014 07:35, Fam Zheng ha scritto:
> This implements the core part of dataplane feature of virtio-scsi.
> 
> A few fields are added in VirtIOSCSICommon to maintain the dataplane
> status. These fields are managed by a new source file:
> virtio-scsi-dataplane.c.
> 
> Most code in this file will run on an iothread, unless otherwise
> commented as in a global mutex context, such as those functions to
> start, stop and setting the iothread property.
> 
> Upon start, we set up guest/host event notifiers, in a same way as
> virtio-blk does. The handlers then pop request from vring and call into
> virtio-scsi.c functions to process it. So we need to make sure make all
> those called functions work with iothread, too.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/scsi/Makefile.objs           |   2 +-
>  hw/scsi/virtio-scsi-dataplane.c | 219 ++++++++++++++++++++++++++++++++++++++++
>  include/hw/virtio/virtio-scsi.h |  19 ++++
>  3 files changed, 239 insertions(+), 1 deletion(-)
>  create mode 100644 hw/scsi/virtio-scsi-dataplane.c
> 
> diff --git a/hw/scsi/Makefile.objs b/hw/scsi/Makefile.objs
> index 121ddc5..40c79d3 100644
> --- a/hw/scsi/Makefile.objs
> +++ b/hw/scsi/Makefile.objs
> @@ -8,6 +8,6 @@ common-obj-$(CONFIG_ESP_PCI) += esp-pci.o
>  obj-$(CONFIG_PSERIES) += spapr_vscsi.o
>  
>  ifeq ($(CONFIG_VIRTIO),y)
> -obj-y += virtio-scsi.o
> +obj-y += virtio-scsi.o virtio-scsi-dataplane.o
>  obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o
>  endif
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> new file mode 100644
> index 0000000..d077b67
> --- /dev/null
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -0,0 +1,219 @@
> +/*
> + * Virtio SCSI dataplane
> + *
> + * Copyright Red Hat, Inc. 2014
> + *
> + * Authors:
> + *   Fam Zheng <famz@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw/virtio/virtio-scsi.h"
> +#include "qemu/error-report.h"
> +#include <hw/scsi/scsi.h>
> +#include <block/scsi.h>
> +#include <hw/virtio/virtio-bus.h>
> +#include "hw/virtio/virtio-access.h"
> +#include "stdio.h"
> +
> +/* Context: QEMU global mutex held */
> +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    s->ctx = iothread_get_aio_context(s->conf.iothread);

assert that it's NULL?
> +
> +    /* Don't try if transport does not support notifiers. */
> +    if (!k->set_guest_notifiers || !k->set_host_notifier) {
> +        fprintf(stderr, "virtio-scsi: Failed to set iothread "
> +                   "(transport does not support notifiers)");
> +        exit(1);
> +    }
> +}
> +
> +static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSICommon *s,
> +                                               VirtQueue *vq,
> +                                               EventNotifierHandler *handler,
> +                                               int n)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    VirtIOSCSIVring *r = g_slice_new(VirtIOSCSIVring);
> +
> +    /* Set up virtqueue notify */
> +    if (k->set_host_notifier(qbus->parent, n, true) != 0) {
> +        fprintf(stderr, "virtio-scsi: Failed to set host notifier\n");
> +        exit(1);
> +    }
> +    r->host_notifier = *virtio_queue_get_host_notifier(vq);
> +    r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
> +    aio_set_event_notifier(s->ctx, &r->host_notifier, handler);
> +
> +    r->parent = s;
> +
> +    if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
> +        fprintf(stderr, "virtio-scsi: VRing setup failed\n");
> +        exit(1);
> +    }
> +    return r;
> +}
> +
> +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
> +                                         VirtIOSCSIVring *vring)
> +{
> +    VirtIOSCSIReq *req = virtio_scsi_init_req(s, NULL);
> +    int r;
> +
> +    req->vring = vring;
> +    r = vring_pop((VirtIODevice *)s, &vring->vring, &req->elem);
> +    if (r < 0) {
> +        virtio_scsi_free_req(req);
> +        req = NULL;
> +    }
> +    return req;
> +}
> +
> +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
> +{
> +    vring_push(&req->vring->vring, &req->elem,
> +               req->qsgl.size + req->resp_iov.size);
> +    event_notifier_set(&req->vring->guest_notifier);
> +}
> +
> +static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier)
> +{
> +    VirtIOSCSIVring *vring = container_of(notifier,
> +                                          VirtIOSCSIVring, host_notifier);
> +    VirtIOSCSI *s = VIRTIO_SCSI(vring->parent);
> +    VirtIOSCSIReq *req;
> +
> +    event_notifier_test_and_clear(notifier);
> +    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
> +        virtio_scsi_handle_ctrl_req(s, req);
> +    }
> +}
> +
> +static void virtio_scsi_iothread_handle_event(EventNotifier *notifier)
> +{
> +    VirtIOSCSIVring *vring = container_of(notifier,
> +                                          VirtIOSCSIVring, host_notifier);
> +    VirtIOSCSICommon *vs = vring->parent;
> +    VirtIOSCSI *s = VIRTIO_SCSI(vs);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +
> +    event_notifier_test_and_clear(notifier);
> +
> +    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
> +        return;
> +    }
> +
> +    if (s->events_dropped) {
> +        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
> +    }
> +}
> +
> +static void virtio_scsi_iothread_handle_cmd(EventNotifier *notifier)
> +{
> +    VirtIOSCSIVring *vring = container_of(notifier,
> +                                          VirtIOSCSIVring, host_notifier);
> +    VirtIOSCSI *s = VIRTIO_SCSI(vring->parent);
> +    VirtIOSCSIReq *req;
> +
> +    event_notifier_test_and_clear(notifier);
> +    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
> +        virtio_scsi_handle_cmd_req(s, req);
> +    }

Perhaps add bdrv_io_plug/bdrv_io_unplug?

> +}
> +
> +/* Context: QEMU global mutex held */

> +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s)
> +{
> +    int i;
> +    int rc;
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +
> +    if (s->dataplane_started ||
> +        s->dataplane_starting ||
> +        s->ctx != iothread_get_aio_context(s->conf.iothread)) {
> +        return;
> +    }
> +
> +    s->dataplane_starting = true;

Please do a bdrv_drain_all() here.  Then you can set the contexts for
all children here too (for the hotplug case: in virtio_scsi_hotplug).

Then you do not have to do it in virtio_scsi_do_tmf and
virtio_scsi_handle_cmd_req.

> +    /* Set up guest notifier (irq) */
> +    rc = k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, true);
> +    if (rc != 0) {
> +        fprintf(stderr, "virtio-scsi: Failed to set guest notifiers, "
> +                "ensure -enable-kvm is set\n");
> +        exit(1);
> +    }
> +
> +    aio_context_acquire(s->ctx);
> +    s->ctrl_vring = virtio_scsi_vring_init(s, s->ctrl_vq,
> +                                           virtio_scsi_iothread_handle_ctrl,
> +                                           0);
> +    s->event_vring = virtio_scsi_vring_init(s, s->event_vq,
> +                                            virtio_scsi_iothread_handle_event,
> +                                            1);
> +    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * s->conf.num_queues);
> +    for (i = 0; i < s->conf.num_queues; i++) {
> +        s->cmd_vrings[i] =
> +            virtio_scsi_vring_init(s, s->cmd_vqs[i],
> +                                   virtio_scsi_iothread_handle_cmd,
> +                                   i + 2);
> +    }
> +
> +    aio_context_release(s->ctx);
> +    s->dataplane_starting = false;
> +    s->dataplane_started = true;
> +}
> +
> +/* Context: QEMU global mutex held */
> +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s)
> +{
> +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> +    int i;
> +
> +    if (!s->dataplane_started || s->dataplane_stopping) {
> +        return;
> +    }
> +    s->dataplane_stopping = true;
> +    assert(s->ctx == iothread_get_aio_context(s->conf.iothread));
> +
> +    aio_context_acquire(s->ctx);
> +
> +    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
> +    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
> +    for (i = 0; i < s->conf.num_queues; i++) {
> +        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
> +    }
> +
> +    bdrv_drain_all(); /* ensure there are no in-flight requests */
> +
> +    aio_context_release(s->ctx);
> +
> +    /* Sync vring state back to virtqueue so that non-dataplane request
> +     * processing can continue when we disable the host notifier below.
> +     */
> +    vring_teardown(&s->ctrl_vring->vring, vdev, 0);
> +    vring_teardown(&s->event_vring->vring, vdev, 1);
> +    for (i = 0; i < s->conf.num_queues; i++) {
> +        vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
> +    }
> +
> +    for (i = 0; i < s->conf.num_queues + 2; i++) {
> +        k->set_host_notifier(qbus->parent, i, false);
> +    }
> +
> +    /* Clean up guest notifier (irq) */
> +    k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, false);
> +    s->dataplane_stopping = false;
> +    s->dataplane_started = false;
> +}
> diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> index 6f92c29..b9f2197 100644
> --- a/include/hw/virtio/virtio-scsi.h
> +++ b/include/hw/virtio/virtio-scsi.h
> @@ -174,6 +174,18 @@ typedef struct VirtIOSCSICommon {
>      VirtQueue *ctrl_vq;
>      VirtQueue *event_vq;
>      VirtQueue **cmd_vqs;
> +
> +    /* Fields for dataplane below */
> +    AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
> +
> +    /* Vring is used instead of vq in dataplane code, because of the underlying
> +     * memory layer thread safety */
> +    VirtIOSCSIVring *ctrl_vring;
> +    VirtIOSCSIVring *event_vring;
> +    VirtIOSCSIVring **cmd_vrings;
> +    bool dataplane_started;
> +    bool dataplane_starting;
> +    bool dataplane_stopping;

Please add these to VirtIOSCSI rather than VirtIOSCSICommon.  Same for
the new functions you declare below.

Paolo

>  } VirtIOSCSICommon;
>  
>  typedef struct {
> @@ -239,4 +251,11 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req);
>  void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
>                              uint32_t event, uint32_t reason);
>  
> +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread);
> +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s);
> +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s);
> +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
> +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
> +                                         VirtIOSCSIVring *vring);
> +
>  #endif /* _QEMU_VIRTIO_SCSI_H */
>
Fam Zheng Sept. 22, 2014, 5:56 a.m. UTC | #4
On Fri, 09/19 11:29, Paolo Bonzini wrote:
> Il 06/08/2014 07:35, Fam Zheng ha scritto:
> > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> > index 6f92c29..b9f2197 100644
> > --- a/include/hw/virtio/virtio-scsi.h
> > +++ b/include/hw/virtio/virtio-scsi.h
> > @@ -174,6 +174,18 @@ typedef struct VirtIOSCSICommon {
> >      VirtQueue *ctrl_vq;
> >      VirtQueue *event_vq;
> >      VirtQueue **cmd_vqs;
> > +
> > +    /* Fields for dataplane below */
> > +    AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
> > +
> > +    /* Vring is used instead of vq in dataplane code, because of the underlying
> > +     * memory layer thread safety */
> > +    VirtIOSCSIVring *ctrl_vring;
> > +    VirtIOSCSIVring *event_vring;
> > +    VirtIOSCSIVring **cmd_vrings;
> > +    bool dataplane_started;
> > +    bool dataplane_starting;
> > +    bool dataplane_stopping;
> 
> Please add these to VirtIOSCSI rather than VirtIOSCSICommon.  Same for
> the new functions you declare below.

What's the rationale, please? Asking because especially the VirtIOSCSIVring
fields are the dataplane counterparts of VirtQueue fields, so putting in
VirtIOSCSI seems unnatural for me.

Fam
Fam Zheng Sept. 22, 2014, 6:14 a.m. UTC | #5
On Fri, 09/19 11:29, Paolo Bonzini wrote:
> Il 06/08/2014 07:35, Fam Zheng ha scritto:
> > +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s)
> > +{
> > +    int i;
> > +    int rc;
> > +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> > +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > +
> > +    if (s->dataplane_started ||
> > +        s->dataplane_starting ||
> > +        s->ctx != iothread_get_aio_context(s->conf.iothread)) {
> > +        return;
> > +    }
> > +
> > +    s->dataplane_starting = true;
> 
> Please do a bdrv_drain_all() here.  Then you can set the contexts for
> all children here too (for the hotplug case: in virtio_scsi_hotplug).
> 
> Then you do not have to do it in virtio_scsi_do_tmf and
> virtio_scsi_handle_cmd_req.

Do we want multiple iothreads in the future? If yes, the aio context will need
to be checked/set for each command, that's why I put it in virtio_scsi_do_tmf
and virtio_scsi_handle_cmd_req.

Fam

> 
> > +    /* Set up guest notifier (irq) */
> > +    rc = k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, true);
> > +    if (rc != 0) {
> > +        fprintf(stderr, "virtio-scsi: Failed to set guest notifiers, "
> > +                "ensure -enable-kvm is set\n");
> > +        exit(1);
> > +    }
> > +
> > +    aio_context_acquire(s->ctx);
> > +    s->ctrl_vring = virtio_scsi_vring_init(s, s->ctrl_vq,
> > +                                           virtio_scsi_iothread_handle_ctrl,
> > +                                           0);
> > +    s->event_vring = virtio_scsi_vring_init(s, s->event_vq,
> > +                                            virtio_scsi_iothread_handle_event,
> > +                                            1);
> > +    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * s->conf.num_queues);
> > +    for (i = 0; i < s->conf.num_queues; i++) {
> > +        s->cmd_vrings[i] =
> > +            virtio_scsi_vring_init(s, s->cmd_vqs[i],
> > +                                   virtio_scsi_iothread_handle_cmd,
> > +                                   i + 2);
> > +    }
> > +
> > +    aio_context_release(s->ctx);
> > +    s->dataplane_starting = false;
> > +    s->dataplane_started = true;
> > +}
> > +
> > +/* Context: QEMU global mutex held */
> > +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s)
> > +{
> > +    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
> > +    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> > +    VirtIODevice *vdev = VIRTIO_DEVICE(s);
> > +    int i;
> > +
> > +    if (!s->dataplane_started || s->dataplane_stopping) {
> > +        return;
> > +    }
> > +    s->dataplane_stopping = true;
> > +    assert(s->ctx == iothread_get_aio_context(s->conf.iothread));
> > +
> > +    aio_context_acquire(s->ctx);
> > +
> > +    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
> > +    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
> > +    for (i = 0; i < s->conf.num_queues; i++) {
> > +        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
> > +    }
> > +
> > +    bdrv_drain_all(); /* ensure there are no in-flight requests */
> > +
> > +    aio_context_release(s->ctx);
> > +
> > +    /* Sync vring state back to virtqueue so that non-dataplane request
> > +     * processing can continue when we disable the host notifier below.
> > +     */
> > +    vring_teardown(&s->ctrl_vring->vring, vdev, 0);
> > +    vring_teardown(&s->event_vring->vring, vdev, 1);
> > +    for (i = 0; i < s->conf.num_queues; i++) {
> > +        vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
> > +    }
> > +
> > +    for (i = 0; i < s->conf.num_queues + 2; i++) {
> > +        k->set_host_notifier(qbus->parent, i, false);
> > +    }
> > +
> > +    /* Clean up guest notifier (irq) */
> > +    k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, false);
> > +    s->dataplane_stopping = false;
> > +    s->dataplane_started = false;
> > +}
> > diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
> > index 6f92c29..b9f2197 100644
> > --- a/include/hw/virtio/virtio-scsi.h
> > +++ b/include/hw/virtio/virtio-scsi.h
> > @@ -174,6 +174,18 @@ typedef struct VirtIOSCSICommon {
> >      VirtQueue *ctrl_vq;
> >      VirtQueue *event_vq;
> >      VirtQueue **cmd_vqs;
> > +
> > +    /* Fields for dataplane below */
> > +    AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
> > +
> > +    /* Vring is used instead of vq in dataplane code, because of the underlying
> > +     * memory layer thread safety */
> > +    VirtIOSCSIVring *ctrl_vring;
> > +    VirtIOSCSIVring *event_vring;
> > +    VirtIOSCSIVring **cmd_vrings;
> > +    bool dataplane_started;
> > +    bool dataplane_starting;
> > +    bool dataplane_stopping;
> 
> Please add these to VirtIOSCSI rather than VirtIOSCSICommon.  Same for
> the new functions you declare below.
> 
> Paolo
> 
> >  } VirtIOSCSICommon;
> >  
> >  typedef struct {
> > @@ -239,4 +251,11 @@ void virtio_scsi_free_req(VirtIOSCSIReq *req);
> >  void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
> >                              uint32_t event, uint32_t reason);
> >  
> > +void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread);
> > +void virtio_scsi_dataplane_start(VirtIOSCSICommon *s);
> > +void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s);
> > +void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
> > +VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
> > +                                         VirtIOSCSIVring *vring);
> > +
> >  #endif /* _QEMU_VIRTIO_SCSI_H */
> > 
>
Paolo Bonzini Sept. 22, 2014, 8:09 a.m. UTC | #6
Il 22/09/2014 07:56, Fam Zheng ha scritto:
>> > 
>> > Please add these to VirtIOSCSI rather than VirtIOSCSICommon.  Same for
>> > the new functions you declare below.
> What's the rationale, please? Asking because especially the VirtIOSCSIVring
> fields are the dataplane counterparts of VirtQueue fields, so putting in
> VirtIOSCSI seems unnatural for me.

Because everything you put in VirtIOSCSICommon will be shared between
virtio-scsi and vhost-scsi, and vhost-scsi need not use neither vring
nor dataplane.

Paolo
Fam Zheng Sept. 22, 2014, 8:33 a.m. UTC | #7
On Mon, 09/22 10:09, Paolo Bonzini wrote:
> Il 22/09/2014 07:56, Fam Zheng ha scritto:
> >> > 
> >> > Please add these to VirtIOSCSI rather than VirtIOSCSICommon.  Same for
> >> > the new functions you declare below.
> > What's the rationale, please? Asking because especially the VirtIOSCSIVring
> > fields are the dataplane counterparts of VirtQueue fields, so putting in
> > VirtIOSCSI seems unnatural for me.
> 
> Because everything you put in VirtIOSCSICommon will be shared between
> virtio-scsi and vhost-scsi, and vhost-scsi need not use neither vring
> nor dataplane.
> 

I see, I'll move it into VirtIOSCSI then!

Fam
diff mbox

Patch

diff --git a/hw/scsi/Makefile.objs b/hw/scsi/Makefile.objs
index 121ddc5..40c79d3 100644
--- a/hw/scsi/Makefile.objs
+++ b/hw/scsi/Makefile.objs
@@ -8,6 +8,6 @@  common-obj-$(CONFIG_ESP_PCI) += esp-pci.o
 obj-$(CONFIG_PSERIES) += spapr_vscsi.o
 
 ifeq ($(CONFIG_VIRTIO),y)
-obj-y += virtio-scsi.o
+obj-y += virtio-scsi.o virtio-scsi-dataplane.o
 obj-$(CONFIG_VHOST_SCSI) += vhost-scsi.o
 endif
diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
new file mode 100644
index 0000000..d077b67
--- /dev/null
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -0,0 +1,219 @@ 
+/*
+ * Virtio SCSI dataplane
+ *
+ * Copyright Red Hat, Inc. 2014
+ *
+ * Authors:
+ *   Fam Zheng <famz@redhat.com>
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ *
+ */
+
+#include "hw/virtio/virtio-scsi.h"
+#include "qemu/error-report.h"
+#include <hw/scsi/scsi.h>
+#include <block/scsi.h>
+#include <hw/virtio/virtio-bus.h>
+#include "hw/virtio/virtio-access.h"
+#include "stdio.h"
+
+/* Context: QEMU global mutex held */
+void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    s->ctx = iothread_get_aio_context(s->conf.iothread);
+
+    /* Don't try if transport does not support notifiers. */
+    if (!k->set_guest_notifiers || !k->set_host_notifier) {
+        fprintf(stderr, "virtio-scsi: Failed to set iothread "
+                   "(transport does not support notifiers)");
+        exit(1);
+    }
+}
+
+static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSICommon *s,
+                                               VirtQueue *vq,
+                                               EventNotifierHandler *handler,
+                                               int n)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtIOSCSIVring *r = g_slice_new(VirtIOSCSIVring);
+
+    /* Set up virtqueue notify */
+    if (k->set_host_notifier(qbus->parent, n, true) != 0) {
+        fprintf(stderr, "virtio-scsi: Failed to set host notifier\n");
+        exit(1);
+    }
+    r->host_notifier = *virtio_queue_get_host_notifier(vq);
+    r->guest_notifier = *virtio_queue_get_guest_notifier(vq);
+    aio_set_event_notifier(s->ctx, &r->host_notifier, handler);
+
+    r->parent = s;
+
+    if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
+        fprintf(stderr, "virtio-scsi: VRing setup failed\n");
+        exit(1);
+    }
+    return r;
+}
+
+VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
+                                         VirtIOSCSIVring *vring)
+{
+    VirtIOSCSIReq *req = virtio_scsi_init_req(s, NULL);
+    int r;
+
+    req->vring = vring;
+    r = vring_pop((VirtIODevice *)s, &vring->vring, &req->elem);
+    if (r < 0) {
+        virtio_scsi_free_req(req);
+        req = NULL;
+    }
+    return req;
+}
+
+void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
+{
+    vring_push(&req->vring->vring, &req->elem,
+               req->qsgl.size + req->resp_iov.size);
+    event_notifier_set(&req->vring->guest_notifier);
+}
+
+static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier)
+{
+    VirtIOSCSIVring *vring = container_of(notifier,
+                                          VirtIOSCSIVring, host_notifier);
+    VirtIOSCSI *s = VIRTIO_SCSI(vring->parent);
+    VirtIOSCSIReq *req;
+
+    event_notifier_test_and_clear(notifier);
+    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
+        virtio_scsi_handle_ctrl_req(s, req);
+    }
+}
+
+static void virtio_scsi_iothread_handle_event(EventNotifier *notifier)
+{
+    VirtIOSCSIVring *vring = container_of(notifier,
+                                          VirtIOSCSIVring, host_notifier);
+    VirtIOSCSICommon *vs = vring->parent;
+    VirtIOSCSI *s = VIRTIO_SCSI(vs);
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+
+    event_notifier_test_and_clear(notifier);
+
+    if (!(vdev->status & VIRTIO_CONFIG_S_DRIVER_OK)) {
+        return;
+    }
+
+    if (s->events_dropped) {
+        virtio_scsi_push_event(s, NULL, VIRTIO_SCSI_T_NO_EVENT, 0);
+    }
+}
+
+static void virtio_scsi_iothread_handle_cmd(EventNotifier *notifier)
+{
+    VirtIOSCSIVring *vring = container_of(notifier,
+                                          VirtIOSCSIVring, host_notifier);
+    VirtIOSCSI *s = VIRTIO_SCSI(vring->parent);
+    VirtIOSCSIReq *req;
+
+    event_notifier_test_and_clear(notifier);
+    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
+        virtio_scsi_handle_cmd_req(s, req);
+    }
+}
+
+/* Context: QEMU global mutex held */
+void virtio_scsi_dataplane_start(VirtIOSCSICommon *s)
+{
+    int i;
+    int rc;
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+
+    if (s->dataplane_started ||
+        s->dataplane_starting ||
+        s->ctx != iothread_get_aio_context(s->conf.iothread)) {
+        return;
+    }
+
+    s->dataplane_starting = true;
+
+    /* Set up guest notifier (irq) */
+    rc = k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, true);
+    if (rc != 0) {
+        fprintf(stderr, "virtio-scsi: Failed to set guest notifiers, "
+                "ensure -enable-kvm is set\n");
+        exit(1);
+    }
+
+    aio_context_acquire(s->ctx);
+    s->ctrl_vring = virtio_scsi_vring_init(s, s->ctrl_vq,
+                                           virtio_scsi_iothread_handle_ctrl,
+                                           0);
+    s->event_vring = virtio_scsi_vring_init(s, s->event_vq,
+                                            virtio_scsi_iothread_handle_event,
+                                            1);
+    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * s->conf.num_queues);
+    for (i = 0; i < s->conf.num_queues; i++) {
+        s->cmd_vrings[i] =
+            virtio_scsi_vring_init(s, s->cmd_vqs[i],
+                                   virtio_scsi_iothread_handle_cmd,
+                                   i + 2);
+    }
+
+    aio_context_release(s->ctx);
+    s->dataplane_starting = false;
+    s->dataplane_started = true;
+}
+
+/* Context: QEMU global mutex held */
+void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s)
+{
+    BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
+    VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtIODevice *vdev = VIRTIO_DEVICE(s);
+    int i;
+
+    if (!s->dataplane_started || s->dataplane_stopping) {
+        return;
+    }
+    s->dataplane_stopping = true;
+    assert(s->ctx == iothread_get_aio_context(s->conf.iothread));
+
+    aio_context_acquire(s->ctx);
+
+    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier, NULL);
+    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier, NULL);
+    for (i = 0; i < s->conf.num_queues; i++) {
+        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier, NULL);
+    }
+
+    bdrv_drain_all(); /* ensure there are no in-flight requests */
+
+    aio_context_release(s->ctx);
+
+    /* Sync vring state back to virtqueue so that non-dataplane request
+     * processing can continue when we disable the host notifier below.
+     */
+    vring_teardown(&s->ctrl_vring->vring, vdev, 0);
+    vring_teardown(&s->event_vring->vring, vdev, 1);
+    for (i = 0; i < s->conf.num_queues; i++) {
+        vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
+    }
+
+    for (i = 0; i < s->conf.num_queues + 2; i++) {
+        k->set_host_notifier(qbus->parent, i, false);
+    }
+
+    /* Clean up guest notifier (irq) */
+    k->set_guest_notifiers(qbus->parent, s->conf.num_queues + 2, false);
+    s->dataplane_stopping = false;
+    s->dataplane_started = false;
+}
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index 6f92c29..b9f2197 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -174,6 +174,18 @@  typedef struct VirtIOSCSICommon {
     VirtQueue *ctrl_vq;
     VirtQueue *event_vq;
     VirtQueue **cmd_vqs;
+
+    /* Fields for dataplane below */
+    AioContext *ctx; /* one iothread per virtio-scsi-pci for now */
+
+    /* Vring is used instead of vq in dataplane code, because of the underlying
+     * memory layer thread safety */
+    VirtIOSCSIVring *ctrl_vring;
+    VirtIOSCSIVring *event_vring;
+    VirtIOSCSIVring **cmd_vrings;
+    bool dataplane_started;
+    bool dataplane_starting;
+    bool dataplane_stopping;
 } VirtIOSCSICommon;
 
 typedef struct {
@@ -239,4 +251,11 @@  void virtio_scsi_free_req(VirtIOSCSIReq *req);
 void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
                             uint32_t event, uint32_t reason);
 
+void virtio_scsi_set_iothread(VirtIOSCSICommon *s, IOThread *iothread);
+void virtio_scsi_dataplane_start(VirtIOSCSICommon *s);
+void virtio_scsi_dataplane_stop(VirtIOSCSICommon *s);
+void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
+VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
+                                         VirtIOSCSIVring *vring);
+
 #endif /* _QEMU_VIRTIO_SCSI_H */