Re: [PATCH 03/40] vdpa: probe descriptor group index for data vqs

[Eugenio Perez Martin]

On Thu, Dec 7, 2023 at 7:53 PM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
> Getting it ahead at initialization time instead of start time allows
> decision making independent of device status, while reducing failure
> possibility in starting device or during migration.
>
> Adding function vhost_vdpa_probe_desc_group() for that end. This
> function will be used to probe the descriptor group for data vqs.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>  net/vhost-vdpa.c | 89 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 89 insertions(+)
>
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 887c329..0cf3147 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -1688,6 +1688,95 @@ out:
>      return r;
>  }
>
> +static int vhost_vdpa_probe_desc_group(int device_fd, uint64_t features,
> +                                       int vq_index, int64_t *desc_grpidx,
> +                                       Error **errp)
> +{
> +    uint64_t backend_features;
> +    int64_t vq_group, desc_group;
> +    uint8_t saved_status = 0;
> +    uint8_t status = 0;
> +    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;
> +    }
> +
> +    if (!(backend_features & BIT_ULL(VHOST_BACKEND_F_DESC_ASID))) {
> +        return 0;
> +    }
> +
> +    r = ioctl(device_fd, VHOST_VDPA_GET_STATUS, &saved_status);
> +    if (unlikely(r)) {
> +        error_setg_errno(errp, -r, "Cannot get device status");
> +        goto out;

Nit, we could return here directly, can't we?

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

missing goto out?

> +    }
> +
> +    status = VIRTIO_CONFIG_S_ACKNOWLEDGE |
> +             VIRTIO_CONFIG_S_DRIVER |
> +             VIRTIO_CONFIG_S_FEATURES_OK;
> +
> +    r = ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> +    if (unlikely(r)) {
> +        error_setg_errno(errp, -r, "Cannot set device status");
> +        goto out;
> +    }
> +
> +    vq_group = vhost_vdpa_get_vring_group(device_fd, vq_index, errp);
> +    if (unlikely(vq_group < 0)) {
> +        if (vq_group != -ENOTSUP) {
> +            r = vq_group;
> +            goto out;
> +        }
> +
> +        /*
> +         * The kernel report VHOST_BACKEND_F_IOTLB_ASID if the vdpa frontend
> +         * support ASID even if the parent driver does not.
> +         */
> +        error_free(*errp);
> +        *errp = NULL;
> +        r = 0;
> +        goto out;
> +    }
> +
> +    desc_group = vhost_vdpa_get_vring_desc_group(device_fd, vq_index,
> +                                                 errp);
> +    if (unlikely(desc_group < 0)) {
> +        r = desc_group;
> +        goto out;
> +    } else if (desc_group != vq_group) {
> +        *desc_grpidx = desc_group;
> +    }
> +    r = 1;
> +
> +out:
> +    status = 0;
> +    ioctl(device_fd, VHOST_VDPA_SET_STATUS, &status);
> +    if (saved_status) {
> +        ioctl(device_fd, VHOST_VDPA_SET_STATUS, &saved_status);
> +    }
> +    return r;
> +}
> +

It is invalid to add static functions without a caller, I think the
compiler will complain about this.

>  static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
>                                         const char *device,
>                                         const char *name,
> --
> 1.8.3.1
>
>