Re: [PATCH] hw/virtio/vhost-user: support obtain vdpa device's mac address automatically

[陈浩]

在 2022/9/21 13:28, Raphael Norwitz 写道:

I have some concerns from the vhost-user-blk side.

 

>On Tue, Sep 13, 2022 at 5:13 PM Hao Chen <chenh@yusur.tech> wrote:

>> 

>> When use dpdk-vdpa tests vdpa device. You need to specify the mac address to

>> start the virtual machine through libvirt or qemu, but now, the libvirt or

>> qemu can call dpdk vdpa vendor driver's ops .get_config through vhost_net_get_config

>> to get the mac address of the vdpa hardware without manual configuration.

>> 

>> Signed-off-by: Hao Chen <chenh@yusur.tech>

> 

>Adding Cindy for comments.

> 

>Thanks

> 

>> ---

>>  hw/block/vhost-user-blk.c |  1 -

>>  hw/net/virtio-net.c       |  3 ++-

>>  hw/virtio/vhost-user.c    | 19 -------------------

>>  3 files changed, 2 insertions(+), 21 deletions(-)

>> 

>> diff --git a/hw/block/vhost-user-blk.c b/hw/block/vhost-user-blk.c

>> index 9117222456..5dca4eab09 100644

>> --- a/hw/block/vhost-user-blk.c

>> +++ b/hw/block/vhost-user-blk.c

>> @@ -337,7 +337,6 @@ static int vhost_user_blk_connect(DeviceState *dev, Error **errp)

>> 

>>      vhost_dev_set_config_notifier(&s->dev, &blk_ops);

>> 

>> -    s->vhost_user.supports_config = true;

 

vhost-user-blk requires the backend to support VHOST_USER_PROTOCOL_F_CONFIG

and vhost_user.supports_config is used to enforce that.

 

Why are you removing it here?

 

 

>>      ret = vhost_dev_init(&s->dev, &s->vhost_user, VHOST_BACKEND_TYPE_USER, 0,

>>                           errp);

>>      if (ret < 0) {

>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c

>> index dd0d056fde..274ea84644 100644

>> --- a/hw/net/virtio-net.c

>> +++ b/hw/net/virtio-net.c

>> @@ -149,7 +149,8 @@ static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)

>>       * Is this VDPA? No peer means not VDPA: there's no way to

>>       * disconnect/reconnect a VDPA peer.

>>       */

>> -    if (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) {

>> +    if ((nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_VDPA) ||

>> +        (nc->peer && nc->peer->info->type == NET_CLIENT_DRIVER_VHOST_USER)) {

>>          ret = vhost_net_get_config(get_vhost_net(nc->peer), (uint8_t *)&netcfg,

>>                                     n->config_size);

>>          if (ret != -1) {

>> diff --git a/hw/virtio/vhost-user.c b/hw/virtio/vhost-user.c

>> index bd24741be8..8b01078249 100644

>> --- a/hw/virtio/vhost-user.c

>> +++ b/hw/virtio/vhost-user.c

>> @@ -2013,8 +2013,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,

>>      }

>> 

 

Why are you removing this? Can you expand on how it helps dpdk-vdpa.

I implemented a function by modifying these places in the patch, and then implementing vhost msg ops '.get_config' in the driver under 'DPDK_SRC/drivers/vdpa/' , qemu can use 'vhost_user_get_config' to send vhost msg to calls the dpdk vdpa vendor driver's ops 'get_config' to obtain the mac address information of the vdpa device. In this way, even if the mac address of the network device parameters configured by the user for qemu vhost user is inconsistent with the mac address of the vdpa hardware, qemu can be corrected to be consistent with the mac address of the vdpa hardware.

When vhost-user interface is used by dpdk-vdpa & qemu, 'vhost_user_get_config' will get the vdpa device's mac address if dpdk vdpa vendor driver implement 'get_config',otherwise, it won't do anything.

When vhost-user interface is used by dpdk vhost-user, it won't do anything.

I think the functions provided by the VHOST_USER_PROTOCOL_F_CONFIG feature can be available separately, but the check here caused VHOST_USER_PROTOCOL_F_CONFIG feature is cleared, which will cause vhost_user_get_config is not working properly in virtio-net at hw/net/virtio_net.c.

Maybe the check here should be moved other side?
The new patch v2 has been sent, please reply there.

 

>>      if (virtio_has_feature(features, VHOST_USER_F_PROTOCOL_FEATURES)) {

>> -        bool supports_f_config = vus->supports_config ||

>> -            (dev->config_ops && dev->config_ops->vhost_dev_config_notifier);

>>          uint64_t protocol_features;

>> 

>>          dev->backend_features |= 1ULL << VHOST_USER_F_PROTOCOL_FEATURES;

>> @@ -2033,23 +2031,6 @@ static int vhost_user_backend_init(struct vhost_dev *dev, void *opaque,

>>           */

>>          protocol_features &= VHOST_USER_PROTOCOL_FEATURE_MASK;

>> 

>> -        if (supports_f_config) {

>> -            if (!virtio_has_feature(protocol_features,

>> -                                    VHOST_USER_PROTOCOL_F_CONFIG)) {

>> -                error_setg(errp, "vhost-user device expecting "

>> -                           "VHOST_USER_PROTOCOL_F_CONFIG but the vhost-user backend does "

>> -                           "not support it.");

>> -                return -EPROTO;

>> -            }

>> -        } else {

>> -            if (virtio_has_feature(protocol_features,

>> -                                   VHOST_USER_PROTOCOL_F_CONFIG)) {

>> -                warn_reportf_err(*errp, "vhost-user backend supports "

>> -                                 "VHOST_USER_PROTOCOL_F_CONFIG but QEMU does not.");

>> -                protocol_features &= ~(1ULL << VHOST_USER_PROTOCOL_F_CONFIG);

>> -            }

>> -        }

>> -

>>          /* final set of protocol features */

>>          dev->protocol_features = protocol_features;

>>          err = vhost_user_set_protocol_features(dev, dev->protocol_features);

>> --

>> 2.27.0

>> 

>