Re: [RFC v2 12/13] vdpa: preemptive kick at enable

[Jason Wang <jasowang@redhat.com>]

在 2023/1/13 17:06, Eugenio Perez Martin 写道:
> On Fri, Jan 13, 2023 at 4:39 AM Jason Wang <jasowang@redhat.com> wrote:
>> On Fri, Jan 13, 2023 at 11:25 AM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>
>>>
>>> On 1/13/2023 10:31 AM, Jason Wang wrote:
>>>> On Fri, Jan 13, 2023 at 1:27 AM Eugenio Pérez <eperezma@redhat.com> wrote:
>>>>> Spuriously kick the destination device's queue so it knows in case there
>>>>> are new descriptors.
>>>>>
>>>>> RFC: This is somehow a gray area. The guest may have placed descriptors
>>>>> in a virtqueue but not kicked it, so it might be surprised if the device
>>>>> starts processing it.
>>>> So I think this is kind of the work of the vDPA parent. For the parent
>>>> that needs this trick, we should do it in the parent driver.
>>> Agree, it looks easier implementing this in parent driver,
>>> I can implement it in ifcvf set_vq_ready right now
>> Great, but please check whether or not it is really needed.
>>
>> Some device implementation could check the available descriptions
>> after DRIVER_OK without waiting for a kick.
>>
> So IIUC we can entirely drop this from the series (and I hope we can).
> But then, what with the devices that does *not* check for them?


It needs mediation in the vDPA parent driver.


>
> If we drop it it seems to me we must mandate devices to check for
> descriptors at queue_enable. The queue could stall if not, isn't it?


I'm not sure, did you see real issue with this? (Note that we don't do 
this for vhost-user-(vDPA))

Btw, the code can result of kick before DRIVER_OK, which seems racy.

Thanks


>
> Thanks!
>
>> Thanks
>>
>>> Thanks
>>> Zhu Lingshan
>>>> Thanks
>>>>
>>>>> However, that information is not in the migration stream and it should
>>>>> be an edge case anyhow, being resilient to parallel notifications from
>>>>> the guest.
>>>>>
>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>> ---
>>>>>    hw/virtio/vhost-vdpa.c | 5 +++++
>>>>>    1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
>>>>> index 40b7e8706a..dff94355dd 100644
>>>>> --- a/hw/virtio/vhost-vdpa.c
>>>>> +++ b/hw/virtio/vhost-vdpa.c
>>>>> @@ -732,11 +732,16 @@ static int vhost_vdpa_set_vring_ready(struct vhost_dev *dev, int ready)
>>>>>        }
>>>>>        trace_vhost_vdpa_set_vring_ready(dev);
>>>>>        for (i = 0; i < dev->nvqs; ++i) {
>>>>> +        VirtQueue *vq;
>>>>>            struct vhost_vring_state state = {
>>>>>                .index = dev->vq_index + i,
>>>>>                .num = 1,
>>>>>            };
>>>>>            vhost_vdpa_call(dev, VHOST_VDPA_SET_VRING_ENABLE, &state);
>>>>> +
>>>>> +        /* Preemptive kick */
>>>>> +        vq = virtio_get_queue(dev->vdev, dev->vq_index + i);
>>>>> +        event_notifier_set(virtio_queue_get_host_notifier(vq));
>>>>>        }
>>>>>        return 0;
>>>>>    }
>>>>> --
>>>>> 2.31.1
>>>>>