Re: [Qemu-devel] [PATCH 14/15] hw/block/virtio-blk: create num_queues vqs if dataplane is enabled

[Michael S. Tsirkin]

On Wed, Jul 30, 2014 at 04:01:59PM +0200, Paolo Bonzini wrote:
> Il 30/07/2014 13:39, Ming Lei ha scritto:
> > Now we only support multi vqs for dataplane case.
> > 
> > Signed-off-by: Ming Lei <address@hidden>
> > ---
> >  hw/block/virtio-blk.c          |   18 +++++++++++++++++-
> >  include/hw/virtio/virtio-blk.h |    1 +
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> > index ab99156..44e9956 100644
> > --- a/hw/block/virtio-blk.c
> > +++ b/hw/block/virtio-blk.c
> > @@ -556,6 +556,7 @@ static void virtio_blk_update_config(VirtIODevice 
> > *vdev, uint8_t *config)
> >      blkcfg.physical_block_exp = get_physical_block_exp(s->conf);
> >      blkcfg.alignment_offset = 0;
> >      blkcfg.wce = bdrv_enable_write_cache(s->bs);
> > +    stw_p(&blkcfg.num_queues, s->blk.num_queues);
> >      memcpy(config, &blkcfg, sizeof(struct virtio_blk_config));
> >  }
> >  
> > @@ -590,6 +591,12 @@ static uint32_t virtio_blk_get_features(VirtIODevice 
> > *vdev, uint32_t features)
> >      if (bdrv_is_read_only(s->bs))
> >          features |= 1 << VIRTIO_BLK_F_RO;
> >  
> > +#ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> > +    if (s->blk.num_queues > 1) {
> > +        features |= 1 << VIRTIO_BLK_F_MQ;
> > +    }
> > +#endif
> > +
> >      return features;
> >  }
> >  
> > @@ -739,8 +746,13 @@ static void virtio_blk_device_realize(DeviceState 
> > *dev, Error **errp)
> >  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> >      Error *err = NULL;
> >  #endif
> > +    int i;
> >      static int virtio_blk_id;
> >  
> > +#ifndef CONFIG_VIRTIO_BLK_DATA_PLANE
> > +    blk->num_queues = 1;
> > +#endif
> > +
> >      if (!blk->conf.bs) {
> >          error_setg(errp, "drive property not set");
> >          return;
> > @@ -765,7 +777,10 @@ static void virtio_blk_device_realize(DeviceState 
> > *dev, Error **errp)
> >      s->rq = NULL;
> >      s->sector_mask = (s->conf->logical_block_size / BDRV_SECTOR_SIZE) - 1;
> >  
> > -    s->vq = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
> > +    s->vqs = g_malloc0(sizeof(VirtQueue *) * blk->num_queues);
> > +    for (i = 0; i < blk->num_queues; i++)
> > +        s->vqs[i] = virtio_add_queue(vdev, 128, virtio_blk_handle_output);
> > +    s->vq = s->vqs[0];
> >      s->complete_request = virtio_blk_complete_request;
> >  #ifdef CONFIG_VIRTIO_BLK_DATA_PLANE
> >      virtio_blk_data_plane_create(vdev, blk, &s->dataplane, &err);
> > @@ -802,6 +817,7 @@ static void virtio_blk_device_unrealize(DeviceState 
> > *dev, Error **errp)
> >      qemu_del_vm_change_state_handler(s->change);
> >      unregister_savevm(dev, "virtio-blk", s);
> >      blockdev_mark_auto_del(s->bs);
> > +    g_free(s->vqs);
> >      virtio_cleanup(vdev);
> >  }
> >  
> > diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> > index ad70c9a..b4609dd 100644
> > --- a/include/hw/virtio/virtio-blk.h
> > +++ b/include/hw/virtio/virtio-blk.h
> > @@ -132,6 +132,7 @@ typedef struct VirtIOBlock {
> >      VirtIODevice parent_obj;
> >      BlockDriverState *bs;
> >      VirtQueue *vq;
> > +    VirtQueue **vqs;
> >      void *rq;
> >      QEMUBH *bh;
> >      BlockConf *conf;
> > 
> 
> Dataplane must not be a change to the guest ABI.  If you implement this
> feature you have to implement it for both dataplane and non-dataplne.
> 
> Paolo

Actually, I think different backends at runtime should be allowed to
change guest visible ABI.  For example if you give qemu a read only
backend this is guest visible right?

However relying on ifdefs is wrong.
It should be a runtime thing.

-- 
MST