linux-kernel - Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in vDPA/mlx5

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c383c897-79ce-43fc-966a-06a4d609390a@digitalocean.com>
Date: Tue, 3 Sep 2024 11:01:41 -0500
From: Carlos Bilbao <cbilbao@...italocean.com>
To: Jason Wang <jasowang@...hat.com>
Cc: Dragos Tatulea <dtatulea@...dia.com>,
 Carlos Bilbao <carlos.bilbao.osdev@...il.com>, mst@...hat.com,
 bilbao@...edu, xuanzhuo@...ux.alibaba.com, eperezma@...hat.com,
 cratiu@...dia.com, lingshan.zhu@...el.com, virtualization@...ts.linux.dev,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] vdpa: Add support to update speed/duplex in
 vDPA/mlx5_vnet

Hello,

On 9/1/24 11:27 PM, Jason Wang wrote:
> On Fri, Aug 30, 2024 at 9:15 PM Carlos Bilbao <cbilbao@...italocean.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@...dia.com> wrote:
>>>> (resending as I accidentally replied only to Carlos)
>>>>
>>>> On 29.08.24 18:16, Carlos Bilbao wrote:
>>>>> From: Carlos Bilbao <cbilbao@...italocean.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@...italocean.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