On 9/21/2022 3:44 PM, Jason Wang wrote:
On Wed, Sep 21, 2022 at 2:00 PM Zhu, Lingshan <lingshan.zhu@xxxxxxxxx> wrote:On 9/21/2022 10:17 AM, Jason Wang wrote:On Tue, Sep 20, 2022 at 5:58 PM Zhu, Lingshan <lingshan.zhu@xxxxxxxxx> wrote:On 9/20/2022 10:02 AM, Jason Wang wrote:On Fri, Sep 9, 2022 at 5:05 PM Zhu Lingshan <lingshan.zhu@xxxxxxxxx> 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@xxxxxxxxx>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 discussionsI think it's more than sufficient to clarify the semantic where it is defined.OK, then only comments in the header fileif (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
ThanksAnd "/dev/vhost-vdpa0" is not a vDPA device but a vhost-vDPA device.will remove this example here. ThanksThanksThanks+ /* new attributes must be added above here */ VDPA_ATTR_MAX, }; -- 2.31.1
