[PATCH V2 0/6] ifcvf/vDPA: support query device config space through netlink

[PATCH V2 0/6] ifcvf/vDPA: support query device config space through netlink

[Zhu Lingshan]

This series allows userspace to query device config space of vDPA
devices and the management devices through netlink,
to get multi-queue, feature bits

This series has introduced a new netlink attr
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, this should be used to query
features of vDPA  devices than the management device.

Please help review.

Thanks!
Zhu Lingshan

Changes from V1:
(1) Use __virito16_to_cpu(true, xxx) for the le16 casting(Jason)
(2) Add a comment in ifcvf_get_config_size(), to explain
why we should return the minimum value of
sizeof(struct virtio_net_config) and the onboard
cap size(Jason)
(3) Introduced a new attr VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES
(4) Show the changes of iproute2 output before and after 5/6 patch(Jason)
(5) Fix cast warning in vdpa_fill_stats_rec() 

Zhu Lingshan (6):
  vDPA/ifcvf: get_config_size should return a value no greater than dev
    implementation
  vDPA/ifcvf: support userspace to query features and MQ of a management
    device
  vDPA: allow userspace to query features of a vDPA device
  vDPA: !FEATURES_OK should not block querying device config space
  vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ
    == 0
  vDPA: fix 'cast to restricted le16' warnings in vdpa.c

 drivers/vdpa/ifcvf/ifcvf_base.c | 25 +++++++++++++++++++++++--
 drivers/vdpa/ifcvf/ifcvf_base.h |  3 +++
 drivers/vdpa/ifcvf/ifcvf_main.c |  3 +++
 drivers/vdpa/vdpa.c             | 32 +++++++++++++++-----------------
 include/uapi/linux/vdpa.h       |  1 +
 5 files changed, 45 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH V2 1/6] vDPA/ifcvf: get_config_size should return a value no greater than dev implementation

[Zhu Lingshan]

ifcvf_get_config_size() should return a virtio device type specific value,
however the ret_value should not be greater than the onboard size of
the device implementation. E.g., for virtio_net, config_size should be
the minimum value of sizeof(struct virtio_net_config) and the onboard
cap size.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 13 +++++++++++--
 drivers/vdpa/ifcvf/ifcvf_base.h |  2 ++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 48c4dadb0c7c..fb957b57941e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -128,6 +128,7 @@ int ifcvf_init_hw(struct ifcvf_hw *hw, struct pci_dev *pdev)
 			break;
 		case VIRTIO_PCI_CAP_DEVICE_CFG:
 			hw->dev_cfg = get_cap_addr(hw, &cap);
+			hw->cap_dev_config_size = le32_to_cpu(cap.length);
 			IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
 			break;
 		}
@@ -233,15 +234,23 @@ int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features)
 u32 ifcvf_get_config_size(struct ifcvf_hw *hw)
 {
 	struct ifcvf_adapter *adapter;
+	u32 net_config_size = sizeof(struct virtio_net_config);
+	u32 blk_config_size = sizeof(struct virtio_blk_config);
+	u32 cap_size = hw->cap_dev_config_size;
 	u32 config_size;
 
 	adapter = vf_to_adapter(hw);
+	/* If the onboard device config space size is greater than
+	 * the size of struct virtio_net/blk_config, only the spec
+	 * implementing contents size is returned, this is very
+	 * unlikely, defensive programming.
+	 */
 	switch (hw->dev_type) {
 	case VIRTIO_ID_NET:
-		config_size = sizeof(struct virtio_net_config);
+		config_size = cap_size >= net_config_size ? net_config_size : cap_size;
 		break;
 	case VIRTIO_ID_BLOCK:
-		config_size = sizeof(struct virtio_blk_config);
+		config_size = cap_size >= blk_config_size ? blk_config_size : cap_size;
 		break;
 	default:
 		config_size = 0;
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index 115b61f4924b..f5563f665cc6 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -87,6 +87,8 @@ struct ifcvf_hw {
 	int config_irq;
 	int vqs_reused_irq;
 	u16 nr_vring;
+	/* VIRTIO_PCI_CAP_DEVICE_CFG size */
+	u32 cap_dev_config_size;
 };
 
 struct ifcvf_adapter {
-- 
2.31.1

[PATCH V2 2/6] vDPA/ifcvf: support userspace to query features and MQ of a management device

[Zhu Lingshan]

Adapting to current netlink interfaces, this commit allows userspace
to query feature bits and MQ capability of a management device.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/ifcvf/ifcvf_base.c | 12 ++++++++++++
 drivers/vdpa/ifcvf/ifcvf_base.h |  1 +
 drivers/vdpa/ifcvf/ifcvf_main.c |  3 +++
 3 files changed, 16 insertions(+)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index fb957b57941e..7c5f1cc93ad9 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -346,6 +346,18 @@ int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num)
 	return 0;
 }
 
+u16 ifcvf_get_max_vq_pairs(struct ifcvf_hw *hw)
+{
+	struct virtio_net_config __iomem *config;
+	u16 val, mq;
+
+	config = hw->dev_cfg;
+	val = vp_ioread16((__le16 __iomem *)&config->max_virtqueue_pairs);
+	mq = le16_to_cpu((__force __le16)val);
+
+	return mq;
+}
+
 static int ifcvf_hw_enable(struct ifcvf_hw *hw)
 {
 	struct virtio_pci_common_cfg __iomem *cfg;
diff --git a/drivers/vdpa/ifcvf/ifcvf_base.h b/drivers/vdpa/ifcvf/ifcvf_base.h
index f5563f665cc6..d54a1bed212e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.h
+++ b/drivers/vdpa/ifcvf/ifcvf_base.h
@@ -130,6 +130,7 @@ u64 ifcvf_get_hw_features(struct ifcvf_hw *hw);
 int ifcvf_verify_min_features(struct ifcvf_hw *hw, u64 features);
 u16 ifcvf_get_vq_state(struct ifcvf_hw *hw, u16 qid);
 int ifcvf_set_vq_state(struct ifcvf_hw *hw, u16 qid, u16 num);
+u16 ifcvf_get_max_vq_pairs(struct ifcvf_hw *hw);
 struct ifcvf_adapter *vf_to_adapter(struct ifcvf_hw *hw);
 int ifcvf_probed_virtio_net(struct ifcvf_hw *hw);
 u32 ifcvf_get_config_size(struct ifcvf_hw *hw);
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 0a5670729412..3ff7096d30f1 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -791,6 +791,9 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 	vf->hw_features = ifcvf_get_hw_features(vf);
 	vf->config_size = ifcvf_get_config_size(vf);
 
+	ifcvf_mgmt_dev->mdev.max_supported_vqs = ifcvf_get_max_vq_pairs(vf);
+	ifcvf_mgmt_dev->mdev.supported_features = vf->hw_features;
+
 	adapter->vdpa.mdev = &ifcvf_mgmt_dev->mdev;
 	ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
 	if (ret) {
-- 
2.31.1

[PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Zhu Lingshan]

This commit adds a new vDPA netlink attribution
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
features of vDPA devices through this new attr.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/vdpa.c       | 13 +++++++++----
 include/uapi/linux/vdpa.h |  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index ebf2f363fbe7..9b0e39b2f022 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
 static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
 {
 	struct virtio_net_config config = {};
-	u64 features;
+	u64 features_device, features_driver;
 	u16 val_u16;
 
 	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
@@ -832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
 		return -EMSGSIZE;
 
-	features = vdev->config->get_driver_features(vdev);
-	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
+	features_driver = vdev->config->get_driver_features(vdev);
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
+			      VDPA_ATTR_PAD))
+		return -EMSGSIZE;
+
+	features_device = vdev->config->get_device_features(vdev);
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device,
 			      VDPA_ATTR_PAD))
 		return -EMSGSIZE;
 
-	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
+	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config);
 }
 
 static int
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 25c55cab3d7c..39f1c3d7c112 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -47,6 +47,7 @@ enum vdpa_attr {
 	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
 	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
 	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
+	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
 
 	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
 	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
-- 
2.31.1

[PATCH V2 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Zhu Lingshan]

Users may want to query the config space of a vDPA device,
to choose a appropriate one for a certain guest. This means the
users need to read the config space before FEATURES_OK, and
the existence of config space contents does not depend on
FEATURES_OK.

The spec says:
The device MUST allow reading of any device-specific configuration
field before FEATURES_OK is set by the driver. This includes
fields which are conditional on feature bits, as long as those
feature bits are offered by the device.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/vdpa.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 9b0e39b2f022..d76b22b2f7ae 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
 {
 	u32 device_id;
 	void *hdr;
-	u8 status;
 	int err;
 
 	down_read(&vdev->cf_lock);
-	status = vdev->config->get_status(vdev);
-	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
-		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not completed");
-		err = -EAGAIN;
-		goto out;
-	}
-
 	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
 			  VDPA_CMD_DEV_CONFIG_GET);
 	if (!hdr) {
-- 
2.31.1

[PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Zhu Lingshan]

If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair,
so when userspace querying queue pair numbers, it should return mq=1
than zero.

Function vdpa_dev_net_config_fill() fills the attributions of the
vDPA devices, so that it should call vdpa_dev_net_mq_config_fill()
so the parameter in vdpa_dev_net_mq_config_fill()
should be feature_device than feature_driver for the
vDPA devices themselves

Before this change, when MQ = 0, iproute2 output:
$vdpa dev config show vdpa0
vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false
max_vq_pairs 0 mtu 1500

After applying this commit, when MQ = 0, iproute2 output:
$vdpa dev config show vdpa0
vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false
max_vq_pairs 1 mtu 1500

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/vdpa.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index d76b22b2f7ae..846dd37f3549 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
 	u16 val_u16;
 
 	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
-		return 0;
+		val_u16 = 1;
+	else
+		val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
 
-	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
 	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
 }
 
@@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
 			      VDPA_ATTR_PAD))
 		return -EMSGSIZE;
 
-	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver, &config);
+	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
 }
 
 static int
-- 
2.31.1

[PATCH V2 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa.c

[Zhu Lingshan]

This commit fixes spars warnings: cast to restricted __le16
in function vdpa_dev_net_config_fill() and
vdpa_fill_stats_rec()

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
 drivers/vdpa/vdpa.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 846dd37f3549..ed49fe46a79e 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -825,11 +825,11 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
 		    config.mac))
 		return -EMSGSIZE;
 
-	val_u16 = le16_to_cpu(config.status);
+	val_u16 = __virtio16_to_cpu(true, config.status);
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
 		return -EMSGSIZE;
 
-	val_u16 = le16_to_cpu(config.mtu);
+	val_u16 = __virtio16_to_cpu(true, config.mtu);
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
 		return -EMSGSIZE;
 
@@ -911,7 +911,7 @@ static int vdpa_fill_stats_rec(struct vdpa_device *vdev, struct sk_buff *msg,
 	}
 	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));
 
-	max_vqp = le16_to_cpu(config.max_virtqueue_pairs);
+	max_vqp = __virtio16_to_cpu(true, config.max_virtqueue_pairs);
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, max_vqp))
 		return -EMSGSIZE;
 
-- 
2.31.1

RE: [PATCH V2 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Parav Pandit via Virtualization]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> 
> Users may want to query the config space of a vDPA device, to choose a
> appropriate one for a certain guest. This means the users need to read the
> config space before FEATURES_OK, and the existence of config space
> contents does not depend on FEATURES_OK.
> 
> The spec says:
> The device MUST allow reading of any device-specific configuration field
> before FEATURES_OK is set by the driver. This includes fields which are
> conditional on feature bits, as long as those feature bits are offered by the
> device.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> 9b0e39b2f022..d76b22b2f7ae 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
> struct sk_buff *msg, u32 portid,  {
>  	u32 device_id;
>  	void *hdr;
> -	u8 status;
>  	int err;
> 
>  	down_read(&vdev->cf_lock);
> -	status = vdev->config->get_status(vdev);
> -	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> -		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
> completed");
> -		err = -EAGAIN;
> -		goto out;
> -	}
> -
>  	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>  			  VDPA_CMD_DEV_CONFIG_GET);
>  	if (!hdr) {
> --
> 2.31.1

Management interface should not mix up the device state machine checks like above.
Hence above code removal is better choice.
Please add fixes tag to this patch.

Reviewed-by: Parav Pandit <parav@nvidia.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH V2 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> 
> Users may want to query the config space of a vDPA device, to choose a
> appropriate one for a certain guest. This means the users need to read the
> config space before FEATURES_OK, and the existence of config space
> contents does not depend on FEATURES_OK.
> 
> The spec says:
> The device MUST allow reading of any device-specific configuration field
> before FEATURES_OK is set by the driver. This includes fields which are
> conditional on feature bits, as long as those feature bits are offered by the
> device.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 8 --------
>  1 file changed, 8 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> 9b0e39b2f022..d76b22b2f7ae 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
> struct sk_buff *msg, u32 portid,  {
>  	u32 device_id;
>  	void *hdr;
> -	u8 status;
>  	int err;
> 
>  	down_read(&vdev->cf_lock);
> -	status = vdev->config->get_status(vdev);
> -	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
> -		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
> completed");
> -		err = -EAGAIN;
> -		goto out;
> -	}
> -
>  	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>  			  VDPA_CMD_DEV_CONFIG_GET);
>  	if (!hdr) {
> --
> 2.31.1

Management interface should not mix up the device state machine checks like above.
Hence above code removal is better choice.
Please add fixes tag to this patch.

Reviewed-by: Parav Pandit <parav@nvidia.com>

RE: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Parav Pandit via Virtualization]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> To: jasowang@redhat.com; mst@redhat.com
> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Parav
> Pandit <parav@nvidia.com>; xieyongji@bytedance.com;
> gautam.dawar@amd.com; Zhu Lingshan <lingshan.zhu@intel.com>
> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace
> when VIRTIO_NET_F_MQ == 0
> 
> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so
> when userspace querying queue pair numbers, it should return mq=1 than
> zero.
> 
> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA
> devices, so that it should call vdpa_dev_net_mq_config_fill() so the
> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than
> feature_driver for the vDPA devices themselves
> 
> Before this change, when MQ = 0, iproute2 output:
> $vdpa dev config show vdpa0
> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0
> mtu 1500
> 
Max_vq_pairs should not be printed when _MQ feature is not negotiated.
Existing code that returns 0 is correct following this line of the spec.

" The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_-
NET_F_RSS is set."
The field doesn't exist when _MQ is not there. Hence, it should not be printed.
Is _RSS offered and is that why you see it?

If not a fix in the iproute2/vdpa should be done.


> After applying this commit, when MQ = 0, iproute2 output:
> $vdpa dev config show vdpa0
> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1
> mtu 1500
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> d76b22b2f7ae..846dd37f3549 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,
>  	u16 val_u16;
> 
>  	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
> -		return 0;
> +		val_u16 = 1;
> +	else
> +		val_u16 = __virtio16_to_cpu(true, config-
> >max_virtqueue_pairs);
> 
> -	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
>  	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> val_u16);  }
> 
> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct
> vdpa_device *vdev, struct sk_buff *ms
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
> +&config);
>  }
> 
>  static int
> --
> 2.31.1

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

RE: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> To: jasowang@redhat.com; mst@redhat.com
> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Parav
> Pandit <parav@nvidia.com>; xieyongji@bytedance.com;
> gautam.dawar@amd.com; Zhu Lingshan <lingshan.zhu@intel.com>
> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace
> when VIRTIO_NET_F_MQ == 0
> 
> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so
> when userspace querying queue pair numbers, it should return mq=1 than
> zero.
> 
> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA
> devices, so that it should call vdpa_dev_net_mq_config_fill() so the
> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than
> feature_driver for the vDPA devices themselves
> 
> Before this change, when MQ = 0, iproute2 output:
> $vdpa dev config show vdpa0
> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0
> mtu 1500
> 
Max_vq_pairs should not be printed when _MQ feature is not negotiated.
Existing code that returns 0 is correct following this line of the spec.

" The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_-
NET_F_RSS is set."
The field doesn't exist when _MQ is not there. Hence, it should not be printed.
Is _RSS offered and is that why you see it?

If not a fix in the iproute2/vdpa should be done.


> After applying this commit, when MQ = 0, iproute2 output:
> $vdpa dev config show vdpa0
> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1
> mtu 1500
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> d76b22b2f7ae..846dd37f3549 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,
>  	u16 val_u16;
> 
>  	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
> -		return 0;
> +		val_u16 = 1;
> +	else
> +		val_u16 = __virtio16_to_cpu(true, config-
> >max_virtqueue_pairs);
> 
> -	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
>  	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
> val_u16);  }
> 
> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct
> vdpa_device *vdev, struct sk_buff *ms
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
> +&config);
>  }
> 
>  static int
> --
> 2.31.1

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit via Virtualization]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> 
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 13 +++++++++----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> ebf2f363fbe7..9b0e39b2f022 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *msg)  {
>  	struct virtio_net_config config = {};
> -	u64 features;
> +	u64 features_device, features_driver;
>  	u16 val_u16;
> 
>  	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *ms
>  	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>  		return -EMSGSIZE;
> 
> -	features = vdev->config->get_driver_features(vdev);
> -	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> +	features_driver = vdev->config->get_driver_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> +			      VDPA_ATTR_PAD))
> +		return -EMSGSIZE;
> +
> +	features_device = vdev->config->get_device_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> +features_device,
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> +&config);
>  }
> 
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index
> 25c55cab3d7c..39f1c3d7c112 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -47,6 +47,7 @@ enum vdpa_attr {
>  	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>  	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>  	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> +	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
> 
How is this different than VDPA_ATTR_DEV_SUPPORTED_FEATURES?
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> 
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 13 +++++++++----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> ebf2f363fbe7..9b0e39b2f022 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *msg)  {
>  	struct virtio_net_config config = {};
> -	u64 features;
> +	u64 features_device, features_driver;
>  	u16 val_u16;
> 
>  	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *ms
>  	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>  		return -EMSGSIZE;
> 
> -	features = vdev->config->get_driver_features(vdev);
> -	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> +	features_driver = vdev->config->get_driver_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> +			      VDPA_ATTR_PAD))
> +		return -EMSGSIZE;
> +
> +	features_device = vdev->config->get_device_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> +features_device,
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> +&config);
>  }
> 
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index
> 25c55cab3d7c..39f1c3d7c112 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -47,6 +47,7 @@ enum vdpa_attr {
>  	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>  	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>  	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> +	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
> 
How is this different than VDPA_ATTR_DEV_SUPPORTED_FEATURES?

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit via Virtualization]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> device
> 
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 13 +++++++++----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> ebf2f363fbe7..9b0e39b2f022 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *msg)  {
>  	struct virtio_net_config config = {};
> -	u64 features;
> +	u64 features_device, features_driver;
>  	u16 val_u16;
> 
>  	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *ms
>  	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>  		return -EMSGSIZE;
> 
> -	features = vdev->config->get_driver_features(vdev);
> -	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> +	features_driver = vdev->config->get_driver_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> +			      VDPA_ATTR_PAD))
> +		return -EMSGSIZE;
> +
> +	features_device = vdev->config->get_device_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> +features_device,
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> +&config);
>  }
> 
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index
> 25c55cab3d7c..39f1c3d7c112 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -47,6 +47,7 @@ enum vdpa_attr {
>  	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>  	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>  	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */

I see now what was done incorrectly with commit cd2629f6df1ca.

Above was done with wrong name prefix that missed MGMTDEV_. :(
Please don't add VDPA_ prefix due to one mistake.
Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device attribute as well.

> +	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
> 

>  	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>  	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> --
> 2.31.1

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

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 6:17 AM
> device
> 
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
> 
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 13 +++++++++----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> ebf2f363fbe7..9b0e39b2f022 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *msg)  {
>  	struct virtio_net_config config = {};
> -	u64 features;
> +	u64 features_device, features_driver;
>  	u16 val_u16;
> 
>  	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
> *vdev, struct sk_buff *ms
>  	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>  		return -EMSGSIZE;
> 
> -	features = vdev->config->get_driver_features(vdev);
> -	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> +	features_driver = vdev->config->get_driver_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> +			      VDPA_ATTR_PAD))
> +		return -EMSGSIZE;
> +
> +	features_device = vdev->config->get_device_features(vdev);
> +	if (nla_put_u64_64bit(msg,
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> +features_device,
>  			      VDPA_ATTR_PAD))
>  		return -EMSGSIZE;
> 
> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> +&config);
>  }
> 
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index
> 25c55cab3d7c..39f1c3d7c112 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -47,6 +47,7 @@ enum vdpa_attr {
>  	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>  	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>  	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */

I see now what was done incorrectly with commit cd2629f6df1ca.

Above was done with wrong name prefix that missed MGMTDEV_. :(
Please don't add VDPA_ prefix due to one mistake.
Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device attribute as well.

> +	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
> 

>  	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>  	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> --
> 2.31.1

Re: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Zhu, Lingshan]

On 6/14/2022 4:36 AM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Monday, June 13, 2022 6:17 AM
>> To: jasowang@redhat.com; mst@redhat.com
>> Cc: virtualization@lists.linux-foundation.org; netdev@vger.kernel.org; Parav
>> Pandit <parav@nvidia.com>; xieyongji@bytedance.com;
>> gautam.dawar@amd.com; Zhu Lingshan <lingshan.zhu@intel.com>
>> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace
>> when VIRTIO_NET_F_MQ == 0
>>
>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue pair, so
>> when userspace querying queue pair numbers, it should return mq=1 than
>> zero.
>>
>> Function vdpa_dev_net_config_fill() fills the attributions of the vDPA
>> devices, so that it should call vdpa_dev_net_mq_config_fill() so the
>> parameter in vdpa_dev_net_mq_config_fill() should be feature_device than
>> feature_driver for the vDPA devices themselves
>>
>> Before this change, when MQ = 0, iproute2 output:
>> $vdpa dev config show vdpa0
>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 0
>> mtu 1500
>>
> Max_vq_pairs should not be printed when _MQ feature is not negotiated.
> Existing code that returns 0 is correct following this line of the spec.
>
> " The following driver-read-only field, max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or VIRTIO_-
> NET_F_RSS is set."
> The field doesn't exist when _MQ is not there. Hence, it should not be printed.
> Is _RSS offered and is that why you see it?
>
> If not a fix in the iproute2/vdpa should be done.
IMHO, The spec says:
The following driver-read-only field, max_virtqueue_pairs only exists if 
VIRTIO_NET_F_MQ is *set*

The spec doesn't say:
The following driver-read-only field, max_virtqueue_pairs only exists if 
VIRTIO_NET_F_MQ is *negotiated*

If a device is capable of of multi-queues, this capability does not 
depend on the driver. We are querying the device, not the driver.

If there is MQ, we print the onboard implemented total number of the 
queue pairs.
If MQ is not set, we will not read the onboard mq number, because it is 
not there as the spec says.
But there should be at least one queue pair to be a functional 
virtio-net, so 1 is printed.

Thanks,
Zhu Lingshan

>
>
>> After applying this commit, when MQ = 0, iproute2 output:
>> $vdpa dev config show vdpa0
>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 1
>> mtu 1500
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/vdpa.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>> d76b22b2f7ae..846dd37f3549 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -806,9 +806,10 @@ static int vdpa_dev_net_mq_config_fill(struct
>> vdpa_device *vdev,
>>   	u16 val_u16;
>>
>>   	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
>> -		return 0;
>> +		val_u16 = 1;
>> +	else
>> +		val_u16 = __virtio16_to_cpu(true, config-
>>> max_virtqueue_pairs);
>> -	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
>>   	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP,
>> val_u16);  }
>>
>> @@ -842,7 +843,7 @@ static int vdpa_dev_net_config_fill(struct
>> vdpa_device *vdev, struct sk_buff *ms
>>   			      VDPA_ATTR_PAD))
>>   		return -EMSGSIZE;
>>
>> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
>> &config);
>> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device,
>> +&config);
>>   }
>>
>>   static int
>> --
>> 2.31.1

Re: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Zhu, Lingshan]

On 6/14/2022 4:42 AM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Monday, June 13, 2022 6:17 AM
>> device
>>
>> This commit adds a new vDPA netlink attribution
>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>> features of vDPA devices through this new attr.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/vdpa.c       | 13 +++++++++----
>>   include/uapi/linux/vdpa.h |  1 +
>>   2 files changed, 10 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>> ebf2f363fbe7..9b0e39b2f022 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
>> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct vdpa_device
>> *vdev, struct sk_buff *msg)  {
>>   	struct virtio_net_config config = {};
>> -	u64 features;
>> +	u64 features_device, features_driver;
>>   	u16 val_u16;
>>
>>   	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct vdpa_device
>> *vdev, struct sk_buff *ms
>>   	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>   		return -EMSGSIZE;
>>
>> -	features = vdev->config->get_driver_features(vdev);
>> -	if (nla_put_u64_64bit(msg,
>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
>> +	features_driver = vdev->config->get_driver_features(vdev);
>> +	if (nla_put_u64_64bit(msg,
>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
>> +			      VDPA_ATTR_PAD))
>> +		return -EMSGSIZE;
>> +
>> +	features_device = vdev->config->get_device_features(vdev);
>> +	if (nla_put_u64_64bit(msg,
>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
>> +features_device,
>>   			      VDPA_ATTR_PAD))
>>   		return -EMSGSIZE;
>>
>> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
>> +&config);
>>   }
>>
>>   static int
>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h index
>> 25c55cab3d7c..39f1c3d7c112 100644
>> --- a/include/uapi/linux/vdpa.h
>> +++ b/include/uapi/linux/vdpa.h
>> @@ -47,6 +47,7 @@ enum vdpa_attr {
>>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> I see now what was done incorrectly with commit cd2629f6df1ca.
>
> Above was done with wrong name prefix that missed MGMTDEV_. :(
> Please don't add VDPA_ prefix due to one mistake.
> Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device attribute as well.
currently we can reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to report device 
features for sure,
however this could confuse the readers since every attr should has its 
own unique
purpose.

I think if we add a new attr is a better practice and can avoid some 
potential bugs.
So I suggest we add a new attr for the device features, maybe 
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES
is not a pretty name, any suggestions?

Thanks,
Zhu Lingshan
>
>> +	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
>>
>>   	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>   	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>> --
>> 2.31.1

Re: [PATCH V2 4/6] vDPA: !FEATURES_OK should not block querying device config space

[Zhu, Lingshan]

On 6/14/2022 4:31 AM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Monday, June 13, 2022 6:17 AM
>>
>> Users may want to query the config space of a vDPA device, to choose a
>> appropriate one for a certain guest. This means the users need to read the
>> config space before FEATURES_OK, and the existence of config space
>> contents does not depend on FEATURES_OK.
>>
>> The spec says:
>> The device MUST allow reading of any device-specific configuration field
>> before FEATURES_OK is set by the driver. This includes fields which are
>> conditional on feature bits, as long as those feature bits are offered by the
>> device.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/vdpa.c | 8 --------
>>   1 file changed, 8 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>> 9b0e39b2f022..d76b22b2f7ae 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -851,17 +851,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev,
>> struct sk_buff *msg, u32 portid,  {
>>   	u32 device_id;
>>   	void *hdr;
>> -	u8 status;
>>   	int err;
>>
>>   	down_read(&vdev->cf_lock);
>> -	status = vdev->config->get_status(vdev);
>> -	if (!(status & VIRTIO_CONFIG_S_FEATURES_OK)) {
>> -		NL_SET_ERR_MSG_MOD(extack, "Features negotiation not
>> completed");
>> -		err = -EAGAIN;
>> -		goto out;
>> -	}
>> -
>>   	hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>   			  VDPA_CMD_DEV_CONFIG_GET);
>>   	if (!hdr) {
>> --
>> 2.31.1
> Management interface should not mix up the device state machine checks like above.
> Hence above code removal is better choice.
> Please add fixes tag to this patch.
will do, thanks for your review

Thanks,
Zhu Lingshan
>
> Reviewed-by: Parav Pandit <parav@nvidia.com>

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 10:53 PM
> 
> 
> On 6/14/2022 4:42 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> device
> >>
> >> This commit adds a new vDPA netlink attribution
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features
> >> of vDPA devices through this new attr.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c       | 13 +++++++++----
> >>   include/uapi/linux/vdpa.h |  1 +
> >>   2 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> ebf2f363fbe7..9b0e39b2f022 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> >> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct
> >> vdpa_device *vdev, struct sk_buff *msg)  {
> >>   	struct virtio_net_config config = {};
> >> -	u64 features;
> >> +	u64 features_device, features_driver;
> >>   	u16 val_u16;
> >>
> >>   	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct
> >> vdpa_device *vdev, struct sk_buff *ms
> >>   	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> >>   		return -EMSGSIZE;
> >>
> >> -	features = vdev->config->get_driver_features(vdev);
> >> -	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> >> +	features_driver = vdev->config->get_driver_features(vdev);
> >> +	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> >> +			      VDPA_ATTR_PAD))
> >> +		return -EMSGSIZE;
> >> +
> >> +	features_device = vdev->config->get_device_features(vdev);
> >> +	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> >> +features_device,
> >>   			      VDPA_ATTR_PAD))
> >>   		return -EMSGSIZE;
> >>
> >> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> >> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> >> +&config);
> >>   }
> >>
> >>   static int
> >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> >> index
> >> 25c55cab3d7c..39f1c3d7c112 100644
> >> --- a/include/uapi/linux/vdpa.h
> >> +++ b/include/uapi/linux/vdpa.h
> >> @@ -47,6 +47,7 @@ enum vdpa_attr {
> >>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> > I see now what was done incorrectly with commit cd2629f6df1ca.
> >
> > Above was done with wrong name prefix that missed MGMTDEV_. :(
> Please
> > don't add VDPA_ prefix due to one mistake.
> > Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device
> attribute as well.
> currently we can reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to report
> device features for sure, however this could confuse the readers since every
> attr should has its own unique purpose.
VDPA_ATTR_DEV_SUPPORTED_FEATURES is supposed to be VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES
And device specific features is supposed to be named as,
VDPA_ATTR_DEV_SUPPORTED_FEATURES.

But it was not done this way in commit cd2629f6df1ca.
This leads to the finding good name problem now.

Given that this new attribute is going to show same or subset of the features of the management device supported features, it is fine to reuse with exception with explicit comment in the UAPI header file.

RE: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit via Virtualization]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 10:53 PM
> 
> 
> On 6/14/2022 4:42 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> device
> >>
> >> This commit adds a new vDPA netlink attribution
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features
> >> of vDPA devices through this new attr.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c       | 13 +++++++++----
> >>   include/uapi/linux/vdpa.h |  1 +
> >>   2 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
> >> ebf2f363fbe7..9b0e39b2f022 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
> >> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct
> >> vdpa_device *vdev, struct sk_buff *msg)  {
> >>   	struct virtio_net_config config = {};
> >> -	u64 features;
> >> +	u64 features_device, features_driver;
> >>   	u16 val_u16;
> >>
> >>   	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
> >> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct
> >> vdpa_device *vdev, struct sk_buff *ms
> >>   	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> >>   		return -EMSGSIZE;
> >>
> >> -	features = vdev->config->get_driver_features(vdev);
> >> -	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
> >> +	features_driver = vdev->config->get_driver_features(vdev);
> >> +	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
> >> +			      VDPA_ATTR_PAD))
> >> +		return -EMSGSIZE;
> >> +
> >> +	features_device = vdev->config->get_device_features(vdev);
> >> +	if (nla_put_u64_64bit(msg,
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> >> +features_device,
> >>   			      VDPA_ATTR_PAD))
> >>   		return -EMSGSIZE;
> >>
> >> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> >> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
> >> +&config);
> >>   }
> >>
> >>   static int
> >> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> >> index
> >> 25c55cab3d7c..39f1c3d7c112 100644
> >> --- a/include/uapi/linux/vdpa.h
> >> +++ b/include/uapi/linux/vdpa.h
> >> @@ -47,6 +47,7 @@ enum vdpa_attr {
> >>   	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
> >>   	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
> >>   	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
> > I see now what was done incorrectly with commit cd2629f6df1ca.
> >
> > Above was done with wrong name prefix that missed MGMTDEV_. :(
> Please
> > don't add VDPA_ prefix due to one mistake.
> > Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device
> attribute as well.
> currently we can reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to report
> device features for sure, however this could confuse the readers since every
> attr should has its own unique purpose.
VDPA_ATTR_DEV_SUPPORTED_FEATURES is supposed to be VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES
And device specific features is supposed to be named as,
VDPA_ATTR_DEV_SUPPORTED_FEATURES.

But it was not done this way in commit cd2629f6df1ca.
This leads to the finding good name problem now.

Given that this new attribute is going to show same or subset of the features of the management device supported features, it is fine to reuse with exception with explicit comment in the UAPI header file.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

RE: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 10:33 PM
> 
> 
> On 6/14/2022 4:36 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> To: jasowang@redhat.com; mst@redhat.com
> >> Cc: virtualization@lists.linux-foundation.org;
> >> netdev@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> >> xieyongji@bytedance.com; gautam.dawar@amd.com; Zhu Lingshan
> >> <lingshan.zhu@intel.com>
> >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to
> >> userspace when VIRTIO_NET_F_MQ == 0
> >>
> >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue
> >> pair, so when userspace querying queue pair numbers, it should return
> >> mq=1 than zero.
> >>
> >> Function vdpa_dev_net_config_fill() fills the attributions of the
> >> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so
> >> the parameter in vdpa_dev_net_mq_config_fill() should be
> >> feature_device than feature_driver for the vDPA devices themselves
> >>
> >> Before this change, when MQ = 0, iproute2 output:
> >> $vdpa dev config show vdpa0
> >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
> >> 0 mtu 1500
> >>
> > Max_vq_pairs should not be printed when _MQ feature is not negotiated.
> > Existing code that returns 0 is correct following this line of the spec.
> >
> > " The following driver-read-only field, max_virtqueue_pairs only
> > exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set."
> > The field doesn't exist when _MQ is not there. Hence, it should not be
> printed.
> > Is _RSS offered and is that why you see it?
> >
> > If not a fix in the iproute2/vdpa should be done.
> IMHO, The spec says:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *set*
> 
> The spec doesn't say:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *negotiated*
> 
> If a device is capable of of multi-queues, this capability does not depend on
> the driver. We are querying the device, not the driver.
> 
> If there is MQ, we print the onboard implemented total number of the
> queue pairs.
> If MQ is not set, we will not read the onboard mq number, because it is not
> there as the spec says.
> But there should be at least one queue pair to be a functional virtio-net, so 1
> is printed.

The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly.

The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :(
commit [2] was bug.

The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features().

Once above fix is applied,
when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
We do not have a concept of management driver yet in the specification.
So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1.
(Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1."

So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer.
I agree that this extra if() condition in the user space can be avoided if kernel always provides it.
But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered.
So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs.

Please submit the fix for [2] to call get_device_features() for purpose of returning config space.
Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES.

I didn’t review these patches and it slipped through the cracks. :(

[1] a64917bc2e9b ("vdpa: Introduce query of device config layout")
[2] a64917bc2e9 ("vdpa: Provide interface to read driver features")

RE: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Parav Pandit via Virtualization]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Monday, June 13, 2022 10:33 PM
> 
> 
> On 6/14/2022 4:36 AM, Parav Pandit wrote:
> >
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Monday, June 13, 2022 6:17 AM
> >> To: jasowang@redhat.com; mst@redhat.com
> >> Cc: virtualization@lists.linux-foundation.org;
> >> netdev@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
> >> xieyongji@bytedance.com; gautam.dawar@amd.com; Zhu Lingshan
> >> <lingshan.zhu@intel.com>
> >> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to
> >> userspace when VIRTIO_NET_F_MQ == 0
> >>
> >> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue
> >> pair, so when userspace querying queue pair numbers, it should return
> >> mq=1 than zero.
> >>
> >> Function vdpa_dev_net_config_fill() fills the attributions of the
> >> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so
> >> the parameter in vdpa_dev_net_mq_config_fill() should be
> >> feature_device than feature_driver for the vDPA devices themselves
> >>
> >> Before this change, when MQ = 0, iproute2 output:
> >> $vdpa dev config show vdpa0
> >> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
> >> 0 mtu 1500
> >>
> > Max_vq_pairs should not be printed when _MQ feature is not negotiated.
> > Existing code that returns 0 is correct following this line of the spec.
> >
> > " The following driver-read-only field, max_virtqueue_pairs only
> > exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set."
> > The field doesn't exist when _MQ is not there. Hence, it should not be
> printed.
> > Is _RSS offered and is that why you see it?
> >
> > If not a fix in the iproute2/vdpa should be done.
> IMHO, The spec says:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *set*
> 
> The spec doesn't say:
> The following driver-read-only field, max_virtqueue_pairs only exists if
> VIRTIO_NET_F_MQ is *negotiated*
> 
> If a device is capable of of multi-queues, this capability does not depend on
> the driver. We are querying the device, not the driver.
> 
> If there is MQ, we print the onboard implemented total number of the
> queue pairs.
> If MQ is not set, we will not read the onboard mq number, because it is not
> there as the spec says.
> But there should be at least one queue pair to be a functional virtio-net, so 1
> is printed.

The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly.

The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :(
commit [2] was bug.

The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features().

Once above fix is applied,
when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
We do not have a concept of management driver yet in the specification.
So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1.
(Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1."

So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer.
I agree that this extra if() condition in the user space can be avoided if kernel always provides it.
But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered.
So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs.

Please submit the fix for [2] to call get_device_features() for purpose of returning config space.
Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES.

I didn’t review these patches and it slipped through the cracks. :(

[1] a64917bc2e9b ("vdpa: Introduce query of device config layout")
[2] a64917bc2e9 ("vdpa: Provide interface to read driver features")
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Zhu, Lingshan]

On 6/15/2022 3:26 AM, Parav Pandit wrote:
>
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Monday, June 13, 2022 10:33 PM
>>
>>
>> On 6/14/2022 4:36 AM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Monday, June 13, 2022 6:17 AM
>>>> To: jasowang@redhat.com; mst@redhat.com
>>>> Cc: virtualization@lists.linux-foundation.org;
>>>> netdev@vger.kernel.org; Parav Pandit <parav@nvidia.com>;
>>>> xieyongji@bytedance.com; gautam.dawar@amd.com; Zhu Lingshan
>>>> <lingshan.zhu@intel.com>
>>>> Subject: [PATCH V2 5/6] vDPA: answer num of queue pairs = 1 to
>>>> userspace when VIRTIO_NET_F_MQ == 0
>>>>
>>>> If VIRTIO_NET_F_MQ == 0, the virtio device should have one queue
>>>> pair, so when userspace querying queue pair numbers, it should return
>>>> mq=1 than zero.
>>>>
>>>> Function vdpa_dev_net_config_fill() fills the attributions of the
>>>> vDPA devices, so that it should call vdpa_dev_net_mq_config_fill() so
>>>> the parameter in vdpa_dev_net_mq_config_fill() should be
>>>> feature_device than feature_driver for the vDPA devices themselves
>>>>
>>>> Before this change, when MQ = 0, iproute2 output:
>>>> $vdpa dev config show vdpa0
>>>> vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs
>>>> 0 mtu 1500
>>>>
>>> Max_vq_pairs should not be printed when _MQ feature is not negotiated.
>>> Existing code that returns 0 is correct following this line of the spec.
>>>
>>> " The following driver-read-only field, max_virtqueue_pairs only
>>> exists if VIRTIO_NET_F_MQ or VIRTIO_- NET_F_RSS is set."
>>> The field doesn't exist when _MQ is not there. Hence, it should not be
>> printed.
>>> Is _RSS offered and is that why you see it?
>>>
>>> If not a fix in the iproute2/vdpa should be done.
>> IMHO, The spec says:
>> The following driver-read-only field, max_virtqueue_pairs only exists if
>> VIRTIO_NET_F_MQ is *set*
>>
>> The spec doesn't say:
>> The following driver-read-only field, max_virtqueue_pairs only exists if
>> VIRTIO_NET_F_MQ is *negotiated*
>>
>> If a device is capable of of multi-queues, this capability does not depend on
>> the driver. We are querying the device, not the driver.
>>
>> If there is MQ, we print the onboard implemented total number of the
>> queue pairs.
>> If MQ is not set, we will not read the onboard mq number, because it is not
>> there as the spec says.
>> But there should be at least one queue pair to be a functional virtio-net, so 1
>> is printed.
> The commit [1] is supposed to show the device configuration layout as what device _offers_ as mentioned in the subject line of the commit [1] very clearly.
>
> The commit [2] changed the original intent of commit [1] even though commit [2] mentioned "fixes" in the patch without any fixes tag. :(
> commit [2] was bug.
>
> The right fix to restore [1] is to report the device features offered using get_device_features() callback instead of get_drivers_features().
already done in 3/6 patch in this series, see:
  static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct 
sk_buff *msg)
  {
         struct virtio_net_config config = {};
-       u64 features;
+       u64 features_device, features_driver;
         u16 val_u16;

and
-       features = vdev->config->get_driver_features(vdev);
-       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features,
+       features_driver = vdev->config->get_driver_features(vdev);
+       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features_driver,
+                             VDPA_ATTR_PAD))
+               return -EMSGSIZE;
+
+       features_device = vdev->config->get_device_features(vdev);
+       if (nla_put_u64_64bit(msg, 
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;

Features of the device and the driver are reported separately.

>
> Once above fix is applied,
> when _MQ is not offered, max_queue_pairs should be treated as 1 by the driver.
Yes, it is done by this patch.
> We do not have a concept of management driver yet in the specification.
> So when _MQ is not offered by the device, either kernel driver can return max_queue_pairs = 1, or the user space caller can see that _MQ is not offered by the device hence treat max_vq_pairs = 1.
> (Like how it is described in the spec as - " Identify and initialize the receive and transmission virtqueues, up to N of each kind. If VIRTIO_NET_F_MQ feature bit is negotiated, N=max_virtqueue_pairs, otherwise identify N=1."
Yes, that's why I set max_queue_pairs = 1 if MQ = 0 in this patch.
>
> So let orchestration layer can certainly derive this N when _MQ feature is not offered, instead of coming from the vdpa management layer.
> I agree that this extra if() condition in the user space can be avoided if kernel always provides it.
> But better to avoid such assumption because we have more such config space attributes. i.e., they exist only when features are offered.
> So to keep it uniform, I prefer we avoid the exception for max_virtqueue_pairs.
>
> Please submit the fix for [2] to call get_device_features() for purpose of returning config space.
> Continue to use get_driver_features() to show VDPA_ATTR_DEV_NEGOTIATED_FEATURES.
This is already there in 3/6 patch, we have these code to report driver 
features:
         features_driver = vdev->config->get_driver_features(vdev);
         if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, 
features_driver,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;

And these code to report device features:
         features_device = vdev->config->get_device_features(vdev);
         if (nla_put_u64_64bit(msg, 
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, features_device,
                               VDPA_ATTR_PAD))
                 return -EMSGSIZE;



>
> I didn’t review these patches and it slipped through the cracks. :(
>
> [1] a64917bc2e9b ("vdpa: Introduce query of device config layout")
> [2] a64917bc2e9 ("vdpa: Provide interface to read driver features")

Re: [PATCH V2 3/6] vDPA: allow userspace to query features of a vDPA device

[Zhu, Lingshan]

On 6/15/2022 2:38 AM, Parav Pandit wrote:
>
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Monday, June 13, 2022 10:53 PM
>>
>>
>> On 6/14/2022 4:42 AM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Monday, June 13, 2022 6:17 AM
>>>> device
>>>>
>>>> This commit adds a new vDPA netlink attribution
>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>> features
>>>> of vDPA devices through this new attr.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c       | 13 +++++++++----
>>>>    include/uapi/linux/vdpa.h |  1 +
>>>>    2 files changed, 10 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index
>>>> ebf2f363fbe7..9b0e39b2f022 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -815,7 +815,7 @@ static int vdpa_dev_net_mq_config_fill(struct
>>>> vdpa_device *vdev,  static int vdpa_dev_net_config_fill(struct
>>>> vdpa_device *vdev, struct sk_buff *msg)  {
>>>>    	struct virtio_net_config config = {};
>>>> -	u64 features;
>>>> +	u64 features_device, features_driver;
>>>>    	u16 val_u16;
>>>>
>>>>    	vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config)); @@ -
>>>> 832,12 +832,17 @@ static int vdpa_dev_net_config_fill(struct
>>>> vdpa_device *vdev, struct sk_buff *ms
>>>>    	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>>>    		return -EMSGSIZE;
>>>>
>>>> -	features = vdev->config->get_driver_features(vdev);
>>>> -	if (nla_put_u64_64bit(msg,
>>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
>>>> +	features_driver = vdev->config->get_driver_features(vdev);
>>>> +	if (nla_put_u64_64bit(msg,
>>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
>>>> +			      VDPA_ATTR_PAD))
>>>> +		return -EMSGSIZE;
>>>> +
>>>> +	features_device = vdev->config->get_device_features(vdev);
>>>> +	if (nla_put_u64_64bit(msg,
>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
>>>> +features_device,
>>>>    			      VDPA_ATTR_PAD))
>>>>    		return -EMSGSIZE;
>>>>
>>>> -	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>>>> +	return vdpa_dev_net_mq_config_fill(vdev, msg, features_driver,
>>>> +&config);
>>>>    }
>>>>
>>>>    static int
>>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>>> index
>>>> 25c55cab3d7c..39f1c3d7c112 100644
>>>> --- a/include/uapi/linux/vdpa.h
>>>> +++ b/include/uapi/linux/vdpa.h
>>>> @@ -47,6 +47,7 @@ enum vdpa_attr {
>>>>    	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
>>>>    	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
>>>>    	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
>>> I see now what was done incorrectly with commit cd2629f6df1ca.
>>>
>>> Above was done with wrong name prefix that missed MGMTDEV_. :(
>> Please
>>> don't add VDPA_ prefix due to one mistake.
>>> Please reuse this VDPA_ATTR_DEV_SUPPORTED_FEATURES for device
>> attribute as well.
>> currently we can reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES to report
>> device features for sure, however this could confuse the readers since every
>> attr should has its own unique purpose.
> VDPA_ATTR_DEV_SUPPORTED_FEATURES is supposed to be VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES
> And device specific features is supposed to be named as,
> VDPA_ATTR_DEV_SUPPORTED_FEATURES.
Yes, that's the point and what I did in my V1 series, but you see if  we 
change original VDPA_ATTR_DEV_SUPPORTED_FEATURES to 
VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES, uAPI has been changed, so iproute2 
may work improperly.
> But it was not done this way in commit cd2629f6df1ca.
> This leads to the finding good name problem now.
>
> Given that this new attribute is going to show same or subset of the features of the management device supported features, it is fine to reuse with exception with explicit comment in the UAPI header file.
Currently I don't see any bugs can be caused by reuse the attr 
VDPA_ATTR_DEV_SUPPORTED_FEATURES.
However I really prefer to do defensive coding, introduce a new attr is 
not a big burden, its a common practice using a dedicated attr for an 
unique type of data, this can help us avoid potential bugs, and less 
confusion for other contributors.

VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES is still better than a ATTR2, but maybe it is not pretty enough, any better namings are more than welcome!!!!!