[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log rou
From: |
Dima Stepanov |
Subject: |
Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine |
Date: |
Tue, 1 Sep 2020 11:36:17 +0300 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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
> > > >
> > > >
[Prev in Thread] |
Current Thread |
[Next in Thread] |
- Re: [PATCH v2 2/7] vhost: check queue state in the vhost_dev_set_log routine,
Dima Stepanov <=