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

[Si-Wei Liu]

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.

-Siwei

> Thanks,
> Stefano