* [PATCH v2 1/7] vdpa: fix improper error message when adding vdpa dev
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
2023-02-02 4:45 ` Parav Pandit via Virtualization
2023-01-31 23:22 ` [PATCH v2 2/7] vdpa: conditionally read STATUS in config space Si-Wei Liu
` (5 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread* RE: [PATCH v2 1/7] vdpa: fix improper error message when adding vdpa dev
2023-01-31 23:22 ` [PATCH v2 1/7] vdpa: fix improper error message when adding vdpa dev Si-Wei Liu
@ 2023-02-02 4:45 ` Parav Pandit via Virtualization
0 siblings, 0 replies; 20+ messages in thread
From: Parav Pandit via Virtualization @ 2023-02-02 4:45 UTC (permalink / raw)
To: Si-Wei Liu, mst, jasowang, Eli Cohen; +Cc: linux-kernel, virtualization
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, January 31, 2023 6:22 PM
>
> 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
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
Please add fixes tag so that older kernel gets this fix.
With that change,
Reviewed-by: Parav Pandit <parav@nvidia.com>
> ---
> 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 2/7] vdpa: conditionally read STATUS in config space
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
2023-01-31 23:22 ` [PATCH v2 1/7] vdpa: fix improper error message when adding vdpa dev Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
2023-02-02 4:58 ` Parav Pandit via Virtualization
2023-01-31 23:22 ` [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute Si-Wei Liu
` (4 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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 <si-wei.liu@oracle.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread* RE: [PATCH v2 2/7] vdpa: conditionally read STATUS in config space
2023-01-31 23:22 ` [PATCH v2 2/7] vdpa: conditionally read STATUS in config space Si-Wei Liu
@ 2023-02-02 4:58 ` Parav Pandit via Virtualization
0 siblings, 0 replies; 20+ messages in thread
From: Parav Pandit via Virtualization @ 2023-02-02 4:58 UTC (permalink / raw)
To: Si-Wei Liu, mst, jasowang, Eli Cohen; +Cc: linux-kernel, virtualization
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, January 31, 2023 6:22 PM
>
> 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 <si-wei.liu@oracle.com>
> ---
> 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
Reviewed-by: Parav Pandit <parav@nvidia.com>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
2023-01-31 23:22 ` [PATCH v2 1/7] vdpa: fix improper error message when adding vdpa dev Si-Wei Liu
2023-01-31 23:22 ` [PATCH v2 2/7] vdpa: conditionally read STATUS in config space Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
2023-02-02 5:05 ` Parav Pandit via Virtualization
2023-01-31 23:22 ` [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features Si-Wei Liu
` (3 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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 <si-wei.liu@oracle.com>
---
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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread* RE: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
2023-01-31 23:22 ` [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute Si-Wei Liu
@ 2023-02-02 5:05 ` Parav Pandit via Virtualization
2023-02-02 21:58 ` Si-Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Parav Pandit via Virtualization @ 2023-02-02 5:05 UTC (permalink / raw)
To: Si-Wei Liu, mst, jasowang, Eli Cohen; +Cc: linux-kernel, virtualization
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Tuesday, January 31, 2023 6:22 PM
>
> 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 <si-wei.liu@oracle.com>
> ---
> 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
Vdpa this layer can likely derive the feature bits for the supplied config fields so that user doesn't need to keep track of both.
Only those feature bits which are unrelated to any config, is what user should be setting.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
2023-02-02 5:05 ` Parav Pandit via Virtualization
@ 2023-02-02 21:58 ` Si-Wei Liu
2023-02-02 22:01 ` Parav Pandit via Virtualization
0 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-02-02 21:58 UTC (permalink / raw)
To: Parav Pandit, mst, jasowang, Eli Cohen; +Cc: linux-kernel, virtualization
On 2/1/2023 9:05 PM, Parav Pandit wrote:
>
>> From: Si-Wei Liu <si-wei.liu@oracle.com>
>> Sent: Tuesday, January 31, 2023 6:22 PM
>>
>> 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 <si-wei.liu@oracle.com>
>> ---
>> 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
> Vdpa this layer can likely derive the feature bits for the supplied config fields so that user doesn't need to keep track of both.
> Only those feature bits which are unrelated to any config, is what user should be setting.
It's not I can't do this, but Jason wanted to have clear semantics
around migration compatibility for the driver, and for that users have
to explicitly provide device_features that we may define new driver
behavior (rather that inheritance which is implicit and not uniformly
define across drivers) for compatibility using the new uAPI.
Thanks,
-Siwei
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* RE: [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute
2023-02-02 21:58 ` Si-Wei Liu
@ 2023-02-02 22:01 ` Parav Pandit via Virtualization
0 siblings, 0 replies; 20+ messages in thread
From: Parav Pandit via Virtualization @ 2023-02-02 22:01 UTC (permalink / raw)
To: Si-Wei Liu, mst, jasowang, Eli Cohen; +Cc: linux-kernel, virtualization
> From: Si-Wei Liu <si-wei.liu@oracle.com>
> Sent: Thursday, February 2, 2023 4:59 PM
>
> On 2/1/2023 9:05 PM, Parav Pandit wrote:
> >
> >> From: Si-Wei Liu <si-wei.liu@oracle.com>
> >> Sent: Tuesday, January 31, 2023 6:22 PM
> >>
> >> 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 <si-wei.liu@oracle.com>
> >> ---
> >> 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
> > Vdpa this layer can likely derive the feature bits for the supplied config fields
> so that user doesn't need to keep track of both.
> > Only those feature bits which are unrelated to any config, is what user should
> be setting.
> It's not I can't do this, but Jason wanted to have clear semantics around
> migration compatibility for the driver, and for that users have to explicitly
> provide device_features that we may define new driver behavior (rather that
> inheritance which is implicit and not uniformly define across drivers) for
> compatibility using the new uAPI.
Make sense to explicitly tell, just requires more careful plumbing on the user space side.
Eventually it will get orchestrated by non user, so it should be fine to explicitly define it.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
` (2 preceding siblings ...)
2023-01-31 23:22 ` [PATCH v2 3/7] vdpa: validate provisioned device features against specified attribute Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
2023-02-02 17:29 ` Michael S. Tsirkin
2023-01-31 23:22 ` [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class Si-Wei Liu
` (2 subsequent siblings)
6 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
Introduce VIRTIO_DEVICE_F_MASK bitmask used for identification
of per-device features. Feature bits VIRTIO_TRANSPORT_F_START
through VIRTIO_TRANSPORT_F_END are reserved for transport
features hence are not counted as per-device features against
the 64bit feature space.
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
include/uapi/linux/virtio_config.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
index 3c05162..3bdc7ed 100644
--- a/include/uapi/linux/virtio_config.h
+++ b/include/uapi/linux/virtio_config.h
@@ -54,6 +54,14 @@
#define VIRTIO_TRANSPORT_F_START 28
#define VIRTIO_TRANSPORT_F_END 41
+/*
+ * 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
+ */
+#define VIRTIO_DEVICE_F_MASK (~0ULL << (VIRTIO_TRANSPORT_F_END + 1) | \
+ ((1ULL << VIRTIO_TRANSPORT_F_START) - 1))
+
#ifndef VIRTIO_CONFIG_NO_LEGACY
/* Do we get callbacks when the ring is completely used, even if we've
* suppressed them? */
--
1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features
2023-01-31 23:22 ` [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features Si-Wei Liu
@ 2023-02-02 17:29 ` Michael S. Tsirkin
2023-02-02 22:00 ` Si-Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2023-02-02 17:29 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: linux-kernel, elic, virtualization
On Tue, Jan 31, 2023 at 03:22:22PM -0800, Si-Wei Liu wrote:
> Introduce VIRTIO_DEVICE_F_MASK bitmask used for identification
> of per-device features. Feature bits VIRTIO_TRANSPORT_F_START
> through VIRTIO_TRANSPORT_F_END are reserved for transport
> features hence are not counted as per-device features against
> the 64bit feature space.
>
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
> include/uapi/linux/virtio_config.h | 8 ++++++++
> 1 file changed, 8 insertions(+)
Pls don't add this in uapi, people tend to depend on this and then
things fail when we extend virtio. For example this won't work with >64
feature bits.
>
> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
> index 3c05162..3bdc7ed 100644
> --- a/include/uapi/linux/virtio_config.h
> +++ b/include/uapi/linux/virtio_config.h
> @@ -54,6 +54,14 @@
> #define VIRTIO_TRANSPORT_F_START 28
> #define VIRTIO_TRANSPORT_F_END 41
>
> +/*
> + * 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
> + */
> +#define VIRTIO_DEVICE_F_MASK (~0ULL << (VIRTIO_TRANSPORT_F_END + 1) | \
> + ((1ULL << VIRTIO_TRANSPORT_F_START) - 1))
> +
> #ifndef VIRTIO_CONFIG_NO_LEGACY
> /* Do we get callbacks when the ring is completely used, even if we've
> * suppressed them? */
> --
> 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features
2023-02-02 17:29 ` Michael S. Tsirkin
@ 2023-02-02 22:00 ` Si-Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-02-02 22:00 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, elic, virtualization
On 2/2/2023 9:29 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2023 at 03:22:22PM -0800, Si-Wei Liu wrote:
>> Introduce VIRTIO_DEVICE_F_MASK bitmask used for identification
>> of per-device features. Feature bits VIRTIO_TRANSPORT_F_START
>> through VIRTIO_TRANSPORT_F_END are reserved for transport
>> features hence are not counted as per-device features against
>> the 64bit feature space.
>>
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> ---
>> include/uapi/linux/virtio_config.h | 8 ++++++++
>> 1 file changed, 8 insertions(+)
> Pls don't add this in uapi, people tend to depend on this and then
> things fail when we extend virtio. For example this won't work with >64
> feature bits.
Good point. Then keep this macro private to vdpa.c for now?
-Siwei
>
>> diff --git a/include/uapi/linux/virtio_config.h b/include/uapi/linux/virtio_config.h
>> index 3c05162..3bdc7ed 100644
>> --- a/include/uapi/linux/virtio_config.h
>> +++ b/include/uapi/linux/virtio_config.h
>> @@ -54,6 +54,14 @@
>> #define VIRTIO_TRANSPORT_F_START 28
>> #define VIRTIO_TRANSPORT_F_END 41
>>
>> +/*
>> + * 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
>> + */
>> +#define VIRTIO_DEVICE_F_MASK (~0ULL << (VIRTIO_TRANSPORT_F_END + 1) | \
>> + ((1ULL << VIRTIO_TRANSPORT_F_START) - 1))
>> +
>> #ifndef VIRTIO_CONFIG_NO_LEGACY
>> /* Do we get callbacks when the ring is completely used, even if we've
>> * suppressed them? */
>> --
>> 1.8.3.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
` (3 preceding siblings ...)
2023-01-31 23:22 ` [PATCH v2 4/7] virtio: VIRTIO_DEVICE_F_MASK for all per-device features Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
2023-02-03 8:09 ` Michael S. Tsirkin
2023-01-31 23:22 ` [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space Si-Wei Liu
2023-01-31 23:22 ` [PATCH v2 7/7] vdpa/mlx5: support device features provisioning Si-Wei Liu
6 siblings, 1 reply; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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 <si-wei.liu@oracle.com>
---
drivers/vdpa/vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
1 file changed, 42 insertions(+), 9 deletions(-)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 1eba978..35a72d6 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;
}
@@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
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 +664,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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class
2023-01-31 23:22 ` [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class Si-Wei Liu
@ 2023-02-03 8:09 ` Michael S. Tsirkin
2023-02-03 19:32 ` Si-Wei Liu
0 siblings, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2023-02-03 8:09 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: linux-kernel, elic, virtualization
On Tue, Jan 31, 2023 at 03:22:23PM -0800, Si-Wei Liu wrote:
> 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 <si-wei.liu@oracle.com>
> ---
> drivers/vdpa/vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 42 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 1eba978..35a72d6 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)
given max value is apparently 64 how important is it that it's unsigned?
Just make it an int.
Also I'd return u64 through a pointer too for consistency.
> +{
> + 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++;
> + }
Better as a for loop. I know you are just moving code if you
want to make it very clear it's a refactoring split
as a separate patch, but ok anyway.
> + 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;
> }
> @@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
> 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 +664,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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class
2023-02-03 8:09 ` Michael S. Tsirkin
@ 2023-02-03 19:32 ` Si-Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-02-03 19:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, elic, virtualization
On 2/3/2023 12:09 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2023 at 03:22:23PM -0800, Si-Wei Liu wrote:
>> 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 <si-wei.liu@oracle.com>
>> ---
>> drivers/vdpa/vdpa.c | 51 ++++++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 42 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 1eba978..35a72d6 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)
> given max value is apparently 64 how important is it that it's unsigned?
> Just make it an int.
Not sure I understand what you really meant/want. I consider making
unsigned is a (good) habit of keeping data type consistent to ensure
non-negative value is returned so callers run free of worry for false
complaint from (dumb) static code analyzer, and the next caller can
promptly interpret possible range of return value just from the function
prototype without having to dig into internals implemented by another
author. If your intent is to limit the range I can certainly make it an
unsigned char or u8, otherwise I don't get why you think int is better
than unsigned int. Does it not conform to the coding standard documented
somewhere?
> Also I'd return u64 through a pointer too for consistency.
Here the intent is to make the class bitmask number mandatory to return,
while the number of classes returned can be optional. If there's future
need to optionally return bitmask, the code can be revisited for sure.
For now I'd just keep it this way for simplicity and readability.
>> +{
>> + 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++;
>> + }
>
> Better as a for loop. I know you are just moving code if you
> want to make it very clear it's a refactoring split
> as a separate patch, but ok anyway.
I can make it a for loop. Generally if just moving code people tend to
keep the original code as-is without refactoring too much (separate
patch needed). But for this simple rewrite it might be okay, it's your call.
Thanks,
-Siwei
>
>> + 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;
>> }
>> @@ -571,8 +584,10 @@ static int vdpa_nl_cmd_dev_add_set_doit(struct sk_buff *skb, struct genl_info *i
>> 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 +664,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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
` (4 preceding siblings ...)
2023-01-31 23:22 ` [PATCH v2 5/7] vdpa: validate device feature provisioning against supported class Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
[not found] ` <bad92641-e10e-cb06-179d-2f4bc11c721d@nvidia.com>
2023-02-03 8:14 ` Michael S. Tsirkin
2023-01-31 23:22 ` [PATCH v2 7/7] vdpa/mlx5: support device features provisioning Si-Wei Liu
6 siblings, 2 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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 <si-wei.liu@oracle.com>
---
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..3d49eae 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 (device_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 (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
+ 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread[parent not found: <bad92641-e10e-cb06-179d-2f4bc11c721d@nvidia.com>]
* Re: [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space
[not found] ` <bad92641-e10e-cb06-179d-2f4bc11c721d@nvidia.com>
@ 2023-02-02 22:18 ` Si-Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-02-02 22:18 UTC (permalink / raw)
To: Eli Cohen, mst, jasowang, parav; +Cc: linux-kernel, virtualization
On 2/2/2023 5:31 AM, Eli Cohen wrote:
>
> On 01/02/2023 1:22, 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 <si-wei.liu@oracle.com>
>> ---
>> 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..3d49eae 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 (device_features & BIT_ULL(VIRTIO_NET_F_MTU)) {
>> + err = query_mtu(mdev, &mtu);
>> + if (err)
>> + goto err_alloc;
> device_features is not defined as a local variable. It is defined in
> the next patch.
Good catch! Will revise and post a v3.
Thanks!
-Siwei
>> - 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 (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
>> + 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);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space
2023-01-31 23:22 ` [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space Si-Wei Liu
[not found] ` <bad92641-e10e-cb06-179d-2f4bc11c721d@nvidia.com>
@ 2023-02-03 8:14 ` Michael S. Tsirkin
2023-02-03 19:37 ` Si-Wei Liu
1 sibling, 1 reply; 20+ messages in thread
From: Michael S. Tsirkin @ 2023-02-03 8:14 UTC (permalink / raw)
To: Si-Wei Liu; +Cc: linux-kernel, elic, virtualization
On Tue, Jan 31, 2023 at 03:22:24PM -0800, 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 <si-wei.liu@oracle.com>
so change the subject pls. it seems to say you are showing them
when you previously didn't, what's going on is something like:
make MTU/status access conditional on feature bits
> ---
> 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..3d49eae 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 (device_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 (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
> + 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space
2023-02-03 8:14 ` Michael S. Tsirkin
@ 2023-02-03 19:37 ` Si-Wei Liu
0 siblings, 0 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-02-03 19:37 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: linux-kernel, elic, virtualization
On 2/3/2023 12:14 AM, Michael S. Tsirkin wrote:
> On Tue, Jan 31, 2023 at 03:22:24PM -0800, 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 <si-wei.liu@oracle.com>
> so change the subject pls. it seems to say you are showing them
> when you previously didn't, what's going on is something like:
>
> make MTU/status access conditional on feature bits
I ever considered something similar as I wrote it, but there's no actual
config space access involved, and what the code is doing is to prohibit
*presenting* the related field to config space subject to feature bit.
So maybe this is more accurate?
make MTU/status presence conditional on feature bits
Thanks,
-Siwei
>
>> ---
>> 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..3d49eae 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 (device_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 (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
>> + 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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v2 7/7] vdpa/mlx5: support device features provisioning
2023-01-31 23:22 [PATCH v2 0/7] features provisioning fixes and mlx5_vdpa support Si-Wei Liu
` (5 preceding siblings ...)
2023-01-31 23:22 ` [PATCH v2 6/7] vdpa/mlx5: conditionally show MTU and STATUS in config space Si-Wei Liu
@ 2023-01-31 23:22 ` Si-Wei Liu
6 siblings, 0 replies; 20+ messages in thread
From: Si-Wei Liu @ 2023-01-31 23:22 UTC (permalink / raw)
To: mst, jasowang, parav, elic; +Cc: linux-kernel, virtualization
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 <si-wei.liu@oracle.com>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 49 ++++++++++++++++++++++++++++++++++-----
1 file changed, 43 insertions(+), 6 deletions(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index 3d49eae..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;
@@ -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
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related [flat|nested] 20+ messages in thread