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/19/2023 3:26 PM, Michael S. Tsirkin wrote:
> On Wed, Jul 19, 2023 at 03:20:03PM -0700, Si-Wei Liu wrote:
>>
>> 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
> Coming from the spec RING_RESET has clearer semantics.
Spec doesn't have clearer semantics on vdpa specifics - such as how does 
RING_RESET interoperate with ASID?
> As long as we support it it is not critical which one
> is the default though.
The point is vdpa vendor drivers may implement RING_RESET for a 
different purpose than live migration. In case they support both I don't 
see a reason why it has to fallback to a slower path given there's a 
faster path. May we should leave this to vendor driver to decide, but I 
am not sure.

-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