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

[PATCH V4 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 and etc.

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 V3:
(1)drop the fixes tags(Parva)
(2)better commit log for patch 1/6(Michael)
(3)assign num_queues to max_supported_vqs than max_vq_pairs(Jason)
(4)initialize virtio pci capabilities in the probe() function.

Changes from V2:
Add fixes tags(Parva)

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 |  13 ++-
 drivers/vdpa/ifcvf/ifcvf_base.h |   2 +
 drivers/vdpa/ifcvf/ifcvf_main.c | 142 +++++++++++++++++---------------
 drivers/vdpa/vdpa.c             |  32 ++++---
 include/uapi/linux/vdpa.h       |   1 +
 5 files changed, 105 insertions(+), 85 deletions(-)

-- 
2.31.1

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

[Zhu Lingshan]

Drivers must not access a BAR outside the capability length,
and for a virtio device, ifcvf driver should not report any non-standard
capability contents to the upper layers.

Function ifcvf_get_config_size() is introduced here to return a safe value
of the device config capability 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..85611be5ccb4 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 = min(cap_size, net_config_size);
 		break;
 	case VIRTIO_ID_BLOCK:
-		config_size = sizeof(struct virtio_blk_config);
+		config_size = min(cap_size, blk_config_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 V4 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.

Currently both the vDPA device and the management device are the VF itself,
thus this ifcvf should initialize the virtio capabilities in probe() before
setting up the struct vdpa_mgmt_dev.

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

diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c
index 0a5670729412..3fd0267873f8 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -752,59 +752,36 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name,
 {
 	struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev;
 	struct ifcvf_adapter *adapter;
+	struct vdpa_device *vdpa_dev;
 	struct pci_dev *pdev;
 	struct ifcvf_hw *vf;
-	struct device *dev;
-	int ret, i;
+	int ret;
 
 	ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev);
-	if (ifcvf_mgmt_dev->adapter)
+	if (!ifcvf_mgmt_dev->adapter)
 		return -EOPNOTSUPP;
 
-	pdev = ifcvf_mgmt_dev->pdev;
-	dev = &pdev->dev;
-	adapter = vdpa_alloc_device(struct ifcvf_adapter, vdpa,
-				    dev, &ifc_vdpa_ops, 1, 1, name, false);
-	if (IS_ERR(adapter)) {
-		IFCVF_ERR(pdev, "Failed to allocate vDPA structure");
-		return PTR_ERR(adapter);
-	}
-
-	ifcvf_mgmt_dev->adapter = adapter;
-
+	adapter = ifcvf_mgmt_dev->adapter;
 	vf = &adapter->vf;
-	vf->dev_type = get_dev_type(pdev);
-	vf->base = pcim_iomap_table(pdev);
+	pdev = adapter->pdev;
+	vdpa_dev = &adapter->vdpa;
 
-	adapter->pdev = pdev;
-	adapter->vdpa.dma_dev = &pdev->dev;
-
-	ret = ifcvf_init_hw(vf, pdev);
-	if (ret) {
-		IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
-		goto err;
-	}
-
-	for (i = 0; i < vf->nr_vring; i++)
-		vf->vring[i].irq = -EINVAL;
-
-	vf->hw_features = ifcvf_get_hw_features(vf);
-	vf->config_size = ifcvf_get_config_size(vf);
+	if (name)
+		ret = dev_set_name(&vdpa_dev->dev, "%s", name);
+	else
+		ret = dev_set_name(&vdpa_dev->dev, "vdpa%u", vdpa_dev->index);
 
-	adapter->vdpa.mdev = &ifcvf_mgmt_dev->mdev;
 	ret = _vdpa_register_device(&adapter->vdpa, vf->nr_vring);
 	if (ret) {
+		put_device(&adapter->vdpa.dev);
 		IFCVF_ERR(pdev, "Failed to register to vDPA bus");
-		goto err;
+		return ret;
 	}
 
 	return 0;
-
-err:
-	put_device(&adapter->vdpa.dev);
-	return ret;
 }
 
+
 static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev)
 {
 	struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev;
@@ -823,61 +800,94 @@ static int ifcvf_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev;
 	struct device *dev = &pdev->dev;
+	struct ifcvf_adapter *adapter;
+	struct ifcvf_hw *vf;
 	u32 dev_type;
-	int ret;
-
-	ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
-	if (!ifcvf_mgmt_dev) {
-		IFCVF_ERR(pdev, "Failed to alloc memory for the vDPA management device\n");
-		return -ENOMEM;
-	}
-
-	dev_type = get_dev_type(pdev);
-	switch (dev_type) {
-	case VIRTIO_ID_NET:
-		ifcvf_mgmt_dev->mdev.id_table = id_table_net;
-		break;
-	case VIRTIO_ID_BLOCK:
-		ifcvf_mgmt_dev->mdev.id_table = id_table_blk;
-		break;
-	default:
-		IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", dev_type);
-		ret = -EOPNOTSUPP;
-		goto err;
-	}
-
-	ifcvf_mgmt_dev->mdev.ops = &ifcvf_vdpa_mgmt_dev_ops;
-	ifcvf_mgmt_dev->mdev.device = dev;
-	ifcvf_mgmt_dev->pdev = pdev;
+	int ret, i;
 
 	ret = pcim_enable_device(pdev);
 	if (ret) {
 		IFCVF_ERR(pdev, "Failed to enable device\n");
-		goto err;
+		return ret;
 	}
-
 	ret = pcim_iomap_regions(pdev, BIT(0) | BIT(2) | BIT(4),
 				 IFCVF_DRIVER_NAME);
 	if (ret) {
 		IFCVF_ERR(pdev, "Failed to request MMIO region\n");
-		goto err;
+		return ret;
 	}
 
 	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(64));
 	if (ret) {
 		IFCVF_ERR(pdev, "No usable DMA configuration\n");
-		goto err;
+		return ret;
 	}
 
 	ret = devm_add_action_or_reset(dev, ifcvf_free_irq_vectors, pdev);
 	if (ret) {
 		IFCVF_ERR(pdev,
 			  "Failed for adding devres for freeing irq vectors\n");
-		goto err;
+		return ret;
 	}
 
 	pci_set_master(pdev);
 
+	adapter = vdpa_alloc_device(struct ifcvf_adapter, vdpa,
+				    dev, &ifc_vdpa_ops, 1, 1, NULL, false);
+	if (IS_ERR(adapter)) {
+		IFCVF_ERR(pdev, "Failed to allocate vDPA structure");
+		return PTR_ERR(adapter);
+	}
+
+	vf = &adapter->vf;
+	vf->dev_type = get_dev_type(pdev);
+	vf->base = pcim_iomap_table(pdev);
+
+	adapter->pdev = pdev;
+	adapter->vdpa.dma_dev = &pdev->dev;
+
+	ret = ifcvf_init_hw(vf, pdev);
+	if (ret) {
+		IFCVF_ERR(pdev, "Failed to init IFCVF hw\n");
+		return ret;
+	}
+
+	for (i = 0; i < vf->nr_vring; i++)
+		vf->vring[i].irq = -EINVAL;
+
+	vf->hw_features = ifcvf_get_hw_features(vf);
+	vf->config_size = ifcvf_get_config_size(vf);
+
+	ifcvf_mgmt_dev = kzalloc(sizeof(struct ifcvf_vdpa_mgmt_dev), GFP_KERNEL);
+	if (!ifcvf_mgmt_dev) {
+		IFCVF_ERR(pdev, "Failed to alloc memory for the vDPA management device\n");
+		return -ENOMEM;
+	}
+
+	ifcvf_mgmt_dev->mdev.ops = &ifcvf_vdpa_mgmt_dev_ops;
+	ifcvf_mgmt_dev->mdev.device = dev;
+	ifcvf_mgmt_dev->adapter = adapter;
+
+	dev_type = get_dev_type(pdev);
+	switch (dev_type) {
+	case VIRTIO_ID_NET:
+		ifcvf_mgmt_dev->mdev.id_table = id_table_net;
+		break;
+	case VIRTIO_ID_BLOCK:
+		ifcvf_mgmt_dev->mdev.id_table = id_table_blk;
+		break;
+	default:
+		IFCVF_ERR(pdev, "VIRTIO ID %u not supported\n", dev_type);
+		ret = -EOPNOTSUPP;
+		goto err;
+	}
+
+	ifcvf_mgmt_dev->mdev.max_supported_vqs = vf->nr_vring;
+	ifcvf_mgmt_dev->mdev.supported_features = vf->hw_features;
+
+	adapter->vdpa.mdev = &ifcvf_mgmt_dev->mdev;
+
+
 	ret = vdpa_mgmtdev_register(&ifcvf_mgmt_dev->mdev);
 	if (ret) {
 		IFCVF_ERR(pdev,
-- 
2.31.1

[PATCH V4 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 V4 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 V4 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 V4 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>
Reviewed-by: Parav Pandit <parav@nvidia.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 V4 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit]

> From: Zhu Lingshan <lingshan.zhu@intel.com>
> Sent: Friday, July 22, 2022 7:53 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 */
> 
I have answered in previous emails.
I disagree with the change.
Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.

MST,
I nack this patch.
As mentioned in the previous versions, also it is missing the example output in the commit log.
Please include example output.

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

RE: [PATCH V4 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: Friday, July 22, 2022 7:53 AM
> 
> 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
> 
No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
Please drop this patch.
Nack on this patch.

> 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 V4 3/6] vDPA: allow userspace to query features of a vDPA device

[Zhu, Lingshan]

On 7/22/2022 9:12 PM, Parav Pandit wrote:
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Friday, July 22, 2022 7:53 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 */
>>
> I have answered in previous emails.
> I disagree with the change.
> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
I believe we have already discussed this before in the V3 thread.
I have told you that reusing this attr will lead to a new race condition.

Pleas refer to the previous thread, and you did not answer my questions 
in that thread.

Thanks,
Zhu Lingshan
>
> MST,
> I nack this patch.
> As mentioned in the previous versions, also it is missing the example output in the commit log.
> Please include example output.
>
>>   	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>   	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>> --
>> 2.31.1

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

[Zhu, Lingshan]

On 7/22/2022 9:14 PM, Parav Pandit wrote:
>
>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>> Sent: Friday, July 22, 2022 7:53 AM
>>
>> 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
>>
> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
> Please drop this patch.
> Nack on this patch.
I believe MST has replied to you in the V3 thread, did you miss that?
>
>> 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 V4 3/6] vDPA: allow userspace to query features of a vDPA device

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Saturday, July 23, 2022 7:24 AM
> 
> 
> On 7/22/2022 9:12 PM, Parav Pandit wrote:
> >> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >> Sent: Friday, July 22, 2022 7:53 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 */
> >>
> > I have answered in previous emails.
> > I disagree with the change.
> > Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
> I believe we have already discussed this before in the V3 thread.
> I have told you that reusing this attr will lead to a new race condition.
>
Returning attribute cannot lead to any race condition.

 
> Pleas refer to the previous thread, and you did not answer my questions in
> that thread.
> 
> Thanks,
> Zhu Lingshan
> >
> > MST,
> > I nack this patch.
> > As mentioned in the previous versions, also it is missing the example
> output in the commit log.
> > Please include example output.
> >
> >>   	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
> >>   	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
> >> --
> >> 2.31.1

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

[Zhu, Lingshan]

On 7/24/2022 11:21 PM, Parav Pandit wrote:
>
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Saturday, July 23, 2022 7:24 AM
>>
>>
>> On 7/22/2022 9:12 PM, Parav Pandit wrote:
>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Friday, July 22, 2022 7:53 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 */
>>>>
>>> I have answered in previous emails.
>>> I disagree with the change.
>>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
>> I believe we have already discussed this before in the V3 thread.
>> I have told you that reusing this attr will lead to a new race condition.
>>
> Returning attribute cannot lead to any race condition.
Please refer to our discussion in the V3 series, I have explained
if re-use this attr, it will be a multiple consumers and multiple 
produces model,
it is a typical racing condition.

>
>   
>> Pleas refer to the previous thread, and you did not answer my questions in
>> that thread.
>>
>> Thanks,
>> Zhu Lingshan
>>> MST,
>>> I nack this patch.
>>> As mentioned in the previous versions, also it is missing the example
>> output in the commit log.
>>> Please include example output.
>>>
>>>>    	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
>>>>    	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
>>>> --
>>>> 2.31.1

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

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Tuesday, July 26, 2022 7:03 AM
> 
> On 7/24/2022 11:21 PM, Parav Pandit wrote:
> >
> >> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> >> Sent: Saturday, July 23, 2022 7:24 AM
> >>
> >>
> >> On 7/22/2022 9:12 PM, Parav Pandit wrote:
> >>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> Sent: Friday, July 22, 2022 7:53 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 */
> >>>>
> >>> I have answered in previous emails.
> >>> I disagree with the change.
> >>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
> >> I believe we have already discussed this before in the V3 thread.
> >> I have told you that reusing this attr will lead to a new race condition.
> >>
> > Returning attribute cannot lead to any race condition.
> Please refer to our discussion in the V3 series, I have explained if re-use this
> attr, it will be a multiple consumers and multiple produces model, it is a
> typical racing condition.

I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA device"
I couldn’t find multiple consumers multiple producers working on same nla message.

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

[Zhu, Lingshan]

On 7/26/2022 7:06 PM, Parav Pandit wrote:
>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>> Sent: Tuesday, July 26, 2022 7:03 AM
>>
>> On 7/24/2022 11:21 PM, Parav Pandit wrote:
>>>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
>>>> Sent: Saturday, July 23, 2022 7:24 AM
>>>>
>>>>
>>>> On 7/22/2022 9:12 PM, Parav Pandit wrote:
>>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> Sent: Friday, July 22, 2022 7:53 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 */
>>>>>>
>>>>> I have answered in previous emails.
>>>>> I disagree with the change.
>>>>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
>>>> I believe we have already discussed this before in the V3 thread.
>>>> I have told you that reusing this attr will lead to a new race condition.
>>>>
>>> Returning attribute cannot lead to any race condition.
>> Please refer to our discussion in the V3 series, I have explained if re-use this
>> attr, it will be a multiple consumers and multiple produces model, it is a
>> typical racing condition.
> I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace to query features of a vDPA device"
> I couldn’t find multiple consumers multiple producers working on same nla message.
If this attr is reused, then there can be multiple iproute2 instances or 
other applications querying feature bits of the management device and 
the vDPA device simultaneously,
and both kernel side management feature bits filler function and vDPA 
device feature bits filler function can write the NLA message at the 
same time. That's the multiple
consumers and producers, and no locks

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

[Michael S. Tsirkin]

On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote:
> 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>


I think at least some discussion and documentation
of this attribute versus VDPA_ATTR_DEV_SUPPORTED_FEATURES
is called for.

Otherwise how do people know which one to use?

We can't send everyone to go read the lkml thread.

> ---
>  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

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

[Parav Pandit]

> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> Sent: Wednesday, July 27, 2022 2:02 AM
> 
> 
> On 7/26/2022 7:06 PM, Parav Pandit wrote:
> >> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> >> Sent: Tuesday, July 26, 2022 7:03 AM
> >>
> >> On 7/24/2022 11:21 PM, Parav Pandit wrote:
> >>>> From: Zhu, Lingshan <lingshan.zhu@intel.com>
> >>>> Sent: Saturday, July 23, 2022 7:24 AM
> >>>>
> >>>>
> >>>> On 7/22/2022 9:12 PM, Parav Pandit wrote:
> >>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>>>> Sent: Friday, July 22, 2022 7:53 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 */
> >>>>>>
> >>>>> I have answered in previous emails.
> >>>>> I disagree with the change.
> >>>>> Please reuse VDPA_ATTR_DEV_SUPPORTED_FEATURES.
> >>>> I believe we have already discussed this before in the V3 thread.
> >>>> I have told you that reusing this attr will lead to a new race condition.
> >>>>
> >>> Returning attribute cannot lead to any race condition.
> >> Please refer to our discussion in the V3 series, I have explained if
> >> re-use this attr, it will be a multiple consumers and multiple
> >> produces model, it is a typical racing condition.
> > I read the emails with subject = " Re: [PATCH V3 3/6] vDPA: allow userspace
> to query features of a vDPA device"
> > I couldn’t find multiple consumers multiple producers working on same nla
> message.
> If this attr is reused, then there can be multiple iproute2 instances or other
> applications querying feature bits of the management device and the vDPA
> device simultaneously, and both kernel side management feature bits filler

> function and vDPA device feature bits filler function can write the NLA
> message at the same time. That's the multiple consumers and producers,
> and no locks
No. Each filling up happens in each process context. There is no race here.

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

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, August 9, 2022 3:24 PM
> 
> On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote:
> > 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>
> 
> 
> I think at least some discussion and documentation of this attribute versus
> VDPA_ATTR_DEV_SUPPORTED_FEATURES is called for.
> 
> Otherwise how do people know which one to use?
> 
> We can't send everyone to go read the lkml thread.

There is no race here. Commit log is missing example anyway.
I explained multiple times that this patch and/or its previous version cannot proceed in same state.

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

[Michael S. Tsirkin]

On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote:
> 
> 
> > From: Zhu Lingshan <lingshan.zhu@intel.com>
> > Sent: Friday, July 22, 2022 7:53 AM
> > 
> > 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
> > 
> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
> Please drop this patch.
> Nack on this patch.

Wrt this patch as far as I'm concerned you guys are bikeshedding.

Parav generally don't send nacks please they are not helpful.

Zhu Lingshan please always address comments in some way.  E.g. refer to
them in the commit log explaining the motivation and the alternatives.
I know you don't agree with Parav but ghosting isn't nice.

I still feel what we should have done is
not return a value, as long as we do return it we might
as well return something reasonable, not 0.

And I like it that this fixes sparse warning actually:
max_virtqueue_pairs it tagged as __virtio, not __le.

However, I am worried there is another bug here:
this is checking driver features. But really max vqs
should not depend on that, it depends on device
features, no?



> > 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 V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, August 9, 2022 3:36 PM

> > > 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
> > >
> > No. We do not want to diverge returning values of config space for fields
> which are not present as discussed in previous versions.
> > Please drop this patch.
> > Nack on this patch.
> 
> Wrt this patch as far as I'm concerned you guys are bikeshedding.
> 
> Parav generally don't send nacks please they are not helpful.

Ok. I explained the technical reasoning that we don't diverge in fields. All are linked to the respective feature bits uniformly.
This I explained repeatedly in almost v1 to v3.
And reporting 1 is mis-leading, because it says _MQ is not negotiated, how come this device tells its config space has max_qp = 1.

But if you want to proceed to diverge kernel on feature bits go ahead. It requires inspection feature but feature.
I just don't see how this can be well maintained.

Commit log doesn't even describe the weird use case that says "as user space, I do not want to read device feature bits and just want to read config space to decide...".
Odd..

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

[Si-Wei Liu]

On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote:
> On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote:
>>
>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>> Sent: Friday, July 22, 2022 7:53 AM
>>>
>>> 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
>>>
>> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
>> Please drop this patch.
>> Nack on this patch.
> Wrt this patch as far as I'm concerned you guys are bikeshedding.
>
> Parav generally don't send nacks please they are not helpful.
>
> Zhu Lingshan please always address comments in some way.  E.g. refer to
> them in the commit log explaining the motivation and the alternatives.
> I know you don't agree with Parav but ghosting isn't nice.
>
> I still feel what we should have done is
> not return a value, as long as we do return it we might
> as well return something reasonable, not 0.
Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is 
not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply 
not there, but userspace (iproute) mistakenly puts a zero value there. 
This is a pattern every tool in iproute follows consistently by large. I 
don't get why kernel has to return something without seeing a very 
convincing use case?

Not to mention spec doesn't give us explicit definition for when the 
field in config space becomes valid and/or the default value at first 
sights as part of feature negotiation. If we want to stick to the model 
Lingshan proposed, maybe fix the spec first then get back on the details?

-Siwei

>
> And I like it that this fixes sparse warning actually:
> max_virtqueue_pairs it tagged as __virtio, not __le.
>
> However, I am worried there is another bug here:
> this is checking driver features. But really max vqs
> should not depend on that, it depends on device
> features, no?
>
>
>
>>> 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 V4 5/6] vDPA: answer num of queue pairs = 1 to userspace when VIRTIO_NET_F_MQ == 0

[Jason Wang]

On Wed, Aug 10, 2022 at 8:54 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>
>
>
> On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote:
> > On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote:
> >>
> >>> From: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> Sent: Friday, July 22, 2022 7:53 AM
> >>>
> >>> 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
> >>>
> >> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
> >> Please drop this patch.
> >> Nack on this patch.
> > Wrt this patch as far as I'm concerned you guys are bikeshedding.
> >
> > Parav generally don't send nacks please they are not helpful.
> >
> > Zhu Lingshan please always address comments in some way.  E.g. refer to
> > them in the commit log explaining the motivation and the alternatives.
> > I know you don't agree with Parav but ghosting isn't nice.
> >
> > I still feel what we should have done is
> > not return a value, as long as we do return it we might
> > as well return something reasonable, not 0.
> Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is
> not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply
> not there, but userspace (iproute) mistakenly puts a zero value there.
> This is a pattern every tool in iproute follows consistently by large. I
> don't get why kernel has to return something without seeing a very
> convincing use case?
>
> Not to mention spec doesn't give us explicit definition for when the
> field in config space becomes valid and/or the default value at first
> sights as part of feature negotiation. If we want to stick to the model
> Lingshan proposed, maybe fix the spec first then get back on the details?

So spec said

"
The following driver-read-only field, max_virtqueue_pairs only exists
if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.
"

My understanding is that the field is always valid if the device
offers the feature.

Btw, even if the spec is unclear, it would be very hard to "fix" it
without introducing a new feature bit, it means we still need to deal
with device without the new feature.

Thanks

>
> -Siwei
>
> >
> > And I like it that this fixes sparse warning actually:
> > max_virtqueue_pairs it tagged as __virtio, not __le.
> >
> > However, I am worried there is another bug here:
> > this is checking driver features. But really max vqs
> > should not depend on that, it depends on device
> > features, no?
> >
> >
> >
> >>> 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
>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
>

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

[Zhu, Lingshan]

On 8/10/2022 3:36 AM, Michael S. Tsirkin wrote:
> On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote:
>>
>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>> Sent: Friday, July 22, 2022 7:53 AM
>>>
>>> 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
>>>
>> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
>> Please drop this patch.
>> Nack on this patch.
> Wrt this patch as far as I'm concerned you guys are bikeshedding.
>
> Parav generally don't send nacks please they are not helpful.
>
> Zhu Lingshan please always address comments in some way.  E.g. refer to
> them in the commit log explaining the motivation and the alternatives.
> I know you don't agree with Parav but ghosting isn't nice.
I can work out a better commit message, currently I have an example on
how to process MQ = 0. I will handle all conditional fields(MTC, MAC...) 
here.
So I will explain them both in general and details for every field.

Thanks,
Zhu Lingshan
>
> I still feel what we should have done is
> not return a value, as long as we do return it we might
> as well return something reasonable, not 0.
>
> And I like it that this fixes sparse warning actually:
> max_virtqueue_pairs it tagged as __virtio, not __le.
>
> However, I am worried there is another bug here:
> this is checking driver features. But really max vqs
> should not depend on that, it depends on device
> features, no?
We have this fix in this patch below:

return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);

this checks device features.

Thanks,
Zhu Lingshan

>
>
>
>>> 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 V4 3/6] vDPA: allow userspace to query features of a vDPA device

[Zhu, Lingshan]

On 8/10/2022 3:24 AM, Michael S. Tsirkin wrote:
> On Fri, Jul 22, 2022 at 07:53:06PM +0800, Zhu Lingshan wrote:
>> 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>
>
> I think at least some discussion and documentation
> of this attribute versus VDPA_ATTR_DEV_SUPPORTED_FEATURES
> is called for.
>
> Otherwise how do people know which one to use?
>
> We can't send everyone to go read the lkml thread.
I will add comments in both this vdpa_dev_net_config_fill() and the 
header file.

Thanks,
Zhu Lingshan
>
>> ---
>>   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

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

[Si-Wei Liu]

On 8/9/2022 6:09 PM, Jason Wang wrote:
> On Wed, Aug 10, 2022 at 8:54 AM Si-Wei Liu <si-wei.liu@oracle.com> wrote:
>>
>>
>> On 8/9/2022 12:36 PM, Michael S. Tsirkin wrote:
>>> On Fri, Jul 22, 2022 at 01:14:42PM +0000, Parav Pandit wrote:
>>>>> From: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> Sent: Friday, July 22, 2022 7:53 AM
>>>>>
>>>>> 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
>>>>>
>>>> No. We do not want to diverge returning values of config space for fields which are not present as discussed in previous versions.
>>>> Please drop this patch.
>>>> Nack on this patch.
>>> Wrt this patch as far as I'm concerned you guys are bikeshedding.
>>>
>>> Parav generally don't send nacks please they are not helpful.
>>>
>>> Zhu Lingshan please always address comments in some way.  E.g. refer to
>>> them in the commit log explaining the motivation and the alternatives.
>>> I know you don't agree with Parav but ghosting isn't nice.
>>>
>>> I still feel what we should have done is
>>> not return a value, as long as we do return it we might
>>> as well return something reasonable, not 0.
>> Maybe I missed something but I don't get this, when VIRTIO_NET_F_MQ is
>> not negotiated, the VDPA_ATTR_DEV_NET_CFG_MAX_VQP attribute is simply
>> not there, but userspace (iproute) mistakenly puts a zero value there.
>> This is a pattern every tool in iproute follows consistently by large. I
>> don't get why kernel has to return something without seeing a very
>> convincing use case?
>>
>> Not to mention spec doesn't give us explicit definition for when the
>> field in config space becomes valid and/or the default value at first
>> sights as part of feature negotiation. If we want to stick to the model
>> Lingshan proposed, maybe fix the spec first then get back on the details?
> So spec said
>
> "
> The following driver-read-only field, max_virtqueue_pairs only exists
> if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.
> "
>
> My understanding is that the field is always valid if the device
> offers the feature.
The tricky part is to deal with VERSION_1 on transitional device that 
determines the endianness of field. I know we don't support !VERSION_1 
vdpa provider for now, but the tool should be made independent of this 
assumption.

For the most of config fields there's no actual valid "default" value 
during feature negotiation until it can be determined after negotiation 
is done. I wonder what is the administrative value if presenting those 
random value to the end user? And there's even special feature like 
VIRTIO_BLK_F_CONFIG_WCE that only present valid feature value after 
negotiation. I'm afraid it may further confuse end user, or it would 
require them to read and understand all of details in spec, which 
apparently contradict to the goal of showing meaningful queue-pair value 
without requiring user to read the spec details.

-Siwei

>
> Btw, even if the spec is unclear, it would be very hard to "fix" it
> without introducing a new feature bit, it means we still need to deal
> with device without the new feature.
>
> Thanks
>
>> -Siwei
>>
>>> And I like it that this fixes sparse warning actually:
>>> max_virtqueue_pairs it tagged as __virtio, not __le.
>>>
>>> However, I am worried there is another bug here:
>>> this is checking driver features. But really max vqs
>>> should not depend on that, it depends on device
>>> features, no?
>>>
>>>
>>>
>>>>> 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://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NE42b1rl66ElGUzHr3b9xXGYCs2Vpb5dkhF0fPXnAyyFYzZZyzsY9NV_Qbf2AZCI3XxC13_nlWfSVN52yIM$
>> _______________________________________________
>> Virtualization mailing list
>> Virtualization@lists.linux-foundation.org
>> https://urldefense.com/v3/__https://lists.linuxfoundation.org/mailman/listinfo/virtualization__;!!ACWV5N9M2RV99hQ!NE42b1rl66ElGUzHr3b9xXGYCs2Vpb5dkhF0fPXnAyyFYzZZyzsY9NV_Qbf2AZCI3XxC13_nlWfSVN52yIM$
>>