On 2/26/19 3:07 PM, Ilya Maximets wrote:
On 26.02.2019 16:43, Maxime Coquelin wrote:
On 2/26/19 2:36 PM, Ilya Maximets wrote:
On 26.02.2019 15:32, Maxime Coquelin wrote:
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
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.
VHOST_USER_MAX is 31 and both new requests are
defined in the same enum VhostUserRequest:
VHOST_USER_GET_CONFIG = 24,
VHOST_USER_SET_CONFIG = 25
I don't think that any change is needed here.
I didn't meant GET_SET_CONFIG specifically. I meant that if we want
something really generic, we would need to do that.
OK. I understand now.
BTW, it would crash as vhost_message_str[VHOST_USER_GET/SET_CONFIG] would not
be defined.
With above change we would also be able to remove VHOST_CRYPTO requests
from vhost_user.c,
Maybe you're looking at the different git HEAD ? I don't see any crypto
related code in vhost_user.c. Only name definition in vhost_message_str.
Yes, I meant removing their definition in vhost_message_str[].
My point is that if we want to have external backends to handle their
specific requests, we should not have to modify vhost_user.c as it
creates a useless dependency.
That's a good point. I agree.
Maybe we'll need some new API to make vhost library more dynamic?
Something like
rte_vhost_message_register(enum VhostUserRequest request,
const char *resuest_str,
vhost_message_handler_t handler);
This could be flexible.
Agree it could be done like that.
Now, I think we have pretty much every thing we need in the API to
implement it, but maybe I'm missing something?
I.e., by implementing the .pre_msg_handle callback and setting its
skip_master to 1, we have the same result. Except that we don't
have the debug message.
Also, it means we would need to either rework all current handlers,
or make "struct virtio_net" part of the API.
So maybe we'll have to come to this, but we would need first to do
a significant rework of the library to move all the net specific
stuff out of the generic vhost part.
Thanks,
Maxime
and we could then work on moving vhost-net bits
out of this file too.
Regards,
Maxime
