| Message ID | 20240315155949.86066-1-kwolf@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [for-9.0,v3] vdpa-dev: Fix initialisation order to restore VDUSE compatibility | expand |
On Fri, Mar 15, 2024 at 04:59:49PM +0100, Kevin Wolf wrote: >VDUSE requires that virtqueues are first enabled before the DRIVER_OK >status flag is set; with the current API of the kernel module, it is >impossible to enable the opposite order in our block export code because >userspace is not notified when a virtqueue is enabled. > >This requirement also mathces the normal initialisation order as done by >the generic vhost code in QEMU. However, commit 6c482547 accidentally >changed the order for vdpa-dev and broke access to VDUSE devices with >this. > >This changes vdpa-dev to use the normal order again and use the standard >vhost callback .vhost_set_vring_enable for this. VDUSE devices can be >used with vdpa-dev again after this fix. > >vhost_net intentionally avoided enabling the vrings for vdpa and does >this manually later while it does enable them for other vhost backends. >Reflect this in the vhost_net code and return early for vdpa, so that >the behaviour doesn't change for this device. > >Cc: qemu-stable@nongnu.org >Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 >Signed-off-by: Kevin Wolf <kwolf@redhat.com> >--- >v2: >- Actually make use of the @enable parameter >- Change vhost_net to preserve the current behaviour > >v3: >- Updated trace point [Stefano] >- Fixed typo in comment [Stefano] > > hw/net/vhost_net.c | 10 ++++++++++ > hw/virtio/vdpa-dev.c | 5 +---- > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > hw/virtio/vhost.c | 8 +++++++- > hw/virtio/trace-events | 2 +- > 5 files changed, 45 insertions(+), 9 deletions(-) LGTM! Reviewed-by: Stefano Garzarella <sgarzare@redhat.com> Thanks, Stefano > >diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c >index e8e1661646..fd1a93701a 100644 >--- a/hw/net/vhost_net.c >+++ b/hw/net/vhost_net.c >@@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > VHostNetState *net = get_vhost_net(nc); > const VhostOps *vhost_ops = net->dev.vhost_ops; > >+ /* >+ * vhost-vdpa network devices need to enable dataplane virtqueues after >+ * DRIVER_OK, so they can recover device state before starting dataplane. >+ * Because of that, we don't enable virtqueues here and leave it to >+ * net/vhost-vdpa.c. >+ */ >+ if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { >+ return 0; >+ } >+ > nc->vring_enable = enable; > > if (vhost_ops && vhost_ops->vhost_set_vring_enable) { >diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c >index eb9ecea83b..13e87f06f6 100644 >--- a/hw/virtio/vdpa-dev.c >+++ b/hw/virtio/vdpa-dev.c >@@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp) > > s->dev.acked_features = vdev->guest_features; > >- ret = vhost_dev_start(&s->dev, vdev, false); >+ ret = vhost_dev_start(&s->dev, vdev, true); > if (ret < 0) { > error_setg_errno(errp, -ret, "Error starting vhost"); > goto err_guest_notifiers; > } >- for (i = 0; i < s->dev.nvqs; ++i) { >- vhost_vdpa_set_vring_ready(&s->vdpa, i); >- } > s->started = true; > > /* >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c >index ddae494ca8..31453466af 100644 >--- a/hw/virtio/vhost-vdpa.c >+++ b/hw/virtio/vhost-vdpa.c >@@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx) > return idx; > } > >-int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) >+static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx, >+ int enable) > { > struct vhost_dev *dev = v->dev; > struct vhost_vring_state state = { > .index = idx, >- .num = 1, >+ .num = enable, > }; > int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state); > >- trace_vhost_vdpa_set_vring_ready(dev, idx, r); >+ trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r); > return r; > } > >+static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable) >+{ >+ struct vhost_vdpa *v = dev->opaque; >+ unsigned int i; >+ int ret; >+ >+ for (i = 0; i < dev->nvqs; ++i) { >+ ret = vhost_vdpa_set_vring_enable_one(v, i, enable); >+ if (ret < 0) { >+ return ret; >+ } >+ } >+ >+ return 0; >+} >+ >+int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) >+{ >+ return vhost_vdpa_set_vring_enable_one(v, idx, 1); >+} >+ > static int vhost_vdpa_set_config_call(struct vhost_dev *dev, > int fd) > { >@@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = { > .vhost_set_features = vhost_vdpa_set_features, > .vhost_reset_device = vhost_vdpa_reset_device, > .vhost_get_vq_index = vhost_vdpa_get_vq_index, >+ .vhost_set_vring_enable = vhost_vdpa_set_vring_enable, > .vhost_get_config = vhost_vdpa_get_config, > .vhost_set_config = vhost_vdpa_set_config, > .vhost_requires_shm_log = NULL, >diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c >index 2c9ac79468..0000a66186 100644 >--- a/hw/virtio/vhost.c >+++ b/hw/virtio/vhost.c >@@ -1984,7 +1984,13 @@ static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable) > return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable); > } > >-/* Host notifiers must be enabled at this point. */ >+/* >+ * Host notifiers must be enabled at this point. >+ * >+ * If @vrings is true, this function will enable all vrings before starting the >+ * device. If it is false, the vring initialization is left to be done by the >+ * caller. >+ */ > int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > { > int i, r; >diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events >index 77905d1994..3f18536868 100644 >--- a/hw/virtio/trace-events >+++ b/hw/virtio/trace-events >@@ -48,7 +48,7 @@ vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRI > vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32 > vhost_vdpa_reset_device(void *dev) "dev: %p" > vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d" >-vhost_vdpa_set_vring_ready(void *dev, unsigned i, int r) "dev: %p, idx: %u, r: %d" >+vhost_vdpa_set_vring_enable_one(void *dev, unsigned i, int enable, int r) "dev: %p, idx: %u, enable: %u, r: %d" > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" > vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 > vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 >-- >2.44.0 >
On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf <kwolf@redhat.com> wrote: > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > status flag is set; with the current API of the kernel module, it is > impossible to enable the opposite order in our block export code because > userspace is not notified when a virtqueue is enabled. > > This requirement also mathces the normal initialisation order as done by > the generic vhost code in QEMU. However, commit 6c482547 accidentally > changed the order for vdpa-dev and broke access to VDUSE devices with > this. > > This changes vdpa-dev to use the normal order again and use the standard > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > used with vdpa-dev again after this fix. > > vhost_net intentionally avoided enabling the vrings for vdpa and does > this manually later while it does enable them for other vhost backends. > Reflect this in the vhost_net code and return early for vdpa, so that > the behaviour doesn't change for this device. > > Cc: qemu-stable@nongnu.org > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > v2: > - Actually make use of the @enable parameter > - Change vhost_net to preserve the current behaviour > > v3: > - Updated trace point [Stefano] > - Fixed typo in comment [Stefano] > > hw/net/vhost_net.c | 10 ++++++++++ > hw/virtio/vdpa-dev.c | 5 +---- > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > hw/virtio/vhost.c | 8 +++++++- > hw/virtio/trace-events | 2 +- > 5 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index e8e1661646..fd1a93701a 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > VHostNetState *net = get_vhost_net(nc); > const VhostOps *vhost_ops = net->dev.vhost_ops; > > + /* > + * vhost-vdpa network devices need to enable dataplane virtqueues after > + * DRIVER_OK, so they can recover device state before starting dataplane. > + * Because of that, we don't enable virtqueues here and leave it to > + * net/vhost-vdpa.c. > + */ > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > + return 0; > + } I think we need some inputs from Eugenio, this is only needed for shadow virtqueue during live migration but not other cases. Thanks
On Mon, Mar 18, 2024 at 12:31:26PM +0800, Jason Wang wrote: > On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > > status flag is set; with the current API of the kernel module, it is > > impossible to enable the opposite order in our block export code because > > userspace is not notified when a virtqueue is enabled. > > > > This requirement also mathces the normal initialisation order as done by > > the generic vhost code in QEMU. However, commit 6c482547 accidentally > > changed the order for vdpa-dev and broke access to VDUSE devices with > > this. > > > > This changes vdpa-dev to use the normal order again and use the standard > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > > used with vdpa-dev again after this fix. > > > > vhost_net intentionally avoided enabling the vrings for vdpa and does > > this manually later while it does enable them for other vhost backends. > > Reflect this in the vhost_net code and return early for vdpa, so that > > the behaviour doesn't change for this device. > > > > Cc: qemu-stable@nongnu.org > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > --- > > v2: > > - Actually make use of the @enable parameter > > - Change vhost_net to preserve the current behaviour > > > > v3: > > - Updated trace point [Stefano] > > - Fixed typo in comment [Stefano] > > > > hw/net/vhost_net.c | 10 ++++++++++ > > hw/virtio/vdpa-dev.c | 5 +---- > > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > > hw/virtio/vhost.c | 8 +++++++- > > hw/virtio/trace-events | 2 +- > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > index e8e1661646..fd1a93701a 100644 > > --- a/hw/net/vhost_net.c > > +++ b/hw/net/vhost_net.c > > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > > VHostNetState *net = get_vhost_net(nc); > > const VhostOps *vhost_ops = net->dev.vhost_ops; > > > > + /* > > + * vhost-vdpa network devices need to enable dataplane virtqueues after > > + * DRIVER_OK, so they can recover device state before starting dataplane. > > + * Because of that, we don't enable virtqueues here and leave it to > > + * net/vhost-vdpa.c. > > + */ > > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > > + return 0; > > + } > > I think we need some inputs from Eugenio, this is only needed for > shadow virtqueue during live migration but not other cases. > > Thanks Yes I think we had a backend flag for this, right? Eugenio can you comment please?
On Fri, Mar 15, 2024 at 04:59:49PM +0100, Kevin Wolf wrote: > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > status flag is set; with the current API of the kernel module, it is > impossible to enable the opposite order in our block export code because > userspace is not notified when a virtqueue is enabled. > > This requirement also mathces the normal initialisation order as done by > the generic vhost code in QEMU. However, commit 6c482547 accidentally > changed the order for vdpa-dev and broke access to VDUSE devices with > this. > > This changes vdpa-dev to use the normal order again and use the standard > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > used with vdpa-dev again after this fix. > > vhost_net intentionally avoided enabling the vrings for vdpa and does > this manually later while it does enable them for other vhost backends. > Reflect this in the vhost_net code and return early for vdpa, so that > the behaviour doesn't change for this device. > > Cc: qemu-stable@nongnu.org > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 Wrong format for the fixes tag. Fixes: 6c4825476a ("vdpa: move vhost_vdpa_set_vring_ready to the caller") is the right one. > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > v2: > - Actually make use of the @enable parameter > - Change vhost_net to preserve the current behaviour > > v3: > - Updated trace point [Stefano] > - Fixed typo in comment [Stefano] > > hw/net/vhost_net.c | 10 ++++++++++ > hw/virtio/vdpa-dev.c | 5 +---- > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > hw/virtio/vhost.c | 8 +++++++- > hw/virtio/trace-events | 2 +- > 5 files changed, 45 insertions(+), 9 deletions(-) > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > index e8e1661646..fd1a93701a 100644 > --- a/hw/net/vhost_net.c > +++ b/hw/net/vhost_net.c > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > VHostNetState *net = get_vhost_net(nc); > const VhostOps *vhost_ops = net->dev.vhost_ops; > > + /* > + * vhost-vdpa network devices need to enable dataplane virtqueues after > + * DRIVER_OK, so they can recover device state before starting dataplane. > + * Because of that, we don't enable virtqueues here and leave it to > + * net/vhost-vdpa.c. > + */ > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > + return 0; > + } > + > nc->vring_enable = enable; > > if (vhost_ops && vhost_ops->vhost_set_vring_enable) { > diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c > index eb9ecea83b..13e87f06f6 100644 > --- a/hw/virtio/vdpa-dev.c > +++ b/hw/virtio/vdpa-dev.c > @@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp) > > s->dev.acked_features = vdev->guest_features; > > - ret = vhost_dev_start(&s->dev, vdev, false); > + ret = vhost_dev_start(&s->dev, vdev, true); > if (ret < 0) { > error_setg_errno(errp, -ret, "Error starting vhost"); > goto err_guest_notifiers; > } > - for (i = 0; i < s->dev.nvqs; ++i) { > - vhost_vdpa_set_vring_ready(&s->vdpa, i); > - } > s->started = true; > > /* > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c > index ddae494ca8..31453466af 100644 > --- a/hw/virtio/vhost-vdpa.c > +++ b/hw/virtio/vhost-vdpa.c > @@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx) > return idx; > } > > -int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > +static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx, > + int enable) > { > struct vhost_dev *dev = v->dev; > struct vhost_vring_state state = { > .index = idx, > - .num = 1, > + .num = enable, > }; > int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state); > > - trace_vhost_vdpa_set_vring_ready(dev, idx, r); > + trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r); > return r; > } > > +static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable) > +{ > + struct vhost_vdpa *v = dev->opaque; > + unsigned int i; > + int ret; > + > + for (i = 0; i < dev->nvqs; ++i) { > + ret = vhost_vdpa_set_vring_enable_one(v, i, enable); > + if (ret < 0) { > + return ret; > + } > + } > + > + return 0; > +} > + > +int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) > +{ > + return vhost_vdpa_set_vring_enable_one(v, idx, 1); > +} > + > static int vhost_vdpa_set_config_call(struct vhost_dev *dev, > int fd) > { > @@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = { > .vhost_set_features = vhost_vdpa_set_features, > .vhost_reset_device = vhost_vdpa_reset_device, > .vhost_get_vq_index = vhost_vdpa_get_vq_index, > + .vhost_set_vring_enable = vhost_vdpa_set_vring_enable, > .vhost_get_config = vhost_vdpa_get_config, > .vhost_set_config = vhost_vdpa_set_config, > .vhost_requires_shm_log = NULL, > diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c > index 2c9ac79468..0000a66186 100644 > --- a/hw/virtio/vhost.c > +++ b/hw/virtio/vhost.c > @@ -1984,7 +1984,13 @@ static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable) > return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable); > } > > -/* Host notifiers must be enabled at this point. */ > +/* > + * Host notifiers must be enabled at this point. > + * > + * If @vrings is true, this function will enable all vrings before starting the > + * device. If it is false, the vring initialization is left to be done by the > + * caller. > + */ > int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) > { > int i, r; > diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events > index 77905d1994..3f18536868 100644 > --- a/hw/virtio/trace-events > +++ b/hw/virtio/trace-events > @@ -48,7 +48,7 @@ vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRI > vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32 > vhost_vdpa_reset_device(void *dev) "dev: %p" > vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d" > -vhost_vdpa_set_vring_ready(void *dev, unsigned i, int r) "dev: %p, idx: %u, r: %d" > +vhost_vdpa_set_vring_enable_one(void *dev, unsigned i, int enable, int r) "dev: %p, idx: %u, enable: %u, r: %d" > vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" > vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 > vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32 > -- > 2.44.0
On Mon, Mar 18, 2024 at 10:02 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Mar 18, 2024 at 12:31:26PM +0800, Jason Wang wrote: > > On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > > > status flag is set; with the current API of the kernel module, it is > > > impossible to enable the opposite order in our block export code because > > > userspace is not notified when a virtqueue is enabled. > > > > > > This requirement also mathces the normal initialisation order as done by > > > the generic vhost code in QEMU. However, commit 6c482547 accidentally > > > changed the order for vdpa-dev and broke access to VDUSE devices with > > > this. > > > > > > This changes vdpa-dev to use the normal order again and use the standard > > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > > > used with vdpa-dev again after this fix. > > > > > > vhost_net intentionally avoided enabling the vrings for vdpa and does > > > this manually later while it does enable them for other vhost backends. > > > Reflect this in the vhost_net code and return early for vdpa, so that > > > the behaviour doesn't change for this device. > > > > > > Cc: qemu-stable@nongnu.org > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > --- > > > v2: > > > - Actually make use of the @enable parameter > > > - Change vhost_net to preserve the current behaviour > > > > > > v3: > > > - Updated trace point [Stefano] > > > - Fixed typo in comment [Stefano] > > > > > > hw/net/vhost_net.c | 10 ++++++++++ > > > hw/virtio/vdpa-dev.c | 5 +---- > > > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > > > hw/virtio/vhost.c | 8 +++++++- > > > hw/virtio/trace-events | 2 +- > > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > > index e8e1661646..fd1a93701a 100644 > > > --- a/hw/net/vhost_net.c > > > +++ b/hw/net/vhost_net.c > > > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > > > VHostNetState *net = get_vhost_net(nc); > > > const VhostOps *vhost_ops = net->dev.vhost_ops; > > > > > > + /* > > > + * vhost-vdpa network devices need to enable dataplane virtqueues after > > > + * DRIVER_OK, so they can recover device state before starting dataplane. > > > + * Because of that, we don't enable virtqueues here and leave it to > > > + * net/vhost-vdpa.c. > > > + */ > > > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > > > + return 0; > > > + } > > > > I think we need some inputs from Eugenio, this is only needed for > > shadow virtqueue during live migration but not other cases. > > > > Thanks > > > Yes I think we had a backend flag for this, right? Eugenio can you > comment please? > We have the VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend flag, right. If the backend does not offer it, it is better to enable all the queues here and add a migration blocker in net/vhost-vdpa.c. So the check should be: nc->info->type == VHOST_VDPA && (backend_features & VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK). I can manage to add the migration blocker on top of this patch. Thanks!
Am 18.03.2024 um 20:27 hat Eugenio Perez Martin geschrieben: > On Mon, Mar 18, 2024 at 10:02 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Mar 18, 2024 at 12:31:26PM +0800, Jason Wang wrote: > > > On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > > > > status flag is set; with the current API of the kernel module, it is > > > > impossible to enable the opposite order in our block export code because > > > > userspace is not notified when a virtqueue is enabled. > > > > > > > > This requirement also mathces the normal initialisation order as done by > > > > the generic vhost code in QEMU. However, commit 6c482547 accidentally > > > > changed the order for vdpa-dev and broke access to VDUSE devices with > > > > this. > > > > > > > > This changes vdpa-dev to use the normal order again and use the standard > > > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > > > > used with vdpa-dev again after this fix. > > > > > > > > vhost_net intentionally avoided enabling the vrings for vdpa and does > > > > this manually later while it does enable them for other vhost backends. > > > > Reflect this in the vhost_net code and return early for vdpa, so that > > > > the behaviour doesn't change for this device. > > > > > > > > Cc: qemu-stable@nongnu.org > > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > --- > > > > v2: > > > > - Actually make use of the @enable parameter > > > > - Change vhost_net to preserve the current behaviour > > > > > > > > v3: > > > > - Updated trace point [Stefano] > > > > - Fixed typo in comment [Stefano] > > > > > > > > hw/net/vhost_net.c | 10 ++++++++++ > > > > hw/virtio/vdpa-dev.c | 5 +---- > > > > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > > > > hw/virtio/vhost.c | 8 +++++++- > > > > hw/virtio/trace-events | 2 +- > > > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > > > index e8e1661646..fd1a93701a 100644 > > > > --- a/hw/net/vhost_net.c > > > > +++ b/hw/net/vhost_net.c > > > > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > > > > VHostNetState *net = get_vhost_net(nc); > > > > const VhostOps *vhost_ops = net->dev.vhost_ops; > > > > > > > > + /* > > > > + * vhost-vdpa network devices need to enable dataplane virtqueues after > > > > + * DRIVER_OK, so they can recover device state before starting dataplane. > > > > + * Because of that, we don't enable virtqueues here and leave it to > > > > + * net/vhost-vdpa.c. > > > > + */ > > > > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > > > > + return 0; > > > > + } > > > > > > I think we need some inputs from Eugenio, this is only needed for > > > shadow virtqueue during live migration but not other cases. > > > > > > Thanks > > > > > > Yes I think we had a backend flag for this, right? Eugenio can you > > comment please? > > > > We have the VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend flag, > right. If the backend does not offer it, it is better to enable all > the queues here and add a migration blocker in net/vhost-vdpa.c. > > So the check should be: > nc->info->type == VHOST_VDPA && (backend_features & > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK). > > I can manage to add the migration blocker on top of this patch. Note that my patch preserves the current behaviour for vhost_net. The callback wasn't implemented for vdpa so far, so we never called anything even if the flag wasn't set. This patch adds an implementation for the callback, so we have to skip it here to have everything in vhost_net work as before - which is what the condition as written does. If we add a check for the flag now (I don't know if that's correct or not), that would be a second, unrelated change of behaviour in the same patch. So if it's necessary, that's a preexisting problem and I'd argue it doesn't belong in this patch, but should be done separately. Kevin
On Tue, Mar 19, 2024 at 11:00 AM Kevin Wolf <kwolf@redhat.com> wrote: > > Am 18.03.2024 um 20:27 hat Eugenio Perez Martin geschrieben: > > On Mon, Mar 18, 2024 at 10:02 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Mar 18, 2024 at 12:31:26PM +0800, Jason Wang wrote: > > > > On Fri, Mar 15, 2024 at 11:59 PM Kevin Wolf <kwolf@redhat.com> wrote: > > > > > > > > > > VDUSE requires that virtqueues are first enabled before the DRIVER_OK > > > > > status flag is set; with the current API of the kernel module, it is > > > > > impossible to enable the opposite order in our block export code because > > > > > userspace is not notified when a virtqueue is enabled. > > > > > > > > > > This requirement also mathces the normal initialisation order as done by > > > > > the generic vhost code in QEMU. However, commit 6c482547 accidentally > > > > > changed the order for vdpa-dev and broke access to VDUSE devices with > > > > > this. > > > > > > > > > > This changes vdpa-dev to use the normal order again and use the standard > > > > > vhost callback .vhost_set_vring_enable for this. VDUSE devices can be > > > > > used with vdpa-dev again after this fix. > > > > > > > > > > vhost_net intentionally avoided enabling the vrings for vdpa and does > > > > > this manually later while it does enable them for other vhost backends. > > > > > Reflect this in the vhost_net code and return early for vdpa, so that > > > > > the behaviour doesn't change for this device. > > > > > > > > > > Cc: qemu-stable@nongnu.org > > > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 > > > > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > > > > > --- > > > > > v2: > > > > > - Actually make use of the @enable parameter > > > > > - Change vhost_net to preserve the current behaviour > > > > > > > > > > v3: > > > > > - Updated trace point [Stefano] > > > > > - Fixed typo in comment [Stefano] > > > > > > > > > > hw/net/vhost_net.c | 10 ++++++++++ > > > > > hw/virtio/vdpa-dev.c | 5 +---- > > > > > hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- > > > > > hw/virtio/vhost.c | 8 +++++++- > > > > > hw/virtio/trace-events | 2 +- > > > > > 5 files changed, 45 insertions(+), 9 deletions(-) > > > > > > > > > > diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c > > > > > index e8e1661646..fd1a93701a 100644 > > > > > --- a/hw/net/vhost_net.c > > > > > +++ b/hw/net/vhost_net.c > > > > > @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) > > > > > VHostNetState *net = get_vhost_net(nc); > > > > > const VhostOps *vhost_ops = net->dev.vhost_ops; > > > > > > > > > > + /* > > > > > + * vhost-vdpa network devices need to enable dataplane virtqueues after > > > > > + * DRIVER_OK, so they can recover device state before starting dataplane. > > > > > + * Because of that, we don't enable virtqueues here and leave it to > > > > > + * net/vhost-vdpa.c. > > > > > + */ > > > > > + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { > > > > > + return 0; > > > > > + } > > > > > > > > I think we need some inputs from Eugenio, this is only needed for > > > > shadow virtqueue during live migration but not other cases. > > > > > > > > Thanks > > > > > > > > > Yes I think we had a backend flag for this, right? Eugenio can you > > > comment please? > > > > > > > We have the VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend flag, > > right. If the backend does not offer it, it is better to enable all > > the queues here and add a migration blocker in net/vhost-vdpa.c. > > > > So the check should be: > > nc->info->type == VHOST_VDPA && (backend_features & > > VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK). > > > > I can manage to add the migration blocker on top of this patch. > > Note that my patch preserves the current behaviour for vhost_net. The > callback wasn't implemented for vdpa so far, so we never called anything > even if the flag wasn't set. This patch adds an implementation for the > callback, so we have to skip it here to have everything in vhost_net > work as before - which is what the condition as written does. > > If we add a check for the flag now (I don't know if that's correct or > not), that would be a second, unrelated change of behaviour in the same > patch. So if it's necessary, that's a preexisting problem and I'd argue > it doesn't belong in this patch, but should be done separately. > Right, that's a very good point. I'll add proper checking on top of your patch when it is merged. Reviewed-by: Eugenio Pérez <eperezma@redhat.com> Thanks!
diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c index e8e1661646..fd1a93701a 100644 --- a/hw/net/vhost_net.c +++ b/hw/net/vhost_net.c @@ -541,6 +541,16 @@ int vhost_set_vring_enable(NetClientState *nc, int enable) VHostNetState *net = get_vhost_net(nc); const VhostOps *vhost_ops = net->dev.vhost_ops; + /* + * vhost-vdpa network devices need to enable dataplane virtqueues after + * DRIVER_OK, so they can recover device state before starting dataplane. + * Because of that, we don't enable virtqueues here and leave it to + * net/vhost-vdpa.c. + */ + if (nc->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) { + return 0; + } + nc->vring_enable = enable; if (vhost_ops && vhost_ops->vhost_set_vring_enable) { diff --git a/hw/virtio/vdpa-dev.c b/hw/virtio/vdpa-dev.c index eb9ecea83b..13e87f06f6 100644 --- a/hw/virtio/vdpa-dev.c +++ b/hw/virtio/vdpa-dev.c @@ -253,14 +253,11 @@ static int vhost_vdpa_device_start(VirtIODevice *vdev, Error **errp) s->dev.acked_features = vdev->guest_features; - ret = vhost_dev_start(&s->dev, vdev, false); + ret = vhost_dev_start(&s->dev, vdev, true); if (ret < 0) { error_setg_errno(errp, -ret, "Error starting vhost"); goto err_guest_notifiers; } - for (i = 0; i < s->dev.nvqs; ++i) { - vhost_vdpa_set_vring_ready(&s->vdpa, i); - } s->started = true; /* diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c index ddae494ca8..31453466af 100644 --- a/hw/virtio/vhost-vdpa.c +++ b/hw/virtio/vhost-vdpa.c @@ -886,19 +886,41 @@ static int vhost_vdpa_get_vq_index(struct vhost_dev *dev, int idx) return idx; } -int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) +static int vhost_vdpa_set_vring_enable_one(struct vhost_vdpa *v, unsigned idx, + int enable) { struct vhost_dev *dev = v->dev; struct vhost_vring_state state = { .index = idx, - .num = 1, + .num = enable, }; int r = vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state); - trace_vhost_vdpa_set_vring_ready(dev, idx, r); + trace_vhost_vdpa_set_vring_enable_one(dev, idx, enable, r); return r; } +static int vhost_vdpa_set_vring_enable(struct vhost_dev *dev, int enable) +{ + struct vhost_vdpa *v = dev->opaque; + unsigned int i; + int ret; + + for (i = 0; i < dev->nvqs; ++i) { + ret = vhost_vdpa_set_vring_enable_one(v, i, enable); + if (ret < 0) { + return ret; + } + } + + return 0; +} + +int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v, unsigned idx) +{ + return vhost_vdpa_set_vring_enable_one(v, idx, 1); +} + static int vhost_vdpa_set_config_call(struct vhost_dev *dev, int fd) { @@ -1514,6 +1536,7 @@ const VhostOps vdpa_ops = { .vhost_set_features = vhost_vdpa_set_features, .vhost_reset_device = vhost_vdpa_reset_device, .vhost_get_vq_index = vhost_vdpa_get_vq_index, + .vhost_set_vring_enable = vhost_vdpa_set_vring_enable, .vhost_get_config = vhost_vdpa_get_config, .vhost_set_config = vhost_vdpa_set_config, .vhost_requires_shm_log = NULL, diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c index 2c9ac79468..0000a66186 100644 --- a/hw/virtio/vhost.c +++ b/hw/virtio/vhost.c @@ -1984,7 +1984,13 @@ static int vhost_dev_set_vring_enable(struct vhost_dev *hdev, int enable) return hdev->vhost_ops->vhost_set_vring_enable(hdev, enable); } -/* Host notifiers must be enabled at this point. */ +/* + * Host notifiers must be enabled at this point. + * + * If @vrings is true, this function will enable all vrings before starting the + * device. If it is false, the vring initialization is left to be done by the + * caller. + */ int vhost_dev_start(struct vhost_dev *hdev, VirtIODevice *vdev, bool vrings) { int i, r; diff --git a/hw/virtio/trace-events b/hw/virtio/trace-events index 77905d1994..3f18536868 100644 --- a/hw/virtio/trace-events +++ b/hw/virtio/trace-events @@ -48,7 +48,7 @@ vhost_vdpa_set_features(void *dev, uint64_t features) "dev: %p features: 0x%"PRI vhost_vdpa_get_device_id(void *dev, uint32_t device_id) "dev: %p device_id %"PRIu32 vhost_vdpa_reset_device(void *dev) "dev: %p" vhost_vdpa_get_vq_index(void *dev, int idx, int vq_idx) "dev: %p idx: %d vq idx: %d" -vhost_vdpa_set_vring_ready(void *dev, unsigned i, int r) "dev: %p, idx: %u, r: %d" +vhost_vdpa_set_vring_enable_one(void *dev, unsigned i, int enable, int r) "dev: %p, idx: %u, enable: %u, r: %d" vhost_vdpa_dump_config(void *dev, const char *line) "dev: %p %s" vhost_vdpa_set_config(void *dev, uint32_t offset, uint32_t size, uint32_t flags) "dev: %p offset: %"PRIu32" size: %"PRIu32" flags: 0x%"PRIx32 vhost_vdpa_get_config(void *dev, void *config, uint32_t config_len) "dev: %p config: %p config_len: %"PRIu32
VDUSE requires that virtqueues are first enabled before the DRIVER_OK status flag is set; with the current API of the kernel module, it is impossible to enable the opposite order in our block export code because userspace is not notified when a virtqueue is enabled. This requirement also mathces the normal initialisation order as done by the generic vhost code in QEMU. However, commit 6c482547 accidentally changed the order for vdpa-dev and broke access to VDUSE devices with this. This changes vdpa-dev to use the normal order again and use the standard vhost callback .vhost_set_vring_enable for this. VDUSE devices can be used with vdpa-dev again after this fix. vhost_net intentionally avoided enabling the vrings for vdpa and does this manually later while it does enable them for other vhost backends. Reflect this in the vhost_net code and return early for vdpa, so that the behaviour doesn't change for this device. Cc: qemu-stable@nongnu.org Fixes: 6c4825476a4351530bcac17abab72295b75ffe98 Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- v2: - Actually make use of the @enable parameter - Change vhost_net to preserve the current behaviour v3: - Updated trace point [Stefano] - Fixed typo in comment [Stefano] hw/net/vhost_net.c | 10 ++++++++++ hw/virtio/vdpa-dev.c | 5 +---- hw/virtio/vhost-vdpa.c | 29 ++++++++++++++++++++++++++--- hw/virtio/vhost.c | 8 +++++++- hw/virtio/trace-events | 2 +- 5 files changed, 45 insertions(+), 9 deletions(-)