[PATCH 0/2] Initialize vDPA speed/duplex and support their updates

[PATCH 0/2] Initialize vDPA speed/duplex and support their updates

[Carlos Bilbao]

From: Carlos Bilbao <cbilbao@digitalocean.com>

Initialize speed and duplex for virtio_net_config to UNKNOWN (mlx5_vdpa
vDPA devices currently do not support VIRTIO_NET_F_SPEED_DUPLEX). Include
logic to update these fields (for VHOST_VDPA_SET_CONFIG) -- even if
hardware support is not capable yet (Make a note of that). Also add warning
messages for out of bounds errors in mlx5_vnet get/set_config logic.

Note: You can test these changes from user space passing a fd of your
file "/dev/vhost-vdpa-%d" to:

void check_config_speed_duplex(int fd) {

    uint8_t *buf;
    uint32_t size;
    struct vhost_vdpa_config *config;
    struct virtio_net_config *net_config;

    if (ioctl(fd, VHOST_VDPA_GET_CONFIG_SIZE, &size) < 0) {
        perror("ioctl failed");
        return;
    }

    config = malloc(sizeof(struct vhost_vdpa_config) + size);

    if (!config) {
        perror("malloc failed");
        return;
    }

    memset(config, 0, sizeof(struct vhost_vdpa_config) + size);
    config->len = size;
    config->off = 0;

    buf = config->buf;

    if (ioctl(fd, VHOST_VDPA_GET_CONFIG, config) < 0) {
        perror("ioctl failed");
    }
    else {
        net_config = (struct virtio_net_config *)buf;

        printf("  Speed: %u Mb\n", net_config->speed);

	if (net_config->duplex == 0)
        	printf("  Half Duplex\n);
	else if (net_config->duplex == 1)
		printf("  Full Duplex\n");
	else
		printf("  Unknown Duplex\n");
    }

    free(config);
}

Carlos Bilbao:
  mlx5_vnet: Set speed and duplex of vDPA devices to UNKNOWN
  vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 46 ++++++++++++++++++++++++++++++-
 drivers/vdpa/vdpa.c               | 27 ++++++++++++++++++
 include/uapi/linux/vdpa.h         |  2 ++
 3 files changed, 74 insertions(+), 1 deletion(-)

[PATCH 1/2] mlx5_vnet: Set speed and duplex of vDPA devices to UNKNOWN

[Carlos Bilbao]

From: Carlos Bilbao <cbilbao@digitalocean.com>

Initialize the speed and duplex fields in virtio_net_config to UNKNOWN.
This is needed because mlx5_vdpa vDPA devicess currently do not support the
VIRTIO_NET_F_SPEED_DUPLEX feature which reports speed and duplex. Add
needed helper cpu_to_mlx5vdpa32() to convert endianness of speed.

Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
---
 drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index fa78e8288ebb..c47009a8b472 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -193,6 +193,11 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
 	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
 }
 
+static __virtio32 cpu_to_mlx5vdpa32(struct mlx5_vdpa_dev *mvdev, u32 val)
+{
+	return __cpu_to_virtio32(mlx5_vdpa_is_little_endian(mvdev), val);
+}
+
 static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
 {
 	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
@@ -3795,6 +3800,13 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
 	init_rwsem(&ndev->reslock);
 	config = &ndev->config;
 
+	/*
+	 * mlx5_vdpa vDPA devices currently don't support reporting or
+	 * setting the speed or duplex.
+	 */
+	config->speed  = cpu_to_mlx5vdpa32(mvdev, SPEED_UNKNOWN);
+	config->duplex = DUPLEX_UNKNOWN;
+
 	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
 		err = config_func_mtu(mdev, add_config->net.mtu);
 		if (err)
-- 
2.34.1

Re: [PATCH 1/2] mlx5_vnet: Set speed and duplex of vDPA devices to UNKNOWN

[Dragos Tatulea]

On 29.08.24 18:16, Carlos Bilbao wrote:
> From: Carlos Bilbao <cbilbao@digitalocean.com>
> 
> Initialize the speed and duplex fields in virtio_net_config to UNKNOWN.
> This is needed because mlx5_vdpa vDPA devicess currently do not support the
> VIRTIO_NET_F_SPEED_DUPLEX feature which reports speed and duplex. Add
> needed helper cpu_to_mlx5vdpa32() to convert endianness of speed.
> 
> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
Nit: prefix is vdpa/mlx5. Once that is fixed, for this patch:
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>

> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index fa78e8288ebb..c47009a8b472 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -193,6 +193,11 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>  }
>  
> +static __virtio32 cpu_to_mlx5vdpa32(struct mlx5_vdpa_dev *mvdev, u32 val)
> +{
> +	return __cpu_to_virtio32(mlx5_vdpa_is_little_endian(mvdev), val);
> +}
> +
>  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>  {
>  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
> @@ -3795,6 +3800,13 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>  	init_rwsem(&ndev->reslock);
>  	config = &ndev->config;
>  
> +	/*
> +	 * mlx5_vdpa vDPA devices currently don't support reporting or
> +	 * setting the speed or duplex.
> +	 */
> +	config->speed  = cpu_to_mlx5vdpa32(mvdev, SPEED_UNKNOWN);
> +	config->duplex = DUPLEX_UNKNOWN;
> +
>  	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
>  		err = config_func_mtu(mdev, add_config->net.mtu);
>  		if (err)

Re: [PATCH 1/2] mlx5_vnet: Set speed and duplex of vDPA devices to UNKNOWN

[Carlos Bilbao]

Hello,

On 8/29/24 1:44 PM, Dragos Tatulea wrote:
>
> On 29.08.24 18:16, Carlos Bilbao wrote:
>> From: Carlos Bilbao <cbilbao@digitalocean.com>
>>
>> Initialize the speed and duplex fields in virtio_net_config to UNKNOWN.
>> This is needed because mlx5_vdpa vDPA devicess currently do not support the
>> VIRTIO_NET_F_SPEED_DUPLEX feature which reports speed and duplex. Add
>> needed helper cpu_to_mlx5vdpa32() to convert endianness of speed.
>>
>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
> Nit: prefix is vdpa/mlx5. Once that is fixed, for this patch:
> Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>


Thank you, will keep that in mind to fix in v2.


>
>> ---
>>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index fa78e8288ebb..c47009a8b472 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -193,6 +193,11 @@ static __virtio16 cpu_to_mlx5vdpa16(struct mlx5_vdpa_dev *mvdev, u16 val)
>>  	return __cpu_to_virtio16(mlx5_vdpa_is_little_endian(mvdev), val);
>>  }
>>  
>> +static __virtio32 cpu_to_mlx5vdpa32(struct mlx5_vdpa_dev *mvdev, u32 val)
>> +{
>> +	return __cpu_to_virtio32(mlx5_vdpa_is_little_endian(mvdev), val);
>> +}
>> +
>>  static u16 ctrl_vq_idx(struct mlx5_vdpa_dev *mvdev)
>>  {
>>  	if (!(mvdev->actual_features & BIT_ULL(VIRTIO_NET_F_MQ)))
>> @@ -3795,6 +3800,13 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name,
>>  	init_rwsem(&ndev->reslock);
>>  	config = &ndev->config;
>>  
>> +	/*
>> +	 * mlx5_vdpa vDPA devices currently don't support reporting or
>> +	 * setting the speed or duplex.
>> +	 */
>> +	config->speed  = cpu_to_mlx5vdpa32(mvdev, SPEED_UNKNOWN);
>> +	config->duplex = DUPLEX_UNKNOWN;
>> +
>>  	if (add_config->mask & BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU)) {
>>  		err = config_func_mtu(mdev, add_config->net.mtu);
>>  		if (err)


Thanks, Carlos

[PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Carlos Bilbao]

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

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Dragos Tatulea]

(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 */

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Carlos Bilbao]

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

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Jason Wang]

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 */
>

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Carlos Bilbao]

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

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Jason Wang]

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

Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5_vnet

[Carlos Bilbao]

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