To restore the device in the destination of a live migration we send the
commands through control virtqueue. For a device to read CVQ it must
have received DRIVER_OK status bit.
However this open a window where the device could start receiving
packets in rx queue 0 before it receive the RSS configuration. To avoid
that, we will not send vring_enable until all configuration is used by
the device.
As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
---
hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 66f054a12c..2ee8009594 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -728,13 +728,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
return idx;
}
+/**
+ * Set ready all vring of the device
+ *
+ * @dev: Vhost device
+ */
static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
{
int i;
trace_vhost_vdpa_set_vring_ready(dev);
- for (i = 0; i < dev->nvqs; ++i) {
+ for (i = 0; i < dev->vq_index_end; ++i) {
struct vhost_vring_state state = {
- .index = dev->vq_index + i,
+ .index = i,
.num = 1,
};
vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
@@ -1097,7 +1102,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
if (unlikely(!ok)) {
return -1;
}
- vhost_vdpa_set_vring_ready(dev);
} else {
ok = vhost_vdpa_svqs_stop(dev);
if (unlikely(!ok)) {
@@ -1111,16 +1115,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
}
if (started) {
+ int r;
+
memory_listener_register(&v->listener, &address_space_memory);
- return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
+ if (unlikely(r)) {
+ return r;
+ }
+ vhost_vdpa_set_vring_ready(dev);
} else {
vhost_vdpa_reset_device(dev);
vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
VIRTIO_CONFIG_S_DRIVER);
memory_listener_unregister(&v->listener);
-
- return 0;
}
+
+ return 0;
}
static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
--
2.31.1
On Wed, Jul 06, 2022 at 08:39:48PM +0200, Eugenio Pérez wrote:
> To restore the device in the destination of a live migration we send the
> commands through control virtqueue. For a device to read CVQ it must
> have received DRIVER_OK status bit.
>
> However this open a window where the device could start receiving
> packets in rx queue 0 before it receive the RSS configuration. To avoid
> that, we will not send vring_enable until all configuration is used by
> the device.
>
> As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
Not a comment on this patch specifically, but generally:
You should know that lots of existing drivers are buggy and
try to poke at the VQs before DRIVER_OK. We are doing our best
to fix them but it's taking forever. For now it's a good
idea to support such drivers even if they are out of spec.
You do that by starting on the first kick in absence of DRIVER_OK.
Further, adding buffers before DRIVER_OK is actually allowed,
as long as you don't kick.
> ---
> hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 66f054a12c..2ee8009594 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -728,13 +728,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> return idx;
> }
>
> +/**
> + * Set ready all vring of the device
> + *
> + * @dev: Vhost device
> + */
> static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> {
> int i;
> trace_vhost_vdpa_set_vring_ready(dev);
> - for (i = 0; i < dev->nvqs; ++i) {
> + for (i = 0; i < dev->vq_index_end; ++i) {
> struct vhost_vring_state state = {
> - .index = dev->vq_index + i,
> + .index = i,
> .num = 1,
> };
> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> @@ -1097,7 +1102,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> if (unlikely(!ok)) {
> return -1;
> }
> - vhost_vdpa_set_vring_ready(dev);
> } else {
> ok = vhost_vdpa_svqs_stop(dev);
> if (unlikely(!ok)) {
> @@ -1111,16 +1115,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> }
>
> if (started) {
> + int r;
> +
> memory_listener_register(&v->listener, &address_space_memory);
> - return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + if (unlikely(r)) {
> + return r;
> + }
> + vhost_vdpa_set_vring_ready(dev);
> } else {
> vhost_vdpa_reset_device(dev);
> vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> VIRTIO_CONFIG_S_DRIVER);
> memory_listener_unregister(&v->listener);
> -
> - return 0;
> }
> +
> + return 0;
> }
>
> static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> --
> 2.31.1
On Wed, Jul 13, 2022 at 7:52 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Jul 06, 2022 at 08:39:48PM +0200, Eugenio Pérez wrote:
> > To restore the device in the destination of a live migration we send the
> > commands through control virtqueue. For a device to read CVQ it must
> > have received DRIVER_OK status bit.
> >
> > However this open a window where the device could start receiving
> > packets in rx queue 0 before it receive the RSS configuration. To avoid
> > that, we will not send vring_enable until all configuration is used by
> > the device.
> >
> > As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>
> Not a comment on this patch specifically, but generally:
>
> You should know that lots of existing drivers are buggy and
> try to poke at the VQs before DRIVER_OK. We are doing our best
> to fix them but it's taking forever. For now it's a good
> idea to support such drivers even if they are out of spec.
I think vhost-vdpa should not need to explicitly handle it, since it
is started after DRIVER_OK. But I think it's a good idea to perform a
fast test. I think those kicks will go to the device's ioeventfd and
the specific virtqueue's handle_output callback.
> You do that by starting on the first kick in absence of DRIVER_OK.
> Further, adding buffers before DRIVER_OK is actually allowed,
> as long as you don't kick.
>
SVQ adds all the buffers after the guest's driver_ok and after set
driver_ok to the device. What it does is to send CVQ buffers before
enabling the data queues with VHOST_VDPA_SET_VRING_ENABLE. Only CVQ is
enabled at this point, but DRIVER_OK has already been sent.
Or am I missing something?
Thanks!
>
> > ---
> > hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 66f054a12c..2ee8009594 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -728,13 +728,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> > return idx;
> > }
> >
> > +/**
> > + * Set ready all vring of the device
> > + *
> > + * @dev: Vhost device
> > + */
> > static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> > {
> > int i;
> > trace_vhost_vdpa_set_vring_ready(dev);
> > - for (i = 0; i < dev->nvqs; ++i) {
> > + for (i = 0; i < dev->vq_index_end; ++i) {
> > struct vhost_vring_state state = {
> > - .index = dev->vq_index + i,
> > + .index = i,
> > .num = 1,
> > };
> > vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> > @@ -1097,7 +1102,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > if (unlikely(!ok)) {
> > return -1;
> > }
> > - vhost_vdpa_set_vring_ready(dev);
> > } else {
> > ok = vhost_vdpa_svqs_stop(dev);
> > if (unlikely(!ok)) {
> > @@ -1111,16 +1115,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > }
> >
> > if (started) {
> > + int r;
> > +
> > memory_listener_register(&v->listener, &address_space_memory);
> > - return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > + r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > + if (unlikely(r)) {
> > + return r;
> > + }
> > + vhost_vdpa_set_vring_ready(dev);
> > } else {
> > vhost_vdpa_reset_device(dev);
> > vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> > VIRTIO_CONFIG_S_DRIVER);
> > memory_listener_unregister(&v->listener);
> > -
> > - return 0;
> > }
> > +
> > + return 0;
> > }
> >
> > static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> > --
> > 2.31.1
>
On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>
> To restore the device in the destination of a live migration we send the
> commands through control virtqueue. For a device to read CVQ it must
> have received DRIVER_OK status bit.
>
> However this open a window where the device could start receiving
> packets in rx queue 0 before it receive the RSS configuration. To avoid
> that, we will not send vring_enable until all configuration is used by
> the device.
>
> As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
I wonder if it's better to delay this to the series that implements
migration since the shadow cvq doesn't depends on this?
>
> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> ---
> hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 66f054a12c..2ee8009594 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -728,13 +728,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> return idx;
> }
>
> +/**
> + * Set ready all vring of the device
> + *
> + * @dev: Vhost device
> + */
> static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> {
> int i;
> trace_vhost_vdpa_set_vring_ready(dev);
> - for (i = 0; i < dev->nvqs; ++i) {
> + for (i = 0; i < dev->vq_index_end; ++i) {
> struct vhost_vring_state state = {
> - .index = dev->vq_index + i,
> + .index = i,
Looks like a cleanup or bugfix which deserves a separate patch?
> .num = 1,
> };
> vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> @@ -1097,7 +1102,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> if (unlikely(!ok)) {
> return -1;
> }
> - vhost_vdpa_set_vring_ready(dev);
> } else {
> ok = vhost_vdpa_svqs_stop(dev);
> if (unlikely(!ok)) {
> @@ -1111,16 +1115,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> }
>
> if (started) {
> + int r;
> +
> memory_listener_register(&v->listener, &address_space_memory);
> - return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> + if (unlikely(r)) {
> + return r;
> + }
> + vhost_vdpa_set_vring_ready(dev);
Interesting, does this mean we only enable the last two queues without
this patch?
Thanks
> } else {
> vhost_vdpa_reset_device(dev);
> vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE |
> VIRTIO_CONFIG_S_DRIVER);
> memory_listener_unregister(&v->listener);
> -
> - return 0;
> }
> +
> + return 0;
> }
>
> static int vhost_vdpa_set_log_base(struct vhost_dev *dev, uint64_t base,
> --
> 2.31.1
>
On Fri, Jul 8, 2022 at 11:06 AM Jason Wang <jasowang@redhat.com> wrote: > > On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <eperezma@redhat.com> wrote: > > > > To restore the device in the destination of a live migration we send the > > commands through control virtqueue. For a device to read CVQ it must > > have received DRIVER_OK status bit. > > > > However this open a window where the device could start receiving > > packets in rx queue 0 before it receive the RSS configuration. To avoid > > that, we will not send vring_enable until all configuration is used by > > the device. > > > > As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps. > > I wonder if it's better to delay this to the series that implements > migration since the shadow cvq doesn't depends on this? > (Forgot to add) this series is already capable of doing migration with CVQ. It's just that it must use SVQ from the moment the source VM boots up, which is far from ideal. Thanks!
On Fri, Jul 8, 2022 at 11:06 AM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jul 7, 2022 at 2:40 AM Eugenio Pérez <eperezma@redhat.com> wrote:
> >
> > To restore the device in the destination of a live migration we send the
> > commands through control virtqueue. For a device to read CVQ it must
> > have received DRIVER_OK status bit.
> >
> > However this open a window where the device could start receiving
> > packets in rx queue 0 before it receive the RSS configuration. To avoid
> > that, we will not send vring_enable until all configuration is used by
> > the device.
> >
> > As a first step, reverse the DRIVER_OK and SET_VRING_ENABLE steps.
>
> I wonder if it's better to delay this to the series that implements
> migration since the shadow cvq doesn't depends on this?
>
> >
> > Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> > ---
> > hw/virtio/vhost-vdpa.c | 22 ++++++++++++++++------
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 66f054a12c..2ee8009594 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -728,13 +728,18 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx)
> > return idx;
> > }
> >
> > +/**
> > + * Set ready all vring of the device
> > + *
> > + * @dev: Vhost device
> > + */
> > static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev)
> > {
> > int i;
> > trace_vhost_vdpa_set_vring_ready(dev);
> > - for (i = 0; i < dev->nvqs; ++i) {
> > + for (i = 0; i < dev->vq_index_end; ++i) {
> > struct vhost_vring_state state = {
> > - .index = dev->vq_index + i,
> > + .index = i,
>
> Looks like a cleanup or bugfix which deserves a separate patch?
>
> > .num = 1,
> > };
> > vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
> > @@ -1097,7 +1102,6 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > if (unlikely(!ok)) {
> > return -1;
> > }
> > - vhost_vdpa_set_vring_ready(dev);
> > } else {
> > ok = vhost_vdpa_svqs_stop(dev);
> > if (unlikely(!ok)) {
> > @@ -1111,16 +1115,22 @@ static int vhost_vdpa_dev_start(struct vhost_dev *dev, bool started)
> > }
> >
> > if (started) {
> > + int r;
> > +
> > memory_listener_register(&v->listener, &address_space_memory);
> > - return vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > + r = vhost_vdpa_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> > + if (unlikely(r)) {
> > + return r;
> > + }
> > + vhost_vdpa_set_vring_ready(dev);
>
> Interesting, does this mean we only enable the last two queues without
> this patch?
>
The function vhost_vdpa_set_vring_ready is changed in this patch.
Instead of enabling only the vrings of the device, it enables all the
vrings of the device, from 0 to dev->vq_index_end.
In the case of networking, vq_index_end changes depending on how CVQ
and MQ are negotiated or not, so we should be safe here.
Based on your comments it's clear that this is an unexpected change
and I need to add that description to the patch message :).
Thanks!
© 2016 - 2026 Red Hat, Inc.