* [PATCH 0/3] vdpa tool support for configuring max VQs
@ 2022-02-07 12:55 Eli Cohen
2022-02-07 12:55 ` [PATCH 1/3] vdpa: Remove unsupported command line option Eli Cohen
` (2 more replies)
0 siblings, 3 replies; 16+ messages in thread
From: Eli Cohen @ 2022-02-07 12:55 UTC (permalink / raw)
To: stephen, netdev; +Cc: jasowang, si-wei.liu, Eli Cohen
The following three patch series contains:
1. Small fix to vdpa help message
2. Introduce missing definitions for virtio feature flags so they can be
used in the 3rd patch.
3. Add support for configuring max number of VQs for a vdpa device and
allow to get negotiated or max features for a vdpa device/mangement
device.
Eli Cohen (3):
vdpa: Remove unsupported command line option
virtio: Define bit numbers for device independent features
vdpa: Add support to configure max number of VQs
include/uapi/linux/virtio_config.h | 16 ++--
vdpa/include/uapi/linux/vdpa.h | 4 +
vdpa/vdpa.c | 115 ++++++++++++++++++++++++++++-
3 files changed, 123 insertions(+), 12 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] vdpa: Remove unsupported command line option
2022-02-07 12:55 [PATCH 0/3] vdpa tool support for configuring max VQs Eli Cohen
@ 2022-02-07 12:55 ` Eli Cohen
2022-02-10 7:50 ` Jason Wang
2022-02-07 12:55 ` [PATCH 2/3] virtio: Define bit numbers for device independent features Eli Cohen
2022-02-07 12:55 ` [PATCH 3/3] vdpa: Add support to configure max number of VQs Eli Cohen
2 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-07 12:55 UTC (permalink / raw)
To: stephen, netdev; +Cc: jasowang, si-wei.liu, Eli Cohen, Jianbo Liu
"-v[erbose]" option is not supported.
Remove it.
Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
vdpa/vdpa.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
index f048e470c929..4ccb564872a0 100644
--- a/vdpa/vdpa.c
+++ b/vdpa/vdpa.c
@@ -711,7 +711,7 @@ static void help(void)
fprintf(stderr,
"Usage: vdpa [ OPTIONS ] OBJECT { COMMAND | help }\n"
"where OBJECT := { mgmtdev | dev }\n"
- " OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
+ " OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] }\n");
}
static int vdpa_cmd(struct vdpa *vdpa, int argc, char **argv)
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-07 12:55 [PATCH 0/3] vdpa tool support for configuring max VQs Eli Cohen
2022-02-07 12:55 ` [PATCH 1/3] vdpa: Remove unsupported command line option Eli Cohen
@ 2022-02-07 12:55 ` Eli Cohen
2022-02-10 7:54 ` Jason Wang
2022-02-07 12:55 ` [PATCH 3/3] vdpa: Add support to configure max number of VQs Eli Cohen
2 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-07 12:55 UTC (permalink / raw)
To: stephen, netdev; +Cc: jasowang, si-wei.liu, Eli Cohen, Jianbo Liu
Define bit fields for device independent feature bits. We need them in a
follow up patch.
Also, define macros for start and end of these feature bits.
Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
include/uapi/linux/virtio_config.h | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 3bf6c8bf8477..6d92cc31a8d3 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -45,14 +45,14 @@
/* We've given up on this device. */
#define VIRTIO_CONFIG_S_FAILED 0x80
-/*
- * Virtio feature bits VIRTIO_TRANSPORT_F_START through
- * VIRTIO_TRANSPORT_F_END are reserved for the transport
- * being used (e.g. virtio_ring, virtio_pci etc.), the
- * rest are per-device feature bits.
- */
-#define VIRTIO_TRANSPORT_F_START 28
-#define VIRTIO_TRANSPORT_F_END 38
+/* Device independent features per virtio spec 1.1 range from 28 to 38 */
+#define VIRTIO_DEV_INDEPENDENT_F_START 28
+#define VIRTIO_DEV_INDEPENDENT_F_END 38
+
+#define VIRTIO_F_RING_INDIRECT_DESC 28
+#define VIRTIO_F_RING_EVENT_IDX 29
+#define VIRTIO_F_IN_ORDER 35
+#define VIRTIO_F_NOTIFICATION_DATA 38
#ifndef VIRTIO_CONFIG_NO_LEGACY
/* Do we get callbacks when the ring is completely used, even if we've
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/3] vdpa: Add support to configure max number of VQs
2022-02-07 12:55 [PATCH 0/3] vdpa tool support for configuring max VQs Eli Cohen
2022-02-07 12:55 ` [PATCH 1/3] vdpa: Remove unsupported command line option Eli Cohen
2022-02-07 12:55 ` [PATCH 2/3] virtio: Define bit numbers for device independent features Eli Cohen
@ 2022-02-07 12:55 ` Eli Cohen
2022-02-10 8:07 ` Jason Wang
2 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-07 12:55 UTC (permalink / raw)
To: stephen, netdev; +Cc: jasowang, si-wei.liu, Eli Cohen, Jianbo Liu
Add support to configure max supported virtqueue pairs for a vdpa
device. For this to be possible, add support for reading management
device's capabilities. Management device capabilities give the user a
hint as to how many virtqueue pairs at most he can ask for. Using this
information the user can choose a valid number of virtqueue pairs when
creating the device.
Examples:
- Show management device capabiliteis:
$ vdpa mgmtdev show
auxiliary/mlx5_core.sf.1:
supported_classes net
max_supported_vqs 257
dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ \
MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
A user can now create a device based on the above information. In the
above case 128 virtqueue pairs at most. The other VQ being for the
control virtqueue.
- Add a vdpa device with 16 data virtqueue pairs
$ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 16
After feature negotiation has been completed, one can read the vdpa
configuration using:
$ vdpa dev config show
vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 16 mtu 1500
negotiated_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS
CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
Signed-off-by: Eli Cohen <elic@nvidia.com>
---
vdpa/include/uapi/linux/vdpa.h | 4 ++
vdpa/vdpa.c | 113 ++++++++++++++++++++++++++++++++-
2 files changed, 114 insertions(+), 3 deletions(-)
diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
index b7eab069988a..171122dd03c9 100644
--- a/vdpa/include/uapi/linux/vdpa.h
+++ b/vdpa/include/uapi/linux/vdpa.h
@@ -40,6 +40,10 @@ enum vdpa_attr {
VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
+ VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
+ VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
+ VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */
+
/* new attributes must be added above here */
VDPA_ATTR_MAX,
};
diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
index 4ccb564872a0..d0dd4196610f 100644
--- a/vdpa/vdpa.c
+++ b/vdpa/vdpa.c
@@ -23,6 +23,7 @@
#define VDPA_OPT_VDEV_HANDLE BIT(3)
#define VDPA_OPT_VDEV_MAC BIT(4)
#define VDPA_OPT_VDEV_MTU BIT(5)
+#define VDPA_OPT_MAX_VQP BIT(6)
struct vdpa_opts {
uint64_t present; /* flags of present items */
@@ -32,6 +33,7 @@ struct vdpa_opts {
unsigned int device_id;
char mac[ETH_ALEN];
uint16_t mtu;
+ uint16_t max_vqp;
};
struct vdpa {
@@ -78,6 +80,9 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
[VDPA_ATTR_DEV_VENDOR_ID] = MNL_TYPE_U32,
[VDPA_ATTR_DEV_MAX_VQS] = MNL_TYPE_U32,
[VDPA_ATTR_DEV_MAX_VQ_SIZE] = MNL_TYPE_U16,
+ [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,
};
static int attr_cb(const struct nlattr *attr, void *data)
@@ -219,6 +224,8 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa)
sizeof(opts->mac), opts->mac);
if (opts->present & VDPA_OPT_VDEV_MTU)
mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MTU, opts->mtu);
+ if (opts->present & VDPA_OPT_MAX_VQP)
+ mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp);
}
static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
@@ -287,6 +294,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
NEXT_ARG_FWD();
o_found |= VDPA_OPT_VDEV_MTU;
+ } else if ((matches(*argv, "max_vqp") == 0) && (o_optional & VDPA_OPT_MAX_VQP)) {
+ NEXT_ARG_FWD();
+ err = vdpa_argv_u16(vdpa, argc, argv, &opts->max_vqp);
+ if (err)
+ return err;
+
+ NEXT_ARG_FWD();
+ o_found |= VDPA_OPT_MAX_VQP;
} else {
fprintf(stderr, "Unknown option \"%s\"\n", *argv);
return -EINVAL;
@@ -385,6 +400,77 @@ static const char *parse_class(int num)
return class ? class : "< unknown class >";
}
+static const char * const net_feature_strs[64] = {
+ [VIRTIO_NET_F_CSUM] = "CSUM",
+ [VIRTIO_NET_F_GUEST_CSUM] = "GUEST_CSUM",
+ [VIRTIO_NET_F_CTRL_GUEST_OFFLOADS] = "CTRL_GUEST_OFFLOADS",
+ [VIRTIO_NET_F_MTU] = "MTU",
+ [VIRTIO_NET_F_MAC] = "MAC",
+ [VIRTIO_NET_F_GUEST_TSO4] = "GUEST_TSO4",
+ [VIRTIO_NET_F_GUEST_TSO6] = "GUEST_TSO6",
+ [VIRTIO_NET_F_GUEST_ECN] = "GUEST_ECN",
+ [VIRTIO_NET_F_GUEST_UFO] = "GUEST_UFO",
+ [VIRTIO_NET_F_HOST_TSO4] = "HOST_TSO4",
+ [VIRTIO_NET_F_HOST_TSO6] = "HOST_TSO6",
+ [VIRTIO_NET_F_HOST_ECN] = "HOST_ECN",
+ [VIRTIO_NET_F_HOST_UFO] = "HOST_UFO",
+ [VIRTIO_NET_F_MRG_RXBUF] = "MRG_RXBUF",
+ [VIRTIO_NET_F_STATUS] = "STATUS",
+ [VIRTIO_NET_F_CTRL_VQ] = "CTRL_VQ",
+ [VIRTIO_NET_F_CTRL_RX] = "CTRL_RX",
+ [VIRTIO_NET_F_CTRL_VLAN] = "CTRL_VLAN",
+ [VIRTIO_NET_F_CTRL_RX_EXTRA] = "CTRL_RX_EXTRA",
+ [VIRTIO_NET_F_GUEST_ANNOUNCE] = "GUEST_ANNOUNCE",
+ [VIRTIO_NET_F_MQ] = "MQ",
+ [VIRTIO_F_NOTIFY_ON_EMPTY] = "NOTIFY_ON_EMPTY",
+ [VIRTIO_NET_F_CTRL_MAC_ADDR] = "CTRL_MAC_ADDR",
+ [VIRTIO_F_ANY_LAYOUT] = "ANY_LAYOUT",
+ [VIRTIO_NET_F_RSC_EXT] = "RSC_EXT",
+ [VIRTIO_NET_F_STANDBY] = "STANDBY",
+};
+
+#define VDPA_EXT_FEATURES_SZ (VIRTIO_DEV_INDEPENDENT_F_END - \
+ VIRTIO_DEV_INDEPENDENT_F_START + 1)
+
+static const char * const ext_feature_strs[VDPA_EXT_FEATURES_SZ] = {
+ [VIRTIO_F_RING_INDIRECT_DESC - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_INDIRECT_DESC",
+ [VIRTIO_F_RING_EVENT_IDX - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_EVENT_IDX",
+ [VIRTIO_F_VERSION_1 - VIRTIO_DEV_INDEPENDENT_F_START] = "VERSION_1",
+ [VIRTIO_F_ACCESS_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ACCESS_PLATFORM",
+ [VIRTIO_F_RING_PACKED - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_PACKED",
+ [VIRTIO_F_IN_ORDER - VIRTIO_DEV_INDEPENDENT_F_START] = "IN_ORDER",
+ [VIRTIO_F_ORDER_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ORDER_PLATFORM",
+ [VIRTIO_F_SR_IOV - VIRTIO_DEV_INDEPENDENT_F_START] = "SR_IOV",
+ [VIRTIO_F_NOTIFICATION_DATA - VIRTIO_DEV_INDEPENDENT_F_START] = "NOTIFICATION_DATA",
+};
+
+static void print_net_features(struct vdpa *vdpa, uint64_t features, bool maxf)
+{
+ const char *s;
+ int i;
+
+ if (maxf)
+ pr_out_array_start(vdpa, "dev_features");
+ else
+ pr_out_array_start(vdpa, "negotiated_features");
+
+ for (i = 0; i < 64; i++) {
+ if (!(features & (1ULL << i)))
+ continue;
+
+ if (i >= VIRTIO_DEV_INDEPENDENT_F_START && i <= VIRTIO_DEV_INDEPENDENT_F_END)
+ s = ext_feature_strs[i - VIRTIO_DEV_INDEPENDENT_F_START];
+ else
+ s = net_feature_strs[i];
+
+ if (!s)
+ print_uint(PRINT_ANY, NULL, " unrecognized_bit_%d", i);
+ else
+ print_string(PRINT_ANY, NULL, " %s", s);
+ }
+ pr_out_array_end(vdpa);
+}
+
static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
struct nlattr **tb)
{
@@ -408,6 +494,22 @@ static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
pr_out_array_end(vdpa);
}
+ if (tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]) {
+ uint16_t num_vqs;
+
+ if (!vdpa->json_output)
+ printf("\n");
+ num_vqs = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]);
+ print_uint(PRINT_ANY, "max_supported_vqs", " max_supported_vqs %d", num_vqs);
+ }
+
+ if (tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]) {
+ uint64_t features;
+
+ features = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]);
+ print_net_features(vdpa, features, true);
+ }
+
pr_out_handle_end(vdpa);
}
@@ -557,7 +659,7 @@ static int cmd_dev_add(struct vdpa *vdpa, int argc, char **argv)
NLM_F_REQUEST | NLM_F_ACK);
err = vdpa_argv_parse_put(nlh, vdpa, argc, argv,
VDPA_OPT_VDEV_MGMTDEV_HANDLE | VDPA_OPT_VDEV_NAME,
- VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU);
+ VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU | VDPA_OPT_MAX_VQP);
if (err)
return err;
@@ -579,9 +681,10 @@ static int cmd_dev_del(struct vdpa *vdpa, int argc, char **argv)
return mnlu_gen_socket_sndrcv(&vdpa->nlg, nlh, NULL, NULL);
}
-static void pr_out_dev_net_config(struct nlattr **tb)
+static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
{
SPRINT_BUF(macaddr);
+ uint64_t val_u64;
uint16_t val_u16;
if (tb[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
@@ -610,6 +713,10 @@ static void pr_out_dev_net_config(struct nlattr **tb)
val_u16 = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_NET_CFG_MTU]);
print_uint(PRINT_ANY, "mtu", "mtu %d ", val_u16);
}
+ if (tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]) {
+ val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
+ print_net_features(vdpa, val_u64, false);
+ }
}
static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
@@ -619,7 +726,7 @@ static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
pr_out_vdev_handle_start(vdpa, tb);
switch (device_id) {
case VIRTIO_ID_NET:
- pr_out_dev_net_config(tb);
+ pr_out_dev_net_config(vdpa, tb);
break;
default:
break;
--
2.34.1
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] vdpa: Remove unsupported command line option
2022-02-07 12:55 ` [PATCH 1/3] vdpa: Remove unsupported command line option Eli Cohen
@ 2022-02-10 7:50 ` Jason Wang
0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2022-02-10 7:50 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
>
> "-v[erbose]" option is not supported.
> Remove it.
>
> Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
Acked-by: Jason Wang <jasowang@redhat.com>
> ---
> vdpa/vdpa.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> index f048e470c929..4ccb564872a0 100644
> --- a/vdpa/vdpa.c
> +++ b/vdpa/vdpa.c
> @@ -711,7 +711,7 @@ static void help(void)
> fprintf(stderr,
> "Usage: vdpa [ OPTIONS ] OBJECT { COMMAND | help }\n"
> "where OBJECT := { mgmtdev | dev }\n"
> - " OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] | -v[erbose] }\n");
> + " OPTIONS := { -V[ersion] | -n[o-nice-names] | -j[son] | -p[retty] }\n");
> }
>
> static int vdpa_cmd(struct vdpa *vdpa, int argc, char **argv)
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-07 12:55 ` [PATCH 2/3] virtio: Define bit numbers for device independent features Eli Cohen
@ 2022-02-10 7:54 ` Jason Wang
2022-02-10 8:30 ` Eli Cohen
0 siblings, 1 reply; 16+ messages in thread
From: Jason Wang @ 2022-02-10 7:54 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
>
> Define bit fields for device independent feature bits. We need them in a
> follow up patch.
>
> Also, define macros for start and end of these feature bits.
>
> Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> include/uapi/linux/virtio_config.h | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index 3bf6c8bf8477..6d92cc31a8d3 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -45,14 +45,14 @@
> /* We've given up on this device. */
> #define VIRTIO_CONFIG_S_FAILED 0x80
>
> -/*
> - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> - * being used (e.g. virtio_ring, virtio_pci etc.), the
> - * rest are per-device feature bits.
> - */
> -#define VIRTIO_TRANSPORT_F_START 28
> -#define VIRTIO_TRANSPORT_F_END 38
> +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> +#define VIRTIO_DEV_INDEPENDENT_F_END 38
Haven't gone through patch 3 but I think it's probably better not
touch uapi stuff. Or we can define those macros in other place?
> +
> +#define VIRTIO_F_RING_INDIRECT_DESC 28
> +#define VIRTIO_F_RING_EVENT_IDX 29
> +#define VIRTIO_F_IN_ORDER 35
> +#define VIRTIO_F_NOTIFICATION_DATA 38
This part belongs to the virtio_ring.h any reason not pull that file
instead of squashing those into virtio_config.h?
Thanks
>
> #ifndef VIRTIO_CONFIG_NO_LEGACY
> /* Do we get callbacks when the ring is completely used, even if we've
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] vdpa: Add support to configure max number of VQs
2022-02-07 12:55 ` [PATCH 3/3] vdpa: Add support to configure max number of VQs Eli Cohen
@ 2022-02-10 8:07 ` Jason Wang
2022-02-10 8:44 ` Eli Cohen
0 siblings, 1 reply; 16+ messages in thread
From: Jason Wang @ 2022-02-10 8:07 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
>
> Add support to configure max supported virtqueue pairs for a vdpa
> device. For this to be possible, add support for reading management
> device's capabilities. Management device capabilities give the user a
> hint as to how many virtqueue pairs at most he can ask for. Using this
> information the user can choose a valid number of virtqueue pairs when
> creating the device.
>
> Examples:
> - Show management device capabiliteis:
> $ vdpa mgmtdev show
> auxiliary/mlx5_core.sf.1:
> supported_classes net
> max_supported_vqs 257
> dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ \
> MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
>
> A user can now create a device based on the above information. In the
> above case 128 virtqueue pairs at most. The other VQ being for the
> control virtqueue.
>
> - Add a vdpa device with 16 data virtqueue pairs
> $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 16
>
> After feature negotiation has been completed, one can read the vdpa
> configuration using:
> $ vdpa dev config show
> vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 16 mtu 1500
> negotiated_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS
> CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
I wonder if lower case is better.
>
> Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> Signed-off-by: Eli Cohen <elic@nvidia.com>
> ---
> vdpa/include/uapi/linux/vdpa.h | 4 ++
> vdpa/vdpa.c | 113 ++++++++++++++++++++++++++++++++-
> 2 files changed, 114 insertions(+), 3 deletions(-)
>
> diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> index b7eab069988a..171122dd03c9 100644
> --- a/vdpa/include/uapi/linux/vdpa.h
> +++ b/vdpa/include/uapi/linux/vdpa.h
> @@ -40,6 +40,10 @@ enum vdpa_attr {
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
> VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
>
> + VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
> + VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
> + VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */
I wonder if it's better to split the patches into three where the
above command could be implemented separately.
> +
> /* new attributes must be added above here */
> VDPA_ATTR_MAX,
> };
> diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> index 4ccb564872a0..d0dd4196610f 100644
> --- a/vdpa/vdpa.c
> +++ b/vdpa/vdpa.c
> @@ -23,6 +23,7 @@
> #define VDPA_OPT_VDEV_HANDLE BIT(3)
> #define VDPA_OPT_VDEV_MAC BIT(4)
> #define VDPA_OPT_VDEV_MTU BIT(5)
> +#define VDPA_OPT_MAX_VQP BIT(6)
>
> struct vdpa_opts {
> uint64_t present; /* flags of present items */
> @@ -32,6 +33,7 @@ struct vdpa_opts {
> unsigned int device_id;
> char mac[ETH_ALEN];
> uint16_t mtu;
> + uint16_t max_vqp;
> };
>
> struct vdpa {
> @@ -78,6 +80,9 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
> [VDPA_ATTR_DEV_VENDOR_ID] = MNL_TYPE_U32,
> [VDPA_ATTR_DEV_MAX_VQS] = MNL_TYPE_U32,
> [VDPA_ATTR_DEV_MAX_VQ_SIZE] = MNL_TYPE_U16,
> + [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,
> };
>
> static int attr_cb(const struct nlattr *attr, void *data)
> @@ -219,6 +224,8 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa)
> sizeof(opts->mac), opts->mac);
> if (opts->present & VDPA_OPT_VDEV_MTU)
> mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MTU, opts->mtu);
> + if (opts->present & VDPA_OPT_MAX_VQP)
> + mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp);
> }
>
> static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> @@ -287,6 +294,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
>
> NEXT_ARG_FWD();
> o_found |= VDPA_OPT_VDEV_MTU;
> + } else if ((matches(*argv, "max_vqp") == 0) && (o_optional & VDPA_OPT_MAX_VQP)) {
> + NEXT_ARG_FWD();
> + err = vdpa_argv_u16(vdpa, argc, argv, &opts->max_vqp);
> + if (err)
> + return err;
> +
> + NEXT_ARG_FWD();
> + o_found |= VDPA_OPT_MAX_VQP;
> } else {
> fprintf(stderr, "Unknown option \"%s\"\n", *argv);
> return -EINVAL;
> @@ -385,6 +400,77 @@ static const char *parse_class(int num)
> return class ? class : "< unknown class >";
> }
>
> +static const char * const net_feature_strs[64] = {
> + [VIRTIO_NET_F_CSUM] = "CSUM",
> + [VIRTIO_NET_F_GUEST_CSUM] = "GUEST_CSUM",
> + [VIRTIO_NET_F_CTRL_GUEST_OFFLOADS] = "CTRL_GUEST_OFFLOADS",
> + [VIRTIO_NET_F_MTU] = "MTU",
> + [VIRTIO_NET_F_MAC] = "MAC",
> + [VIRTIO_NET_F_GUEST_TSO4] = "GUEST_TSO4",
> + [VIRTIO_NET_F_GUEST_TSO6] = "GUEST_TSO6",
> + [VIRTIO_NET_F_GUEST_ECN] = "GUEST_ECN",
> + [VIRTIO_NET_F_GUEST_UFO] = "GUEST_UFO",
> + [VIRTIO_NET_F_HOST_TSO4] = "HOST_TSO4",
> + [VIRTIO_NET_F_HOST_TSO6] = "HOST_TSO6",
> + [VIRTIO_NET_F_HOST_ECN] = "HOST_ECN",
> + [VIRTIO_NET_F_HOST_UFO] = "HOST_UFO",
> + [VIRTIO_NET_F_MRG_RXBUF] = "MRG_RXBUF",
> + [VIRTIO_NET_F_STATUS] = "STATUS",
> + [VIRTIO_NET_F_CTRL_VQ] = "CTRL_VQ",
> + [VIRTIO_NET_F_CTRL_RX] = "CTRL_RX",
> + [VIRTIO_NET_F_CTRL_VLAN] = "CTRL_VLAN",
> + [VIRTIO_NET_F_CTRL_RX_EXTRA] = "CTRL_RX_EXTRA",
> + [VIRTIO_NET_F_GUEST_ANNOUNCE] = "GUEST_ANNOUNCE",
> + [VIRTIO_NET_F_MQ] = "MQ",
> + [VIRTIO_F_NOTIFY_ON_EMPTY] = "NOTIFY_ON_EMPTY",
> + [VIRTIO_NET_F_CTRL_MAC_ADDR] = "CTRL_MAC_ADDR",
> + [VIRTIO_F_ANY_LAYOUT] = "ANY_LAYOUT",
> + [VIRTIO_NET_F_RSC_EXT] = "RSC_EXT",
> + [VIRTIO_NET_F_STANDBY] = "STANDBY",
> +};
It seems we are still missing things that are already supported in the
Linux uapi. I think it's better to support them. E.g the RSS and
SPEED_DUPLEX etc.
> +
> +#define VDPA_EXT_FEATURES_SZ (VIRTIO_DEV_INDEPENDENT_F_END - \
> + VIRTIO_DEV_INDEPENDENT_F_START + 1)
> +
> +static const char * const ext_feature_strs[VDPA_EXT_FEATURES_SZ] = {
> + [VIRTIO_F_RING_INDIRECT_DESC - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_INDIRECT_DESC",
> + [VIRTIO_F_RING_EVENT_IDX - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_EVENT_IDX",
> + [VIRTIO_F_VERSION_1 - VIRTIO_DEV_INDEPENDENT_F_START] = "VERSION_1",
> + [VIRTIO_F_ACCESS_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ACCESS_PLATFORM",
> + [VIRTIO_F_RING_PACKED - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_PACKED",
> + [VIRTIO_F_IN_ORDER - VIRTIO_DEV_INDEPENDENT_F_START] = "IN_ORDER",
> + [VIRTIO_F_ORDER_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ORDER_PLATFORM",
> + [VIRTIO_F_SR_IOV - VIRTIO_DEV_INDEPENDENT_F_START] = "SR_IOV",
> + [VIRTIO_F_NOTIFICATION_DATA - VIRTIO_DEV_INDEPENDENT_F_START] = "NOTIFICATION_DATA",
> +};
> +
> +static void print_net_features(struct vdpa *vdpa, uint64_t features, bool maxf)
> +{
> + const char *s;
> + int i;
> +
> + if (maxf)
> + pr_out_array_start(vdpa, "dev_features");
> + else
> + pr_out_array_start(vdpa, "negotiated_features");
> +
> + for (i = 0; i < 64; i++) {
> + if (!(features & (1ULL << i)))
> + continue;
> +
> + if (i >= VIRTIO_DEV_INDEPENDENT_F_START && i <= VIRTIO_DEV_INDEPENDENT_F_END)
I don't see any issue that just use VIRTIO_TRANSPORT_F_START and
VIRTIO_TRANSPORT_F_END (even if END can change).
> + s = ext_feature_strs[i - VIRTIO_DEV_INDEPENDENT_F_START];
> + else
> + s = net_feature_strs[i];
> +
> + if (!s)
> + print_uint(PRINT_ANY, NULL, " unrecognized_bit_%d", i);
> + else
> + print_string(PRINT_ANY, NULL, " %s", s);
> + }
> + pr_out_array_end(vdpa);
> +}
> +
> static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> struct nlattr **tb)
> {
> @@ -408,6 +494,22 @@ static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> pr_out_array_end(vdpa);
> }
>
> + if (tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]) {
> + uint16_t num_vqs;
> +
> + if (!vdpa->json_output)
> + printf("\n");
> + num_vqs = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]);
> + print_uint(PRINT_ANY, "max_supported_vqs", " max_supported_vqs %d", num_vqs);
> + }
> +
> + if (tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]) {
> + uint64_t features;
> +
> + features = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]);
> + print_net_features(vdpa, features, true);
Do we need to check whether it's a networking device before trying to
print the feature and for other type devices we can simply print the
bit number as a startup?
Thanks
> + }
> +
> pr_out_handle_end(vdpa);
> }
>
> @@ -557,7 +659,7 @@ static int cmd_dev_add(struct vdpa *vdpa, int argc, char **argv)
> NLM_F_REQUEST | NLM_F_ACK);
> err = vdpa_argv_parse_put(nlh, vdpa, argc, argv,
> VDPA_OPT_VDEV_MGMTDEV_HANDLE | VDPA_OPT_VDEV_NAME,
> - VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU);
> + VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU | VDPA_OPT_MAX_VQP);
> if (err)
> return err;
>
> @@ -579,9 +681,10 @@ static int cmd_dev_del(struct vdpa *vdpa, int argc, char **argv)
> return mnlu_gen_socket_sndrcv(&vdpa->nlg, nlh, NULL, NULL);
> }
>
> -static void pr_out_dev_net_config(struct nlattr **tb)
> +static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
> {
> SPRINT_BUF(macaddr);
> + uint64_t val_u64;
> uint16_t val_u16;
>
> if (tb[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> @@ -610,6 +713,10 @@ static void pr_out_dev_net_config(struct nlattr **tb)
> val_u16 = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_NET_CFG_MTU]);
> print_uint(PRINT_ANY, "mtu", "mtu %d ", val_u16);
> }
> + if (tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]) {
> + val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
> + print_net_features(vdpa, val_u64, false);
> + }
> }
>
> static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> @@ -619,7 +726,7 @@ static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> pr_out_vdev_handle_start(vdpa, tb);
> switch (device_id) {
> case VIRTIO_ID_NET:
> - pr_out_dev_net_config(tb);
> + pr_out_dev_net_config(vdpa, tb);
> break;
> default:
> break;
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-10 7:54 ` Jason Wang
@ 2022-02-10 8:30 ` Eli Cohen
2022-02-10 8:35 ` Jason Wang
0 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-10 8:30 UTC (permalink / raw)
To: Jason Wang; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> > Define bit fields for device independent feature bits. We need them in a
> > follow up patch.
> >
> > Also, define macros for start and end of these feature bits.
> >
> > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > 1 file changed, 8 insertions(+), 8 deletions(-)
> >
> > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > --- a/include/uapi/linux/virtio_config.h
> > +++ b/include/uapi/linux/virtio_config.h
> > @@ -45,14 +45,14 @@
> > /* We've given up on this device. */
> > #define VIRTIO_CONFIG_S_FAILED 0x80
> >
> > -/*
> > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > - * rest are per-device feature bits.
> > - */
> > -#define VIRTIO_TRANSPORT_F_START 28
> > -#define VIRTIO_TRANSPORT_F_END 38
> > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
>
> Haven't gone through patch 3 but I think it's probably better not
> touch uapi stuff. Or we can define those macros in other place?
>
I can put it in vdpa.c
> > +
> > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > +#define VIRTIO_F_RING_EVENT_IDX 29
> > +#define VIRTIO_F_IN_ORDER 35
> > +#define VIRTIO_F_NOTIFICATION_DATA 38
>
> This part belongs to the virtio_ring.h any reason not pull that file
> instead of squashing those into virtio_config.h?
>
Not sure what you mean here. I can't find virtio_ring.h in my tree.
> Thanks
>
> >
> > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > /* Do we get callbacks when the ring is completely used, even if we've
> > --
> > 2.34.1
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-10 8:30 ` Eli Cohen
@ 2022-02-10 8:35 ` Jason Wang
2022-02-10 9:27 ` Eli Cohen
0 siblings, 1 reply; 16+ messages in thread
From: Jason Wang @ 2022-02-10 8:35 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > Define bit fields for device independent feature bits. We need them in a
> > > follow up patch.
> > >
> > > Also, define macros for start and end of these feature bits.
> > >
> > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > > --- a/include/uapi/linux/virtio_config.h
> > > +++ b/include/uapi/linux/virtio_config.h
> > > @@ -45,14 +45,14 @@
> > > /* We've given up on this device. */
> > > #define VIRTIO_CONFIG_S_FAILED 0x80
> > >
> > > -/*
> > > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > > - * rest are per-device feature bits.
> > > - */
> > > -#define VIRTIO_TRANSPORT_F_START 28
> > > -#define VIRTIO_TRANSPORT_F_END 38
> > > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
> >
> > Haven't gone through patch 3 but I think it's probably better not
> > touch uapi stuff. Or we can define those macros in other place?
> >
>
> I can put it in vdpa.c
>
> > > +
> > > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > > +#define VIRTIO_F_RING_EVENT_IDX 29
> > > +#define VIRTIO_F_IN_ORDER 35
> > > +#define VIRTIO_F_NOTIFICATION_DATA 38
> >
> > This part belongs to the virtio_ring.h any reason not pull that file
> > instead of squashing those into virtio_config.h?
> >
>
> Not sure what you mean here. I can't find virtio_ring.h in my tree.
I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
Thanks
>
> > Thanks
> >
> > >
> > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > /* Do we get callbacks when the ring is completely used, even if we've
> > > --
> > > 2.34.1
> > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] vdpa: Add support to configure max number of VQs
2022-02-10 8:07 ` Jason Wang
@ 2022-02-10 8:44 ` Eli Cohen
2022-02-10 8:51 ` Jason Wang
0 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-10 8:44 UTC (permalink / raw)
To: Jason Wang; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Thu, Feb 10, 2022 at 04:07:24PM +0800, Jason Wang wrote:
> On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> > Add support to configure max supported virtqueue pairs for a vdpa
> > device. For this to be possible, add support for reading management
> > device's capabilities. Management device capabilities give the user a
> > hint as to how many virtqueue pairs at most he can ask for. Using this
> > information the user can choose a valid number of virtqueue pairs when
> > creating the device.
> >
> > Examples:
> > - Show management device capabiliteis:
> > $ vdpa mgmtdev show
> > auxiliary/mlx5_core.sf.1:
> > supported_classes net
> > max_supported_vqs 257
> > dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ \
> > MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
> >
> > A user can now create a device based on the above information. In the
> > above case 128 virtqueue pairs at most. The other VQ being for the
> > control virtqueue.
> >
> > - Add a vdpa device with 16 data virtqueue pairs
> > $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 16
> >
> > After feature negotiation has been completed, one can read the vdpa
> > configuration using:
> > $ vdpa dev config show
> > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 16 mtu 1500
> > negotiated_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS
> > CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
>
> I wonder if lower case is better.
>
I thought the capital letters will emphasize the fact that these are
flag bits. Also, note the matching kernel patches have this documented
in the change log with capital letters.
> >
> > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > ---
> > vdpa/include/uapi/linux/vdpa.h | 4 ++
> > vdpa/vdpa.c | 113 ++++++++++++++++++++++++++++++++-
> > 2 files changed, 114 insertions(+), 3 deletions(-)
> >
> > diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> > index b7eab069988a..171122dd03c9 100644
> > --- a/vdpa/include/uapi/linux/vdpa.h
> > +++ b/vdpa/include/uapi/linux/vdpa.h
> > @@ -40,6 +40,10 @@ enum vdpa_attr {
> > VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
> > VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
> >
> > + VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
> > + VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
> > + VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */
>
> I wonder if it's better to split the patches into three where the
> above command could be implemented separately.
I already sent three. You mean split the third patch into three?
>
> > +
> > /* new attributes must be added above here */
> > VDPA_ATTR_MAX,
> > };
> > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> > index 4ccb564872a0..d0dd4196610f 100644
> > --- a/vdpa/vdpa.c
> > +++ b/vdpa/vdpa.c
> > @@ -23,6 +23,7 @@
> > #define VDPA_OPT_VDEV_HANDLE BIT(3)
> > #define VDPA_OPT_VDEV_MAC BIT(4)
> > #define VDPA_OPT_VDEV_MTU BIT(5)
> > +#define VDPA_OPT_MAX_VQP BIT(6)
> >
> > struct vdpa_opts {
> > uint64_t present; /* flags of present items */
> > @@ -32,6 +33,7 @@ struct vdpa_opts {
> > unsigned int device_id;
> > char mac[ETH_ALEN];
> > uint16_t mtu;
> > + uint16_t max_vqp;
> > };
> >
> > struct vdpa {
> > @@ -78,6 +80,9 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
> > [VDPA_ATTR_DEV_VENDOR_ID] = MNL_TYPE_U32,
> > [VDPA_ATTR_DEV_MAX_VQS] = MNL_TYPE_U32,
> > [VDPA_ATTR_DEV_MAX_VQ_SIZE] = MNL_TYPE_U16,
> > + [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,
> > };
> >
> > static int attr_cb(const struct nlattr *attr, void *data)
> > @@ -219,6 +224,8 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa)
> > sizeof(opts->mac), opts->mac);
> > if (opts->present & VDPA_OPT_VDEV_MTU)
> > mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MTU, opts->mtu);
> > + if (opts->present & VDPA_OPT_MAX_VQP)
> > + mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp);
> > }
> >
> > static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> > @@ -287,6 +294,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> >
> > NEXT_ARG_FWD();
> > o_found |= VDPA_OPT_VDEV_MTU;
> > + } else if ((matches(*argv, "max_vqp") == 0) && (o_optional & VDPA_OPT_MAX_VQP)) {
> > + NEXT_ARG_FWD();
> > + err = vdpa_argv_u16(vdpa, argc, argv, &opts->max_vqp);
> > + if (err)
> > + return err;
> > +
> > + NEXT_ARG_FWD();
> > + o_found |= VDPA_OPT_MAX_VQP;
> > } else {
> > fprintf(stderr, "Unknown option \"%s\"\n", *argv);
> > return -EINVAL;
> > @@ -385,6 +400,77 @@ static const char *parse_class(int num)
> > return class ? class : "< unknown class >";
> > }
> >
> > +static const char * const net_feature_strs[64] = {
> > + [VIRTIO_NET_F_CSUM] = "CSUM",
> > + [VIRTIO_NET_F_GUEST_CSUM] = "GUEST_CSUM",
> > + [VIRTIO_NET_F_CTRL_GUEST_OFFLOADS] = "CTRL_GUEST_OFFLOADS",
> > + [VIRTIO_NET_F_MTU] = "MTU",
> > + [VIRTIO_NET_F_MAC] = "MAC",
> > + [VIRTIO_NET_F_GUEST_TSO4] = "GUEST_TSO4",
> > + [VIRTIO_NET_F_GUEST_TSO6] = "GUEST_TSO6",
> > + [VIRTIO_NET_F_GUEST_ECN] = "GUEST_ECN",
> > + [VIRTIO_NET_F_GUEST_UFO] = "GUEST_UFO",
> > + [VIRTIO_NET_F_HOST_TSO4] = "HOST_TSO4",
> > + [VIRTIO_NET_F_HOST_TSO6] = "HOST_TSO6",
> > + [VIRTIO_NET_F_HOST_ECN] = "HOST_ECN",
> > + [VIRTIO_NET_F_HOST_UFO] = "HOST_UFO",
> > + [VIRTIO_NET_F_MRG_RXBUF] = "MRG_RXBUF",
> > + [VIRTIO_NET_F_STATUS] = "STATUS",
> > + [VIRTIO_NET_F_CTRL_VQ] = "CTRL_VQ",
> > + [VIRTIO_NET_F_CTRL_RX] = "CTRL_RX",
> > + [VIRTIO_NET_F_CTRL_VLAN] = "CTRL_VLAN",
> > + [VIRTIO_NET_F_CTRL_RX_EXTRA] = "CTRL_RX_EXTRA",
> > + [VIRTIO_NET_F_GUEST_ANNOUNCE] = "GUEST_ANNOUNCE",
> > + [VIRTIO_NET_F_MQ] = "MQ",
> > + [VIRTIO_F_NOTIFY_ON_EMPTY] = "NOTIFY_ON_EMPTY",
> > + [VIRTIO_NET_F_CTRL_MAC_ADDR] = "CTRL_MAC_ADDR",
> > + [VIRTIO_F_ANY_LAYOUT] = "ANY_LAYOUT",
> > + [VIRTIO_NET_F_RSC_EXT] = "RSC_EXT",
> > + [VIRTIO_NET_F_STANDBY] = "STANDBY",
> > +};
>
> It seems we are still missing things that are already supported in the
> Linux uapi. I think it's better to support them. E.g the RSS and
> SPEED_DUPLEX etc.
>
Will do.
> > +
> > +#define VDPA_EXT_FEATURES_SZ (VIRTIO_DEV_INDEPENDENT_F_END - \
> > + VIRTIO_DEV_INDEPENDENT_F_START + 1)
> > +
> > +static const char * const ext_feature_strs[VDPA_EXT_FEATURES_SZ] = {
> > + [VIRTIO_F_RING_INDIRECT_DESC - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_INDIRECT_DESC",
> > + [VIRTIO_F_RING_EVENT_IDX - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_EVENT_IDX",
> > + [VIRTIO_F_VERSION_1 - VIRTIO_DEV_INDEPENDENT_F_START] = "VERSION_1",
> > + [VIRTIO_F_ACCESS_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ACCESS_PLATFORM",
> > + [VIRTIO_F_RING_PACKED - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_PACKED",
> > + [VIRTIO_F_IN_ORDER - VIRTIO_DEV_INDEPENDENT_F_START] = "IN_ORDER",
> > + [VIRTIO_F_ORDER_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ORDER_PLATFORM",
> > + [VIRTIO_F_SR_IOV - VIRTIO_DEV_INDEPENDENT_F_START] = "SR_IOV",
> > + [VIRTIO_F_NOTIFICATION_DATA - VIRTIO_DEV_INDEPENDENT_F_START] = "NOTIFICATION_DATA",
> > +};
> > +
> > +static void print_net_features(struct vdpa *vdpa, uint64_t features, bool maxf)
> > +{
> > + const char *s;
> > + int i;
> > +
> > + if (maxf)
> > + pr_out_array_start(vdpa, "dev_features");
> > + else
> > + pr_out_array_start(vdpa, "negotiated_features");
> > +
> > + for (i = 0; i < 64; i++) {
> > + if (!(features & (1ULL << i)))
> > + continue;
> > +
> > + if (i >= VIRTIO_DEV_INDEPENDENT_F_START && i <= VIRTIO_DEV_INDEPENDENT_F_END)
>
> I don't see any issue that just use VIRTIO_TRANSPORT_F_START and
> VIRTIO_TRANSPORT_F_END (even if END can change).
I don't get you
>
> > + s = ext_feature_strs[i - VIRTIO_DEV_INDEPENDENT_F_START];
> > + else
> > + s = net_feature_strs[i];
> > +
> > + if (!s)
> > + print_uint(PRINT_ANY, NULL, " unrecognized_bit_%d", i);
> > + else
> > + print_string(PRINT_ANY, NULL, " %s", s);
> > + }
> > + pr_out_array_end(vdpa);
> > +}
> > +
> > static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> > struct nlattr **tb)
> > {
> > @@ -408,6 +494,22 @@ static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> > pr_out_array_end(vdpa);
> > }
> >
> > + if (tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]) {
> > + uint16_t num_vqs;
> > +
> > + if (!vdpa->json_output)
> > + printf("\n");
> > + num_vqs = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]);
> > + print_uint(PRINT_ANY, "max_supported_vqs", " max_supported_vqs %d", num_vqs);
> > + }
> > +
> > + if (tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]) {
> > + uint64_t features;
> > +
> > + features = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]);
> > + print_net_features(vdpa, features, true);
>
> Do we need to check whether it's a networking device before trying to
> print the feature
Yes, will fix
> and for other type devices we can simply print the
> bit number as a startup?
>
Why not add proper support (e.g. strings) for other types of devices when intoduced?
> Thanks
>
> > + }
> > +
> > pr_out_handle_end(vdpa);
> > }
> >
> > @@ -557,7 +659,7 @@ static int cmd_dev_add(struct vdpa *vdpa, int argc, char **argv)
> > NLM_F_REQUEST | NLM_F_ACK);
> > err = vdpa_argv_parse_put(nlh, vdpa, argc, argv,
> > VDPA_OPT_VDEV_MGMTDEV_HANDLE | VDPA_OPT_VDEV_NAME,
> > - VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU);
> > + VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU | VDPA_OPT_MAX_VQP);
> > if (err)
> > return err;
> >
> > @@ -579,9 +681,10 @@ static int cmd_dev_del(struct vdpa *vdpa, int argc, char **argv)
> > return mnlu_gen_socket_sndrcv(&vdpa->nlg, nlh, NULL, NULL);
> > }
> >
> > -static void pr_out_dev_net_config(struct nlattr **tb)
> > +static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
> > {
> > SPRINT_BUF(macaddr);
> > + uint64_t val_u64;
> > uint16_t val_u16;
> >
> > if (tb[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > @@ -610,6 +713,10 @@ static void pr_out_dev_net_config(struct nlattr **tb)
> > val_u16 = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_NET_CFG_MTU]);
> > print_uint(PRINT_ANY, "mtu", "mtu %d ", val_u16);
> > }
> > + if (tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]) {
> > + val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
> > + print_net_features(vdpa, val_u64, false);
> > + }
> > }
> >
> > static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> > @@ -619,7 +726,7 @@ static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> > pr_out_vdev_handle_start(vdpa, tb);
> > switch (device_id) {
> > case VIRTIO_ID_NET:
> > - pr_out_dev_net_config(tb);
> > + pr_out_dev_net_config(vdpa, tb);
> > break;
> > default:
> > break;
> > --
> > 2.34.1
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] vdpa: Add support to configure max number of VQs
2022-02-10 8:44 ` Eli Cohen
@ 2022-02-10 8:51 ` Jason Wang
0 siblings, 0 replies; 16+ messages in thread
From: Jason Wang @ 2022-02-10 8:51 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Thu, Feb 10, 2022 at 4:44 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Thu, Feb 10, 2022 at 04:07:24PM +0800, Jason Wang wrote:
> > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > >
> > > Add support to configure max supported virtqueue pairs for a vdpa
> > > device. For this to be possible, add support for reading management
> > > device's capabilities. Management device capabilities give the user a
> > > hint as to how many virtqueue pairs at most he can ask for. Using this
> > > information the user can choose a valid number of virtqueue pairs when
> > > creating the device.
> > >
> > > Examples:
> > > - Show management device capabiliteis:
> > > $ vdpa mgmtdev show
> > > auxiliary/mlx5_core.sf.1:
> > > supported_classes net
> > > max_supported_vqs 257
> > > dev_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ \
> > > MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
> > >
> > > A user can now create a device based on the above information. In the
> > > above case 128 virtqueue pairs at most. The other VQ being for the
> > > control virtqueue.
> > >
> > > - Add a vdpa device with 16 data virtqueue pairs
> > > $ vdpa dev add name vdpa-a mgmtdev auxiliary/mlx5_core.sf.1 max_vqp 16
> > >
> > > After feature negotiation has been completed, one can read the vdpa
> > > configuration using:
> > > $ vdpa dev config show
> > > vdpa-a: mac 00:00:00:00:88:88 link up link_announce false max_vq_pairs 16 mtu 1500
> > > negotiated_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS
> > > CTRL_VQ MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
> >
> > I wonder if lower case is better.
> >
>
> I thought the capital letters will emphasize the fact that these are
> flag bits. Also, note the matching kernel patches have this documented
> in the change log with capital letters.
Ok, that's fine.
>
> > >
> > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > ---
> > > vdpa/include/uapi/linux/vdpa.h | 4 ++
> > > vdpa/vdpa.c | 113 ++++++++++++++++++++++++++++++++-
> > > 2 files changed, 114 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/vdpa/include/uapi/linux/vdpa.h b/vdpa/include/uapi/linux/vdpa.h
> > > index b7eab069988a..171122dd03c9 100644
> > > --- a/vdpa/include/uapi/linux/vdpa.h
> > > +++ b/vdpa/include/uapi/linux/vdpa.h
> > > @@ -40,6 +40,10 @@ enum vdpa_attr {
> > > VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
> > > VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
> > >
> > > + VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
> > > + VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
> > > + VDPA_ATTR_DEV_SUPPORTED_FEATURES, /* u64 */
> >
> > I wonder if it's better to split the patches into three where the
> > above command could be implemented separately.
>
> I already sent three. You mean split the third patch into three?
Yes, since it introduces three functions
1) specify max_vqp
2) get supported features
3) get negotiated features
(Or at least two, 2 and 3 can be merged somehow)
>
> >
> > > +
> > > /* new attributes must be added above here */
> > > VDPA_ATTR_MAX,
> > > };
> > > diff --git a/vdpa/vdpa.c b/vdpa/vdpa.c
> > > index 4ccb564872a0..d0dd4196610f 100644
> > > --- a/vdpa/vdpa.c
> > > +++ b/vdpa/vdpa.c
> > > @@ -23,6 +23,7 @@
> > > #define VDPA_OPT_VDEV_HANDLE BIT(3)
> > > #define VDPA_OPT_VDEV_MAC BIT(4)
> > > #define VDPA_OPT_VDEV_MTU BIT(5)
> > > +#define VDPA_OPT_MAX_VQP BIT(6)
> > >
> > > struct vdpa_opts {
> > > uint64_t present; /* flags of present items */
> > > @@ -32,6 +33,7 @@ struct vdpa_opts {
> > > unsigned int device_id;
> > > char mac[ETH_ALEN];
> > > uint16_t mtu;
> > > + uint16_t max_vqp;
> > > };
> > >
> > > struct vdpa {
> > > @@ -78,6 +80,9 @@ static const enum mnl_attr_data_type vdpa_policy[VDPA_ATTR_MAX + 1] = {
> > > [VDPA_ATTR_DEV_VENDOR_ID] = MNL_TYPE_U32,
> > > [VDPA_ATTR_DEV_MAX_VQS] = MNL_TYPE_U32,
> > > [VDPA_ATTR_DEV_MAX_VQ_SIZE] = MNL_TYPE_U16,
> > > + [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,
> > > };
> > >
> > > static int attr_cb(const struct nlattr *attr, void *data)
> > > @@ -219,6 +224,8 @@ static void vdpa_opts_put(struct nlmsghdr *nlh, struct vdpa *vdpa)
> > > sizeof(opts->mac), opts->mac);
> > > if (opts->present & VDPA_OPT_VDEV_MTU)
> > > mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MTU, opts->mtu);
> > > + if (opts->present & VDPA_OPT_MAX_VQP)
> > > + mnl_attr_put_u16(nlh, VDPA_ATTR_DEV_NET_CFG_MAX_VQP, opts->max_vqp);
> > > }
> > >
> > > static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> > > @@ -287,6 +294,14 @@ static int vdpa_argv_parse(struct vdpa *vdpa, int argc, char **argv,
> > >
> > > NEXT_ARG_FWD();
> > > o_found |= VDPA_OPT_VDEV_MTU;
> > > + } else if ((matches(*argv, "max_vqp") == 0) && (o_optional & VDPA_OPT_MAX_VQP)) {
> > > + NEXT_ARG_FWD();
> > > + err = vdpa_argv_u16(vdpa, argc, argv, &opts->max_vqp);
> > > + if (err)
> > > + return err;
> > > +
> > > + NEXT_ARG_FWD();
> > > + o_found |= VDPA_OPT_MAX_VQP;
> > > } else {
> > > fprintf(stderr, "Unknown option \"%s\"\n", *argv);
> > > return -EINVAL;
> > > @@ -385,6 +400,77 @@ static const char *parse_class(int num)
> > > return class ? class : "< unknown class >";
> > > }
> > >
> > > +static const char * const net_feature_strs[64] = {
> > > + [VIRTIO_NET_F_CSUM] = "CSUM",
> > > + [VIRTIO_NET_F_GUEST_CSUM] = "GUEST_CSUM",
> > > + [VIRTIO_NET_F_CTRL_GUEST_OFFLOADS] = "CTRL_GUEST_OFFLOADS",
> > > + [VIRTIO_NET_F_MTU] = "MTU",
> > > + [VIRTIO_NET_F_MAC] = "MAC",
> > > + [VIRTIO_NET_F_GUEST_TSO4] = "GUEST_TSO4",
> > > + [VIRTIO_NET_F_GUEST_TSO6] = "GUEST_TSO6",
> > > + [VIRTIO_NET_F_GUEST_ECN] = "GUEST_ECN",
> > > + [VIRTIO_NET_F_GUEST_UFO] = "GUEST_UFO",
> > > + [VIRTIO_NET_F_HOST_TSO4] = "HOST_TSO4",
> > > + [VIRTIO_NET_F_HOST_TSO6] = "HOST_TSO6",
> > > + [VIRTIO_NET_F_HOST_ECN] = "HOST_ECN",
> > > + [VIRTIO_NET_F_HOST_UFO] = "HOST_UFO",
> > > + [VIRTIO_NET_F_MRG_RXBUF] = "MRG_RXBUF",
> > > + [VIRTIO_NET_F_STATUS] = "STATUS",
> > > + [VIRTIO_NET_F_CTRL_VQ] = "CTRL_VQ",
> > > + [VIRTIO_NET_F_CTRL_RX] = "CTRL_RX",
> > > + [VIRTIO_NET_F_CTRL_VLAN] = "CTRL_VLAN",
> > > + [VIRTIO_NET_F_CTRL_RX_EXTRA] = "CTRL_RX_EXTRA",
> > > + [VIRTIO_NET_F_GUEST_ANNOUNCE] = "GUEST_ANNOUNCE",
> > > + [VIRTIO_NET_F_MQ] = "MQ",
> > > + [VIRTIO_F_NOTIFY_ON_EMPTY] = "NOTIFY_ON_EMPTY",
> > > + [VIRTIO_NET_F_CTRL_MAC_ADDR] = "CTRL_MAC_ADDR",
> > > + [VIRTIO_F_ANY_LAYOUT] = "ANY_LAYOUT",
> > > + [VIRTIO_NET_F_RSC_EXT] = "RSC_EXT",
> > > + [VIRTIO_NET_F_STANDBY] = "STANDBY",
> > > +};
> >
> > It seems we are still missing things that are already supported in the
> > Linux uapi. I think it's better to support them. E.g the RSS and
> > SPEED_DUPLEX etc.
> >
>
> Will do.
>
> > > +
> > > +#define VDPA_EXT_FEATURES_SZ (VIRTIO_DEV_INDEPENDENT_F_END - \
> > > + VIRTIO_DEV_INDEPENDENT_F_START + 1)
> > > +
> > > +static const char * const ext_feature_strs[VDPA_EXT_FEATURES_SZ] = {
> > > + [VIRTIO_F_RING_INDIRECT_DESC - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_INDIRECT_DESC",
> > > + [VIRTIO_F_RING_EVENT_IDX - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_EVENT_IDX",
> > > + [VIRTIO_F_VERSION_1 - VIRTIO_DEV_INDEPENDENT_F_START] = "VERSION_1",
> > > + [VIRTIO_F_ACCESS_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ACCESS_PLATFORM",
> > > + [VIRTIO_F_RING_PACKED - VIRTIO_DEV_INDEPENDENT_F_START] = "RING_PACKED",
> > > + [VIRTIO_F_IN_ORDER - VIRTIO_DEV_INDEPENDENT_F_START] = "IN_ORDER",
> > > + [VIRTIO_F_ORDER_PLATFORM - VIRTIO_DEV_INDEPENDENT_F_START] = "ORDER_PLATFORM",
> > > + [VIRTIO_F_SR_IOV - VIRTIO_DEV_INDEPENDENT_F_START] = "SR_IOV",
> > > + [VIRTIO_F_NOTIFICATION_DATA - VIRTIO_DEV_INDEPENDENT_F_START] = "NOTIFICATION_DATA",
> > > +};
> > > +
> > > +static void print_net_features(struct vdpa *vdpa, uint64_t features, bool maxf)
> > > +{
> > > + const char *s;
> > > + int i;
> > > +
> > > + if (maxf)
> > > + pr_out_array_start(vdpa, "dev_features");
> > > + else
> > > + pr_out_array_start(vdpa, "negotiated_features");
> > > +
> > > + for (i = 0; i < 64; i++) {
> > > + if (!(features & (1ULL << i)))
> > > + continue;
> > > +
> > > + if (i >= VIRTIO_DEV_INDEPENDENT_F_START && i <= VIRTIO_DEV_INDEPENDENT_F_END)
> >
> > I don't see any issue that just use VIRTIO_TRANSPORT_F_START and
> > VIRTIO_TRANSPORT_F_END (even if END can change).
>
> I don't get you
I meant any reason we can't simply use VIRTIO_TRANSPORT_F_START/END?
>
> >
> > > + s = ext_feature_strs[i - VIRTIO_DEV_INDEPENDENT_F_START];
> > > + else
> > > + s = net_feature_strs[i];
> > > +
> > > + if (!s)
> > > + print_uint(PRINT_ANY, NULL, " unrecognized_bit_%d", i);
> > > + else
> > > + print_string(PRINT_ANY, NULL, " %s", s);
> > > + }
> > > + pr_out_array_end(vdpa);
> > > +}
> > > +
> > > static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> > > struct nlattr **tb)
> > > {
> > > @@ -408,6 +494,22 @@ static void pr_out_mgmtdev_show(struct vdpa *vdpa, const struct nlmsghdr *nlh,
> > > pr_out_array_end(vdpa);
> > > }
> > >
> > > + if (tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]) {
> > > + uint16_t num_vqs;
> > > +
> > > + if (!vdpa->json_output)
> > > + printf("\n");
> > > + num_vqs = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_MGMTDEV_MAX_VQS]);
> > > + print_uint(PRINT_ANY, "max_supported_vqs", " max_supported_vqs %d", num_vqs);
> > > + }
> > > +
> > > + if (tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]) {
> > > + uint64_t features;
> > > +
> > > + features = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_SUPPORTED_FEATURES]);
> > > + print_net_features(vdpa, features, true);
> >
> > Do we need to check whether it's a networking device before trying to
> > print the feature
>
> Yes, will fix
>
> > and for other type devices we can simply print the
> > bit number as a startup?
> >
>
> Why not add proper support (e.g. strings) for other types of devices when intoduced?
Then the vdpa tool won't show any features which seems sub-optimal.
Note that we've already had virtio-blk parent:
IFCVF, simulator and VDUSE.
Thanks
>
> > Thanks
> >
> > > + }
> > > +
> > > pr_out_handle_end(vdpa);
> > > }
> > >
> > > @@ -557,7 +659,7 @@ static int cmd_dev_add(struct vdpa *vdpa, int argc, char **argv)
> > > NLM_F_REQUEST | NLM_F_ACK);
> > > err = vdpa_argv_parse_put(nlh, vdpa, argc, argv,
> > > VDPA_OPT_VDEV_MGMTDEV_HANDLE | VDPA_OPT_VDEV_NAME,
> > > - VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU);
> > > + VDPA_OPT_VDEV_MAC | VDPA_OPT_VDEV_MTU | VDPA_OPT_MAX_VQP);
> > > if (err)
> > > return err;
> > >
> > > @@ -579,9 +681,10 @@ static int cmd_dev_del(struct vdpa *vdpa, int argc, char **argv)
> > > return mnlu_gen_socket_sndrcv(&vdpa->nlg, nlh, NULL, NULL);
> > > }
> > >
> > > -static void pr_out_dev_net_config(struct nlattr **tb)
> > > +static void pr_out_dev_net_config(struct vdpa *vdpa, struct nlattr **tb)
> > > {
> > > SPRINT_BUF(macaddr);
> > > + uint64_t val_u64;
> > > uint16_t val_u16;
> > >
> > > if (tb[VDPA_ATTR_DEV_NET_CFG_MACADDR]) {
> > > @@ -610,6 +713,10 @@ static void pr_out_dev_net_config(struct nlattr **tb)
> > > val_u16 = mnl_attr_get_u16(tb[VDPA_ATTR_DEV_NET_CFG_MTU]);
> > > print_uint(PRINT_ANY, "mtu", "mtu %d ", val_u16);
> > > }
> > > + if (tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]) {
> > > + val_u64 = mnl_attr_get_u64(tb[VDPA_ATTR_DEV_NEGOTIATED_FEATURES]);
> > > + print_net_features(vdpa, val_u64, false);
> > > + }
> > > }
> > >
> > > static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> > > @@ -619,7 +726,7 @@ static void pr_out_dev_config(struct vdpa *vdpa, struct nlattr **tb)
> > > pr_out_vdev_handle_start(vdpa, tb);
> > > switch (device_id) {
> > > case VIRTIO_ID_NET:
> > > - pr_out_dev_net_config(tb);
> > > + pr_out_dev_net_config(vdpa, tb);
> > > break;
> > > default:
> > > break;
> > > --
> > > 2.34.1
> > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-10 8:35 ` Jason Wang
@ 2022-02-10 9:27 ` Eli Cohen
2022-02-14 6:06 ` Jason Wang
0 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-10 9:27 UTC (permalink / raw)
To: Jason Wang; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Thu, Feb 10, 2022 at 04:35:28PM +0800, Jason Wang wrote:
> On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> > On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> > > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > > >
> > > > Define bit fields for device independent feature bits. We need them in a
> > > > follow up patch.
> > > >
> > > > Also, define macros for start and end of these feature bits.
> > > >
> > > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > ---
> > > > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > > > --- a/include/uapi/linux/virtio_config.h
> > > > +++ b/include/uapi/linux/virtio_config.h
> > > > @@ -45,14 +45,14 @@
> > > > /* We've given up on this device. */
> > > > #define VIRTIO_CONFIG_S_FAILED 0x80
> > > >
> > > > -/*
> > > > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > > > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > > > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > > > - * rest are per-device feature bits.
> > > > - */
> > > > -#define VIRTIO_TRANSPORT_F_START 28
> > > > -#define VIRTIO_TRANSPORT_F_END 38
> > > > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > > > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > > > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
> > >
> > > Haven't gone through patch 3 but I think it's probably better not
> > > touch uapi stuff. Or we can define those macros in other place?
> > >
> >
> > I can put it in vdpa.c
> >
> > > > +
> > > > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > > > +#define VIRTIO_F_RING_EVENT_IDX 29
> > > > +#define VIRTIO_F_IN_ORDER 35
> > > > +#define VIRTIO_F_NOTIFICATION_DATA 38
> > >
> > > This part belongs to the virtio_ring.h any reason not pull that file
> > > instead of squashing those into virtio_config.h?
> > >
> >
> > Not sure what you mean here. I can't find virtio_ring.h in my tree.
>
> I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
I will still miss VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_NOTIFICATION_DATA
which are only defined in drivers/net/ethernet/sfc/mcdi_pcol.h for block
devices only.
What would you suggest to do with them? Maybe define them in vdpa.c?
>
> Thanks
>
> >
> > > Thanks
> > >
> > > >
> > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > --
> > > > 2.34.1
> > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-10 9:27 ` Eli Cohen
@ 2022-02-14 6:06 ` Jason Wang
2022-02-16 7:15 ` Eli Cohen
0 siblings, 1 reply; 16+ messages in thread
From: Jason Wang @ 2022-02-14 6:06 UTC (permalink / raw)
To: Eli Cohen; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
在 2022/2/10 下午5:27, Eli Cohen 写道:
> On Thu, Feb 10, 2022 at 04:35:28PM +0800, Jason Wang wrote:
>> On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
>>> On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
>>>> On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
>>>>> Define bit fields for device independent feature bits. We need them in a
>>>>> follow up patch.
>>>>>
>>>>> Also, define macros for start and end of these feature bits.
>>>>>
>>>>> Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
>>>>> Signed-off-by: Eli Cohen <elic@nvidia.com>
>>>>> ---
>>>>> include/uapi/linux/virtio_config.h | 16 ++++++++--------
>>>>> 1 file changed, 8 insertions(+), 8 deletions(-)
>>>>>
>>>>> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
>>>>> index 3bf6c8bf8477..6d92cc31a8d3 100644
>>>>> --- a/include/uapi/linux/virtio_config.h
>>>>> +++ b/include/uapi/linux/virtio_config.h
>>>>> @@ -45,14 +45,14 @@
>>>>> /* We've given up on this device. */
>>>>> #define VIRTIO_CONFIG_S_FAILED 0x80
>>>>>
>>>>> -/*
>>>>> - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
>>>>> - * VIRTIO_TRANSPORT_F_END are reserved for the transport
>>>>> - * being used (e.g. virtio_ring, virtio_pci etc.), the
>>>>> - * rest are per-device feature bits.
>>>>> - */
>>>>> -#define VIRTIO_TRANSPORT_F_START 28
>>>>> -#define VIRTIO_TRANSPORT_F_END 38
>>>>> +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
>>>>> +#define VIRTIO_DEV_INDEPENDENT_F_START 28
>>>>> +#define VIRTIO_DEV_INDEPENDENT_F_END 38
>>>> Haven't gone through patch 3 but I think it's probably better not
>>>> touch uapi stuff. Or we can define those macros in other place?
>>>>
>>> I can put it in vdpa.c
>>>
>>>>> +
>>>>> +#define VIRTIO_F_RING_INDIRECT_DESC 28
>>>>> +#define VIRTIO_F_RING_EVENT_IDX 29
>>>>> +#define VIRTIO_F_IN_ORDER 35
>>>>> +#define VIRTIO_F_NOTIFICATION_DATA 38
>>>> This part belongs to the virtio_ring.h any reason not pull that file
>>>> instead of squashing those into virtio_config.h?
>>>>
>>> Not sure what you mean here. I can't find virtio_ring.h in my tree.
>> I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
> I will still miss VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_NOTIFICATION_DATA
> which are only defined in drivers/net/ethernet/sfc/mcdi_pcol.h for block
> devices only.
>
> What would you suggest to do with them? Maybe define them in vdpa.c?
I meant maybe we need a full synchronization from the current Linux uapi
headers for virtio_config.h and and add virtio_ring.h here.
Thanks
>
>> Thanks
>>
>>>> Thanks
>>>>
>>>>> #ifndef VIRTIO_CONFIG_NO_LEGACY
>>>>> /* Do we get callbacks when the ring is completely used, even if we've
>>>>> --
>>>>> 2.34.1
>>>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-14 6:06 ` Jason Wang
@ 2022-02-16 7:15 ` Eli Cohen
2022-02-17 6:06 ` Jason Wang
0 siblings, 1 reply; 16+ messages in thread
From: Eli Cohen @ 2022-02-16 7:15 UTC (permalink / raw)
To: Jason Wang; +Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu
On Mon, Feb 14, 2022 at 02:06:54PM +0800, Jason Wang wrote:
>
> 在 2022/2/10 下午5:27, Eli Cohen 写道:
> > On Thu, Feb 10, 2022 at 04:35:28PM +0800, Jason Wang wrote:
> > > On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> > > > > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > Define bit fields for device independent feature bits. We need them in a
> > > > > > follow up patch.
> > > > > >
> > > > > > Also, define macros for start and end of these feature bits.
> > > > > >
> > > > > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > ---
> > > > > > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > > > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > @@ -45,14 +45,14 @@
> > > > > > /* We've given up on this device. */
> > > > > > #define VIRTIO_CONFIG_S_FAILED 0x80
> > > > > >
> > > > > > -/*
> > > > > > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > > > > > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > > > > > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > > > > > - * rest are per-device feature bits.
> > > > > > - */
> > > > > > -#define VIRTIO_TRANSPORT_F_START 28
> > > > > > -#define VIRTIO_TRANSPORT_F_END 38
> > > > > > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
> > > > > Haven't gone through patch 3 but I think it's probably better not
> > > > > touch uapi stuff. Or we can define those macros in other place?
> > > > >
> > > > I can put it in vdpa.c
> > > >
> > > > > > +
> > > > > > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > > > > > +#define VIRTIO_F_RING_EVENT_IDX 29
> > > > > > +#define VIRTIO_F_IN_ORDER 35
> > > > > > +#define VIRTIO_F_NOTIFICATION_DATA 38
> > > > > This part belongs to the virtio_ring.h any reason not pull that file
> > > > > instead of squashing those into virtio_config.h?
> > > > >
> > > > Not sure what you mean here. I can't find virtio_ring.h in my tree.
> > > I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
> > I will still miss VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_NOTIFICATION_DATA
> > which are only defined in drivers/net/ethernet/sfc/mcdi_pcol.h for block
> > devices only.
> >
> > What would you suggest to do with them? Maybe define them in vdpa.c?
>
>
> I meant maybe we need a full synchronization from the current Linux uapi
> headers for virtio_config.h and and add virtio_ring.h here.
>
virtio_config.h is updatd already and virtio_ring.h does not add any
flag definition that we're missing.
The flags I was missing are
+#define VIRTIO_F_IN_ORDER 35
+#define VIRTIO_F_NOTIFICATION_DATA 38
and both of these do not appear in the linux headers. They appear as
block specific flags:
drivers/net/ethernet/sfc/mcdi_pcol.h:21471:#define VIRTIO_BLK_CONFIG_VIRTIO_F_IN_ORDER_LBN 35
drivers/net/ethernet/sfc/mcdi_pcol.h:21480:#define VIRTIO_BLK_CONFIG_VIRTIO_F_NOTIFICATION_DATA_LBN 38
So I just defined these two in vdpa.c (in patch v1).
> Thanks
>
>
> >
> > > Thanks
> > >
> > > > > Thanks
> > > > >
> > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > --
> > > > > > 2.34.1
> > > > > >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-16 7:15 ` Eli Cohen
@ 2022-02-17 6:06 ` Jason Wang
2022-02-17 8:26 ` Eli Cohen
0 siblings, 1 reply; 16+ messages in thread
From: Jason Wang @ 2022-02-17 6:06 UTC (permalink / raw)
To: Eli Cohen
Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu, Gautam Dawar
On Wed, Feb 16, 2022 at 3:16 PM Eli Cohen <elic@nvidia.com> wrote:
>
> On Mon, Feb 14, 2022 at 02:06:54PM +0800, Jason Wang wrote:
> >
> > 在 2022/2/10 下午5:27, Eli Cohen 写道:
> > > On Thu, Feb 10, 2022 at 04:35:28PM +0800, Jason Wang wrote:
> > > > On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> > > > > > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > Define bit fields for device independent feature bits. We need them in a
> > > > > > > follow up patch.
> > > > > > >
> > > > > > > Also, define macros for start and end of these feature bits.
> > > > > > >
> > > > > > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > ---
> > > > > > > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > > > > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > @@ -45,14 +45,14 @@
> > > > > > > /* We've given up on this device. */
> > > > > > > #define VIRTIO_CONFIG_S_FAILED 0x80
> > > > > > >
> > > > > > > -/*
> > > > > > > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > > > > > > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > > > > > > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > > > > > > - * rest are per-device feature bits.
> > > > > > > - */
> > > > > > > -#define VIRTIO_TRANSPORT_F_START 28
> > > > > > > -#define VIRTIO_TRANSPORT_F_END 38
> > > > > > > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
> > > > > > Haven't gone through patch 3 but I think it's probably better not
> > > > > > touch uapi stuff. Or we can define those macros in other place?
> > > > > >
> > > > > I can put it in vdpa.c
> > > > >
> > > > > > > +
> > > > > > > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > > > > > > +#define VIRTIO_F_RING_EVENT_IDX 29
> > > > > > > +#define VIRTIO_F_IN_ORDER 35
> > > > > > > +#define VIRTIO_F_NOTIFICATION_DATA 38
> > > > > > This part belongs to the virtio_ring.h any reason not pull that file
> > > > > > instead of squashing those into virtio_config.h?
> > > > > >
> > > > > Not sure what you mean here. I can't find virtio_ring.h in my tree.
> > > > I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
> > > I will still miss VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_NOTIFICATION_DATA
> > > which are only defined in drivers/net/ethernet/sfc/mcdi_pcol.h for block
> > > devices only.
> > >
> > > What would you suggest to do with them? Maybe define them in vdpa.c?
> >
> >
> > I meant maybe we need a full synchronization from the current Linux uapi
> > headers for virtio_config.h and and add virtio_ring.h here.
> >
>
> virtio_config.h is updatd already and virtio_ring.h does not add any
> flag definition that we're missing.
>
> The flags I was missing are
> +#define VIRTIO_F_IN_ORDER 35
> +#define VIRTIO_F_NOTIFICATION_DATA 38
Right, so Gautam posted a path for _F_IN_ORDER [1].
We probably need another patch for NOTIFICATION_DATA.
>
> and both of these do not appear in the linux headers. They appear as
> block specific flags:
>
> drivers/net/ethernet/sfc/mcdi_pcol.h:21471:#define VIRTIO_BLK_CONFIG_VIRTIO_F_IN_ORDER_LBN 35
> drivers/net/ethernet/sfc/mcdi_pcol.h:21480:#define VIRTIO_BLK_CONFIG_VIRTIO_F_NOTIFICATION_DATA_LBN 38
>
> So I just defined these two in vdpa.c (in patch v1).
Fine, but we need to remove them if we get update from linux kernel uapi headers
[1] https://lkml.org/lkml/2022/2/15/43
Thanks
>
> > Thanks
> >
> >
> > >
> > > > Thanks
> > > >
> > > > > > Thanks
> > > > > >
> > > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > > --
> > > > > > > 2.34.1
> > > > > > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] virtio: Define bit numbers for device independent features
2022-02-17 6:06 ` Jason Wang
@ 2022-02-17 8:26 ` Eli Cohen
0 siblings, 0 replies; 16+ messages in thread
From: Eli Cohen @ 2022-02-17 8:26 UTC (permalink / raw)
To: Jason Wang
Cc: Hemminger, Stephen, netdev, Si-Wei Liu, Jianbo Liu, Gautam Dawar
On Thu, Feb 17, 2022 at 02:06:39PM +0800, Jason Wang wrote:
> On Wed, Feb 16, 2022 at 3:16 PM Eli Cohen <elic@nvidia.com> wrote:
> >
> > On Mon, Feb 14, 2022 at 02:06:54PM +0800, Jason Wang wrote:
> > >
> > > 在 2022/2/10 下午5:27, Eli Cohen 写道:
> > > > On Thu, Feb 10, 2022 at 04:35:28PM +0800, Jason Wang wrote:
> > > > > On Thu, Feb 10, 2022 at 4:31 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > On Thu, Feb 10, 2022 at 03:54:57PM +0800, Jason Wang wrote:
> > > > > > > On Mon, Feb 7, 2022 at 8:56 PM Eli Cohen <elic@nvidia.com> wrote:
> > > > > > > > Define bit fields for device independent feature bits. We need them in a
> > > > > > > > follow up patch.
> > > > > > > >
> > > > > > > > Also, define macros for start and end of these feature bits.
> > > > > > > >
> > > > > > > > Reviewed-by: Jianbo Liu <jianbol@nvidia.com>
> > > > > > > > Signed-off-by: Eli Cohen <elic@nvidia.com>
> > > > > > > > ---
> > > > > > > > include/uapi/linux/virtio_config.h | 16 ++++++++--------
> > > > > > > > 1 file changed, 8 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> > > > > > > > index 3bf6c8bf8477..6d92cc31a8d3 100644
> > > > > > > > --- a/include/uapi/linux/virtio_config.h
> > > > > > > > +++ b/include/uapi/linux/virtio_config.h
> > > > > > > > @@ -45,14 +45,14 @@
> > > > > > > > /* We've given up on this device. */
> > > > > > > > #define VIRTIO_CONFIG_S_FAILED 0x80
> > > > > > > >
> > > > > > > > -/*
> > > > > > > > - * Virtio feature bits VIRTIO_TRANSPORT_F_START through
> > > > > > > > - * VIRTIO_TRANSPORT_F_END are reserved for the transport
> > > > > > > > - * being used (e.g. virtio_ring, virtio_pci etc.), the
> > > > > > > > - * rest are per-device feature bits.
> > > > > > > > - */
> > > > > > > > -#define VIRTIO_TRANSPORT_F_START 28
> > > > > > > > -#define VIRTIO_TRANSPORT_F_END 38
> > > > > > > > +/* Device independent features per virtio spec 1.1 range from 28 to 38 */
> > > > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_START 28
> > > > > > > > +#define VIRTIO_DEV_INDEPENDENT_F_END 38
> > > > > > > Haven't gone through patch 3 but I think it's probably better not
> > > > > > > touch uapi stuff. Or we can define those macros in other place?
> > > > > > >
> > > > > > I can put it in vdpa.c
> > > > > >
> > > > > > > > +
> > > > > > > > +#define VIRTIO_F_RING_INDIRECT_DESC 28
> > > > > > > > +#define VIRTIO_F_RING_EVENT_IDX 29
> > > > > > > > +#define VIRTIO_F_IN_ORDER 35
> > > > > > > > +#define VIRTIO_F_NOTIFICATION_DATA 38
> > > > > > > This part belongs to the virtio_ring.h any reason not pull that file
> > > > > > > instead of squashing those into virtio_config.h?
> > > > > > >
> > > > > > Not sure what you mean here. I can't find virtio_ring.h in my tree.
> > > > > I meant just copy the virtio_ring.h in the linux tree. It seems cleaner.
> > > > I will still miss VIRTIO_F_ORDER_PLATFORM and VIRTIO_F_NOTIFICATION_DATA
> > > > which are only defined in drivers/net/ethernet/sfc/mcdi_pcol.h for block
> > > > devices only.
> > > >
> > > > What would you suggest to do with them? Maybe define them in vdpa.c?
> > >
> > >
> > > I meant maybe we need a full synchronization from the current Linux uapi
> > > headers for virtio_config.h and and add virtio_ring.h here.
> > >
> >
> > virtio_config.h is updatd already and virtio_ring.h does not add any
> > flag definition that we're missing.
> >
> > The flags I was missing are
> > +#define VIRTIO_F_IN_ORDER 35
> > +#define VIRTIO_F_NOTIFICATION_DATA 38
>
> Right, so Gautam posted a path for _F_IN_ORDER [1].
>
> We probably need another patch for NOTIFICATION_DATA.
>
OK, I will send a patch NOTIFICATION_DATA. Once they are accepted, I
will follow up with a patch to remove from iproute2.
>
> >
> > and both of these do not appear in the linux headers. They appear as
> > block specific flags:
> >
> > drivers/net/ethernet/sfc/mcdi_pcol.h:21471:#define VIRTIO_BLK_CONFIG_VIRTIO_F_IN_ORDER_LBN 35
> > drivers/net/ethernet/sfc/mcdi_pcol.h:21480:#define VIRTIO_BLK_CONFIG_VIRTIO_F_NOTIFICATION_DATA_LBN 38
> >
> > So I just defined these two in vdpa.c (in patch v1).
>
> Fine, but we need to remove them if we get update from linux kernel uapi headers
>
> [1] https://lkml.org/lkml/2022/2/15/43
>
> Thanks
>
> >
> > > Thanks
> > >
> > >
> > > >
> > > > > Thanks
> > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > > #ifndef VIRTIO_CONFIG_NO_LEGACY
> > > > > > > > /* Do we get callbacks when the ring is completely used, even if we've
> > > > > > > > --
> > > > > > > > 2.34.1
> > > > > > > >
> > >
> >
>
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2022-02-17 8:26 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-07 12:55 [PATCH 0/3] vdpa tool support for configuring max VQs Eli Cohen
2022-02-07 12:55 ` [PATCH 1/3] vdpa: Remove unsupported command line option Eli Cohen
2022-02-10 7:50 ` Jason Wang
2022-02-07 12:55 ` [PATCH 2/3] virtio: Define bit numbers for device independent features Eli Cohen
2022-02-10 7:54 ` Jason Wang
2022-02-10 8:30 ` Eli Cohen
2022-02-10 8:35 ` Jason Wang
2022-02-10 9:27 ` Eli Cohen
2022-02-14 6:06 ` Jason Wang
2022-02-16 7:15 ` Eli Cohen
2022-02-17 6:06 ` Jason Wang
2022-02-17 8:26 ` Eli Cohen
2022-02-07 12:55 ` [PATCH 3/3] vdpa: Add support to configure max number of VQs Eli Cohen
2022-02-10 8:07 ` Jason Wang
2022-02-10 8:44 ` Eli Cohen
2022-02-10 8:51 ` Jason Wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).