Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 7, 2024 at 11:18 AM Kevin Wolf wrote:
>
> Am 06.02.2024 um 17:44 hat Eugenio Perez Martin geschrieben:
> > On Mon, Feb 5, 2024 at 2:49 PM Kevin Wolf wrote:
> > >
> > > Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> > > > On Fri, Feb 2, 2024 at 2:25 PM 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.
> > > > >
> > > > > Cc: qemu-sta...@nongnu.org
> > > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > > > > Signed-off-by: Kevin Wolf
> > > > > ---
> > > > > hw/virtio/vdpa-dev.c | 5 +
> > > > > hw/virtio/vhost-vdpa.c | 17 +
> > > > > 2 files changed, 18 insertions(+), 4 deletions(-)
> > > > >
> > > > > 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(>dev, vdev, false);
> > > > > +ret = vhost_dev_start(>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(>vdpa, i);
> > > > > -}
> > > > > s->started = true;
> > > > >
> > > > > /*
> > > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > > index 3a43beb312..c4574d56c5 100644
> > > > > --- a/hw/virtio/vhost-vdpa.c
> > > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa
> > > > > *v, unsigned idx)
> > > > > 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_ready(v, i);
> > > > > +if (ret < 0) {
> > > > > +return ret;
> > > > > +}
> > > > > +}
> > > > > +
> > > > > +return 0;
> > > > > +}
> > > > > +
> > > > > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > > > > int fd)
> > > > > {
> > > > > @@ -1524,6 +1540,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,
> > > >
> > > > vhost-vdpa net enables CVQ before dataplane ones to configure all the
> > > > device in the destination of a live migration. To go back again to
> > > > this callback would cause the device to enable the dataplane before
> > > > virtqueues are configured again.
> > >
> > > Not that it makes a difference, but I don't think it would actually be
> > > going back. Even before your commit 6c482547, we were not making use of
> > > the generic callback but just called the function in a slightly
> > > different place (but less different than after commit 6c482547).
> > >
> > > But anyway... Why don't the other vhost backend need the same for
> > > vhost-net then? Do they just not support live migration?
> >
> > They don't support control virtqueue. More specifically, control
> > virtqueue is handled directly in QEMU.
>
> So the network device already has to special case vdpa instead of using
> the same code for all vhost backends? :-/
>
> > > I don't know the code well enough to say where the problem is, but if
> > > vhost-vdpa networking code relies on the usual vhost operations not
> > > being implemented and bypasses VhostOps to replace it,
Re: Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 7, 2024 at 4:47 PM Stefano Garzarella wrote: > > On Wed, Feb 07, 2024 at 11:17:34AM +0800, Jason Wang wrote: > >On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella > >wrote: > >> > >> On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote: > >> >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella > >> >wrote: > >> >> > >> >> On Fri, Feb 02, 2024 at 02:25:21PM +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. > >> > > >> >Did this mean virtio-blk will enable a virtqueue after DRIVER_OK? > >> > >> It's not specific to virtio-blk, but to the generic vdpa device we have > >> in QEMU (i.e. vhost-vdpa-device). Yep, after commit > >> 6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after > >> DRIVER_OK. > > > >Right. > > > >> > >> >Sepc is not clear about this and that's why we introduce > >> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK. > >> > >> Ah, I didn't know about this new feature. So after commit > >> 6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not > >> complying with the specification, right? > > > >Kind of, but as stated, it's just because spec is unclear about the > >behaviour. There's a chance that spec will explicitly support it in > >the future. > > > >> > >> > > >> >> > >> >> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message, > >> > > >> >I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is > >> >negotiated. > >> > >> At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not > >> negotiated, should we return an error in vhost-vdpa kernel module if > >> VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set? > > > >I'm not sure if this can break some setups or not. It might be better > >to leave it as is? > > As I also answered in the kernel patch, IMHO we have to return an error, > because any setups are broken anyway (see the problem we're fixing with is > this patch), For VDUSE probably yes, but not for other parents. It's easy to mandate on day 0 but might be hard for now. For mlx5e, it supports the semantic VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK even before the feature bit is introduced. And we don't do other checks like for example setting vq address, size after DRIVER_OK. We can hear from others. > so at this point it's better to return an error, so they > don't spend time figuring out why the VDUSE device doesn't work. > > > > >Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if > >parent support vq_ready after driver_ok. > > So we have to assume that it doesn't support it, to be more > conservative, right? > > >With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support > >vq_ready after driver_ok. > > > >> > >> >If this is truth, it seems a little more complicated, for > >> >example the get_backend_features needs to be forward to the userspace? > >> > >> I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES > >> for this? Or do you mean userspace on the VDUSE side? > > > >Yes, since in this case the parent is in the userspace, there's no way > >for VDUSE to know if user space supports vq_ready after driver_ok or > >not. > > > >As you may have noticed, we don't have a message for vq_ready which > >implies that vq_ready after driver_ok can't be supported. > > Yep, I see. > > > > >> > >> >This seems suboptimal to implement this in the spec first and then we > >> >can leverage the features. Or we can have another parameter for the > >> >ioctl that creates the vduse device. > >> > >> I got a little lost, though in vhost-user, the device can always expect > >> a vring_enable/disable, so I thought it was not complicated in VDUSE. > > > >Yes, the problem is assuming we have a message for vq_ready, there > >could be a "legacy" userspace that doesn't support that. So in that > >case, VDUSE needs to know if the userspace parent can support that or > >not. > > I think VDUSE needs to negotiate features (if it doesn't already) as > vhost-user does with the backend. Also for new future functionality. It negotiates virtio features but not vhost backend features. Thanks
Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
Jason Wang 于2024年2月7日周三 11:17写道：
>
> On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella wrote:
> >
> > On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
> > >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella
> > >wrote:
> > >>
> > >> On Fri, Feb 02, 2024 at 02:25:21PM +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.
> > >
> > >Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
> >
> > It's not specific to virtio-blk, but to the generic vdpa device we have
> > in QEMU (i.e. vhost-vdpa-device). Yep, after commit
> > 6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
> > DRIVER_OK.
>
> Right.
>
> >
> > >Sepc is not clear about this and that's why we introduce
> > >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> >
> > Ah, I didn't know about this new feature. So after commit
> > 6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
> > complying with the specification, right?
>
> Kind of, but as stated, it's just because spec is unclear about the
> behaviour. There's a chance that spec will explicitly support it in
> the future.
>
> >
> > >
> > >>
> > >> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
> > >
> > >I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
> > >negotiated.
> >
> > At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
> > negotiated, should we return an error in vhost-vdpa kernel module if
> > VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
>
> I'm not sure if this can break some setups or not. It might be better
> to leave it as is?
>
> Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if
> parent support vq_ready after driver_ok.
> With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support
> vq_ready after driver_ok.
>
> >
> > >If this is truth, it seems a little more complicated, for
> > >example the get_backend_features needs to be forward to the userspace?
> >
> > I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
> > for this? Or do you mean userspace on the VDUSE side?
>
> Yes, since in this case the parent is in the userspace, there's no way
> for VDUSE to know if user space supports vq_ready after driver_ok or
> not.
>
> As you may have noticed, we don't have a message for vq_ready which
> implies that vq_ready after driver_ok can't be supported.
>
> >
> > >This seems suboptimal to implement this in the spec first and then we
> > >can leverage the features. Or we can have another parameter for the
> > >ioctl that creates the vduse device.
> >
> > I got a little lost, though in vhost-user, the device can always expect
> > a vring_enable/disable, so I thought it was not complicated in VDUSE.
>
> Yes, the problem is assuming we have a message for vq_ready, there
> could be a "legacy" userspace that doesn't support that. So in that
> case, VDUSE needs to know if the userspace parent can support that or
> not.
>
> >
> > >
> > >> I'll start another thread about that, but in the meantime I agree that
> > >> we should fix QEMU since we need to work properly with old kernels as
> > >> well.
> > >>
> > >> >
> > >> >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.
> > >>
> > >> I like this approach and the patch LGTM, but I'm a bit worried about
> > >> this function in hw/net/vhost_net.c:
> > >>
> > >> int vhost_set_vring_enable(NetClientState *nc, int enable)
> > >> {
> > >> VHostNetState *net = get_vhost_net(nc);
> > >> const VhostOps *vhost_ops = net->dev.vhost_ops;
> > >>
> > >> nc->vring_enable = enable;
> > >>
> > >> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> > >> return vhost_ops->vhost_set_vring_enable(>dev, enable);
> > >> }
> > >>
> > >> return 0;
> > >> }
> > >>
> > >> @Eugenio, @Jason, should we change some things there if vhost-vdpa
> > >> implements the vhost_set_vring_enable callback?
> > >
> > >Eugenio may know more, I remember we need to enable cvq first for
> > >shadow virtqueue to restore some states.
> > >
> > >>
> > >> Do you remember why we didn't implement it from the beginning?
> > >
> > >It seems the vrings parameter is introduced after vhost-vdpa is
> > >implemented.
> >
> > Sorry, I mean why we didn't implement the
Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 07, 2024 at 11:18:54AM +0100, Kevin Wolf wrote:
Am 06.02.2024 um 17:44 hat Eugenio Perez Martin geschrieben:
On Mon, Feb 5, 2024 at 2:49 PM Kevin Wolf wrote:
>
> Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> > On Fri, Feb 2, 2024 at 2:25 PM 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.
> > >
> > > Cc: qemu-sta...@nongnu.org
> > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > > Signed-off-by: Kevin Wolf
> > > ---
> > > hw/virtio/vdpa-dev.c | 5 +
> > > hw/virtio/vhost-vdpa.c | 17 +
> > > 2 files changed, 18 insertions(+), 4 deletions(-)
> > >
> > > 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(>dev, vdev, false);
> > > +ret = vhost_dev_start(>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(>vdpa, i);
> > > -}
> > > s->started = true;
> > >
> > > /*
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 3a43beb312..c4574d56c5 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
unsigned idx)
> > > 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_ready(v, i);
> > > +if (ret < 0) {
> > > +return ret;
> > > +}
> > > +}
> > > +
> > > +return 0;
> > > +}
> > > +
> > > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > > int fd)
> > > {
> > > @@ -1524,6 +1540,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,
> >
> > vhost-vdpa net enables CVQ before dataplane ones to configure all the
> > device in the destination of a live migration. To go back again to
> > this callback would cause the device to enable the dataplane before
> > virtqueues are configured again.
>
> Not that it makes a difference, but I don't think it would actually be
> going back. Even before your commit 6c482547, we were not making use of
> the generic callback but just called the function in a slightly
> different place (but less different than after commit 6c482547).
>
> But anyway... Why don't the other vhost backend need the same for
> vhost-net then? Do they just not support live migration?
They don't support control virtqueue. More specifically, control
virtqueue is handled directly in QEMU.
So the network device already has to special case vdpa instead of using
the same code for all vhost backends? :-/
> I don't know the code well enough to say where the problem is, but if
> vhost-vdpa networking code relies on the usual vhost operations not
> being implemented and bypasses VhostOps to replace it, that sounds like
> a design problem to me.
I don't follow this. What vhost operation is expected not to be implemented?
You were concerned about implementing .vhost_set_vring_enable in
vdpa_ops like my patch does. So it seems that the networking code
requires that it is not implemented?
On the other hand, for vhost-vdpa, the callback seems to be called in
exactly the right place where virtqueues need to be enabled, like for
other vhost devices.
> Maybe
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
Am 06.02.2024 um 17:44 hat Eugenio Perez Martin geschrieben:
> On Mon, Feb 5, 2024 at 2:49 PM Kevin Wolf wrote:
> >
> > Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> > > On Fri, Feb 2, 2024 at 2:25 PM 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.
> > > >
> > > > Cc: qemu-sta...@nongnu.org
> > > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > > > Signed-off-by: Kevin Wolf
> > > > ---
> > > > hw/virtio/vdpa-dev.c | 5 +
> > > > hw/virtio/vhost-vdpa.c | 17 +
> > > > 2 files changed, 18 insertions(+), 4 deletions(-)
> > > >
> > > > 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(>dev, vdev, false);
> > > > +ret = vhost_dev_start(>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(>vdpa, i);
> > > > -}
> > > > s->started = true;
> > > >
> > > > /*
> > > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > > index 3a43beb312..c4574d56c5 100644
> > > > --- a/hw/virtio/vhost-vdpa.c
> > > > +++ b/hw/virtio/vhost-vdpa.c
> > > > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa
> > > > *v, unsigned idx)
> > > > 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_ready(v, i);
> > > > +if (ret < 0) {
> > > > +return ret;
> > > > +}
> > > > +}
> > > > +
> > > > +return 0;
> > > > +}
> > > > +
> > > > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > > > int fd)
> > > > {
> > > > @@ -1524,6 +1540,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,
> > >
> > > vhost-vdpa net enables CVQ before dataplane ones to configure all the
> > > device in the destination of a live migration. To go back again to
> > > this callback would cause the device to enable the dataplane before
> > > virtqueues are configured again.
> >
> > Not that it makes a difference, but I don't think it would actually be
> > going back. Even before your commit 6c482547, we were not making use of
> > the generic callback but just called the function in a slightly
> > different place (but less different than after commit 6c482547).
> >
> > But anyway... Why don't the other vhost backend need the same for
> > vhost-net then? Do they just not support live migration?
>
> They don't support control virtqueue. More specifically, control
> virtqueue is handled directly in QEMU.
So the network device already has to special case vdpa instead of using
the same code for all vhost backends? :-/
> > I don't know the code well enough to say where the problem is, but if
> > vhost-vdpa networking code relies on the usual vhost operations not
> > being implemented and bypasses VhostOps to replace it, that sounds like
> > a design problem to me.
>
> I don't follow this. What vhost operation is expected not to be implemented?
You were concerned about implementing .vhost_set_vring_enable in
vdpa_ops like my patch does. So it seems that the networking code
requires that it is not implemented?
On the
Re: Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 07, 2024 at 04:05:29PM +0800, Cindy Lu wrote:
Jason Wang 于2024年2月7日周三 11:17写道：
On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella wrote:
>
> On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
> >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella
wrote:
> >>
> >> On Fri, Feb 02, 2024 at 02:25:21PM +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.
> >
> >Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
>
> It's not specific to virtio-blk, but to the generic vdpa device we have
> in QEMU (i.e. vhost-vdpa-device). Yep, after commit
> 6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
> DRIVER_OK.
Right.
>
> >Sepc is not clear about this and that's why we introduce
> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
>
> Ah, I didn't know about this new feature. So after commit
> 6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
> complying with the specification, right?
Kind of, but as stated, it's just because spec is unclear about the
behaviour. There's a chance that spec will explicitly support it in
the future.
>
> >
> >>
> >> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
> >
> >I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
> >negotiated.
>
> At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
> negotiated, should we return an error in vhost-vdpa kernel module if
> VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
I'm not sure if this can break some setups or not. It might be better
to leave it as is?
Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if
parent support vq_ready after driver_ok.
With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support
vq_ready after driver_ok.
>
> >If this is truth, it seems a little more complicated, for
> >example the get_backend_features needs to be forward to the userspace?
>
> I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
> for this? Or do you mean userspace on the VDUSE side?
Yes, since in this case the parent is in the userspace, there's no way
for VDUSE to know if user space supports vq_ready after driver_ok or
not.
As you may have noticed, we don't have a message for vq_ready which
implies that vq_ready after driver_ok can't be supported.
>
> >This seems suboptimal to implement this in the spec first and then we
> >can leverage the features. Or we can have another parameter for the
> >ioctl that creates the vduse device.
>
> I got a little lost, though in vhost-user, the device can always expect
> a vring_enable/disable, so I thought it was not complicated in VDUSE.
Yes, the problem is assuming we have a message for vq_ready, there
could be a "legacy" userspace that doesn't support that. So in that
case, VDUSE needs to know if the userspace parent can support that or
not.
>
> >
> >> I'll start another thread about that, but in the meantime I agree that
> >> we should fix QEMU since we need to work properly with old kernels as
> >> well.
> >>
> >> >
> >> >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.
> >>
> >> I like this approach and the patch LGTM, but I'm a bit worried about
> >> this function in hw/net/vhost_net.c:
> >>
> >> int vhost_set_vring_enable(NetClientState *nc, int enable)
> >> {
> >> VHostNetState *net = get_vhost_net(nc);
> >> const VhostOps *vhost_ops = net->dev.vhost_ops;
> >>
> >> nc->vring_enable = enable;
> >>
> >> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> >> return vhost_ops->vhost_set_vring_enable(>dev, enable);
> >> }
> >>
> >> return 0;
> >> }
> >>
> >> @Eugenio, @Jason, should we change some things there if vhost-vdpa
> >> implements the vhost_set_vring_enable callback?
> >
> >Eugenio may know more, I remember we need to enable cvq first for
> >shadow virtqueue to restore some states.
> >
> >>
> >> Do you remember why we didn't implement it from the beginning?
> >
> >It seems the vrings parameter is introduced after vhost-vdpa is
> >implemented.
>
> Sorry, I mean why we didn't implement the vhost_set_vring_enable
> callback for vhost-vdpa from the beginning.
Adding Cindy who writes those codes for more thoughts.
it's a really long time ago, and I can't remember it clearly.
Re: Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Wed, Feb 07, 2024 at 11:17:34AM +0800, Jason Wang wrote:
On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella wrote:
On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
>On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella wrote:
>>
>> On Fri, Feb 02, 2024 at 02:25:21PM +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.
>
>Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
It's not specific to virtio-blk, but to the generic vdpa device we have
in QEMU (i.e. vhost-vdpa-device). Yep, after commit
6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
DRIVER_OK.
Right.
>Sepc is not clear about this and that's why we introduce
>VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
Ah, I didn't know about this new feature. So after commit
6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
complying with the specification, right?
Kind of, but as stated, it's just because spec is unclear about the
behaviour. There's a chance that spec will explicitly support it in
the future.
>
>>
>> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
>
>I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
>negotiated.
At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
negotiated, should we return an error in vhost-vdpa kernel module if
VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
I'm not sure if this can break some setups or not. It might be better
to leave it as is?
As I also answered in the kernel patch, IMHO we have to return an error,
because any setups are broken anyway (see the problem we're fixing with
this patch), so at this point it's better to return an error, so they
don't spend time figuring out why the VDUSE device doesn't work.
Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if
parent support vq_ready after driver_ok.
So we have to assume that it doesn't support it, to be more
conservative, right?
With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support
vq_ready after driver_ok.
>If this is truth, it seems a little more complicated, for
>example the get_backend_features needs to be forward to the userspace?
I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
for this? Or do you mean userspace on the VDUSE side?
Yes, since in this case the parent is in the userspace, there's no way
for VDUSE to know if user space supports vq_ready after driver_ok or
not.
As you may have noticed, we don't have a message for vq_ready which
implies that vq_ready after driver_ok can't be supported.
Yep, I see.
>This seems suboptimal to implement this in the spec first and then we
>can leverage the features. Or we can have another parameter for the
>ioctl that creates the vduse device.
I got a little lost, though in vhost-user, the device can always expect
a vring_enable/disable, so I thought it was not complicated in VDUSE.
Yes, the problem is assuming we have a message for vq_ready, there
could be a "legacy" userspace that doesn't support that. So in that
case, VDUSE needs to know if the userspace parent can support that or
not.
I think VDUSE needs to negotiate features (if it doesn't already) as
vhost-user does with the backend. Also for new future functionality.
>
>> I'll start another thread about that, but in the meantime I agree that
>> we should fix QEMU since we need to work properly with old kernels as
>> well.
>>
>> >
>> >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.
>>
>> I like this approach and the patch LGTM, but I'm a bit worried about
>> this function in hw/net/vhost_net.c:
>>
>> int vhost_set_vring_enable(NetClientState *nc, int enable)
>> {
>> VHostNetState *net = get_vhost_net(nc);
>> const VhostOps *vhost_ops = net->dev.vhost_ops;
>>
>> nc->vring_enable = enable;
>>
>> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
>> return vhost_ops->vhost_set_vring_enable(>dev, enable);
>> }
>>
>> return 0;
>> }
>>
>> @Eugenio, @Jason, should we change some things there if vhost-vdpa
>> implements the vhost_set_vring_enable callback?
>
>Eugenio may know more, I remember we need to enable cvq first for
>shadow virtqueue to restore some states.
>
>>
>> Do you remember why we didn't implement
Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Tue, Feb 6, 2024 at 4:31 PM Stefano Garzarella wrote:
>
> On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
> >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella
> >wrote:
> >>
> >> On Fri, Feb 02, 2024 at 02:25:21PM +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.
> >
> >Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
>
> It's not specific to virtio-blk, but to the generic vdpa device we have
> in QEMU (i.e. vhost-vdpa-device). Yep, after commit
> 6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
> DRIVER_OK.
Right.
>
> >Sepc is not clear about this and that's why we introduce
> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
>
> Ah, I didn't know about this new feature. So after commit
> 6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
> complying with the specification, right?
Kind of, but as stated, it's just because spec is unclear about the
behaviour. There's a chance that spec will explicitly support it in
the future.
>
> >
> >>
> >> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
> >
> >I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
> >negotiated.
>
> At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
> negotiated, should we return an error in vhost-vdpa kernel module if
> VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
I'm not sure if this can break some setups or not. It might be better
to leave it as is?
Without VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we don't know if
parent support vq_ready after driver_ok.
With VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK, we know parent support
vq_ready after driver_ok.
>
> >If this is truth, it seems a little more complicated, for
> >example the get_backend_features needs to be forward to the userspace?
>
> I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
> for this? Or do you mean userspace on the VDUSE side?
Yes, since in this case the parent is in the userspace, there's no way
for VDUSE to know if user space supports vq_ready after driver_ok or
not.
As you may have noticed, we don't have a message for vq_ready which
implies that vq_ready after driver_ok can't be supported.
>
> >This seems suboptimal to implement this in the spec first and then we
> >can leverage the features. Or we can have another parameter for the
> >ioctl that creates the vduse device.
>
> I got a little lost, though in vhost-user, the device can always expect
> a vring_enable/disable, so I thought it was not complicated in VDUSE.
Yes, the problem is assuming we have a message for vq_ready, there
could be a "legacy" userspace that doesn't support that. So in that
case, VDUSE needs to know if the userspace parent can support that or
not.
>
> >
> >> I'll start another thread about that, but in the meantime I agree that
> >> we should fix QEMU since we need to work properly with old kernels as
> >> well.
> >>
> >> >
> >> >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.
> >>
> >> I like this approach and the patch LGTM, but I'm a bit worried about
> >> this function in hw/net/vhost_net.c:
> >>
> >> int vhost_set_vring_enable(NetClientState *nc, int enable)
> >> {
> >> VHostNetState *net = get_vhost_net(nc);
> >> const VhostOps *vhost_ops = net->dev.vhost_ops;
> >>
> >> nc->vring_enable = enable;
> >>
> >> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> >> return vhost_ops->vhost_set_vring_enable(>dev, enable);
> >> }
> >>
> >> return 0;
> >> }
> >>
> >> @Eugenio, @Jason, should we change some things there if vhost-vdpa
> >> implements the vhost_set_vring_enable callback?
> >
> >Eugenio may know more, I remember we need to enable cvq first for
> >shadow virtqueue to restore some states.
> >
> >>
> >> Do you remember why we didn't implement it from the beginning?
> >
> >It seems the vrings parameter is introduced after vhost-vdpa is
> >implemented.
>
> Sorry, I mean why we didn't implement the vhost_set_vring_enable
> callback for vhost-vdpa from the beginning.
Adding Cindy who writes those codes for more thoughts.
Thanks
>
> Thanks,
> Stefano
>
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Mon, Feb 5, 2024 at 2:49 PM Kevin Wolf wrote:
>
> Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> > On Fri, Feb 2, 2024 at 2:25 PM 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.
> > >
> > > Cc: qemu-sta...@nongnu.org
> > > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > > Signed-off-by: Kevin Wolf
> > > ---
> > > hw/virtio/vdpa-dev.c | 5 +
> > > hw/virtio/vhost-vdpa.c | 17 +
> > > 2 files changed, 18 insertions(+), 4 deletions(-)
> > >
> > > 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(>dev, vdev, false);
> > > +ret = vhost_dev_start(>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(>vdpa, i);
> > > -}
> > > s->started = true;
> > >
> > > /*
> > > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > > index 3a43beb312..c4574d56c5 100644
> > > --- a/hw/virtio/vhost-vdpa.c
> > > +++ b/hw/virtio/vhost-vdpa.c
> > > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
> > > unsigned idx)
> > > 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_ready(v, i);
> > > +if (ret < 0) {
> > > +return ret;
> > > +}
> > > +}
> > > +
> > > +return 0;
> > > +}
> > > +
> > > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > > int fd)
> > > {
> > > @@ -1524,6 +1540,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,
> >
> > vhost-vdpa net enables CVQ before dataplane ones to configure all the
> > device in the destination of a live migration. To go back again to
> > this callback would cause the device to enable the dataplane before
> > virtqueues are configured again.
>
> Not that it makes a difference, but I don't think it would actually be
> going back. Even before your commit 6c482547, we were not making use of
> the generic callback but just called the function in a slightly
> different place (but less different than after commit 6c482547).
>
> But anyway... Why don't the other vhost backend need the same for
> vhost-net then? Do they just not support live migration?
>
They don't support control virtqueue. More specifically, control
virtqueue is handled directly in QEMU.
> I don't know the code well enough to say where the problem is, but if
> vhost-vdpa networking code relies on the usual vhost operations not
> being implemented and bypasses VhostOps to replace it, that sounds like
> a design problem to me.
I don't follow this. What vhost operation is expected not to be implemented?
> Maybe VhostOps needs a new operation to enable
> just a single virtqueue that can be used by the networking code instead?
>
> > How does VDUSE userspace knows how many queues should enable? Can't
> > the kernel perform the necessary actions after DRIVER_OK, like
> > configuring the kick etc?
>
> Not sure if I understand the question. The vdpa kernel interface always
> enables individual queues, so the VDUSE userspace will enable whatever
> queues it was asked to enable. The only restriction is that the queues
> need to be enabled before setting DRIVER_OK.
>
> The
Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Tue, Feb 6, 2024 at 9:31 AM Stefano Garzarella wrote:
>
> On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
> >On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella
> >wrote:
> >>
> >> On Fri, Feb 02, 2024 at 02:25:21PM +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.
> >
> >Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
>
> It's not specific to virtio-blk, but to the generic vdpa device we have
> in QEMU (i.e. vhost-vdpa-device). Yep, after commit
> 6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
> DRIVER_OK.
>
> >Sepc is not clear about this and that's why we introduce
> >VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
>
> Ah, I didn't know about this new feature. So after commit
> 6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
> complying with the specification, right?
>
> >
> >>
> >> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
> >
> >I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
> >negotiated.
>
> At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
> negotiated, should we return an error in vhost-vdpa kernel module if
> VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
I just sent a patch:
https://lore.kernel.org/virtualization/20240206145154.118044-1-sgarz...@redhat.com/T/#u
Then I discovered that we never check the return value of
vhost_vdpa_set_vring_ready() in QEMU.
I'll send a patch for that!
Stefano
>
> >If this is truth, it seems a little more complicated, for
> >example the get_backend_features needs to be forward to the userspace?
>
> I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
> for this? Or do you mean userspace on the VDUSE side?
>
> >This seems suboptimal to implement this in the spec first and then we
> >can leverage the features. Or we can have another parameter for the
> >ioctl that creates the vduse device.
>
> I got a little lost, though in vhost-user, the device can always expect
> a vring_enable/disable, so I thought it was not complicated in VDUSE.
>
> >
> >> I'll start another thread about that, but in the meantime I agree that
> >> we should fix QEMU since we need to work properly with old kernels as
> >> well.
> >>
> >> >
> >> >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.
> >>
> >> I like this approach and the patch LGTM, but I'm a bit worried about
> >> this function in hw/net/vhost_net.c:
> >>
> >> int vhost_set_vring_enable(NetClientState *nc, int enable)
> >> {
> >> VHostNetState *net = get_vhost_net(nc);
> >> const VhostOps *vhost_ops = net->dev.vhost_ops;
> >>
> >> nc->vring_enable = enable;
> >>
> >> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> >> return vhost_ops->vhost_set_vring_enable(>dev, enable);
> >> }
> >>
> >> return 0;
> >> }
> >>
> >> @Eugenio, @Jason, should we change some things there if vhost-vdpa
> >> implements the vhost_set_vring_enable callback?
> >
> >Eugenio may know more, I remember we need to enable cvq first for
> >shadow virtqueue to restore some states.
> >
> >>
> >> Do you remember why we didn't implement it from the beginning?
> >
> >It seems the vrings parameter is introduced after vhost-vdpa is
> >implemented.
>
> Sorry, I mean why we didn't implement the vhost_set_vring_enable
> callback for vhost-vdpa from the beginning.
>
> Thanks,
> Stefano
Re: Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Tue, Feb 06, 2024 at 10:47:40AM +0800, Jason Wang wrote:
On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella wrote:
On Fri, Feb 02, 2024 at 02:25:21PM +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.
Did this mean virtio-blk will enable a virtqueue after DRIVER_OK?
It's not specific to virtio-blk, but to the generic vdpa device we have
in QEMU (i.e. vhost-vdpa-device). Yep, after commit
6c4825476a4351530bcac17abab72295b75ffe98, virtqueues are enabled after
DRIVER_OK.
Sepc is not clear about this and that's why we introduce
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
Ah, I didn't know about this new feature. So after commit
6c4825476a4351530bcac17abab72295b75ffe98 the vhost-vdpa-device is not
complying with the specification, right?
Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
negotiated.
At this point yes. But if VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is not
negotiated, should we return an error in vhost-vdpa kernel module if
VHOST_VDPA_SET_VRING_ENABLE is called when DRIVER_OK is already set?
If this is truth, it seems a little more complicated, for
example the get_backend_features needs to be forward to the userspace?
I'm not understanding, don't we already have VHOST_GET_BACKEND_FEATURES
for this? Or do you mean userspace on the VDUSE side?
This seems suboptimal to implement this in the spec first and then we
can leverage the features. Or we can have another parameter for the
ioctl that creates the vduse device.
I got a little lost, though in vhost-user, the device can always expect
a vring_enable/disable, so I thought it was not complicated in VDUSE.
I'll start another thread about that, but in the meantime I agree that
we should fix QEMU since we need to work properly with old kernels as
well.
>
>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.
I like this approach and the patch LGTM, but I'm a bit worried about
this function in hw/net/vhost_net.c:
int vhost_set_vring_enable(NetClientState *nc, int enable)
{
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
return vhost_ops->vhost_set_vring_enable(>dev, enable);
}
return 0;
}
@Eugenio, @Jason, should we change some things there if vhost-vdpa
implements the vhost_set_vring_enable callback?
Eugenio may know more, I remember we need to enable cvq first for
shadow virtqueue to restore some states.
Do you remember why we didn't implement it from the beginning?
It seems the vrings parameter is introduced after vhost-vdpa is
implemented.
Sorry, I mean why we didn't implement the vhost_set_vring_enable
callback for vhost-vdpa from the beginning.
Thanks,
Stefano
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Mon, Feb 5, 2024 at 6:51 PM Stefano Garzarella wrote:
>
> On Fri, Feb 02, 2024 at 02:25:21PM +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.
Did this mean virtio-blk will enable a virtqueue after DRIVER_OK? Sepc
is not clear about this and that's why we introduce
VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
>
> Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
I think you meant when VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK is
negotiated. If this is truth, it seems a little more complicated, for
example the get_backend_features needs to be forward to the userspace?
This seems suboptimal to implement this in the spec first and then we
can leverage the features. Or we can have another parameter for the
ioctl that creates the vduse device.
> I'll start another thread about that, but in the meantime I agree that
> we should fix QEMU since we need to work properly with old kernels as
> well.
>
> >
> >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.
>
> I like this approach and the patch LGTM, but I'm a bit worried about
> this function in hw/net/vhost_net.c:
>
> int vhost_set_vring_enable(NetClientState *nc, int enable)
> {
> VHostNetState *net = get_vhost_net(nc);
> const VhostOps *vhost_ops = net->dev.vhost_ops;
>
> nc->vring_enable = enable;
>
> if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
> return vhost_ops->vhost_set_vring_enable(>dev, enable);
> }
>
> return 0;
> }
>
> @Eugenio, @Jason, should we change some things there if vhost-vdpa
> implements the vhost_set_vring_enable callback?
Eugenio may know more, I remember we need to enable cvq first for
shadow virtqueue to restore some states.
>
> Do you remember why we didn't implement it from the beginning?
It seems the vrings parameter is introduced after vhost-vdpa is implemented.
Thanks
>
> Thanks,
> Stefano
>
> >
> >Cc: qemu-sta...@nongnu.org
> >Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> >Signed-off-by: Kevin Wolf
> >---
> > hw/virtio/vdpa-dev.c | 5 +
> > hw/virtio/vhost-vdpa.c | 17 +
> > 2 files changed, 18 insertions(+), 4 deletions(-)
> >
> >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(>dev, vdev, false);
> >+ret = vhost_dev_start(>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(>vdpa, i);
> >-}
> > s->started = true;
> >
> > /*
> >diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >index 3a43beb312..c4574d56c5 100644
> >--- a/hw/virtio/vhost-vdpa.c
> >+++ b/hw/virtio/vhost-vdpa.c
> >@@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
> >unsigned idx)
> > 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_ready(v, i);
> >+if (ret < 0) {
> >+return ret;
> >+}
> >+}
> >+
> >+return 0;
> >+}
> >+
> > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> >int fd)
> > {
> >@@ -1524,6 +1540,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,
> >--
> >2.43.0
> >
>
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
Am 05.02.2024 um 13:22 hat Eugenio Perez Martin geschrieben:
> On Fri, Feb 2, 2024 at 2:25 PM 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.
> >
> > Cc: qemu-sta...@nongnu.org
> > Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> > Signed-off-by: Kevin Wolf
> > ---
> > hw/virtio/vdpa-dev.c | 5 +
> > hw/virtio/vhost-vdpa.c | 17 +
> > 2 files changed, 18 insertions(+), 4 deletions(-)
> >
> > 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(>dev, vdev, false);
> > +ret = vhost_dev_start(>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(>vdpa, i);
> > -}
> > s->started = true;
> >
> > /*
> > diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> > index 3a43beb312..c4574d56c5 100644
> > --- a/hw/virtio/vhost-vdpa.c
> > +++ b/hw/virtio/vhost-vdpa.c
> > @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
> > unsigned idx)
> > 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_ready(v, i);
> > +if (ret < 0) {
> > +return ret;
> > +}
> > +}
> > +
> > +return 0;
> > +}
> > +
> > static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> > int fd)
> > {
> > @@ -1524,6 +1540,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,
>
> vhost-vdpa net enables CVQ before dataplane ones to configure all the
> device in the destination of a live migration. To go back again to
> this callback would cause the device to enable the dataplane before
> virtqueues are configured again.
Not that it makes a difference, but I don't think it would actually be
going back. Even before your commit 6c482547, we were not making use of
the generic callback but just called the function in a slightly
different place (but less different than after commit 6c482547).
But anyway... Why don't the other vhost backend need the same for
vhost-net then? Do they just not support live migration?
I don't know the code well enough to say where the problem is, but if
vhost-vdpa networking code relies on the usual vhost operations not
being implemented and bypasses VhostOps to replace it, that sounds like
a design problem to me. Maybe VhostOps needs a new operation to enable
just a single virtqueue that can be used by the networking code instead?
> How does VDUSE userspace knows how many queues should enable? Can't
> the kernel perform the necessary actions after DRIVER_OK, like
> configuring the kick etc?
Not sure if I understand the question. The vdpa kernel interface always
enables individual queues, so the VDUSE userspace will enable whatever
queues it was asked to enable. The only restriction is that the queues
need to be enabled before setting DRIVER_OK.
The interface that enables all virtqueues at once seems to be just
.vhost_set_vring_enable in QEMU.
Kevin
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
Am 02.02.2024 um 14:25 hat Kevin Wolf geschrieben:
> 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.
>
> Cc: qemu-sta...@nongnu.org
> Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> Signed-off-by: Kevin Wolf
> ---
> hw/virtio/vdpa-dev.c | 5 +
> hw/virtio/vhost-vdpa.c | 17 +
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> 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(>dev, vdev, false);
> +ret = vhost_dev_start(>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(>vdpa, i);
> -}
> s->started = true;
>
> /*
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 3a43beb312..c4574d56c5 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
> unsigned idx)
> 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_ready(v, i);
> +if (ret < 0) {
> +return ret;
> +}
> +}
> +
> +return 0;
> +}
Oops, this forgets to actually use the @enable parameter, and always
enables the queue even if the caller wanted to disable it.
I'll fix this in a v2, but I'd first like to see if something has to
change to address Stefano's concern, too.
Kevin
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Fri, Feb 2, 2024 at 2:25 PM 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.
>
> Cc: qemu-sta...@nongnu.org
> Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
> Signed-off-by: Kevin Wolf
> ---
> hw/virtio/vdpa-dev.c | 5 +
> hw/virtio/vhost-vdpa.c | 17 +
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> 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(>dev, vdev, false);
> +ret = vhost_dev_start(>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(>vdpa, i);
> -}
> s->started = true;
>
> /*
> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> index 3a43beb312..c4574d56c5 100644
> --- a/hw/virtio/vhost-vdpa.c
> +++ b/hw/virtio/vhost-vdpa.c
> @@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
> unsigned idx)
> 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_ready(v, i);
> +if (ret < 0) {
> +return ret;
> +}
> +}
> +
> +return 0;
> +}
> +
> static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
> int fd)
> {
> @@ -1524,6 +1540,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,
> --
> 2.43.0
>
vhost-vdpa net enables CVQ before dataplane ones to configure all the
device in the destination of a live migration. To go back again to
this callback would cause the device to enable the dataplane before
virtqueues are configured again.
How does VDUSE userspace knows how many queues should enable? Can't
the kernel perform the necessary actions after DRIVER_OK, like
configuring the kick etc?
Thanks!
Re: [PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
On Fri, Feb 02, 2024 at 02:25:21PM +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.
Yeah, IMHO the VDUSE protocol is missing a VDUSE_SET_VQ_READY message,
I'll start another thread about that, but in the meantime I agree that
we should fix QEMU since we need to work properly with old kernels as
well.
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.
I like this approach and the patch LGTM, but I'm a bit worried about
this function in hw/net/vhost_net.c:
int vhost_set_vring_enable(NetClientState *nc, int enable)
{
VHostNetState *net = get_vhost_net(nc);
const VhostOps *vhost_ops = net->dev.vhost_ops;
nc->vring_enable = enable;
if (vhost_ops && vhost_ops->vhost_set_vring_enable) {
return vhost_ops->vhost_set_vring_enable(>dev, enable);
}
return 0;
}
@Eugenio, @Jason, should we change some things there if vhost-vdpa
implements the vhost_set_vring_enable callback?
Do you remember why we didn't implement it from the beginning?
Thanks,
Stefano
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 17 +
2 files changed, 18 insertions(+), 4 deletions(-)
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(>dev, vdev, false);
+ret = vhost_dev_start(>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(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3a43beb312..c4574d56c5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
unsigned idx)
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_ready(v, i);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1524,6 +1540,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,
--
2.43.0
[PATCH] vdpa-dev: Fix initialisation order to restore VDUSE compatibility
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.
Cc: qemu-sta...@nongnu.org
Fixes: 6c4825476a4351530bcac17abab72295b75ffe98
Signed-off-by: Kevin Wolf
---
hw/virtio/vdpa-dev.c | 5 +
hw/virtio/vhost-vdpa.c | 17 +
2 files changed, 18 insertions(+), 4 deletions(-)
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(>dev, vdev, false);
+ret = vhost_dev_start(>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(>vdpa, i);
-}
s->started = true;
/*
diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
index 3a43beb312..c4574d56c5 100644
--- a/hw/virtio/vhost-vdpa.c
+++ b/hw/virtio/vhost-vdpa.c
@@ -904,6 +904,22 @@ int vhost_vdpa_set_vring_ready(struct vhost_vdpa *v,
unsigned idx)
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_ready(v, i);
+if (ret < 0) {
+return ret;
+}
+}
+
+return 0;
+}
+
static int vhost_vdpa_set_config_call(struct vhost_dev *dev,
int fd)
{
@@ -1524,6 +1540,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,
--
2.43.0
