* [PATCH 0/4] Conditionally read fields in dev cfg space
@ 2022-09-09 8:57 Zhu Lingshan
2022-09-09 8:57 ` [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan
` (3 more replies)
0 siblings, 4 replies; 28+ messages in thread
From: Zhu Lingshan @ 2022-09-09 8:57 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, netdev, kvm, 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() than vdpa_get_config_unlocked()
to read the device config spcae, so no raeces in
vdpa_set_features_unlocked()
Thanks!
Zhu Lingshan (4):
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: 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
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-09 8:57 [PATCH 0/4] Conditionally read fields in dev cfg space Zhu Lingshan
@ 2022-09-09 8:57 ` Zhu Lingshan
2022-09-20 2:02 ` Jason Wang
2022-09-09 8:57 ` [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set Zhu Lingshan
` (2 subsequent siblings)
3 siblings, 1 reply; 28+ messages in thread
From: Zhu Lingshan @ 2022-09-09 8:57 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, netdev, kvm, 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() than
vdpa_get_config_unlocked() to read the device config
spcae, so no raeces in vdpa_set_features_unlocked()
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
include/uapi/linux/vdpa.h | 4 ++++
2 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index c06c02704461..798a02c7aa94 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
err = -EMSGSIZE;
goto msg_err;
}
+
+ /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
mdev->supported_features, VDPA_ATTR_PAD)) {
err = -EMSGSIZE;
@@ -815,10 +817,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 +834,19 @@ 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);
+
+ /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
+ 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..97531b52dcbe 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 */
+ /* features of a 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 */
+ /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
+ VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
+
/* new attributes must be added above here */
VDPA_ATTR_MAX,
};
--
2.31.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-09 8:57 [PATCH 0/4] Conditionally read fields in dev cfg space Zhu Lingshan
2022-09-09 8:57 ` [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan
@ 2022-09-09 8:57 ` Zhu Lingshan
2022-09-20 2:16 ` Jason Wang
2022-09-09 8:57 ` [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence Zhu Lingshan
2022-09-09 8:57 ` [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space Zhu Lingshan
3 siblings, 1 reply; 28+ messages in thread
From: Zhu Lingshan @ 2022-09-09 8:57 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, netdev, kvm, Zhu Lingshan
vdpa_dev_net_config_fill() should only report driver features
to userspace after features negotiation is done.
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
drivers/vdpa/vdpa.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 798a02c7aa94..29d7e8858e6f 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
struct virtio_net_config config = {};
u64 features_device, features_driver;
u16 val_u16;
+ u8 status;
vdev->config->get_config(vdev, 0, &config, sizeof(config));
@@ -834,10 +835,14 @@ 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;
+ /* 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))
+ return -EMSGSIZE;
+ }
features_device = vdev->config->get_device_features(vdev);
--
2.31.1
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
2022-09-09 8:57 [PATCH 0/4] Conditionally read fields in dev cfg space Zhu Lingshan
2022-09-09 8:57 ` [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan
2022-09-09 8:57 ` [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set Zhu Lingshan
@ 2022-09-09 8:57 ` Zhu Lingshan
2022-09-20 2:17 ` Jason Wang
2022-09-09 8:57 ` [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space Zhu Lingshan
3 siblings, 1 reply; 28+ messages in thread
From: Zhu Lingshan @ 2022-09-09 8:57 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, netdev, kvm, 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.
This commit also fixes:
1) a spase warning by replacing le16_to_cpu with __virtio16_to_cpu.
2) vdpa_dev_net_mq_config_fill() should checks device features
for MQ than driver features.
3) struct vdpa_device *vdev is not needed for
vdpa_dev_net_mq_config_fill(), unused.
Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
---
drivers/vdpa/vdpa.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 29d7e8858e6f..f8ff61232421 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -801,16 +801,17 @@ 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);
+ val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
+
return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
}
@@ -851,7 +852,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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space
2022-09-09 8:57 [PATCH 0/4] Conditionally read fields in dev cfg space Zhu Lingshan
` (2 preceding siblings ...)
2022-09-09 8:57 ` [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence Zhu Lingshan
@ 2022-09-09 8:57 ` Zhu Lingshan
2022-09-20 2:17 ` Jason Wang
3 siblings, 1 reply; 28+ messages in thread
From: Zhu Lingshan @ 2022-09-09 8:57 UTC (permalink / raw)
To: jasowang, mst; +Cc: virtualization, netdev, kvm, 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>
---
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 f8ff61232421..b332388d3375 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -815,6 +815,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 = {};
@@ -824,18 +847,10 @@ 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;
-
/* only read driver features after the feature negotiation is done */
status = vdev->config->get_status(vdev);
if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
@@ -852,6 +867,12 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
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
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-09 8:57 ` [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan
@ 2022-09-20 2:02 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:02 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: mst, virtualization, netdev, kvm
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
>
> This commit invokes vdpa_config_ops.get_config() than
> vdpa_get_config_unlocked() to read the device config
> spcae, so no raeces in vdpa_set_features_unlocked()
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
It's better to share the userspace code as well.
> ---
> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> include/uapi/linux/vdpa.h | 4 ++++
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index c06c02704461..798a02c7aa94 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> err = -EMSGSIZE;
> goto msg_err;
> }
> +
> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
The code explains itself, there's no need for the comment.
> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> mdev->supported_features, VDPA_ATTR_PAD)) {
> err = -EMSGSIZE;
> @@ -815,10 +817,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 +834,19 @@ 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);
> +
> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> + 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..97531b52dcbe 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 */
> + /* features of a 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 */
>
> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
Thanks
> +
> /* new attributes must be added above here */
> VDPA_ATTR_MAX,
> };
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
@ 2022-09-20 2:02 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:02 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: netdev, virtualization, kvm, mst
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> This commit adds a new vDPA netlink attribution
> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> features of vDPA devices through this new attr.
>
> This commit invokes vdpa_config_ops.get_config() than
> vdpa_get_config_unlocked() to read the device config
> spcae, so no raeces in vdpa_set_features_unlocked()
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
It's better to share the userspace code as well.
> ---
> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> include/uapi/linux/vdpa.h | 4 ++++
> 2 files changed, 18 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index c06c02704461..798a02c7aa94 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> err = -EMSGSIZE;
> goto msg_err;
> }
> +
> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
The code explains itself, there's no need for the comment.
> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> mdev->supported_features, VDPA_ATTR_PAD)) {
> err = -EMSGSIZE;
> @@ -815,10 +817,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 +834,19 @@ 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);
> +
> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> + 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..97531b52dcbe 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 */
> + /* features of a 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 */
>
> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
Thanks
> +
> /* new attributes must be added above here */
> VDPA_ATTR_MAX,
> };
> --
> 2.31.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-09 8:57 ` [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set Zhu Lingshan
@ 2022-09-20 2:16 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:16 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: mst, virtualization, netdev, kvm
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> vdpa_dev_net_config_fill() should only report driver features
> to userspace after features negotiation is done.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/vdpa.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 798a02c7aa94..29d7e8858e6f 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> struct virtio_net_config config = {};
> u64 features_device, features_driver;
> u16 val_u16;
> + u8 status;
>
> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>
> @@ -834,10 +835,14 @@ 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;
> + /* only read driver features after the feature negotiation is done */
> + status = vdev->config->get_status(vdev);
> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
Any reason this is not checked in its caller as what it used to do before?
Thanks
> + 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);
>
> --
> 2.31.1
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
@ 2022-09-20 2:16 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:16 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: netdev, virtualization, kvm, mst
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> vdpa_dev_net_config_fill() should only report driver features
> to userspace after features negotiation is done.
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/vdpa.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 798a02c7aa94..29d7e8858e6f 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> struct virtio_net_config config = {};
> u64 features_device, features_driver;
> u16 val_u16;
> + u8 status;
>
> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>
> @@ -834,10 +835,14 @@ 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;
> + /* only read driver features after the feature negotiation is done */
> + status = vdev->config->get_status(vdev);
> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
Any reason this is not checked in its caller as what it used to do before?
Thanks
> + 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);
>
> --
> 2.31.1
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
2022-09-09 8:57 ` [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence Zhu Lingshan
@ 2022-09-20 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:17 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: netdev, virtualization, kvm, mst
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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.
>
> This commit also fixes:
> 1) a spase warning by replacing le16_to_cpu with __virtio16_to_cpu.
> 2) vdpa_dev_net_mq_config_fill() should checks device features
> for MQ than driver features.
> 3) struct vdpa_device *vdev is not needed for
> vdpa_dev_net_mq_config_fill(), unused.
Let's do those in separate patches please.
Thanks
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/vdpa.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 29d7e8858e6f..f8ff61232421 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -801,16 +801,17 @@ 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);
> + val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
> +
> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
> }
>
> @@ -851,7 +852,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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
@ 2022-09-20 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:17 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: mst, virtualization, netdev, kvm
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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.
>
> This commit also fixes:
> 1) a spase warning by replacing le16_to_cpu with __virtio16_to_cpu.
> 2) vdpa_dev_net_mq_config_fill() should checks device features
> for MQ than driver features.
> 3) struct vdpa_device *vdev is not needed for
> vdpa_dev_net_mq_config_fill(), unused.
Let's do those in separate patches please.
Thanks
>
> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> ---
> drivers/vdpa/vdpa.c | 11 ++++++-----
> 1 file changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 29d7e8858e6f..f8ff61232421 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -801,16 +801,17 @@ 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);
> + val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
> +
> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
> }
>
> @@ -851,7 +852,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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space
2022-09-09 8:57 ` [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space Zhu Lingshan
@ 2022-09-20 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:17 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: mst, virtualization, netdev, kvm
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 f8ff61232421..b332388d3375 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,6 +815,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 = {};
> @@ -824,18 +847,10 @@ 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;
> -
> /* only read driver features after the feature negotiation is done */
> status = vdev->config->get_status(vdev);
> if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> @@ -852,6 +867,12 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> 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
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space
@ 2022-09-20 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-20 2:17 UTC (permalink / raw)
To: Zhu Lingshan; +Cc: netdev, virtualization, kvm, mst
On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>
> 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 f8ff61232421..b332388d3375 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -815,6 +815,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 = {};
> @@ -824,18 +847,10 @@ 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;
> -
> /* only read driver features after the feature negotiation is done */
> status = vdev->config->get_status(vdev);
> if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> @@ -852,6 +867,12 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> 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
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-20 2:16 ` Jason Wang
(?)
@ 2022-09-20 5:46 ` Zhu, Lingshan
2022-09-21 2:14 ` Jason Wang
-1 siblings, 1 reply; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-20 5:46 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/20/2022 10:16 AM, Jason Wang wrote:
> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> vdpa_dev_net_config_fill() should only report driver features
>> to userspace after features negotiation is done.
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/vdpa.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 798a02c7aa94..29d7e8858e6f 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>> struct virtio_net_config config = {};
>> u64 features_device, features_driver;
>> u16 val_u16;
>> + u8 status;
>>
>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>
>> @@ -834,10 +835,14 @@ 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;
>> + /* only read driver features after the feature negotiation is done */
>> + status = vdev->config->get_status(vdev);
>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> Any reason this is not checked in its caller as what it used to do before?
will check the existence of vdev->config->get_status before calling it in V2
Thanks,
Zhu Lingshan
>
> Thanks
>
>> + 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);
>>
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence
2022-09-20 2:17 ` Jason Wang
(?)
@ 2022-09-20 9:54 ` Zhu, Lingshan
-1 siblings, 0 replies; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-20 9:54 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/20/2022 10:17 AM, Jason Wang wrote:
> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> 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.
>>
>> This commit also fixes:
>> 1) a spase warning by replacing le16_to_cpu with __virtio16_to_cpu.
>> 2) vdpa_dev_net_mq_config_fill() should checks device features
>> for MQ than driver features.
>> 3) struct vdpa_device *vdev is not needed for
>> vdpa_dev_net_mq_config_fill(), unused.
> Let's do those in separate patches please.
will do, thanks!
>
> Thanks
>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>> ---
>> drivers/vdpa/vdpa.c | 11 ++++++-----
>> 1 file changed, 6 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 29d7e8858e6f..f8ff61232421 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -801,16 +801,17 @@ 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);
>> + val_u16 = __virtio16_to_cpu(true, config->max_virtqueue_pairs);
>> +
>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, val_u16);
>> }
>>
>> @@ -851,7 +852,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
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-20 2:02 ` Jason Wang
(?)
@ 2022-09-20 9:58 ` Zhu, Lingshan
2022-09-21 2:17 ` Jason Wang
-1 siblings, 1 reply; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-20 9:58 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/20/2022 10:02 AM, Jason Wang wrote:
> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>> This commit adds a new vDPA netlink attribution
>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>> features of vDPA devices through this new attr.
>>
>> This commit invokes vdpa_config_ops.get_config() than
>> vdpa_get_config_unlocked() to read the device config
>> spcae, so no raeces in vdpa_set_features_unlocked()
>>
>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> It's better to share the userspace code as well.
OK, will share it in V2.
>
>> ---
>> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
>> include/uapi/linux/vdpa.h | 4 ++++
>> 2 files changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index c06c02704461..798a02c7aa94 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>> err = -EMSGSIZE;
>> goto msg_err;
>> }
>> +
>> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
> The code explains itself, there's no need for the comment.
these comments are required in other discussions
>
>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
>> mdev->supported_features, VDPA_ATTR_PAD)) {
>> err = -EMSGSIZE;
>> @@ -815,10 +817,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 +834,19 @@ 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);
>> +
>> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
>> + 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..97531b52dcbe 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 */
>> + /* features of a 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 */
>>
>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
This is to report a vDPA device features, and
VDPA_ATTR_DEV_SUPPORTED_FEATURES
is used for reporting the management device features, we have a long
discussion
on this before.
>
> Thanks
>
>> +
>> /* new attributes must be added above here */
>> VDPA_ATTR_MAX,
>> };
>> --
>> 2.31.1
>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-20 5:46 ` Zhu, Lingshan
@ 2022-09-21 2:14 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 2:14 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: mst, virtualization, netdev, kvm
On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/20/2022 10:16 AM, Jason Wang wrote:
> > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> vdpa_dev_net_config_fill() should only report driver features
> >> to userspace after features negotiation is done.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >> drivers/vdpa/vdpa.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 798a02c7aa94..29d7e8858e6f 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >> struct virtio_net_config config = {};
> >> u64 features_device, features_driver;
> >> u16 val_u16;
> >> + u8 status;
> >>
> >> vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>
> >> @@ -834,10 +835,14 @@ 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;
> >> + /* only read driver features after the feature negotiation is done */
> >> + status = vdev->config->get_status(vdev);
> >> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> > Any reason this is not checked in its caller as what it used to do before?
> will check the existence of vdev->config->get_status before calling it in V2
Just to clarify, I meant to check FEAUTRES_OK in the caller -
vdpa_dev_config_fill() otherwise each type needs to repeat this in
their specific codes.
Thanks
>
> Thanks,
> Zhu Lingshan
> >
> > Thanks
> >
> >> + 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);
> >>
> >> --
> >> 2.31.1
> >>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
@ 2022-09-21 2:14 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 2:14 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: netdev, virtualization, kvm, mst
On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/20/2022 10:16 AM, Jason Wang wrote:
> > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> vdpa_dev_net_config_fill() should only report driver features
> >> to userspace after features negotiation is done.
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >> ---
> >> drivers/vdpa/vdpa.c | 13 +++++++++----
> >> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index 798a02c7aa94..29d7e8858e6f 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >> struct virtio_net_config config = {};
> >> u64 features_device, features_driver;
> >> u16 val_u16;
> >> + u8 status;
> >>
> >> vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>
> >> @@ -834,10 +835,14 @@ 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;
> >> + /* only read driver features after the feature negotiation is done */
> >> + status = vdev->config->get_status(vdev);
> >> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> > Any reason this is not checked in its caller as what it used to do before?
> will check the existence of vdev->config->get_status before calling it in V2
Just to clarify, I meant to check FEAUTRES_OK in the caller -
vdpa_dev_config_fill() otherwise each type needs to repeat this in
their specific codes.
Thanks
>
> Thanks,
> Zhu Lingshan
> >
> > Thanks
> >
> >> + 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);
> >>
> >> --
> >> 2.31.1
> >>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-20 9:58 ` Zhu, Lingshan
@ 2022-09-21 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 2:17 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: mst, virtualization, netdev, kvm
On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/20/2022 10:02 AM, Jason Wang wrote:
> > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit adds a new vDPA netlink attribution
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> >> features of vDPA devices through this new attr.
> >>
> >> This commit invokes vdpa_config_ops.get_config() than
> >> vdpa_get_config_unlocked() to read the device config
> >> spcae, so no raeces in vdpa_set_features_unlocked()
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > It's better to share the userspace code as well.
> OK, will share it in V2.
> >
> >> ---
> >> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> >> include/uapi/linux/vdpa.h | 4 ++++
> >> 2 files changed, 18 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index c06c02704461..798a02c7aa94 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >> err = -EMSGSIZE;
> >> goto msg_err;
> >> }
> >> +
> >> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
> > The code explains itself, there's no need for the comment.
> these comments are required in other discussions
I think it's more than sufficient to clarify the semantic where it is defined.
> >
> >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> >> mdev->supported_features, VDPA_ATTR_PAD)) {
> >> err = -EMSGSIZE;
> >> @@ -815,10 +817,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 +834,19 @@ 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);
> >> +
> >> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> >> + 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..97531b52dcbe 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 */
> >> + /* features of a 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 */
> >>
> >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
> > What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
> This is to report a vDPA device features, and
> VDPA_ATTR_DEV_SUPPORTED_FEATURES
> is used for reporting the management device features, we have a long
> discussion
> on this before.
Yes, but the comment is not clear in many ways:
" features of a vDPA management device" sounds like features that is
out of the scope of the virtio.
And
"/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
Thanks
> >
> > Thanks
> >
> >> +
> >> /* new attributes must be added above here */
> >> VDPA_ATTR_MAX,
> >> };
> >> --
> >> 2.31.1
> >>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
@ 2022-09-21 2:17 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 2:17 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: netdev, virtualization, kvm, mst
On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/20/2022 10:02 AM, Jason Wang wrote:
> > On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >> This commit adds a new vDPA netlink attribution
> >> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> >> features of vDPA devices through this new attr.
> >>
> >> This commit invokes vdpa_config_ops.get_config() than
> >> vdpa_get_config_unlocked() to read the device config
> >> spcae, so no raeces in vdpa_set_features_unlocked()
> >>
> >> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> > It's better to share the userspace code as well.
> OK, will share it in V2.
> >
> >> ---
> >> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> >> include/uapi/linux/vdpa.h | 4 ++++
> >> 2 files changed, 18 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >> index c06c02704461..798a02c7aa94 100644
> >> --- a/drivers/vdpa/vdpa.c
> >> +++ b/drivers/vdpa/vdpa.c
> >> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >> err = -EMSGSIZE;
> >> goto msg_err;
> >> }
> >> +
> >> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
> > The code explains itself, there's no need for the comment.
> these comments are required in other discussions
I think it's more than sufficient to clarify the semantic where it is defined.
> >
> >> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> >> mdev->supported_features, VDPA_ATTR_PAD)) {
> >> err = -EMSGSIZE;
> >> @@ -815,10 +817,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 +834,19 @@ 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);
> >> +
> >> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> >> + 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..97531b52dcbe 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 */
> >> + /* features of a 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 */
> >>
> >> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> >> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
> > What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
> This is to report a vDPA device features, and
> VDPA_ATTR_DEV_SUPPORTED_FEATURES
> is used for reporting the management device features, we have a long
> discussion
> on this before.
Yes, but the comment is not clear in many ways:
" features of a vDPA management device" sounds like features that is
out of the scope of the virtio.
And
"/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
Thanks
> >
> > Thanks
> >
> >> +
> >> /* new attributes must be added above here */
> >> VDPA_ATTR_MAX,
> >> };
> >> --
> >> 2.31.1
> >>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-21 2:14 ` Jason Wang
(?)
@ 2022-09-21 5:38 ` Zhu, Lingshan
2022-09-21 7:43 ` Jason Wang
-1 siblings, 1 reply; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-21 5:38 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/21/2022 10:14 AM, Jason Wang wrote:
> On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 9/20/2022 10:16 AM, Jason Wang wrote:
>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> vdpa_dev_net_config_fill() should only report driver features
>>>> to userspace after features negotiation is done.
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>> ---
>>>> drivers/vdpa/vdpa.c | 13 +++++++++----
>>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index 798a02c7aa94..29d7e8858e6f 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>>> struct virtio_net_config config = {};
>>>> u64 features_device, features_driver;
>>>> u16 val_u16;
>>>> + u8 status;
>>>>
>>>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>
>>>> @@ -834,10 +835,14 @@ 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;
>>>> + /* only read driver features after the feature negotiation is done */
>>>> + status = vdev->config->get_status(vdev);
>>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
>>> Any reason this is not checked in its caller as what it used to do before?
>> will check the existence of vdev->config->get_status before calling it in V2
> Just to clarify, I meant to check FEAUTRES_OK in the caller -
> vdpa_dev_config_fill() otherwise each type needs to repeat this in
> their specific codes.
if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then
!FEATURES_OK will block reporting all attributions, for example
the device features and virtio device config space fields in this series
and device status.
Currently only driver features needs to check FEATURES_OK.
Or did I missed anything?
Thanks
>
> Thanks
>
>> Thanks,
>> Zhu Lingshan
>>> Thanks
>>>
>>>> + 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);
>>>>
>>>> --
>>>> 2.31.1
>>>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-21 2:17 ` Jason Wang
(?)
@ 2022-09-21 5:59 ` Zhu, Lingshan
2022-09-21 7:44 ` Jason Wang
-1 siblings, 1 reply; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-21 5:59 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/21/2022 10:17 AM, Jason Wang wrote:
> On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 9/20/2022 10:02 AM, Jason Wang wrote:
>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>> This commit adds a new vDPA netlink attribution
>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>>>> features of vDPA devices through this new attr.
>>>>
>>>> This commit invokes vdpa_config_ops.get_config() than
>>>> vdpa_get_config_unlocked() to read the device config
>>>> spcae, so no raeces in vdpa_set_features_unlocked()
>>>>
>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>> It's better to share the userspace code as well.
>> OK, will share it in V2.
>>>> ---
>>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
>>>> include/uapi/linux/vdpa.h | 4 ++++
>>>> 2 files changed, 18 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>> index c06c02704461..798a02c7aa94 100644
>>>> --- a/drivers/vdpa/vdpa.c
>>>> +++ b/drivers/vdpa/vdpa.c
>>>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>> err = -EMSGSIZE;
>>>> goto msg_err;
>>>> }
>>>> +
>>>> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
>>> The code explains itself, there's no need for the comment.
>> these comments are required in other discussions
> I think it's more than sufficient to clarify the semantic where it is defined.
OK, then only comments in the header file
>
>>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
>>>> mdev->supported_features, VDPA_ATTR_PAD)) {
>>>> err = -EMSGSIZE;
>>>> @@ -815,10 +817,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 +834,19 @@ 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);
>>>> +
>>>> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
>>>> + 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..97531b52dcbe 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 */
>>>> + /* features of a 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 */
>>>>
>>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
>>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
>>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
>> This is to report a vDPA device features, and
>> VDPA_ATTR_DEV_SUPPORTED_FEATURES
>> is used for reporting the management device features, we have a long
>> discussion
>> on this before.
> Yes, but the comment is not clear in many ways:
>
> " features of a vDPA management device" sounds like features that is
> out of the scope of the virtio.
I think the term "vDPA device" implies that it is a virtio device.
So how about: "virtio features of a vDPA management device"
>
> And
>
> "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
will remove this example here.
Thanks
>
> Thanks
>
>>> Thanks
>>>
>>>> +
>>>> /* new attributes must be added above here */
>>>> VDPA_ATTR_MAX,
>>>> };
>>>> --
>>>> 2.31.1
>>>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-21 5:38 ` Zhu, Lingshan
@ 2022-09-21 7:43 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 7:43 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: mst, virtualization, netdev, kvm
On Wed, Sep 21, 2022 at 1:39 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/21/2022 10:14 AM, Jason Wang wrote:
> > On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 9/20/2022 10:16 AM, Jason Wang wrote:
> >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> vdpa_dev_net_config_fill() should only report driver features
> >>>> to userspace after features negotiation is done.
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> ---
> >>>> drivers/vdpa/vdpa.c | 13 +++++++++----
> >>>> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>>> index 798a02c7aa94..29d7e8858e6f 100644
> >>>> --- a/drivers/vdpa/vdpa.c
> >>>> +++ b/drivers/vdpa/vdpa.c
> >>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >>>> struct virtio_net_config config = {};
> >>>> u64 features_device, features_driver;
> >>>> u16 val_u16;
> >>>> + u8 status;
> >>>>
> >>>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>>>
> >>>> @@ -834,10 +835,14 @@ 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;
> >>>> + /* only read driver features after the feature negotiation is done */
> >>>> + status = vdev->config->get_status(vdev);
> >>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> >>> Any reason this is not checked in its caller as what it used to do before?
> >> will check the existence of vdev->config->get_status before calling it in V2
> > Just to clarify, I meant to check FEAUTRES_OK in the caller -
> > vdpa_dev_config_fill() otherwise each type needs to repeat this in
> > their specific codes.
> if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then
> !FEATURES_OK will block reporting all attributions, for example
> the device features and virtio device config space fields in this series
> and device status.
> Currently only driver features needs to check FEATURES_OK.
> Or did I missed anything?
I don't see much difference, we just move the following part to the
caller, it is not the config but the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
is blocked.
Thanks
>
> Thanks
> >
> > Thanks
> >
> >> Thanks,
> >> Zhu Lingshan
> >>> Thanks
> >>>
> >>>> + 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);
> >>>>
> >>>> --
> >>>> 2.31.1
> >>>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
@ 2022-09-21 7:43 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 7:43 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: netdev, virtualization, kvm, mst
On Wed, Sep 21, 2022 at 1:39 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/21/2022 10:14 AM, Jason Wang wrote:
> > On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 9/20/2022 10:16 AM, Jason Wang wrote:
> >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> vdpa_dev_net_config_fill() should only report driver features
> >>>> to userspace after features negotiation is done.
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>>> ---
> >>>> drivers/vdpa/vdpa.c | 13 +++++++++----
> >>>> 1 file changed, 9 insertions(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>>> index 798a02c7aa94..29d7e8858e6f 100644
> >>>> --- a/drivers/vdpa/vdpa.c
> >>>> +++ b/drivers/vdpa/vdpa.c
> >>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >>>> struct virtio_net_config config = {};
> >>>> u64 features_device, features_driver;
> >>>> u16 val_u16;
> >>>> + u8 status;
> >>>>
> >>>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
> >>>>
> >>>> @@ -834,10 +835,14 @@ 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;
> >>>> + /* only read driver features after the feature negotiation is done */
> >>>> + status = vdev->config->get_status(vdev);
> >>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
> >>> Any reason this is not checked in its caller as what it used to do before?
> >> will check the existence of vdev->config->get_status before calling it in V2
> > Just to clarify, I meant to check FEAUTRES_OK in the caller -
> > vdpa_dev_config_fill() otherwise each type needs to repeat this in
> > their specific codes.
> if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then
> !FEATURES_OK will block reporting all attributions, for example
> the device features and virtio device config space fields in this series
> and device status.
> Currently only driver features needs to check FEATURES_OK.
> Or did I missed anything?
I don't see much difference, we just move the following part to the
caller, it is not the config but the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
is blocked.
Thanks
>
> Thanks
> >
> > Thanks
> >
> >> Thanks,
> >> Zhu Lingshan
> >>> Thanks
> >>>
> >>>> + 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);
> >>>>
> >>>> --
> >>>> 2.31.1
> >>>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-21 5:59 ` Zhu, Lingshan
@ 2022-09-21 7:44 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 7:44 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: mst, virtualization, netdev, kvm
On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/21/2022 10:17 AM, Jason Wang wrote:
> > On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 9/20/2022 10:02 AM, Jason Wang wrote:
> >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> This commit adds a new vDPA netlink attribution
> >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> >>>> features of vDPA devices through this new attr.
> >>>>
> >>>> This commit invokes vdpa_config_ops.get_config() than
> >>>> vdpa_get_config_unlocked() to read the device config
> >>>> spcae, so no raeces in vdpa_set_features_unlocked()
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> It's better to share the userspace code as well.
> >> OK, will share it in V2.
> >>>> ---
> >>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> >>>> include/uapi/linux/vdpa.h | 4 ++++
> >>>> 2 files changed, 18 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>>> index c06c02704461..798a02c7aa94 100644
> >>>> --- a/drivers/vdpa/vdpa.c
> >>>> +++ b/drivers/vdpa/vdpa.c
> >>>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >>>> err = -EMSGSIZE;
> >>>> goto msg_err;
> >>>> }
> >>>> +
> >>>> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
> >>> The code explains itself, there's no need for the comment.
> >> these comments are required in other discussions
> > I think it's more than sufficient to clarify the semantic where it is defined.
> OK, then only comments in the header file
> >
> >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> >>>> mdev->supported_features, VDPA_ATTR_PAD)) {
> >>>> err = -EMSGSIZE;
> >>>> @@ -815,10 +817,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 +834,19 @@ 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);
> >>>> +
> >>>> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> >>>> + 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..97531b52dcbe 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 */
> >>>> + /* features of a 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 */
> >>>>
> >>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
> >>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
> >> This is to report a vDPA device features, and
> >> VDPA_ATTR_DEV_SUPPORTED_FEATURES
> >> is used for reporting the management device features, we have a long
> >> discussion
> >> on this before.
> > Yes, but the comment is not clear in many ways:
> >
> > " features of a vDPA management device" sounds like features that is
> > out of the scope of the virtio.
> I think the term "vDPA device" implies that it is a virtio device.
> So how about: "virtio features of a vDPA management device"
Not a native speaker, but how about "virtio features that are
supported by the vDPA management device?"
Thanks
> >
> > And
> >
> > "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
> will remove this example here.
>
> Thanks
> >
> > Thanks
> >
> >>> Thanks
> >>>
> >>>> +
> >>>> /* new attributes must be added above here */
> >>>> VDPA_ATTR_MAX,
> >>>> };
> >>>> --
> >>>> 2.31.1
> >>>>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
@ 2022-09-21 7:44 ` Jason Wang
0 siblings, 0 replies; 28+ messages in thread
From: Jason Wang @ 2022-09-21 7:44 UTC (permalink / raw)
To: Zhu, Lingshan; +Cc: netdev, virtualization, kvm, mst
On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>
>
>
> On 9/21/2022 10:17 AM, Jason Wang wrote:
> > On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
> >>
> >>
> >> On 9/20/2022 10:02 AM, Jason Wang wrote:
> >>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
> >>>> This commit adds a new vDPA netlink attribution
> >>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
> >>>> features of vDPA devices through this new attr.
> >>>>
> >>>> This commit invokes vdpa_config_ops.get_config() than
> >>>> vdpa_get_config_unlocked() to read the device config
> >>>> spcae, so no raeces in vdpa_set_features_unlocked()
> >>>>
> >>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
> >>> It's better to share the userspace code as well.
> >> OK, will share it in V2.
> >>>> ---
> >>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
> >>>> include/uapi/linux/vdpa.h | 4 ++++
> >>>> 2 files changed, 18 insertions(+), 5 deletions(-)
> >>>>
> >>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> >>>> index c06c02704461..798a02c7aa94 100644
> >>>> --- a/drivers/vdpa/vdpa.c
> >>>> +++ b/drivers/vdpa/vdpa.c
> >>>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
> >>>> err = -EMSGSIZE;
> >>>> goto msg_err;
> >>>> }
> >>>> +
> >>>> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
> >>> The code explains itself, there's no need for the comment.
> >> these comments are required in other discussions
> > I think it's more than sufficient to clarify the semantic where it is defined.
> OK, then only comments in the header file
> >
> >>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
> >>>> mdev->supported_features, VDPA_ATTR_PAD)) {
> >>>> err = -EMSGSIZE;
> >>>> @@ -815,10 +817,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 +834,19 @@ 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);
> >>>> +
> >>>> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
> >>>> + 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..97531b52dcbe 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 */
> >>>> + /* features of a 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 */
> >>>>
> >>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
> >>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
> >>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
> >> This is to report a vDPA device features, and
> >> VDPA_ATTR_DEV_SUPPORTED_FEATURES
> >> is used for reporting the management device features, we have a long
> >> discussion
> >> on this before.
> > Yes, but the comment is not clear in many ways:
> >
> > " features of a vDPA management device" sounds like features that is
> > out of the scope of the virtio.
> I think the term "vDPA device" implies that it is a virtio device.
> So how about: "virtio features of a vDPA management device"
Not a native speaker, but how about "virtio features that are
supported by the vDPA management device?"
Thanks
> >
> > And
> >
> > "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
> will remove this example here.
>
> Thanks
> >
> > Thanks
> >
> >>> Thanks
> >>>
> >>>> +
> >>>> /* new attributes must be added above here */
> >>>> VDPA_ATTR_MAX,
> >>>> };
> >>>> --
> >>>> 2.31.1
> >>>>
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device
2022-09-21 7:44 ` Jason Wang
(?)
@ 2022-09-21 7:51 ` Zhu, Lingshan
-1 siblings, 0 replies; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-21 7:51 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/21/2022 3:44 PM, Jason Wang wrote:
> On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 9/21/2022 10:17 AM, Jason Wang wrote:
>>> On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>
>>>> On 9/20/2022 10:02 AM, Jason Wang wrote:
>>>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> This commit adds a new vDPA netlink attribution
>>>>>> VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query
>>>>>> features of vDPA devices through this new attr.
>>>>>>
>>>>>> This commit invokes vdpa_config_ops.get_config() than
>>>>>> vdpa_get_config_unlocked() to read the device config
>>>>>> spcae, so no raeces in vdpa_set_features_unlocked()
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>> It's better to share the userspace code as well.
>>>> OK, will share it in V2.
>>>>>> ---
>>>>>> drivers/vdpa/vdpa.c | 19 ++++++++++++++-----
>>>>>> include/uapi/linux/vdpa.h | 4 ++++
>>>>>> 2 files changed, 18 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>>> index c06c02704461..798a02c7aa94 100644
>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>> @@ -491,6 +491,8 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
>>>>>> err = -EMSGSIZE;
>>>>>> goto msg_err;
>>>>>> }
>>>>>> +
>>>>>> + /* report features of a vDPA management device through VDPA_ATTR_DEV_SUPPORTED_FEATURES */
>>>>> The code explains itself, there's no need for the comment.
>>>> these comments are required in other discussions
>>> I think it's more than sufficient to clarify the semantic where it is defined.
>> OK, then only comments in the header file
>>>>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_SUPPORTED_FEATURES,
>>>>>> mdev->supported_features, VDPA_ATTR_PAD)) {
>>>>>> err = -EMSGSIZE;
>>>>>> @@ -815,10 +817,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 +834,19 @@ 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);
>>>>>> +
>>>>>> + /* report features of a vDPA device through VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES */
>>>>>> + 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..97531b52dcbe 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 */
>>>>>> + /* features of a 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 */
>>>>>>
>>>>>> + /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
>>>>>> + VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES, /* u64 */
>>>>> What's the difference between this and VDPA_ATTR_DEV_SUPPORTED_FEATURES?
>>>> This is to report a vDPA device features, and
>>>> VDPA_ATTR_DEV_SUPPORTED_FEATURES
>>>> is used for reporting the management device features, we have a long
>>>> discussion
>>>> on this before.
>>> Yes, but the comment is not clear in many ways:
>>>
>>> " features of a vDPA management device" sounds like features that is
>>> out of the scope of the virtio.
>> I think the term "vDPA device" implies that it is a virtio device.
>> So how about: "virtio features of a vDPA management device"
> Not a native speaker, but how about "virtio features that are
> supported by the vDPA management device?"
OK
>
> Thanks
>
>>> And
>>>
>>> "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.
>> will remove this example here.
>>
>> Thanks
>>> Thanks
>>>
>>>>> Thanks
>>>>>
>>>>>> +
>>>>>> /* new attributes must be added above here */
>>>>>> VDPA_ATTR_MAX,
>>>>>> };
>>>>>> --
>>>>>> 2.31.1
>>>>>>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set
2022-09-21 7:43 ` Jason Wang
(?)
@ 2022-09-21 7:53 ` Zhu, Lingshan
-1 siblings, 0 replies; 28+ messages in thread
From: Zhu, Lingshan @ 2022-09-21 7:53 UTC (permalink / raw)
To: Jason Wang; +Cc: mst, virtualization, netdev, kvm
On 9/21/2022 3:43 PM, Jason Wang wrote:
> On Wed, Sep 21, 2022 at 1:39 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>
>>
>> On 9/21/2022 10:14 AM, Jason Wang wrote:
>>> On Tue, Sep 20, 2022 at 1:46 PM Zhu, Lingshan <lingshan.zhu@intel.com> wrote:
>>>>
>>>> On 9/20/2022 10:16 AM, Jason Wang wrote:
>>>>> On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@intel.com> wrote:
>>>>>> vdpa_dev_net_config_fill() should only report driver features
>>>>>> to userspace after features negotiation is done.
>>>>>>
>>>>>> Signed-off-by: Zhu Lingshan <lingshan.zhu@intel.com>
>>>>>> ---
>>>>>> drivers/vdpa/vdpa.c | 13 +++++++++----
>>>>>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>>>>> index 798a02c7aa94..29d7e8858e6f 100644
>>>>>> --- a/drivers/vdpa/vdpa.c
>>>>>> +++ b/drivers/vdpa/vdpa.c
>>>>>> @@ -819,6 +819,7 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>>>>> struct virtio_net_config config = {};
>>>>>> u64 features_device, features_driver;
>>>>>> u16 val_u16;
>>>>>> + u8 status;
>>>>>>
>>>>>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>>>>>
>>>>>> @@ -834,10 +835,14 @@ 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;
>>>>>> + /* only read driver features after the feature negotiation is done */
>>>>>> + status = vdev->config->get_status(vdev);
>>>>>> + if (status & VIRTIO_CONFIG_S_FEATURES_OK) {
>>>>> Any reason this is not checked in its caller as what it used to do before?
>>>> will check the existence of vdev->config->get_status before calling it in V2
>>> Just to clarify, I meant to check FEAUTRES_OK in the caller -
>>> vdpa_dev_config_fill() otherwise each type needs to repeat this in
>>> their specific codes.
>> if we check FEATURES_OK in the caller vdpa_dev_config_fill(), then
>> !FEATURES_OK will block reporting all attributions, for example
>> the device features and virtio device config space fields in this series
>> and device status.
>> Currently only driver features needs to check FEATURES_OK.
>> Or did I missed anything?
> I don't see much difference, we just move the following part to the
> caller, it is not the config but the VDPA_ATTR_DEV_NEGOTIATED_FEATURES
> is blocked.
OK, I get you. V2 will check FEATURES_OK and report driver features in
vdpa_dev_config_fill()
Thanks
Zhu Lingshan
>
> Thanks
>
>> Thanks
>>> Thanks
>>>
>>>> Thanks,
>>>> Zhu Lingshan
>>>>> Thanks
>>>>>
>>>>>> + 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);
>>>>>>
>>>>>> --
>>>>>> 2.31.1
>>>>>>
^ permalink raw reply [flat|nested] 28+ messages in thread
end of thread, other threads:[~2022-09-21 7:53 UTC | newest]
Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-09 8:57 [PATCH 0/4] Conditionally read fields in dev cfg space Zhu Lingshan
2022-09-09 8:57 ` [PATCH 1/4] vDPA: allow userspace to query features of a vDPA device Zhu Lingshan
2022-09-20 2:02 ` Jason Wang
2022-09-20 2:02 ` Jason Wang
2022-09-20 9:58 ` Zhu, Lingshan
2022-09-21 2:17 ` Jason Wang
2022-09-21 2:17 ` Jason Wang
2022-09-21 5:59 ` Zhu, Lingshan
2022-09-21 7:44 ` Jason Wang
2022-09-21 7:44 ` Jason Wang
2022-09-21 7:51 ` Zhu, Lingshan
2022-09-09 8:57 ` [PATCH 2/4] vDPA: only report driver features if FEATURES_OK is set Zhu Lingshan
2022-09-20 2:16 ` Jason Wang
2022-09-20 2:16 ` Jason Wang
2022-09-20 5:46 ` Zhu, Lingshan
2022-09-21 2:14 ` Jason Wang
2022-09-21 2:14 ` Jason Wang
2022-09-21 5:38 ` Zhu, Lingshan
2022-09-21 7:43 ` Jason Wang
2022-09-21 7:43 ` Jason Wang
2022-09-21 7:53 ` Zhu, Lingshan
2022-09-09 8:57 ` [PATCH 3/4] vDPA: check VIRTIO_NET_F_RSS for max_virtqueue_paris's presence Zhu Lingshan
2022-09-20 2:17 ` Jason Wang
2022-09-20 2:17 ` Jason Wang
2022-09-20 9:54 ` Zhu, Lingshan
2022-09-09 8:57 ` [PATCH 4/4] vDPA: Conditionally read MTU and MAC in dev cfg space Zhu Lingshan
2022-09-20 2:17 ` Jason Wang
2022-09-20 2:17 ` Jason Wang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.