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

[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.

>
> 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

>
> 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;
>