diff mbox series

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

Message ID 1701970793-6865-4-git-send-email-si-wei.liu@oracle.com (mailing list archive)
State New, archived
Headers show
Series vdpa-net: improve migration downtime through descriptor ASID and persistent IOTLB | expand

Commit Message

Si-Wei Liu Dec. 7, 2023, 5:39 p.m. UTC 
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(+)

Comments

Eugenio Perez Martin Dec. 11, 2023, 6:49 p.m. UTC | #1 
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
>
>
Jason Wang Jan. 11, 2024, 4:02 a.m. UTC | #2 
On Fri, Dec 8, 2023 at 2:53 AM 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;
> +    }

I wonder what's the reason for the status being saved and restored?

We don't do this in vhost_vdpa_probe_cvq_isolation().

Thanks
diff mbox series

Patch

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;
+    }
+
+    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");
+    }
+
+    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;
+}
+
 static NetClientState *net_vhost_vdpa_init(NetClientState *peer,
                                        const char *device,
                                        const char *name,