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@xxxxxxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxxxxxx>
>> ---
>> 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
