[Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Paolo Bonzini]

Currently, dataplane threads are shut down during migration because
vring.c is not able to track dirty memory.  However, all the relevant
parts of QEMU have been made thread-safe now, so we can drop vring.c
completely.  With these patches, virtio-dataplane is now simply "virtio
with ioeventfd in a different AioContext".

Paolo Bonzini (8):
  block-migration: acquire AioContext as necessary
  vring: make vring_enable_notification return void
  virtio: add AioContext-specific function for host notifiers
  virtio: export vring_notify as virtio_should_notify
  virtio-blk: fix "disabled data plane" mode
  virtio-blk: do not use vring in dataplane
  virtio-scsi: do not use vring in dataplane
  vring: remove

 hw/block/dataplane/virtio-blk.c               | 130 +-----
 hw/block/dataplane/virtio-blk.h               |   1 +
 hw/block/virtio-blk.c                         |  51 +--
 hw/scsi/virtio-scsi-dataplane.c               | 196 ++-------
 hw/scsi/virtio-scsi.c                         |  52 +--
 hw/virtio/Makefile.objs                       |   1 -
 hw/virtio/dataplane/Makefile.objs             |   1 -
 hw/virtio/dataplane/vring.c                   | 549 --------------------------
 hw/virtio/virtio.c                            |  20 +-
 include/hw/virtio/dataplane/vring-accessors.h |  75 ----
 include/hw/virtio/dataplane/vring.h           |  51 ---
 include/hw/virtio/virtio-blk.h                |   4 +-
 include/hw/virtio/virtio-scsi.h               |  21 +-
 include/hw/virtio/virtio.h                    |   3 +
 migration/block.c                             |  61 ++-
 trace-events                                  |   3 -
 16 files changed, 134 insertions(+), 1085 deletions(-)
 delete mode 100644 hw/virtio/dataplane/Makefile.objs
 delete mode 100644 hw/virtio/dataplane/vring.c
 delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
 delete mode 100644 include/hw/virtio/dataplane/vring.h

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/8] block-migration: acquire AioContext as necessary

[Paolo Bonzini]

This is needed because dataplane will run during block migration as well.

The block device migration code is quite liberal in taking the iothread
mutex.  For simplicity, keep it the same way, even though one could
actually choose between the BQL (for regular BlockDriverStates) and
the AioContext (for dataplane BlockDriverStates).  When the block layer
is made fully thread safe, aio_context_acquire shall go away altogether.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 migration/block.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 49 insertions(+), 12 deletions(-)

diff --git a/migration/block.c b/migration/block.c
index a444058..6dd2327 100644
--- a/migration/block.c
+++ b/migration/block.c
@@ -60,9 +60,15 @@ typedef struct BlkMigDevState {
     int64_t cur_sector;
     int64_t cur_dirty;
 
-    /* Protected by block migration lock.  */
+    /* Data in the aio_bitmap is protected by block migration lock.
+     * Allocation and free happen during setup and cleanup respectively.
+     */
     unsigned long *aio_bitmap;
+
+    /* Protected by block migration lock.  */
     int64_t completed_sectors;
+
+    /* Protected by iothread lock / AioContext.  */
     BdrvDirtyBitmap *dirty_bitmap;
     Error *blocker;
 } BlkMigDevState;
@@ -100,7 +106,7 @@ typedef struct BlkMigState {
     int prev_progress;
     int bulk_completed;
 
-    /* Lock must be taken _inside_ the iothread lock.  */
+    /* Lock must be taken _inside_ the iothread lock and any AioContexts.  */
     QemuMutex lock;
 } BlkMigState;
 
@@ -264,11 +270,13 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
 
     if (bmds->shared_base) {
         qemu_mutex_lock_iothread();
+        aio_context_acquire(bdrv_get_aio_context(bs));
         while (cur_sector < total_sectors &&
                !bdrv_is_allocated(bs, cur_sector, MAX_IS_ALLOCATED_SEARCH,
                                   &nr_sectors)) {
             cur_sector += nr_sectors;
         }
+        aio_context_release(bdrv_get_aio_context(bs));
         qemu_mutex_unlock_iothread();
     }
 
@@ -302,11 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
     block_mig_state.submitted++;
     blk_mig_unlock();
 
+    /* We do not know if bs is under the main thread (and thus does
+     * not acquire the AioContext when doing AIO) or rather under
+     * dataplane.  Thus acquire both the iothread mutex and the
+     * AioContext.
+     *
+     * This is ugly and will disappear when we make bdrv_* thread-safe,
+     * without the need to acquire the AioContext.
+     */
     qemu_mutex_lock_iothread();
+    aio_context_acquire(bdrv_get_aio_context(bmds->bs));
     blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
                                 nr_sectors, blk_mig_read_cb, blk);
 
     bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
+    aio_context_release(bdrv_get_aio_context(bmds->bs));
     qemu_mutex_unlock_iothread();
 
     bmds->cur_sector = cur_sector + nr_sectors;
@@ -321,8 +339,9 @@ static int set_dirty_tracking(void)
     int ret;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
+        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */
         bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
                                                       NULL, NULL);
         if (!bmds->dirty_bitmap) {
             ret = -errno;
             goto fail;
@@ -332,11 +352,14 @@ static int set_dirty_tracking(void)
     return ret;
 }
 
+/* Called with iothread lock taken.  */
+
 static void unset_dirty_tracking(void)
 {
     BlkMigDevState *bmds;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
+        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */
         bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
     }
 }
@@ -444,7 +470,7 @@ static void blk_mig_reset_dirty_cursor(void)
     }
 }
 
-/* Called with iothread lock taken.  */
+/* Called with iothread lock and AioContext taken.  */
 
 static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds,
                                  int is_async)
@@ -527,7 +553,9 @@ static int blk_mig_save_dirty_block(QEMUFile *f, int is_async)
     int ret = 1;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
+        aio_context_acquire(bdrv_get_aio_context(bmds->bs));
         ret = mig_save_device_dirty(f, bmds, is_async);
+        aio_context_release(bdrv_get_aio_context(bmds->bs));
         if (ret <= 0) {
             break;
         }
@@ -585,7 +613,9 @@ static int64_t get_remaining_dirty(void)
     int64_t dirty = 0;
 
     QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
+        aio_context_acquire(bdrv_get_aio_context(bmds->bs));
         dirty += bdrv_get_dirty_count(bmds->dirty_bitmap);
+        aio_context_release(bdrv_get_aio_context(bmds->bs));
     }
 
     return dirty << BDRV_SECTOR_BITS;
@@ -597,21 +627,28 @@ static void block_migration_cleanup(void *opaque)
 {
     BlkMigDevState *bmds;
     BlkMigBlock *blk;
+    AioContext *ctx;
 
     bdrv_drain_all();
 
     unset_dirty_tracking();
 
-    blk_mig_lock();
     while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
         bdrv_op_unblock_all(bmds->bs, bmds->blocker);
         error_free(bmds->blocker);
+
+        /* Save ctx, because bmds->bs can disappear during bdrv_unref.  */
+        ctx = bdrv_get_aio_context(bmds->bs);
+        aio_context_acquire(ctx);
         bdrv_unref(bmds->bs);
+        aio_context_release(ctx);
+
         g_free(bmds->aio_bitmap);
         g_free(bmds);
     }
 
+    blk_mig_lock();
     while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
         QSIMPLEQ_REMOVE_HEAD(&block_mig_state.blk_list, entry);
         g_free(blk->buf);
@@ -633,13 +670,12 @@ static int block_save_setup(QEMUFile *f, void *opaque)
     /* start track dirty blocks */
     ret = set_dirty_tracking();
 
+    qemu_mutex_unlock_iothread();
+
     if (ret) {
-        qemu_mutex_unlock_iothread();
         return ret;
     }
 
-    qemu_mutex_unlock_iothread();
-
     ret = flush_blks(f);
     blk_mig_reset_dirty_cursor();
     qemu_put_be64(f, BLK_MIG_FLAG_EOS);
@@ -761,17 +797,18 @@ static void block_save_pending(QEMUFile *f, void *opaque, uint64_t max_size,
     uint64_t pending;
 
     qemu_mutex_lock_iothread();
+    pending = get_remaining_dirty();
+    qemu_mutex_unlock_iothread();
+
     blk_mig_lock();
-    pending = get_remaining_dirty() +
-                       block_mig_state.submitted * BLOCK_SIZE +
-                       block_mig_state.read_done * BLOCK_SIZE;
+    pending += block_mig_state.submitted * BLOCK_SIZE +
+               block_mig_state.read_done * BLOCK_SIZE;
+    blk_mig_unlock();
 
     /* Report at least one block pending during bulk phase */
     if (pending <= max_size && !block_mig_state.bulk_completed) {
         pending = max_size + BLOCK_SIZE;
     }
-    blk_mig_unlock();
-    qemu_mutex_unlock_iothread();
 
     DPRINTF("Enter save live pending  %" PRIu64 "\n", pending);
     /* We don't do postcopy */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 1/8] block-migration: acquire AioContext as necessary

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> This is needed because dataplane will run during block migration as well.
> 
> The block device migration code is quite liberal in taking the iothread
> mutex.  For simplicity, keep it the same way, even though one could
> actually choose between the BQL (for regular BlockDriverStates) and
> the AioContext (for dataplane BlockDriverStates).  When the block layer
> is made fully thread safe, aio_context_acquire shall go away altogether.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  migration/block.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 49 insertions(+), 12 deletions(-)
> 
> diff --git a/migration/block.c b/migration/block.c
> index a444058..6dd2327 100644
> --- a/migration/block.c
> +++ b/migration/block.c
> @@ -60,9 +60,15 @@ typedef struct BlkMigDevState {
>      int64_t cur_sector;
>      int64_t cur_dirty;
>  
> -    /* Protected by block migration lock.  */
> +    /* Data in the aio_bitmap is protected by block migration lock.
> +     * Allocation and free happen during setup and cleanup respectively.
> +     */
>      unsigned long *aio_bitmap;
> +
> +    /* Protected by block migration lock.  */
>      int64_t completed_sectors;
> +
> +    /* Protected by iothread lock / AioContext.  */
>      BdrvDirtyBitmap *dirty_bitmap;
>      Error *blocker;
>  } BlkMigDevState;
> @@ -100,7 +106,7 @@ typedef struct BlkMigState {
>      int prev_progress;
>      int bulk_completed;
>  
> -    /* Lock must be taken _inside_ the iothread lock.  */
> +    /* Lock must be taken _inside_ the iothread lock and any AioContexts.  */
>      QemuMutex lock;
>  } BlkMigState;
>  
> @@ -264,11 +270,13 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>  
>      if (bmds->shared_base) {
>          qemu_mutex_lock_iothread();
> +        aio_context_acquire(bdrv_get_aio_context(bs));
>          while (cur_sector < total_sectors &&
>                 !bdrv_is_allocated(bs, cur_sector, MAX_IS_ALLOCATED_SEARCH,
>                                    &nr_sectors)) {
>              cur_sector += nr_sectors;
>          }
> +        aio_context_release(bdrv_get_aio_context(bs));
>          qemu_mutex_unlock_iothread();
>      }
>  
> @@ -302,11 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
>      block_mig_state.submitted++;
>      blk_mig_unlock();
>  
> +    /* We do not know if bs is under the main thread (and thus does
> +     * not acquire the AioContext when doing AIO) or rather under
> +     * dataplane.  Thus acquire both the iothread mutex and the
> +     * AioContext.
> +     *
> +     * This is ugly and will disappear when we make bdrv_* thread-safe,
> +     * without the need to acquire the AioContext.
> +     */
>      qemu_mutex_lock_iothread();
> +    aio_context_acquire(bdrv_get_aio_context(bmds->bs));
>      blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
>                                  nr_sectors, blk_mig_read_cb, blk);
>  
>      bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
> +    aio_context_release(bdrv_get_aio_context(bmds->bs));
>      qemu_mutex_unlock_iothread();
>  
>      bmds->cur_sector = cur_sector + nr_sectors;
> @@ -321,8 +339,9 @@ static int set_dirty_tracking(void)
>      int ret;
>  
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> +        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */

Why? I don't think it is safe today.  The BDS state is mutated and it can race
with bdrv_set_dirty() etc. (Also the refresh_total_sectors in bdrv_nb_sectors
can even do read/write, no?)

>          bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
>                                                        NULL, NULL);
>          if (!bmds->dirty_bitmap) {
>              ret = -errno;
>              goto fail;
> @@ -332,11 +352,14 @@ static int set_dirty_tracking(void)
>      return ret;
>  }
>  
> +/* Called with iothread lock taken.  */
> +
>  static void unset_dirty_tracking(void)
>  {
>      BlkMigDevState *bmds;
>  
>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> +        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */

Ditto.

>          bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
>      }
>  }
> @@ -597,21 +627,28 @@ static void block_migration_cleanup(void *opaque)
>  {
>      BlkMigDevState *bmds;
>      BlkMigBlock *blk;
> +    AioContext *ctx;
>  
>      bdrv_drain_all();
>  
>      unset_dirty_tracking();
>  
> -    blk_mig_lock();

Why is it okay to skip the blk_mig_lock() for block_mig_state.bmds_list?

>      while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
>          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
>          bdrv_op_unblock_all(bmds->bs, bmds->blocker);
>          error_free(bmds->blocker);
> +
> +        /* Save ctx, because bmds->bs can disappear during bdrv_unref.  */
> +        ctx = bdrv_get_aio_context(bmds->bs);
> +        aio_context_acquire(ctx);
>          bdrv_unref(bmds->bs);
> +        aio_context_release(ctx);
> +
>          g_free(bmds->aio_bitmap);
>          g_free(bmds);
>      }
>  
> +    blk_mig_lock();
>      while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
>          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.blk_list, entry);
>          g_free(blk->buf);

Re: [Qemu-devel] [PATCH 1/8] block-migration: acquire AioContext as necessary

[Michael S. Tsirkin]

ping
Paolo - were you going to address these questions?
Or did I miss it?

On Tue, Feb 16, 2016 at 03:17:11PM +0800, Fam Zheng wrote:
> On Sun, 02/14 18:17, Paolo Bonzini wrote:
> > This is needed because dataplane will run during block migration as well.
> > 
> > The block device migration code is quite liberal in taking the iothread
> > mutex.  For simplicity, keep it the same way, even though one could
> > actually choose between the BQL (for regular BlockDriverStates) and
> > the AioContext (for dataplane BlockDriverStates).  When the block layer
> > is made fully thread safe, aio_context_acquire shall go away altogether.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> > ---
> >  migration/block.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 49 insertions(+), 12 deletions(-)
> > 
> > diff --git a/migration/block.c b/migration/block.c
> > index a444058..6dd2327 100644
> > --- a/migration/block.c
> > +++ b/migration/block.c
> > @@ -60,9 +60,15 @@ typedef struct BlkMigDevState {
> >      int64_t cur_sector;
> >      int64_t cur_dirty;
> >  
> > -    /* Protected by block migration lock.  */
> > +    /* Data in the aio_bitmap is protected by block migration lock.
> > +     * Allocation and free happen during setup and cleanup respectively.
> > +     */
> >      unsigned long *aio_bitmap;
> > +
> > +    /* Protected by block migration lock.  */
> >      int64_t completed_sectors;
> > +
> > +    /* Protected by iothread lock / AioContext.  */
> >      BdrvDirtyBitmap *dirty_bitmap;
> >      Error *blocker;
> >  } BlkMigDevState;
> > @@ -100,7 +106,7 @@ typedef struct BlkMigState {
> >      int prev_progress;
> >      int bulk_completed;
> >  
> > -    /* Lock must be taken _inside_ the iothread lock.  */
> > +    /* Lock must be taken _inside_ the iothread lock and any AioContexts.  */
> >      QemuMutex lock;
> >  } BlkMigState;
> >  
> > @@ -264,11 +270,13 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> >  
> >      if (bmds->shared_base) {
> >          qemu_mutex_lock_iothread();
> > +        aio_context_acquire(bdrv_get_aio_context(bs));
> >          while (cur_sector < total_sectors &&
> >                 !bdrv_is_allocated(bs, cur_sector, MAX_IS_ALLOCATED_SEARCH,
> >                                    &nr_sectors)) {
> >              cur_sector += nr_sectors;
> >          }
> > +        aio_context_release(bdrv_get_aio_context(bs));
> >          qemu_mutex_unlock_iothread();
> >      }
> >  
> > @@ -302,11 +310,21 @@ static int mig_save_device_bulk(QEMUFile *f, BlkMigDevState *bmds)
> >      block_mig_state.submitted++;
> >      blk_mig_unlock();
> >  
> > +    /* We do not know if bs is under the main thread (and thus does
> > +     * not acquire the AioContext when doing AIO) or rather under
> > +     * dataplane.  Thus acquire both the iothread mutex and the
> > +     * AioContext.
> > +     *
> > +     * This is ugly and will disappear when we make bdrv_* thread-safe,
> > +     * without the need to acquire the AioContext.
> > +     */
> >      qemu_mutex_lock_iothread();
> > +    aio_context_acquire(bdrv_get_aio_context(bmds->bs));
> >      blk->aiocb = bdrv_aio_readv(bs, cur_sector, &blk->qiov,
> >                                  nr_sectors, blk_mig_read_cb, blk);
> >  
> >      bdrv_reset_dirty_bitmap(bmds->dirty_bitmap, cur_sector, nr_sectors);
> > +    aio_context_release(bdrv_get_aio_context(bmds->bs));
> >      qemu_mutex_unlock_iothread();
> >  
> >      bmds->cur_sector = cur_sector + nr_sectors;
> > @@ -321,8 +339,9 @@ static int set_dirty_tracking(void)
> >      int ret;
> >  
> >      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> > +        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */
> 
> Why? I don't think it is safe today.  The BDS state is mutated and it can race
> with bdrv_set_dirty() etc. (Also the refresh_total_sectors in bdrv_nb_sectors
> can even do read/write, no?)
> 
> >          bmds->dirty_bitmap = bdrv_create_dirty_bitmap(bmds->bs, BLOCK_SIZE,
> >                                                        NULL, NULL);
> >          if (!bmds->dirty_bitmap) {
> >              ret = -errno;
> >              goto fail;
> > @@ -332,11 +352,14 @@ static int set_dirty_tracking(void)
> >      return ret;
> >  }
> >  
> > +/* Called with iothread lock taken.  */
> > +
> >  static void unset_dirty_tracking(void)
> >  {
> >      BlkMigDevState *bmds;
> >  
> >      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
> > +        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */
> 
> Ditto.
> 
> >          bdrv_release_dirty_bitmap(bmds->bs, bmds->dirty_bitmap);
> >      }
> >  }
> > @@ -597,21 +627,28 @@ static void block_migration_cleanup(void *opaque)
> >  {
> >      BlkMigDevState *bmds;
> >      BlkMigBlock *blk;
> > +    AioContext *ctx;
> >  
> >      bdrv_drain_all();
> >  
> >      unset_dirty_tracking();
> >  
> > -    blk_mig_lock();
> 
> Why is it okay to skip the blk_mig_lock() for block_mig_state.bmds_list?
> 
> >      while ((bmds = QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) != NULL) {
> >          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry);
> >          bdrv_op_unblock_all(bmds->bs, bmds->blocker);
> >          error_free(bmds->blocker);
> > +
> > +        /* Save ctx, because bmds->bs can disappear during bdrv_unref.  */
> > +        ctx = bdrv_get_aio_context(bmds->bs);
> > +        aio_context_acquire(ctx);
> >          bdrv_unref(bmds->bs);
> > +        aio_context_release(ctx);
> > +
> >          g_free(bmds->aio_bitmap);
> >          g_free(bmds);
> >      }
> >  
> > +    blk_mig_lock();
> >      while ((blk = QSIMPLEQ_FIRST(&block_mig_state.blk_list)) != NULL) {
> >          QSIMPLEQ_REMOVE_HEAD(&block_mig_state.blk_list, entry);
> >          g_free(blk->buf);

Re: [Qemu-devel] [PATCH 1/8] block-migration: acquire AioContext as necessary

[Paolo Bonzini]

On 16/02/2016 08:17, Fam Zheng wrote:
>> @@ -321,8 +339,9 @@ static int set_dirty_tracking(void)
>>      int ret;
>>  
>>      QSIMPLEQ_FOREACH(bmds, &block_mig_state.bmds_list, entry) {
>> +        /* Creating/dropping dirty bitmaps only requires the big QEMU lock.  */
> 
> Why? I don't think it is safe today.  The BDS state is mutated and it can race
> with bdrv_set_dirty() etc.

You're completely right.

> (Also the refresh_total_sectors in bdrv_nb_sectors
> can even do read/write, no?)

refresh_total_sectors will just do a lseek(SEEK_END) basically.  So
that's safe.

Paolo

[Qemu-devel] [PATCH 2/8] vring: make vring_enable_notification return void

[Paolo Bonzini]

Make the API more similar to the regular virtqueue API.  This will
help when modifying the code to not use vring.c anymore.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c     | 3 ++-
 hw/virtio/dataplane/vring.c         | 3 +--
 include/hw/virtio/dataplane/vring.h | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 0d99781..03b81bc 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -128,7 +128,8 @@ static void handle_notify(EventNotifier *e)
             /* Re-enable guest->host notifies and stop processing the vring.
              * But if the guest has snuck in more descriptors, keep processing.
              */
-            if (vring_enable_notification(s->vdev, &s->vring)) {
+            vring_enable_notification(s->vdev, &s->vring);
+            if (!vring_more_avail(s->vdev, &s->vring)) {
                 break;
             }
         } else { /* fatal error */
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
index 4308d9f..157e8b8 100644
--- a/hw/virtio/dataplane/vring.c
+++ b/hw/virtio/dataplane/vring.c
@@ -175,7 +175,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
  *
  * Return true if the vring is empty, false if there are more requests.
  */
-bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
+void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
 {
     if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
         vring_avail_event(&vring->vr) = vring->vr.avail->idx;
@@ -183,7 +183,6 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
         vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
     }
     smp_mb(); /* ensure update is seen before reading avail_idx */
-    return !vring_more_avail(vdev, vring);
 }
 
 /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
index e80985e..e1c2a65 100644
--- a/include/hw/virtio/dataplane/vring.h
+++ b/include/hw/virtio/dataplane/vring.h
@@ -42,7 +42,7 @@ static inline void vring_set_broken(Vring *vring)
 bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
 void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
 void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
-bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
+void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
 bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
 void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
 void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 2/8] vring: make vring_enable_notification return void

[Cornelia Huck]

On Sun, 14 Feb 2016 18:17:05 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Make the API more similar to the regular virtqueue API.  This will
> help when modifying the code to not use vring.c anymore.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c     | 3 ++-
>  hw/virtio/dataplane/vring.c         | 3 +--
>  include/hw/virtio/dataplane/vring.h | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 

> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 4308d9f..157e8b8 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -175,7 +175,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
>   *
>   * Return true if the vring is empty, false if there are more requests.
>   */

I realize that this is going away, but the comment above is now a bit
irritating :)

> -bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
> +void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
>  {
>      if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
>          vring_avail_event(&vring->vr) = vring->vr.avail->idx;
> @@ -183,7 +183,6 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
>          vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
>      }
>      smp_mb(); /* ensure update is seen before reading avail_idx */
> -    return !vring_more_avail(vdev, vring);
>  }
> 
>  /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */

Otherwise

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 2/8] vring: make vring_enable_notification return void

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Make the API more similar to the regular virtqueue API.  This will
> help when modifying the code to not use vring.c anymore.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c     | 3 ++-
>  hw/virtio/dataplane/vring.c         | 3 +--
>  include/hw/virtio/dataplane/vring.h | 2 +-
>  3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 0d99781..03b81bc 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -128,7 +128,8 @@ static void handle_notify(EventNotifier *e)
>              /* Re-enable guest->host notifies and stop processing the vring.
>               * But if the guest has snuck in more descriptors, keep processing.
>               */
> -            if (vring_enable_notification(s->vdev, &s->vring)) {
> +            vring_enable_notification(s->vdev, &s->vring);
> +            if (!vring_more_avail(s->vdev, &s->vring)) {
>                  break;
>              }
>          } else { /* fatal error */
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> index 4308d9f..157e8b8 100644
> --- a/hw/virtio/dataplane/vring.c
> +++ b/hw/virtio/dataplane/vring.c
> @@ -175,7 +175,7 @@ void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
>   *
>   * Return true if the vring is empty, false if there are more requests.
>   */
> -bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
> +void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
>  {
>      if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
>          vring_avail_event(&vring->vr) = vring->vr.avail->idx;
> @@ -183,7 +183,6 @@ bool vring_enable_notification(VirtIODevice *vdev, Vring *vring)
>          vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
>      }
>      smp_mb(); /* ensure update is seen before reading avail_idx */
> -    return !vring_more_avail(vdev, vring);
>  }
>  
>  /* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
> diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
> index e80985e..e1c2a65 100644
> --- a/include/hw/virtio/dataplane/vring.h
> +++ b/include/hw/virtio/dataplane/vring.h
> @@ -42,7 +42,7 @@ static inline void vring_set_broken(Vring *vring)
>  bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
>  void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
>  void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
> -bool vring_enable_notification(VirtIODevice *vdev, Vring *vring);
> +void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
>  bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
>  void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
>  void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> -- 
> 1.8.3.1
> 
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 3/8] virtio: add AioContext-specific function for host notifiers

[Paolo Bonzini]

This is used to register ioeventfd with a dataplane thread.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio.c         | 16 ++++++++++++++++
 include/hw/virtio/virtio.h |  2 ++
 2 files changed, 18 insertions(+)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 90f2545..3a5cca4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1785,6 +1785,22 @@ static void virtio_queue_host_notifier_read(EventNotifier *n)
     }
 }
 
+void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
+                                                bool assign, bool set_handler)
+{
+    if (assign && set_handler) {
+        aio_set_event_notifier(ctx, &vq->host_notifier, true,
+                               virtio_queue_host_notifier_read);
+    } else {
+        aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
+    }
+    if (!assign) {
+        /* Test and clear notifier before after disabling event,
+         * in case poll callback didn't have time to run. */
+        virtio_queue_host_notifier_read(&vq->host_notifier);
+    }
+}
+
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler)
 {
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 108cdb0..4ce01a1 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -248,6 +248,8 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
 EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
 void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler);
+void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
+                                                bool assign, bool set_handler);
 void virtio_queue_notify_vq(VirtQueue *vq);
 void virtio_irq(VirtQueue *vq);
 VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 3/8] virtio: add AioContext-specific function for host notifiers

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> This is used to register ioeventfd with a dataplane thread.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio.c         | 16 ++++++++++++++++
>  include/hw/virtio/virtio.h |  2 ++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 90f2545..3a5cca4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1785,6 +1785,22 @@ static void virtio_queue_host_notifier_read(EventNotifier *n)
>      }
>  }
>  
> +void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
> +                                                bool assign, bool set_handler)
> +{
> +    if (assign && set_handler) {
> +        aio_set_event_notifier(ctx, &vq->host_notifier, true,
> +                               virtio_queue_host_notifier_read);
> +    } else {
> +        aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
> +    }
> +    if (!assign) {
> +        /* Test and clear notifier before after disabling event,

Does "before after" mean "after"? :)

Reviewed-by: Fam Zheng <famz@redhat.com>

> +         * in case poll callback didn't have time to run. */
> +        virtio_queue_host_notifier_read(&vq->host_notifier);
> +    }
> +}
> +
>  void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler)
>  {
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 108cdb0..4ce01a1 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -248,6 +248,8 @@ void virtio_queue_set_guest_notifier_fd_handler(VirtQueue *vq, bool assign,
>  EventNotifier *virtio_queue_get_host_notifier(VirtQueue *vq);
>  void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler);
> +void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
> +                                                bool assign, bool set_handler);
>  void virtio_queue_notify_vq(VirtQueue *vq);
>  void virtio_irq(VirtQueue *vq);
>  VirtQueue *virtio_vector_first_queue(VirtIODevice *vdev, uint16_t vector);
> -- 
> 1.8.3.1
> 
> 
> 

Re: [Qemu-devel] [PATCH 3/8] virtio: add AioContext-specific function for host notifiers

[Cornelia Huck]

On Tue, 16 Feb 2016 15:20:17 +0800
Fam Zheng <famz@redhat.com> wrote:

> On Sun, 02/14 18:17, Paolo Bonzini wrote:

> > +void virtio_queue_aio_set_host_notifier_handler(VirtQueue *vq, AioContext *ctx,
> > +                                                bool assign, bool set_handler)
> > +{
> > +    if (assign && set_handler) {
> > +        aio_set_event_notifier(ctx, &vq->host_notifier, true,
> > +                               virtio_queue_host_notifier_read);
> > +    } else {
> > +        aio_set_event_notifier(ctx, &vq->host_notifier, true, NULL);
> > +    }
> > +    if (!assign) {
> > +        /* Test and clear notifier before after disabling event,
> 
> Does "before after" mean "after"? :)

I think that was copied verbatim from
virtio_queue_set_host_notifier_fd_handler :)

[Qemu-devel] [PATCH 4/8] virtio: export vring_notify as virtio_should_notify

[Paolo Bonzini]

Virtio dataplane needs to trigger the irq manually through the
guest notifier.  Export virtio_should_notify so that it can be
used around event_notifier_set.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/virtio.c         | 4 ++--
 include/hw/virtio/virtio.h | 1 +
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 3a5cca4..abb97f4 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1162,7 +1162,7 @@ void virtio_irq(VirtQueue *vq)
     virtio_notify_vector(vq->vdev, vq->vector);
 }
 
-static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
+bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
     uint16_t old, new;
     bool v;
@@ -1187,7 +1187,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
 
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
 {
-    if (!vring_notify(vdev, vq)) {
+    if (!virtio_should_notify(vdev, vq)) {
         return;
     }
 
diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
index 4ce01a1..5884228 100644
--- a/include/hw/virtio/virtio.h
+++ b/include/hw/virtio/virtio.h
@@ -162,6 +162,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
                                unsigned int *out_bytes,
                                unsigned max_in_bytes, unsigned max_out_bytes);
 
+bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
 void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
 
 void virtio_save(VirtIODevice *vdev, QEMUFile *f);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 4/8] virtio: export vring_notify as virtio_should_notify

[Cornelia Huck]

On Sun, 14 Feb 2016 18:17:07 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> Virtio dataplane needs to trigger the irq manually through the
> guest notifier.  Export virtio_should_notify so that it can be
> used around event_notifier_set.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio.c         | 4 ++--
>  include/hw/virtio/virtio.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 

The name is better as well :)

Acked-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 4/8] virtio: export vring_notify as virtio_should_notify

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Virtio dataplane needs to trigger the irq manually through the
> guest notifier.  Export virtio_should_notify so that it can be
> used around event_notifier_set.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/virtio.c         | 4 ++--
>  include/hw/virtio/virtio.h | 1 +
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 3a5cca4..abb97f4 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1162,7 +1162,7 @@ void virtio_irq(VirtQueue *vq)
>      virtio_notify_vector(vq->vdev, vq->vector);
>  }
>  
> -static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
> +bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
>      uint16_t old, new;
>      bool v;
> @@ -1187,7 +1187,7 @@ static bool vring_notify(VirtIODevice *vdev, VirtQueue *vq)
>  
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq)
>  {
> -    if (!vring_notify(vdev, vq)) {
> +    if (!virtio_should_notify(vdev, vq)) {
>          return;
>      }
>  
> diff --git a/include/hw/virtio/virtio.h b/include/hw/virtio/virtio.h
> index 4ce01a1..5884228 100644
> --- a/include/hw/virtio/virtio.h
> +++ b/include/hw/virtio/virtio.h
> @@ -162,6 +162,7 @@ void virtqueue_get_avail_bytes(VirtQueue *vq, unsigned int *in_bytes,
>                                 unsigned int *out_bytes,
>                                 unsigned max_in_bytes, unsigned max_out_bytes);
>  
> +bool virtio_should_notify(VirtIODevice *vdev, VirtQueue *vq);
>  void virtio_notify(VirtIODevice *vdev, VirtQueue *vq);
>  
>  void virtio_save(VirtIODevice *vdev, QEMUFile *f);
> -- 
> 1.8.3.1
> 
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

In disabled mode, virtio-blk dataplane seems to be enabled, but flow
actually goes through the normal virtio path.  This patch simplifies a bit
the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
might be called even if s->dataplane is not NULL.

This is a bit tricky, because the current check for s->dataplane will
always trigger, causing a continuous stream of calls to
virtio_blk_data_plane_start.  Unfortunately, these calls will not
do anything.  To fix this, set the "started" flag even in disabled
mode, and skip virtio_blk_data_plane_start if the started flag is true.
The resulting changes also prepare the code for the next patch, were
virtio-blk dataplane will reuse the same virtio_blk_handle_output function
as "regular" virtio-blk.

Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
to move s->dataplane->started inside struct VirtIOBlock.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 21 +++++++++------------
 hw/block/virtio-blk.c           |  2 +-
 include/hw/virtio/virtio-blk.h  |  1 +
 3 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 03b81bc..cc521c1 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -28,7 +28,6 @@
 #include "qom/object_interfaces.h"
 
 struct VirtIOBlockDataPlane {
-    bool started;
     bool starting;
     bool stopping;
     bool disabled;
@@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtQueue *vq;
     int r;
 
-    if (s->started || s->disabled) {
-        return;
-    }
-
-    if (s->starting) {
+    if (vblk->dataplane_started || s->starting) {
         return;
     }
 
@@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     vblk->complete_request = complete_request_vring;
 
     s->starting = false;
-    s->started = true;
+    vblk->dataplane_started = true;
     trace_virtio_blk_data_plane_start(s);
 
     blk_set_aio_context(s->conf->conf.blk, s->ctx);
@@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
     vring_teardown(&s->vring, s->vdev, 0);
-    s->disabled = true;
   fail_vring:
+    s->disabled = true;
     s->starting = false;
+    vblk->dataplane_started = true;
 }
 
 /* Context: QEMU global mutex held */
@@ -331,13 +327,14 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
 
+    if (!vblk->dataplane_started || s->stopping) {
+        return;
+    }
 
     /* Better luck next time. */
     if (s->disabled) {
         s->disabled = false;
-        return;
-    }
-    if (!s->started || s->stopping) {
+        vblk->dataplane_started = false;
         return;
     }
     s->stopping = true;
@@ -364,6 +361,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, 1, false);
 
-    s->started = false;
+    vblk->dataplane_started = false;
     s->stopping = false;
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index c427698..e04c8f5 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -589,7 +589,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
     /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
      * dataplane here instead of waiting for .set_status().
      */
-    if (s->dataplane) {
+    if (s->dataplane && !s->dataplane_started) {
         virtio_blk_data_plane_start(s->dataplane);
         return;
     }
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 199bb0e..781969d 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
     /* Function to push to vq and notify guest */
     void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
     Notifier migration_state_notifier;
+    bool dataplane_started;
     struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
 
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Cornelia Huck]

On Sun, 14 Feb 2016 18:17:08 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> In disabled mode, virtio-blk dataplane seems to be enabled, but flow
> actually goes through the normal virtio path.  This patch simplifies a bit
> the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
> might be called even if s->dataplane is not NULL.
> 
> This is a bit tricky, because the current check for s->dataplane will
> always trigger, causing a continuous stream of calls to
> virtio_blk_data_plane_start.  Unfortunately, these calls will not
> do anything.  
> To fix this, set the "started" flag even in disabled
> mode, and skip virtio_blk_data_plane_start if the started flag is true.
> The resulting changes also prepare the code for the next patch, were
> virtio-blk dataplane will reuse the same virtio_blk_handle_output function
> as "regular" virtio-blk.
> 
> Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
> to move s->dataplane->started inside struct VirtIOBlock.

It seems a bit odd to me that ->started is the only state that is not
inside the dataplane struct... this approach saves a function call for
an accessor, though.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 21 +++++++++------------
>  hw/block/virtio-blk.c           |  2 +-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 

> @@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      k->set_guest_notifiers(qbus->parent, 1, false);
>    fail_guest_notifiers:
>      vring_teardown(&s->vring, s->vdev, 0);
> -    s->disabled = true;
>    fail_vring:
> +    s->disabled = true;

I originally introduced ->disabled to cover (possibly temporary)
failures to set up notifiers, but I guess this doesn't hurt.

>      s->starting = false;
> +    vblk->dataplane_started = true;
>  }
> 
>  /* Context: QEMU global mutex held */

After spending some time wrapping my head around it again, this looks
sane to me.

Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 15/02/2016 18:58, Cornelia Huck wrote:
> It seems a bit odd to me that ->started is the only state that is not
> inside the dataplane struct... this approach saves a function call for
> an accessor, though.

Actually, I can do better by moving the flag entirely within
hw/block/virtio-blk.c:

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index cc521c1..29db94b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -263,7 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     VirtQueue *vq;
     int r;
 
-    if (vblk->dataplane_started || s->starting) {
+    if (s->starting) {
         return;
     }
 
@@ -295,7 +295,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     vblk->complete_request = complete_request_vring;
 
     s->starting = false;
-    vblk->dataplane_started = true;
     trace_virtio_blk_data_plane_start(s);
 
     blk_set_aio_context(s->conf->conf.blk, s->ctx);
@@ -317,7 +316,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
   fail_vring:
     s->disabled = true;
     s->starting = false;
-    vblk->dataplane_started = true;
 }
 
 /* Context: QEMU global mutex held */
@@ -327,14 +325,13 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
 
-    if (!vblk->dataplane_started || s->stopping) {
+    if (s->stopping) {
         return;
     }
 
     /* Better luck next time. */
     if (s->disabled) {
         s->disabled = false;
-        vblk->dataplane_started = false;
         return;
     }
     s->stopping = true;
@@ -361,6 +358,5 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
     /* Clean up guest notifier (irq) */
     k->set_guest_notifiers(qbus->parent, 1, false);
 
-    vblk->dataplane_started = false;
     s->stopping = false;
 }
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e04c8f5..34bae8e 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -590,6 +590,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
      * dataplane here instead of waiting for .set_status().
      */
     if (s->dataplane && !s->dataplane_started) {
+        s->dataplane_started = true;
         virtio_blk_data_plane_start(s->dataplane);
         return;
     }
@@ -658,8 +659,9 @@ static void virtio_blk_reset(VirtIODevice *vdev)
     aio_context_acquire(ctx);
     blk_drain(s->blk);
 
-    if (s->dataplane) {
+    if (s->dataplane && s->dataplane_started) {
         virtio_blk_data_plane_stop(s->dataplane);
+        s->dataplane_started = false;
     }
     aio_context_release(ctx);
 

Does it look better?

Paolo

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Cornelia Huck]

On Tue, 16 Feb 2016 16:45:24 +0100
Paolo Bonzini <pbonzini@redhat.com> wrote:

> On 15/02/2016 18:58, Cornelia Huck wrote:
> > It seems a bit odd to me that ->started is the only state that is not
> > inside the dataplane struct... this approach saves a function call for
> > an accessor, though.
> 
> Actually, I can do better by moving the flag entirely within
> hw/block/virtio-blk.c:
> 

> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e04c8f5..34bae8e 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -590,6 +590,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>       * dataplane here instead of waiting for .set_status().
>       */
>      if (s->dataplane && !s->dataplane_started) {
> +        s->dataplane_started = true;
>          virtio_blk_data_plane_start(s->dataplane);
>          return;
>      }
> @@ -658,8 +659,9 @@ static void virtio_blk_reset(VirtIODevice *vdev)
>      aio_context_acquire(ctx);
>      blk_drain(s->blk);
> 
> -    if (s->dataplane) {
> +    if (s->dataplane && s->dataplane_started) {
>          virtio_blk_data_plane_stop(s->dataplane);
> +        s->dataplane_started = false;
>      }
>      aio_context_release(ctx);
> 
> 
> Does it look better?

I think yes.

Hm... this seems to guarantee that _start()/_stop() never runs
concurrently, doesn't it? Could we get rid of the ->starting/->stopping
flags as well?

...and ->disabled as well, since we try just once until we stop?

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 16/02/2016 17:15, Cornelia Huck wrote:
> Hm... this seems to guarantee that _start()/_stop() never runs
> concurrently, doesn't it? Could we get rid of the ->starting/->stopping
> flags as well?
> 
> ...and ->disabled as well, since we try just once until we stop?

Sounds like that, yes.

Paolo

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> In disabled mode, virtio-blk dataplane seems to be enabled, but flow
> actually goes through the normal virtio path.  This patch simplifies a bit
> the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
> might be called even if s->dataplane is not NULL.
> 
> This is a bit tricky, because the current check for s->dataplane will
> always trigger, causing a continuous stream of calls to
> virtio_blk_data_plane_start.  Unfortunately, these calls will not
> do anything.  To fix this, set the "started" flag even in disabled
> mode, and skip virtio_blk_data_plane_start if the started flag is true.
> The resulting changes also prepare the code for the next patch, were
> virtio-blk dataplane will reuse the same virtio_blk_handle_output function
> as "regular" virtio-blk.
> 
> Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
> to move s->dataplane->started inside struct VirtIOBlock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 21 +++++++++------------
>  hw/block/virtio-blk.c           |  2 +-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 03b81bc..cc521c1 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -28,7 +28,6 @@
>  #include "qom/object_interfaces.h"
>  
>  struct VirtIOBlockDataPlane {
> -    bool started;
>      bool starting;
>      bool stopping;
>      bool disabled;
> @@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      VirtQueue *vq;
>      int r;
>  
> -    if (s->started || s->disabled) {
> -        return;
> -    }
> -
> -    if (s->starting) {
> +    if (vblk->dataplane_started || s->starting) {
>          return;
>      }
>  
> @@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      vblk->complete_request = complete_request_vring;
>  
>      s->starting = false;
> -    s->started = true;
> +    vblk->dataplane_started = true;
>      trace_virtio_blk_data_plane_start(s);
>  
>      blk_set_aio_context(s->conf->conf.blk, s->ctx);
> @@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      k->set_guest_notifiers(qbus->parent, 1, false);
>    fail_guest_notifiers:
>      vring_teardown(&s->vring, s->vdev, 0);
> -    s->disabled = true;
>    fail_vring:
> +    s->disabled = true;
>      s->starting = false;

Worth a comment here, or at definition of dataplane_started, explaining the
trick said in the commit message?

Reviewed-by: Fam Zheng <famz@redhat.com>

> +    vblk->dataplane_started = true;
>  }
>  

<...>

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 02/14/2016 06:17 PM, Paolo Bonzini wrote:
> In disabled mode, virtio-blk dataplane seems to be enabled, but flow
> actually goes through the normal virtio path.  This patch simplifies a bit
> the handling of disabled mode.  In disabled mode, virtio_blk_handle_output
> might be called even if s->dataplane is not NULL.
> 
> This is a bit tricky, because the current check for s->dataplane will
> always trigger, causing a continuous stream of calls to
> virtio_blk_data_plane_start.  Unfortunately, these calls will not
> do anything.  To fix this, set the "started" flag even in disabled
> mode, and skip virtio_blk_data_plane_start if the started flag is true.
> The resulting changes also prepare the code for the next patch, were
> virtio-blk dataplane will reuse the same virtio_blk_handle_output function
> as "regular" virtio-blk.
> 
> Because struct VirtIOBlockDataPlane is opaque in virtio-blk.c, we have
> to move s->dataplane->started inside struct VirtIOBlock.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

I have some random crashes at startup 
                
                Stack trace of thread 48326:
                #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
                #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
                #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)


that I was able to bisect.
commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
2906cddfecff21af20eedab43288b485a679f9ac^ does not.

I will try to find somebody that looks into that - unless you have an idea.

Christian

> ---

>  hw/block/dataplane/virtio-blk.c | 21 +++++++++------------
>  hw/block/virtio-blk.c           |  2 +-
>  include/hw/virtio/virtio-blk.h  |  1 +
>  3 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 03b81bc..cc521c1 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -28,7 +28,6 @@
>  #include "qom/object_interfaces.h"
> 
>  struct VirtIOBlockDataPlane {
> -    bool started;
>      bool starting;
>      bool stopping;
>      bool disabled;
> @@ -264,11 +263,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      VirtQueue *vq;
>      int r;
> 
> -    if (s->started || s->disabled) {
> -        return;
> -    }
> -
> -    if (s->starting) {
> +    if (vblk->dataplane_started || s->starting) {
>          return;
>      }
> 
> @@ -300,7 +295,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      vblk->complete_request = complete_request_vring;
> 
>      s->starting = false;
> -    s->started = true;
> +    vblk->dataplane_started = true;
>      trace_virtio_blk_data_plane_start(s);
> 
>      blk_set_aio_context(s->conf->conf.blk, s->ctx);
> @@ -319,9 +314,10 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      k->set_guest_notifiers(qbus->parent, 1, false);
>    fail_guest_notifiers:
>      vring_teardown(&s->vring, s->vdev, 0);
> -    s->disabled = true;
>    fail_vring:
> +    s->disabled = true;
>      s->starting = false;
> +    vblk->dataplane_started = true;
>  }
> 
>  /* Context: QEMU global mutex held */
> @@ -331,13 +327,14 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> 
> +    if (!vblk->dataplane_started || s->stopping) {
> +        return;
> +    }
> 
>      /* Better luck next time. */
>      if (s->disabled) {
>          s->disabled = false;
> -        return;
> -    }
> -    if (!s->started || s->stopping) {
> +        vblk->dataplane_started = false;
>          return;
>      }
>      s->stopping = true;
> @@ -364,6 +361,6 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>      /* Clean up guest notifier (irq) */
>      k->set_guest_notifiers(qbus->parent, 1, false);
> 
> -    s->started = false;
> +    vblk->dataplane_started = false;
>      s->stopping = false;
>  }
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index c427698..e04c8f5 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -589,7 +589,7 @@ static void virtio_blk_handle_output(VirtIODevice *vdev, VirtQueue *vq)
>      /* Some guests kick before setting VIRTIO_CONFIG_S_DRIVER_OK so start
>       * dataplane here instead of waiting for .set_status().
>       */
> -    if (s->dataplane) {
> +    if (s->dataplane && !s->dataplane_started) {
>          virtio_blk_data_plane_start(s->dataplane);
>          return;
>      }
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 199bb0e..781969d 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -56,6 +56,7 @@ typedef struct VirtIOBlock {
>      /* Function to push to vq and notify guest */
>      void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
>      Notifier migration_state_notifier;
> +    bool dataplane_started;
>      struct VirtIOBlockDataPlane *dataplane;
>  } VirtIOBlock;
> 

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 09/03/2016 13:21, Christian Borntraeger wrote:
> I have some random crashes at startup 
>                 
>                 Stack trace of thread 48326:
>                 #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
>                 #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
>                 #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)
> 
> 
> that I was able to bisect.
> commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
> 2906cddfecff21af20eedab43288b485a679f9ac^ does not.
> 
> I will try to find somebody that looks into that - unless you have an idea.

The only random idea is to move

    vblk->dataplane_started = true

to the beginning of virtio_blk_data_plane_start rather than the end.

Paolo

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 03/09/2016 01:55 PM, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 13:21, Christian Borntraeger wrote:
>> I have some random crashes at startup 
>>                 
>>                 Stack trace of thread 48326:
>>                 #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
>>                 #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
>>                 #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)
>>
>>
>> that I was able to bisect.
>> commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
>> 2906cddfecff21af20eedab43288b485a679f9ac^ does not.
>>
>> I will try to find somebody that looks into that - unless you have an idea.
> 
> The only random idea is to move
> 
>     vblk->dataplane_started = true
> 
> to the beginning of virtio_blk_data_plane_start rather than the end.
> 
> Paolo
> 

Indeed

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 36f3d2b..1908d59 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -195,6 +195,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     if (vblk->dataplane_started || s->starting) {
         return;
     }
+    vblk->dataplane_started = true;
 
     s->starting = true;
     s->vq = virtio_get_queue(s->vdev, 0);
@@ -235,7 +236,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
   fail_guest_notifiers:
     s->disabled = true;
     s->starting = false;
-    vblk->dataplane_started = true;
 }
 
 /* Context: QEMU global mutex held */

seems to fix the issue. 

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 03/09/2016 02:02 PM, Christian Borntraeger wrote:
> On 03/09/2016 01:55 PM, Paolo Bonzini wrote:
>>
>>
>> On 09/03/2016 13:21, Christian Borntraeger wrote:
>>> I have some random crashes at startup 
>>>                 
>>>                 Stack trace of thread 48326:
>>>                 #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
>>>                 #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
>>>                 #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)
>>>
>>>
>>> that I was able to bisect.
>>> commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
>>> 2906cddfecff21af20eedab43288b485a679f9ac^ does not.
>>>
>>> I will try to find somebody that looks into that - unless you have an idea.
>>
>> The only random idea is to move
>>
>>     vblk->dataplane_started = true
>>
>> to the beginning of virtio_blk_data_plane_start rather than the end.
>>
>> Paolo
>>
> 
> Indeed
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 36f3d2b..1908d59 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -195,6 +195,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      if (vblk->dataplane_started || s->starting) {
>          return;
>      }
> +    vblk->dataplane_started = true;
>  
>      s->starting = true;
>      s->vq = virtio_get_queue(s->vdev, 0);
> @@ -235,7 +236,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>    fail_guest_notifiers:
>      s->disabled = true;
>      s->starting = false;
> -    vblk->dataplane_started = true;
>  }
>  
>  /* Context: QEMU global mutex held */
> 
> seems to fix the issue. 

Hmmm, no another crash, just seems to happen less often.

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Cornelia Huck]

On Wed, 9 Mar 2016 14:05:20 +0100
Christian Borntraeger <borntraeger@de.ibm.com> wrote:

> On 03/09/2016 02:02 PM, Christian Borntraeger wrote:
> > On 03/09/2016 01:55 PM, Paolo Bonzini wrote:
> >>
> >>
> >> On 09/03/2016 13:21, Christian Borntraeger wrote:
> >>> I have some random crashes at startup 
> >>>                 
> >>>                 Stack trace of thread 48326:
> >>>                 #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
> >>>                 #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
> >>>                 #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)
> >>>
> >>>
> >>> that I was able to bisect.
> >>> commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
> >>> 2906cddfecff21af20eedab43288b485a679f9ac^ does not.
> >>>
> >>> I will try to find somebody that looks into that - unless you have an idea.
> >>
> >> The only random idea is to move
> >>
> >>     vblk->dataplane_started = true
> >>
> >> to the beginning of virtio_blk_data_plane_start rather than the end.
> >>
> >> Paolo
> >>
> > 
> > Indeed
> > 
> > diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> > index 36f3d2b..1908d59 100644
> > --- a/hw/block/dataplane/virtio-blk.c
> > +++ b/hw/block/dataplane/virtio-blk.c
> > @@ -195,6 +195,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> >      if (vblk->dataplane_started || s->starting) {
> >          return;
> >      }
> > +    vblk->dataplane_started = true;
> >  
> >      s->starting = true;
> >      s->vq = virtio_get_queue(s->vdev, 0);
> > @@ -235,7 +236,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> >    fail_guest_notifiers:
> >      s->disabled = true;
> >      s->starting = false;
> > -    vblk->dataplane_started = true;
> >  }
> >  
> >  /* Context: QEMU global mutex held */
> > 
> > seems to fix the issue. 
> 
> Hmmm, no another crash, just seems to happen less often. 

What about the proposal in <56C34414.90809@redhat.com>, i.e. move
setting the started flag out of dataplane entirely?

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 03/09/2016 01:55 PM, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 13:21, Christian Borntraeger wrote:
>> I have some random crashes at startup 
>>                 
>>                 Stack trace of thread 48326:
>>                 #0  0x000002aa2e0cce46 bdrv_co_do_rw (qemu-system-s390x)
>>                 #1  0x000002aa2e159e8e coroutine_trampoline (qemu-system-s390x)
>>                 #2  0x000003ffbc35150a __makecontext_ret (libc.so.6)
>>
>>
>> that I was able to bisect.
>> commit 2906cddfecff21af20eedab43288b485a679f9ac does crash regularly, 
>> 2906cddfecff21af20eedab43288b485a679f9ac^ does not.
>>
>> I will try to find somebody that looks into that - unless you have an idea.
> 
> The only random idea is to move
> 
>     vblk->dataplane_started = true
> 
> to the beginning of virtio_blk_data_plane_start rather than the end.
> 
> Paolo

FWIW, it seems that this patch triggers this error, the "tracked_request_begin"
that I reported yesterday and / or some early read issues from the bootloader
in a random fashion.
Using 2906cddfecff21af20eedab43288b485a679f9ac^ seems to work all the time,
moving around vblk->dataplane_started = true also triggers all 3 types
of bugs, e.g.

Thread 1 (Thread 0x3ffaabff910 (LWP 32782)):
#0  0x0000000010329a70 in bdrv_co_do_rw (opaque=0x0) at /home/cborntra/REPOS/qemu/block/io.c:2170
#1  0x00000000103b2e7a in coroutine_trampoline (i0=1023, i1=-2147470992) at /home/cborntra/REPOS/qemu/util/coroutine-ucontext.c:79
#2  0x000003ffac85150a in __makecontext_ret () from /lib64/libc.so.6
(gdb) list
2165	
2166	/* Invoke bdrv_co_do_readv/bdrv_co_do_writev */
2167	static void coroutine_fn bdrv_co_do_rw(void *opaque)
2168	{
2169	    BlockAIOCBCoroutine *acb = opaque;
2170	    BlockDriverState *bs = acb->common.bs;
2171	
2172	    if (!acb->is_write) {
2173	        acb->req.error = bdrv_co_do_readv(bs, acb->req.sector,
2174	            acb->req.nb_sectors, acb->req.qiov, acb->req.flags);



I will try to find somebody to work on this.

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 09/03/2016 15:29, Christian Borntraeger wrote:
> FWIW, it seems that this patch triggers this error, the "tracked_request_begin"
> that I reported yesterday and / or some early read issues from the bootloader
> in a random fashion.
> Using 2906cddfecff21af20eedab43288b485a679f9ac^ seems to work all the time,
> moving around vblk->dataplane_started = true also triggers all 3 types
> of bugs

In all likelihood, the bug is that virtio_blk_handle_output is being
called in two threads.

It's not clear to me how that's possible, though.

Paolo

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Wed, 03/09 17:17, Paolo Bonzini wrote:
> 
> 
> On 09/03/2016 15:29, Christian Borntraeger wrote:
> > FWIW, it seems that this patch triggers this error, the "tracked_request_begin"
> > that I reported yesterday and / or some early read issues from the bootloader
> > in a random fashion.
> > Using 2906cddfecff21af20eedab43288b485a679f9ac^ seems to work all the time,
> > moving around vblk->dataplane_started = true also triggers all 3 types
> > of bugs
> 
> In all likelihood, the bug is that virtio_blk_handle_output is being
> called in two threads.
> 
> It's not clear to me how that's possible, though.

The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
suspicious:

       main thread                                          iothread
----------------------------------------------------------------------------
    virtio_blk_handle_output()
     virtio_blk_data_plane_start()
      vblk->dataplane_started = true;
      blk_set_aio_context()
       bdrv_set_aio_context()
        bdrv_drain()
         aio_poll()
          <snip...>
           virtio_blk_handle_output()
            /* s->dataplane_started is true */
!!!   ->    virtio_blk_handle_request()
         event_notifier_set(ioeventfd)
                                                    aio_poll()
                                                     virtio_blk_handle_request()

Christian, could you try the followed patch? The aio_poll above is replaced
with a "limited aio_poll" that doesn't disptach ioeventfd.

(Note: perhaps moving "vblk->dataplane_started = true;" after
blk_set_aio_context() also *works around* this.)

---

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
 
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-    bdrv_drain(bs); /* ensure there are no in-flight requests */
+    /* ensure there are no in-flight requests */
+    bdrv_drained_begin(bs);
+    bdrv_drained_end(bs);
 
     bdrv_detach_aio_context(bs);

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 03/10/2016 02:51 AM, Fam Zheng wrote:
[...]
> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
> suspicious:
> 
>        main thread                                          iothread
> ----------------------------------------------------------------------------
>     virtio_blk_handle_output()
>      virtio_blk_data_plane_start()
>       vblk->dataplane_started = true;
>       blk_set_aio_context()
>        bdrv_set_aio_context()
>         bdrv_drain()
>          aio_poll()
>           <snip...>
>            virtio_blk_handle_output()
>             /* s->dataplane_started is true */
> !!!   ->    virtio_blk_handle_request()
>          event_notifier_set(ioeventfd)
>                                                     aio_poll()
>                                                      virtio_blk_handle_request()
> 
> Christian, could you try the followed patch? The aio_poll above is replaced
> with a "limited aio_poll" that doesn't disptach ioeventfd.
> 
> (Note: perhaps moving "vblk->dataplane_started = true;" after
> blk_set_aio_context() also *works around* this.)
> 
> ---
> 
> diff --git a/block.c b/block.c
> index ba24b8e..e37e8f7 100644
> --- a/block.c
> +++ b/block.c
> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
> 
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>  {
> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
> +    /* ensure there are no in-flight requests */
> +    bdrv_drained_begin(bs);
> +    bdrv_drained_end(bs);
> 
>      bdrv_detach_aio_context(bs);
> 

That seems to do the trick. 

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Christian Borntraeger]

On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
> On 03/10/2016 02:51 AM, Fam Zheng wrote:
> [...]
>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
>> suspicious:
>>
>>        main thread                                          iothread
>> ----------------------------------------------------------------------------
>>     virtio_blk_handle_output()
>>      virtio_blk_data_plane_start()
>>       vblk->dataplane_started = true;
>>       blk_set_aio_context()
>>        bdrv_set_aio_context()
>>         bdrv_drain()
>>          aio_poll()
>>           <snip...>
>>            virtio_blk_handle_output()
>>             /* s->dataplane_started is true */
>> !!!   ->    virtio_blk_handle_request()
>>          event_notifier_set(ioeventfd)
>>                                                     aio_poll()
>>                                                      virtio_blk_handle_request()
>>
>> Christian, could you try the followed patch? The aio_poll above is replaced
>> with a "limited aio_poll" that doesn't disptach ioeventfd.
>>
>> (Note: perhaps moving "vblk->dataplane_started = true;" after
>> blk_set_aio_context() also *works around* this.)
>>
>> ---
>>
>> diff --git a/block.c b/block.c
>> index ba24b8e..e37e8f7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>
>>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>  {
>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>> +    /* ensure there are no in-flight requests */
>> +    bdrv_drained_begin(bs);
>> +    bdrv_drained_end(bs);
>>
>>      bdrv_detach_aio_context(bs);
>>
> 
> That seems to do the trick. 

Or not. Crashed again :-(

here is a trace with debugging enabled. The opaque value is zero, which is not good.

#0  0x0000000010329f98 in bdrv_co_do_rw (opaque=0x0) at block/io.c:2170
#1  0x00000000103b33a2 in coroutine_trampoline (i0=1023, i1=1946159824) at qemu/util/coroutine-ucontext.c:79
#2  0x000003ff7d9d150a in __makecontext_ret () from /lib64/libc.so.6

Still no idea why.

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 10/03/2016 10:40, Christian Borntraeger wrote:
> On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
>> On 03/10/2016 02:51 AM, Fam Zheng wrote:
>> [...]
>>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
>>> suspicious:
>>>
>>>        main thread                                          iothread
>>> ----------------------------------------------------------------------------
>>>     virtio_blk_handle_output()
>>>      virtio_blk_data_plane_start()
>>>       vblk->dataplane_started = true;
>>>       blk_set_aio_context()
>>>        bdrv_set_aio_context()
>>>         bdrv_drain()
>>>          aio_poll()
>>>           <snip...>
>>>            virtio_blk_handle_output()
>>>             /* s->dataplane_started is true */
>>> !!!   ->    virtio_blk_handle_request()
>>>          event_notifier_set(ioeventfd)
>>>                                                     aio_poll()
>>>                                                      virtio_blk_handle_request()
>>>
>>> Christian, could you try the followed patch? The aio_poll above is replaced
>>> with a "limited aio_poll" that doesn't disptach ioeventfd.
>>>
>>> (Note: perhaps moving "vblk->dataplane_started = true;" after
>>> blk_set_aio_context() also *works around* this.)
>>>
>>> ---
>>>
>>> diff --git a/block.c b/block.c
>>> index ba24b8e..e37e8f7 100644
>>> --- a/block.c
>>> +++ b/block.c
>>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>>
>>>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>>  {
>>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>>> +    /* ensure there are no in-flight requests */
>>> +    bdrv_drained_begin(bs);
>>> +    bdrv_drained_end(bs);
>>>
>>>      bdrv_detach_aio_context(bs);
>>>
>>
>> That seems to do the trick. 
> 
> Or not. Crashed again :-(

I would put bdrv_drained_end just before aio_context_release.

But secondarily, I'm thinking of making the logic simpler to understand 
in two ways:

1) adding a mutex around virtio_blk_data_plane_start/stop.

2) moving

    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);

to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
true starts the processing.

Thanks,

Paolo

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[tu bo]

Using the latest qemu from master, and got a new qemu crash as below,

(gdb) bt
#0  0x000003ffabb3b650 in raise () from /lib64/libc.so.6
#1  0x000003ffabb3ced8 in abort () from /lib64/libc.so.6
#2  0x0000000010384c30 in qemu_coroutine_enter (co=0x10a2ed40, 
opaque=0x0) at util/qemu-coroutine.c:112
#3  0x00000000102fd5c2 in bdrv_co_io_em_complete (opaque=0x3ff22beb518, 
ret=0) at block/io.c:2311
#4  0x00000000102f1428 in qemu_laio_process_completion (s=0x10a25e30, 
laiocb=0x3ffa400a2a0) at block/linux-aio.c:92
#5  0x00000000102f15e8 in qemu_laio_completion_bh (opaque=0x10a25e30) at 
block/linux-aio.c:139
#6  0x0000000010281d70 in aio_bh_call (bh=0x109e3580) at async.c:65
#7  0x0000000010281eb8 in aio_bh_poll (ctx=0x109efe10) at async.c:93
#8  0x000000001029538e in aio_dispatch (ctx=0x109efe10) at aio-posix.c:306
#9  0x0000000010295da6 in aio_poll (ctx=0x109efe10, blocking=false) at 
aio-posix.c:475
#10 0x000000001014662e in iothread_run (opaque=0x109ef8d0) at iothread.c:46
#11 0x000003ffabd084c6 in start_thread () from /lib64/libpthread.so.0
#12 0x000003ffabc02ec2 in thread_start () from /lib64/libc.so.6
(gdb) frame 2
#2  0x0000000010384c30 in qemu_coroutine_enter (co=0x10a2ed40, 
opaque=0x0) at util/qemu-coroutine.c:112
112	        abort();
(gdb) list
107	
108	    trace_qemu_coroutine_enter(self, co, opaque);
109	
110	    if (co->caller) {
111	        fprintf(stderr, "Co-routine re-entered recursively\n");
112	        abort();
113	    }
114	
115	    co->caller = self;
116	    co->entry_arg = opaque;


Messages in the log file of "/var/log/libvirt/qemu/" as below,
LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin 
QEMU_AUDIO_DRV=none /usr/bin/qemu-kvm -name rt_vm2 -S -machine 
s390-ccw-virtio-2.6,accel=kvm,usb=off -m 2048 -realtime mlock=off -smp 
2,sockets=2,cores=1,threads=1 -object iothread,id=iothread1 -uuid 
80cfa525-b35b-4341-aa20-a581bb528fbf -nographic -no-user-config 
-nodefaults -chardev 
socket,id=charmonitor,path=/var/lib/libvirt/qemu/domain-rt_vm2/monitor.sock,server,nowait 
-mon chardev=charmonitor,id=monitor,mode=control -rtc base=utc 
-no-shutdown -boot strict=on -drive 
file=/dev/mapper/36005076305ffc1ae0000000000008036,format=raw,if=none,id=drive-virtio-disk0,cache=none,aio=native 
-device 
virtio-blk-ccw,iothread=iothread1,scsi=off,devno=fe.0.0001,drive=drive-virtio-disk0,id=virtio-disk0,bootindex=1 
-netdev tap,fd=27,id=hostnet0,vhost=on,vhostfd=29 -device 
virtio-net-ccw,netdev=hostnet0,id=net0,mac=02:e7:24:dc:dc:11,devno=fe.0.0000 
-netdev tap,fd=30,id=hostnet1,vhost=on,vhostfd=31 -device 
virtio-net-ccw,netdev=hostnet1,id=net1,mac=52:54:00:e3:0a:44,devno=fe.0.0002 
-chardev pty,id=charconsole0 -device 
sclpconsole,chardev=charconsole0,id=console0 -device 
virtio-balloon-ccw,id=balloon0,devno=fe.3.ffba -msg timestamp=on
char device redirected to /dev/pts/6 (label charconsole0)

Co-routine re-entered recursively
2016-03-14 09:05:37.075+0000: shutting down


On 03/11/2016 06:28 PM, Paolo Bonzini wrote:
>
>
> On 10/03/2016 10:40, Christian Borntraeger wrote:
>> On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
>>> On 03/10/2016 02:51 AM, Fam Zheng wrote:
>>> [...]
>>>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
>>>> suspicious:
>>>>
>>>>         main thread                                          iothread
>>>> ----------------------------------------------------------------------------
>>>>      virtio_blk_handle_output()
>>>>       virtio_blk_data_plane_start()
>>>>        vblk->dataplane_started = true;
>>>>        blk_set_aio_context()
>>>>         bdrv_set_aio_context()
>>>>          bdrv_drain()
>>>>           aio_poll()
>>>>            <snip...>
>>>>             virtio_blk_handle_output()
>>>>              /* s->dataplane_started is true */
>>>> !!!   ->    virtio_blk_handle_request()
>>>>           event_notifier_set(ioeventfd)
>>>>                                                      aio_poll()
>>>>                                                       virtio_blk_handle_request()
>>>>
>>>> Christian, could you try the followed patch? The aio_poll above is replaced
>>>> with a "limited aio_poll" that doesn't disptach ioeventfd.
>>>>
>>>> (Note: perhaps moving "vblk->dataplane_started = true;" after
>>>> blk_set_aio_context() also *works around* this.)
>>>>
>>>> ---
>>>>
>>>> diff --git a/block.c b/block.c
>>>> index ba24b8e..e37e8f7 100644
>>>> --- a/block.c
>>>> +++ b/block.c
>>>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>>>
>>>>   void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>>>   {
>>>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>>>> +    /* ensure there are no in-flight requests */
>>>> +    bdrv_drained_begin(bs);
>>>> +    bdrv_drained_end(bs);
>>>>
>>>>       bdrv_detach_aio_context(bs);
>>>>
>>>
>>> That seems to do the trick.
>>
>> Or not. Crashed again :-(
>
> I would put bdrv_drained_end just before aio_context_release.
>
> But secondarily, I'm thinking of making the logic simpler to understand
> in two ways:
>
> 1) adding a mutex around virtio_blk_data_plane_start/stop.
>
> 2) moving
>
>      event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>      virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>
> to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
> true starts the processing.
>
> Thanks,
>
> Paolo
>

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Fri, 03/11 11:28, Paolo Bonzini wrote:
> 
> 
> On 10/03/2016 10:40, Christian Borntraeger wrote:
> > On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
> >> On 03/10/2016 02:51 AM, Fam Zheng wrote:
> >> [...]
> >>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
> >>> suspicious:
> >>>
> >>>        main thread                                          iothread
> >>> ----------------------------------------------------------------------------
> >>>     virtio_blk_handle_output()
> >>>      virtio_blk_data_plane_start()
> >>>       vblk->dataplane_started = true;
> >>>       blk_set_aio_context()
> >>>        bdrv_set_aio_context()
> >>>         bdrv_drain()
> >>>          aio_poll()
> >>>           <snip...>
> >>>            virtio_blk_handle_output()
> >>>             /* s->dataplane_started is true */
> >>> !!!   ->    virtio_blk_handle_request()
> >>>          event_notifier_set(ioeventfd)
> >>>                                                     aio_poll()
> >>>                                                      virtio_blk_handle_request()
> >>>
> >>> Christian, could you try the followed patch? The aio_poll above is replaced
> >>> with a "limited aio_poll" that doesn't disptach ioeventfd.
> >>>
> >>> (Note: perhaps moving "vblk->dataplane_started = true;" after
> >>> blk_set_aio_context() also *works around* this.)
> >>>
> >>> ---
> >>>
> >>> diff --git a/block.c b/block.c
> >>> index ba24b8e..e37e8f7 100644
> >>> --- a/block.c
> >>> +++ b/block.c
> >>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
> >>>
> >>>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
> >>>  {
> >>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
> >>> +    /* ensure there are no in-flight requests */
> >>> +    bdrv_drained_begin(bs);
> >>> +    bdrv_drained_end(bs);
> >>>
> >>>      bdrv_detach_aio_context(bs);
> >>>
> >>
> >> That seems to do the trick. 
> > 
> > Or not. Crashed again :-(
> 
> I would put bdrv_drained_end just before aio_context_release.

This won't work. bdrv_drained_end must be called with the same ctx as
bdrv_drained_begin, which is only true before bdrv_detach_aio_context().

> 
> But secondarily, I'm thinking of making the logic simpler to understand 
> in two ways:
> 
> 1) adding a mutex around virtio_blk_data_plane_start/stop.
> 
> 2) moving
> 
>     event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> 
> to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
> true starts the processing.

Like this? If it captures your idea, could Bo or Christian help test?

---

>From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
From: Fam Zheng <famz@redhat.com>
Date: Thu, 10 Mar 2016 10:26:36 +0800
Subject: [PATCH] virtio-blk dataplane start crash fix

Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Fam Zheng <famz@redhat.com>
---
 block.c                         |  4 +++-
 hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++-------
 2 files changed, 35 insertions(+), 8 deletions(-)

diff --git a/block.c b/block.c
index ba24b8e..e37e8f7 100644
--- a/block.c
+++ b/block.c
@@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
 
 void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
 {
-    bdrv_drain(bs); /* ensure there are no in-flight requests */
+    /* ensure there are no in-flight requests */
+    bdrv_drained_begin(bs);
+    bdrv_drained_end(bs);
 
     bdrv_detach_aio_context(bs);
 
diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 36f3d2b..6db5c22 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {
 
     /* Operation blocker on BDS */
     Error *blocker;
+
+    QemuMutex start_lock;
 };
 
 /* Raise an interrupt to signal guest, if necessary */
@@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
     s = g_new0(VirtIOBlockDataPlane, 1);
     s->vdev = vdev;
     s->conf = conf;
+    qemu_mutex_init(&s->start_lock);
 
     if (conf->iothread) {
         s->iothread = conf->iothread;
@@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     g_free(s);
 }
 
+typedef struct {
+    VirtIOBlockDataPlane *s;
+    QEMUBH *bh;
+} VirtIOBlockStartData;
+
+static void virtio_blk_data_plane_start_bh_cb(void *opaque)
+{
+    VirtIOBlockStartData *data = opaque;
+    VirtIOBlockDataPlane *s = data->s;
+
+    /* Kick right away to begin processing requests already in vring */
+    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
+
+    /* Get this show started by hooking up our callbacks */
+    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
+
+    qemu_bh_delete(data->bh);
+    g_free(data);
+}
+
 /* Context: QEMU global mutex held */
 void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
+    VirtIOBlockStartData *data;
     int r;
 
+    qemu_mutex_lock(&s->start_lock);
     if (vblk->dataplane_started || s->starting) {
+        qemu_mutex_unlock(&s->start_lock);
         return;
     }
 
@@ -221,13 +247,11 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 
     blk_set_aio_context(s->conf->conf.blk, s->ctx);
 
-    /* Kick right away to begin processing requests already in vring */
-    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
-
-    /* Get this show started by hooking up our callbacks */
-    aio_context_acquire(s->ctx);
-    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
-    aio_context_release(s->ctx);
+    data = g_new(VirtIOBlockStartData, 1);
+    data->s = s;
+    data->bh = aio_bh_new(s->ctx, virtio_blk_data_plane_start_bh_cb, data);
+    qemu_bh_schedule(data->bh);
+    qemu_mutex_unlock(&s->start_lock);
     return;
 
   fail_host_notifier:
@@ -236,6 +260,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     s->disabled = true;
     s->starting = false;
     vblk->dataplane_started = true;
+    qemu_mutex_unlock(&s->start_lock);
 }
 
 /* Context: QEMU global mutex held */
-- 
2.4.3

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Cornelia Huck]

On Tue, 15 Mar 2016 20:45:30 +0800
Fam Zheng <famz@redhat.com> wrote:

> On Fri, 03/11 11:28, Paolo Bonzini wrote:

> > But secondarily, I'm thinking of making the logic simpler to understand 
> > in two ways:
> > 
> > 1) adding a mutex around virtio_blk_data_plane_start/stop.
> > 
> > 2) moving
> > 
> >     event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> >     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> > 
> > to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
> > takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
> > true starts the processing.
> 
> Like this? If it captures your idea, could Bo or Christian help test?
> 
> ---
> 
> From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
> From: Fam Zheng <famz@redhat.com>
> Date: Thu, 10 Mar 2016 10:26:36 +0800
> Subject: [PATCH] virtio-blk dataplane start crash fix
> 
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  block.c                         |  4 +++-
>  hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++-------
>  2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/block.c b/block.c
> index ba24b8e..e37e8f7 100644
> --- a/block.c
> +++ b/block.c
> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
> 
>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>  {
> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
> +    /* ensure there are no in-flight requests */
> +    bdrv_drained_begin(bs);
> +    bdrv_drained_end(bs);
> 
>      bdrv_detach_aio_context(bs);
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 36f3d2b..6db5c22 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {
> 
>      /* Operation blocker on BDS */
>      Error *blocker;
> +
> +    QemuMutex start_lock;
>  };
> 
>  /* Raise an interrupt to signal guest, if necessary */
> @@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>      s = g_new0(VirtIOBlockDataPlane, 1);
>      s->vdev = vdev;
>      s->conf = conf;
> +    qemu_mutex_init(&s->start_lock);
> 
>      if (conf->iothread) {
>          s->iothread = conf->iothread;
> @@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>      g_free(s);
>  }
> 
> +typedef struct {
> +    VirtIOBlockDataPlane *s;
> +    QEMUBH *bh;
> +} VirtIOBlockStartData;
> +
> +static void virtio_blk_data_plane_start_bh_cb(void *opaque)
> +{
> +    VirtIOBlockStartData *data = opaque;
> +    VirtIOBlockDataPlane *s = data->s;

Won't you need to check here whether ->started is still set?

> +
> +    /* Kick right away to begin processing requests already in vring */
> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> +
> +    /* Get this show started by hooking up our callbacks */
> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> +
> +    qemu_bh_delete(data->bh);
> +    g_free(data);
> +}
> +
>  /* Context: QEMU global mutex held */
>  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> +    VirtIOBlockStartData *data;
>      int r;
> 
> +    qemu_mutex_lock(&s->start_lock);
>      if (vblk->dataplane_started || s->starting) {

Do we still need ->starting with the new mutex?

> +        qemu_mutex_unlock(&s->start_lock);
>          return;
>      }
> 
> @@ -221,13 +247,11 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> 
>      blk_set_aio_context(s->conf->conf.blk, s->ctx);
> 
> -    /* Kick right away to begin processing requests already in vring */
> -    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> -
> -    /* Get this show started by hooking up our callbacks */
> -    aio_context_acquire(s->ctx);
> -    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> -    aio_context_release(s->ctx);
> +    data = g_new(VirtIOBlockStartData, 1);
> +    data->s = s;
> +    data->bh = aio_bh_new(s->ctx, virtio_blk_data_plane_start_bh_cb, data);
> +    qemu_bh_schedule(data->bh);
> +    qemu_mutex_unlock(&s->start_lock);
>      return;
> 
>    fail_host_notifier:
> @@ -236,6 +260,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      s->disabled = true;
>      s->starting = false;
>      vblk->dataplane_started = true;
> +    qemu_mutex_unlock(&s->start_lock);
>  }
> 
>  /* Context: QEMU global mutex held */

Do you also need to do something in _stop()?

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Paolo Bonzini]

On 15/03/2016 14:18, Cornelia Huck wrote:
> On Tue, 15 Mar 2016 20:45:30 +0800
> Fam Zheng <famz@redhat.com> wrote:
> 
>> On Fri, 03/11 11:28, Paolo Bonzini wrote:
> 
>>> But secondarily, I'm thinking of making the logic simpler to understand 
>>> in two ways:
>>>
>>> 1) adding a mutex around virtio_blk_data_plane_start/stop.
>>>
>>> 2) moving
>>>
>>>     event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>>>     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>>>
>>> to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
>>> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
>>> true starts the processing.
>>
>> Like this? If it captures your idea, could Bo or Christian help test?
>>
>> ---
>>
>> From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
>> From: Fam Zheng <famz@redhat.com>
>> Date: Thu, 10 Mar 2016 10:26:36 +0800
>> Subject: [PATCH] virtio-blk dataplane start crash fix
>>
>> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
>> Signed-off-by: Fam Zheng <famz@redhat.com>
>> ---
>>  block.c                         |  4 +++-
>>  hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++-------
>>  2 files changed, 35 insertions(+), 8 deletions(-)
>>
>> diff --git a/block.c b/block.c
>> index ba24b8e..e37e8f7 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>
>>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>  {
>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>> +    /* ensure there are no in-flight requests */
>> +    bdrv_drained_begin(bs);
>> +    bdrv_drained_end(bs);

I'm not sure that this is necessary.  An empty section should be the
same as plain old bdrv_drain.

>>      bdrv_detach_aio_context(bs);
>>
>> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
>> index 36f3d2b..6db5c22 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {
>>
>>      /* Operation blocker on BDS */
>>      Error *blocker;
>> +
>> +    QemuMutex start_lock;
>>  };
>>
>>  /* Raise an interrupt to signal guest, if necessary */
>> @@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>>      s = g_new0(VirtIOBlockDataPlane, 1);
>>      s->vdev = vdev;
>>      s->conf = conf;
>> +    qemu_mutex_init(&s->start_lock);
>>
>>      if (conf->iothread) {
>>          s->iothread = conf->iothread;
>> @@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>>      g_free(s);
>>  }
>>
>> +typedef struct {
>> +    VirtIOBlockDataPlane *s;
>> +    QEMUBH *bh;
>> +} VirtIOBlockStartData;
>> +
>> +static void virtio_blk_data_plane_start_bh_cb(void *opaque)
>> +{
>> +    VirtIOBlockStartData *data = opaque;
>> +    VirtIOBlockDataPlane *s = data->s;
> 
> Won't you need to check here whether ->started is still set?

Yes.

>> +
>> +    /* Kick right away to begin processing requests already in vring */
>> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>> +
>> +    /* Get this show started by hooking up our callbacks */
>> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>> +
>> +    qemu_bh_delete(data->bh);
>> +    g_free(data);
>> +}
>> +
>>  /* Context: QEMU global mutex held */
>>  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>>  {
>>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
>>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
>> +    VirtIOBlockStartData *data;
>>      int r;
>>
>> +    qemu_mutex_lock(&s->start_lock);
>>      if (vblk->dataplane_started || s->starting) {
> 
> Do we still need ->starting with the new mutex?

No, but really we shouldn't have needed it before either. :)  So a task
for another day.

>> +        qemu_mutex_unlock(&s->start_lock);
>>          return;
>>      }
>>
>>  /* Context: QEMU global mutex held */
> 
> Do you also need to do something in _stop()?

_stop definitely needs to take the mutex too.

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[Fam Zheng]

On Tue, 03/15 15:08, Paolo Bonzini wrote:
> 
> 
> On 15/03/2016 14:18, Cornelia Huck wrote:
> > On Tue, 15 Mar 2016 20:45:30 +0800
> > Fam Zheng <famz@redhat.com> wrote:
> > 
> >> On Fri, 03/11 11:28, Paolo Bonzini wrote:
> > 
> >>> But secondarily, I'm thinking of making the logic simpler to understand 
> >>> in two ways:
> >>>
> >>> 1) adding a mutex around virtio_blk_data_plane_start/stop.
> >>>
> >>> 2) moving
> >>>
> >>>     event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> >>>     virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> >>>
> >>> to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
> >>> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
> >>> true starts the processing.
> >>
> >> Like this? If it captures your idea, could Bo or Christian help test?
> >>
> >> ---
> >>
> >> From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
> >> From: Fam Zheng <famz@redhat.com>
> >> Date: Thu, 10 Mar 2016 10:26:36 +0800
> >> Subject: [PATCH] virtio-blk dataplane start crash fix
> >>
> >> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> >> Signed-off-by: Fam Zheng <famz@redhat.com>
> >> ---
> >>  block.c                         |  4 +++-
> >>  hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++-------
> >>  2 files changed, 35 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/block.c b/block.c
> >> index ba24b8e..e37e8f7 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
> >>
> >>  void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
> >>  {
> >> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
> >> +    /* ensure there are no in-flight requests */
> >> +    bdrv_drained_begin(bs);
> >> +    bdrv_drained_end(bs);
> 
> I'm not sure that this is necessary.  An empty section should be the
> same as plain old bdrv_drain.

Slighly different. This wraps aio_poll of bdrv_drain with
aio_disable_external/aio_enable_external, which avoids a nested
virtio_blk_handle_output as explained in my earlier message.

> 
> >>      bdrv_detach_aio_context(bs);
> >>
> >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> >> index 36f3d2b..6db5c22 100644
> >> --- a/hw/block/dataplane/virtio-blk.c
> >> +++ b/hw/block/dataplane/virtio-blk.c
> >> @@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {
> >>
> >>      /* Operation blocker on BDS */
> >>      Error *blocker;
> >> +
> >> +    QemuMutex start_lock;
> >>  };
> >>
> >>  /* Raise an interrupt to signal guest, if necessary */
> >> @@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
> >>      s = g_new0(VirtIOBlockDataPlane, 1);
> >>      s->vdev = vdev;
> >>      s->conf = conf;
> >> +    qemu_mutex_init(&s->start_lock);
> >>
> >>      if (conf->iothread) {
> >>          s->iothread = conf->iothread;
> >> @@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
> >>      g_free(s);
> >>  }
> >>
> >> +typedef struct {
> >> +    VirtIOBlockDataPlane *s;
> >> +    QEMUBH *bh;
> >> +} VirtIOBlockStartData;
> >> +
> >> +static void virtio_blk_data_plane_start_bh_cb(void *opaque)
> >> +{
> >> +    VirtIOBlockStartData *data = opaque;
> >> +    VirtIOBlockDataPlane *s = data->s;
> > 
> > Won't you need to check here whether ->started is still set?
> 
> Yes.
> 
> >> +
> >> +    /* Kick right away to begin processing requests already in vring */
> >> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> >> +
> >> +    /* Get this show started by hooking up our callbacks */
> >> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> >> +
> >> +    qemu_bh_delete(data->bh);
> >> +    g_free(data);
> >> +}
> >> +
> >>  /* Context: QEMU global mutex held */
> >>  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
> >>  {
> >>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
> >>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> >>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> >> +    VirtIOBlockStartData *data;
> >>      int r;
> >>
> >> +    qemu_mutex_lock(&s->start_lock);
> >>      if (vblk->dataplane_started || s->starting) {
> > 
> > Do we still need ->starting with the new mutex?
> 
> No, but really we shouldn't have needed it before either. :)  So a task
> for another day.
> 
> >> +        qemu_mutex_unlock(&s->start_lock);
> >>          return;
> >>      }
> >>
> >>  /* Context: QEMU global mutex held */
> > 
> > Do you also need to do something in _stop()?
> 
> _stop definitely needs to take the mutex too.

Will fix this and above and send as a top level email.

Fam

Re: [Qemu-devel] [PATCH 5/8] virtio-blk: fix "disabled data plane" mode

[tu bo]

On 03/15/2016 08:45 PM, Fam Zheng wrote:
> On Fri, 03/11 11:28, Paolo Bonzini wrote:
>>
>>
>> On 10/03/2016 10:40, Christian Borntraeger wrote:
>>> On 03/10/2016 10:03 AM, Christian Borntraeger wrote:
>>>> On 03/10/2016 02:51 AM, Fam Zheng wrote:
>>>> [...]
>>>>> The aio_poll() inside "blk_set_aio_context(s->conf->conf.blk, s->ctx)" looks
>>>>> suspicious:
>>>>>
>>>>>         main thread                                          iothread
>>>>> ----------------------------------------------------------------------------
>>>>>      virtio_blk_handle_output()
>>>>>       virtio_blk_data_plane_start()
>>>>>        vblk->dataplane_started = true;
>>>>>        blk_set_aio_context()
>>>>>         bdrv_set_aio_context()
>>>>>          bdrv_drain()
>>>>>           aio_poll()
>>>>>            <snip...>
>>>>>             virtio_blk_handle_output()
>>>>>              /* s->dataplane_started is true */
>>>>> !!!   ->    virtio_blk_handle_request()
>>>>>           event_notifier_set(ioeventfd)
>>>>>                                                      aio_poll()
>>>>>                                                       virtio_blk_handle_request()
>>>>>
>>>>> Christian, could you try the followed patch? The aio_poll above is replaced
>>>>> with a "limited aio_poll" that doesn't disptach ioeventfd.
>>>>>
>>>>> (Note: perhaps moving "vblk->dataplane_started = true;" after
>>>>> blk_set_aio_context() also *works around* this.)
>>>>>
>>>>> ---
>>>>>
>>>>> diff --git a/block.c b/block.c
>>>>> index ba24b8e..e37e8f7 100644
>>>>> --- a/block.c
>>>>> +++ b/block.c
>>>>> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>>>>>
>>>>>   void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>>>>>   {
>>>>> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
>>>>> +    /* ensure there are no in-flight requests */
>>>>> +    bdrv_drained_begin(bs);
>>>>> +    bdrv_drained_end(bs);
>>>>>
>>>>>       bdrv_detach_aio_context(bs);
>>>>>
>>>>
>>>> That seems to do the trick.
>>>
>>> Or not. Crashed again :-(
>>
>> I would put bdrv_drained_end just before aio_context_release.
>
> This won't work. bdrv_drained_end must be called with the same ctx as
> bdrv_drained_begin, which is only true before bdrv_detach_aio_context().
>
>>
>> But secondarily, I'm thinking of making the logic simpler to understand
>> in two ways:
>>
>> 1) adding a mutex around virtio_blk_data_plane_start/stop.
>>
>> 2) moving
>>
>>      event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>>      virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>>
>> to a bottom half (created with aio_bh_new in s->ctx).  The bottom half
>> takes the mutex, checks again "if (vblk->dataplane_started)" and if it's
>> true starts the processing.
>
> Like this? If it captures your idea, could Bo or Christian help test?
>
>

With this patch, I still can get qemu crash as before,
(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x000002aa17f5a4a6 in coroutine_trampoline (i0=<optimized out>, 
i1=-1677707808) at util/coroutine-ucontext.c:79
#2  0x000003ffac25150a in __makecontext_ret () from /lib64/libc.so.6


Good news is that frequency of qemu crash is much less that before.


---
>
>  From b5b8886693828d498ee184fc7d4e13d8c06cdf39 Mon Sep 17 00:00:00 2001
> From: Fam Zheng <famz@redhat.com>
> Date: Thu, 10 Mar 2016 10:26:36 +0800
> Subject: [PATCH] virtio-blk dataplane start crash fix
>
> Suggested-by: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>   block.c                         |  4 +++-
>   hw/block/dataplane/virtio-blk.c | 39 ++++++++++++++++++++++++++++++++-------
>   2 files changed, 35 insertions(+), 8 deletions(-)
>
> diff --git a/block.c b/block.c
> index ba24b8e..e37e8f7 100644
> --- a/block.c
> +++ b/block.c
> @@ -4093,7 +4093,9 @@ void bdrv_attach_aio_context(BlockDriverState *bs,
>
>   void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context)
>   {
> -    bdrv_drain(bs); /* ensure there are no in-flight requests */
> +    /* ensure there are no in-flight requests */
> +    bdrv_drained_begin(bs);
> +    bdrv_drained_end(bs);
>
>       bdrv_detach_aio_context(bs);
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 36f3d2b..6db5c22 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -49,6 +49,8 @@ struct VirtIOBlockDataPlane {
>
>       /* Operation blocker on BDS */
>       Error *blocker;
> +
> +    QemuMutex start_lock;
>   };
>
>   /* Raise an interrupt to signal guest, if necessary */
> @@ -150,6 +152,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>       s = g_new0(VirtIOBlockDataPlane, 1);
>       s->vdev = vdev;
>       s->conf = conf;
> +    qemu_mutex_init(&s->start_lock);
>
>       if (conf->iothread) {
>           s->iothread = conf->iothread;
> @@ -184,15 +187,38 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>       g_free(s);
>   }
>
> +typedef struct {
> +    VirtIOBlockDataPlane *s;
> +    QEMUBH *bh;
> +} VirtIOBlockStartData;
> +
> +static void virtio_blk_data_plane_start_bh_cb(void *opaque)
> +{
> +    VirtIOBlockStartData *data = opaque;
> +    VirtIOBlockDataPlane *s = data->s;
> +
> +    /* Kick right away to begin processing requests already in vring */
> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> +
> +    /* Get this show started by hooking up our callbacks */
> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> +
> +    qemu_bh_delete(data->bh);
> +    g_free(data);
> +}
> +
>   /* Context: QEMU global mutex held */
>   void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>   {
>       BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
>       VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>       VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> +    VirtIOBlockStartData *data;
>       int r;
>
> +    qemu_mutex_lock(&s->start_lock);
>       if (vblk->dataplane_started || s->starting) {
> +        qemu_mutex_unlock(&s->start_lock);
>           return;
>       }
>
> @@ -221,13 +247,11 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>
>       blk_set_aio_context(s->conf->conf.blk, s->ctx);
>
> -    /* Kick right away to begin processing requests already in vring */
> -    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
> -
> -    /* Get this show started by hooking up our callbacks */
> -    aio_context_acquire(s->ctx);
> -    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
> -    aio_context_release(s->ctx);
> +    data = g_new(VirtIOBlockStartData, 1);
> +    data->s = s;
> +    data->bh = aio_bh_new(s->ctx, virtio_blk_data_plane_start_bh_cb, data);
> +    qemu_bh_schedule(data->bh);
> +    qemu_mutex_unlock(&s->start_lock);
>       return;
>
>     fail_host_notifier:
> @@ -236,6 +260,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>       s->disabled = true;
>       s->starting = false;
>       vblk->dataplane_started = true;
> +    qemu_mutex_unlock(&s->start_lock);
>   }
>
>   /* Context: QEMU global mutex held */
>

[Qemu-devel] [PATCH 6/8] virtio-blk: do not use vring in dataplane

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/dataplane/virtio-blk.c | 112 +++++-----------------------------------
 hw/block/dataplane/virtio-blk.h |   1 +
 hw/block/virtio-blk.c           |  49 +++---------------
 include/hw/virtio/virtio-blk.h  |   3 --
 4 files changed, 19 insertions(+), 146 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index cc521c1..36f3d2b 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -18,8 +18,6 @@
 #include "qemu/thread.h"
 #include "qemu/error-report.h"
 #include "hw/virtio/virtio-access.h"
-#include "hw/virtio/dataplane/vring.h"
-#include "hw/virtio/dataplane/vring-accessors.h"
 #include "sysemu/block-backend.h"
 #include "hw/virtio/virtio-blk.h"
 #include "virtio-blk.h"
@@ -35,7 +33,7 @@ struct VirtIOBlockDataPlane {
     VirtIOBlkConf *conf;
 
     VirtIODevice *vdev;
-    Vring vring;                    /* virtqueue vring */
+    VirtQueue *vq;                  /* virtqueue vring */
     EventNotifier *guest_notifier;  /* irq */
     QEMUBH *bh;                     /* bh for guest notification */
 
@@ -48,94 +46,26 @@ struct VirtIOBlockDataPlane {
      */
     IOThread *iothread;
     AioContext *ctx;
-    EventNotifier host_notifier;    /* doorbell */
 
     /* Operation blocker on BDS */
     Error *blocker;
-    void (*saved_complete_request)(struct VirtIOBlockReq *req,
-                                   unsigned char status);
 };
 
 /* Raise an interrupt to signal guest, if necessary */
-static void notify_guest(VirtIOBlockDataPlane *s)
+void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s)
 {
-    if (!vring_should_notify(s->vdev, &s->vring)) {
-        return;
-    }
-
-    event_notifier_set(s->guest_notifier);
+    qemu_bh_schedule(s->bh);
 }
 
 static void notify_guest_bh(void *opaque)
 {
     VirtIOBlockDataPlane *s = opaque;
 
-    notify_guest(s);
-}
-
-static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
-{
-    VirtIOBlockDataPlane *s = req->dev->dataplane;
-    stb_p(&req->in->status, status);
-
-    vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem, req->in_len);
-
-    /* Suppress notification to guest by BH and its scheduled
-     * flag because requests are completed as a batch after io
-     * plug & unplug is introduced, and the BH can still be
-     * executed in dataplane aio context even after it is
-     * stopped, so needn't worry about notification loss with BH.
-     */
-    qemu_bh_schedule(s->bh);
-}
-
-static void handle_notify(EventNotifier *e)
-{
-    VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
-                                           host_notifier);
-    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
-
-    event_notifier_test_and_clear(&s->host_notifier);
-    blk_io_plug(s->conf->conf.blk);
-    for (;;) {
-        MultiReqBuffer mrb = {};
-
-        /* Disable guest->host notifies to avoid unnecessary vmexits */
-        vring_disable_notification(s->vdev, &s->vring);
-
-        for (;;) {
-            VirtIOBlockReq *req = vring_pop(s->vdev, &s->vring,
-                                            sizeof(VirtIOBlockReq));
-
-            if (req == NULL) {
-                break; /* no more requests */
-            }
-
-            virtio_blk_init_request(vblk, req);
-            trace_virtio_blk_data_plane_process_request(s, req->elem.out_num,
-                                                        req->elem.in_num,
-                                                        req->elem.index);
-
-            virtio_blk_handle_request(req, &mrb);
-        }
-
-        if (mrb.num_reqs) {
-            virtio_blk_submit_multireq(s->conf->conf.blk, &mrb);
-        }
-
-        if (likely(!vring_more_avail(s->vdev, &s->vring))) { /* vring emptied */
-            /* Re-enable guest->host notifies and stop processing the vring.
-             * But if the guest has snuck in more descriptors, keep processing.
-             */
-            vring_enable_notification(s->vdev, &s->vring);
-            if (!vring_more_avail(s->vdev, &s->vring)) {
-                break;
-            }
-        } else { /* fatal error */
-            break;
-        }
+    if (!virtio_should_notify(s->vdev, s->vq)) {
+        return;
     }
-    blk_io_unplug(s->conf->conf.blk);
+
+    event_notifier_set(s->guest_notifier);
 }
 
 static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
@@ -260,7 +190,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
     VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
-    VirtQueue *vq;
     int r;
 
     if (vblk->dataplane_started || s->starting) {
@@ -268,11 +197,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     }
 
     s->starting = true;
-
-    vq = virtio_get_queue(s->vdev, 0);
-    if (!vring_setup(&s->vring, s->vdev, 0)) {
-        goto fail_vring;
-    }
+    s->vq = virtio_get_queue(s->vdev, 0);
 
     /* Set up guest notifier (irq) */
     r = k->set_guest_notifiers(qbus->parent, 1, true);
@@ -281,7 +206,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
                 "ensure -enable-kvm is set\n", r);
         goto fail_guest_notifiers;
     }
-    s->guest_notifier = virtio_queue_get_guest_notifier(vq);
+    s->guest_notifier = virtio_queue_get_guest_notifier(s->vq);
 
     /* Set up virtqueue notify */
     r = k->set_host_notifier(qbus->parent, 0, true);
@@ -289,10 +214,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
         fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
         goto fail_host_notifier;
     }
-    s->host_notifier = *virtio_queue_get_host_notifier(vq);
-
-    s->saved_complete_request = vblk->complete_request;
-    vblk->complete_request = complete_request_vring;
 
     s->starting = false;
     vblk->dataplane_started = true;
@@ -301,20 +222,17 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     blk_set_aio_context(s->conf->conf.blk, s->ctx);
 
     /* Kick right away to begin processing requests already in vring */
-    event_notifier_set(virtio_queue_get_host_notifier(vq));
+    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
 
     /* Get this show started by hooking up our callbacks */
     aio_context_acquire(s->ctx);
-    aio_set_event_notifier(s->ctx, &s->host_notifier, true,
-                           handle_notify);
+    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
     aio_context_release(s->ctx);
     return;
 
   fail_host_notifier:
     k->set_guest_notifiers(qbus->parent, 1, false);
   fail_guest_notifiers:
-    vring_teardown(&s->vring, s->vdev, 0);
-  fail_vring:
     s->disabled = true;
     s->starting = false;
     vblk->dataplane_started = true;
@@ -338,24 +256,18 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
         return;
     }
     s->stopping = true;
-    vblk->complete_request = s->saved_complete_request;
     trace_virtio_blk_data_plane_stop(s);
 
     aio_context_acquire(s->ctx);
 
     /* Stop notifications for new requests from guest */
-    aio_set_event_notifier(s->ctx, &s->host_notifier, true, NULL);
+    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
 
     /* Drain and switch bs back to the QEMU main loop */
     blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
 
     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->vring, s->vdev, 0);
-
     k->set_host_notifier(qbus->parent, 0, false);
 
     /* Clean up guest notifier (irq) */
diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
index c88d40e..0714c11 100644
--- a/hw/block/dataplane/virtio-blk.h
+++ b/hw/block/dataplane/virtio-blk.h
@@ -26,5 +26,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s);
 void virtio_blk_data_plane_drain(VirtIOBlockDataPlane *s);
+void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s);
 
 #endif /* HW_DATAPLANE_VIRTIO_BLK_H */
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index e04c8f5..cb710f1 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -21,7 +21,6 @@
 #include "sysemu/blockdev.h"
 #include "hw/virtio/virtio-blk.h"
 #include "dataplane/virtio-blk.h"
-#include "migration/migration.h"
 #include "block/scsi.h"
 #ifdef __linux__
 # include <scsi/sg.h>
@@ -45,8 +44,7 @@ void virtio_blk_free_request(VirtIOBlockReq *req)
     }
 }
 
-static void virtio_blk_complete_request(VirtIOBlockReq *req,
-                                        unsigned char status)
+static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
 {
     VirtIOBlock *s = req->dev;
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
@@ -55,12 +53,11 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
 
     stb_p(&req->in->status, status);
     virtqueue_push(s->vq, &req->elem, req->in_len);
-    virtio_notify(vdev, s->vq);
-}
-
-static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
-{
-    req->dev->complete_request(req, status);
+    if (s->dataplane) {
+        virtio_blk_data_plane_notify(s->dataplane);
+    } else {
+        virtio_notify(vdev, s->vq);
+    }
 }
 
 static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
@@ -852,36 +849,6 @@ static const BlockDevOps virtio_block_ops = {
     .resize_cb = virtio_blk_resize,
 };
 
-/* Disable dataplane thread during live migration since it does not
- * update the dirty memory bitmap yet.
- */
-static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
-{
-    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
-                                  migration_state_notifier);
-    MigrationState *mig = data;
-    Error *err = NULL;
-
-    if (migration_in_setup(mig)) {
-        if (!s->dataplane) {
-            return;
-        }
-        virtio_blk_data_plane_destroy(s->dataplane);
-        s->dataplane = NULL;
-    } else if (migration_has_finished(mig) ||
-               migration_has_failed(mig)) {
-        if (s->dataplane) {
-            return;
-        }
-        blk_drain_all(); /* complete in-flight non-dataplane requests */
-        virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->conf,
-                                     &s->dataplane, &err);
-        if (err != NULL) {
-            error_report_err(err);
-        }
-    }
-}
-
 static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -916,15 +883,12 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
     s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
 
     s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
-    s->complete_request = virtio_blk_complete_request;
     virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
     if (err != NULL) {
         error_propagate(errp, err);
         virtio_cleanup(vdev);
         return;
     }
-    s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
-    add_migration_state_change_notifier(&s->migration_state_notifier);
 
     s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
     register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
@@ -940,7 +904,6 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOBlock *s = VIRTIO_BLK(dev);
 
-    remove_migration_state_change_notifier(&s->migration_state_notifier);
     virtio_blk_data_plane_destroy(s->dataplane);
     s->dataplane = NULL;
     qemu_del_vm_change_state_handler(s->change);
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 781969d..ae84d92 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -53,9 +53,6 @@ typedef struct VirtIOBlock {
     unsigned short sector_mask;
     bool original_wce;
     VMChangeStateEntry *change;
-    /* Function to push to vq and notify guest */
-    void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
-    Notifier migration_state_notifier;
     bool dataplane_started;
     struct VirtIOBlockDataPlane *dataplane;
 } VirtIOBlock;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 6/8] virtio-blk: do not use vring in dataplane

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 112 +++++-----------------------------------
>  hw/block/dataplane/virtio-blk.h |   1 +
>  hw/block/virtio-blk.c           |  49 +++---------------
>  include/hw/virtio/virtio-blk.h  |   3 --
>  4 files changed, 19 insertions(+), 146 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index cc521c1..36f3d2b 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -18,8 +18,6 @@
>  #include "qemu/thread.h"
>  #include "qemu/error-report.h"
>  #include "hw/virtio/virtio-access.h"
> -#include "hw/virtio/dataplane/vring.h"
> -#include "hw/virtio/dataplane/vring-accessors.h"
>  #include "sysemu/block-backend.h"
>  #include "hw/virtio/virtio-blk.h"
>  #include "virtio-blk.h"
> @@ -35,7 +33,7 @@ struct VirtIOBlockDataPlane {
>      VirtIOBlkConf *conf;
>  
>      VirtIODevice *vdev;
> -    Vring vring;                    /* virtqueue vring */
> +    VirtQueue *vq;                  /* virtqueue vring */
>      EventNotifier *guest_notifier;  /* irq */
>      QEMUBH *bh;                     /* bh for guest notification */
>  
> @@ -48,94 +46,26 @@ struct VirtIOBlockDataPlane {
>       */
>      IOThread *iothread;
>      AioContext *ctx;
> -    EventNotifier host_notifier;    /* doorbell */
>  
>      /* Operation blocker on BDS */
>      Error *blocker;
> -    void (*saved_complete_request)(struct VirtIOBlockReq *req,
> -                                   unsigned char status);
>  };
>  
>  /* Raise an interrupt to signal guest, if necessary */
> -static void notify_guest(VirtIOBlockDataPlane *s)
> +void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s)
>  {
> -    if (!vring_should_notify(s->vdev, &s->vring)) {
> -        return;
> -    }
> -
> -    event_notifier_set(s->guest_notifier);
> +    qemu_bh_schedule(s->bh);
>  }
>  
>  static void notify_guest_bh(void *opaque)
>  {
>      VirtIOBlockDataPlane *s = opaque;
>  
> -    notify_guest(s);
> -}
> -
> -static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
> -{
> -    VirtIOBlockDataPlane *s = req->dev->dataplane;
> -    stb_p(&req->in->status, status);
> -
> -    vring_push(s->vdev, &req->dev->dataplane->vring, &req->elem, req->in_len);
> -
> -    /* Suppress notification to guest by BH and its scheduled
> -     * flag because requests are completed as a batch after io
> -     * plug & unplug is introduced, and the BH can still be
> -     * executed in dataplane aio context even after it is
> -     * stopped, so needn't worry about notification loss with BH.
> -     */
> -    qemu_bh_schedule(s->bh);
> -}
> -
> -static void handle_notify(EventNotifier *e)
> -{
> -    VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane,
> -                                           host_notifier);
> -    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> -
> -    event_notifier_test_and_clear(&s->host_notifier);
> -    blk_io_plug(s->conf->conf.blk);
> -    for (;;) {
> -        MultiReqBuffer mrb = {};
> -
> -        /* Disable guest->host notifies to avoid unnecessary vmexits */
> -        vring_disable_notification(s->vdev, &s->vring);
> -
> -        for (;;) {
> -            VirtIOBlockReq *req = vring_pop(s->vdev, &s->vring,
> -                                            sizeof(VirtIOBlockReq));
> -
> -            if (req == NULL) {
> -                break; /* no more requests */
> -            }
> -
> -            virtio_blk_init_request(vblk, req);
> -            trace_virtio_blk_data_plane_process_request(s, req->elem.out_num,
> -                                                        req->elem.in_num,
> -                                                        req->elem.index);
> -
> -            virtio_blk_handle_request(req, &mrb);
> -        }
> -
> -        if (mrb.num_reqs) {
> -            virtio_blk_submit_multireq(s->conf->conf.blk, &mrb);
> -        }
> -
> -        if (likely(!vring_more_avail(s->vdev, &s->vring))) { /* vring emptied */
> -            /* Re-enable guest->host notifies and stop processing the vring.
> -             * But if the guest has snuck in more descriptors, keep processing.
> -             */
> -            vring_enable_notification(s->vdev, &s->vring);
> -            if (!vring_more_avail(s->vdev, &s->vring)) {
> -                break;
> -            }
> -        } else { /* fatal error */
> -            break;
> -        }
> +    if (!virtio_should_notify(s->vdev, s->vq)) {
> +        return;
>      }
> -    blk_io_unplug(s->conf->conf.blk);
> +
> +    event_notifier_set(s->guest_notifier);
>  }
>  
>  static void data_plane_set_up_op_blockers(VirtIOBlockDataPlane *s)
> @@ -260,7 +190,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
>      VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
> -    VirtQueue *vq;
>      int r;
>  
>      if (vblk->dataplane_started || s->starting) {
> @@ -268,11 +197,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      }
>  
>      s->starting = true;
> -
> -    vq = virtio_get_queue(s->vdev, 0);
> -    if (!vring_setup(&s->vring, s->vdev, 0)) {
> -        goto fail_vring;
> -    }
> +    s->vq = virtio_get_queue(s->vdev, 0);
>  
>      /* Set up guest notifier (irq) */
>      r = k->set_guest_notifiers(qbus->parent, 1, true);
> @@ -281,7 +206,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>                  "ensure -enable-kvm is set\n", r);
>          goto fail_guest_notifiers;
>      }
> -    s->guest_notifier = virtio_queue_get_guest_notifier(vq);
> +    s->guest_notifier = virtio_queue_get_guest_notifier(s->vq);
>  
>      /* Set up virtqueue notify */
>      r = k->set_host_notifier(qbus->parent, 0, true);
> @@ -289,10 +214,6 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>          fprintf(stderr, "virtio-blk failed to set host notifier (%d)\n", r);
>          goto fail_host_notifier;
>      }
> -    s->host_notifier = *virtio_queue_get_host_notifier(vq);
> -
> -    s->saved_complete_request = vblk->complete_request;
> -    vblk->complete_request = complete_request_vring;
>  
>      s->starting = false;
>      vblk->dataplane_started = true;
> @@ -301,20 +222,17 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      blk_set_aio_context(s->conf->conf.blk, s->ctx);
>  
>      /* Kick right away to begin processing requests already in vring */
> -    event_notifier_set(virtio_queue_get_host_notifier(vq));
> +    event_notifier_set(virtio_queue_get_host_notifier(s->vq));
>  
>      /* Get this show started by hooking up our callbacks */
>      aio_context_acquire(s->ctx);
> -    aio_set_event_notifier(s->ctx, &s->host_notifier, true,
> -                           handle_notify);
> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, true, true);
>      aio_context_release(s->ctx);
>      return;
>  
>    fail_host_notifier:
>      k->set_guest_notifiers(qbus->parent, 1, false);
>    fail_guest_notifiers:
> -    vring_teardown(&s->vring, s->vdev, 0);
> -  fail_vring:
>      s->disabled = true;
>      s->starting = false;
>      vblk->dataplane_started = true;
> @@ -338,24 +256,18 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s)
>          return;
>      }
>      s->stopping = true;
> -    vblk->complete_request = s->saved_complete_request;
>      trace_virtio_blk_data_plane_stop(s);
>  
>      aio_context_acquire(s->ctx);
>  
>      /* Stop notifications for new requests from guest */
> -    aio_set_event_notifier(s->ctx, &s->host_notifier, true, NULL);
> +    virtio_queue_aio_set_host_notifier_handler(s->vq, s->ctx, false, false);
>  
>      /* Drain and switch bs back to the QEMU main loop */
>      blk_set_aio_context(s->conf->conf.blk, qemu_get_aio_context());
>  
>      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->vring, s->vdev, 0);
> -
>      k->set_host_notifier(qbus->parent, 0, false);
>  
>      /* Clean up guest notifier (irq) */
> diff --git a/hw/block/dataplane/virtio-blk.h b/hw/block/dataplane/virtio-blk.h
> index c88d40e..0714c11 100644
> --- a/hw/block/dataplane/virtio-blk.h
> +++ b/hw/block/dataplane/virtio-blk.h
> @@ -26,5 +26,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s);
>  void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s);
>  void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s);
>  void virtio_blk_data_plane_drain(VirtIOBlockDataPlane *s);
> +void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s);
>  
>  #endif /* HW_DATAPLANE_VIRTIO_BLK_H */
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index e04c8f5..cb710f1 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -21,7 +21,6 @@
>  #include "sysemu/blockdev.h"
>  #include "hw/virtio/virtio-blk.h"
>  #include "dataplane/virtio-blk.h"
> -#include "migration/migration.h"
>  #include "block/scsi.h"
>  #ifdef __linux__
>  # include <scsi/sg.h>
> @@ -45,8 +44,7 @@ void virtio_blk_free_request(VirtIOBlockReq *req)
>      }
>  }
>  
> -static void virtio_blk_complete_request(VirtIOBlockReq *req,
> -                                        unsigned char status)
> +static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
>  {
>      VirtIOBlock *s = req->dev;
>      VirtIODevice *vdev = VIRTIO_DEVICE(s);
> @@ -55,12 +53,11 @@ static void virtio_blk_complete_request(VirtIOBlockReq *req,
>  
>      stb_p(&req->in->status, status);
>      virtqueue_push(s->vq, &req->elem, req->in_len);
> -    virtio_notify(vdev, s->vq);
> -}
> -
> -static void virtio_blk_req_complete(VirtIOBlockReq *req, unsigned char status)
> -{
> -    req->dev->complete_request(req, status);
> +    if (s->dataplane) {
> +        virtio_blk_data_plane_notify(s->dataplane);
> +    } else {
> +        virtio_notify(vdev, s->vq);
> +    }
>  }
>  
>  static int virtio_blk_handle_rw_error(VirtIOBlockReq *req, int error,
> @@ -852,36 +849,6 @@ static const BlockDevOps virtio_block_ops = {
>      .resize_cb = virtio_blk_resize,
>  };
>  
> -/* Disable dataplane thread during live migration since it does not
> - * update the dirty memory bitmap yet.
> - */
> -static void virtio_blk_migration_state_changed(Notifier *notifier, void *data)
> -{
> -    VirtIOBlock *s = container_of(notifier, VirtIOBlock,
> -                                  migration_state_notifier);
> -    MigrationState *mig = data;
> -    Error *err = NULL;
> -
> -    if (migration_in_setup(mig)) {
> -        if (!s->dataplane) {
> -            return;
> -        }
> -        virtio_blk_data_plane_destroy(s->dataplane);
> -        s->dataplane = NULL;
> -    } else if (migration_has_finished(mig) ||
> -               migration_has_failed(mig)) {
> -        if (s->dataplane) {
> -            return;
> -        }
> -        blk_drain_all(); /* complete in-flight non-dataplane requests */
> -        virtio_blk_data_plane_create(VIRTIO_DEVICE(s), &s->conf,
> -                                     &s->dataplane, &err);
> -        if (err != NULL) {
> -            error_report_err(err);
> -        }
> -    }
> -}
> -
>  static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>  {
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> @@ -916,15 +883,12 @@ static void virtio_blk_device_realize(DeviceState *dev, Error **errp)
>      s->sector_mask = (s->conf.conf.logical_block_size / BDRV_SECTOR_SIZE) - 1;
>  
>      s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
> -    s->complete_request = virtio_blk_complete_request;
>      virtio_blk_data_plane_create(vdev, conf, &s->dataplane, &err);
>      if (err != NULL) {
>          error_propagate(errp, err);
>          virtio_cleanup(vdev);
>          return;
>      }
> -    s->migration_state_notifier.notify = virtio_blk_migration_state_changed;
> -    add_migration_state_change_notifier(&s->migration_state_notifier);
>  
>      s->change = qemu_add_vm_change_state_handler(virtio_blk_dma_restart_cb, s);
>      register_savevm(dev, "virtio-blk", virtio_blk_id++, 2,
> @@ -940,7 +904,6 @@ static void virtio_blk_device_unrealize(DeviceState *dev, Error **errp)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOBlock *s = VIRTIO_BLK(dev);
>  
> -    remove_migration_state_change_notifier(&s->migration_state_notifier);
>      virtio_blk_data_plane_destroy(s->dataplane);
>      s->dataplane = NULL;
>      qemu_del_vm_change_state_handler(s->change);
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 781969d..ae84d92 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -53,9 +53,6 @@ typedef struct VirtIOBlock {
>      unsigned short sector_mask;
>      bool original_wce;
>      VMChangeStateEntry *change;
> -    /* Function to push to vq and notify guest */
> -    void (*complete_request)(struct VirtIOBlockReq *req, unsigned char status);
> -    Notifier migration_state_notifier;
>      bool dataplane_started;
>      struct VirtIOBlockDataPlane *dataplane;
>  } VirtIOBlock;
> -- 
> 1.8.3.1
> 
> 
> 

Reviewed-by: Fam Zheng <famz@redhat.com>

[Qemu-devel] [PATCH 7/8] virtio-scsi: do not use vring in dataplane

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/scsi/virtio-scsi-dataplane.c | 196 +++++-----------------------------------
 hw/scsi/virtio-scsi.c           |  52 ++---------
 include/hw/virtio/virtio-scsi.h |  21 +----
 3 files changed, 35 insertions(+), 234 deletions(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 8340326..367e476 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -39,14 +39,10 @@ void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread)
     }
 }
 
-static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
-                                               VirtQueue *vq,
-                                               EventNotifierHandler *handler,
-                                               int n)
+static int virtio_scsi_vring_init(VirtIOSCSI *s, VirtQueue *vq, int n)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
-    VirtIOSCSIVring *r;
     int rc;
 
     /* Set up virtqueue notify */
@@ -55,105 +51,17 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI *s,
         fprintf(stderr, "virtio-scsi: Failed to set host notifier (%d)\n",
                 rc);
         s->dataplane_fenced = true;
-        return NULL;
+        return rc;
     }
 
-    r = g_new(VirtIOSCSIVring, 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, true, handler);
-
-    r->parent = s;
-
-    if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
-        fprintf(stderr, "virtio-scsi: VRing setup failed\n");
-        goto fail_vring;
-    }
-    return r;
-
-fail_vring:
-    aio_set_event_notifier(s->ctx, &r->host_notifier, true, NULL);
-    k->set_host_notifier(qbus->parent, n, false);
-    g_free(r);
-    return NULL;
-}
-
-VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
-                                         VirtIOSCSIVring *vring)
-{
-    VirtIOSCSICommon *vs = (VirtIOSCSICommon *)s;
-    VirtIOSCSIReq *req;
-
-    req = vring_pop((VirtIODevice *)s, &vring->vring,
-                    sizeof(VirtIOSCSIReq) + vs->cdb_size);
-    if (!req) {
-        return NULL;
-    }
-    virtio_scsi_init_req(s, NULL, req);
-    req->vring = vring;
-    return req;
-}
-
-void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(req->vring->parent);
-
-    vring_push(vdev, &req->vring->vring, &req->elem,
-               req->qsgl.size + req->resp_iov.size);
-
-    if (vring_should_notify(vdev, &req->vring->vring)) {
-        event_notifier_set(&req->vring->guest_notifier);
-    }
+    virtio_queue_aio_set_host_notifier_handler(vq, s->ctx, true, true);
+    return 0;
 }
 
-static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier)
+void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req)
 {
-    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);
-    VirtIOSCSI *s = vring->parent;
-    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 = (VirtIOSCSI *)vring->parent;
-    VirtIOSCSIReq *req, *next;
-    QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
-
-    event_notifier_test_and_clear(notifier);
-    while ((req = virtio_scsi_pop_req_vring(s, vring))) {
-        if (virtio_scsi_handle_cmd_req_prepare(s, req)) {
-            QTAILQ_INSERT_TAIL(&reqs, req, next);
-        }
-    }
-
-    QTAILQ_FOREACH_SAFE(req, &reqs, next, next) {
-        virtio_scsi_handle_cmd_req_submit(s, req);
+    if (virtio_should_notify(vdev, req->vq)) {
+        event_notifier_set(virtio_queue_get_guest_notifier(req->vq));
     }
 }
 
@@ -163,46 +71,10 @@ static void virtio_scsi_clear_aio(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
-    if (s->ctrl_vring) {
-        aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                               true, NULL);
-    }
-    if (s->event_vring) {
-        aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                               true, NULL);
-    }
-    if (s->cmd_vrings) {
-        for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                                   true, NULL);
-        }
-    }
-}
-
-static void virtio_scsi_vring_teardown(VirtIOSCSI *s)
-{
-    VirtIODevice *vdev = VIRTIO_DEVICE(s);
-    VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
-    int i;
-
-    if (s->ctrl_vring) {
-        vring_teardown(&s->ctrl_vring->vring, vdev, 0);
-        g_free(s->ctrl_vring);
-        s->ctrl_vring = NULL;
-    }
-    if (s->event_vring) {
-        vring_teardown(&s->event_vring->vring, vdev, 1);
-        g_free(s->event_vring);
-        s->event_vring = NULL;
-    }
-    if (s->cmd_vrings) {
-        for (i = 0; i < vs->conf.num_queues && s->cmd_vrings[i]; i++) {
-            vring_teardown(&s->cmd_vrings[i]->vring, vdev, 2 + i);
-            g_free(s->cmd_vrings[i]);
-            s->cmd_vrings[i] = NULL;
-        }
-        free(s->cmd_vrings);
-        s->cmd_vrings = NULL;
+    virtio_queue_aio_set_host_notifier_handler(vs->ctrl_vq, s->ctx, false, false);
+    virtio_queue_aio_set_host_notifier_handler(vs->event_vq, s->ctx, false, false);
+    for (i = 0; i < vs->conf.num_queues; i++) {
+        virtio_queue_aio_set_host_notifier_handler(vs->cmd_vqs[i], s->ctx, false, false);
     }
 }
 
@@ -229,30 +101,21 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     if (rc != 0) {
         fprintf(stderr, "virtio-scsi: Failed to set guest notifiers (%d), "
                 "ensure -enable-kvm is set\n", rc);
-        s->dataplane_fenced = true;
         goto fail_guest_notifiers;
     }
 
     aio_context_acquire(s->ctx);
-    s->ctrl_vring = virtio_scsi_vring_init(s, vs->ctrl_vq,
-                                           virtio_scsi_iothread_handle_ctrl,
-                                           0);
-    if (!s->ctrl_vring) {
+    rc = virtio_scsi_vring_init(s, vs->ctrl_vq, 0);
+    if (rc) {
         goto fail_vrings;
     }
-    s->event_vring = virtio_scsi_vring_init(s, vs->event_vq,
-                                            virtio_scsi_iothread_handle_event,
-                                            1);
-    if (!s->event_vring) {
+    rc = virtio_scsi_vring_init(s, vs->event_vq, 1);
+    if (rc) {
         goto fail_vrings;
     }
-    s->cmd_vrings = g_new(VirtIOSCSIVring *, vs->conf.num_queues);
     for (i = 0; i < vs->conf.num_queues; i++) {
-        s->cmd_vrings[i] =
-            virtio_scsi_vring_init(s, vs->cmd_vqs[i],
-                                   virtio_scsi_iothread_handle_cmd,
-                                   i + 2);
-        if (!s->cmd_vrings[i]) {
+        rc = virtio_scsi_vring_init(s, vs->cmd_vqs[i], i + 2);
+        if (rc) {
             goto fail_vrings;
         }
     }
@@ -265,13 +128,14 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
 fail_vrings:
     virtio_scsi_clear_aio(s);
     aio_context_release(s->ctx);
-    virtio_scsi_vring_teardown(s);
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         k->set_host_notifier(qbus->parent, i, false);
     }
     k->set_guest_notifiers(qbus->parent, vs->conf.num_queues + 2, false);
 fail_guest_notifiers:
+    s->dataplane_fenced = true;
     s->dataplane_starting = false;
+    s->dataplane_started = true;
 }
 
 /* Context: QEMU global mutex held */
@@ -282,12 +146,14 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(s);
     int i;
 
+    if (!s->dataplane_started || s->dataplane_stopping) {
+        return;
+    }
+
     /* Better luck next time. */
     if (s->dataplane_fenced) {
         s->dataplane_fenced = false;
-        return;
-    }
-    if (!s->dataplane_started || s->dataplane_stopping) {
+        s->dataplane_started = false;
         return;
     }
     s->dataplane_stopping = true;
@@ -295,24 +161,12 @@ void virtio_scsi_dataplane_stop(VirtIOSCSI *s)
 
     aio_context_acquire(s->ctx);
 
-    aio_set_event_notifier(s->ctx, &s->ctrl_vring->host_notifier,
-                           true, NULL);
-    aio_set_event_notifier(s->ctx, &s->event_vring->host_notifier,
-                           true, NULL);
-    for (i = 0; i < vs->conf.num_queues; i++) {
-        aio_set_event_notifier(s->ctx, &s->cmd_vrings[i]->host_notifier,
-                               true, NULL);
-    }
+    virtio_scsi_clear_aio(s);
 
     blk_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.
-     */
-    virtio_scsi_vring_teardown(s);
-
     for (i = 0; i < vs->conf.num_queues + 2; i++) {
         k->set_host_notifier(qbus->parent, i, false);
     }
diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
index 5b29bac..0c30d2e 100644
--- a/hw/scsi/virtio-scsi.c
+++ b/hw/scsi/virtio-scsi.c
@@ -23,7 +23,6 @@
 #include <block/scsi.h>
 #include <hw/virtio/virtio-bus.h>
 #include "hw/virtio/virtio-access.h"
-#include "migration/migration.h"
 
 static inline int virtio_scsi_get_lun(uint8_t *lun)
 {
@@ -43,7 +42,8 @@ static inline SCSIDevice *virtio_scsi_device_find(VirtIOSCSI *s, uint8_t *lun)
 
 void virtio_scsi_init_req(VirtIOSCSI *s, VirtQueue *vq, VirtIOSCSIReq *req)
 {
-    const size_t zero_skip = offsetof(VirtIOSCSIReq, vring);
+    const size_t zero_skip =
+        offsetof(VirtIOSCSIReq, resp_iov) + sizeof(req->resp_iov);
 
     req->vq = vq;
     req->dev = s;
@@ -66,11 +66,10 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
     VirtIODevice *vdev = VIRTIO_DEVICE(s);
 
     qemu_iovec_from_buf(&req->resp_iov, 0, &req->resp, req->resp_size);
-    if (req->vring) {
-        assert(req->vq == NULL);
-        virtio_scsi_vring_push_notify(req);
+    virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
+    if (s->dataplane_started) {
+        virtio_scsi_dataplane_notify(vdev, req);
     } else {
-        virtqueue_push(vq, &req->elem, req->qsgl.size + req->resp_iov.size);
         virtio_notify(vdev, vq);
     }
 
@@ -417,7 +416,7 @@ static void virtio_scsi_handle_ctrl(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSI *s = (VirtIOSCSI *)vdev;
     VirtIOSCSIReq *req;
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -567,7 +566,7 @@ static void virtio_scsi_handle_cmd(VirtIODevice *vdev, VirtQueue *vq)
     VirtIOSCSIReq *req, *next;
     QTAILQ_HEAD(, VirtIOSCSIReq) reqs = QTAILQ_HEAD_INITIALIZER(reqs);
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -687,11 +686,7 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
         aio_context_acquire(s->ctx);
     }
 
-    if (s->dataplane_started) {
-        req = virtio_scsi_pop_req_vring(s, s->event_vring);
-    } else {
-        req = virtio_scsi_pop_req(s, vs->event_vq);
-    }
+    req = virtio_scsi_pop_req(s, vs->event_vq);
     if (!req) {
         s->events_dropped = true;
         goto out;
@@ -733,7 +728,7 @@ static void virtio_scsi_handle_event(VirtIODevice *vdev, VirtQueue *vq)
 {
     VirtIOSCSI *s = VIRTIO_SCSI(vdev);
 
-    if (s->ctx && !s->dataplane_disabled) {
+    if (s->ctx && !s->dataplane_started) {
         virtio_scsi_dataplane_start(s);
         return;
     }
@@ -901,31 +896,6 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
     }
 }
 
-/* Disable dataplane thread during live migration since it does not
- * update the dirty memory bitmap yet.
- */
-static void virtio_scsi_migration_state_changed(Notifier *notifier, void *data)
-{
-    VirtIOSCSI *s = container_of(notifier, VirtIOSCSI,
-                                 migration_state_notifier);
-    MigrationState *mig = data;
-
-    if (migration_in_setup(mig)) {
-        if (!s->dataplane_started) {
-            return;
-        }
-        virtio_scsi_dataplane_stop(s);
-        s->dataplane_disabled = true;
-    } else if (migration_has_finished(mig) ||
-               migration_has_failed(mig)) {
-        if (s->dataplane_started) {
-            return;
-        }
-        blk_drain_all(); /* complete in-flight non-dataplane requests */
-        s->dataplane_disabled = false;
-    }
-}
-
 static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
 {
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
@@ -956,8 +926,6 @@ static void virtio_scsi_device_realize(DeviceState *dev, Error **errp)
 
     register_savevm(dev, "virtio-scsi", virtio_scsi_id++, 1,
                     virtio_scsi_save, virtio_scsi_load, s);
-    s->migration_state_notifier.notify = virtio_scsi_migration_state_changed;
-    add_migration_state_change_notifier(&s->migration_state_notifier);
 
     error_setg(&s->blocker, "block device is in use by data plane");
 
@@ -991,8 +959,6 @@ static void virtio_scsi_device_unrealize(DeviceState *dev, Error **errp)
     error_free(s->blocker);
 
     unregister_savevm(dev, "virtio-scsi", s);
-    remove_migration_state_change_notifier(&s->migration_state_notifier);
-
     virtio_scsi_common_unrealize(dev, errp);
 }
 
diff --git a/include/hw/virtio/virtio-scsi.h b/include/hw/virtio/virtio-scsi.h
index a8029aa..209eaa4 100644
--- a/include/hw/virtio/virtio-scsi.h
+++ b/include/hw/virtio/virtio-scsi.h
@@ -22,7 +22,6 @@
 #include "hw/pci/pci.h"
 #include "hw/scsi/scsi.h"
 #include "sysemu/iothread.h"
-#include "hw/virtio/dataplane/vring.h"
 
 #define TYPE_VIRTIO_SCSI_COMMON "virtio-scsi-common"
 #define VIRTIO_SCSI_COMMON(obj) \
@@ -58,13 +57,6 @@ struct VirtIOSCSIConf {
 
 struct VirtIOSCSI;
 
-typedef struct {
-    struct VirtIOSCSI *parent;
-    Vring vring;
-    EventNotifier host_notifier;
-    EventNotifier guest_notifier;
-} VirtIOSCSIVring;
-
 typedef struct VirtIOSCSICommon {
     VirtIODevice parent_obj;
     VirtIOSCSIConf conf;
@@ -96,18 +88,12 @@ typedef struct VirtIOSCSI {
     QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) insert_notifiers;
     QTAILQ_HEAD(, VirtIOSCSIBlkChangeNotifier) remove_notifiers;
 
-    /* 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;
     bool dataplane_disabled;
     bool dataplane_fenced;
     Error *blocker;
-    Notifier migration_state_notifier;
     uint32_t host_features;
 } VirtIOSCSI;
 
@@ -123,9 +109,6 @@ typedef struct VirtIOSCSIReq {
     QEMUSGList qsgl;
     QEMUIOVector resp_iov;
 
-    /* Set by dataplane code. */
-    VirtIOSCSIVring *vring;
-
     union {
         /* Used for two-stage request submission */
         QTAILQ_ENTRY(VirtIOSCSIReq) next;
@@ -168,8 +151,6 @@ void virtio_scsi_push_event(VirtIOSCSI *s, SCSIDevice *dev,
 void virtio_scsi_set_iothread(VirtIOSCSI *s, IOThread *iothread);
 void virtio_scsi_dataplane_start(VirtIOSCSI *s);
 void virtio_scsi_dataplane_stop(VirtIOSCSI *s);
-void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req);
-VirtIOSCSIReq *virtio_scsi_pop_req_vring(VirtIOSCSI *s,
-                                         VirtIOSCSIVring *vring);
+void virtio_scsi_dataplane_notify(VirtIODevice *vdev, VirtIOSCSIReq *req);
 
 #endif /* _QEMU_VIRTIO_SCSI_H */
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 7/8] virtio-scsi: do not use vring in dataplane

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
>  typedef struct VirtIOSCSICommon {

<...>

>      bool dataplane_disabled;

This was only set by virtio_scsi_migration_state_changed and should be replaced
by dataplane_started and dataplane_fenced now.

Fam

[Qemu-devel] [PATCH 8/8] vring: remove

[Paolo Bonzini]

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/virtio/Makefile.objs                       |   1 -
 hw/virtio/dataplane/Makefile.objs             |   1 -
 hw/virtio/dataplane/vring.c                   | 548 --------------------------
 include/hw/virtio/dataplane/vring-accessors.h |  75 ----
 include/hw/virtio/dataplane/vring.h           |  51 ---
 trace-events                                  |   3 -
 6 files changed, 679 deletions(-)
 delete mode 100644 hw/virtio/dataplane/Makefile.objs
 delete mode 100644 hw/virtio/dataplane/vring.c
 delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
 delete mode 100644 include/hw/virtio/dataplane/vring.h

diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
index 19b224a..3e2b175 100644
--- a/hw/virtio/Makefile.objs
+++ b/hw/virtio/Makefile.objs
@@ -2,7 +2,6 @@ common-obj-y += virtio-rng.o
 common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
 common-obj-y += virtio-bus.o
 common-obj-y += virtio-mmio.o
-obj-$(CONFIG_VIRTIO) += dataplane/
 
 obj-y += virtio.o virtio-balloon.o 
 obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Makefile.objs
deleted file mode 100644
index 753a9ca..0000000
--- a/hw/virtio/dataplane/Makefile.objs
+++ /dev/null
@@ -1 +0,0 @@
-obj-y += vring.o
diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
deleted file mode 100644
index 157e8b8..0000000
--- a/hw/virtio/dataplane/vring.c
+++ /dev/null
@@ -1,548 +0,0 @@
-/* Copyright 2012 Red Hat, Inc.
- * Copyright IBM, Corp. 2012
- *
- * Based on Linux 2.6.39 vhost code:
- * Copyright (C) 2009 Red Hat, Inc.
- * Copyright (C) 2006 Rusty Russell IBM Corporation
- *
- * Author: Michael S. Tsirkin <mst@redhat.com>
- *         Stefan Hajnoczi <stefanha@redhat.com>
- *
- * Inspiration, some code, and most witty comments come from
- * Documentation/virtual/lguest/lguest.c, by Rusty Russell
- *
- * This work is licensed under the terms of the GNU GPL, version 2.
- */
-
-#include "qemu/osdep.h"
-#include "trace.h"
-#include "hw/hw.h"
-#include "exec/memory.h"
-#include "exec/address-spaces.h"
-#include "hw/virtio/virtio-access.h"
-#include "hw/virtio/dataplane/vring.h"
-#include "hw/virtio/dataplane/vring-accessors.h"
-#include "qemu/error-report.h"
-
-/* vring_map can be coupled with vring_unmap or (if you still have the
- * value returned in *mr) memory_region_unref.
- * Returns NULL on failure.
- * Callers that can handle a partial mapping must supply mapped_len pointer to
- * get the actual length mapped.
- * Passing mapped_len == NULL requires either a full mapping or a failure.
- */
-static void *vring_map(MemoryRegion **mr, hwaddr phys,
-                       hwaddr len, hwaddr *mapped_len,
-                       bool is_write)
-{
-    MemoryRegionSection section = memory_region_find(get_system_memory(), phys, len);
-    uint64_t size;
-
-    if (!section.mr) {
-        goto out;
-    }
-
-    size = int128_get64(section.size);
-    assert(size);
-
-    /* Passing mapped_len == NULL requires either a full mapping or a failure. */
-    if (!mapped_len && size < len) {
-        goto out;
-    }
-
-    if (is_write && section.readonly) {
-        goto out;
-    }
-    if (!memory_region_is_ram(section.mr)) {
-        goto out;
-    }
-
-    /* Ignore regions with dirty logging, we cannot mark them dirty */
-    if (memory_region_get_dirty_log_mask(section.mr)) {
-        goto out;
-    }
-
-    if (mapped_len) {
-        *mapped_len = MIN(size, len);
-    }
-
-    *mr = section.mr;
-    return memory_region_get_ram_ptr(section.mr) + section.offset_within_region;
-
-out:
-    memory_region_unref(section.mr);
-    *mr = NULL;
-    return NULL;
-}
-
-static void vring_unmap(void *buffer, bool is_write)
-{
-    ram_addr_t addr;
-    MemoryRegion *mr;
-
-    mr = qemu_ram_addr_from_host(buffer, &addr);
-    memory_region_unref(mr);
-}
-
-/* Map the guest's vring to host memory */
-bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
-{
-    struct vring *vr = &vring->vr;
-    hwaddr addr;
-    hwaddr size;
-    void *ptr;
-
-    vring->broken = false;
-    vr->num = virtio_queue_get_num(vdev, n);
-
-    addr = virtio_queue_get_desc_addr(vdev, n);
-    size = virtio_queue_get_desc_size(vdev, n);
-    /* Map the descriptor area as read only */
-    ptr = vring_map(&vring->mr_desc, addr, size, NULL, false);
-    if (!ptr) {
-        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring desc "
-                     "at 0x%" HWADDR_PRIx,
-                      size, addr);
-        goto out_err_desc;
-    }
-    vr->desc = ptr;
-
-    addr = virtio_queue_get_avail_addr(vdev, n);
-    size = virtio_queue_get_avail_size(vdev, n);
-    /* Add the size of the used_event_idx */
-    size += sizeof(uint16_t);
-    /* Map the driver area as read only */
-    ptr = vring_map(&vring->mr_avail, addr, size, NULL, false);
-    if (!ptr) {
-        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring avail "
-                     "at 0x%" HWADDR_PRIx,
-                      size, addr);
-        goto out_err_avail;
-    }
-    vr->avail = ptr;
-
-    addr = virtio_queue_get_used_addr(vdev, n);
-    size = virtio_queue_get_used_size(vdev, n);
-    /* Add the size of the avail_event_idx */
-    size += sizeof(uint16_t);
-    /* Map the device area as read-write */
-    ptr = vring_map(&vring->mr_used, addr, size, NULL, true);
-    if (!ptr) {
-        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring used "
-                     "at 0x%" HWADDR_PRIx,
-                      size, addr);
-        goto out_err_used;
-    }
-    vr->used = ptr;
-
-    vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
-    vring->last_used_idx = vring_get_used_idx(vdev, vring);
-    vring->signalled_used = 0;
-    vring->signalled_used_valid = false;
-
-    trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
-                      vring->vr.desc, vring->vr.avail, vring->vr.used);
-    return true;
-
-out_err_used:
-    memory_region_unref(vring->mr_avail);
-out_err_avail:
-    memory_region_unref(vring->mr_desc);
-out_err_desc:
-    vring->broken = true;
-    return false;
-}
-
-void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
-{
-    virtio_queue_set_last_avail_idx(vdev, n, vring->last_avail_idx);
-    virtio_queue_invalidate_signalled_used(vdev, n);
-
-    memory_region_unref(vring->mr_desc);
-    memory_region_unref(vring->mr_avail);
-    memory_region_unref(vring->mr_used);
-}
-
-/* Disable guest->host notifies */
-void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
-{
-    if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
-    }
-}
-
-/* Enable guest->host notifies
- *
- * Return true if the vring is empty, false if there are more requests.
- */
-void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
-{
-    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) = vring->vr.avail->idx;
-    } else {
-        vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
-    }
-    smp_mb(); /* ensure update is seen before reading avail_idx */
-}
-
-/* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
-bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
-{
-    uint16_t old, new;
-    bool v;
-    /* Flush out used index updates. This is paired
-     * with the barrier that the Guest executes when enabling
-     * interrupts. */
-    smp_mb();
-
-    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
-        unlikely(!vring_more_avail(vdev, vring))) {
-        return true;
-    }
-
-    if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        return !(vring_get_avail_flags(vdev, vring) &
-                 VRING_AVAIL_F_NO_INTERRUPT);
-    }
-    old = vring->signalled_used;
-    v = vring->signalled_used_valid;
-    new = vring->signalled_used = vring->last_used_idx;
-    vring->signalled_used_valid = true;
-
-    if (unlikely(!v)) {
-        return true;
-    }
-
-    return vring_need_event(virtio_tswap16(vdev, vring_used_event(&vring->vr)),
-                            new, old);
-}
-
-typedef struct VirtQueueCurrentElement {
-    unsigned in_num;
-    unsigned out_num;
-    hwaddr addr[VIRTQUEUE_MAX_SIZE];
-    struct iovec iov[VIRTQUEUE_MAX_SIZE];
-} VirtQueueCurrentElement;
-
-static int get_desc(Vring *vring, VirtQueueCurrentElement *elem,
-                    struct vring_desc *desc)
-{
-    unsigned *num;
-    struct iovec *iov;
-    hwaddr *addr;
-    MemoryRegion *mr;
-    hwaddr len;
-
-    if (desc->flags & VRING_DESC_F_WRITE) {
-        num = &elem->in_num;
-        iov = &elem->iov[elem->out_num + *num];
-        addr = &elem->addr[elem->out_num + *num];
-    } else {
-        num = &elem->out_num;
-        iov = &elem->iov[*num];
-        addr = &elem->addr[*num];
-
-        /* If it's an output descriptor, they're all supposed
-         * to come before any input descriptors. */
-        if (unlikely(elem->in_num)) {
-            error_report("Descriptor has out after in");
-            return -EFAULT;
-        }
-    }
-
-    while (desc->len) {
-        /* Stop for now if there are not enough iovecs available. */
-        if (*num >= VIRTQUEUE_MAX_SIZE) {
-            error_report("Invalid SG num: %u", *num);
-            return -EFAULT;
-        }
-
-        iov->iov_base = vring_map(&mr, desc->addr, desc->len, &len,
-                                  desc->flags & VRING_DESC_F_WRITE);
-        if (!iov->iov_base) {
-            error_report("Failed to map descriptor addr %#" PRIx64 " len %u",
-                         (uint64_t)desc->addr, desc->len);
-            return -EFAULT;
-        }
-
-        /* The MemoryRegion is looked up again and unref'ed later, leave the
-         * ref in place.  */
-        (iov++)->iov_len = len;
-        *addr++ = desc->addr;
-        desc->len -= len;
-        desc->addr += len;
-        *num += 1;
-    }
-
-    return 0;
-}
-
-static void copy_in_vring_desc(VirtIODevice *vdev,
-                               const struct vring_desc *guest,
-                               struct vring_desc *host)
-{
-    host->addr = virtio_ldq_p(vdev, &guest->addr);
-    host->len = virtio_ldl_p(vdev, &guest->len);
-    host->flags = virtio_lduw_p(vdev, &guest->flags);
-    host->next = virtio_lduw_p(vdev, &guest->next);
-}
-
-static bool read_vring_desc(VirtIODevice *vdev,
-                            hwaddr guest,
-                            struct vring_desc *host)
-{
-    if (address_space_read(&address_space_memory, guest, MEMTXATTRS_UNSPECIFIED,
-                           (uint8_t *)host, sizeof *host)) {
-        return false;
-    }
-    host->addr = virtio_tswap64(vdev, host->addr);
-    host->len = virtio_tswap32(vdev, host->len);
-    host->flags = virtio_tswap16(vdev, host->flags);
-    host->next = virtio_tswap16(vdev, host->next);
-    return true;
-}
-
-/* This is stolen from linux/drivers/vhost/vhost.c. */
-static int get_indirect(VirtIODevice *vdev, Vring *vring,
-                        VirtQueueCurrentElement *cur_elem,
-                        struct vring_desc *indirect)
-{
-    struct vring_desc desc;
-    unsigned int i = 0, count, found = 0;
-    int ret;
-
-    /* Sanity check */
-    if (unlikely(indirect->len % sizeof(desc))) {
-        error_report("Invalid length in indirect descriptor: "
-                     "len %#x not multiple of %#zx",
-                     indirect->len, sizeof(desc));
-        vring->broken = true;
-        return -EFAULT;
-    }
-
-    count = indirect->len / sizeof(desc);
-    /* Buffers are chained via a 16 bit next field, so
-     * we can have at most 2^16 of these. */
-    if (unlikely(count > USHRT_MAX + 1)) {
-        error_report("Indirect buffer length too big: %d", indirect->len);
-        vring->broken = true;
-        return -EFAULT;
-    }
-
-    do {
-        /* Translate indirect descriptor */
-        if (!read_vring_desc(vdev, indirect->addr + found * sizeof(desc),
-                             &desc)) {
-            error_report("Failed to read indirect descriptor "
-                         "addr %#" PRIx64 " len %zu",
-                         (uint64_t)indirect->addr + found * sizeof(desc),
-                         sizeof(desc));
-            vring->broken = true;
-            return -EFAULT;
-        }
-
-        /* Ensure descriptor has been loaded before accessing fields */
-        barrier(); /* read_barrier_depends(); */
-
-        if (unlikely(++found > count)) {
-            error_report("Loop detected: last one at %u "
-                         "indirect size %u", i, count);
-            vring->broken = true;
-            return -EFAULT;
-        }
-
-        if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
-            error_report("Nested indirect descriptor");
-            vring->broken = true;
-            return -EFAULT;
-        }
-
-        ret = get_desc(vring, cur_elem, &desc);
-        if (ret < 0) {
-            vring->broken |= (ret == -EFAULT);
-            return ret;
-        }
-        i = desc.next;
-    } while (desc.flags & VRING_DESC_F_NEXT);
-    return 0;
-}
-
-static void vring_unmap_element(VirtQueueElement *elem)
-{
-    int i;
-
-    /* This assumes that the iovecs, if changed, are never moved past
-     * the end of the valid area.  This is true if iovec manipulations
-     * are done with iov_discard_front and iov_discard_back.
-     */
-    for (i = 0; i < elem->out_num; i++) {
-        vring_unmap(elem->out_sg[i].iov_base, false);
-    }
-
-    for (i = 0; i < elem->in_num; i++) {
-        vring_unmap(elem->in_sg[i].iov_base, true);
-    }
-}
-
-/* This looks in the virtqueue and for the first available buffer, and converts
- * it to an iovec for convenient access.  Since descriptors consist of some
- * number of output then some number of input descriptors, it's actually two
- * iovecs, but we pack them into one and note how many of each there were.
- *
- * This function returns the descriptor number found, or vq->num (which is
- * never a valid descriptor number) if none was found.  A negative code is
- * returned on error.
- *
- * Stolen from linux/drivers/vhost/vhost.c.
- */
-void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz)
-{
-    struct vring_desc desc;
-    unsigned int i, head, found = 0, num = vring->vr.num;
-    uint16_t avail_idx, last_avail_idx;
-    VirtQueueCurrentElement cur_elem;
-    VirtQueueElement *elem = NULL;
-    int ret;
-
-    /* If there was a fatal error then refuse operation */
-    if (vring->broken) {
-        ret = -EFAULT;
-        goto out;
-    }
-
-    cur_elem.in_num = cur_elem.out_num = 0;
-
-    /* Check it isn't doing very strange things with descriptor numbers. */
-    last_avail_idx = vring->last_avail_idx;
-    avail_idx = vring_get_avail_idx(vdev, vring);
-    barrier(); /* load indices now and not again later */
-
-    if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
-        error_report("Guest moved used index from %u to %u",
-                     last_avail_idx, avail_idx);
-        ret = -EFAULT;
-        goto out;
-    }
-
-    /* If there's nothing new since last we looked. */
-    if (avail_idx == last_avail_idx) {
-        ret = -EAGAIN;
-        goto out;
-    }
-
-    /* Only get avail ring entries after they have been exposed by guest. */
-    smp_rmb();
-
-    /* Grab the next descriptor number they're advertising, and increment
-     * the index we've seen. */
-    head = vring_get_avail_ring(vdev, vring, last_avail_idx % num);
-
-    /* If their number is silly, that's an error. */
-    if (unlikely(head >= num)) {
-        error_report("Guest says index %u > %u is available", head, num);
-        ret = -EFAULT;
-        goto out;
-    }
-
-    i = head;
-    do {
-        if (unlikely(i >= num)) {
-            error_report("Desc index is %u > %u, head = %u", i, num, head);
-            ret = -EFAULT;
-            goto out;
-        }
-        if (unlikely(++found > num)) {
-            error_report("Loop detected: last one at %u vq size %u head %u",
-                         i, num, head);
-            ret = -EFAULT;
-            goto out;
-        }
-        copy_in_vring_desc(vdev, &vring->vr.desc[i], &desc);
-
-        /* Ensure descriptor is loaded before accessing fields */
-        barrier();
-
-        if (desc.flags & VRING_DESC_F_INDIRECT) {
-            ret = get_indirect(vdev, vring, &cur_elem, &desc);
-            if (ret < 0) {
-                goto out;
-            }
-            continue;
-        }
-
-        ret = get_desc(vring, &cur_elem, &desc);
-        if (ret < 0) {
-            goto out;
-        }
-
-        i = desc.next;
-    } while (desc.flags & VRING_DESC_F_NEXT);
-
-    /* On success, increment avail index. */
-    vring->last_avail_idx++;
-    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
-        vring_avail_event(&vring->vr) =
-            virtio_tswap16(vdev, vring->last_avail_idx);
-    }
-
-    /* Now copy what we have collected and mapped */
-    elem = virtqueue_alloc_element(sz, cur_elem.out_num, cur_elem.in_num);
-    elem->index = head;
-    for (i = 0; i < cur_elem.out_num; i++) {
-        elem->out_addr[i] = cur_elem.addr[i];
-        elem->out_sg[i] = cur_elem.iov[i];
-    }
-    for (i = 0; i < cur_elem.in_num; i++) {
-        elem->in_addr[i] = cur_elem.addr[cur_elem.out_num + i];
-        elem->in_sg[i] = cur_elem.iov[cur_elem.out_num + i];
-    }
-
-    return elem;
-
-out:
-    assert(ret < 0);
-    if (ret == -EFAULT) {
-        vring->broken = true;
-    }
-
-    for (i = 0; i < cur_elem.out_num + cur_elem.in_num; i++) {
-        vring_unmap(cur_elem.iov[i].iov_base, false);
-    }
-
-    g_free(elem);
-    return NULL;
-}
-
-/* After we've used one of their buffers, we tell them about it.
- *
- * Stolen from linux/drivers/vhost/vhost.c.
- */
-void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
-                int len)
-{
-    unsigned int head = elem->index;
-    uint16_t new;
-
-    vring_unmap_element(elem);
-
-    /* Don't touch vring if a fatal error occurred */
-    if (vring->broken) {
-        return;
-    }
-
-    /* The virtqueue contains a ring of used buffers.  Get a pointer to the
-     * next entry in that used ring. */
-    vring_set_used_ring_id(vdev, vring, vring->last_used_idx % vring->vr.num,
-                           head);
-    vring_set_used_ring_len(vdev, vring, vring->last_used_idx % vring->vr.num,
-                            len);
-
-    /* Make sure buffer is written before we update index. */
-    smp_wmb();
-
-    new = ++vring->last_used_idx;
-    vring_set_used_idx(vdev, vring, new);
-    if (unlikely((int16_t)(new - vring->signalled_used) < (uint16_t)1)) {
-        vring->signalled_used_valid = false;
-    }
-}
diff --git a/include/hw/virtio/dataplane/vring-accessors.h b/include/hw/virtio/dataplane/vring-accessors.h
deleted file mode 100644
index 815c19b..0000000
--- a/include/hw/virtio/dataplane/vring-accessors.h
+++ /dev/null
@@ -1,75 +0,0 @@
-#ifndef VRING_ACCESSORS_H
-#define VRING_ACCESSORS_H
-
-#include "standard-headers/linux/virtio_ring.h"
-#include "hw/virtio/virtio.h"
-#include "hw/virtio/virtio-access.h"
-
-static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vring)
-{
-    return virtio_tswap16(vdev, vring->vr.used->idx);
-}
-
-static inline void vring_set_used_idx(VirtIODevice *vdev, Vring *vring,
-                                      uint16_t idx)
-{
-    vring->vr.used->idx = virtio_tswap16(vdev, idx);
-}
-
-static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vring)
-{
-    return virtio_tswap16(vdev, vring->vr.avail->idx);
-}
-
-static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *vring,
-                                            int i)
-{
-    return virtio_tswap16(vdev, vring->vr.avail->ring[i]);
-}
-
-static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vring,
-                                          int i, uint32_t id)
-{
-    vring->vr.used->ring[i].id = virtio_tswap32(vdev, id);
-}
-
-static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vring,
-                                          int i, uint32_t len)
-{
-    vring->vr.used->ring[i].len = virtio_tswap32(vdev, len);
-}
-
-static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *vring)
-{
-    return virtio_tswap16(vdev, vring->vr.used->flags);
-}
-
-static inline uint16_t vring_get_avail_flags(VirtIODevice *vdev, Vring *vring)
-{
-    return virtio_tswap16(vdev, vring->vr.avail->flags);
-}
-
-static inline void vring_set_used_flags(VirtIODevice *vdev, Vring *vring,
-                                        uint16_t flags)
-{
-    vring->vr.used->flags |= virtio_tswap16(vdev, flags);
-}
-
-static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vring,
-                                          uint16_t flags)
-{
-    vring->vr.used->flags &= virtio_tswap16(vdev, ~flags);
-}
-
-static inline unsigned int vring_get_num(Vring *vring)
-{
-    return vring->vr.num;
-}
-
-/* Are there more descriptors available? */
-static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
-{
-    return vring_get_avail_idx(vdev, vring) != vring->last_avail_idx;
-}
-
-#endif
diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
deleted file mode 100644
index e1c2a65..0000000
--- a/include/hw/virtio/dataplane/vring.h
+++ /dev/null
@@ -1,51 +0,0 @@
-/* Copyright 2012 Red Hat, Inc. and/or its affiliates
- * Copyright IBM, Corp. 2012
- *
- * Based on Linux 2.6.39 vhost code:
- * Copyright (C) 2009 Red Hat, Inc.
- * Copyright (C) 2006 Rusty Russell IBM Corporation
- *
- * Author: Michael S. Tsirkin <mst@redhat.com>
- *         Stefan Hajnoczi <stefanha@redhat.com>
- *
- * Inspiration, some code, and most witty comments come from
- * Documentation/virtual/lguest/lguest.c, by Rusty Russell
- *
- * This work is licensed under the terms of the GNU GPL, version 2.
- */
-
-#ifndef VRING_H
-#define VRING_H
-
-#include "qemu-common.h"
-#include "standard-headers/linux/virtio_ring.h"
-#include "hw/virtio/virtio.h"
-
-typedef struct {
-    MemoryRegion *mr_desc;          /* memory region for the vring desc */
-    MemoryRegion *mr_avail;         /* memory region for the vring avail */
-    MemoryRegion *mr_used;          /* memory region for the vring used */
-    struct vring vr;                /* virtqueue vring mapped to host memory */
-    uint16_t last_avail_idx;        /* last processed avail ring index */
-    uint16_t last_used_idx;         /* last processed used ring index */
-    uint16_t signalled_used;        /* EVENT_IDX state */
-    bool signalled_used_valid;
-    bool broken;                    /* was there a fatal error? */
-} Vring;
-
-/* Fail future vring_pop() and vring_push() calls until reset */
-static inline void vring_set_broken(Vring *vring)
-{
-    vring->broken = true;
-}
-
-bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
-void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
-void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
-void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
-bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
-void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
-void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
-                int len);
-
-#endif /* VRING_H */
diff --git a/trace-events b/trace-events
index f986c81..61a133f 100644
--- a/trace-events
+++ b/trace-events
@@ -127,9 +127,6 @@ virtio_blk_data_plane_start(void *s) "dataplane %p"
 virtio_blk_data_plane_stop(void *s) "dataplane %p"
 virtio_blk_data_plane_process_request(void *s, unsigned int out_num, unsigned int in_num, unsigned int head) "dataplane %p out_num %u in_num %u head %u"
 
-# hw/virtio/dataplane/vring.c
-vring_setup(uint64_t physical, void *desc, void *avail, void *used) "vring physical %#"PRIx64" desc %p avail %p used %p"
-
 # thread-pool.c
 thread_pool_submit(void *pool, void *req, void *opaque) "pool %p req %p opaque %p"
 thread_pool_complete(void *pool, void *req, void *opaque, int ret) "pool %p req %p opaque %p ret %d"
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 8/8] vring: remove

[Fam Zheng]

On Sun, 02/14 18:17, Paolo Bonzini wrote:
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/virtio/Makefile.objs                       |   1 -
>  hw/virtio/dataplane/Makefile.objs             |   1 -
>  hw/virtio/dataplane/vring.c                   | 548 --------------------------
>  include/hw/virtio/dataplane/vring-accessors.h |  75 ----
>  include/hw/virtio/dataplane/vring.h           |  51 ---
>  trace-events                                  |   3 -
>  6 files changed, 679 deletions(-)
>  delete mode 100644 hw/virtio/dataplane/Makefile.objs
>  delete mode 100644 hw/virtio/dataplane/vring.c
>  delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
>  delete mode 100644 include/hw/virtio/dataplane/vring.h

Bravo!

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> diff --git a/hw/virtio/Makefile.objs b/hw/virtio/Makefile.objs
> index 19b224a..3e2b175 100644
> --- a/hw/virtio/Makefile.objs
> +++ b/hw/virtio/Makefile.objs
> @@ -2,7 +2,6 @@ common-obj-y += virtio-rng.o
>  common-obj-$(CONFIG_VIRTIO_PCI) += virtio-pci.o
>  common-obj-y += virtio-bus.o
>  common-obj-y += virtio-mmio.o
> -obj-$(CONFIG_VIRTIO) += dataplane/
>  
>  obj-y += virtio.o virtio-balloon.o 
>  obj-$(CONFIG_LINUX) += vhost.o vhost-backend.o vhost-user.o
> diff --git a/hw/virtio/dataplane/Makefile.objs b/hw/virtio/dataplane/Makefile.objs
> deleted file mode 100644
> index 753a9ca..0000000
> --- a/hw/virtio/dataplane/Makefile.objs
> +++ /dev/null
> @@ -1 +0,0 @@
> -obj-y += vring.o
> diff --git a/hw/virtio/dataplane/vring.c b/hw/virtio/dataplane/vring.c
> deleted file mode 100644
> index 157e8b8..0000000
> --- a/hw/virtio/dataplane/vring.c
> +++ /dev/null
> @@ -1,548 +0,0 @@
> -/* Copyright 2012 Red Hat, Inc.
> - * Copyright IBM, Corp. 2012
> - *
> - * Based on Linux 2.6.39 vhost code:
> - * Copyright (C) 2009 Red Hat, Inc.
> - * Copyright (C) 2006 Rusty Russell IBM Corporation
> - *
> - * Author: Michael S. Tsirkin <mst@redhat.com>
> - *         Stefan Hajnoczi <stefanha@redhat.com>
> - *
> - * Inspiration, some code, and most witty comments come from
> - * Documentation/virtual/lguest/lguest.c, by Rusty Russell
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2.
> - */
> -
> -#include "qemu/osdep.h"
> -#include "trace.h"
> -#include "hw/hw.h"
> -#include "exec/memory.h"
> -#include "exec/address-spaces.h"
> -#include "hw/virtio/virtio-access.h"
> -#include "hw/virtio/dataplane/vring.h"
> -#include "hw/virtio/dataplane/vring-accessors.h"
> -#include "qemu/error-report.h"
> -
> -/* vring_map can be coupled with vring_unmap or (if you still have the
> - * value returned in *mr) memory_region_unref.
> - * Returns NULL on failure.
> - * Callers that can handle a partial mapping must supply mapped_len pointer to
> - * get the actual length mapped.
> - * Passing mapped_len == NULL requires either a full mapping or a failure.
> - */
> -static void *vring_map(MemoryRegion **mr, hwaddr phys,
> -                       hwaddr len, hwaddr *mapped_len,
> -                       bool is_write)
> -{
> -    MemoryRegionSection section = memory_region_find(get_system_memory(), phys, len);
> -    uint64_t size;
> -
> -    if (!section.mr) {
> -        goto out;
> -    }
> -
> -    size = int128_get64(section.size);
> -    assert(size);
> -
> -    /* Passing mapped_len == NULL requires either a full mapping or a failure. */
> -    if (!mapped_len && size < len) {
> -        goto out;
> -    }
> -
> -    if (is_write && section.readonly) {
> -        goto out;
> -    }
> -    if (!memory_region_is_ram(section.mr)) {
> -        goto out;
> -    }
> -
> -    /* Ignore regions with dirty logging, we cannot mark them dirty */
> -    if (memory_region_get_dirty_log_mask(section.mr)) {
> -        goto out;
> -    }
> -
> -    if (mapped_len) {
> -        *mapped_len = MIN(size, len);
> -    }
> -
> -    *mr = section.mr;
> -    return memory_region_get_ram_ptr(section.mr) + section.offset_within_region;
> -
> -out:
> -    memory_region_unref(section.mr);
> -    *mr = NULL;
> -    return NULL;
> -}
> -
> -static void vring_unmap(void *buffer, bool is_write)
> -{
> -    ram_addr_t addr;
> -    MemoryRegion *mr;
> -
> -    mr = qemu_ram_addr_from_host(buffer, &addr);
> -    memory_region_unref(mr);
> -}
> -
> -/* Map the guest's vring to host memory */
> -bool vring_setup(Vring *vring, VirtIODevice *vdev, int n)
> -{
> -    struct vring *vr = &vring->vr;
> -    hwaddr addr;
> -    hwaddr size;
> -    void *ptr;
> -
> -    vring->broken = false;
> -    vr->num = virtio_queue_get_num(vdev, n);
> -
> -    addr = virtio_queue_get_desc_addr(vdev, n);
> -    size = virtio_queue_get_desc_size(vdev, n);
> -    /* Map the descriptor area as read only */
> -    ptr = vring_map(&vring->mr_desc, addr, size, NULL, false);
> -    if (!ptr) {
> -        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring desc "
> -                     "at 0x%" HWADDR_PRIx,
> -                      size, addr);
> -        goto out_err_desc;
> -    }
> -    vr->desc = ptr;
> -
> -    addr = virtio_queue_get_avail_addr(vdev, n);
> -    size = virtio_queue_get_avail_size(vdev, n);
> -    /* Add the size of the used_event_idx */
> -    size += sizeof(uint16_t);
> -    /* Map the driver area as read only */
> -    ptr = vring_map(&vring->mr_avail, addr, size, NULL, false);
> -    if (!ptr) {
> -        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring avail "
> -                     "at 0x%" HWADDR_PRIx,
> -                      size, addr);
> -        goto out_err_avail;
> -    }
> -    vr->avail = ptr;
> -
> -    addr = virtio_queue_get_used_addr(vdev, n);
> -    size = virtio_queue_get_used_size(vdev, n);
> -    /* Add the size of the avail_event_idx */
> -    size += sizeof(uint16_t);
> -    /* Map the device area as read-write */
> -    ptr = vring_map(&vring->mr_used, addr, size, NULL, true);
> -    if (!ptr) {
> -        error_report("Failed to map 0x%" HWADDR_PRIx " byte for vring used "
> -                     "at 0x%" HWADDR_PRIx,
> -                      size, addr);
> -        goto out_err_used;
> -    }
> -    vr->used = ptr;
> -
> -    vring->last_avail_idx = virtio_queue_get_last_avail_idx(vdev, n);
> -    vring->last_used_idx = vring_get_used_idx(vdev, vring);
> -    vring->signalled_used = 0;
> -    vring->signalled_used_valid = false;
> -
> -    trace_vring_setup(virtio_queue_get_ring_addr(vdev, n),
> -                      vring->vr.desc, vring->vr.avail, vring->vr.used);
> -    return true;
> -
> -out_err_used:
> -    memory_region_unref(vring->mr_avail);
> -out_err_avail:
> -    memory_region_unref(vring->mr_desc);
> -out_err_desc:
> -    vring->broken = true;
> -    return false;
> -}
> -
> -void vring_teardown(Vring *vring, VirtIODevice *vdev, int n)
> -{
> -    virtio_queue_set_last_avail_idx(vdev, n, vring->last_avail_idx);
> -    virtio_queue_invalidate_signalled_used(vdev, n);
> -
> -    memory_region_unref(vring->mr_desc);
> -    memory_region_unref(vring->mr_avail);
> -    memory_region_unref(vring->mr_used);
> -}
> -
> -/* Disable guest->host notifies */
> -void vring_disable_notification(VirtIODevice *vdev, Vring *vring)
> -{
> -    if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        vring_set_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
> -    }
> -}
> -
> -/* Enable guest->host notifies
> - *
> - * Return true if the vring is empty, false if there are more requests.
> - */
> -void vring_enable_notification(VirtIODevice *vdev, Vring *vring)
> -{
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        vring_avail_event(&vring->vr) = vring->vr.avail->idx;
> -    } else {
> -        vring_clear_used_flags(vdev, vring, VRING_USED_F_NO_NOTIFY);
> -    }
> -    smp_mb(); /* ensure update is seen before reading avail_idx */
> -}
> -
> -/* This is stolen from linux/drivers/vhost/vhost.c:vhost_notify() */
> -bool vring_should_notify(VirtIODevice *vdev, Vring *vring)
> -{
> -    uint16_t old, new;
> -    bool v;
> -    /* Flush out used index updates. This is paired
> -     * with the barrier that the Guest executes when enabling
> -     * interrupts. */
> -    smp_mb();
> -
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_F_NOTIFY_ON_EMPTY) &&
> -        unlikely(!vring_more_avail(vdev, vring))) {
> -        return true;
> -    }
> -
> -    if (!virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        return !(vring_get_avail_flags(vdev, vring) &
> -                 VRING_AVAIL_F_NO_INTERRUPT);
> -    }
> -    old = vring->signalled_used;
> -    v = vring->signalled_used_valid;
> -    new = vring->signalled_used = vring->last_used_idx;
> -    vring->signalled_used_valid = true;
> -
> -    if (unlikely(!v)) {
> -        return true;
> -    }
> -
> -    return vring_need_event(virtio_tswap16(vdev, vring_used_event(&vring->vr)),
> -                            new, old);
> -}
> -
> -typedef struct VirtQueueCurrentElement {
> -    unsigned in_num;
> -    unsigned out_num;
> -    hwaddr addr[VIRTQUEUE_MAX_SIZE];
> -    struct iovec iov[VIRTQUEUE_MAX_SIZE];
> -} VirtQueueCurrentElement;
> -
> -static int get_desc(Vring *vring, VirtQueueCurrentElement *elem,
> -                    struct vring_desc *desc)
> -{
> -    unsigned *num;
> -    struct iovec *iov;
> -    hwaddr *addr;
> -    MemoryRegion *mr;
> -    hwaddr len;
> -
> -    if (desc->flags & VRING_DESC_F_WRITE) {
> -        num = &elem->in_num;
> -        iov = &elem->iov[elem->out_num + *num];
> -        addr = &elem->addr[elem->out_num + *num];
> -    } else {
> -        num = &elem->out_num;
> -        iov = &elem->iov[*num];
> -        addr = &elem->addr[*num];
> -
> -        /* If it's an output descriptor, they're all supposed
> -         * to come before any input descriptors. */
> -        if (unlikely(elem->in_num)) {
> -            error_report("Descriptor has out after in");
> -            return -EFAULT;
> -        }
> -    }
> -
> -    while (desc->len) {
> -        /* Stop for now if there are not enough iovecs available. */
> -        if (*num >= VIRTQUEUE_MAX_SIZE) {
> -            error_report("Invalid SG num: %u", *num);
> -            return -EFAULT;
> -        }
> -
> -        iov->iov_base = vring_map(&mr, desc->addr, desc->len, &len,
> -                                  desc->flags & VRING_DESC_F_WRITE);
> -        if (!iov->iov_base) {
> -            error_report("Failed to map descriptor addr %#" PRIx64 " len %u",
> -                         (uint64_t)desc->addr, desc->len);
> -            return -EFAULT;
> -        }
> -
> -        /* The MemoryRegion is looked up again and unref'ed later, leave the
> -         * ref in place.  */
> -        (iov++)->iov_len = len;
> -        *addr++ = desc->addr;
> -        desc->len -= len;
> -        desc->addr += len;
> -        *num += 1;
> -    }
> -
> -    return 0;
> -}
> -
> -static void copy_in_vring_desc(VirtIODevice *vdev,
> -                               const struct vring_desc *guest,
> -                               struct vring_desc *host)
> -{
> -    host->addr = virtio_ldq_p(vdev, &guest->addr);
> -    host->len = virtio_ldl_p(vdev, &guest->len);
> -    host->flags = virtio_lduw_p(vdev, &guest->flags);
> -    host->next = virtio_lduw_p(vdev, &guest->next);
> -}
> -
> -static bool read_vring_desc(VirtIODevice *vdev,
> -                            hwaddr guest,
> -                            struct vring_desc *host)
> -{
> -    if (address_space_read(&address_space_memory, guest, MEMTXATTRS_UNSPECIFIED,
> -                           (uint8_t *)host, sizeof *host)) {
> -        return false;
> -    }
> -    host->addr = virtio_tswap64(vdev, host->addr);
> -    host->len = virtio_tswap32(vdev, host->len);
> -    host->flags = virtio_tswap16(vdev, host->flags);
> -    host->next = virtio_tswap16(vdev, host->next);
> -    return true;
> -}
> -
> -/* This is stolen from linux/drivers/vhost/vhost.c. */
> -static int get_indirect(VirtIODevice *vdev, Vring *vring,
> -                        VirtQueueCurrentElement *cur_elem,
> -                        struct vring_desc *indirect)
> -{
> -    struct vring_desc desc;
> -    unsigned int i = 0, count, found = 0;
> -    int ret;
> -
> -    /* Sanity check */
> -    if (unlikely(indirect->len % sizeof(desc))) {
> -        error_report("Invalid length in indirect descriptor: "
> -                     "len %#x not multiple of %#zx",
> -                     indirect->len, sizeof(desc));
> -        vring->broken = true;
> -        return -EFAULT;
> -    }
> -
> -    count = indirect->len / sizeof(desc);
> -    /* Buffers are chained via a 16 bit next field, so
> -     * we can have at most 2^16 of these. */
> -    if (unlikely(count > USHRT_MAX + 1)) {
> -        error_report("Indirect buffer length too big: %d", indirect->len);
> -        vring->broken = true;
> -        return -EFAULT;
> -    }
> -
> -    do {
> -        /* Translate indirect descriptor */
> -        if (!read_vring_desc(vdev, indirect->addr + found * sizeof(desc),
> -                             &desc)) {
> -            error_report("Failed to read indirect descriptor "
> -                         "addr %#" PRIx64 " len %zu",
> -                         (uint64_t)indirect->addr + found * sizeof(desc),
> -                         sizeof(desc));
> -            vring->broken = true;
> -            return -EFAULT;
> -        }
> -
> -        /* Ensure descriptor has been loaded before accessing fields */
> -        barrier(); /* read_barrier_depends(); */
> -
> -        if (unlikely(++found > count)) {
> -            error_report("Loop detected: last one at %u "
> -                         "indirect size %u", i, count);
> -            vring->broken = true;
> -            return -EFAULT;
> -        }
> -
> -        if (unlikely(desc.flags & VRING_DESC_F_INDIRECT)) {
> -            error_report("Nested indirect descriptor");
> -            vring->broken = true;
> -            return -EFAULT;
> -        }
> -
> -        ret = get_desc(vring, cur_elem, &desc);
> -        if (ret < 0) {
> -            vring->broken |= (ret == -EFAULT);
> -            return ret;
> -        }
> -        i = desc.next;
> -    } while (desc.flags & VRING_DESC_F_NEXT);
> -    return 0;
> -}
> -
> -static void vring_unmap_element(VirtQueueElement *elem)
> -{
> -    int i;
> -
> -    /* This assumes that the iovecs, if changed, are never moved past
> -     * the end of the valid area.  This is true if iovec manipulations
> -     * are done with iov_discard_front and iov_discard_back.
> -     */
> -    for (i = 0; i < elem->out_num; i++) {
> -        vring_unmap(elem->out_sg[i].iov_base, false);
> -    }
> -
> -    for (i = 0; i < elem->in_num; i++) {
> -        vring_unmap(elem->in_sg[i].iov_base, true);
> -    }
> -}
> -
> -/* This looks in the virtqueue and for the first available buffer, and converts
> - * it to an iovec for convenient access.  Since descriptors consist of some
> - * number of output then some number of input descriptors, it's actually two
> - * iovecs, but we pack them into one and note how many of each there were.
> - *
> - * This function returns the descriptor number found, or vq->num (which is
> - * never a valid descriptor number) if none was found.  A negative code is
> - * returned on error.
> - *
> - * Stolen from linux/drivers/vhost/vhost.c.
> - */
> -void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz)
> -{
> -    struct vring_desc desc;
> -    unsigned int i, head, found = 0, num = vring->vr.num;
> -    uint16_t avail_idx, last_avail_idx;
> -    VirtQueueCurrentElement cur_elem;
> -    VirtQueueElement *elem = NULL;
> -    int ret;
> -
> -    /* If there was a fatal error then refuse operation */
> -    if (vring->broken) {
> -        ret = -EFAULT;
> -        goto out;
> -    }
> -
> -    cur_elem.in_num = cur_elem.out_num = 0;
> -
> -    /* Check it isn't doing very strange things with descriptor numbers. */
> -    last_avail_idx = vring->last_avail_idx;
> -    avail_idx = vring_get_avail_idx(vdev, vring);
> -    barrier(); /* load indices now and not again later */
> -
> -    if (unlikely((uint16_t)(avail_idx - last_avail_idx) > num)) {
> -        error_report("Guest moved used index from %u to %u",
> -                     last_avail_idx, avail_idx);
> -        ret = -EFAULT;
> -        goto out;
> -    }
> -
> -    /* If there's nothing new since last we looked. */
> -    if (avail_idx == last_avail_idx) {
> -        ret = -EAGAIN;
> -        goto out;
> -    }
> -
> -    /* Only get avail ring entries after they have been exposed by guest. */
> -    smp_rmb();
> -
> -    /* Grab the next descriptor number they're advertising, and increment
> -     * the index we've seen. */
> -    head = vring_get_avail_ring(vdev, vring, last_avail_idx % num);
> -
> -    /* If their number is silly, that's an error. */
> -    if (unlikely(head >= num)) {
> -        error_report("Guest says index %u > %u is available", head, num);
> -        ret = -EFAULT;
> -        goto out;
> -    }
> -
> -    i = head;
> -    do {
> -        if (unlikely(i >= num)) {
> -            error_report("Desc index is %u > %u, head = %u", i, num, head);
> -            ret = -EFAULT;
> -            goto out;
> -        }
> -        if (unlikely(++found > num)) {
> -            error_report("Loop detected: last one at %u vq size %u head %u",
> -                         i, num, head);
> -            ret = -EFAULT;
> -            goto out;
> -        }
> -        copy_in_vring_desc(vdev, &vring->vr.desc[i], &desc);
> -
> -        /* Ensure descriptor is loaded before accessing fields */
> -        barrier();
> -
> -        if (desc.flags & VRING_DESC_F_INDIRECT) {
> -            ret = get_indirect(vdev, vring, &cur_elem, &desc);
> -            if (ret < 0) {
> -                goto out;
> -            }
> -            continue;
> -        }
> -
> -        ret = get_desc(vring, &cur_elem, &desc);
> -        if (ret < 0) {
> -            goto out;
> -        }
> -
> -        i = desc.next;
> -    } while (desc.flags & VRING_DESC_F_NEXT);
> -
> -    /* On success, increment avail index. */
> -    vring->last_avail_idx++;
> -    if (virtio_vdev_has_feature(vdev, VIRTIO_RING_F_EVENT_IDX)) {
> -        vring_avail_event(&vring->vr) =
> -            virtio_tswap16(vdev, vring->last_avail_idx);
> -    }
> -
> -    /* Now copy what we have collected and mapped */
> -    elem = virtqueue_alloc_element(sz, cur_elem.out_num, cur_elem.in_num);
> -    elem->index = head;
> -    for (i = 0; i < cur_elem.out_num; i++) {
> -        elem->out_addr[i] = cur_elem.addr[i];
> -        elem->out_sg[i] = cur_elem.iov[i];
> -    }
> -    for (i = 0; i < cur_elem.in_num; i++) {
> -        elem->in_addr[i] = cur_elem.addr[cur_elem.out_num + i];
> -        elem->in_sg[i] = cur_elem.iov[cur_elem.out_num + i];
> -    }
> -
> -    return elem;
> -
> -out:
> -    assert(ret < 0);
> -    if (ret == -EFAULT) {
> -        vring->broken = true;
> -    }
> -
> -    for (i = 0; i < cur_elem.out_num + cur_elem.in_num; i++) {
> -        vring_unmap(cur_elem.iov[i].iov_base, false);
> -    }
> -
> -    g_free(elem);
> -    return NULL;
> -}
> -
> -/* After we've used one of their buffers, we tell them about it.
> - *
> - * Stolen from linux/drivers/vhost/vhost.c.
> - */
> -void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> -                int len)
> -{
> -    unsigned int head = elem->index;
> -    uint16_t new;
> -
> -    vring_unmap_element(elem);
> -
> -    /* Don't touch vring if a fatal error occurred */
> -    if (vring->broken) {
> -        return;
> -    }
> -
> -    /* The virtqueue contains a ring of used buffers.  Get a pointer to the
> -     * next entry in that used ring. */
> -    vring_set_used_ring_id(vdev, vring, vring->last_used_idx % vring->vr.num,
> -                           head);
> -    vring_set_used_ring_len(vdev, vring, vring->last_used_idx % vring->vr.num,
> -                            len);
> -
> -    /* Make sure buffer is written before we update index. */
> -    smp_wmb();
> -
> -    new = ++vring->last_used_idx;
> -    vring_set_used_idx(vdev, vring, new);
> -    if (unlikely((int16_t)(new - vring->signalled_used) < (uint16_t)1)) {
> -        vring->signalled_used_valid = false;
> -    }
> -}
> diff --git a/include/hw/virtio/dataplane/vring-accessors.h b/include/hw/virtio/dataplane/vring-accessors.h
> deleted file mode 100644
> index 815c19b..0000000
> --- a/include/hw/virtio/dataplane/vring-accessors.h
> +++ /dev/null
> @@ -1,75 +0,0 @@
> -#ifndef VRING_ACCESSORS_H
> -#define VRING_ACCESSORS_H
> -
> -#include "standard-headers/linux/virtio_ring.h"
> -#include "hw/virtio/virtio.h"
> -#include "hw/virtio/virtio-access.h"
> -
> -static inline uint16_t vring_get_used_idx(VirtIODevice *vdev, Vring *vring)
> -{
> -    return virtio_tswap16(vdev, vring->vr.used->idx);
> -}
> -
> -static inline void vring_set_used_idx(VirtIODevice *vdev, Vring *vring,
> -                                      uint16_t idx)
> -{
> -    vring->vr.used->idx = virtio_tswap16(vdev, idx);
> -}
> -
> -static inline uint16_t vring_get_avail_idx(VirtIODevice *vdev, Vring *vring)
> -{
> -    return virtio_tswap16(vdev, vring->vr.avail->idx);
> -}
> -
> -static inline uint16_t vring_get_avail_ring(VirtIODevice *vdev, Vring *vring,
> -                                            int i)
> -{
> -    return virtio_tswap16(vdev, vring->vr.avail->ring[i]);
> -}
> -
> -static inline void vring_set_used_ring_id(VirtIODevice *vdev, Vring *vring,
> -                                          int i, uint32_t id)
> -{
> -    vring->vr.used->ring[i].id = virtio_tswap32(vdev, id);
> -}
> -
> -static inline void vring_set_used_ring_len(VirtIODevice *vdev, Vring *vring,
> -                                          int i, uint32_t len)
> -{
> -    vring->vr.used->ring[i].len = virtio_tswap32(vdev, len);
> -}
> -
> -static inline uint16_t vring_get_used_flags(VirtIODevice *vdev, Vring *vring)
> -{
> -    return virtio_tswap16(vdev, vring->vr.used->flags);
> -}
> -
> -static inline uint16_t vring_get_avail_flags(VirtIODevice *vdev, Vring *vring)
> -{
> -    return virtio_tswap16(vdev, vring->vr.avail->flags);
> -}
> -
> -static inline void vring_set_used_flags(VirtIODevice *vdev, Vring *vring,
> -                                        uint16_t flags)
> -{
> -    vring->vr.used->flags |= virtio_tswap16(vdev, flags);
> -}
> -
> -static inline void vring_clear_used_flags(VirtIODevice *vdev, Vring *vring,
> -                                          uint16_t flags)
> -{
> -    vring->vr.used->flags &= virtio_tswap16(vdev, ~flags);
> -}
> -
> -static inline unsigned int vring_get_num(Vring *vring)
> -{
> -    return vring->vr.num;
> -}
> -
> -/* Are there more descriptors available? */
> -static inline bool vring_more_avail(VirtIODevice *vdev, Vring *vring)
> -{
> -    return vring_get_avail_idx(vdev, vring) != vring->last_avail_idx;
> -}
> -
> -#endif
> diff --git a/include/hw/virtio/dataplane/vring.h b/include/hw/virtio/dataplane/vring.h
> deleted file mode 100644
> index e1c2a65..0000000
> --- a/include/hw/virtio/dataplane/vring.h
> +++ /dev/null
> @@ -1,51 +0,0 @@
> -/* Copyright 2012 Red Hat, Inc. and/or its affiliates
> - * Copyright IBM, Corp. 2012
> - *
> - * Based on Linux 2.6.39 vhost code:
> - * Copyright (C) 2009 Red Hat, Inc.
> - * Copyright (C) 2006 Rusty Russell IBM Corporation
> - *
> - * Author: Michael S. Tsirkin <mst@redhat.com>
> - *         Stefan Hajnoczi <stefanha@redhat.com>
> - *
> - * Inspiration, some code, and most witty comments come from
> - * Documentation/virtual/lguest/lguest.c, by Rusty Russell
> - *
> - * This work is licensed under the terms of the GNU GPL, version 2.
> - */
> -
> -#ifndef VRING_H
> -#define VRING_H
> -
> -#include "qemu-common.h"
> -#include "standard-headers/linux/virtio_ring.h"
> -#include "hw/virtio/virtio.h"
> -
> -typedef struct {
> -    MemoryRegion *mr_desc;          /* memory region for the vring desc */
> -    MemoryRegion *mr_avail;         /* memory region for the vring avail */
> -    MemoryRegion *mr_used;          /* memory region for the vring used */
> -    struct vring vr;                /* virtqueue vring mapped to host memory */
> -    uint16_t last_avail_idx;        /* last processed avail ring index */
> -    uint16_t last_used_idx;         /* last processed used ring index */
> -    uint16_t signalled_used;        /* EVENT_IDX state */
> -    bool signalled_used_valid;
> -    bool broken;                    /* was there a fatal error? */
> -} Vring;
> -
> -/* Fail future vring_pop() and vring_push() calls until reset */
> -static inline void vring_set_broken(Vring *vring)
> -{
> -    vring->broken = true;
> -}
> -
> -bool vring_setup(Vring *vring, VirtIODevice *vdev, int n);
> -void vring_teardown(Vring *vring, VirtIODevice *vdev, int n);
> -void vring_disable_notification(VirtIODevice *vdev, Vring *vring);
> -void vring_enable_notification(VirtIODevice *vdev, Vring *vring);
> -bool vring_should_notify(VirtIODevice *vdev, Vring *vring);
> -void *vring_pop(VirtIODevice *vdev, Vring *vring, size_t sz);
> -void vring_push(VirtIODevice *vdev, Vring *vring, VirtQueueElement *elem,
> -                int len);
> -
> -#endif /* VRING_H */
> diff --git a/trace-events b/trace-events
> index f986c81..61a133f 100644
> --- a/trace-events
> +++ b/trace-events
> @@ -127,9 +127,6 @@ virtio_blk_data_plane_start(void *s) "dataplane %p"
>  virtio_blk_data_plane_stop(void *s) "dataplane %p"
>  virtio_blk_data_plane_process_request(void *s, unsigned int out_num, unsigned int in_num, unsigned int head) "dataplane %p out_num %u in_num %u head %u"
>  
> -# hw/virtio/dataplane/vring.c
> -vring_setup(uint64_t physical, void *desc, void *avail, void *used) "vring physical %#"PRIx64" desc %p avail %p used %p"
> -
>  # thread-pool.c
>  thread_pool_submit(void *pool, void *req, void *opaque) "pool %p req %p opaque %p"
>  thread_pool_complete(void *pool, void *req, void *opaque, int ret) "pool %p req %p opaque %p ret %d"
> -- 
> 1.8.3.1
> 
> 

Re: [Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Christian Borntraeger]

On 02/14/2016 06:17 PM, Paolo Bonzini wrote:
> Currently, dataplane threads are shut down during migration because
> vring.c is not able to track dirty memory.  However, all the relevant
> parts of QEMU have been made thread-safe now, so we can drop vring.c
> completely.  With these patches, virtio-dataplane is now simply "virtio
> with ioeventfd in a different AioContext".

Can you provide a branch somewhere? Then I can at least test even if I
have no time review.


> 
> Paolo Bonzini (8):
>   block-migration: acquire AioContext as necessary
>   vring: make vring_enable_notification return void
>   virtio: add AioContext-specific function for host notifiers
>   virtio: export vring_notify as virtio_should_notify
>   virtio-blk: fix "disabled data plane" mode
>   virtio-blk: do not use vring in dataplane
>   virtio-scsi: do not use vring in dataplane
>   vring: remove
> 
>  hw/block/dataplane/virtio-blk.c               | 130 +-----
>  hw/block/dataplane/virtio-blk.h               |   1 +
>  hw/block/virtio-blk.c                         |  51 +--
>  hw/scsi/virtio-scsi-dataplane.c               | 196 ++-------
>  hw/scsi/virtio-scsi.c                         |  52 +--
>  hw/virtio/Makefile.objs                       |   1 -
>  hw/virtio/dataplane/Makefile.objs             |   1 -
>  hw/virtio/dataplane/vring.c                   | 549 --------------------------
>  hw/virtio/virtio.c                            |  20 +-
>  include/hw/virtio/dataplane/vring-accessors.h |  75 ----
>  include/hw/virtio/dataplane/vring.h           |  51 ---
>  include/hw/virtio/virtio-blk.h                |   4 +-
>  include/hw/virtio/virtio-scsi.h               |  21 +-
>  include/hw/virtio/virtio.h                    |   3 +
>  migration/block.c                             |  61 ++-
>  trace-events                                  |   3 -
>  16 files changed, 134 insertions(+), 1085 deletions(-)
>  delete mode 100644 hw/virtio/dataplane/Makefile.objs
>  delete mode 100644 hw/virtio/dataplane/vring.c
>  delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
>  delete mode 100644 include/hw/virtio/dataplane/vring.h
> 

Re: [Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Paolo Bonzini]

On 16/02/2016 09:57, Christian Borntraeger wrote:
> > Currently, dataplane threads are shut down during migration because
> > vring.c is not able to track dirty memory.  However, all the relevant
> > parts of QEMU have been made thread-safe now, so we can drop vring.c
> > completely.  With these patches, virtio-dataplane is now simply "virtio
> > with ioeventfd in a different AioContext".
> 
> Can you provide a branch somewhere? Then I can at least test even if I
> have no time review.

Sure---the branch is on my github repo and it's called
"dataplane-migration".

Paolo

Re: [Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Michael S. Tsirkin]

On Sun, Feb 14, 2016 at 06:17:03PM +0100, Paolo Bonzini wrote:
> Currently, dataplane threads are shut down during migration because
> vring.c is not able to track dirty memory.  However, all the relevant
> parts of QEMU have been made thread-safe now, so we can drop vring.c
> completely.  With these patches, virtio-dataplane is now simply "virtio
> with ioeventfd in a different AioContext".

virtio bits look OK to me.
Removing flags like started and disabled can happen later.

> 
> Paolo Bonzini (8):
>   block-migration: acquire AioContext as necessary
>   vring: make vring_enable_notification return void
>   virtio: add AioContext-specific function for host notifiers
>   virtio: export vring_notify as virtio_should_notify
>   virtio-blk: fix "disabled data plane" mode
>   virtio-blk: do not use vring in dataplane
>   virtio-scsi: do not use vring in dataplane
>   vring: remove
> 
>  hw/block/dataplane/virtio-blk.c               | 130 +-----
>  hw/block/dataplane/virtio-blk.h               |   1 +
>  hw/block/virtio-blk.c                         |  51 +--
>  hw/scsi/virtio-scsi-dataplane.c               | 196 ++-------
>  hw/scsi/virtio-scsi.c                         |  52 +--
>  hw/virtio/Makefile.objs                       |   1 -
>  hw/virtio/dataplane/Makefile.objs             |   1 -
>  hw/virtio/dataplane/vring.c                   | 549 --------------------------
>  hw/virtio/virtio.c                            |  20 +-
>  include/hw/virtio/dataplane/vring-accessors.h |  75 ----
>  include/hw/virtio/dataplane/vring.h           |  51 ---
>  include/hw/virtio/virtio-blk.h                |   4 +-
>  include/hw/virtio/virtio-scsi.h               |  21 +-
>  include/hw/virtio/virtio.h                    |   3 +
>  migration/block.c                             |  61 ++-
>  trace-events                                  |   3 -
>  16 files changed, 134 insertions(+), 1085 deletions(-)
>  delete mode 100644 hw/virtio/dataplane/Makefile.objs
>  delete mode 100644 hw/virtio/dataplane/vring.c
>  delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
>  delete mode 100644 include/hw/virtio/dataplane/vring.h
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Paolo Bonzini]

On 19/02/2016 08:42, Michael S. Tsirkin wrote:
> On Sun, Feb 14, 2016 at 06:17:03PM +0100, Paolo Bonzini wrote:
> > Currently, dataplane threads are shut down during migration because
> > vring.c is not able to track dirty memory.  However, all the relevant
> > parts of QEMU have been made thread-safe now, so we can drop vring.c
> > completely.  With these patches, virtio-dataplane is now simply "virtio
> > with ioeventfd in a different AioContext".
> 
> virtio bits look OK to me.
> Removing flags like started and disabled can happen later.

Ok, as you prefer!

I'll reply to Fam now, I had missed his remark.

Paolo

Re: [Qemu-devel] [PATCH 0/8] virtio: allow migration with dataplane

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 2180 bytes --]

On Sun, Feb 14, 2016 at 06:17:03PM +0100, Paolo Bonzini wrote:
> Currently, dataplane threads are shut down during migration because
> vring.c is not able to track dirty memory.  However, all the relevant
> parts of QEMU have been made thread-safe now, so we can drop vring.c
> completely.  With these patches, virtio-dataplane is now simply "virtio
> with ioeventfd in a different AioContext".
> 
> Paolo Bonzini (8):
>   block-migration: acquire AioContext as necessary
>   vring: make vring_enable_notification return void
>   virtio: add AioContext-specific function for host notifiers
>   virtio: export vring_notify as virtio_should_notify
>   virtio-blk: fix "disabled data plane" mode
>   virtio-blk: do not use vring in dataplane
>   virtio-scsi: do not use vring in dataplane
>   vring: remove
> 
>  hw/block/dataplane/virtio-blk.c               | 130 +-----
>  hw/block/dataplane/virtio-blk.h               |   1 +
>  hw/block/virtio-blk.c                         |  51 +--
>  hw/scsi/virtio-scsi-dataplane.c               | 196 ++-------
>  hw/scsi/virtio-scsi.c                         |  52 +--
>  hw/virtio/Makefile.objs                       |   1 -
>  hw/virtio/dataplane/Makefile.objs             |   1 -
>  hw/virtio/dataplane/vring.c                   | 549 --------------------------
>  hw/virtio/virtio.c                            |  20 +-
>  include/hw/virtio/dataplane/vring-accessors.h |  75 ----
>  include/hw/virtio/dataplane/vring.h           |  51 ---
>  include/hw/virtio/virtio-blk.h                |   4 +-
>  include/hw/virtio/virtio-scsi.h               |  21 +-
>  include/hw/virtio/virtio.h                    |   3 +
>  migration/block.c                             |  61 ++-
>  trace-events                                  |   3 -
>  16 files changed, 134 insertions(+), 1085 deletions(-)
>  delete mode 100644 hw/virtio/dataplane/Makefile.objs
>  delete mode 100644 hw/virtio/dataplane/vring.c
>  delete mode 100644 include/hw/virtio/dataplane/vring-accessors.h
>  delete mode 100644 include/hw/virtio/dataplane/vring.h
> 
> -- 
> 1.8.3.1
> 
> 

Acked-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]