> -----Original Message----- > From: Zhang, Roy Fan > Sent: Wednesday, March 21, 2018 5:12 PM > To: Maxime Coquelin <maxime.coque...@redhat.com>; dev@dpdk.org; > Kulasek, TomaszX <tomaszx.kula...@intel.com>; Wodkowski, PawelX > <pawelx.wodkow...@intel.com> > Cc: jianjay.z...@huawei.com; y...@fridaylinux.org; Tan, Jianfeng > <jianfeng....@intel.com>; Bie, Tiwei <tiwei....@intel.com> > Subject: RE: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user > private info structure > > Hi Maxime, > > Thanks a lot for the fast reply. > > > -----Original Message----- > > From: Maxime Coquelin [mailto:maxime.coque...@redhat.com] > > Sent: Wednesday, March 21, 2018 1:03 PM > > To: Zhang, Roy Fan <roy.fan.zh...@intel.com>; dev@dpdk.org; Kulasek, > > TomaszX <tomaszx.kula...@intel.com>; Wodkowski, PawelX > > <pawelx.wodkow...@intel.com> > > Cc: jianjay.z...@huawei.com; y...@fridaylinux.org; Tan, Jianfeng > > <jianfeng....@intel.com>; Bie, Tiwei <tiwei....@intel.com> > > Subject: Re: [dpdk-dev] [PATCH v2 01/10] lib/librte_vhost: add vhost user > > private info structure > > > > Hi, > > > > On 03/21/2018 10:10 AM, Zhang, Roy Fan wrote: > > > Hi Maxime, > > > > > > Thanks for the review. > > > > > >> > > >> I think this include isn't needed looking at the rest of the patch. > > > > > > I agree. I will remove this line here. > > > > > >>> #define VHOST_USER_VERSION 0x1 > > >>> > > >>> +typedef int (*msg_handler)(struct virtio_net *dev, struct > > >>> +VhostUserMsg > > >> *msg, > > >>> + int fd); > > >>> + > > >>> +struct vhost_user_dev_priv { > > >>> + msg_handler vhost_user_msg_handler; > > >>> + char data[0]; > > >>> +}; > > >>> > > >>> /* vhost_user.c */ > > >>> int vhost_user_msg_handler(int vid, int fd); > > >>> > > >> > > >> I think the wording is a bit misleading, I'm fine with having a > > >> private_data pointer, but it should only be used by the external backend. > > >> > > >> Maybe what you need here is a new API to be to register a callback > > >> for the external backend to handle specific requests. > > > > > > That's exactly what I need. > > > Shall I rework the code like this? > > > > These new API are to be placed in rte_vhost.h, else it won't be exported. > > > > > /* vhost.h */ > > > struct virtio_net { > > > .... > > > void *extern_data; /*<< private data for external backend */ > > > > > > } > > > > Looks good, you may need to add getter and setter APIs as struct virtio_net > > isn't part of the API. > > > > > /* vhost_user.h */ > > > typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg > > *msg, > > > int fd); > > > > I wonder if the fd is really necessary, as if a reply is to be sent, it can > > be done > > by the vhost lib. > > Same as the messages VHOST_USER_GET_PROTOCOL_FEATURES and > VHOST_USER_GET_VRING_BASE etc, the external backend such as vhost crypto > will require to send an reply independently. > In case of vhost crypto, the message create_session will require the backend > to > send a session id back to the frontend. > > I tried to set VHOST_USER_NEED_REPLY bit in the vhost crypto message > handler but it will cause problem, qemu returns " qemu-system-x86_64: > Received bad msg size." > > Another solution is to pass a variable back from the message handler to > indicate sending the reply instead. > So the function prototype can be > typedef int (*msg_handler)(struct virtio_net *dev, struct VhostUserMsg *msg, > uint32_t *require_reply); > > > > > struct vhost_user_dev_extern { > > > msg_handler post_vhost_user_msg_handler; > > > char data[0]; > > Why is this filed needed? > > Either this way, or using a pointer. The idea is to allocate contiguous memory > when creating new vhost_user_dev_extern structure. The former way is slightly > more performant to avoid another memory read, but I can live with the pointer > instead :-) > > > > }; > > I would change this to: > > struct rte_vhost_user_dev_extern_ops { > > rte_vhost_msg_handler pre_vhost_user_msg_handler; > > rte_vhost_msg_handler post_vhost_user_msg_handler; }; > > > > > int > > > vhost_user_register_call_back(struct virtio_net *dev, msg_handler > > > post_msg_handler); > > > > and something like: > > rte_vhost_user_register_extern_ops(struct virtio_net *dev, struct > > rte_vhost_user_dev_extern_ops *ops); > > Great idea!
Can we add this handler in struct vhost_device_ops? There is filed void *reserved[2]. We can use first element to define this handler. > > > >> > > >> Also, it might be interesting for the external backend to register > > >> callbacks for existing requests. For example > > >> .pre_vhost_user_msg_handler and .post_vhost_user_msg_handler. > > Doing > > >> so, the external backend could for example catch beforehand any > > >> change that could affect resources being used. Tomasz, Pawel, do you > > think that could help for the issue you reported? > > >> > > > > > > > > > I think it is definitely a good idea. However there will be a problem. As > > vhost_crypto does not require pre_vhost_user_msg_handler I think it may > > not appropriate to add pre_vhost_user_msg_handler in this patchset. > > > > I see, but please add the pre_ callback directly in this series, as we know > > it > > will be useful (thanks Pawel). > > It will avoid breaking the API again when we'll need it. > > Will do. > > > > > Thanks, > > Maxime > > > Thanks a million Maxime. > > > > > >> Thanks, > > >> Maxime > > Thanks, > Fan