On Fri, Aug 30, 2024 at 9:15 PM Carlos Bilbao <cbil...@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 <dtatu...@nvidia.com> wrote:
> >> (resending as I accidentally replied only to Carlos)
> >>
> >> On 29.08.24 18:16, Carlos Bilbao wrote:
> >>> From: Carlos Bilbao <cbil...@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 <cbil...@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
