Re: [PATCH] vdpa: reject F_ENABLE_AFTER_DRIVER_OK if backend does not support it

[Si-Wei Liu <si-wei.liu@oracle.com>]

On 7/5/2023 11:07 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 05, 2023 at 05:07:11PM -0700, Shannon Nelson wrote:
>> On 7/5/23 11:27 AM, Eugenio Perez Martin wrote:
>>> On Wed, Jul 5, 2023 at 9:50 AM Jason Wang <jasowang@redhat.com> wrote:
>>>> On Tue, Jul 4, 2023 at 11:45 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>> On Tue, Jul 04, 2023 at 01:36:11PM +0200, Eugenio Perez Martin wrote:
>>>>>> On Tue, Jul 4, 2023 at 12:38 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>> On Tue, Jul 04, 2023 at 12:25:32PM +0200, Eugenio Perez Martin wrote:
>>>>>>>> On Mon, Jul 3, 2023 at 4:52 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>>>>>>> On Mon, Jul 03, 2023 at 04:22:18PM +0200, Eugenio Pérez wrote:
>>>>>>>>>> With the current code it is accepted as long as userland send it.
>>>>>>>>>>
>>>>>>>>>> Although userland should not set a feature flag that has not been
>>>>>>>>>> offered to it with VHOST_GET_BACKEND_FEATURES, the current code will not
>>>>>>>>>> complain for it.
>>>>>>>>>>
>>>>>>>>>> Since there is no specific reason for any parent to reject that backend
>>>>>>>>>> feature bit when it has been proposed, let's control it at vdpa frontend
>>>>>>>>>> level. Future patches may move this control to the parent driver.
>>>>>>>>>>
>>>>>>>>>> Fixes: 967800d2d52e ("vdpa: accept VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK backend feature")
>>>>>>>>>> Signed-off-by: Eugenio Pérez <eperezma@redhat.com>
>>>>>>>>> Please do send v3. And again, I don't want to send "after driver ok" hack
>>>>>>>>> upstream at all, I merged it in next just to give it some testing.
>>>>>>>>> We want RING_ACCESS_AFTER_KICK or some such.
>>>>>>>>>
>>>>>>>> Current devices do not support that semantic.
>>>>>>> Which devices specifically access the ring after DRIVER_OK but before
>>>>>>> a kick?
>> The PDS vdpa device can deal with a call to .set_vq_ready after DRIVER_OK is
>> set.  And I'm told that our VQ activity should start without a kick.
>>
>> Our vdpa device FW doesn't currently have support for VIRTIO_F_RING_RESET,
>> but I believe it could be added without too much trouble.
>>
>> sln
>>
> OK it seems clear at least in the current version pds needs
> VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK.
> However can we also code up the RING_RESET path as the default?
What's the rationale of making RING_RESET path the default? Noted this 
is on a performance critical path (for live migration downtime), did we 
ever get consensus from every or most hardware vendors that RING_RESET 
has lower cost in terms of latency overall than ENABLE_AFTER_DRIVER_OK? 
I think (RING)RESET in general falls on the slow path for hardware, 
while I assume either RING_RESET or ENABLE_AFTER_DRIVER_OK doesn't 
matters much on software backed vdpa e.g. vp_vdpa. Maybe should make 
ENABLE_AFTER_DRIVER_OK as the default?

-Siwei

> Then down the road vendors can choose what to do.
>
>
>
>
>
>>>>>> Previous versions of the QEMU LM series did a spurious kick to start
>>>>>> traffic at the LM destination [1]. When it was proposed, that spurious
>>>>>> kick was removed from the series because to check for descriptors
>>>>>> after driver_ok, even without a kick, was considered work of the
>>>>>> parent driver.
>>>>>>
>>>>>> I'm ok to go back to this spurious kick, but I'm not sure if the hw
>>>>>> will read the ring before the kick actually. I can ask.
>>>>>>
>>>>>> Thanks!
>>>>>>
>>>>>> [1] https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg02775.html
>>>>> Let's find out. We need to check for ENABLE_AFTER_DRIVER_OK too, no?
>>>> My understanding is [1] assuming ACCESS_AFTER_KICK. This seems
>>>> sub-optimal than assuming ENABLE_AFTER_DRIVER_OK.
>>>>
>>>> But this reminds me one thing, as the thread is going too long, I
>>>> wonder if we simply assume ENABLE_AFTER_DRIVER_OK if RING_RESET is
>>>> supported?
>>>>
>>> The problem with that is that the device needs to support all
>>> RING_RESET, like to be able to change vq address etc after DRIVER_OK.
>>> Not all HW support it.
>>>
>>> We just need the subset of having the dataplane freezed until all CVQ
>>> commands have been consumed. I'm sure current vDPA code already
>>> supports it in some devices, like MLX and PSD.
>>>
>>> Thanks!
>>>
>>>> Thanks
>>>>
>>>>>
>>>>>
>>>>>>>> My plan was to convert
>>>>>>>> it in vp_vdpa if needed, and reuse the current vdpa ops. Sorry if I
>>>>>>>> was not explicit enough.
>>>>>>>>
>>>>>>>> The only solution I can see to that is to trap & emulate in the vdpa
>>>>>>>> (parent?) driver, as talked in virtio-comment. But that complicates
>>>>>>>> the architecture:
>>>>>>>> * Offer VHOST_BACKEND_F_RING_ACCESS_AFTER_KICK
>>>>>>>> * Store vq enable state separately, at
>>>>>>>> vdpa->config->set_vq_ready(true), but not transmit that enable to hw
>>>>>>>> * Store the doorbell state separately, but do not configure it to the
>>>>>>>> device directly.
>>>>>>>>
>>>>>>>> But how to recover if the device cannot configure them at kick time,
>>>>>>>> for example?
>>>>>>>>
>>>>>>>> Maybe we can just fail if the parent driver does not support enabling
>>>>>>>> the vq after DRIVER_OK? That way no new feature flag is needed.
>>>>>>>>
>>>>>>>> Thanks!
>>>>>>>>
>>>>>>>>>> ---
>>>>>>>>>> Sent with Fixes: tag pointing to git.kernel.org/pub/scm/linux/kernel/git/mst
>>>>>>>>>> commit. Please let me know if I should send a v3 of [1] instead.
>>>>>>>>>>
>>>>>>>>>> [1] https://lore.kernel.org/lkml/20230609121244-mutt-send-email-mst@kernel.org/T/
>>>>>>>>>> ---
>>>>>>>>>>    drivers/vhost/vdpa.c | 7 +++++--
>>>>>>>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>>>>>>>>> index e1abf29fed5b..a7e554352351 100644
>>>>>>>>>> --- a/drivers/vhost/vdpa.c
>>>>>>>>>> +++ b/drivers/vhost/vdpa.c
>>>>>>>>>> @@ -681,18 +681,21 @@ static long vhost_vdpa_unlocked_ioctl(struct file *filep,
>>>>>>>>>>    {
>>>>>>>>>>         struct vhost_vdpa *v = filep->private_data;
>>>>>>>>>>         struct vhost_dev *d = &v->vdev;
>>>>>>>>>> +     const struct vdpa_config_ops *ops = v->vdpa->config;
>>>>>>>>>>         void __user *argp = (void __user *)arg;
>>>>>>>>>>         u64 __user *featurep = argp;
>>>>>>>>>> -     u64 features;
>>>>>>>>>> +     u64 features, parent_features = 0;
>>>>>>>>>>         long r = 0;
>>>>>>>>>>
>>>>>>>>>>         if (cmd == VHOST_SET_BACKEND_FEATURES) {
>>>>>>>>>>                 if (copy_from_user(&features, featurep, sizeof(features)))
>>>>>>>>>>                         return -EFAULT;
>>>>>>>>>> +             if (ops->get_backend_features)
>>>>>>>>>> +                     parent_features = ops->get_backend_features(v->vdpa);
>>>>>>>>>>                 if (features & ~(VHOST_VDPA_BACKEND_FEATURES |
>>>>>>>>>>                                  BIT_ULL(VHOST_BACKEND_F_SUSPEND) |
>>>>>>>>>>                                  BIT_ULL(VHOST_BACKEND_F_RESUME) |
>>>>>>>>>> -                              BIT_ULL(VHOST_BACKEND_F_ENABLE_AFTER_DRIVER_OK)))
>>>>>>>>>> +                              parent_features))
>>>>>>>>>>                         return -EOPNOTSUPP;
>>>>>>>>>>                 if ((features & BIT_ULL(VHOST_BACKEND_F_SUSPEND)) &&
>>>>>>>>>>                      !vhost_vdpa_can_suspend(v))
>>>>>>>>>> --
>>>>>>>>>> 2.39.3
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization