On Mon, Oct 19, 2015 at 06:41:38PM +0200, Thibaut Collet wrote:
> On Mon, Oct 19, 2015 at 5:41 PM, Michael S. Tsirkin <m...@redhat.com> wrote:
> > On Mon, Oct 19, 2015 at 02:59:27PM +0200, Thibaut Collet wrote:
> >> When a live migration is started the log address to mark dirty pages is
> >> provided
> >> to the vhost backend through the vhost_dev_set_log function.
> >> This function is called for each queue pairs but the queue index is
> >> wrongly set:
> >> always set to the first queue pair. Then vhost backend lost descriptor
> >> addresses
> >> of the queue pairs greater than 1 and behaviour of the vhost backend is
> >> unpredictable.
> >>
> >> The queue index is computed by taking account of the vq_index (to retrieve
> >> the
> >> queue pair index) and calling the vhost_get_vq_index method of the backend.
> >>
> >> Signed-off-by: Thibaut Collet <thibaut.col...@6wind.com>
> >
> > This needs some thought to make sure we don't break the kernel vhost.
>
> For kernel vhost my patch does nothing has vhost_get_vq_index method
> for vhost kernel subtract dev->vq_index (that was just added before)
> and idx is still equal to i.
>
> >
> > I queued this temporarily to enable your testing but I think it would be
> > preferable to make vhost_virtqueue_set_addr for vhost_user call
> > vhost_get_vq_index internally.
> >
>
> If I call the vhost_get_vq_index internally by vhost_user when
> vhost_virtqueue_set_addr is called I will break the
> vhost_virtqueue_start: this function calls the vhost_get_vq_index
> function for vhost user and vhost kernel to initializes the queue.
So drop vhost_get_vq_index from there as well then?
> >
> >
> >> ---
> >> hw/virtio/vhost.c | 8 +++++---
> >> 1 file changed, 5 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
> >> index feeaaa4..de29968 100644
> >> --- a/hw/virtio/vhost.c
> >> +++ b/hw/virtio/vhost.c
> >> @@ -656,13 +656,14 @@ static int vhost_dev_set_features(struct vhost_dev
> >> *dev, bool enable_log)
> >>
> >> static int vhost_dev_set_log(struct vhost_dev *dev, bool enable_log)
> >> {
> >> - int r, t, i;
> >> + int r, t, i, idx;
> >> r = vhost_dev_set_features(dev, enable_log);
> >> if (r < 0) {
> >> goto err_features;
> >> }
> >> for (i = 0; i < dev->nvqs; ++i) {
> >> - r = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> >> + idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> >> + r = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >> enable_log);
> >> if (r < 0) {
> >> goto err_vq;
> >> @@ -671,7 +672,8 @@ static int vhost_dev_set_log(struct vhost_dev *dev,
> >> bool enable_log)
> >> return 0;
> >> err_vq:
> >> for (; i >= 0; --i) {
> >> - t = vhost_virtqueue_set_addr(dev, dev->vqs + i, i,
> >> + idx = dev->vhost_ops->vhost_get_vq_index(dev, dev->vq_index + i);
> >> + t = vhost_virtqueue_set_addr(dev, dev->vqs + i, idx,
> >> dev->log_enabled);
> >> assert(t >= 0);
> >> }
> >> --
> >> 2.1.4
