On Mon, Apr 26, 2021 at 11:05 PM Stefan Hajnoczi <stefa...@redhat.com> wrote:
>
> On Thu, Apr 08, 2021 at 06:12:52PM +0800, Xie Yongji wrote:
> > +static const int vdpa_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_MQ,
> > + VIRTIO_BLK_F_RO,
> > + VIRTIO_BLK_F_FLUSH,
> > + VIRTIO_BLK_F_CONFIG_WCE,
> > + VIRTIO_BLK_F_DISCARD,
> > + VIRTIO_BLK_F_WRITE_ZEROES,
> > + VIRTIO_F_VERSION_1,
> > + VIRTIO_RING_F_INDIRECT_DESC,
> > + VIRTIO_RING_F_EVENT_IDX,
> > + VIRTIO_F_NOTIFY_ON_EMPTY,
> > + VHOST_INVALID_FEATURE_BIT
> > +};
>
> Please add VIRTIO_F_RING_PACKED so it can be implemented in vDPA in the
> future without changes to the QEMU vhost-vdpa-blk.c code.
>
Sure.
> > +static void vhost_vdpa_blk_device_realize(DeviceState *dev, Error **errp)
> > +{
> > + VirtIODevice *vdev = VIRTIO_DEVICE(dev);
> > + VHostVdpaBlk *s = VHOST_VDPA_BLK(vdev);
> > + VHostBlkCommon *vbc = VHOST_BLK_COMMON(s);
> > + Error *err = NULL;
> > + int ret;
> > +
> > + s->vdpa.device_fd = qemu_open_old(s->vdpa_dev, O_RDWR);
> > + if (s->vdpa.device_fd == -1) {
> > + error_setg(errp, "vhost-vdpa-blk: open %s failed: %s",
> > + s->vdpa_dev, strerror(errno));
> > + return;
> > + }
> > +
> > + vhost_blk_common_realize(vbc, vhost_vdpa_blk_handle_output, &err);
> > + if (err != NULL) {
> > + error_propagate(errp, err);
> > + goto blk_err;
> > + }
> > +
> > + vbc->vhost_vqs = g_new0(struct vhost_virtqueue, vbc->num_queues);
>
> This is already done by vhost_blk_common_realize(). The old pointer is
> overwritten and leaked here.
>
Will fix it.
> > +static const VMStateDescription vmstate_vhost_vdpa_blk = {
> > + .name = "vhost-vdpa-blk",
> > + .minimum_version_id = 1,
> > + .version_id = 1,
> > + .fields = (VMStateField[]) {
> > + VMSTATE_VIRTIO_DEVICE,
> > + VMSTATE_END_OF_LIST()
> > + },
> > +};
>
> vdpa-blk does not support live migration yet. Please remove this.
>
> Does hw/virtio/vhost.c should automatically register a migration
> blocker? If not, please register one.
>
Will do it.
> > +#define TYPE_VHOST_VDPA_BLK "vhost-vdpa-blk"
>
> At this stage vdpa-blk is still very new and in development. I suggest
> naming it x-vhost-vdpa-blk so that incompatible changes can still be
> made to the command-line/APIs. "x-" can be removed later when the
> feature has matured.
OK.
Thanks,
Yongji
