On 2/27/19 2:31 AM, Liu, Changpeng wrote:
-----Original Message-----
From: Maxime Coquelin [mailto:maxime.coque...@redhat.com]
Sent: Tuesday, February 26, 2019 8:32 PM
To: Ilya Maximets <i.maxim...@samsung.com>; Liu, Changpeng
<changpeng....@intel.com>; dev@dpdk.org
Cc: Stojaczyk, Dariusz <dariusz.stojac...@intel.com>; Bie, Tiwei
<tiwei....@intel.com>; Wang, Zhihong <zhihong.w...@intel.com>; Jason Wang
<jasow...@redhat.com>
Subject: Re: vhost: add virtio configuration space access socket messages
On 2/26/19 9:42 AM, Ilya Maximets wrote:
On 26.02.2019 11:13, Liu, Changpeng wrote:
-----Original Message-----
From: Ilya Maximets [mailto:i.maxim...@samsung.com]
Sent: Tuesday, February 26, 2019 3:39 PM
To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
Cc: Stojaczyk, Dariusz <dariusz.stojac...@intel.com>;
maxime.coque...@redhat.com; Bie, Tiwei <tiwei....@intel.com>; Wang,
Zhihong <zhihong.w...@intel.com>; Jason Wang <jasow...@redhat.com>
Subject: Re: vhost: add virtio configuration space access socket messages
On 26.02.2019 10:01, Liu, Changpeng wrote:
-----Original Message-----
From: Ilya Maximets [mailto:i.maxim...@samsung.com]
Sent: Monday, February 25, 2019 9:20 PM
To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
Cc: Stojaczyk, Dariusz <dariusz.stojac...@intel.com>;
maxime.coque...@redhat.com; Bie, Tiwei <tiwei....@intel.com>; Wang,
Zhihong <zhihong.w...@intel.com>; Jason Wang <jasow...@redhat.com>
Subject: Re: vhost: add virtio configuration space access socket messages
On 25.02.2019 10:51, Changpeng Liu wrote:
This patch adds new vhost user messages GET_CONFIG and SET_CONFIG
used to get/set virtio device's PCI configuration space.
Beside the fact that some additional description and reasoning required,
I do not see the usage of this feature. You're defining the flag
VHOST_USER_PROTOCOL_F_CONFIG, but it's never used. So, none of
dpdk
vhost
backends (vdpa, vhost-user) will use this feature.
You, probably, missed adding it to VHOST_USER_PROTOCOL_FEATURES or
VDPA_SUPPORTED_PROTOCOL_FEATURES.
From the other side, current implementation forces application to
properly
implement the get/set_config callbacks. Otherwise, receiving of the
messages
will result in RTE_VHOST_MSG_RESULT_ERR and subsequent vhost
disconnection.
This looks strange, because supported protocol features normally enabled
by
default. Am I misunderstood something ?
QEMU will not send the messages if VHOST_USER_PROTOCOL_F_CONFIG
wasn't enabled.
So, you're going to enable it only by explicit call to
'rte_vhost_driver_set_features' ?
In this case I'm assuming that you're implementing your own vhost backend.
But why you're not using 'dev->extern_ops' and corresponding
'pre_msg_handle'
or 'post_msg_handle' to handle your GET/SET_CONFIG messages like it does
'vhost_crypto' backend ?
The patch was developed one year ago, while DPDK didn't have external ops.
So, maybe it's time to reconsider the implementation.
+1
Okay, I will only add related messages definition in this patch and remove the
Callbacks.
What we need to do is fix vhost lib so that you don't have anything to
do in dpdk to add support for the new requests.
So we need to fix the few bits in vhost_user_msg_handler() I mentionned
yesterday. And I also notice we are missing
rte_vhost_driver_set_protocol_feature(), so that you can advertise
VHOST_USER_PROTOCOL_F_CONFIG support by your external backend.
I'll try to cook the dpdk patch today.
Cheers,
Maxime
The get_config/set_config was defined for all the virtio devices, so I think it
makes
more sense adding here.
VHOST_USER_*_CRYPTO_SESSION messages are defined for all the virtio
devices
too, however they makes sense for vhost_crypto backend only. These
messages
(GET/SET_CONFIG) makes sense only when callbacks (get/set_config) are
implemented, so IMHO it's better to implement their handlers along with the
callbacks, i.e. inside the implementation of your vhost backend.
Maxime, Tiwei, what do you think ?
I would prefer it to be implemented in SPDK directly as a pre_handler
callback, as I don't foresee a need for it for other backends, and it
would avoid breaking the API.
It would imply fixing the beginning of vhost_user_msg_handler() to
accept requests > VHOST_USER_MAX and add necessary check before doing
the debug logs.
With above change we would also be able to remove VHOST_CRYPTO requests
from vhost_user.c, and we could then work on moving vhost-net bits
out of this file too.
Regards,
Maxime
