On Tue, Oct 18, 2022 at 3:55 PM Zhu, Lingshan [off-list ref] wrote:

On 10/18/2022 3:49 PM, Jason Wang wrote:

quoted

On Tue, Oct 18, 2022 at 3:46 PM Zhu, Lingshan [off-list ref] wrote:

quoted

On 10/18/2022 3:30 PM, Jason Wang wrote:

quoted

On Tue, Oct 18, 2022 at 3:28 PM Zhu, Lingshan [off-list ref] wrote:

quoted

On 10/18/2022 2:44 PM, Jason Wang wrote:

quoted

On Tue, Oct 18, 2022 at 10:20 AM Zhu, Lingshan [off-list ref] wrote:

quoted

On 10/17/2022 3:13 PM, Jason Wang wrote:

quoted

On Mon, Oct 17, 2022 at 3:13 PM Jason Wang [off-list ref] wrote:

quoted

On Fri, Oct 14, 2022 at 5:50 PM Zhu Lingshan [off-list ref] wrote:

quoted

This commit implements support for reading vdpa device
features in iproute2.

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

Signed-off-by: Zhu Lingshan <redacted>

Note that Si Wei proposed to unify the two new attributes:

https://patchew.org/linux/1665422823-18364-1-git-send-email-si-wei.liu@oracle.com/

I think we have discussed this before, there should be two netlink
attributes to report management device features and vDPA device features,
they are different type of devices, this unification introduces
unnecessary couplings

I suggest going through the above patch, both attributes are for the
vDPA device only.

It seems not vDPA device only, from above patch, we see it re-uses
VDPA_ATTR_DEV_FEATURES for reporting vDPA device features

Yes, anything wrong with this? The device features could be
provisioned via netlink.

I think the best netlink practice is to let every attr has its own
and unique purpose, to prevent potential bugs. I think we have discussed
this before that re-using
an attr does not save any resource.

They have exactly the same semantic which is the device features for vDPA.

VDPA_ATTR_DEV_FEATURES is introduced by my features provisioning
series, which is used for the userspace to "set" the device features.
VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES is introduced in one of your
previous patches, which is used for userspace to "get" the device
features.

so basically we are just combining and renaming the attr,
if so, fine with me, get and set may never conflict and in
totally different netlink contexts.

Yes, I think so.

Thanks

Thanks

quoted

quoted

And iprout2 has already updated the uapi header.

Yes, but iproute2 has the same schedule as kernel release, so it's not
too late to fix.

Thanks

quoted

Thanks

quoted

Thanks

quoted

Thanks

quoted

Thanks

quoted

Thanks

quoted

Thanks

quoted

quoted

---
     vdpa/vdpa.c | 15 +++++++++++++--
     1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
index b73e40b4..89844e92 100644
--- a/vdpa/vdpa.c
+++ b/vdpa/vdpa.c

@@ -87,6 +87,8 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
            [VDPA_ATTR_DEV_NEGOTIATED_FEATURES] = MNL_TYPE_U64,
            [VDPA_ATTR_DEV_MGMTDEV_MAX_VQS] = MNL_TYPE_U32,
            [VDPA_ATTR_DEV_SUPPORTED_FEATURES] = MNL_TYPE_U64,
+       [VDPA_ATTR_DEV_FEATURES] = MNL_TYPE_U64,
+       [VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES] = MNL_TYPE_U64,
     };

     static int attr_cb(const struct nlattr *attr, void *data)

@@ -482,7 +484,7 @@ static const char * const *dev_to_feature_str[] = {

     #define NUM_FEATURE_BITS 64

-static void print_features(struct vdpa *vdpa, uint64_t features, bool mgmtdevf,
+static void print_features(struct vdpa *vdpa, uint64_t features, bool devf,
                               uint16_t dev_id)
     {
            const char * const *feature_strs = NULL;

@@ -492,7 +494,7 @@ static void print_features(struct vdpa *vdpa, uint64_t features, bool mgmtdevf,
            if (dev_id < ARRAY_SIZE(dev_to_feature_str))
                    feature_strs = dev_to_feature_str[dev_id];

-       if (mgmtdevf)
+       if (devf)
                    pr_out_array_start(vdpa, "dev_features");
            else
                    pr_out_array_start(vdpa, "negotiated_features");

@@ -771,6 +773,15 @@ static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
                    val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
                    print_features(vdpa, val_u64, false, dev_id);
            }
+       if (tb[VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES]) {
+               uint16_t dev_id = 0;
+
+               if (tb[VDPA_ATTR_DEV_ID])
+                       dev_id = mnl_attr_get_u32(tb[VDPA_ATTR_DEV_ID]);
+
+               val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_VDPA_DEV_SUPPORTED_FEATURES]);
+               print_features(vdpa, val_u64, true, dev_id);
+       }
     }

     static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
--

2.31.1