On Thu, Jul 27, 2017 at 10:00:50AM +0800, Changpeng Liu wrote: > +static void vhost_user_blk_set_config(VirtIODevice *vdev, const uint8_t > *config) > +{ > + VHostUserBlk *s = VHOST_USER_BLK(vdev); > + struct virtio_blk_config blkcfg; > + > + memcpy(&blkcfg, config, sizeof(blkcfg)); > + > + if (blkcfg.wce != s->config_wce) { > + error_report("vhost-user-blk: does not support the operation");
If vhost-user-blk doesn't support the operation then please remove the VIRTIO_BLK_F_CONFIG_WCE feature bit. That way the guest knows it cannot modify this field. > +static void vhost_user_blk_device_realize(DeviceState *dev, Error **errp) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > + VHostUserBlk *s = VHOST_USER_BLK(vdev); > + int ret; > + uint64_t size; > + > + if (!s->chardev.chr) { > + error_setg(errp, "vhost-user-blk: chardev is mandatary"); mandatory > + return; > + } > + > + if (!s->num_queues) { > + error_setg(errp, "vhost-user-blk: invalid number of IO queues"); > + return; > + } > + > + if (!s->queue_size) { > + error_setg(errp, "vhost-user-blk: invalid count of the IO queue"); "count of the IO queue" sounds like number of queues. I suggest saying "queue size must be non-zero". > + return; > + } > + > + if (!s->size) { > + error_setg(errp, "vhost-user-blk: block capacity must be assigned," > + "size can be specified by GiB or MiB"); > + return; > + } > + > + ret = qemu_strtosz_MiB(s->size, NULL, &size); > + if (ret < 0) { > + error_setg(errp, "vhost-user-blk: invalid size %s in GiB/MiB", > s->size); > + return; > + } > + s->capacity = size / 512; > + > + /* block size with default 512 Bytes */ > + if (!s->blkcfg.logical_block_size) { > + s->blkcfg.logical_block_size = 512; > + } > + > + virtio_init(vdev, "virtio-blk", VIRTIO_ID_BLOCK, > + sizeof(struct virtio_blk_config)); > + virtio_add_queue(vdev, s->queue_size, vhost_user_blk_handle_output); > + > + s->dev.nvqs = s->num_queues; > + s->dev.vqs = g_new(struct vhost_virtqueue, s->dev.nvqs); Please test multi-queue, it's currently broken. virtio_add_queue() must be called for each queue. > + s->dev.vq_index = 0; > + s->dev.backend_features = 0; > + > + ret = vhost_dev_init(&s->dev, (void *)&s->chardev, The compiler automatically converts any pointer type to void * without a warning in C. (This is different from C++!) The (void *) cast can be dropped. > + VHOST_BACKEND_TYPE_USER, 0); > + if (ret < 0) { > + error_setg(errp, "vhost-user-blk: vhost initialization failed: %s", > + strerror(-ret)); If realize fails unrealize() will not be called. Cleanup must be performed here. > + return; > + } > +} > + > +static void vhost_user_blk_device_unrealize(DeviceState *dev, Error **errp) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > + VHostUserBlk *s = VHOST_USER_BLK(dev); > + > + vhost_user_blk_set_status(vdev, 0); > + vhost_dev_cleanup(&s->dev); > + g_free(s->dev.vqs); > + virtio_cleanup(vdev); > +} > + > +static void vhost_user_blk_instance_init(Object *obj) > +{ > + VHostUserBlk *s = VHOST_USER_BLK(obj); > + > + device_add_bootindex_property(obj, &s->bootindex, "bootindex", > + "/disk@0,0", DEVICE(obj), NULL); > +} > + > +static const VMStateDescription vmstate_vhost_user_blk = { > + .name = "vhost-user-blk", > + .minimum_version_id = 2, > + .version_id = 2, Why is version_id 2? There has never been a vhost-user-blk device in qemu.git before, so I would expect version to be 1. > + .fields = (VMStateField[]) { > + VMSTATE_VIRTIO_DEVICE, > + VMSTATE_END_OF_LIST() > + }, > +}; > + > +static Property vhost_user_blk_properties[] = { > + DEFINE_PROP_CHR("chardev", VHostUserBlk, chardev), > + DEFINE_BLOCK_PROPERTIES(VHostUserBlk, blkcfg), DEFINE_BLOCK_PROPERTIES cannot be used directly because it includes the 'drive' (blkcfg.blk) parameter. The command-line should not allow a drive= parameter. Also, parameters like the discard granularity, optimal I/O size, logical block size, etc can be initialized to sensible defaults by QEMU's block layer when drive= is used. Since you are bypassing QEMU's block layer there is no way for QEMU to set good defaults. Are you relying on the managment tools populating these parameters with the correct values from the vhost-user-blk process? Or did you have some other mechanism in mind? > + DEFINE_BLOCK_CHS_PROPERTIES(VHostUserBlk, blkcfg), > + DEFINE_PROP_STRING("size", VHostUserBlk, size), virtio-blk supports online resize. QEMU and the vhost-user-blk process need a way to exchange disk capacity information for online resize. Please add the necessary vhost-user protocol messages now so size information can be automatically exchanged and updated for resize. > +typedef struct VHostUserBlk { > + VirtIODevice parent_obj; > + CharBackend chardev; > + Error *migration_blocker; This field is unused, please drop it.
signature.asc
Description: PGP signature