On 27.02.2019 12:04, Maxime Coquelin wrote: > > > 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?
Yes, looks like we have. > > 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. Sure. This should work. > > Also, it means we would need to either rework all current handlers, > or make "struct virtio_net" part of the API. Actually, 'vhost_crypto' already uses the 'struct virtio_net'. So, it's kind of a part of the API anyway. Do we need a special API to get 'extern_data' ? Right now 'vhost_crypto' gets it directly from the 'vhost_net' structure. 'vhost_crypto' seems very strange though. I don't think that it works. (example calls 'set_features' via 'crypto_create' inside the 'new_device' callback, this should not work). Also, it uses a lot of internal stuff for which we have public APIs. > 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. Sure. It seems to me that we need to split out network related stuff from the 'struct virtio_net' to a separate 'struct vhost_net' and rename 'struct virtio_net' to, for example, 'struct virtio_dev'. Making some kind of inheritance between them. Maybe having a 'backend' pointer to the corresponding 'struct vhost_net/crypto/scsi' and replace the 'extern_data' with it. > > Thanks, > Maxime >> >>> >>>>> and we could then work on moving vhost-net bits >>>>> out of this file too. >>>>> >>>>> Regards, >>>>> Maxime >>>>> >>>>> >>>>> >>> >>> > >
