Re: [PULL 28/53] vdpa: move CVQ isolation check to net_init_vhost_vdpa

[Peter Maydell]

On Mon, 26 Jun 2023 at 13:29, Michael S. Tsirkin <mst@redhat.com> wrote:
>
> From: Eugenio Pérez <eperezma@redhat.com>
>
> Evaluating it at start time instead of initialization time may make the
> guest capable of dynamically adding or removing migration blockers.
>
> Also, moving to initialization reduces the number of ioctls in the
> migration, reducing failure possibilities.
>
> As a drawback we need to check for CVQ isolation twice: one time with no
> MQ negotiated and another one acking it, as long as the device supports
> it.  This is because Vring ASID / group management is based on vq
> indexes, but we don't know the index of CVQ before negotiating MQ.

I was looking at this code because of a Coverity report.
That turned out to be a false positive, but I did notice
something here that looks like it might be wrong:

>
> +/**
> + * Probe if CVQ is isolated
> + *
> + * @device_fd         The vdpa device fd
> + * @features          Features offered by the device.
> + * @cvq_index         The control vq pair index
> + *
> + * Returns <0 in case of failure, 0 if false and 1 if true.
> + */
> +static int vhost_vdpa_probe_cvq_isolation(int device_fd, uint64_t features,
> +                                          int cvq_index, Error **errp)
> +{
> +    uint64_t backend_features;
> +    int64_t cvq_group;
> +    uint8_t status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> +                     VIRTIO_CONFIG_S_DRIVER |
> +                     VIRTIO_CONFIG_S_FEATURES_OK;
> +    int r;
> +
> +    ERRP_GUARD();
> +
> +    r = ioctl(device_fd, VHOST_GET_BACKEND_FEATURES, &backend_features);
> +    if (unlikely(r < 0)) {
> +        error_setg_errno(errp, errno, "Cannot get vdpa backend_features");
> +        return r;
> +    }
> +
> +    if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_IOTLB_ASID))) {
> +        return 0;
> +    }
> +
> +    r = ioctl(device_fd, VHOST_SET_FEATURES, &features);
> +    if (unlikely(r)) {
> +        error_setg_errno(errp, errno, "Cannot set features");

Shouldn't we have a 'return r' (or maybe a 'goto out') here ?
Otherwise we'll just plough onward and attempt to continue
execution...

> +    }
> +
> +    r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> +    if (unlikely(r)) {
> +        error_setg_errno(errp, -r, "Cannot set device features");

Isn't this trying to set device status, not features ?

> +        goto out;
> +    }

thanks
-- PMM