Re: [PATCH 1/2] vDPA: allow userspace to query features of a vDPA device

On 8/16/2022 10:07 AM, Parav Pandit wrote:

quoted

From: Zhu, Lingshan <redacted>
Sent: Monday, August 15, 2022 9:49 PM

On 8/16/2022 2:15 AM, Si-Wei Liu wrote:

quoted

On 8/15/2022 2:26 AM, Zhu Lingshan wrote:

quoted

This commit adds a new vDPA netlink attribution
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES. Userspace can query

features

quoted

quoted

of vDPA devices through this new attr.

Signed-off-by: Zhu Lingshan <redacted>
---
   drivers/vdpa/vdpa.c       | 17 +++++++++++++----
   include/uapi/linux/vdpa.h |  3 +++
   2 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index

c06c02704461..efb55a06e961 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,7 +817,7 @@ static int vdpa_dev_net_mq_config_fill(struct

vdpa_device *vdev,
   static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
struct sk_buff *msg)
   {
       struct virtio_net_config config = {};
-    u64 features;
+    u64 features_device, features_driver;
       u16 val_u16;
         vdpa_get_config_unlocked(vdev, 0, &config, sizeof(config));

@@ -832,12 +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,

quoted

quoted

features,
+    features_driver = vdev->config->get_driver_features(vdev);
+    if (nla_put_u64_64bit(msg,

VDPA_ATTR_DEV_NEGOTIATED_FEATURES,

quoted

quoted

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..d171b92ef522 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h

@@ -46,7 +46,10 @@ enum vdpa_attr {
         VDPA_ATTR_DEV_NEGOTIATED_FEATURES,    /* u64 */
       VDPA_ATTR_DEV_MGMTDEV_MAX_VQS,        /* u32 */
+    /* features of a vDPA management device */

Say comment as.
       /* Features of the vDPA management device matching the virtio feature bits 0 to 63 when queried on the mgmt. device.
         *  When returned on the vdpa device, it indicates virtio feature bits 0 to 63 of the vdpa device
         */

This still suggests to re-use the attr VDPA_ATTR_DEV_SUPPORTED_FEATURES, 
I think a new attr should be better and easier
for others.

quoted

quoted

quoted

       VDPA_ATTR_DEV_SUPPORTED_FEATURES,    /* u64 */
+    /* features of a vDPA device, e.g., /dev/vhost-vdpa0 */
+    VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES,    /* u64 */

Append to the end, please. Otherwise it breaks userspace ABI.

OK, will fix it in V2

I have read Se-Wei comment in the v4.
However I disagree, we don’t need to continue the past mistake done with the naming.

Please add a comment that VDPA_ATTR_DEV_SUPPORTED_FEATURES like above about the exception.
We established that there is no race condition either.
So no need to add new UAPI for some past history.

Yes, no race conditions, the thing is, it is not a good practice to 
re-use a netlink attr,
every attr should has its own purpose, no need to confuse others, I 
think we had this discussion before.

Thanks