Re: [PATCH 7/7] vhost-vdpa: backend feature should set only once

[Eugenio Perez Martin]

On Wed, Mar 30, 2022 at 7:32 PM Stefano Garzarella <sgarzare@redhat.com> wrote:
>
> On Wed, Mar 30, 2022 at 10:12:42AM -0700, Si-Wei Liu wrote:
> >
> >
> >On 3/30/2022 9:24 AM, Stefano Garzarella wrote:
> >>On Tue, Mar 29, 2022 at 11:33:17PM -0700, Si-Wei Liu wrote:
> >>>The vhost_vdpa_one_time_request() branch in
> >>>vhost_vdpa_set_backend_cap() incorrectly sends down
> >>>iotls on vhost_dev with non-zero index. This may
> >>
> >>Little typo s/iotls/ioctls
> >Thanks! Will correct it in v2.
> >
> >>
> >>>end up with multiple VHOST_SET_BACKEND_FEATURES
> >>>ioctl calls sent down on the vhost-vdpa fd that is
> >>>shared between all these vhost_dev's.
> >>>
> >>>To fix it, send down ioctl only once via the first
> >>>vhost_dev with index 0. Toggle the polarity of the
> >>>vhost_vdpa_one_time_request() test would do the trick.
> >>>
> >>>Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> >>>---
> >>>hw/virtio/vhost-vdpa.c | 2 +-
> >>>1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>>diff --git a/hw/virtio/vhost-vdpa.c b/hw/virtio/vhost-vdpa.c
> >>>index c5ed7a3..27ea706 100644
> >>>--- a/hw/virtio/vhost-vdpa.c
> >>>+++ b/hw/virtio/vhost-vdpa.c
> >>>@@ -665,7 +665,7 @@ static int vhost_vdpa_set_backend_cap(struct
> >>>vhost_dev *dev)
> >>>
> >>>    features &= f;
> >>>
> >>>-    if (vhost_vdpa_one_time_request(dev)) {
> >>>+    if (!vhost_vdpa_one_time_request(dev)) {
> >>>        r = vhost_vdpa_call(dev, VHOST_SET_BACKEND_FEATURES, &features);
> >>>        if (r) {
> >>>            return -EFAULT;
> >>>--
> >>>1.8.3.1
> >>>
> >>>
> >>
> >>With that:
> >>
> >>Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >>
> >>
> >>
> >>Unrelated to this patch, but the name vhost_vdpa_one_time_request()
> >>is confusing IMHO.
> >Coincidentally I got the same feeling and wanted to rename it to
> >something else, too.
> >
> >>
> >>Not that I'm good with names, but how about we change it to
> >>vhost_vdpa_skip_one_time_request()?
> >How about vhost_vdpa_request_already_applied()? seems to be more
> >readable in the context.
>
> That's fine too, except you can't discern that it's a single request per
> device, so maybe I'd add "dev," but I don't know if it gets too long:
>
> vhost_vdpa_dev_request_already_applied()
>
> And I would also add a comment to the function to explain that we use
> that function for requests that only need to be applied once, regardless
> of the number of queues.
>

In my opinion it should express what it checks: vhost_vdpa_first, or
vhost_vdpa_first_dev, vhost_vdpa_first_queue... and to add a comment
like the one you propose. I think the context of the caller gives
enough information.

I would add that the use is for requests that only need / must be
applied once *and before setting up queues*, *at the beginning of
operation*, or similar, because we do a similar check with
dev->vq_index_end and these are not exchangeable.

Thanks!

> Thanks,
> Stefano
>