Re: [PATCH 08/16] vhost: add op to enable or disable a single vring

[Kangjie Xu]

在 2022/7/28 10:41, Jason Wang 写道:
> On Wed, Jul 27, 2022 at 3:05 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> > 在 2022/7/27 12:55, Jason Wang 写道:
> > > On Tue, Jul 26, 2022 at 2:39 PM Kangjie Xu <kangjie.xu@linux.alibaba.com> wrote:
> > > > 在 2022/7/26 11:49, Jason Wang 写道:
> > > > > 在 2022/7/18 19:17, Kangjie Xu 写道:
> > > > > > The interface to set enable status for a single vring is lacked in
> > > > > > VhostOps, since the vhost_set_vring_enable_op will manipulate all
> > > > > > virtqueues in a device.
> > > > > >
> > > > > > Resetting a single vq will rely on this interface. It requires a
> > > > > > reply to indicate that the reset operation is finished, so the
> > > > > > parameter, wait_for_reply, is added.
> > > > > The wait reply seems to be a implementation specific thing. Can we
> > > > > hide it?
> > > > >
> > > > > Thanks
> > > > I do not hide wait_for_reply here because when stopping the queue, a
> > > > reply is needed to ensure that the message has been processed and queue
> > > > has been disabled.
> > > This needs to be done at vhost-backend level instead of the general vhost code.
> > >
> > > E.g vhost-kernel or vDPA is using ioctl() which is synchronous.
> > Yeah, I understand your concerns, will fix it in the next version.
> > > > When restarting the queue, we do not need a reply, which is the same as
> > > > what qemu does in vhost_dev_start().
> > > >
> > > > So I add this parameter to distinguish the two cases.
> > > >
> > > > I think one alternative implementation is to add a interface in
> > > > VhostOps: queue_reset(). In this way details can be hidden. What do you
> > > > think of this solution? Does it look better?
> > > Let me ask it differently, under which case can we call this function
> > > with wait_for_reply = false?
> > >
> > > Thanks
> > Cases when we do not need to wait until that the reply has been
> > processed. Actually in dev_start(), or dev_stop(),
> But we don't need to send virtqueue reset requests in those cases?

You're right, we don't need to do this in those cases.

Thanks

> > they do not wait for
> > replies when enabling/disabling vqs.
> >
> > Specifically, vhost_set_vring_enable() will call it with wait_for_reply
> > = false.
> >
> > In our vq reset scenario, it should be synchronous because the driver
> > need to modify queues after the device stop using the vq in the backend.
> > Undefined errors will occur If the device is still using the queue when
> > the driver is making modifications to it,
> >
> > Back to our implementation, we will not expose this parameter in the
> > next version.
> Ok.
>
> Thanks
>
> > Thanks.
> >
> > > > Thanks
> > > >
> > > > > > Signed-off-by: Kangjie Xu <kangjie.xu@linux.alibaba.com>
> > > > > > Signed-off-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
> > > > > > ---
> > > > > >     include/hw/virtio/vhost-backend.h | 4 ++++
> > > > > >     1 file changed, 4 insertions(+)
> > > > > >
> > > > > > diff --git a/include/hw/virtio/vhost-backend.h
> > > > > > b/include/hw/virtio/vhost-backend.h
> > > > > > index eab46d7f0b..7bddd1e9a0 100644
> > > > > > --- a/include/hw/virtio/vhost-backend.h
> > > > > > +++ b/include/hw/virtio/vhost-backend.h
> > > > > > @@ -81,6 +81,9 @@ typedef int (*vhost_set_backend_cap_op)(struct
> > > > > > vhost_dev *dev);
> > > > > >     typedef int (*vhost_set_owner_op)(struct vhost_dev *dev);
> > > > > >     typedef int (*vhost_reset_device_op)(struct vhost_dev *dev);
> > > > > >     typedef int (*vhost_get_vq_index_op)(struct vhost_dev *dev, int idx);
> > > > > > +typedef int (*vhost_set_single_vring_enable_op)(struct vhost_dev *dev,
> > > > > > +                                                int index, int enable,
> > > > > > +                                                bool wait_for_reply);
> > > > > >     typedef int (*vhost_set_vring_enable_op)(struct vhost_dev *dev,
> > > > > >                                              int enable);
> > > > > >     typedef bool (*vhost_requires_shm_log_op)(struct vhost_dev *dev);
> > > > > > @@ -155,6 +158,7 @@ typedef struct VhostOps {
> > > > > >         vhost_set_owner_op vhost_set_owner;
> > > > > >         vhost_reset_device_op vhost_reset_device;
> > > > > >         vhost_get_vq_index_op vhost_get_vq_index;
> > > > > > +    vhost_set_single_vring_enable_op vhost_set_single_vring_enable;
> > > > > >         vhost_set_vring_enable_op vhost_set_vring_enable;
> > > > > >         vhost_requires_shm_log_op vhost_requires_shm_log;
> > > > > >         vhost_migration_done_op vhost_migration_done;