[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [RFC PATCH v9 03/23] vdpa: delay set_vring_ready after DRIVER_OK
From: |
Eugenio Perez Martin |
Subject: |
Re: [RFC PATCH v9 03/23] vdpa: delay set_vring_ready after DRIVER_OK |
Date: |
Wed, 13 Jul 2022 08:18:13 +0200 |
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
>
[RFC PATCH v9 12/23] vhost: Add opaque member to SVQElement, Eugenio Pérez, 2022/07/06