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

[Eugenio Perez Martin]

On Wed, Mar 30, 2022 at 8:33 AM Si-Wei Liu <si-wei.liu@oracle.com> 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
> 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.
>

Not only that. This means that qemu thinks the device supports iotlb
batching as long as the device does not have cvq. If vdpa does not
support batching, it will return an error later with no possibility of
doing it ok. Some open questions:

Should we make the vdpa driver return error as long as a feature is
used but not set by qemu, or let it as undefined? I guess we have to
keep the batching at least without checking so the kernel supports old
versions of qemu.

On the other hand, should we return an error if IOTLB_MSG_V2 is not
supported here? We're basically assuming it in other functions.

> 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>

Acked-by: Eugenio Pérez <eperezma@redhat.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
>