| Message ID | a2eca26b79e1fcc30128a266bfa416237366c533.1598257838.git.dimastep@yandex-team.ru |
|---|---|
| State | New |
| Headers | show |
| Series | vhost-user-blk: fix the migration issue and enhance qtests | expand |
On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > If the vhost-user-blk daemon provides only one virtqueue, but device was > added with several queues, then QEMU will send more VHOST-USER command > than expected by daemon side. The vhost_virtqueue_start() routine > handles such case by checking the return value from the > virtio_queue_get_desc_addr() function call. Add the same check to the > vhost_dev_set_log() routine. > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > --- > hw/virtio/vhost.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index ffef7ab..a33ffd4 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > { > int r, i, idx; > + hwaddr addr; > + > r = vhost_dev_set_features(dev, enable_log); > if (r < 0) { > goto err_features; > } > for (i = 0; i < dev->nvqs; ++i) { > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); > + addr = virtio_queue_get_desc_addr(dev->vdev, idx); > + if (!addr) { > + /* > + * The queue might not be ready for start. If this > + * is the case there is no reason to continue the process. > + * The similar logic is used by the vhost_virtqueue_start() > + * routine. > + */ Shouldn’t we goto err_vq here to reset the logging state of any vqs which have already been set? > + break; > + } > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > enable_log); > if (r < 0) { > -- > 2.7.4 > >
On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote: > On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > > > If the vhost-user-blk daemon provides only one virtqueue, but device was > > added with several queues, then QEMU will send more VHOST-USER command > > than expected by daemon side. The vhost_virtqueue_start() routine > > handles such case by checking the return value from the > > virtio_queue_get_desc_addr() function call. Add the same check to the > > vhost_dev_set_log() routine. > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > --- > > hw/virtio/vhost.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > index ffef7ab..a33ffd4 100644 > > --- a/hw/virtio/vhost.c > > +++ b/hw/virtio/vhost.c > > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > > { > > int r, i, idx; > > + hwaddr addr; > > + > > r = vhost_dev_set_features(dev, enable_log); > > if (r < 0) { > > goto err_features; > > } > > for (i = 0; i < dev->nvqs; ++i) { > > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); > > + addr = virtio_queue_get_desc_addr(dev->vdev, idx); > > + if (!addr) { > > + /* > > + * The queue might not be ready for start. If this > > + * is the case there is no reason to continue the process. > > + * The similar logic is used by the vhost_virtqueue_start() > > + * routine. > > + */ > > Shouldn’t we goto err_vq here to reset the logging state of any vqs > which have already been set? As i understand it, no we shouldn't reset the state of other queues. In general it is pretty valid case. Let's assume that the backend vhost-user device supports only two queues. But for instance, the QEMU command line is using value 4 to define number of virtqueues of such device. In this case only 2 queues will be initializaed. I've tried to reflect it in the comment section, that the vhost_virtqueue_start() routine has been alread made the same: "if a queue isn't ready for start, just return 0 without any error" So i made the same here. I've found this issue, while testing migration with the default vhost-user-blk daemon. It fails with assert or sigsegv (don't remember), because it receives NULL for the queues it doesn't have. In general the daemon should not fall, because of unexpected VHOST_USER communication, but also there is no reason for QEMU to send additional packets. > > > + break; > > + } > > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > > enable_log); > > if (r < 0) { > > -- > > 2.7.4 > > > >
On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote: > > On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > > > > > If the vhost-user-blk daemon provides only one virtqueue, but device was > > > added with several queues, then QEMU will send more VHOST-USER command > > > than expected by daemon side. The vhost_virtqueue_start() routine > > > handles such case by checking the return value from the > > > virtio_queue_get_desc_addr() function call. Add the same check to the > > > vhost_dev_set_log() routine. > > > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > > --- > > > hw/virtio/vhost.c | 12 ++++++++++++ > > > 1 file changed, 12 insertions(+) > > > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > index ffef7ab..a33ffd4 100644 > > > --- a/hw/virtio/vhost.c > > > +++ b/hw/virtio/vhost.c > > > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, > > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > > > { > > > int r, i, idx; > > > + hwaddr addr; > > > + > > > r = vhost_dev_set_features(dev, enable_log); > > > if (r < 0) { > > > goto err_features; > > > } > > > for (i = 0; i < dev->nvqs; ++i) { > > > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); > > > + addr = virtio_queue_get_desc_addr(dev->vdev, idx); > > > + if (!addr) { > > > + /* > > > + * The queue might not be ready for start. If this > > > + * is the case there is no reason to continue the process. > > > + * The similar logic is used by the vhost_virtqueue_start() > > > + * routine. > > > + */ > > > > Shouldn’t we goto err_vq here to reset the logging state of any vqs > > which have already been set? > As i understand it, no we shouldn't reset the state of other queues. In > general it is pretty valid case. Let's assume that the backend > vhost-user device supports only two queues. But for instance, the QEMU > command line is using value 4 to define number of virtqueues of such > device. In this case only 2 queues will be initializaed. I see - makes more sense now. > > I've tried to reflect it in the comment section, that the > vhost_virtqueue_start() routine has been alread made the same: > "if a queue isn't ready for start, just return 0 without any error" > So i made the same here. > In your example is a reason why, if queue 3 is uninitialized, queue 4 must also be uninitialized? I realize queue 4 being initialized while queue 3 is not is a strange case, but it may still make the code more robust to use a "continue" here instead of a "break". This also seems more like the logic in vhost_virtqueue_start()/vhost_dev_start(). > I've found this issue, while testing migration with the default > vhost-user-blk daemon. It fails with assert or sigsegv (don't remember), > because it receives NULL for the queues it doesn't have. In general > the daemon should not fall, because of unexpected VHOST_USER > communication, but also there is no reason for QEMU to send additional > packets. > > > > > > + break; > > > + } > > > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > > > enable_log); > > > if (r < 0) { > > > -- > > > 2.7.4 > > > > > >
On Mon, Aug 31, 2020 at 11:22:14PM -0400, Raphael Norwitz wrote: > On Mon, Aug 31, 2020 at 4:37 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > > > On Thu, Aug 27, 2020 at 09:46:03PM -0400, Raphael Norwitz wrote: > > > On Mon, Aug 24, 2020 at 4:41 AM Dima Stepanov <dimastep@yandex-team.ru> wrote: > > > > > > > > If the vhost-user-blk daemon provides only one virtqueue, but device was > > > > added with several queues, then QEMU will send more VHOST-USER command > > > > than expected by daemon side. The vhost_virtqueue_start() routine > > > > handles such case by checking the return value from the > > > > virtio_queue_get_desc_addr() function call. Add the same check to the > > > > vhost_dev_set_log() routine. > > > > > > > > Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> > > > > --- > > > > hw/virtio/vhost.c | 12 ++++++++++++ > > > > 1 file changed, 12 insertions(+) > > > > > > > > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > > > > index ffef7ab..a33ffd4 100644 > > > > --- a/hw/virtio/vhost.c > > > > +++ b/hw/virtio/vhost.c > > > > @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, > > > > static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) > > > > { > > > > int r, i, idx; > > > > + hwaddr addr; > > > > + > > > > r = vhost_dev_set_features(dev, enable_log); > > > > if (r < 0) { > > > > goto err_features; > > > > } > > > > for (i = 0; i < dev->nvqs; ++i) { > > > > idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); > > > > + addr = virtio_queue_get_desc_addr(dev->vdev, idx); > > > > + if (!addr) { > > > > + /* > > > > + * The queue might not be ready for start. If this > > > > + * is the case there is no reason to continue the process. > > > > + * The similar logic is used by the vhost_virtqueue_start() > > > > + * routine. > > > > + */ > > > > > > Shouldn’t we goto err_vq here to reset the logging state of any vqs > > > which have already been set? > > As i understand it, no we shouldn't reset the state of other queues. In > > general it is pretty valid case. Let's assume that the backend > > vhost-user device supports only two queues. But for instance, the QEMU > > command line is using value 4 to define number of virtqueues of such > > device. In this case only 2 queues will be initializaed. > > I see - makes more sense now. > > > > > I've tried to reflect it in the comment section, that the > > vhost_virtqueue_start() routine has been alread made the same: > > "if a queue isn't ready for start, just return 0 without any error" > > So i made the same here. > > > > In your example is a reason why, if queue 3 is uninitialized, queue 4 > must also be uninitialized? I realize queue 4 being initialized while > queue 3 is not is a strange case, but it may still make the code more > robust to use a "continue" here instead of a "break". This also seems > more like the logic in vhost_virtqueue_start()/vhost_dev_start(). Good point! Should really use "continue" instead of a "break" to keep the logic. Will update it in v4. Hope to get some review feedback for the qtest framework update aswell ). > > > I've found this issue, while testing migration with the default > > vhost-user-blk daemon. It fails with assert or sigsegv (don't remember), > > because it receives NULL for the queues it doesn't have. In general > > the daemon should not fall, because of unexpected VHOST_USER > > communication, but also there is no reason for QEMU to send additional > > packets. > > > > > > > > > + break; > > > > + } > > > > r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, > > > > enable_log); > > > > if (r < 0) { > > > > -- > > > > 2.7.4 > > > > > > > >
diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index ffef7ab..a33ffd4 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -825,12 +825,24 @@ static int vhost_dev_set_features(struct vhost_dev *dev, static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log) { int r, i, idx; + hwaddr addr; + r = vhost_dev_set_features(dev, enable_log); if (r < 0) { goto err_features; } for (i = 0; i < dev->nvqs; ++i) { idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i); + addr = virtio_queue_get_desc_addr(dev->vdev, idx); + if (!addr) { + /* + * The queue might not be ready for start. If this + * is the case there is no reason to continue the process. + * The similar logic is used by the vhost_virtqueue_start() + * routine. + */ + break; + } r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx, enable_log); if (r < 0) {
If the vhost-user-blk daemon provides only one virtqueue, but device was added with several queues, then QEMU will send more VHOST-USER command than expected by daemon side. The vhost_virtqueue_start() routine handles such case by checking the return value from the virtio_queue_get_desc_addr() function call. Add the same check to the vhost_dev_set_log() routine. Signed-off-by: Dima Stepanov <dimastep@yandex-team.ru> --- hw/virtio/vhost.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)