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

? 2022/2/18 ??8:35, Eugenio Perez Martin ??:
> On Tue, Feb 8, 2022 at 4:23 AM Jason Wang <jasowang at redhat.com>
wrote:
>>
>> ? 2022/1/31 ??11:34, Eugenio Perez Martin ??:
>>> On Sat, Jan 29, 2022 at 9:06 AM Jason Wang <jasowang at
redhat.com> wrote:
>>>> ? 2022/1/22 ??4:27, Eugenio P?rez ??:
>>>>> Signed-off-by: Eugenio P?rez <eperezma at 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.

Any reason for this? I guess you meant multiqueue. If yes, it should not 
be much difference since we have idx as the parameter.

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

I wonder what happens if MSI-X is masking in guest. So if I understand 
correctly, we don't disable the eventfd from device? If yes, this seems 
suboptinal.

Thanks

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