[RFC] Why is set_config not supported in mlx5_vnet?

[RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello,

I'm debugging my vDPA setup, and when using ioctl to retrieve the
configuration, I noticed that it's running in half duplex mode:

Configuration data (24 bytes):
  MAC address: (Mac address)
  Status: 0x0001
  Max virtqueue pairs: 8
  MTU: 1500
  Speed: 0 Mb
  Duplex: Half Duplex
  RSS max key size: 0
  RSS max indirection table length: 0
  Supported hash types: 0x00000000

I believe this might be contributing to the underperformance of vDPA. While
looking into how to change this option for Mellanox, I read the following
kernel code in mlx5_vnet.c:

static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
                 unsigned int len)
{
    /* not supported */
}

I was wondering why this is the case. Is there another way for me to change
these configuration settings?

Thank you in advance,
Carlos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Andrew Lunn]

On Fri, Aug 23, 2024 at 11:54:13AM -0500, Carlos Bilbao wrote:
> Hello,
> 
> I'm debugging my vDPA setup, and when using ioctl to retrieve the
> configuration, I noticed that it's running in half duplex mode:
> 
> Configuration data (24 bytes):
>   MAC address: (Mac address)
>   Status: 0x0001
>   Max virtqueue pairs: 8
>   MTU: 1500
>   Speed: 0 Mb
>   Duplex: Half Duplex

If the speed is 0, does duplex even matter?

	Andrew

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Dragos Tatulea]

On 23.08.24 18:54, Carlos Bilbao wrote:
> Hello,
> 
> I'm debugging my vDPA setup, and when using ioctl to retrieve the
> configuration, I noticed that it's running in half duplex mode:
> 
> Configuration data (24 bytes):
>   MAC address: (Mac address)
>   Status: 0x0001
>   Max virtqueue pairs: 8
>   MTU: 1500
>   Speed: 0 Mb
>   Duplex: Half Duplex
>   RSS max key size: 0
>   RSS max indirection table length: 0
>   Supported hash types: 0x00000000
> 
> I believe this might be contributing to the underperformance of vDPA.
mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
feature which reports speed and duplex. You can check the state on the
PF.


> While looking into how to change this option for Mellanox, I read the following
> kernel code in mlx5_vnet.c:
> 
> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>                  unsigned int len)
> {
>     /* not supported */
> }
> 
> I was wondering why this is the case.
TBH, I don't know why it was not added. But in general, the control VQ is the
better way as it's dynamic.

> Is there another way for me to change
> these configuration settings?
> 
The configuration is done using control VQ for most things (MTU, MAC, VQs,
etc). Make sure that you have the CTRL_VQ feature set (should be on by
default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
show`.

Thanks,
Dragos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Andrew Lunn]

On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
> 
> 
> On 23.08.24 18:54, Carlos Bilbao wrote:
> > Hello,
> > 
> > I'm debugging my vDPA setup, and when using ioctl to retrieve the
> > configuration, I noticed that it's running in half duplex mode:
> > 
> > Configuration data (24 bytes):
> >   MAC address: (Mac address)
> >   Status: 0x0001
> >   Max virtqueue pairs: 8
> >   MTU: 1500
> >   Speed: 0 Mb
> >   Duplex: Half Duplex
> >   RSS max key size: 0
> >   RSS max indirection table length: 0
> >   Supported hash types: 0x00000000
> > 
> > I believe this might be contributing to the underperformance of vDPA.
> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
> feature which reports speed and duplex. You can check the state on the
> PF.

Then it should probably report DUPLEX_UNKNOWN.

The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
this just looks buggy in general.

     Andrew

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello Dragos,

On 8/26/24 4:06 AM, Dragos Tatulea wrote:
>
> On 23.08.24 18:54, Carlos Bilbao wrote:
>> Hello,
>>
>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>> configuration, I noticed that it's running in half duplex mode:
>>
>> Configuration data (24 bytes):
>>   MAC address: (Mac address)
>>   Status: 0x0001
>>   Max virtqueue pairs: 8
>>   MTU: 1500
>>   Speed: 0 Mb
>>   Duplex: Half Duplex
>>   RSS max key size: 0
>>   RSS max indirection table length: 0
>>   Supported hash types: 0x00000000
>>
>> I believe this might be contributing to the underperformance of vDPA.
> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
> feature which reports speed and duplex. You can check the state on the
> PF.


According to ethtool, all my devices are running at full duplex. I assume I
can disregard this configuration output from the module then.


>
>> While looking into how to change this option for Mellanox, I read the following
>> kernel code in mlx5_vnet.c:
>>
>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>                  unsigned int len)
>> {
>>     /* not supported */
>> }
>>
>> I was wondering why this is the case.
> TBH, I don't know why it was not added. But in general, the control VQ is the
> better way as it's dynamic.
>
>> Is there another way for me to change
>> these configuration settings?
>>
> The configuration is done using control VQ for most things (MTU, MAC, VQs,
> etc). Make sure that you have the CTRL_VQ feature set (should be on by
> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
> show`.


I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
use the control VQ to get/set vDPA configuration values?


>
> Thanks,
> Dragos


Thank you!
Carlos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Dragos Tatulea]

On 26.08.24 16:26, Carlos Bilbao wrote:
> Hello Dragos,
> 
> On 8/26/24 4:06 AM, Dragos Tatulea wrote:
>>
>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>> Hello,
>>>
>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>> configuration, I noticed that it's running in half duplex mode:
>>>
>>> Configuration data (24 bytes):
>>>   MAC address: (Mac address)
>>>   Status: 0x0001
>>>   Max virtqueue pairs: 8
>>>   MTU: 1500
>>>   Speed: 0 Mb
>>>   Duplex: Half Duplex
>>>   RSS max key size: 0
>>>   RSS max indirection table length: 0
>>>   Supported hash types: 0x00000000
>>>
>>> I believe this might be contributing to the underperformance of vDPA.
>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>> feature which reports speed and duplex. You can check the state on the
>> PF.
> 
> 
> According to ethtool, all my devices are running at full duplex. I assume I
> can disregard this configuration output from the module then.
> 
Yep.

> 
>>
>>> While looking into how to change this option for Mellanox, I read the following
>>> kernel code in mlx5_vnet.c:
>>>
>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>>                  unsigned int len)
>>> {
>>>     /* not supported */
>>> }
>>>
>>> I was wondering why this is the case.
>> TBH, I don't know why it was not added. But in general, the control VQ is the
>> better way as it's dynamic.
>>
>>> Is there another way for me to change
>>> these configuration settings?
>>>
>> The configuration is done using control VQ for most things (MTU, MAC, VQs,
>> etc). Make sure that you have the CTRL_VQ feature set (should be on by
>> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
>> show`.
> 
> 
> I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
> use the control VQ to get/set vDPA configuration values?
> 
>
You are most likely using it already through through qemu. You can check
if the CTR_VQ feature also shows up in the output of `vdpa dev config show`.

What values are you trying to configure btw?

Thanks,
Dragos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Dragos Tatulea]

On 26.08.24 16:24, Andrew Lunn wrote:
> On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
>>
>>
>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>> Hello,
>>>
>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>> configuration, I noticed that it's running in half duplex mode:
>>>
>>> Configuration data (24 bytes):
>>>   MAC address: (Mac address)
>>>   Status: 0x0001
>>>   Max virtqueue pairs: 8
>>>   MTU: 1500
>>>   Speed: 0 Mb
>>>   Duplex: Half Duplex
>>>   RSS max key size: 0
>>>   RSS max indirection table length: 0
>>>   Supported hash types: 0x00000000
>>>
>>> I believe this might be contributing to the underperformance of vDPA.
>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>> feature which reports speed and duplex. You can check the state on the
>> PF.
> 
> Then it should probably report DUPLEX_UNKNOWN.
> 
> The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
> this just looks buggy in general.
>
The virtio spec doesn't mention what those values should be when
VIRTIO_NET_F_SPEED_DUPLEX is not supported.

Jason, should vdpa_dev_net_config_fill() initialize the speed/duplex
fields to SPEED/DUPLEX_UNKNOWN instead of 0?

Thanks,
Dragos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello,

On 8/26/24 10:53 AM, Dragos Tatulea wrote:
>
> On 26.08.24 16:26, Carlos Bilbao wrote:
>> Hello Dragos,
>>
>> On 8/26/24 4:06 AM, Dragos Tatulea wrote:
>>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>>> Hello,
>>>>
>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>>> configuration, I noticed that it's running in half duplex mode:
>>>>
>>>> Configuration data (24 bytes):
>>>>   MAC address: (Mac address)
>>>>   Status: 0x0001
>>>>   Max virtqueue pairs: 8
>>>>   MTU: 1500
>>>>   Speed: 0 Mb
>>>>   Duplex: Half Duplex
>>>>   RSS max key size: 0
>>>>   RSS max indirection table length: 0
>>>>   Supported hash types: 0x00000000
>>>>
>>>> I believe this might be contributing to the underperformance of vDPA.
>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>>> feature which reports speed and duplex. You can check the state on the
>>> PF.
>>
>> According to ethtool, all my devices are running at full duplex. I assume I
>> can disregard this configuration output from the module then.
>>
> Yep.
>
>>>> While looking into how to change this option for Mellanox, I read the following
>>>> kernel code in mlx5_vnet.c:
>>>>
>>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>>>                  unsigned int len)
>>>> {
>>>>     /* not supported */
>>>> }
>>>>
>>>> I was wondering why this is the case.
>>> TBH, I don't know why it was not added. But in general, the control VQ is the
>>> better way as it's dynamic.
>>>
>>>> Is there another way for me to change
>>>> these configuration settings?
>>>>
>>> The configuration is done using control VQ for most things (MTU, MAC, VQs,
>>> etc). Make sure that you have the CTRL_VQ feature set (should be on by
>>> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
>>> show`.
>>
>> I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
>> use the control VQ to get/set vDPA configuration values?
>>
>>
> You are most likely using it already through through qemu. You can check
> if the CTR_VQ feature also shows up in the output of `vdpa dev config show`.
>
> What values are you trying to configure btw?


Yes, CTRL_VQ also shows up in vdpa dev config show. There isn't a specific
value I want to configure ATM, but my vDPA isn't performing as expected, so
I'm investigating potential issues. Below is the code I used to retrieve
the configuration from the driver; I'd be happy to send it as a patch if
you or someone else reviews it.


>
> Thanks,
> Dragos


Thanks,
Carlos

---

From ab6ea66c926eaf1e95eb5d73bc23183e0021ee27 Mon Sep 17 00:00:00 2001
From: Carlos Bilbao <bilbao@vt.edu>
Date: Sat, 24 Aug 2024 00:24:56 +0000
Subject: [PATCH] mlx5: Add support to update the vDPA configuration

This is needed for VHOST_VDPA_SET_CONFIG.

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

diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
index b56aae3f7be3..da31c743b2b9 100644
--- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
+++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
@@ -2909,14 +2909,32 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
     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))
+    if (offset + len <= sizeof(struct virtio_net_config)) {
         memcpy(buf, (u8 *)&ndev->config + offset, len);
+        }
+        else
+        {
+            printk(KERN_ERR "%s: Offset and length out of bounds\n",
+            __func__);
+        }
+
 }

 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))
+    {
+        memcpy((u8 *)&ndev->config + offset, buf, len);
+    }
+    else
+    {
+        printk(KERN_ERR "%s: Offset and length out of bounds\n",
+        __func__);
+    }
 }

 static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
--
2.34.1

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Jason Wang]

On Tue, Aug 27, 2024 at 12:11 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
>
> On 26.08.24 16:24, Andrew Lunn wrote:
> > On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
> >>
> >>
> >> On 23.08.24 18:54, Carlos Bilbao wrote:
> >>> Hello,
> >>>
> >>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
> >>> configuration, I noticed that it's running in half duplex mode:
> >>>
> >>> Configuration data (24 bytes):
> >>>   MAC address: (Mac address)
> >>>   Status: 0x0001
> >>>   Max virtqueue pairs: 8
> >>>   MTU: 1500
> >>>   Speed: 0 Mb
> >>>   Duplex: Half Duplex
> >>>   RSS max key size: 0
> >>>   RSS max indirection table length: 0
> >>>   Supported hash types: 0x00000000
> >>>
> >>> I believe this might be contributing to the underperformance of vDPA.
> >> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
> >> feature which reports speed and duplex. You can check the state on the
> >> PF.
> >
> > Then it should probably report DUPLEX_UNKNOWN.
> >
> > The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
> > this just looks buggy in general.
> >
> The virtio spec doesn't mention what those values should be when
> VIRTIO_NET_F_SPEED_DUPLEX is not supported.
>
> Jason, should vdpa_dev_net_config_fill() initialize the speed/duplex
> fields to SPEED/DUPLEX_UNKNOWN instead of 0?

Spec said

"""
The following two fields, speed and duplex, only exist if
VIRTIO_NET_F_SPEED_DUPLEX is set.
"""

So my understanding is that it is undefined behaviour, and those
fields seems useless before feature negotiation. For safety, it might
be better to initialize them as UNKOWN.

Thanks


>
> Thanks,
> Dragos
>

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Jason Wang]

On Tue, Aug 27, 2024 at 3:23 AM Carlos Bilbao <cbilbao@digitalocean.com> wrote:
>
> Hello,
>
> On 8/26/24 10:53 AM, Dragos Tatulea wrote:
> >
> > On 26.08.24 16:26, Carlos Bilbao wrote:
> >> Hello Dragos,
> >>
> >> On 8/26/24 4:06 AM, Dragos Tatulea wrote:
> >>> On 23.08.24 18:54, Carlos Bilbao wrote:
> >>>> Hello,
> >>>>
> >>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
> >>>> configuration, I noticed that it's running in half duplex mode:
> >>>>
> >>>> Configuration data (24 bytes):
> >>>>   MAC address: (Mac address)
> >>>>   Status: 0x0001
> >>>>   Max virtqueue pairs: 8
> >>>>   MTU: 1500
> >>>>   Speed: 0 Mb
> >>>>   Duplex: Half Duplex
> >>>>   RSS max key size: 0
> >>>>   RSS max indirection table length: 0
> >>>>   Supported hash types: 0x00000000
> >>>>
> >>>> I believe this might be contributing to the underperformance of vDPA.
> >>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
> >>> feature which reports speed and duplex. You can check the state on the
> >>> PF.
> >>
> >> According to ethtool, all my devices are running at full duplex. I assume I
> >> can disregard this configuration output from the module then.
> >>
> > Yep.
> >
> >>>> While looking into how to change this option for Mellanox, I read the following
> >>>> kernel code in mlx5_vnet.c:
> >>>>
> >>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
> >>>>                  unsigned int len)
> >>>> {
> >>>>     /* not supported */
> >>>> }
> >>>>
> >>>> I was wondering why this is the case.
> >>> TBH, I don't know why it was not added. But in general, the control VQ is the
> >>> better way as it's dynamic.
> >>>
> >>>> Is there another way for me to change
> >>>> these configuration settings?
> >>>>
> >>> The configuration is done using control VQ for most things (MTU, MAC, VQs,
> >>> etc). Make sure that you have the CTRL_VQ feature set (should be on by
> >>> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
> >>> show`.
> >>
> >> I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
> >> use the control VQ to get/set vDPA configuration values?
> >>
> >>
> > You are most likely using it already through through qemu. You can check
> > if the CTR_VQ feature also shows up in the output of `vdpa dev config show`.
> >
> > What values are you trying to configure btw?
>
>
> Yes, CTRL_VQ also shows up in vdpa dev config show. There isn't a specific
> value I want to configure ATM, but my vDPA isn't performing as expected, so
> I'm investigating potential issues. Below is the code I used to retrieve
> the configuration from the driver; I'd be happy to send it as a patch if
> you or someone else reviews it.
>
>
> >
> > Thanks,
> > Dragos
>
>
> Thanks,
> Carlos
>
> ---
>
> From ab6ea66c926eaf1e95eb5d73bc23183e0021ee27 Mon Sep 17 00:00:00 2001
> From: Carlos Bilbao <bilbao@vt.edu>
> Date: Sat, 24 Aug 2024 00:24:56 +0000
> Subject: [PATCH] mlx5: Add support to update the vDPA configuration
>
> This is needed for VHOST_VDPA_SET_CONFIG.
>
> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
> ---
>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> index b56aae3f7be3..da31c743b2b9 100644
> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
> @@ -2909,14 +2909,32 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>      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))
> +    if (offset + len <= sizeof(struct virtio_net_config)) {
>          memcpy(buf, (u8 *)&ndev->config + offset, len);
> +        }
> +        else
> +        {
> +            printk(KERN_ERR "%s: Offset and length out of bounds\n",
> +            __func__);
> +        }
> +
>  }
>
>  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))
> +    {
> +        memcpy((u8 *)&ndev->config + offset, buf, len);
> +    }
> +    else
> +    {
> +        printk(KERN_ERR "%s: Offset and length out of bounds\n",
> +        __func__);
> +    }
>  }

This should follow the virtio-spec, for modern virtio-net devices,
most of the fields are read only.

Thanks

>
>  static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
> --
> 2.34.1
>
>

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Dragos Tatulea]

On 27.08.24 04:03, Jason Wang wrote:
> On Tue, Aug 27, 2024 at 12:11 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>
>>
>> On 26.08.24 16:24, Andrew Lunn wrote:
>>> On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
>>>>
>>>>
>>>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>>>> Hello,
>>>>>
>>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>>>> configuration, I noticed that it's running in half duplex mode:
>>>>>
>>>>> Configuration data (24 bytes):
>>>>>   MAC address: (Mac address)
>>>>>   Status: 0x0001
>>>>>   Max virtqueue pairs: 8
>>>>>   MTU: 1500
>>>>>   Speed: 0 Mb
>>>>>   Duplex: Half Duplex
>>>>>   RSS max key size: 0
>>>>>   RSS max indirection table length: 0
>>>>>   Supported hash types: 0x00000000
>>>>>
>>>>> I believe this might be contributing to the underperformance of vDPA.
>>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>>>> feature which reports speed and duplex. You can check the state on the
>>>> PF.
>>>
>>> Then it should probably report DUPLEX_UNKNOWN.
>>>
>>> The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
>>> this just looks buggy in general.
>>>
>> The virtio spec doesn't mention what those values should be when
>> VIRTIO_NET_F_SPEED_DUPLEX is not supported.
>>
>> Jason, should vdpa_dev_net_config_fill() initialize the speed/duplex
>> fields to SPEED/DUPLEX_UNKNOWN instead of 0?
> 
> Spec said
> 
> """
> The following two fields, speed and duplex, only exist if
> VIRTIO_NET_F_SPEED_DUPLEX is set.
> """
> 
> So my understanding is that it is undefined behaviour, and those
> fields seems useless before feature negotiation. For safety, it might
> be better to initialize them as UNKOWN.
> 
After a closer look my statement doesn't make sense: the device will copy
the virtio_net_config bytes on top.

The solution is to initialize these fields to UNKNOWN in the driver. Will send
a patch to fix this.

Thanks,
Dragos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello,

On 8/26/24 9:07 PM, Jason Wang wrote:
> On Tue, Aug 27, 2024 at 3:23 AM Carlos Bilbao <cbilbao@digitalocean.com> wrote:
>> Hello,
>>
>> On 8/26/24 10:53 AM, Dragos Tatulea wrote:
>>> On 26.08.24 16:26, Carlos Bilbao wrote:
>>>> Hello Dragos,
>>>>
>>>> On 8/26/24 4:06 AM, Dragos Tatulea wrote:
>>>>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>>>>> configuration, I noticed that it's running in half duplex mode:
>>>>>>
>>>>>> Configuration data (24 bytes):
>>>>>>   MAC address: (Mac address)
>>>>>>   Status: 0x0001
>>>>>>   Max virtqueue pairs: 8
>>>>>>   MTU: 1500
>>>>>>   Speed: 0 Mb
>>>>>>   Duplex: Half Duplex
>>>>>>   RSS max key size: 0
>>>>>>   RSS max indirection table length: 0
>>>>>>   Supported hash types: 0x00000000
>>>>>>
>>>>>> I believe this might be contributing to the underperformance of vDPA.
>>>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>>>>> feature which reports speed and duplex. You can check the state on the
>>>>> PF.
>>>> According to ethtool, all my devices are running at full duplex. I assume I
>>>> can disregard this configuration output from the module then.
>>>>
>>> Yep.
>>>
>>>>>> While looking into how to change this option for Mellanox, I read the following
>>>>>> kernel code in mlx5_vnet.c:
>>>>>>
>>>>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>>>>>                  unsigned int len)
>>>>>> {
>>>>>>     /* not supported */
>>>>>> }
>>>>>>
>>>>>> I was wondering why this is the case.
>>>>> TBH, I don't know why it was not added. But in general, the control VQ is the
>>>>> better way as it's dynamic.
>>>>>
>>>>>> Is there another way for me to change
>>>>>> these configuration settings?
>>>>>>
>>>>> The configuration is done using control VQ for most things (MTU, MAC, VQs,
>>>>> etc). Make sure that you have the CTRL_VQ feature set (should be on by
>>>>> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
>>>>> show`.
>>>> I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
>>>> use the control VQ to get/set vDPA configuration values?
>>>>
>>>>
>>> You are most likely using it already through through qemu. You can check
>>> if the CTR_VQ feature also shows up in the output of `vdpa dev config show`.
>>>
>>> What values are you trying to configure btw?
>>
>> Yes, CTRL_VQ also shows up in vdpa dev config show. There isn't a specific
>> value I want to configure ATM, but my vDPA isn't performing as expected, so
>> I'm investigating potential issues. Below is the code I used to retrieve
>> the configuration from the driver; I'd be happy to send it as a patch if
>> you or someone else reviews it.
>>
>>
>>> Thanks,
>>> Dragos
>>
>> Thanks,
>> Carlos
>>
>> ---
>>
>> From ab6ea66c926eaf1e95eb5d73bc23183e0021ee27 Mon Sep 17 00:00:00 2001
>> From: Carlos Bilbao <bilbao@vt.edu>
>> Date: Sat, 24 Aug 2024 00:24:56 +0000
>> Subject: [PATCH] mlx5: Add support to update the vDPA configuration
>>
>> This is needed for VHOST_VDPA_SET_CONFIG.
>>
>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
>> ---
>>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b56aae3f7be3..da31c743b2b9 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -2909,14 +2909,32 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>>      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))
>> +    if (offset + len <= sizeof(struct virtio_net_config)) {
>>          memcpy(buf, (u8 *)&ndev->config + offset, len);
>> +        }
>> +        else
>> +        {
>> +            printk(KERN_ERR "%s: Offset and length out of bounds\n",
>> +            __func__);
>> +        }
>> +
>>  }
>>
>>  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))
>> +    {
>> +        memcpy((u8 *)&ndev->config + offset, buf, len);
>> +    }
>> +    else
>> +    {
>> +        printk(KERN_ERR "%s: Offset and length out of bounds\n",
>> +        __func__);
>> +    }
>>  }
> This should follow the virtio-spec, for modern virtio-net devices,
> most of the fields are read only.


Ack, according to:

https://docs.oasis-open.org/virtio/virtio/v1.2/csd01/virtio-v1.2-csd01.html

I believe only duplex and speed can be changed. Will resend patch.


> Thanks
>
>>  static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>> --
>> 2.34.1
>>
>>

Thanks,
Carlos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Jason Wang]

On Wed, Aug 28, 2024 at 12:55 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>
>
>
> On 27.08.24 04:03, Jason Wang wrote:
> > On Tue, Aug 27, 2024 at 12:11 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
> >>
> >>
> >> On 26.08.24 16:24, Andrew Lunn wrote:
> >>> On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
> >>>>
> >>>>
> >>>> On 23.08.24 18:54, Carlos Bilbao wrote:
> >>>>> Hello,
> >>>>>
> >>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
> >>>>> configuration, I noticed that it's running in half duplex mode:
> >>>>>
> >>>>> Configuration data (24 bytes):
> >>>>>   MAC address: (Mac address)
> >>>>>   Status: 0x0001
> >>>>>   Max virtqueue pairs: 8
> >>>>>   MTU: 1500
> >>>>>   Speed: 0 Mb
> >>>>>   Duplex: Half Duplex
> >>>>>   RSS max key size: 0
> >>>>>   RSS max indirection table length: 0
> >>>>>   Supported hash types: 0x00000000
> >>>>>
> >>>>> I believe this might be contributing to the underperformance of vDPA.
> >>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
> >>>> feature which reports speed and duplex. You can check the state on the
> >>>> PF.
> >>>
> >>> Then it should probably report DUPLEX_UNKNOWN.
> >>>
> >>> The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
> >>> this just looks buggy in general.
> >>>
> >> The virtio spec doesn't mention what those values should be when
> >> VIRTIO_NET_F_SPEED_DUPLEX is not supported.
> >>
> >> Jason, should vdpa_dev_net_config_fill() initialize the speed/duplex
> >> fields to SPEED/DUPLEX_UNKNOWN instead of 0?
> >
> > Spec said
> >
> > """
> > The following two fields, speed and duplex, only exist if
> > VIRTIO_NET_F_SPEED_DUPLEX is set.
> > """
> >
> > So my understanding is that it is undefined behaviour, and those
> > fields seems useless before feature negotiation. For safety, it might
> > be better to initialize them as UNKOWN.
> >
> After a closer look my statement doesn't make sense: the device will copy
> the virtio_net_config bytes on top.
>
> The solution is to initialize these fields to UNKNOWN in the driver. Will send
> a patch to fix this.
>

For "driver", I guess you meant virtio-net?

Thanks

> Thanks,
> Dragos
>

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello,

On 8/26/24 9:07 PM, Jason Wang wrote:
> On Tue, Aug 27, 2024 at 3:23 AM Carlos Bilbao <cbilbao@digitalocean.com> wrote:
>> Hello,
>>
>> On 8/26/24 10:53 AM, Dragos Tatulea wrote:
>>> On 26.08.24 16:26, Carlos Bilbao wrote:
>>>> Hello Dragos,
>>>>
>>>> On 8/26/24 4:06 AM, Dragos Tatulea wrote:
>>>>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>>>>> configuration, I noticed that it's running in half duplex mode:
>>>>>>
>>>>>> Configuration data (24 bytes):
>>>>>>   MAC address: (Mac address)
>>>>>>   Status: 0x0001
>>>>>>   Max virtqueue pairs: 8
>>>>>>   MTU: 1500
>>>>>>   Speed: 0 Mb
>>>>>>   Duplex: Half Duplex
>>>>>>   RSS max key size: 0
>>>>>>   RSS max indirection table length: 0
>>>>>>   Supported hash types: 0x00000000
>>>>>>
>>>>>> I believe this might be contributing to the underperformance of vDPA.
>>>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>>>>> feature which reports speed and duplex. You can check the state on the
>>>>> PF.
>>>> According to ethtool, all my devices are running at full duplex. I assume I
>>>> can disregard this configuration output from the module then.
>>>>
>>> Yep.
>>>
>>>>>> While looking into how to change this option for Mellanox, I read the following
>>>>>> kernel code in mlx5_vnet.c:
>>>>>>
>>>>>> static void mlx5_vdpa_set_config(struct vdpa_device *vdev, unsigned int offset, const void *buf,
>>>>>>                  unsigned int len)
>>>>>> {
>>>>>>     /* not supported */
>>>>>> }
>>>>>>
>>>>>> I was wondering why this is the case.
>>>>> TBH, I don't know why it was not added. But in general, the control VQ is the
>>>>> better way as it's dynamic.
>>>>>
>>>>>> Is there another way for me to change
>>>>>> these configuration settings?
>>>>>>
>>>>> The configuration is done using control VQ for most things (MTU, MAC, VQs,
>>>>> etc). Make sure that you have the CTRL_VQ feature set (should be on by
>>>>> default). It should appear in `vdpa mgmtdev show` and `vdpa dev config
>>>>> show`.
>>>> I see that CTRL_VQ is indeed enabled. Is there any documentation on how to
>>>> use the control VQ to get/set vDPA configuration values?
>>>>
>>>>
>>> You are most likely using it already through through qemu. You can check
>>> if the CTR_VQ feature also shows up in the output of `vdpa dev config show`.
>>>
>>> What values are you trying to configure btw?
>>
>> Yes, CTRL_VQ also shows up in vdpa dev config show. There isn't a specific
>> value I want to configure ATM, but my vDPA isn't performing as expected, so
>> I'm investigating potential issues. Below is the code I used to retrieve
>> the configuration from the driver; I'd be happy to send it as a patch if
>> you or someone else reviews it.
>>
>>
>>> Thanks,
>>> Dragos
>>
>> Thanks,
>> Carlos
>>
>> ---
>>
>> From ab6ea66c926eaf1e95eb5d73bc23183e0021ee27 Mon Sep 17 00:00:00 2001
>> From: Carlos Bilbao <bilbao@vt.edu>
>> Date: Sat, 24 Aug 2024 00:24:56 +0000
>> Subject: [PATCH] mlx5: Add support to update the vDPA configuration
>>
>> This is needed for VHOST_VDPA_SET_CONFIG.
>>
>> Signed-off-by: Carlos Bilbao <cbilbao@digitalocean.com>
>> ---
>>  drivers/vdpa/mlx5/net/mlx5_vnet.c | 22 ++++++++++++++++++++--
>>  1 file changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> index b56aae3f7be3..da31c743b2b9 100644
>> --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c
>> @@ -2909,14 +2909,32 @@ static void mlx5_vdpa_get_config(struct vdpa_device *vdev, unsigned int offset,
>>      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))
>> +    if (offset + len <= sizeof(struct virtio_net_config)) {
>>          memcpy(buf, (u8 *)&ndev->config + offset, len);
>> +        }
>> +        else
>> +        {
>> +            printk(KERN_ERR "%s: Offset and length out of bounds\n",
>> +            __func__);
>> +        }
>> +
>>  }
>>
>>  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))
>> +    {
>> +        memcpy((u8 *)&ndev->config + offset, buf, len);
>> +    }
>> +    else
>> +    {
>> +        printk(KERN_ERR "%s: Offset and length out of bounds\n",
>> +        __func__);
>> +    }
>>  }
> This should follow the virtio-spec, for modern virtio-net devices,
> most of the fields are read only.


From mlx5_vnet.c function mlx5v_probe:

mgtdev->mgtdev.config_attr_mask = BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MACADDR) |
                    BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MAX_VQP) |
                    BIT_ULL(VDPA_ATTR_DEV_NET_CFG_MTU) |
                    BIT_ULL(VDPA_ATTR_DEV_FEATURES);

Does this mean these are the fields that set_config can update? I'm a bit
confused because, according to the virtio spec, I thought only speed and
duplex were not read-only -- but I was also told updating them isn't
supported by vDPA devices.


> Thanks
>
>>  static u32 mlx5_vdpa_get_generation(struct vdpa_device *vdev)
>> --
>> 2.34.1
>>
>>

Thanks, Carlos

Re: [RFC] Why is set_config not supported in mlx5_vnet?

[Carlos Bilbao]

Hello,

On 8/27/24 11:54 AM, Dragos Tatulea wrote:
>
> On 27.08.24 04:03, Jason Wang wrote:
>> On Tue, Aug 27, 2024 at 12:11 AM Dragos Tatulea <dtatulea@nvidia.com> wrote:
>>>
>>> On 26.08.24 16:24, Andrew Lunn wrote:
>>>> On Mon, Aug 26, 2024 at 11:06:09AM +0200, Dragos Tatulea wrote:
>>>>>
>>>>> On 23.08.24 18:54, Carlos Bilbao wrote:
>>>>>> Hello,
>>>>>>
>>>>>> I'm debugging my vDPA setup, and when using ioctl to retrieve the
>>>>>> configuration, I noticed that it's running in half duplex mode:
>>>>>>
>>>>>> Configuration data (24 bytes):
>>>>>>   MAC address: (Mac address)
>>>>>>   Status: 0x0001
>>>>>>   Max virtqueue pairs: 8
>>>>>>   MTU: 1500
>>>>>>   Speed: 0 Mb
>>>>>>   Duplex: Half Duplex
>>>>>>   RSS max key size: 0
>>>>>>   RSS max indirection table length: 0
>>>>>>   Supported hash types: 0x00000000
>>>>>>
>>>>>> I believe this might be contributing to the underperformance of vDPA.
>>>>> mlx5_vdpa vDPA devicess currently do not support the VIRTIO_NET_F_SPEED_DUPLEX
>>>>> feature which reports speed and duplex. You can check the state on the
>>>>> PF.
>>>> Then it should probably report DUPLEX_UNKNOWN.
>>>>
>>>> The speed of 0 also suggests SPEED_UNKNOWN is not being returned. So
>>>> this just looks buggy in general.
>>>>
>>> The virtio spec doesn't mention what those values should be when
>>> VIRTIO_NET_F_SPEED_DUPLEX is not supported.
>>>
>>> Jason, should vdpa_dev_net_config_fill() initialize the speed/duplex
>>> fields to SPEED/DUPLEX_UNKNOWN instead of 0?
>> Spec said
>>
>> """
>> The following two fields, speed and duplex, only exist if
>> VIRTIO_NET_F_SPEED_DUPLEX is set.
>> """
>>
>> So my understanding is that it is undefined behaviour, and those
>> fields seems useless before feature negotiation. For safety, it might
>> be better to initialize them as UNKOWN.
>>
> After a closer look my statement doesn't make sense: the device will copy
> the virtio_net_config bytes on top.
>
> The solution is to initialize these fields to UNKNOWN in the driver. Will send
> a patch to fix this.


With Dragos' permission, I'm sending a first draft of this now.


>
> Thanks,
> Dragos


Thanks, Carlos