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! > >> > >> 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
