Re: [Qemu-devel] [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk host device

[Liu, Changpeng]

> -----Original Message-----
> From: Paolo Bonzini [mailto:address@hidden
> Sent: Thursday, October 19, 2017 7:33 PM
> To: Liu, Changpeng <address@hidden>; address@hidden
> Cc: address@hidden; address@hidden; address@hidden;
> address@hidden; Harris, James R <address@hidden>
> Subject: Re: [PATCH v4 2/4] vhost-user-blk: introduce a new vhost-user-blk 
> host
> device
> 
> On 19/10/2017 07:24, Changpeng Liu wrote:
> >    ;;
> >    --enable-vhost-scsi) vhost_scsi="yes"
> >    ;;
> > +  --disable-vhost-user-blk) vhost_user_blk="no"
> > +  ;;
> > +  --enable-vhost-user-blk) vhost_user_blk="yes"
> > +  ;;
> >    --disable-vhost-vsock) vhost_vsock="no"
> >    ;;
> >    --enable-vhost-vsock) vhost_vsock="yes"
> > @@ -1511,6 +1517,7 @@ disabled with --disable-FEATURE, default is enabled if
> available:
> >    cap-ng          libcap-ng support
> >    attr            attr and xattr support
> >    vhost-net       vhost-net acceleration support
> > +  vhost-user-blk  VM virtio-blk acceleration in user space
> 
> Please use default-configs instead of a new configure switch.  See how
> CONFIG_VHOST_USER_SCSI is used in default-configs/pci.mak and
> default-configs/s390x-softmmu.mak.
Ok, thanks.
> 
> >
> > +static const int user_feature_bits[] = {
> > +    VIRTIO_BLK_F_SIZE_MAX,
> > +    VIRTIO_BLK_F_SEG_MAX,
> > +    VIRTIO_BLK_F_GEOMETRY,
> > +    VIRTIO_BLK_F_BLK_SIZE,
> > +    VIRTIO_BLK_F_TOPOLOGY,
> > +    VIRTIO_BLK_F_SCSI,
> 
> Please omit VIRTIO_BLK_F_SCSI, it's a legacy option that is not anymore
> part of virtio 1.0.
ok
> 
> > +    VIRTIO_BLK_F_MQ,
> > +    VIRTIO_BLK_F_RO,
> > +    VIRTIO_BLK_F_FLUSH,
> > +    VIRTIO_BLK_F_BARRIER,
> 
> Same for VIRTIO_BLK_F_BARRIER.
> 
> > +    VIRTIO_BLK_F_WCE,
> 
> And VIRTIO_BLK_F_WCE is the same as VIRTIO_BLK_F_FLUSH, so it can be
> removed too.  Please include VIRTIO_BLK_F_CONFIG_WCE instead, since you
> are supporting it in vhost_user_blk_set_config.
Ok.
> 
> > +    VIRTIO_F_VERSION_1,
> > +    VIRTIO_RING_F_INDIRECT_DESC,
> > +    VIRTIO_RING_F_EVENT_IDX,
> > +    VIRTIO_F_NOTIFY_ON_EMPTY,
> > +    VHOST_INVALID_FEATURE_BIT
> > +};
> 
> >
> > +static const TypeInfo vhost_user_blk_info = {
> > +    .name = TYPE_VHOST_USER_BLK,
> > +    .parent = TYPE_VIRTIO_DEVICE,
> > +    .instance_size = sizeof(VHostUserBlk),
> > +    .instance_init = vhost_user_blk_instance_init,
> > +    .class_init = vhost_user_blk_class_init,
> > +};
> > +
> 
> There is some code duplication, so maybe it's worth introducing a common
> superclass like TYPE_VIRTIO_SCSI_COMMON.  I'll let others comment on
> whether this is a requirement.
> 
> Paolo