From: Carlos Bilbao <cbilbao@digitalocean.com>
Include support to update the vDPA configuration fields of speed and
duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
values to UNKNOWN. Also add a warning message for when
mlx5_vdpa_get_config() receives offset and length out of bounds.
Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
---
drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
include/uapi/linux/vdpa.h | 2 ++
3 files changed, 62 insertions(+), 1 deletion(-)
diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index c47009a8b472..a44bb2072eec 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
if (offset + len <= sizeof(struct virtio_net_config))
memcpy(buf, (u8 *)&ndev->config + offset, len);
+ else
+ mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
}
static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
unsigned int len)
{
- /* not supported */
+ struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
+ struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
+
+ if (offset + len > sizeof(struct virtio_net_config)) {
+ mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
+ return;
+ }
+
+ /*
+ * Note that this will update the speed/duplex configuration fields
+ * but the hardware support to actually perform this change does
+ * not exist yet.
+ */
+ switch (offset) {
+ case offsetof(struct virtio_net_config, speed):
+ if (len == sizeof(((struct virtio_net_config *) 0)->speed))
+ memcpy(&ndev->config.speed, buf, len);
+ else
+ mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
+ break;
+
+ case offsetof(struct virtio_net_config, duplex):
+ if (len == sizeof(((struct virtio_net_config *)0)->duplex))
+ memcpy(&ndev->config.duplex, buf, len);
+ else
+ mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
+ break;
+
+ default:
+ mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
+ }
}
static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
index 4dbd2e55a288..b920e4405f6d 100644
--- a/drivers/vdpa/vdpa.c
+++ b/drivers/vdpa/vdpa.c
@@ -15,6 +15,7 @@
#include <net/genetlink.h>
#include <linux/mod_devicetable.h>
#include <linux/virtio_ids.h>
+#include <uapi/linux/ethtool.h>
static LIST_HEAD(mdev_head);
/* A global mutex that protects vdpa management device and device level operations. */
@@ -919,6 +920,22 @@ static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
}
+static int vdpa_dev_net_speed_config_fill(struct sk_buff *msg, u64 features,
+ struct virtio_net_config *config)
+{
+ __le32 speed = cpu_to_le32(SPEED_UNKNOWN);
+
+ return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, sizeof(speed), &speed);
+}
+
+static int vdpa_dev_net_duplex_config_fill(struct sk_buff *msg, u64 features,
+ struct virtio_net_config *config)
+{
+ u8 duplex = DUPLEX_UNKNOWN;
+
+ return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, sizeof(duplex), &duplex);
+}
+
static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
{
struct virtio_net_config config = {};
@@ -940,6 +957,16 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
return -EMSGSIZE;
+ /*
+ * mlx5_vdpa vDPA devicess currently do not support the
+ * VIRTIO_NET_F_SPEED_DUPLEX feature, which reports speed and
+ * duplex; hence these are set to UNKNOWN for now.
+ */
+ if (vdpa_dev_net_speed_config_fill(msg, features_device, &config))
+ return -EMSGSIZE;
+
+ if (vdpa_dev_net_duplex_config_fill(msg, features_device, &config))
+ return -EMSGSIZE;
return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
}
diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
index 842bf1201ac4..1c64ee0dd7b1 100644
--- a/include/uapi/linux/vdpa.h
+++ b/include/uapi/linux/vdpa.h
@@ -43,6 +43,8 @@ enum vdpa_attr {
VDPA_ATTR_DEV_NET_STATUS, /* u8 */
VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
+ VDPA_ATTR_DEV_NET_CFG_SPEED, /* u32 */
+ VDPA_ATTR_DEV_NET_CFG_DUPLEX, /* u8 */
VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
--
2.34.1(resending as I accidentally replied only to Carlos)
On 29.08.24 18:16, Carlos Bilbao wrote:
> From: Carlos Bilbao <cbilbao@digitalocean.com>
>
> Include support to update the vDPA configuration fields of speed and
> duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
> mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
> values to UNKNOWN. Also add a warning message for when
> mlx5_vdpa_get_config() receives offset and length out of bounds.
>
> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
> ---
> drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
> drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
> include/uapi/linux/vdpa.h | 2 ++
> 3 files changed, 62 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index c47009a8b472..a44bb2072eec 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>
> if (offset + len <= sizeof(struct virtio_net_config))
> memcpy(buf, (u8 *)&ndev->config + offset, len);
> + else
> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> }
>
> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
> unsigned int len)
> {
> - /* not supported */
> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> +
> + if (offset + len > sizeof(struct virtio_net_config)) {
> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> + return;
> + }
> +
> + /*
> + * Note that this will update the speed/duplex configuration fields
> + * but the hardware support to actually perform this change does
> + * not exist yet.
> + */
> + switch (offset) {
> + case offsetof(struct virtio_net_config, speed):
> + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
> + memcpy(&ndev->config.speed, buf, len);
> + else
> + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
> + break;
> +
> + case offsetof(struct virtio_net_config, duplex):
> + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
> + memcpy(&ndev->config.duplex, buf, len);
> + else
> + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
> + break;
> +
> + default:
> + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
This will trigger noise in dmesg because there is a MAC configuration here.
> + }
I would prefer that the .set_config remains a stub TBH. Setting the fields here is
misleading: the user might deduce that the configuration worked when they read the
values and see that they were updated.
Thanks,
dragos
> }
>
> static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> index 4dbd2e55a288..b920e4405f6d 100644
> --- a/drivers/vdpa/vdpa.c
> +++ b/drivers/vdpa/vdpa.c
> @@ -15,6 +15,7 @@
> #include <net/genetlink.h>
> #include <linux/mod_devicetable.h>
> #include <linux/virtio_ids.h>
> +#include <uapi/linux/ethtool.h>
>
> static LIST_HEAD(mdev_head);
> /* A global mutex that protects vdpa management device and device level operations. */
> @@ -919,6 +920,22 @@ static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
> }
>
> +static int vdpa_dev_net_speed_config_fill(struct sk_buff *msg, u64 features,
> + struct virtio_net_config *config)
> +{
> + __le32 speed = cpu_to_le32(SPEED_UNKNOWN);
> +
> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, sizeof(speed), &speed);
> +}
> +
> +static int vdpa_dev_net_duplex_config_fill(struct sk_buff *msg, u64 features,
> + struct virtio_net_config *config)
> +{
> + u8 duplex = DUPLEX_UNKNOWN;
> +
> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, sizeof(duplex), &duplex);
> +}
> +
> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> {
> struct virtio_net_config config = {};
> @@ -940,6 +957,16 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>
> if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
> return -EMSGSIZE;
> + /*
> + * mlx5_vdpa vDPA devicess currently do not support the
> + * VIRTIO_NET_F_SPEED_DUPLEX feature, which reports speed and
> + * duplex; hence these are set to UNKNOWN for now.
> + */
> + if (vdpa_dev_net_speed_config_fill(msg, features_device, &config))
> + return -EMSGSIZE;
> +
> + if (vdpa_dev_net_duplex_config_fill(msg, features_device, &config))
> + return -EMSGSIZE;
>
> return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
> }
> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> index 842bf1201ac4..1c64ee0dd7b1 100644
> --- a/include/uapi/linux/vdpa.h
> +++ b/include/uapi/linux/vdpa.h
> @@ -43,6 +43,8 @@ enum vdpa_attr {
> VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
> VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
> + VDPA_ATTR_DEV_NET_CFG_SPEED, /* u32 */
> + VDPA_ATTR_DEV_NET_CFG_DUPLEX, /* u8 */
>
> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
On Fri, Aug 30, 2024 at 5:08 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
> (resending as I accidentally replied only to Carlos)
>
> On 29.08.24 18:16, Carlos Bilbao wrote:
> > From: Carlos Bilbao <cbilbao@digitalocean.com>
> >
> > Include support to update the vDPA configuration fields of speed and
> > duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
> > mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
> > values to UNKNOWN. Also add a warning message for when
> > mlx5_vdpa_get_config() receives offset and length out of bounds.
> >
> > Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
> > ---
> > drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
> > drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
> > include/uapi/linux/vdpa.h | 2 ++
> > 3 files changed, 62 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > index c47009a8b472..a44bb2072eec 100644
> > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> > @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> >
> > if (offset + len <= sizeof(struct virtio_net_config))
> > memcpy(buf, (u8 *)&ndev->config + offset, len);
> > + else
> > + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> > }
> >
> > static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
> > unsigned int len)
> > {
> > - /* not supported */
> > + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> > + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> > +
> > + if (offset + len > sizeof(struct virtio_net_config)) {
> > + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> > + return;
> > + }
> > +
> > + /*
> > + * Note that this will update the speed/duplex configuration fields
> > + * but the hardware support to actually perform this change does
> > + * not exist yet.
> > + */
> > + switch (offset) {
> > + case offsetof(struct virtio_net_config, speed):
> > + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
> > + memcpy(&ndev->config.speed, buf, len);
> > + else
> > + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
> > + break;
> > +
> > + case offsetof(struct virtio_net_config, duplex):
> > + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
> > + memcpy(&ndev->config.duplex, buf, len);
> > + else
> > + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
> > + break;
> > +
> > + default:
> > + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
> This will trigger noise in dmesg because there is a MAC configuration here.
> > + }
> I would prefer that the .set_config remains a stub TBH. Setting the fields here is
> misleading: the user might deduce that the configuration worked when they read the
> values and see that they were updated.
Yes, and actually, those fields are read-only according to the spec:
"""
The network device has the following device configuration layout. All
of the device configuration fields are read-only for the driver.
"""
Thanks
>
> Thanks,
> dragos
> > }
> >
> > static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
> > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
> > index 4dbd2e55a288..b920e4405f6d 100644
> > --- a/drivers/vdpa/vdpa.c
> > +++ b/drivers/vdpa/vdpa.c
> > @@ -15,6 +15,7 @@
> > #include <net/genetlink.h>
> > #include <linux/mod_devicetable.h>
> > #include <linux/virtio_ids.h>
> > +#include <uapi/linux/ethtool.h>
> >
> > static LIST_HEAD(mdev_head);
> > /* A global mutex that protects vdpa management device and device level operations. */
> > @@ -919,6 +920,22 @@ static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
> > return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
> > }
> >
> > +static int vdpa_dev_net_speed_config_fill(struct sk_buff *msg, u64 features,
> > + struct virtio_net_config *config)
> > +{
> > + __le32 speed = cpu_to_le32(SPEED_UNKNOWN);
> > +
> > + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, sizeof(speed), &speed);
> > +}
> > +
> > +static int vdpa_dev_net_duplex_config_fill(struct sk_buff *msg, u64 features,
> > + struct virtio_net_config *config)
> > +{
> > + u8 duplex = DUPLEX_UNKNOWN;
> > +
> > + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, sizeof(duplex), &duplex);
> > +}
> > +
> > static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
> > {
> > struct virtio_net_config config = {};
> > @@ -940,6 +957,16 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
> >
> > if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
> > return -EMSGSIZE;
> > + /*
> > + * mlx5_vdpa vDPA devicess currently do not support the
> > + * VIRTIO_NET_F_SPEED_DUPLEX feature, which reports speed and
> > + * duplex; hence these are set to UNKNOWN for now.
> > + */
> > + if (vdpa_dev_net_speed_config_fill(msg, features_device, &config))
> > + return -EMSGSIZE;
> > +
> > + if (vdpa_dev_net_duplex_config_fill(msg, features_device, &config))
> > + return -EMSGSIZE;
> >
> > return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
> > }
> > diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
> > index 842bf1201ac4..1c64ee0dd7b1 100644
> > --- a/include/uapi/linux/vdpa.h
> > +++ b/include/uapi/linux/vdpa.h
> > @@ -43,6 +43,8 @@ enum vdpa_attr {
> > VDPA_ATTR_DEV_NET_STATUS, /* u8 */
> > VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
> > VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
> > + VDPA_ATTR_DEV_NET_CFG_SPEED, /* u32 */
> > + VDPA_ATTR_DEV_NET_CFG_DUPLEX, /* u8 */
> >
> > VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
> > VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
>
Hello,
On 8/29/24 9:31 PM, Jason Wang wrote:
> On Fri, Aug 30, 2024 at 5:08 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>> (resending as I accidentally replied only to Carlos)
>>
>> On 29.08.24 18:16, Carlos Bilbao wrote:
>>> From: Carlos Bilbao <cbilbao@digitalocean.com>
>>>
>>> Include support to update the vDPA configuration fields of speed and
>>> duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
>>> mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
>>> values to UNKNOWN. Also add a warning message for when
>>> mlx5_vdpa_get_config() receives offset and length out of bounds.
>>>
>>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
>>> ---
>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
>>> drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
>>> include/uapi/linux/vdpa.h | 2 ++
>>> 3 files changed, 62 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> index c47009a8b472..a44bb2072eec 100644
>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>> @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>>>
>>> if (offset + len <= sizeof(struct virtio_net_config))
>>> memcpy(buf, (u8 *)&ndev->config + offset, len);
>>> + else
>>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>>> }
>>>
>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>> unsigned int len)
>>> {
>>> - /* not supported */
>>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>> +
>>> + if (offset + len > sizeof(struct virtio_net_config)) {
>>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>>> + return;
>>> + }
>>> +
>>> + /*
>>> + * Note that this will update the speed/duplex configuration fields
>>> + * but the hardware support to actually perform this change does
>>> + * not exist yet.
>>> + */
>>> + switch (offset) {
>>> + case offsetof(struct virtio_net_config, speed):
>>> + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
>>> + memcpy(&ndev->config.speed, buf, len);
>>> + else
>>> + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
>>> + break;
>>> +
>>> + case offsetof(struct virtio_net_config, duplex):
>>> + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
>>> + memcpy(&ndev->config.duplex, buf, len);
>>> + else
>>> + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
>>> + break;
>>> +
>>> + default:
>>> + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
>> This will trigger noise in dmesg because there is a MAC configuration here.
>>> + }
>> I would prefer that the .set_config remains a stub TBH. Setting the fields here is
>> misleading: the user might deduce that the configuration worked when they read the
>> values and see that they were updated.
> Yes, and actually, those fields are read-only according to the spec:
>
> """
> The network device has the following device configuration layout. All
> of the device configuration fields are read-only for the driver.
> """
>
> Thanks
Should I go ahead and remove the ioctl then?
>
>> Thanks,
>> dragos
>>> }
>>>
>>> static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>>> index 4dbd2e55a288..b920e4405f6d 100644
>>> --- a/drivers/vdpa/vdpa.c
>>> +++ b/drivers/vdpa/vdpa.c
>>> @@ -15,6 +15,7 @@
>>> #include <net/genetlink.h>
>>> #include <linux/mod_devicetable.h>
>>> #include <linux/virtio_ids.h>
>>> +#include <uapi/linux/ethtool.h>
>>>
>>> static LIST_HEAD(mdev_head);
>>> /* A global mutex that protects vdpa management device and device level operations. */
>>> @@ -919,6 +920,22 @@ static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
>>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>>> }
>>>
>>> +static int vdpa_dev_net_speed_config_fill(struct sk_buff *msg, u64 features,
>>> + struct virtio_net_config *config)
>>> +{
>>> + __le32 speed = cpu_to_le32(SPEED_UNKNOWN);
>>> +
>>> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, sizeof(speed), &speed);
>>> +}
>>> +
>>> +static int vdpa_dev_net_duplex_config_fill(struct sk_buff *msg, u64 features,
>>> + struct virtio_net_config *config)
>>> +{
>>> + u8 duplex = DUPLEX_UNKNOWN;
>>> +
>>> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, sizeof(duplex), &duplex);
>>> +}
>>> +
>>> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
>>> {
>>> struct virtio_net_config config = {};
>>> @@ -940,6 +957,16 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>>
>>> if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
>>> return -EMSGSIZE;
>>> + /*
>>> + * mlx5_vdpa vDPA devicess currently do not support the
>>> + * VIRTIO_NET_F_SPEED_DUPLEX feature, which reports speed and
>>> + * duplex; hence these are set to UNKNOWN for now.
>>> + */
>>> + if (vdpa_dev_net_speed_config_fill(msg, features_device, &config))
>>> + return -EMSGSIZE;
>>> +
>>> + if (vdpa_dev_net_duplex_config_fill(msg, features_device, &config))
>>> + return -EMSGSIZE;
>>>
>>> return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>>> }
>>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>>> index 842bf1201ac4..1c64ee0dd7b1 100644
>>> --- a/include/uapi/linux/vdpa.h
>>> +++ b/include/uapi/linux/vdpa.h
>>> @@ -43,6 +43,8 @@ enum vdpa_attr {
>>> VDPA_ATTR_DEV_NET_STATUS, /* u8 */
>>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
>>> VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
>>> + VDPA_ATTR_DEV_NET_CFG_SPEED, /* u32 */
>>> + VDPA_ATTR_DEV_NET_CFG_DUPLEX, /* u8 */
>>>
>>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
>>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
Thanks, Carlos
On Fri, Aug 30, 2024 at 9:15 PM Carlos Bilbao <cbilbao@digitalocean.com> wrote:
>
> Hello,
>
> On 8/29/24 9:31 PM, Jason Wang wrote:
> > On Fri, Aug 30, 2024 at 5:08 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >> (resending as I accidentally replied only to Carlos)
> >>
> >> On 29.08.24 18:16, Carlos Bilbao wrote:
> >>> From: Carlos Bilbao <cbilbao@digitalocean.com>
> >>>
> >>> Include support to update the vDPA configuration fields of speed and
> >>> duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
> >>> mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
> >>> values to UNKNOWN. Also add a warning message for when
> >>> mlx5_vdpa_get_config() receives offset and length out of bounds.
> >>>
> >>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
> >>> ---
> >>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
> >>> drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
> >>> include/uapi/linux/vdpa.h | 2 ++
> >>> 3 files changed, 62 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> index c47009a8b472..a44bb2072eec 100644
> >>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> >>> @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
> >>>
> >>> if (offset + len <= sizeof(struct virtio_net_config))
> >>> memcpy(buf, (u8 *)&ndev->config + offset, len);
> >>> + else
> >>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> >>> }
> >>>
> >>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
> >>> unsigned int len)
> >>> {
> >>> - /* not supported */
> >>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
> >>> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
> >>> +
> >>> + if (offset + len > sizeof(struct virtio_net_config)) {
> >>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
> >>> + return;
> >>> + }
> >>> +
> >>> + /*
> >>> + * Note that this will update the speed/duplex configuration fields
> >>> + * but the hardware support to actually perform this change does
> >>> + * not exist yet.
> >>> + */
> >>> + switch (offset) {
> >>> + case offsetof(struct virtio_net_config, speed):
> >>> + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
> >>> + memcpy(&ndev->config.speed, buf, len);
> >>> + else
> >>> + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
> >>> + break;
> >>> +
> >>> + case offsetof(struct virtio_net_config, duplex):
> >>> + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
> >>> + memcpy(&ndev->config.duplex, buf, len);
> >>> + else
> >>> + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
> >>> + break;
> >>> +
> >>> + default:
> >>> + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
> >> This will trigger noise in dmesg because there is a MAC configuration here.
> >>> + }
> >> I would prefer that the .set_config remains a stub TBH. Setting the fields here is
> >> misleading: the user might deduce that the configuration worked when they read the
> >> values and see that they were updated.
> > Yes, and actually, those fields are read-only according to the spec:
> >
> > """
> > The network device has the following device configuration layout. All
> > of the device configuration fields are read-only for the driver.
> > """
> >
> > Thanks
>
>
> Should I go ahead and remove the ioctl then?
If you meant mlx5_vdpa_set_config, I think yes.
Thanks
Hello,
On 9/1/24 11:27 PM, Jason Wang wrote:
> On Fri, Aug 30, 2024 at 9:15 PM Carlos Bilbao <cbilbao@digitalocean.com> wrote:
>> Hello,
>>
>> On 8/29/24 9:31 PM, Jason Wang wrote:
>>> On Fri, Aug 30, 2024 at 5:08 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>> (resending as I accidentally replied only to Carlos)
>>>>
>>>> On 29.08.24 18:16, Carlos Bilbao wrote:
>>>>> From: Carlos Bilbao <cbilbao@digitalocean.com>
>>>>>
>>>>> Include support to update the vDPA configuration fields of speed and
>>>>> duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
>>>>> mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
>>>>> values to UNKNOWN. Also add a warning message for when
>>>>> mlx5_vdpa_get_config() receives offset and length out of bounds.
>>>>>
>>>>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
>>>>> ---
>>>>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
>>>>> drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
>>>>> include/uapi/linux/vdpa.h | 2 ++
>>>>> 3 files changed, 62 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> index c47009a8b472..a44bb2072eec 100644
>>>>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>>>>> @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>>>>>
>>>>> if (offset + len <= sizeof(struct virtio_net_config))
>>>>> memcpy(buf, (u8 *)&ndev->config + offset, len);
>>>>> + else
>>>>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>>>>> }
>>>>>
>>>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>>>> unsigned int len)
>>>>> {
>>>>> - /* not supported */
>>>>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>>>>> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>>>>> +
>>>>> + if (offset + len > sizeof(struct virtio_net_config)) {
>>>>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>>>>> + return;
>>>>> + }
>>>>> +
>>>>> + /*
>>>>> + * Note that this will update the speed/duplex configuration fields
>>>>> + * but the hardware support to actually perform this change does
>>>>> + * not exist yet.
>>>>> + */
>>>>> + switch (offset) {
>>>>> + case offsetof(struct virtio_net_config, speed):
>>>>> + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
>>>>> + memcpy(&ndev->config.speed, buf, len);
>>>>> + else
>>>>> + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
>>>>> + break;
>>>>> +
>>>>> + case offsetof(struct virtio_net_config, duplex):
>>>>> + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
>>>>> + memcpy(&ndev->config.duplex, buf, len);
>>>>> + else
>>>>> + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
>>>>> + break;
>>>>> +
>>>>> + default:
>>>>> + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
>>>> This will trigger noise in dmesg because there is a MAC configuration here.
>>>>> + }
>>>> I would prefer that the .set_config remains a stub TBH. Setting the fields here is
>>>> misleading: the user might deduce that the configuration worked when they read the
>>>> values and see that they were updated.
>>> Yes, and actually, those fields are read-only according to the spec:
>>>
>>> """
>>> The network device has the following device configuration layout. All
>>> of the device configuration fields are read-only for the driver.
>>> """
>>>
>>> Thanks
>>
>> Should I go ahead and remove the ioctl then?
> If you meant mlx5_vdpa_set_config, I think yes.
Ack, I will send a new patch set in which the second commit gets rid of the
ioctl -- but not only for mlx5 but for all vDPA implementations.
>
> Thanks
>
Thanks, Carlos
Hello,
On 8/29/24 4:07 PM, Dragos Tatulea wrote:
> (resending as I accidentally replied only to Carlos)
>
> On 29.08.24 18:16, Carlos Bilbao wrote:
>> From: Carlos Bilbao <cbilbao@digitalocean.com>
>>
>> Include support to update the vDPA configuration fields of speed and
>> duplex (as needed by VHOST_VDPA_SET_CONFIG). This includes function
>> mlx5_vdpa_set_config() as well as changes in vdpa.c to fill the initial
>> values to UNKNOWN. Also add a warning message for when
>> mlx5_vdpa_get_config() receives offset and length out of bounds.
>>
>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
>> ---
>> drivers/vdpa/mlx5/net/mlx5_vnet.c | 34 ++++++++++++++++++++++++++++++-
>> drivers/vdpa/vdpa.c | 27 ++++++++++++++++++++++++
>> include/uapi/linux/vdpa.h | 2 ++
>> 3 files changed, 62 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index c47009a8b472..a44bb2072eec 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -3221,12 +3221,44 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>>
>> if (offset + len <= sizeof(struct virtio_net_config))
>> memcpy(buf, (u8 *)&ndev->config + offset, len);
>> + else
>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>> }
>>
>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>> unsigned int len)
>> {
>> - /* not supported */
>> + struct mlx5_vdpa_dev *mvdev = to_mvdev(vdev);
>> + struct mlx5_vdpa_net *ndev = to_mlx5_vdpa_ndev(mvdev);
>> +
>> + if (offset + len > sizeof(struct virtio_net_config)) {
>> + mlx5_vdpa_warn(mvdev, "Offset and length out of bounds\n");
>> + return;
>> + }
>> +
>> + /*
>> + * Note that this will update the speed/duplex configuration fields
>> + * but the hardware support to actually perform this change does
>> + * not exist yet.
>> + */
>> + switch (offset) {
>> + case offsetof(struct virtio_net_config, speed):
>> + if (len == sizeof(((struct virtio_net_config *) 0)->speed))
>> + memcpy(&ndev->config.speed, buf, len);
>> + else
>> + mlx5_vdpa_warn(mvdev, "Invalid length for speed.\n");
>> + break;
>> +
>> + case offsetof(struct virtio_net_config, duplex):
>> + if (len == sizeof(((struct virtio_net_config *)0)->duplex))
>> + memcpy(&ndev->config.duplex, buf, len);
>> + else
>> + mlx5_vdpa_warn(mvdev, "Invalid length for duplex.\n");
>> + break;
>> +
>> + default:
>> + mlx5_vdpa_warn(mvdev, "Configuration field not supported.\n");
> This will trigger noise in dmesg because there is a MAC configuration here.
>> + }
> I would prefer that the .set_config remains a stub TBH. Setting the fields here is
> misleading: the user might deduce that the configuration worked when they read the
> values and see that they were updated.
Well, people might already assume that the values are updated because there
is an ioctl available to user space (VHOST_VDPA_SET_CONFIG) that doesn't
warn or return an error - or at least I did.
But, I understand your concern. I propose that we at least return an error
in the ioctl if the requested config updated is not supported. We could
also structure this patch so that it can be used when or if hardware
support becomes available in the future.
Is there a way to query the hardware capabilities of the card, e.g., MSRs
or other methods? Do you recommend any technical manual?
Or, if the ioctl uses, for example, /dev/vhost-vdpa-2, does its speed and
duplex settings correspond to those of the tap device it links to? This
information can be checked and changed using the definitions in
include/uapi/linux/ethtool.h.
Thank you for taking the time to read and answer me.
>
> Thanks,
> dragos
>> }
>>
>> static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>> diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c
>> index 4dbd2e55a288..b920e4405f6d 100644
>> --- a/drivers/vdpa/vdpa.c
>> +++ b/drivers/vdpa/vdpa.c
>> @@ -15,6 +15,7 @@
>> #include <net/genetlink.h>
>> #include <linux/mod_devicetable.h>
>> #include <linux/virtio_ids.h>
>> +#include <uapi/linux/ethtool.h>
>>
>> static LIST_HEAD(mdev_head);
>> /* A global mutex that protects vdpa management device and device level operations. */
>> @@ -919,6 +920,22 @@ static int vdpa_dev_net_status_config_fill(struct sk_buff *msg, u64 features,
>> return nla_put_u16(msg, VDPA_ATTR_DEV_NET_STATUS, val_u16);
>> }
>>
>> +static int vdpa_dev_net_speed_config_fill(struct sk_buff *msg, u64 features,
>> + struct virtio_net_config *config)
>> +{
>> + __le32 speed = cpu_to_le32(SPEED_UNKNOWN);
>> +
>> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_SPEED, sizeof(speed), &speed);
>> +}
>> +
>> +static int vdpa_dev_net_duplex_config_fill(struct sk_buff *msg, u64 features,
>> + struct virtio_net_config *config)
>> +{
>> + u8 duplex = DUPLEX_UNKNOWN;
>> +
>> + return nla_put(msg, VDPA_ATTR_DEV_NET_CFG_DUPLEX, sizeof(duplex), &duplex);
>> +}
>> +
>> static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *msg)
>> {
>> struct virtio_net_config config = {};
>> @@ -940,6 +957,16 @@ static int vdpa_dev_net_config_fill(struct vdpa_device *vdev, struct sk_buff *ms
>>
>> if (vdpa_dev_net_status_config_fill(msg, features_device, &config))
>> return -EMSGSIZE;
>> + /*
>> + * mlx5_vdpa vDPA devicess currently do not support the
>> + * VIRTIO_NET_F_SPEED_DUPLEX feature, which reports speed and
>> + * duplex; hence these are set to UNKNOWN for now.
>> + */
>> + if (vdpa_dev_net_speed_config_fill(msg, features_device, &config))
>> + return -EMSGSIZE;
>> +
>> + if (vdpa_dev_net_duplex_config_fill(msg, features_device, &config))
>> + return -EMSGSIZE;
>>
>> return vdpa_dev_net_mq_config_fill(msg, features_device, &config);
>> }
>> diff --git a/include/uapi/linux/vdpa.h b/include/uapi/linux/vdpa.h
>> index 842bf1201ac4..1c64ee0dd7b1 100644
>> --- a/include/uapi/linux/vdpa.h
>> +++ b/include/uapi/linux/vdpa.h
>> @@ -43,6 +43,8 @@ enum vdpa_attr {
>> VDPA_ATTR_DEV_NET_STATUS, /* u8 */
>> VDPA_ATTR_DEV_NET_CFG_MAX_VQP, /* u16 */
>> VDPA_ATTR_DEV_NET_CFG_MTU, /* u16 */
>> + VDPA_ATTR_DEV_NET_CFG_SPEED, /* u32 */
>> + VDPA_ATTR_DEV_NET_CFG_DUPLEX, /* u8 */
>>
>> VDPA_ATTR_DEV_NEGOTIATED_FEATURES, /* u64 */
>> VDPA_ATTR_DEV_MGMTDEV_MAX_VQS, /* u32 */
Thanks, Carlos
© 2016 - 2026 Red Hat, Inc.