Re: [PATCH 5/7] vdpa: Add support for querying control virtqueue index

On Wed, Dec 8, 2021 at 2:07 PM Eli Cohen [off-list ref] wrote:

On Wed, Dec 08, 2021 at 11:25:01AM +0800, Jason Wang wrote:

quoted

On Tue, Dec 7, 2021 at 3:57 PM Eli Cohen [off-list ref] wrote:

quoted

On Fri, Dec 03, 2021 at 10:35:30AM +0800, Jason Wang wrote:

quoted

On Thu, Dec 2, 2021 at 3:58 AM Eli Cohen [off-list ref] wrote:

quoted

Add netlink attribute and callback function to query the control VQ
index of a device.

It's better to explain the motivation of this. Actually we can deduce
it from the max_virtqueue_paris if I was not wrong.

Right, we can deduce it. But I think, let's let the kernel deduce it.
e.g., we can avoid the new get_ctrl_vq_idx() callback, but still return
the value to userspace with VDPA_ATTR_DEV_CTRL_VQ_IDX.

quoted

So it means the
value varies depending on if VIRTIO_NET_F_MQ

I guest you meant VIRTIO_NET_F_CTRL_VQ

quoted

is negotiated which can
be changed after a query. This may probably confuse the userspace
sometime.

No sure I follow you on this. The CVQ index is dictated after the call
to _vdpa_register_device() and cannot change. Whether there is a CVQ or
isn't depends on VIRTIO_NET_F_CTRL_VQ but the index doesn't change.

Looks not, according to the spec, consider we have a device with 2
queue pairs plus cvq.

When MQ is negotiated, cvq index is 4.
When MQ is not negotiated, cvq index is 2.

When you say MQ negotiated, do you mean both ways or just offered by the
upstream driver?

I meant MQ feature is enabled from the driver and accepted by the device.

I think I saw different behavior and I am going to check again.

BTW, could you send reference to where the specs refers to the index of
the control virtqueue?

You may refer spec chapter 5.1.2:
https://docs.oasis-open.org/virtio/virtio/v1.1/csprd01/virtio-v1.1-csprd01.html#x1-1960002

Thanks

quoted

Thanks

quoted

quoted

Thanks

quoted

Signed-off-by: Eli Cohen <redacted>
---
 drivers/vdpa/vdpa.c       | 18 ++++++++++++++++++
 include/linux/vdpa.h      |  1 +
 include/uapi/linux/vdpa.h |  1 +
 3 files changed, 20 insertions(+)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index f06f949d5fa1..ca3ab0f46188 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c

@@ -712,6 +712,20 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
        return msg->len;
 }

+static int vdpa_dev_net_ctrl_vq_fill(struct vdpa_device *vdev,
+                                    struct sk_buff *msg, u64 features)
+{
+       u16 val_u16 = 0;
+
+       if (features & BIT_ULL(VIRTIO_NET_F_CTRL_VQ) &&
+           vdev->config->get_ctrl_vq_idx) {
+               val_u16 = vdev->config->get_ctrl_vq_idx(vdev);
+               return nla_put_u16(msg, VDPA_ATTR_DEV_CTRL_VQ_IDX, val_u16);
+       }
+
+       return 0;
+}
+
 static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
                                       struct sk_buff *msg, u64 features,
                                       const struct virtio_net_config *config)

@@ -730,6 +744,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
        struct virtio_net_config config = {};
        u64 features;
        u16 val_u16;
+       int err;

        vdpa_get_config(vdev, 0, &config, sizeof(config));

@@ -746,6 +761,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
                return -EMSGSIZE;

        features = vdev->config->get_features(vdev);
+       err = vdpa_dev_net_ctrl_vq_fill(vdev, msg, features);
+       if (err)
+               return err;

        return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
 }

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index 820621c59365..fca9bfeed9ba 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -274,6 +274,7 @@ struct vdpa_config_ops {
        (*get_vq_notification)(struct vdpa_device *vdev, u16 idx);
        /* vq irq is not expected to be changed once DRIVER_OK is set */
        int (*get_vq_irq)(struct vdpa_device *vdev, u16 idx);
+       u16 (*get_ctrl_vq_idx)(struct vdpa_device *vdev);

        /* Device ops */
        u32 (*get_vq_align)(struct vdpa_device *vdev);

diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index a252f06f9dfd..2e3a7f89f42d 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h

@@ -34,6 +34,7 @@ enum vdpa_attr {
        VDPA_ATTR_DEV_MAX_VQS,                  /* u32 */
        VDPA_ATTR_DEV_MAX_VQ_SIZE,              /* u16 */
        VDPA_ATTR_DEV_MIN_VQ_SIZE,              /* u16 */
+       VDPA_ATTR_DEV_CTRL_VQ_IDX,              /* u16 */

        VDPA_ATTR_DEV_NET_CFG_MACADDR,          /* binary */
        VDPA_ATTR_DEV_NET_STATUS,               /* u8 */
--

2.33.1

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization