This patchset is pre-requisite to export and provision device
config attributes and features for vdpa live migration, in a way
backward and forward compatibility can be retained. The follow up
work [1] will need to be built around the new feature provisioning
uAPI, with which it's easier to formalize migration compatibility
support at the driver level.
Thanks,
-Siwei
[1] [PATCH v3 0/4] vDPA: initial config export via "vdpa dev show"
https://lore.kernel.org/virtualization/[email protected]/
---
v2 -> v3:
- fix incorrect reference of local variable in future patch
- prohibit per-device bitmask macro from exposure in uapi header
- add fixes tag
v1 -> v2:
- include specific attribute info to error message
- move conditional feature presence in mlx5_vdpa config space
to a separate patch
- remove redundant check
---
Si-Wei Liu (6):
vdpa: fix improper error message when adding vdpa dev
vdpa: conditionally read STATUS in config space
vdpa: validate provisioned device features against specified attribute
vdpa: validate device feature provisioning against supported class
vdpa/mlx5: conditionally show MTU and STATUS in config space
vdpa/mlx5: support device features provisioning
drivers/vdpa/mlx5/net/mlx5_vnet.c | 71 +++++++++++++++++++++-----
drivers/vdpa/vdpa.c | 105 ++++++++++++++++++++++++++++++++------
2 files changed, 146 insertions(+), 30 deletions(-)
--
1.8.3.1
In below example, before the fix, mtu attribute is supported
by the parent mgmtdev, but the error message showing "All
provided are not supported" is just misleading.
$ vdpa mgmtdev show
vdpasim_net:
supported_classes net
max_supported_vqs 3
dev_features MTU MAC CTRL_VQ CTRL_MAC_ADDR ANY_LAYOUT VERSION_1 ACCESS_PLATFORM
$ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
Error: vdpa: All provided attributes are not supported.
kernel answers: Operation not supported
After fix, the relevant error message will be like:
$ vdpa dev add mgmtdev vdpasim_net name vdpasim0 mtu 5000 max_vqp 2
Error: vdpa: Some provided attributes are not supported: 0x1000.
kernel answers: Operation not supported
Fixes: d8ca2fa5be1b ("vdpa: Enable user to set mac and mtu of vdpa device")
Signed-off-by: Si-Wei Liu <[email protected]>
Reviewed-by: Parav Pandit <[email protected]>
Reviewed-by: Eli Cohen <[email protected]>
---
drivers/vdpa/vdpa.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 8ef7aa1..3a82ca78 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -622,9 +622,11 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
err = PTR_ERR(mdev);
goto err;
}
+
if ((config.mask & mdev->config_attr_mask) != config.mask) {
- NL_SET_ERR_MSG_MOD(info->extack,
- "All provided attributes are not supported");
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "Some provided attributes are not supported: 0x%llx",
+ config.mask & ~mdev->config_attr_mask);
err = -EOPNOTSUPP;
goto err;
}
--
1.8.3.1
Today when device features are explicitly provisioned, the features
user supplied may contain device class specific features that are
not supported by the parent managment device. On the other hand,
when parent managment device supports more than one class, the
device features to provision may be ambiguous if none of the class
specific attributes is provided at the same time. Validate these
cases and prompt appropriate user errors accordingly.
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/vdpa.c | 61 +++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 52 insertions(+), 9 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1eba978..4aa2160 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -460,12 +460,30 @@ static int vdpa_nl_mgmtdev_handle_fill(struct sk_buff *msg, const struct vdpa_mg
return 0;
}
+static u64 vdpa_mgmtdev_get_classes(const struct vdpa_mgmt_dev *mdev,
+ unsigned int *nclasses)
+{
+ u64 supported_classes = 0;
+ unsigned int n = 0;
+ int i = 0;
+
+ while (mdev->id_table[i].device) {
+ if (mdev->id_table[i].device <= 63) {
+ supported_classes |= BIT_ULL(mdev->id_table[i].device);
+ n++;
+ }
+ i++;
+ }
+ if (nclasses)
+ *nclasses = n;
+
+ return supported_classes;
+}
+
static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *msg,
u32 portid, u32 seq, int flags)
{
- u64 supported_classes = 0;
void *hdr;
- int i = 0;
int err;
hdr = genlmsg_put(msg, portid, seq, &vdpa_nl_family, flags, VDPA_CMD_MGMTDEV_NEW);
@@ -475,14 +493,9 @@ static int vdpa_mgmtdev_fill(const struct vdpa_mgmt_dev *mdev, struct sk_buff *m
if (err)
goto msg_err;
- while (mdev->id_table[i].device) {
- if (mdev->id_table[i].device <= 63)
- supported_classes |= BIT_ULL(mdev->id_table[i].device);
- i++;
- }
-
if (nla_put_u64_64bit(msg, VDPA_ATTR_MGMTDEV_SUPPORTED_CLASSES,
- supported_classes, VDPA_ATTR_UNSPEC)) {
+ vdpa_mgmtdev_get_classes(mdev, NULL),
+ VDPA_ATTR_UNSPEC)) {
err = -EMSGSIZE;
goto msg_err;
}
@@ -566,13 +579,25 @@ static int vdpa_nl_cmd_mgmtdev_get_doit(struct sk_buff *skb, struct genl_info *i
BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU) | \
BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP))
+/*
+ * Bitmask for all per-device features: feature bits VIRTIO_TRANSPORT_F_START
+ * through VIRTIO_TRANSPORT_F_END are unset, i.e. 0xfffffc000fffffff for
+ * all 64bit features. If the features are extended beyond 64 bits, or new
+ * "holes" are reserved for other type of features than per-device, this
+ * macro would have to be updated.
+ */
+#define VIRTIO_DEVICE_F_MASK (~0ULL << (VIRTIO_TRANSPORT_F_END + 1) | \
+ ((1ULL << VIRTIO_TRANSPORT_F_START) - 1))
+
static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *info)
{
struct vdpa_dev_set_config config = {};
struct nlattr **nl_attrs = info->attrs;
struct vdpa_mgmt_dev *mdev;
+ unsigned int ncls = 0;
const u8 *macaddr;
const char *name;
+ u64 classes;
int err = 0;
if (!info->attrs[VDPA_ATTR_DEV_NAME])
@@ -649,6 +674,24 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
goto err;
}
+ classes = vdpa_mgmtdev_get_classes(mdev, &ncls);
+ if (config.mask & VDPA_DEV_NET_ATTRS_MASK &&
+ !(classes & BIT_ULL(VIRTIO_ID_NET))) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "Network class attributes provided on unsupported management device");
+ err = -EINVAL;
+ goto err;
+ }
+ if (!(config.mask & VDPA_DEV_NET_ATTRS_MASK) &&
+ config.mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES) &&
+ classes & BIT_ULL(VIRTIO_ID_NET) && ncls > 1 &&
+ config.device_features & VIRTIO_DEVICE_F_MASK) {
+ NL_SET_ERR_MSG_MOD(info->extack,
+ "Management device supports multi-class while device features specified are ambiguous");
+ err = -EINVAL;
+ goto err;
+ }
+
err = mdev->ops->dev_add(mdev, name, &config);
err:
up_write(&vdpa_dev_lock);
--
1.8.3.1
With device feature provisioning, there's a chance for misconfiguration
that the vdpa feature attribute supplied in 'vdpa dev add' command doesn't
get selected on the device_features to be provisioned. For instance, when
a @mac attribute is specified, the corresponding feature bit _F_MAC in
device_features should be set for consistency. If there's conflict on
provisioned features against the attribute, it should be treated as an
error to fail the ambiguous command. Noted the opposite is not
necessarily true, for e.g. it's okay to have _F_MAC set in device_features
without providing a corresponding @mac attribute, in which case the vdpa
vendor driver could load certain default value for attribute that is not
explicitly specified.
Generalize this check in vdpa core so that there's no duplicate code in
each vendor driver.
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/vdpa.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 21c8aa3..1eba978 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
}
if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
+ u64 missing = 0x0ULL;
+
config.device_features =
nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MAC);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MTU);
+ if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
+ config.net.max_vq_pairs > 1 &&
+ !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
+ missing |= BIT_ULL(VIRTIO_NET_F_MQ);
+ if (missing) {
+ NL_SET_ERR_MSG_FMT_MOD(info->extack,
+ "Missing features 0x%llx for provided attributes",
+ missing);
+ return -EINVAL;
+ }
config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
}
--
1.8.3.1
This patch implements features provisioning for mlx5_vdpa.
1) Validate the provisioned features are a subset of the parent
features.
2) Clearing features that are not wanted by userspace.
For example:
# vdpa mgmtdev show
pci/0000:41:04.2:
supported_classes net
max_supported_vqs 65
dev_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ CTRL_VLAN MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
1) Provision vDPA device with all features derived from the parent
# vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2
# vdpa dev config show
vdpa1: mac e4:11:c6:d3:45:f0 link up link_announce false max_vq_pairs 1 mtu 1500
negotiated_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ CTRL_VLAN MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
2) Provision vDPA device with a subset of parent features
# vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2 device_features 0x300020000
# vdpa dev config show
vdpa1:
negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 53 +++++++++++++++++++++++++++++++++------
1 file changed, 45 insertions(+), 8 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 867ac18..b40dd1a 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2183,6 +2183,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
+ mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MAC);
return mlx_vdpa_features;
}
@@ -3062,6 +3063,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
struct mlx5_vdpa_dev *mvdev;
struct mlx5_vdpa_net *ndev;
struct mlx5_core_dev *mdev;
+ u64 device_features;
u32 max_vqs;
u16 mtu;
int err;
@@ -3070,6 +3072,24 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
return -ENOSPC;
mdev = mgtdev->madev->mdev;
+ device_features = mgtdev->mgtdev.supported_features;
+ if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) {
+ if (add_config->device_features & ~device_features) {
+ dev_warn(mdev->device,
+ "The provisioned features 0x%llx are not supported by this device with features 0x%llx\n",
+ add_config->device_features, device_features);
+ return -EINVAL;
+ }
+ device_features &= add_config->device_features;
+ }
+ if (!(device_features & BIT_ULL(VIRTIO_F_VERSION_1) &&
+ device_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) {
+ dev_warn(mdev->device,
+ "Must provision minimum features 0x%llx for this device",
+ BIT_ULL(VIRTIO_F_VERSION_1) | BIT_ULL(VIRTIO_F_ACCESS_PLATFORM));
+ return -EOPNOTSUPP;
+ }
+
if (!(MLX5_CAP_DEV_VDPA_EMULATION(mdev, virtio_queue_type) &
MLX5_VIRTIO_EMULATION_CAP_VIRTIO_QUEUE_TYPE_SPLIT)) {
dev_warn(mdev->device, "missing support for split virtqueues\n");
@@ -3098,7 +3118,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
if (IS_ERR(ndev))
return PTR_ERR(ndev);
- ndev->mvdev.mlx_features = mgtdev->mgtdev.supported_features;
ndev->mvdev.max_vqs = max_vqs;
mvdev = &ndev->mvdev;
mvdev->mdev = mdev;
@@ -3120,7 +3139,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
goto err_alloc;
}
- if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
+ if (device_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
err = query_mtu(mdev, &mtu);
if (err)
goto err_alloc;
@@ -3128,7 +3147,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
}
- if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
+ if (device_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
if (get_link_state(mvdev))
ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
else
@@ -3137,7 +3156,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
- } else {
+ /* No bother setting mac address in config if not going to provision _F_MAC */
+ } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) == 0 ||
+ device_features & BIT_ULL(VIRTIO_NET_F_MAC)) {
err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
if (err)
goto err_alloc;
@@ -3148,11 +3169,26 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
err = mlx5_mpfs_add_mac(pfmdev, config->mac);
if (err)
goto err_alloc;
-
- ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
+ } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) == 0) {
+ /*
+ * We used to clear _F_MAC feature bit if seeing
+ * zero mac address when device features are not
+ * specifically provisioned. Keep the behaviour
+ * so old scripts do not break.
+ */
+ device_features &= ~BIT_ULL(VIRTIO_NET_F_MAC);
+ } else if (device_features & BIT_ULL(VIRTIO_NET_F_MAC)) {
+ /* Don't provision zero mac address for _F_MAC */
+ mlx5_vdpa_warn(&ndev->mvdev,
+ "No mac address provisioned?\n");
+ err = -EINVAL;
+ goto err_alloc;
}
- config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
+ if (device_features & BIT_ULL(VIRTIO_NET_F_MQ))
+ config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
+
+ ndev->mvdev.mlx_features = device_features;
mvdev->vdev.dma_dev = &mdev->pdev->dev;
err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
if (err)
@@ -3249,7 +3285,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
mgtdev->mgtdev.id_table = id_table;
mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP) |
- BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU);
+ BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU) |
+ BIT_ULL(VDPA_ATTR_DEV_FEATURES);
mgtdev->mgtdev.max_supported_vqs =
MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues) + 1;
mgtdev->mgtdev.supported_features = get_supported_features(mdev);
--
1.8.3.1
The spec says:
status only exists if VIRTIO_NET_F_STATUS is set
Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
STATUS conditionally depending on the feature bits.
Signed-off-by: Si-Wei Liu <[email protected]>
Reviewed-by: Parav Pandit <[email protected]>
---
drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
1 file changed, 15 insertions(+), 5 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 3a82ca78..21c8aa3 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct sk_buff *msg, u64 features,
sizeof(config->mac), config->mac);
}
+static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
+ const struct virtio_net_config *config)
+{
+ u16 val_u16;
+
+ if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
+ return 0;
+
+ val_u16 = __virtio16_to_cpu(true, config->status);
+ return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
+}
+
static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
{
struct virtio_net_config config = {};
u64 features_device;
- u16 val_u16;
vdev->config->get_config(vdev, 0, &config, sizeof(config));
- val_u16 = __virtio16_to_cpu(true, config.status);
- if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
- return -EMSGSIZE;
-
features_device = vdev->config->get_device_features(vdev);
if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, features_device,
@@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
return -EMSGSIZE;
+ if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
+ return -EMSGSIZE;
+
return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
}
--
1.8.3.1
The spec says:
mtu only exists if VIRTIO_NET_F_MTU is set
status only exists if VIRTIO_NET_F_STATUS is set
We should only show MTU and STATUS conditionally depending on
the feature bits.
Signed-off-by: Si-Wei Liu <[email protected]>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 3a6dbbc6..867ac18 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
struct mlx5_vdpa_wq_ent *wqent;
if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
+ if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
+ return NOTIFY_DONE;
switch (eqe->sub_type) {
case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
@@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
goto err_alloc;
}
- err = query_mtu(mdev, &mtu);
- if (err)
- goto err_alloc;
+ if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
+ err = query_mtu(mdev, &mtu);
+ if (err)
+ goto err_alloc;
- ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+ ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
+ }
- if (get_link_state(mvdev))
- ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
- else
- ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
+ if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
+ if (get_link_state(mvdev))
+ ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
+ else
+ ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
+ }
if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
--
1.8.3.1
On 03/02/2023 7:01, Si-Wei Liu wrote:
> The spec says:
> status only exists if VIRTIO_NET_F_STATUS is set
>
> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
> STATUS conditionally depending on the feature bits.
>
> Signed-off-by: Si-Wei Liu <[email protected]>
> Reviewed-by: Parav Pandit <[email protected]>
> ---
> drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
> 1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 3a82ca78..21c8aa3 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct sk_buff *msg, u64 features,
> sizeof(config->mac), config->mac);
> }
>
> +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
> + const struct virtio_net_config *config)
> +{
> + u16 val_u16;
> +
> + if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
> + return 0;
Instead of returning 0 here, it would be better to explicitly put 0 in
the message field for
VDPA_ATTR_DEV_NET_STATUS
> +
> + val_u16 = __virtio16_to_cpu(true, config->status);
> + return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
> +}
> +
> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> {
> struct virtio_net_config config = {};
> u64 features_device;
> - u16 val_u16;
>
> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>
> - val_u16 = __virtio16_to_cpu(true, config.status);
> - if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
> - return -EMSGSIZE;
> -
> features_device = vdev->config->get_device_features(vdev);
>
> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES, features_device,
> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
> return -EMSGSIZE;
>
> + if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
> + return -EMSGSIZE;
> +
> return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
> }
>
On 03/02/2023 7:02, Si-Wei Liu wrote:
> With device feature provisioning, there's a chance for misconfiguration
> that the vdpa feature attribute supplied in 'vdpa dev add' command doesn't
> get selected on the device_features to be provisioned. For instance, when
> a @mac attribute is specified, the corresponding feature bit _F_MAC in
> device_features should be set for consistency. If there's conflict on
> provisioned features against the attribute, it should be treated as an
> error to fail the ambiguous command. Noted the opposite is not
> necessarily true, for e.g. it's okay to have _F_MAC set in device_features
> without providing a corresponding @mac attribute, in which case the vdpa
> vendor driver could load certain default value for attribute that is not
> explicitly specified.
>
> Generalize this check in vdpa core so that there's no duplicate code in
> each vendor driver.
>
> Signed-off-by: Si-Wei Liu <[email protected]>
Reviewed-by: Eli Cohen <[email protected]>
> ---
> drivers/vdpa/vdpa.c | 18 ++++++++++++++++++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 21c8aa3..1eba978 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -601,8 +601,26 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> config.mask |= BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP);
> }
> if (nl_attrs[VDPA_ATTR_DEV_FEATURES]) {
> + u64 missing = 0x0ULL;
> +
> config.device_features =
> nla_get_u64(nl_attrs[VDPA_ATTR_DEV_FEATURES]);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MACADDR] &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MAC)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MAC);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MTU] &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MTU)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MTU);
> + if (nl_attrs[VDPA_ATTR_DEV_NET_CFG_MAX_VQP] &&
> + config.net.max_vq_pairs > 1 &&
> + !(config.device_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> + missing |= BIT_ULL(VIRTIO_NET_F_MQ);
> + if (missing) {
> + NL_SET_ERR_MSG_FMT_MOD(info->extack,
> + "Missing features 0x%llx for provided attributes",
> + missing);
> + return -EINVAL;
> + }
> config.mask |= BIT_ULL(VDPA_ATTR_DEV_FEATURES);
> }
>
On 03/02/2023 7:02, Si-Wei Liu wrote:
> The spec says:
> mtu only exists if VIRTIO_NET_F_MTU is set
> status only exists if VIRTIO_NET_F_STATUS is set
>
> We should only show MTU and STATUS conditionally depending on
> the feature bits.
>
> Signed-off-by: Si-Wei Liu <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 3a6dbbc6..867ac18 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block *nb, unsigned long event, void *p
> struct mlx5_vdpa_wq_ent *wqent;
>
> if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
> + if (!(ndev->mvdev.actual_features & BIT_ULL(VIRTIO_NET_F_STATUS)))
> + return NOTIFY_DONE;
> switch (eqe->sub_type) {
> case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
> case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_alloc;
> }
>
> - err = query_mtu(mdev, &mtu);
> - if (err)
> - goto err_alloc;
> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
VIRTIO_NET_F_MTU is offered by the device. So conditional is always true. We are not done with feature negotiation at this stage so you may still set a value to device mtu if MTU won't be negotiated eventually. But that is not a problem because the spec says:
VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If
offered by the device, device
advises driver about the value of its maximum MTU. If negotiated, the
driver uses mtu as the maximum
MTU value.
So the driver will use whatever value is there only if negotiated.
> + err = query_mtu(mdev, &mtu);
> + if (err)
> + goto err_alloc;
>
> - ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> + ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> + }
>
> - if (get_link_state(mvdev))
> - ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> - else
> - ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> + if (get_link_state(mvdev))
> + ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> + else
> + ndev->config.status &= cpu_to_mlx5vdpa16(mvdev, ~VIRTIO_NET_S_LINK_UP);
> + }
Same thing here. Feature negotiation is not complete yet and if
VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will ignore this value.
>
> if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
On 2/5/2023 1:36 AM, Eli Cohen wrote:
>
> On 03/02/2023 7:02, Si-Wei Liu wrote:
>> The spec says:
>> mtu only exists if VIRTIO_NET_F_MTU is set
>> status only exists if VIRTIO_NET_F_STATUS is set
>>
>> We should only show MTU and STATUS conditionally depending on
>> the feature bits.
>>
>> Signed-off-by: Si-Wei Liu <[email protected]>
>> ---
>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index 3a6dbbc6..867ac18 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block
>> *nb, unsigned long event, void *p
>> struct mlx5_vdpa_wq_ent *wqent;
>> if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
>> + if (!(ndev->mvdev.actual_features &
>> BIT_ULL(VIRTIO_NET_F_STATUS)))
>> + return NOTIFY_DONE;
>> switch (eqe->sub_type) {
>> case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>> case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
>> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct
>> vdpa_mgmt_dev *v_mdev, const char *name,
>> goto err_alloc;
>> }
>> - err = query_mtu(mdev, &mtu);
>> - if (err)
>> - goto err_alloc;
>> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
>
> VIRTIO_NET_F_MTU is offered by the device. So conditional is always true.
With the next patch in series that selectively provisions device
features to mlx5_vdpa, this conditional will not be always true. That
was the reason why I made patch 5 and 6 in a single commit, as this
conditional will only be needed until feature provisioning is supported.
Basically patch 5 and 6 are logically connected and technically should
be separated out. I'm now puzzled, what was your thought then the change
in patch 5 shouldn't be part of patch 6?
> We are not done with feature negotiation at this stage so you
'You' are who? device, driver or guest user?
> may still set a value to device mtu if MTU won't be negotiated
> eventually. But that is not a problem because the spec says:
>
> VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If
> offered by the device, device
> advises driver about the value of its maximum MTU. If negotiated, the
> driver uses mtu as the maximum
>
> MTU value.
>
> So the driver will use whatever value is there only if negotiated.
My understanding is that 'vdpa dev config' now displays user provisioned
config, or default config value advertised by the device, rather than
what config driver will actually use.
>
>> + err = query_mtu(mdev, &mtu);
>> + if (err)
>> + goto err_alloc;
>> - ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>> + ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>> + }
>> - if (get_link_state(mvdev))
>> - ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
>> VIRTIO_NET_S_LINK_UP);
>> - else
>> - ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
>> ~VIRTIO_NET_S_LINK_UP);
>> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
>> + if (get_link_state(mvdev))
>> + ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
>> VIRTIO_NET_S_LINK_UP);
>> + else
>> + ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
>> ~VIRTIO_NET_S_LINK_UP);
>> + }
> Same thing here. Feature negotiation is not complete yet and if
>
> VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will
> ignore this value.
See above. With feature provisioning, whether the VIRTIO_NET_F_STATUS
feature is advertised by device is subject to the device_features value
provisioned by the host admin users.
-Siwei
>
>> if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>> memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
On 2/5/2023 12:26 AM, Eli Cohen wrote:
>
> On 03/02/2023 7:01, Si-Wei Liu wrote:
>> The spec says:
>> status only exists if VIRTIO_NET_F_STATUS is set
>>
>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>> STATUS conditionally depending on the feature bits.
>>
>> Signed-off-by: Si-Wei Liu <[email protected]>
>> Reviewed-by: Parav Pandit <[email protected]>
>> ---
>> drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 3a82ca78..21c8aa3 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct
>> sk_buff *msg, u64 features,
>> sizeof(config->mac), config->mac);
>> }
>> +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg,
>> u64 features,
>> + const struct virtio_net_config *config)
>> +{
>> + u16 val_u16;
>> +
>> + if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>> + return 0;
> Instead of returning 0 here, it would be better to explicitly put 0 in
> the message field for
>
> VDPA_ATTR_DEV_NET_STATUS
In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC
in dev cfg space"), the userspace must now show the config space field
presented by the device *as-is*. If the feature bit is not offered by
device, the relevant field will not be displayed in 'vdpa dev config'
output. For instance, MAC address won't be shown if the MAC feature is
not supported/offered by the device (note this has nothing to do with
negotiated features), even though the vdpa parent may have a non-zero
MAC address of its own. I think STATUS should not be different from MAC
and MTU here.
Regards,
-Siwei
>
>> +
>> + val_u16 = __virtio16_to_cpu(true, config->status);
>> + return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>> +}
>> +
>> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>> struct sk_buff *msg)
>> {
>> struct virtio_net_config config = {};
>> u64 features_device;
>> - u16 val_u16;
>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>> - val_u16 = __virtio16_to_cpu(true, config.status);
>> - if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>> - return -EMSGSIZE;
>> -
>> features_device = vdev->config->get_device_features(vdev);
>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
>> features_device,
>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct
>> vdpa_device *vdev, struct sk_buff *ms
>> if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>> return -EMSGSIZE;
>> + if (vdpa_dev_net_status_config_fill(msg, features_device,
>> &config))
>> + return -EMSGSIZE;
>> +
>> return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>> }
On 06/02/2023 6:53, Si-Wei Liu wrote:
>
>
> On 2/5/2023 12:26 AM, Eli Cohen wrote:
>>
>> On 03/02/2023 7:01, Si-Wei Liu wrote:
>>> The spec says:
>>> status only exists if VIRTIO_NET_F_STATUS is set
>>>
>>> Similar to MAC and MTU, vdpa_dev_net_config_fill() should read
>>> STATUS conditionally depending on the feature bits.
>>>
>>> Signed-off-by: Si-Wei Liu <[email protected]>
>>> Reviewed-by: Parav Pandit <[email protected]>
>>> ---
>>> drivers/vdpa/vdpa.c | 20 +++++++++++++++-----
>>> 1 file changed, 15 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 3a82ca78..21c8aa3 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -843,18 +843,25 @@ static int vdpa_dev_net_mac_config_fill(struct
>>> sk_buff *msg, u64 features,
>>> sizeof(config->mac), config->mac);
>>> }
>>> +static int vdpa_dev_net_status_config_fill(struct sk_buff *msg,
>>> u64 features,
>>> + const struct virtio_net_config *config)
>>> +{
>>> + u16 val_u16;
>>> +
>>> + if ((features & BIT_ULL(VIRTIO_NET_F_STATUS)) == 0)
>>> + return 0;
>> Instead of returning 0 here, it would be better to explicitly put 0
>> in the message field for
>>
>> VDPA_ATTR_DEV_NET_STATUS
> In light of commit 41a2ad927aa2 ("vDPA: conditionally read MTU and MAC
> in dev cfg space"), the userspace must now show the config space field
> presented by the device *as-is*. If the feature bit is not offered by
> device, the relevant field will not be displayed in 'vdpa dev config'
> output. For instance, MAC address won't be shown if the MAC feature is
> not supported/offered by the device (note this has nothing to do with
> negotiated features), even though the vdpa parent may have a non-zero
> MAC address of its own. I think STATUS should not be different from
> MAC and MTU here.
OK, I see your point.
Reviewed-by: Eli Cohen <[email protected]>
>
> Regards,
> -Siwei
>
>>
>>> +
>>> + val_u16 = __virtio16_to_cpu(true, config->status);
>>> + return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>>> +}
>>> +
>>> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev,
>>> struct sk_buff *msg)
>>> {
>>> struct virtio_net_config config = {};
>>> u64 features_device;
>>> - u16 val_u16;
>>> vdev->config->get_config(vdev, 0, &config, sizeof(config));
>>> - val_u16 = __virtio16_to_cpu(true, config.status);
>>> - if (nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16))
>>> - return -EMSGSIZE;
>>> -
>>> features_device = vdev->config->get_device_features(vdev);
>>> if (nla_put_u64_64bit(msg, VDPA_ATTR_DEV_FEATURES,
>>> features_device,
>>> @@ -867,6 +874,9 @@ static int vdpa_dev_net_config_fill(struct
>>> vdpa_device *vdev, struct sk_buff *ms
>>> if (vdpa_dev_net_mac_config_fill(msg, features_device, &config))
>>> return -EMSGSIZE;
>>> + if (vdpa_dev_net_status_config_fill(msg, features_device,
>>> &config))
>>> + return -EMSGSIZE;
>>> +
>>> return vdpa_dev_net_mq_config_fill(msg, features_device,
>>> &config);
>>> }
>
On 06/02/2023 6:53, Si-Wei Liu wrote:
>
>
> On 2/5/2023 1:36 AM, Eli Cohen wrote:
>>
>> On 03/02/2023 7:02, Si-Wei Liu wrote:
>>> The spec says:
>>> mtu only exists if VIRTIO_NET_F_MTU is set
>>> status only exists if VIRTIO_NET_F_STATUS is set
>>>
>>> We should only show MTU and STATUS conditionally depending on
>>> the feature bits.
>>>
>>> Signed-off-by: Si-Wei Liu <[email protected]>
>>> ---
>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++--------
>>> 1 file changed, 14 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index 3a6dbbc6..867ac18 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -3009,6 +3009,8 @@ static int event_handler(struct notifier_block
>>> *nb, unsigned long event, void *p
>>> struct mlx5_vdpa_wq_ent *wqent;
>>> if (event == MLX5_EVENT_TYPE_PORT_CHANGE) {
>>> + if (!(ndev->mvdev.actual_features &
>>> BIT_ULL(VIRTIO_NET_F_STATUS)))
>>> + return NOTIFY_DONE;
>>> switch (eqe->sub_type) {
>>> case MLX5_PORT_CHANGE_SUBTYPE_DOWN:
>>> case MLX5_PORT_CHANGE_SUBTYPE_ACTIVE:
>>> @@ -3118,16 +3120,20 @@ static int mlx5_vdpa_dev_add(struct
>>> vdpa_mgmt_dev *v_mdev, const char *name,
>>> goto err_alloc;
>>> }
>>> - err = query_mtu(mdev, &mtu);
>>> - if (err)
>>> - goto err_alloc;
>>> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
>>
>> VIRTIO_NET_F_MTU is offered by the device. So conditional is always
>> true.
> With the next patch in series that selectively provisions device
> features to mlx5_vdpa, this conditional will not be always true. That
> was the reason why I made patch 5 and 6 in a single commit, as this
> conditional will only be needed until feature provisioning is
> supported. Basically patch 5 and 6 are logically connected and
> technically should be separated out. I'm now puzzled, what was your
> thought then the change in patch 5 shouldn't be part of patch 6?
No, I think breaking into two patches is the right way to go.
I missed the fact the for setting MTU and MAC you need the device to
*offer* the feature and does not depend on negotiation.
>
>> We are not done with feature negotiation at this stage so you
> 'You' are who? device, driver or guest user?
>
>> may still set a value to device mtu if MTU won't be negotiated
>> eventually. But that is not a problem because the spec says:
>>
>> VIRTIO_NET_F_MTU(3) Device maximum MTU reporting is supported. If
>> offered by the device, device
>> advises driver about the value of its maximum MTU. If negotiated, the
>> driver uses mtu as the maximum
>>
>> MTU value.
>>
>> So the driver will use whatever value is there only if negotiated.
> My understanding is that 'vdpa dev config' now displays user
> provisioned config, or default config value advertised by the device,
> rather than what config driver will actually use.
Reviewed-by: Eli Cohen <[email protected]>
>
>>
>>> + err = query_mtu(mdev, &mtu);
>>> + if (err)
>>> + goto err_alloc;
>>> - ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>>> + ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
>>> + }
>>> - if (get_link_state(mvdev))
>>> - ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
>>> VIRTIO_NET_S_LINK_UP);
>>> - else
>>> - ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
>>> ~VIRTIO_NET_S_LINK_UP);
>>> + if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
>>> + if (get_link_state(mvdev))
>>> + ndev->config.status |= cpu_to_mlx5vdpa16(mvdev,
>>> VIRTIO_NET_S_LINK_UP);
>>> + else
>>> + ndev->config.status &= cpu_to_mlx5vdpa16(mvdev,
>>> ~VIRTIO_NET_S_LINK_UP);
>>> + }
>> Same thing here. Feature negotiation is not complete yet and if
>>
>> VIRTIO_NET_F_STATUS ends up not being negotiated, the driver will
>> ignore this value.
> See above. With feature provisioning, whether the VIRTIO_NET_F_STATUS
> feature is advertised by device is subject to the device_features
> value provisioned by the host admin users.
>
> -Siwei
>
>>
>>> if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
>>> memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
>
On 03/02/2023 7:02, Si-Wei Liu wrote:
> This patch implements features provisioning for mlx5_vdpa.
>
> 1) Validate the provisioned features are a subset of the parent
> features.
> 2) Clearing features that are not wanted by userspace.
>
> For example:
>
> # vdpa mgmtdev show
> pci/0000:41:04.2:
> supported_classes net
> max_supported_vqs 65
> dev_features CSUM GUEST_CSUM MTU MAC HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ CTRL_VLAN MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
>
> 1) Provision vDPA device with all features derived from the parent
>
> # vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2
> # vdpa dev config show
> vdpa1: mac e4:11:c6:d3:45:f0 link up link_announce false max_vq_pairs 1 mtu 1500
> negotiated_features CSUM GUEST_CSUM MTU HOST_TSO4 HOST_TSO6 STATUS CTRL_VQ CTRL_VLAN MQ CTRL_MAC_ADDR VERSION_1 ACCESS_PLATFORM
>
> 2) Provision vDPA device with a subset of parent features
>
> # vdpa dev add name vdpa1 mgmtdev pci/0000:41:04.2 device_features 0x300020000
> # vdpa dev config show
> vdpa1:
> negotiated_features CTRL_VQ VERSION_1 ACCESS_PLATFORM
>
> Signed-off-by: Si-Wei Liu <[email protected]>
Reviewed-by: Eli Cohen <[email protected]>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 53 +++++++++++++++++++++++++++++++++------
> 1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index 867ac18..b40dd1a 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2183,6 +2183,7 @@ static u64 get_supported_features(struct mlx5_core_dev *mdev)
> mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_STATUS);
> mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MTU);
> mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_CTRL_VLAN);
> + mlx_vdpa_features |= BIT_ULL(VIRTIO_NET_F_MAC);
>
> return mlx_vdpa_features;
> }
> @@ -3062,6 +3063,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> struct mlx5_vdpa_dev *mvdev;
> struct mlx5_vdpa_net *ndev;
> struct mlx5_core_dev *mdev;
> + u64 device_features;
> u32 max_vqs;
> u16 mtu;
> int err;
> @@ -3070,6 +3072,24 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> return -ENOSPC;
>
> mdev = mgtdev->madev->mdev;
> + device_features = mgtdev->mgtdev.supported_features;
> + if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) {
> + if (add_config->device_features & ~device_features) {
> + dev_warn(mdev->device,
> + "The provisioned features 0x%llx are not supported by this device with features 0x%llx\n",
> + add_config->device_features, device_features);
> + return -EINVAL;
> + }
> + device_features &= add_config->device_features;
> + }
> + if (!(device_features & BIT_ULL(VIRTIO_F_VERSION_1) &&
> + device_features & BIT_ULL(VIRTIO_F_ACCESS_PLATFORM))) {
> + dev_warn(mdev->device,
> + "Must provision minimum features 0x%llx for this device",
> + BIT_ULL(VIRTIO_F_VERSION_1) | BIT_ULL(VIRTIO_F_ACCESS_PLATFORM));
> + return -EOPNOTSUPP;
> + }
> +
> if (!(MLX5_CAP_DEV_VDPA_EMULATION(mdev, virtio_queue_type) &
> MLX5_VIRTIO_EMULATION_CAP_VIRTIO_QUEUE_TYPE_SPLIT)) {
> dev_warn(mdev->device, "missing support for split virtqueues\n");
> @@ -3098,7 +3118,6 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> if (IS_ERR(ndev))
> return PTR_ERR(ndev);
>
> - ndev->mvdev.mlx_features = mgtdev->mgtdev.supported_features;
> ndev->mvdev.max_vqs = max_vqs;
> mvdev = &ndev->mvdev;
> mvdev->mdev = mdev;
> @@ -3120,7 +3139,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> goto err_alloc;
> }
>
> - if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
> + if (device_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
> err = query_mtu(mdev, &mtu);
> if (err)
> goto err_alloc;
> @@ -3128,7 +3147,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> ndev->config.mtu = cpu_to_mlx5vdpa16(mvdev, mtu);
> }
>
> - if (ndev->mvdev.mlx_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> + if (device_features & BIT_ULL(VIRTIO_NET_F_STATUS)) {
> if (get_link_state(mvdev))
> ndev->config.status |= cpu_to_mlx5vdpa16(mvdev, VIRTIO_NET_S_LINK_UP);
> else
> @@ -3137,7 +3156,9 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>
> if (add_config->mask & (1 << VDPA_ATTR_DEV_NET_CFG_MACADDR)) {
> memcpy(ndev->config.mac, add_config->net.mac, ETH_ALEN);
> - } else {
> + /* No bother setting mac address in config if not going to provision _F_MAC */
> + } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) == 0 ||
> + device_features & BIT_ULL(VIRTIO_NET_F_MAC)) {
> err = mlx5_query_nic_vport_mac_address(mdev, 0, 0, config->mac);
> if (err)
> goto err_alloc;
> @@ -3148,11 +3169,26 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
> err = mlx5_mpfs_add_mac(pfmdev, config->mac);
> if (err)
> goto err_alloc;
> -
> - ndev->mvdev.mlx_features |= BIT_ULL(VIRTIO_NET_F_MAC);
> + } else if ((add_config->mask & BIT_ULL(VDPA_ATTR_DEV_FEATURES)) == 0) {
> + /*
> + * We used to clear _F_MAC feature bit if seeing
> + * zero mac address when device features are not
> + * specifically provisioned. Keep the behaviour
> + * so old scripts do not break.
> + */
> + device_features &= ~BIT_ULL(VIRTIO_NET_F_MAC);
> + } else if (device_features & BIT_ULL(VIRTIO_NET_F_MAC)) {
> + /* Don't provision zero mac address for _F_MAC */
> + mlx5_vdpa_warn(&ndev->mvdev,
> + "No mac address provisioned?\n");
> + err = -EINVAL;
> + goto err_alloc;
> }
>
> - config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
> + if (device_features & BIT_ULL(VIRTIO_NET_F_MQ))
> + config->max_virtqueue_pairs = cpu_to_mlx5vdpa16(mvdev, max_vqs / 2);
> +
> + ndev->mvdev.mlx_features = device_features;
> mvdev->vdev.dma_dev = &mdev->pdev->dev;
> err = mlx5_vdpa_alloc_resources(&ndev->mvdev);
> if (err)
> @@ -3249,7 +3285,8 @@ static int mlx5v_probe(struct auxiliary_device *adev,
> mgtdev->mgtdev.id_table = id_table;
> mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
> BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP) |
> - BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU);
> + BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU) |
> + BIT_ULL(VDPA_ATTR_DEV_FEATURES);
> mgtdev->mgtdev.max_supported_vqs =
> MLX5_CAP_DEV_VDPA_EMULATION(mdev, max_num_virtio_queues) + 1;
> mgtdev->mgtdev.supported_features = get_supported_features(mdev);