Re: [PATCH 09/31] vhost-vdpa: Take into account SVQ in vhost_vdpa_set_vring_call

[Eugenio Perez Martin]

On Tue, Feb 8, 2022 at 4:23 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 在 2022/1/31 下午11:34, Eugenio Perez Martin 写道:
> > On Sat, Jan 29, 2022 at 9:06 AM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 在 2022/1/22 上午4:27, Eugenio Pérez 写道:
> >>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
> >>> ---
> >>>    hw/virtio/vhost-vdpa.c | 20 ++++++++++++++++++--
> >>>    1 file changed, 18 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>> index 18de14f0fb..029f98feee 100644
> >>> --- a/hw/virtio/vhost-vdpa.c
> >>> +++ b/hw/virtio/vhost-vdpa.c
> >>> @@ -687,13 +687,29 @@ static int vhost_vdpa_set_vring_kick(struct 
> >>> vhost_dev *dev,
> >>>        }
> >>>    }
> >>>
> >>> -static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
> >>> -                                       struct vhost_vring_file *file)
> >>> +static int vhost_vdpa_set_vring_dev_call(struct vhost_dev *dev,
> >>> +                                         struct vhost_vring_file *file)
> >>>    {
> >>>        trace_vhost_vdpa_set_vring_call(dev, file->index, file->fd);
> >>>        return vhost_vdpa_call(dev, VHOST_SET_VRING_CALL, file);
> >>>    }
> >>>
> >>> +static int vhost_vdpa_set_vring_call(struct vhost_dev *dev,
> >>> +                                     struct vhost_vring_file *file)
> >>> +{
> >>> +    struct vhost_vdpa *v = dev->opaque;
> >>> +
> >>> +    if (v->shadow_vqs_enabled) {
> >>> +        int vdpa_idx = vhost_vdpa_get_vq_index(dev, file->index);
> >>> +        VhostShadowVirtqueue *svq = g_ptr_array_index(v->shadow_vqs, 
> >>> vdpa_idx);
> >>> +
> >>> +        vhost_svq_set_guest_call_notifier(svq, file->fd);
> >>
> >> Two questions here (had similar questions for vring kick):
> >>
> >> 1) Any reason that we setup the eventfd for vhost-vdpa in
> >> vhost_vdpa_svq_setup() not here?
> >>
> > I'm not sure what you mean.
> >
> > The guest->SVQ call and kick fds are set here and at
> > vhost_vdpa_set_vring_kick. The event notifier handler of the guest ->
> > SVQ kick_fd is set at vhost_vdpa_set_vring_kick /
> > vhost_svq_set_svq_kick_fd. The guest -> SVQ call fd has no event
> > notifier handler since we don't poll it.
> >
> > On the other hand, the connection SVQ <-> device uses the same fds
> > from the beginning to the end, and they will not change with, for
> > example, call fd masking. That's why it's setup from
> > vhost_vdpa_svq_setup. Delaying to vhost_vdpa_set_vring_call would make
> > us add way more logic there.
>
>
> More logic in general shadow vq code but less codes for vhost-vdpa
> specific code I think.
>
> E.g for we can move the kick set logic from vhost_vdpa_svq_set_fds() to
> here.
>

But they are different fds. vhost_vdpa_svq_set_fds sets the
SVQ<->device. This function sets the SVQ->guest call file descriptor.

To move the logic of vhost_vdpa_svq_set_fds here would imply either:
a) Logic to know if we are receiving the first call fd or not. That
code is not in the series at the moment, because setting at
vhost_vdpa_dev_start tells the difference for free. Is just adding
code, not moving.
b) Logic to set again *the same* file descriptor to device, with logic
to tell if we have missed calls. That logic is not implemented for
device->SVQ call file descriptor, because we are assuming it never
changes from vhost_vdpa_svq_set_fds. So this is again adding code.

At this moment, we have:
vhost_vdpa_svq_set_fds:
  set SVQ<->device fds

vhost_vdpa_set_vring_call:
  set guest<-SVQ call

vhost_vdpa_set_vring_kick:
  set guest->SVQ kick.

If I understood correctly, the alternative would be something like:
vhost_vdpa_set_vring_call:
  set guest<-SVQ call
  if(!vq->call_set) {
    - set SVQ<-device call.
    - vq->call_set = true
  }

vhost_vdpa_set_vring_kick:
  set guest<-SVQ call
  if(!vq->dev_kick_set) {
    - set guest->device kick.
    - vq->dev_kick_set = true
  }

dev_reset / dev_stop:
for vq in vqs:
  vq->dev_kick_set = vq->dev_call_set = false
...

Or have I misunderstood something?

Thanks!

> Thanks
>
>
> >
> >> 2) The call could be disabled by using -1 as the fd, I don't see any
> >> code to deal with that.
> >>
> > Right, I didn't take that into account. vhost-kernel takes also -1 as
> > kick_fd to unbind, so SVQ can be reworked to take that into account
> > for sure.
> >
> > Thanks!
> >
> >> Thanks
> >>
> >>
> >>> +        return 0;
> >>> +    } else {
> >>> +        return vhost_vdpa_set_vring_dev_call(dev, file);
> >>> +    }
> >>> +}
> >>> +
> >>>    /**
> >>>     * Set shadow virtqueue descriptors to the device
> >>>     *
>