[PATCH V3 0/6] Conditionally read fields in dev cfg space

[PATCH V3 0/6] Conditionally read fields in dev cfg space

[Zhu Lingshan]

This series intends to read the fields in virtio-net device
configuration space conditionally on the feature bits,
this means:

MTU exists if VIRTIO_NET_F_MTU is set
MAC exists if VIRTIO_NET_F_NET is set
MQ exists if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.

This series report device features to userspace and invokes
vdpa_config_ops.get_config() rather than
vdpa_get_config_unlocked() to read the device config spcae,
so no races in vdpa_set_features_unlocked()

Thanks!

Changes form V2:
remove unnacessary checking for vdev->config->get_status (Jason)

Changes from V1:
1)Better comments for VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
only in the header file(Jason)
2)Split original 3/4 into separate patches(Jason)
3)Check FEATURES_OK for reporting driver features
in vdpa_dev_config_fill (Jason)
4) Add iproute2 example for reporting device features

Zhu Lingshan (6):
  vDPA: allow userspace to query features of a vDPA device
  vDPA: only report driver features if FEATURES_OK is set
  vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
  vDPA: check virtio device features to detect MQ
  vDPA: fix spars cast warning in vdpa_dev_net_mq_config_fill
  vDPA: conditionally read MTU and MAC in dev cfg space

 drivers/vdpa/vdpa.c       | 68 ++++++++++++++++++++++++++++++---------
 include/uapi/linux/vdpa.h |  4 +++
 2 files changed, 56 insertions(+), 16 deletions(-)

-- 
2.31.1

[PATCH V3 1/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.

This commit invokes vdpa_config_ops.get_config()
rather than vdpa_get_config_unlocked() to read
the device config spcae, so no races in
vdpa_set_features_unlocked()

Userspace tool iproute2 example:
$ vdpa dev config show vdpa0
vdpa0: mac 00:e8:ca:11:be:05 link up link_announce false max_vq_pairs 4 mtu 1500
  negotiated_features MRG_RXBUF CTRL_VQ MQ VERSION_1 ACCESS_PLATFORM
  dev_features MTU MAC MRG_RXBUF CTRL_VQ MQ ANY_LAYOUT VERSION_1 ACCESS_PLATFORM

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

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index c06c02704461..1363b42c9d28 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -815,10 +815,10 @@ 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));
+	vdev->config->get_config(vdev, 0, &config, sizeof(config));
 
 	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac),
 		    config.mac))
@@ -832,12 +832,18 @@ 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..07474183fdb3 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -46,12 +46,16 @@ enum vdpa_attr {
 
 	VDPA_ATTR_DEV_NEGOTIATED_FEATURES,	/* u64 */
 	VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,		/* u32 */
+	/* virtio features that are supported by the vDPA management device */
 	VDPA_ATTR_DEV_SUPPORTED_FEATURES,	/* u64 */
 
 	VDPA_ATTR_DEV_QUEUE_INDEX,              /* u32 */
 	VDPA_ATTR_DEV_VENDOR_ATTR_NAME,		/* string */
 	VDPA_ATTR_DEV_VENDOR_ATTR_VALUE,        /* u64 */
 
+	/* virtio features that are supported by the vDPA device */
+	VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,	/* u64 */
+
 	/* new attributes must be added above here */
 	VDPA_ATTR_MAX,
 };
-- 
2.31.1

[PATCH V3 2/6] vDPA: only report driver features if FEATURES_OK is set

[Zhu Lingshan]

This commit reports driver features to user space
only after FEATURES_OK is features negotiation is done.

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

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1363b42c9d28..52b7f5d23127 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_device, features_driver;
+	u64 features_device;
 	u16 val_u16;
 
 	vdev->config->get_config(vdev, 0, &config, sizeof(config));
@@ -832,11 +832,6 @@ 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_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,
@@ -850,6 +845,8 @@ static int
 vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid, u32 seq,
 		     int flags, struct netlink_ext_ack *extack)
 {
+	u64 features_driver;
+	u8 status = 0;
 	u32 device_id;
 	void *hdr;
 	int err;
@@ -873,6 +870,17 @@ vdpa_dev_config_fill(struct vdpa_device *vdev, struct sk_buff *msg, u32 portid,
 		goto msg_err;
 	}
 
+	/* only read driver features after the feature negotiation is done */
+	status = vdev->config->get_status(vdev);
+	if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
+		features_driver = vdev->config->get_driver_features(vdev);
+		if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_NEGOTIATED_FEATURES, features_driver,
+				      VDPA_ATTR_PAD)) {
+			err = -EMSGSIZE;
+			goto msg_err;
+		}
+	}
+
 	switch (device_id) {
 	case VIRTIO_ID_NET:
 		err = vdpa_dev_net_config_fill(vdev, msg);
-- 
2.31.1

[PATCH V3 3/6] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence

[Zhu Lingshan]

virtio 1.2 spec says:
max_virtqueue_pairs only exists if VIRTIO_NET_F_MQ or
VIRTIO_NET_F_RSS is set.

So when reporint MQ to userspace, it should check both
VIRTIO_NET_F_MQ and VIRTIO_NET_F_RSS.

unused parameter struct vdpa_device *vdev is removed

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.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 52b7f5d23127..70448deb9cd9 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -799,13 +799,13 @@ static int vdpa_nl_cmd_dev_get_dumpit(struct sk_buff *msg, struct netlink_callba
 	return msg->len;
 }
 
-static int vdpa_dev_net_mq_config_fill(struct vdpa_device *vdev,
-				       struct sk_buff *msg, u64 features,
+static int vdpa_dev_net_mq_config_fill(struct sk_buff *msg, u64 features,
 				       const struct virtio_net_config *config)
 {
 	u16 val_u16;
 
-	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0)
+	if ((features & BIT_ULL(VIRTIO_NET_F_MQ)) == 0 &&
+	    (features & BIT_ULL(VIRTIO_NET_F_RSS)) == 0)
 		return 0;
 
 	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
-- 
2.31.1

[PATCH V3 4/6] vDPA: check virtio device features to detect MQ

[Zhu Lingshan]

vdpa_dev_net_mq_config_fill() should checks device features
for MQ than driver features.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 70448deb9cd9..4b13bb7f355d 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -838,7 +838,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(msg, features_device, &config);
 }
 
 static int
-- 
2.31.1

[PATCH V3 5/6] vDPA: fix spars cast warning in vdpa_dev_net_mq_config_fill

[Zhu Lingshan]

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

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 4b13bb7f355d..f10403c86af7 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -808,7 +808,8 @@ static int vdpa_dev_net_mq_config_fill(struct sk_buff *msg, u64 features,
 	    (features & BIT_ULL(VIRTIO_NET_F_RSS)) == 0)
 		return 0;
 
-	val_u16 = le16_to_cpu(config->max_virtqueue_pairs);
+	val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
+
 	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
 }
 
-- 
2.31.1

[PATCH V3 6/6] vDPA: conditionally read MTU and MAC in dev cfg space

[Zhu Lingshan]

The spec says:
mtu only exists if VIRTIO_NET_F_MTU is set
The mac address field always exists (though
is only valid if VIRTIO_NET_F_MAC is set)

So vdpa_dev_net_config_fill() should read MTU and MAC
conditionally on the feature bits.

Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/vdpa/vdpa.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index f10403c86af7..02491341f9ee 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -813,6 +813,29 @@ static int vdpa_dev_net_mq_config_fill(struct sk_buff *msg, u64 features,
 	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
 }
 
+static int vdpa_dev_net_mtu_config_fill(struct sk_buff *msg, u64 features,
+					const struct virtio_net_config *config)
+{
+	u16 val_u16;
+
+	if ((features & BIT_ULL(VIRTIO_NET_F_MTU)) == 0)
+		return 0;
+
+	val_u16 = __virtio16_to_cpu(true, config->mtu);
+
+	return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16);
+}
+
+static int vdpa_dev_net_mac_config_fill(struct sk_buff *msg, u64 features,
+					const struct virtio_net_config *config)
+{
+	if ((features & BIT_ULL(VIRTIO_NET_F_MAC)) == 0)
+		return 0;
+
+	return  nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR,
+			sizeof(config->mac), config->mac);
+}
+
 static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
 {
 	struct virtio_net_config config = {};
@@ -821,24 +844,22 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
 
 	vdev->config->get_config(vdev, 0, &config, sizeof(config));
 
-	if (nla_put(msg, VDPA_ATTR_DEV_NET_CFG_MACADDR, sizeof(config.mac),
-		    config.mac))
-		return -EMSGSIZE;
-
 	val_u16 = __virtio16_to_cpu(true, config.status);
 	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
 		return -EMSGSIZE;
 
-	val_u16 = __virtio16_to_cpu(true, config.mtu);
-	if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MTU, val_u16))
-		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;
 
+	if (vdpa_dev_net_mtu_config_fill(msg, features_device, &config))
+		return -EMSGSIZE;
+
+	if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
+		return -EMSGSIZE;
+
 	return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
 }
 
-- 
2.31.1

Re: [PATCH V3 0/6] Conditionally read fields in dev cfg space

[Zhu, Lingshan]

Hi Michael,

Jason starts his vacation this afternoon, and next week is our national 
holiday.
He has acked 3 ~ 6 of this series before, and I have made improvements 
based on his comments.
Do you have any comments on patches 1 and 2?

Thanks,
Zhu Lingshan
On 9/29/2022 9:45 AM, Zhu Lingshan wrote:
> This series intends to read the fields in virtio-net device
> configuration space conditionally on the feature bits,
> this means:
>
> MTU exists if VIRTIO_NET_F_MTU is set
> MAC exists if VIRTIO_NET_F_NET is set
> MQ exists if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.
>
> This series report device features to userspace and invokes
> vdpa_config_ops.get_config() rather than
> vdpa_get_config_unlocked() to read the device config spcae,
> so no races in vdpa_set_features_unlocked()
>
> Thanks!
>
> Changes form V2:
> remove unnacessary checking for vdev->config->get_status (Jason)
>
> Changes from V1:
> 1)Better comments for VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> only in the header file(Jason)
> 2)Split original 3/4 into separate patches(Jason)
> 3)Check FEATURES_OK for reporting driver features
> in vdpa_dev_config_fill (Jason)
> 4) Add iproute2 example for reporting device features
>
> Zhu Lingshan (6):
>    vDPA: allow userspace to query features of a vDPA device
>    vDPA: only report driver features if FEATURES_OK is set
>    vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
>    vDPA: check virtio device features to detect MQ
>    vDPA: fix spars cast warning in vdpa_dev_net_mq_config_fill
>    vDPA: conditionally read MTU and MAC in dev cfg space
>
>   drivers/vdpa/vdpa.c       | 68 ++++++++++++++++++++++++++++++---------
>   include/uapi/linux/vdpa.h |  4 +++
>   2 files changed, 56 insertions(+), 16 deletions(-)
>

Re: [PATCH V3 0/6] Conditionally read fields in dev cfg space

[Michael S. Tsirkin]

On Thu, Sep 29, 2022 at 03:23:46PM +0800, Zhu, Lingshan wrote:
> Hi Michael,
> 
> Jason starts his vacation this afternoon, and next week is our national
> holiday.
> He has acked 3 ~ 6 of this series before, and I have made improvements based
> on his comments.
> Do you have any comments on patches 1 and 2?


No, I'll merge for next.

> Thanks,
> Zhu Lingshan
> On 9/29/2022 9:45 AM, Zhu Lingshan wrote:
> > This series intends to read the fields in virtio-net device
> > configuration space conditionally on the feature bits,
> > this means:
> > 
> > MTU exists if VIRTIO_NET_F_MTU is set
> > MAC exists if VIRTIO_NET_F_NET is set
> > MQ exists if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.
> > 
> > This series report device features to userspace and invokes
> > vdpa_config_ops.get_config() rather than
> > vdpa_get_config_unlocked() to read the device config spcae,
> > so no races in vdpa_set_features_unlocked()
> > 
> > Thanks!
> > 
> > Changes form V2:
> > remove unnacessary checking for vdev->config->get_status (Jason)
> > 
> > Changes from V1:
> > 1)Better comments for VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
> > only in the header file(Jason)
> > 2)Split original 3/4 into separate patches(Jason)
> > 3)Check FEATURES_OK for reporting driver features
> > in vdpa_dev_config_fill (Jason)
> > 4) Add iproute2 example for reporting device features
> > 
> > Zhu Lingshan (6):
> >    vDPA: allow userspace to query features of a vDPA device
> >    vDPA: only report driver features if FEATURES_OK is set
> >    vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
> >    vDPA: check virtio device features to detect MQ
> >    vDPA: fix spars cast warning in vdpa_dev_net_mq_config_fill
> >    vDPA: conditionally read MTU and MAC in dev cfg space
> > 
> >   drivers/vdpa/vdpa.c       | 68 ++++++++++++++++++++++++++++++---------
> >   include/uapi/linux/vdpa.h |  4 +++
> >   2 files changed, 56 insertions(+), 16 deletions(-)
> > 

Re: [PATCH V3 0/6] Conditionally read fields in dev cfg space

[Zhu, Lingshan]

On 9/29/2022 3:38 PM, Michael S. Tsirkin wrote:
> On Thu, Sep 29, 2022 at 03:23:46PM +0800, Zhu, Lingshan wrote:
>> Hi Michael,
>>
>> Jason starts his vacation this afternoon, and next week is our national
>> holiday.
>> He has acked 3 ~ 6 of this series before, and I have made improvements based
>> on his comments.
>> Do you have any comments on patches 1 and 2?
>
> No, I'll merge for next.
Thanks!
>
>> Thanks,
>> Zhu Lingshan
>> On 9/29/2022 9:45 AM, Zhu Lingshan wrote:
>>> This series intends to read the fields in virtio-net device
>>> configuration space conditionally on the feature bits,
>>> this means:
>>>
>>> MTU exists if VIRTIO_NET_F_MTU is set
>>> MAC exists if VIRTIO_NET_F_NET is set
>>> MQ exists if VIRTIO_NET_F_MQ or VIRTIO_NET_F_RSS is set.
>>>
>>> This series report device features to userspace and invokes
>>> vdpa_config_ops.get_config() rather than
>>> vdpa_get_config_unlocked() to read the device config spcae,
>>> so no races in vdpa_set_features_unlocked()
>>>
>>> Thanks!
>>>
>>> Changes form V2:
>>> remove unnacessary checking for vdev->config->get_status (Jason)
>>>
>>> Changes from V1:
>>> 1)Better comments for VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,
>>> only in the header file(Jason)
>>> 2)Split original 3/4 into separate patches(Jason)
>>> 3)Check FEATURES_OK for reporting driver features
>>> in vdpa_dev_config_fill (Jason)
>>> 4) Add iproute2 example for reporting device features
>>>
>>> Zhu Lingshan (6):
>>>     vDPA: allow userspace to query features of a vDPA device
>>>     vDPA: only report driver features if FEATURES_OK is set
>>>     vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
>>>     vDPA: check virtio device features to detect MQ
>>>     vDPA: fix spars cast warning in vdpa_dev_net_mq_config_fill
>>>     vDPA: conditionally read MTU and MAC in dev cfg space
>>>
>>>    drivers/vdpa/vdpa.c       | 68 ++++++++++++++++++++++++++++++---------
>>>    include/uapi/linux/vdpa.h |  4 +++
>>>    2 files changed, 56 insertions(+), 16 deletions(-)
>>>