Re: [PATCH 1/2] vdpa: Allow to configure max data virtqueues

On 12/1/2021 2:03 AM, Eli Cohen wrote:

On Tue, Nov 30, 2021 at 05:24:03PM -0800, Si-Wei Liu wrote:

quoted

On 11/30/2021 1:48 AM, Eli Cohen wrote:

quoted

Allow to configure the max virtqueues for a device.

Signed-off-by: Eli Cohen <redacted>
---
   drivers/vdpa/vdpa.c  | 16 +++++++++++++++-
   include/linux/vdpa.h |  1 +
   2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 7332a74a4b00..e185ec2ee851 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c

@@ -480,7 +480,8 @@ vdpa_nl_cmd_mgmtdev_get_dumpit(struct sk_buff *msg, struct netlink_callback *cb)
   }
   #define VDPA_DEV_NET_ATTRS_MASK ((1 << VDPA_ATTR_DEV_NET_CFG_MACADDR) | \
-				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU))
+				 (1 << VDPA_ATTR_DEV_NET_CFG_MTU) | \
+				 (1 << VDPA_ATTR_DEV_NET_CFG_MAX_VQP))

It seems VDPA_ATTR_DEV_MAX_VQS (u32) is what you want (# of data virtqueues
instead of # of data virtqueue pairs)? Not sure what's possible use of
VDPA_ATTR_DEV_NET_CFG_MAX_VQP, was it to dump/display the config space
max_virtqueue_pairs value (u16, 1-32768) for virtio-net? Why there's such
quasi-duplicate attribute introduced in the first place?

VDPA_ATTR_DEV_MAX_VQS currently returns vdev->nvqs which equals whatever
passed to _vdpa_register_device(). The latter depends on the value
provided by (struct vdpa_dev_set_config).max_virtqueues. It's the only
way to let the user know if there are other virtqueues besides the data
virtqueues. For example, if we support control virtqueue and configured
2 data QPs, we will return 5.

That's right. Then let VDPA_ATTR_DEV_MAX_VQS continue to be read-only as is.

Maybe we should add attributes to add aditional virtqueues like control
virtqueue and their index. They could be returned by
vdpa_dev_net_config_fill().

Probably this info would need to return to QEMU for proper virtq 
modeling via vDPA ioctls rather than just netlink API. Agreed it's 
virtio-net specific config.

VDPA_ATTR_DEV_NET_CFG_MAX_VQP seems the right choice since it is used to
provid the requested number of data virtqueues when creating the device.
Maybe we need to change the name to VDPA_ATTR_DEV_NET_CFG_MAX_VQ and
then this patch makes more sense.

I would just keep the name for VDPA_ATTR_DEV_NET_CFG_MAX_VQP and have 
user configure the number of data queue pairs instead. Very few virtio 
types come with virtqueue in pairs, so this has to be vdpa net specific 
config.

Thanks,
-Siwei

I will post another series to address all these issues.

quoted

Not even sure VDPA_ATTR_DEV_MAX_VQS by definition should include other
virtqueues as well: such as control virtqueue or event virtqueue. Hence the
name will be more applicable to vdpa devices of other virtio type than just
virtio-net. Otherwise I would think this attribute is slightly misnamed
(max_data_vqs might be a proper name).

quoted

   static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
   {

@@ -506,6 +507,19 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
   			nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU]);
   		config.mask |= (1 << VDPA_ATTR_DEV_NET_CFG_MTU);
   	}
+	if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]) {
+		config.max_virtqueues = nla_get_u16(nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP]);
+		if (config.max_virtqueues < 2) {
+			NL_SET_ERR_MSG_MOD(info->extack, "At least two virtqueues are required");
+			return -EINVAL;
+		}
+		if ((config.max_virtqueues - 1) & config.max_virtqueues) {
+			NL_SET_ERR_MSG_MOD(info->extack,
+					   "Must provide power of two number of virtqueues");

Why there's such limitation for the number of vDPA virtqueues? I thought the
software virtio doesn't have this limitation (power of two).

Right, the limitation comes from mlx5_vdpa. I will post a patch later to
remove that limitation.

quoted

-Siwei

quoted

+			return -EINVAL;
+		}
+		config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
+	}
   	/* Skip checking capability if user didn't prefer to configure any
   	 * device networking attributes. It is likely that user might have used

diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h
index c3011ccda430..2f0b09c6d1ae 100644
--- a/include/linux/vdpa.h
+++ b/include/linux/vdpa.h

@@ -101,6 +101,7 @@ struct vdpa_dev_set_config {
   		u16 mtu;
   	} net;
   	u64 mask;
+	u16 max_virtqueues;
   };
   /**

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