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

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

[Zhu Lingshan]

This series allows userspace to query device config space through
netlink, to get multi-queue, feature bits, device features and
driver features.

This series also has fixed some issues of misusing
VDPA_ATTR_DEV_SUPPORTED_FEATURES, this should be used for virtio devices
than the management device.

Please help review.

Thanks!
Zhu Lingshan

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/ifcvf: support userspace to query device feature bits
  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_dev_net_config_fill()

 drivers/vdpa/ifcvf/ifcvf_base.c | 20 ++++++++++++++++++--
 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, 40 insertions(+), 19 deletions(-)

-- 
2.31.1

[PATCH 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 | 8 ++++++--
 drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
index 48c4dadb0c7c..6bccc8291c26 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,18 @@ 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);
 	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 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 6bccc8291c26..7be703b5d1f4 100644
--- a/drivers/vdpa/ifcvf/ifcvf_base.c
+++ b/drivers/vdpa/ifcvf/ifcvf_base.c
@@ -341,6 +341,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  = (struct virtio_net_config __iomem *)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 4366320fb68d..0c3af30b297e 100644
--- a/drivers/vdpa/ifcvf/ifcvf_main.c
+++ b/drivers/vdpa/ifcvf/ifcvf_main.c
@@ -786,6 +786,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 3/6] vDPA/ifcvf: support userspace to query device feature bits

[Zhu Lingshan]

This commit supports userspace to query device feature bits
by filling the relevant netlink attribute.

There are two types of netlink attributes:
VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
VDPA_ATTR_MGMTDEV_XXXX work for the management devices.

This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
this attr is for a virtio device, not management devices.

Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
for virtio devices feature bits.

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

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 2b75c00b1005..c820dd2b0307 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
 		err = -EMSGSIZE;
 		goto msg_err;
 	}
-	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,
 			      mdev->supported_features, VDPA_ATTR_PAD)) {
 		err = -EMSGSIZE;
 		goto msg_err;
@@ -827,7 +827,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));
@@ -844,12 +844,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;
 
-	return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
+	features_device = vdev->config->get_device_features(vdev);
+	if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
+			      VDPA_ATTR_PAD))
+		return -EMSGSIZE;
+
+	return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
 }
 
 static int
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 1061d8d2d09d..70a3672c288f 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -30,6 +30,7 @@ enum vdpa_attr {
 	VDPA_ATTR_MGMTDEV_BUS_NAME,		/* string */
 	VDPA_ATTR_MGMTDEV_DEV_NAME,		/* string */
 	VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,	/* u64 */
+	VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,	/* u64 */
 
 	VDPA_ATTR_DEV_NAME,			/* string */
 	VDPA_ATTR_DEV_ID,			/* u32 */
-- 
2.31.1

[PATCH 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.

This commit removes FEATURES_OK blocker in vdpa_dev_config_fill()
which calls vdpa_dev_net_config_fill() for virtio-net

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 c820dd2b0307..030d96bdeed2 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -863,17 +863,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
 {
 	u32 device_id;
 	void *hdr;
-	u8 status;
 	int err;
 
 	mutex_lock(&vdev->cf_mutex);
-	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 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

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

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 030d96bdeed2..50a11ece603e 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
 }
 
-- 
2.31.1

[PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Zhu Lingshan]

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

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

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 50a11ece603e..2719ce9962fc 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -837,11 +837,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 = le16_to_cpu((__force __le16)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 = le16_to_cpu((__force __le16)config.mtu);
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
 		return -EMSGSIZE;
 
-- 
2.31.1

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 | 8 ++++++--
>  drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 48c4dadb0c7c..6bccc8291c26 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);

One possible issue here is that, if the hardware have more size than
spec, we may end up with a migration compatibility issue.

It looks to me we'd better build the config size based on the
features, e.g it looks to me for net, we should probably use

offset_of(struct virtio_net_config, mtu)?

>                         IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
>                         break;
>                 }
> @@ -233,15 +234,18 @@ 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);
>         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;

I don't get the code here, any chance that net_config_size could be zero?

Thanks

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

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 | 8 ++++++--
>  drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
>  2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
> index 48c4dadb0c7c..6bccc8291c26 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);

One possible issue here is that, if the hardware have more size than
spec, we may end up with a migration compatibility issue.

It looks to me we'd better build the config size based on the
features, e.g it looks to me for net, we should probably use

offset_of(struct virtio_net_config, mtu)?

>                         IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
>                         break;
>                 }
> @@ -233,15 +234,18 @@ 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);
>         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;

I don't get the code here, any chance that net_config_size could be zero?

Thanks

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

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

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 6bccc8291c26..7be703b5d1f4 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -341,6 +341,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  = (struct virtio_net_config __iomem *)hw->dev_cfg;

Any reason we need the cast here? (cast from void * seems not necessary).

> +       val = vp_ioread16((__le16 __iomem *)&config->max_virtqueue_pairs);

I don't see any __le16 cast for the callers of vp_ioread16, anything
make max_virtqueue_pairs different here?

Thanks

> +       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 4366320fb68d..0c3af30b297e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -786,6 +786,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);

Btw, I think current IFCVF doesn't support the provisioning of a
$max_qps that is smaller than what hardware did.

Then I wonder if we need a min_supported_vqs attribute or doing
mediation in the ifcvf parent.

Thanks

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

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 6bccc8291c26..7be703b5d1f4 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
> @@ -341,6 +341,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  = (struct virtio_net_config __iomem *)hw->dev_cfg;

Any reason we need the cast here? (cast from void * seems not necessary).

> +       val = vp_ioread16((__le16 __iomem *)&config->max_virtqueue_pairs);

I don't see any __le16 cast for the callers of vp_ioread16, anything
make max_virtqueue_pairs different here?

Thanks

> +       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 4366320fb68d..0c3af30b297e 100644
> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
> @@ -786,6 +786,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);

Btw, I think current IFCVF doesn't support the provisioning of a
$max_qps that is smaller than what hardware did.

Then I wonder if we need a min_supported_vqs attribute or doing
mediation in the ifcvf parent.

Thanks

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

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

Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit supports userspace to query device feature bits
> by filling the relevant netlink attribute.
>
> There are two types of netlink attributes:
> VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
> VDPA_ATTR_MGMTDEV_XXXX work for the management devices.
>
> This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> this attr is for a virtio device, not management devices.
>
> Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
> reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
> for virtio devices feature bits.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 15 ++++++++++-----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..c820dd2b0307 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>                 err = -EMSGSIZE;
>                 goto msg_err;
>         }
> -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,

Adding Eli and Parav.

If I understand correctly, we can't provision virtio features right
now. This means, the vDPA instance should have the same features as
its parent (management device).

And it seems to me if we can do things like this, we need first allow
the features to be provisioned. (And this change breaks uABI)

Thanks


>                               mdev->supported_features, VDPA_ATTR_PAD)) {
>                 err = -EMSGSIZE;
>                 goto msg_err;
> @@ -827,7 +827,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));
> @@ -844,12 +844,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;
>
> -       return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +       features_device = vdev->config->get_device_features(vdev);
> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
> +                             VDPA_ATTR_PAD))
> +               return -EMSGSIZE;
> +
> +       return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
>  }
>
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 1061d8d2d09d..70a3672c288f 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -30,6 +30,7 @@ enum vdpa_attr {
>         VDPA_ATTR_MGMTDEV_BUS_NAME,             /* string */
>         VDPA_ATTR_MGMTDEV_DEV_NAME,             /* string */
>         VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,    /* u64 */
> +       VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,   /* u64 */
>
>         VDPA_ATTR_DEV_NAME,                     /* string */
>         VDPA_ATTR_DEV_ID,                       /* u32 */
> --
> 2.31.1
>

Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit supports userspace to query device feature bits
> by filling the relevant netlink attribute.
>
> There are two types of netlink attributes:
> VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
> VDPA_ATTR_MGMTDEV_XXXX work for the management devices.
>
> This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> this attr is for a virtio device, not management devices.
>
> Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
> reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
> for virtio devices feature bits.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c       | 15 ++++++++++-----
>  include/uapi/linux/vdpa.h |  1 +
>  2 files changed, 11 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 2b75c00b1005..c820dd2b0307 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>                 err = -EMSGSIZE;
>                 goto msg_err;
>         }
> -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,

Adding Eli and Parav.

If I understand correctly, we can't provision virtio features right
now. This means, the vDPA instance should have the same features as
its parent (management device).

And it seems to me if we can do things like this, we need first allow
the features to be provisioned. (And this change breaks uABI)

Thanks


>                               mdev->supported_features, VDPA_ATTR_PAD)) {
>                 err = -EMSGSIZE;
>                 goto msg_err;
> @@ -827,7 +827,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));
> @@ -844,12 +844,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;
>
> -       return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> +       features_device = vdev->config->get_device_features(vdev);
> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
> +                             VDPA_ATTR_PAD))
> +               return -EMSGSIZE;
> +
> +       return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
>  }
>
>  static int
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 1061d8d2d09d..70a3672c288f 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -30,6 +30,7 @@ enum vdpa_attr {
>         VDPA_ATTR_MGMTDEV_BUS_NAME,             /* string */
>         VDPA_ATTR_MGMTDEV_DEV_NAME,             /* string */
>         VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,    /* u64 */
> +       VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,   /* u64 */
>
>         VDPA_ATTR_DEV_NAME,                     /* string */
>         VDPA_ATTR_DEV_ID,                       /* u32 */
> --
> 2.31.1
>

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

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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.

Quotes from the spec:

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

>
> This commit removes FEATURES_OK blocker in vdpa_dev_config_fill()
> which calls vdpa_dev_net_config_fill() for virtio-net
>
> 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 c820dd2b0307..030d96bdeed2 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -863,17 +863,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>  {
>         u32 device_id;
>         void *hdr;
> -       u8 status;
>         int err;
>
>         mutex_lock(&vdev->cf_mutex);
> -       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;
> -       }

So we had the following in vdpa_dev_net_config_fill():

        features = vdev->config->get_driver_features(vdev);
        if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
                              VDPA_ATTR_PAD))
                return -EMSGSIZE;

It looks to me we need to switch to using get_device_features() instead.

Thanks

> -
>         hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>                           VDPA_CMD_DEV_CONFIG_GET);
>         if (!hdr) {
> --
> 2.31.1
>

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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.

Quotes from the spec:

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

>
> This commit removes FEATURES_OK blocker in vdpa_dev_config_fill()
> which calls vdpa_dev_net_config_fill() for virtio-net
>
> 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 c820dd2b0307..030d96bdeed2 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -863,17 +863,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>  {
>         u32 device_id;
>         void *hdr;
> -       u8 status;
>         int err;
>
>         mutex_lock(&vdev->cf_mutex);
> -       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;
> -       }

So we had the following in vdpa_dev_net_config_fill():

        features = vdev->config->get_driver_features(vdev);
        if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
                              VDPA_ATTR_PAD))
                return -EMSGSIZE;

It looks to me we need to switch to using get_device_features() instead.

Thanks

> -
>         hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>                           VDPA_CMD_DEV_CONFIG_GET);
>         if (!hdr) {
> --
> 2.31.1
>

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

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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

Spec said:

"max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"

So we are probably fine.

Thanks

>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 030d96bdeed2..50a11ece603e 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
>  }
>
> --
> 2.31.1
>

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

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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

Spec said:

"max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"

So we are probably fine.

Thanks

>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 030d96bdeed2..50a11ece603e 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
>  }
>
> --
> 2.31.1
>

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

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit fixes spars warnings: cast to restricted __le16
> in function vdpa_dev_net_config_fill()
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 50a11ece603e..2719ce9962fc 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);

Can we use virtio accessors like virtio16_to_cpu()?

Thanks

>         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>                 return -EMSGSIZE;
>
> -       val_u16 = le16_to_cpu(config.mtu);
> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
>         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>                 return -EMSGSIZE;
>
> --
> 2.31.1
>

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Jason Wang]

On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit fixes spars warnings: cast to restricted __le16
> in function vdpa_dev_net_config_fill()
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
>  drivers/vdpa/vdpa.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 50a11ece603e..2719ce9962fc 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);

Can we use virtio accessors like virtio16_to_cpu()?

Thanks

>         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>                 return -EMSGSIZE;
>
> -       val_u16 = le16_to_cpu(config.mtu);
> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
>         if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>                 return -EMSGSIZE;
>
> --
> 2.31.1
>

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

RE: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits

[Eli Cohen]

> -----Original Message-----
> From: Jason Wang <jasowang@redhat.com>
> Sent: Thursday, June 2, 2022 10:32 AM
> To: Zhu Lingshan <lingshan.zhu@intel.com>
> Cc: mst <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; netdev <netdev@vger.kernel.org>; Eli Cohen
> <elic@nvidia.com>; Parav Pandit <parav@nvidia.com>
> Subject: Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits
> 
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >
> > This commit supports userspace to query device feature bits
> > by filling the relevant netlink attribute.
> >
> > There are two types of netlink attributes:
> > VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
> > VDPA_ATTR_MGMTDEV_XXXX work for the management devices.
> >
> > This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> > this attr is for a virtio device, not management devices.
> >
> > Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
> > reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
> > for virtio devices feature bits.
> >
> > Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > ---
> >  drivers/vdpa/vdpa.c       | 15 ++++++++++-----
> >  include/uapi/linux/vdpa.h |  1 +
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 2b75c00b1005..c820dd2b0307 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >                 err = -EMSGSIZE;
> >                 goto msg_err;
> >         }
> > -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> > +       if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,
> 
> Adding Eli and Parav.
> 
> If I understand correctly, we can't provision virtio features right
> now. This means, the vDPA instance should have the same features as
> its parent (management device).

A management device should be read as "a management device capable of
instantiating a virtio device". Thus, I see no reason to introduce another attribute.
VDPA_ATTR_DEV_SUPPORTED_FEATURES is the capability of the management
device to spawn a virtio device with certain features.

> 
> And it seems to me if we can do things like this, we need first allow
> the features to be provisioned. (And this change breaks uABI)
> 
> Thanks
> 
> 
> >                               mdev->supported_features, VDPA_ATTR_PAD)) {
> >                 err = -EMSGSIZE;
> >                 goto msg_err;
> > @@ -827,7 +827,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));
> > @@ -844,12 +844,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;
> >
> > -       return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
> > +       features_device = vdev->config->get_device_features(vdev);
> > +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
> > +                             VDPA_ATTR_PAD))
> > +               return -EMSGSIZE;
> > +
> > +       return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
> >  }
> >
> >  static int
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 1061d8d2d09d..70a3672c288f 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -30,6 +30,7 @@ enum vdpa_attr {
> >         VDPA_ATTR_MGMTDEV_BUS_NAME,             /* string */
> >         VDPA_ATTR_MGMTDEV_DEV_NAME,             /* string */
> >         VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,    /* u64 */
> > +       VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,   /* u64 */
> >
> >         VDPA_ATTR_DEV_NAME,                     /* string */
> >         VDPA_ATTR_DEV_ID,                       /* u32 */
> > --
> > 2.31.1
> >

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

[Zhu, Lingshan]

On 6/2/2022 3:11 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> 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 | 8 ++++++--
>>   drivers/vdpa/ifcvf/ifcvf_base.h | 2 ++
>>   2 files changed, 8 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/ifcvf/ifcvf_base.c b/drivers/vdpa/ifcvf/ifcvf_base.c
>> index 48c4dadb0c7c..6bccc8291c26 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);
> One possible issue here is that, if the hardware have more size than
> spec, we may end up with a migration compatibility issue.
>
> It looks to me we'd better build the config size based on the
> features, e.g it looks to me for net, we should probably use
>
> offset_of(struct virtio_net_config, mtu)?
Hi Jason,

our ifcvf devices have nothing out of the spec in the device config 
space, so I think we can trust the cap size
>
>>                          IFCVF_DBG(pdev, "hw->dev_cfg = %p\n", hw->dev_cfg);
>>                          break;
>>                  }
>> @@ -233,15 +234,18 @@ 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);
>>          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;
> I don't get the code here, any chance that net_config_size could be zero?
This means, if the capability size is more than the size of structure 
virtio-net-cofnig,
there may be something out of the spec, then we only migrate the spec 
contents, which is
a defensive coding. If the capability size is smaller than the size in 
spec, means
the capability is a sub-set of the spec contents, we only migrate the 
onboard contents.

Sorry for the confusing, I will add a comment here.

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

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

[Zhu, Lingshan]

On 6/2/2022 3:21 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> 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 6bccc8291c26..7be703b5d1f4 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_base.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_base.c
>> @@ -341,6 +341,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  = (struct virtio_net_config __iomem *)hw->dev_cfg;
> Any reason we need the cast here? (cast from void * seems not necessary).
This cast is unnecessary.
>
>> +       val = vp_ioread16((__le16 __iomem *)&config->max_virtqueue_pairs);
> I don't see any __le16 cast for the callers of vp_ioread16, anything
> make max_virtqueue_pairs different here?
I think we still need it here since hw->dev_cfg and its contents are __iomem

THanks
>
> Thanks
>
>> +       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 4366320fb68d..0c3af30b297e 100644
>> --- a/drivers/vdpa/ifcvf/ifcvf_main.c
>> +++ b/drivers/vdpa/ifcvf/ifcvf_main.c
>> @@ -786,6 +786,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);
> Btw, I think current IFCVF doesn't support the provisioning of a
> $max_qps that is smaller than what hardware did.
>
> Then I wonder if we need a min_supported_vqs attribute or doing
> mediation in the ifcvf parent.
>
> Thanks
>
>> +       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
>>

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

[Zhu, Lingshan]

On 6/2/2022 3:36 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> 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.
> Quotes from the spec:
>
> "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."
>
>> This commit removes FEATURES_OK blocker in vdpa_dev_config_fill()
>> which calls vdpa_dev_net_config_fill() for virtio-net
>>
>> 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 c820dd2b0307..030d96bdeed2 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -863,17 +863,9 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
>>   {
>>          u32 device_id;
>>          void *hdr;
>> -       u8 status;
>>          int err;
>>
>>          mutex_lock(&vdev->cf_mutex);
>> -       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;
>> -       }
> So we had the following in vdpa_dev_net_config_fill():
>
>          features = vdev->config->get_driver_features(vdev);
>          if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features,
>                                VDPA_ATTR_PAD))
>                  return -EMSGSIZE;
>
> It looks to me we need to switch to using get_device_features() instead.
This is done in the 3/6 patch

Thanks,
ZHu Lingshan
>
> Thanks
>
>> -
>>          hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags,
>>                            VDPA_CMD_DEV_CONFIG_GET);
>>          if (!hdr) {
>> --
>> 2.31.1
>>

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

[Zhu, Lingshan]

On 6/2/2022 3:38 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> 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
> Spec said:
>
> "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"
>
> So we are probably fine.
I thinks it is asking how many queue 
pairs(VDPA_ATTR_DEV_NET_CFG_MAX_VQP), so answering 0 may not be correct.

Thanks,
Zhu Lingshan
>
> Thanks
>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/vdpa.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 030d96bdeed2..50a11ece603e 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
>>   }
>>
>> --
>> 2.31.1
>>

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Zhu, Lingshan]

On 6/2/2022 3:40 PM, Jason Wang wrote:
> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This commit fixes spars warnings: cast to restricted __le16
>> in function vdpa_dev_net_config_fill()
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>>   drivers/vdpa/vdpa.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 50a11ece603e..2719ce9962fc 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);
> Can we use virtio accessors like virtio16_to_cpu()?
I will work out a vdpa16_to_cpu()

Thanks,
Zhu Lingshan
>
> Thanks
>
>>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>                  return -EMSGSIZE;
>>
>> -       val_u16 = le16_to_cpu(config.mtu);
>> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
>>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>                  return -EMSGSIZE;
>>
>> --
>> 2.31.1
>>

Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits

[Zhu, Lingshan]

On 6/2/2022 3:46 PM, Eli Cohen wrote:
>
>> -----Original Message-----
>> From: Jason Wang <jasowang@redhat.com>
>> Sent: Thursday, June 2, 2022 10:32 AM
>> To: Zhu Lingshan <lingshan.zhu@intel.com>
>> Cc: mst <mst@redhat.com>; virtualization <virtualization@lists.linux-foundation.org>; netdev <netdev@vger.kernel.org>; Eli Cohen
>> <elic@nvidia.com>; Parav Pandit <parav@nvidia.com>
>> Subject: Re: [PATCH 3/6] vDPA/ifcvf: support userspace to query device feature bits
>>
>> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>> This commit supports userspace to query device feature bits
>>> by filling the relevant netlink attribute.
>>>
>>> There are two types of netlink attributes:
>>> VDPA_ATTR_DEV_XXXX work for virtio devices config space, and
>>> VDPA_ATTR_MGMTDEV_XXXX work for the management devices.
>>>
>>> This commit fixes a misuse of VDPA_ATTR_DEV_SUPPORTED_FEATURES,
>>> this attr is for a virtio device, not management devices.
>>>
>>> Thus VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES is introduced for
>>> reporting management device features, and VDPA_ATTR_DEV_SUPPORTED_FEATURES
>>> for virtio devices feature bits.
>>>
>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> ---
>>>   drivers/vdpa/vdpa.c       | 15 ++++++++++-----
>>>   include/uapi/linux/vdpa.h |  1 +
>>>   2 files changed, 11 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 2b75c00b1005..c820dd2b0307 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -508,7 +508,7 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>                  err = -EMSGSIZE;
>>>                  goto msg_err;
>>>          }
>>> -       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
>>> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,
>> Adding Eli and Parav.
>>
>> If I understand correctly, we can't provision virtio features right
>> now. This means, the vDPA instance should have the same features as
>> its parent (management device).
> A management device should be read as "a management device capable of
> instantiating a virtio device". Thus, I see no reason to introduce another attribute.
> VDPA_ATTR_DEV_SUPPORTED_FEATURES is the capability of the management
> device to spawn a virtio device with certain features.
This may not be true because the "managed" device may have different 
feature bits than the "management" device.
E.g., we can choose not to provide MQ feature for a managed device.

As Jason said, it is about provisioning, and we need two different attrs 
anyway.

Thanks,
Zhu Lingshan
>
>> And it seems to me if we can do things like this, we need first allow
>> the features to be provisioned. (And this change breaks uABI)
>>
>> Thanks
>>
>>
>>>                                mdev->supported_features, VDPA_ATTR_PAD)) {
>>>                  err = -EMSGSIZE;
>>>                  goto msg_err;
>>> @@ -827,7 +827,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));
>>> @@ -844,12 +844,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;
>>>
>>> -       return vdpa_dev_net_mq_config_fill(vdev, msg, features, &config);
>>> +       features_device = vdev->config->get_device_features(vdev);
>>> +       if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES, features_device,
>>> +                             VDPA_ATTR_PAD))
>>> +               return -EMSGSIZE;
>>> +
>>> +       return vdpa_dev_net_mq_config_fill(vdev, msg, features_device, &config);
>>>   }
>>>
>>>   static int
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 1061d8d2d09d..70a3672c288f 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -30,6 +30,7 @@ enum vdpa_attr {
>>>          VDPA_ATTR_MGMTDEV_BUS_NAME,             /* string */
>>>          VDPA_ATTR_MGMTDEV_DEV_NAME,             /* string */
>>>          VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,    /* u64 */
>>> +       VDPA_ATTR_MGMTDEV_SUPPORTED_FEATURES,   /* u64 */
>>>
>>>          VDPA_ATTR_DEV_NAME,                     /* string */
>>>          VDPA_ATTR_DEV_ID,                       /* u32 */
>>> --
>>> 2.31.1
>>>

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

[Jason Wang]

On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/2/2022 3:38 PM, Jason Wang wrote:
> > On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> 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
> > Spec said:
> >
> > "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"
> >
> > So we are probably fine.
> I thinks it is asking how many queue
> pairs(VDPA_ATTR_DEV_NET_CFG_MAX_VQP), so answering 0 may not be correct.
>
> Thanks,
> Zhu Lingshan

Please add the result of the userspace vdpa tool before and after this
patch in the changlog in next version.

Thanks

> >
> > Thanks
> >
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 030d96bdeed2..50a11ece603e 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
> >>   }
> >>
> >> --
> >> 2.31.1
> >>
>

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

[Jason Wang]

On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/2/2022 3:38 PM, Jason Wang wrote:
> > On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> 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
> > Spec said:
> >
> > "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"
> >
> > So we are probably fine.
> I thinks it is asking how many queue
> pairs(VDPA_ATTR_DEV_NET_CFG_MAX_VQP), so answering 0 may not be correct.
>
> Thanks,
> Zhu Lingshan

Please add the result of the userspace vdpa tool before and after this
patch in the changlog in next version.

Thanks

> >
> > Thanks
> >
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c | 5 +++--
> >>   1 file changed, 3 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 030d96bdeed2..50a11ece603e 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
> >>   }
> >>
> >> --
> >> 2.31.1
> >>
>

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

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Jason Wang]

On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/2/2022 3:40 PM, Jason Wang wrote:
> > On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit fixes spars warnings: cast to restricted __le16
> >> in function vdpa_dev_net_config_fill()
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 50a11ece603e..2719ce9962fc 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);
> > Can we use virtio accessors like virtio16_to_cpu()?
> I will work out a vdpa16_to_cpu()

I meant __virtio16_to_cpu(true, xxx) actually here.

Thanks

>
> Thanks,
> Zhu Lingshan
> >
> > Thanks
> >
> >>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> >>                  return -EMSGSIZE;
> >>
> >> -       val_u16 = le16_to_cpu(config.mtu);
> >> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
> >>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> >>                  return -EMSGSIZE;
> >>
> >> --
> >> 2.31.1
> >>
>

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Jason Wang]

On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 6/2/2022 3:40 PM, Jason Wang wrote:
> > On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit fixes spars warnings: cast to restricted __le16
> >> in function vdpa_dev_net_config_fill()
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >>   drivers/vdpa/vdpa.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 50a11ece603e..2719ce9962fc 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);
> > Can we use virtio accessors like virtio16_to_cpu()?
> I will work out a vdpa16_to_cpu()

I meant __virtio16_to_cpu(true, xxx) actually here.

Thanks

>
> Thanks,
> Zhu Lingshan
> >
> > Thanks
> >
> >>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> >>                  return -EMSGSIZE;
> >>
> >> -       val_u16 = le16_to_cpu(config.mtu);
> >> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
> >>          if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
> >>                  return -EMSGSIZE;
> >>
> >> --
> >> 2.31.1
> >>
>

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

Re: [PATCH 6/6] vDPA: fix 'cast to restricted le16' warnings in vdpa_dev_net_config_fill()

[Zhu, Lingshan]

On 6/7/2022 2:15 PM, Jason Wang wrote:
> On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 6/2/2022 3:40 PM, Jason Wang wrote:
>>> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit fixes spars warnings: cast to restricted __le16
>>>> in function vdpa_dev_net_config_fill()
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c | 4 ++--
>>>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index 50a11ece603e..2719ce9962fc 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -837,11 +837,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 = le16_to_cpu((__force __le16)config.status);
>>> Can we use virtio accessors like virtio16_to_cpu()?
>> I will work out a vdpa16_to_cpu()
> I meant __virtio16_to_cpu(true, xxx) actually here.
sure, this can work!
>
> Thanks
>
>> Thanks,
>> Zhu Lingshan
>>> Thanks
>>>
>>>>           if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>>>                   return -EMSGSIZE;
>>>>
>>>> -       val_u16 = le16_to_cpu(config.mtu);
>>>> +       val_u16 = le16_to_cpu((__force __le16)config.mtu);
>>>>           if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
>>>>                   return -EMSGSIZE;
>>>>
>>>> --
>>>> 2.31.1
>>>>

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

[Zhu, Lingshan]

On 6/7/2022 2:14 PM, Jason Wang wrote:
> On Mon, Jun 6, 2022 at 4:22 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 6/2/2022 3:38 PM, Jason Wang wrote:
>>> On Thu, Jun 2, 2022 at 10:48 AM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> 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
>>> Spec said:
>>>
>>> "max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ is set"
>>>
>>> So we are probably fine.
>> I thinks it is asking how many queue
>> pairs(VDPA_ATTR_DEV_NET_CFG_MAX_VQP), so answering 0 may not be correct.
>>
>> Thanks,
>> Zhu Lingshan
> Please add the result of the userspace vdpa tool before and after this
> patch in the changlog in next version.
sure!
>
> Thanks
>
>>> Thanks
>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>>    drivers/vdpa/vdpa.c | 5 +++--
>>>>    1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index 030d96bdeed2..50a11ece603e 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -818,9 +818,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 = le16_to_cpu((__force __le16)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);
>>>>    }
>>>>
>>>> --
>>>> 2.31.1
>>>>