Re: [dpdk-dev] [RFC 0/3] net/virtio: add vdpa device config support

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Adrian,

Thanks for your feedback.

On 4/16/21 9:28 AM, Adrian Moreno wrote:
> 
> 
> On 3/18/21 11:35 PM, Maxime Coquelin wrote:
>> This patch adds vDPA device config space requests support.
>> For now, it only adds MAC address get and set. It may be
>> extended in next revision to support other configs like
>> link state.
>>
>> Regarding the MAC selection strategy, if devargs MAC address
>> is set by the user and valid, the driver tries to store it
>> in the device config space, then it reads the MAC address
>> back from the device config, which will be used. If not set
>> in devargs or invalid, it tries to read it from the device.
>> If it fails, a random MAC will be used.
>>
>> I'm interrested to know your feedback on this strategy.
> 
> In general, I think it's a reasonable strategy. Once we have cq support, things
> will be a bit easier.
> 
> Some questions:
> How should we interpret failure to configure the mac (i.e: after set and get,
> they still don't match)? Should we fail virtio_user_dev_init if the
> configuration provided by devargs is not successfully applied?
> 
> Should a zero mac be treated differntly as qemu does? [1]
> 
> [1]
> https://patchwork.ozlabs.org/project/qemu-devel/patch/20210302142014.141135-3-mst@redhat.com/

Testing with ConnectX-6 Dx, I can see that the device does not advertise
VIRTIO_NET_F_MAC, so with this is series, it just doesn't try to read it
from the device.

My understanding is that in Qemu, the feature is forced[0], which
explains why it reads zeroes.

> 
>> It has been tested with vDPA simulator, which only supports
>> getting the MAC address, and witch CX6 which supports neither
>> getting or setting MAC address (and so devarg or random MAC is
>> used). IFCVF driver seems to support both getting and setting
>> the MAC, I have a try with it before next revision.
> 
> Does cx6 negotiate VIRTIO_NET_F_MAC?

Nope.
I haven't tested yet with IFCVF.

Maxime

>>
>> Maxime Coquelin (3):
>>   net/virtio: keep device and frontend features separated
>>   net/virtio: add device config support to vDPA
>>   net/virtio: add MAC device config getter and setter
>>
>>  drivers/net/virtio/virtio_user/vhost.h        |  3 +
>>  drivers/net/virtio/virtio_user/vhost_vdpa.c   | 69 +++++++++++++++
>>  .../net/virtio/virtio_user/virtio_user_dev.c  | 88 +++++++++++++++----
>>  .../net/virtio/virtio_user/virtio_user_dev.h  |  2 +
>>  drivers/net/virtio/virtio_user_ethdev.c       | 12 ++-
>>  5 files changed, 151 insertions(+), 23 deletions(-)
>>
> 

[0]: https://elixir.bootlin.com/qemu/latest/source/hw/net/virtio-net.c#L3078