On Mon, 10 Oct 2016 13:53:34 +0200
Paolo Bonzini <pbonz...@redhat.com> wrote:
> Override start_ioeventfd and stop_ioeventfd to start/stop the
> whole dataplane logic. This has some positive side effects:
>
> - no need anymore for virtio_add_queue_aio (i.e. a revert of
> commit 0ff841f6d138904d514efa1d885bcaf54583852d)
>
> - no need anymore to switch from generic ioeventfd handlers to
> dataplane
>
> It detects some errors better:
>
> $ qemu-system-x86_64 -object iothread,id=io \
> -drive id=null,file=null-aio://,if=none,format=raw \
> -device virtio-blk-pci,ioeventfd=off,iothread=io,drive=null
> qemu-system-x86_64: -device
> virtio-blk-pci,ioeventfd=off,iothread=io,drive=null:
> ioeventfd is required for iothread
>
> while previously it would have started just fine.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> ---
> v1->v2: improvement in error message consistency
> loosen virtio_blk_set_status assertion [Christian]
>
> hw/block/dataplane/virtio-blk.c | 74
> +++++++++++++++++++++++++----------------
> hw/block/dataplane/virtio-blk.h | 6 ++--
> hw/block/virtio-blk.c | 15 ++++-----
> 3 files changed, 55 insertions(+), 40 deletions(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 9b268f4..d96f45c 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -83,28 +83,33 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev,
> VirtIOBlkConf *conf,
> Error **errp)
> {
> VirtIOBlockDataPlane *s;
> - BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
> + BusState *qbus = qdev_get_parent_bus(DEVICE(vdev));
Unrelated change?
> VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>
> *dataplane = NULL;
>
> - if (!conf->iothread) {
> - return;
> - }
> -
> /* Don't try if transport does not support notifiers. */
I'd move this comment together with the next block, otherwise this is
confusing.
> - if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> - error_setg(errp,
> - "device is incompatible with dataplane "
> - "(transport does not support notifiers)");
> - return;
> - }
> + if (conf->iothread) {
> + if (!k->set_guest_notifiers || !k->ioeventfd_assign) {
> + error_setg(errp,
> + "device is incompatible with iothread "
> + "(transport does not support notifiers)");
> + return;
> + }
> + if (!virtio_device_ioeventfd_enabled(vdev)) {
> + error_setg(errp, "ioeventfd is required for iothread");
> + return;
> + }
>
> - /* If dataplane is (re-)enabled while the guest is running there could be
> - * block jobs that can conflict.
> - */
> - if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE, errp)) {
> - error_prepend(errp, "cannot start dataplane thread: ");
> + /* If dataplane is (re-)enabled while the guest is running there
> could
> + * be block jobs that can conflict.
> + */
> + if (blk_op_is_blocked(conf->conf.blk, BLOCK_OP_TYPE_DATAPLANE,
> errp)) {
> + error_prepend(errp, "cannot start virtio-blk dataplane: ");
> + return;
> + }
> + }
> + if (!virtio_device_ioeventfd_enabled(vdev)) {
> return;
> }
>
(...)
> @@ -124,14 +133,18 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev,
> VirtIOBlkConf *conf,
> /* Context: QEMU global mutex held */
> void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
> {
> + VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> +
> if (!s) {
You already dereferenced s above...
> return;
> }
>
> - virtio_blk_data_plane_stop(s);
> + assert(!vblk->dataplane_started);
> g_free(s->batch_notify_vqs);
> qemu_bh_delete(s->bh);
> - object_unref(OBJECT(s->iothread));
> + if (s->iothread) {
> + object_unref(OBJECT(s->iothread));
> + }
> g_free(s);
> }
>
In general, I think this looks good, but I'll have a look at the end
result as well.
