Re: [dpdk-dev] [PATCH 3/3] vhost: support VFIO based accelerator

[Tiwei Bie <tiwei.bie@intel.com>]

On Tue, Mar 06, 2018 at 03:24:27PM +0100, Maxime Coquelin wrote:
> On 03/06/2018 11:43 AM, Tiwei Bie wrote:
[...]
> > +
> > +static int vhost_user_slave_set_vring_file(struct virtio_net *dev,
> > +					   uint32_t request,
> > +					   struct vhost_vring_file *file)
> Why passing the request as an argument?
> It seems to be called only with the same request ID.

I thought there may be other requests that also need to
send a file descriptor for a ring in the future. So I
made this a common routine. Maybe it's not really helpful.
I won't pass the request as an argument in next version.

> 
> > +{
> > +	int *fdp = NULL;
> > +	size_t fd_num = 0;
> > +	int ret;
> > +	struct VhostUserMsg msg = {
> > +		.request.slave = request,
> > +		.flags = VHOST_USER_VERSION | VHOST_USER_NEED_REPLY,
> > +		.payload.u64 = file->index & VHOST_USER_VRING_IDX_MASK,
> > +		.size = sizeof(msg.payload.u64),
> > +	};
> > +
> > +	if (file->fd < 0)
> > +		msg.payload.u64 |= VHOST_USER_VRING_NOFD_MASK;
> > +	else {
> > +		fdp = &file->fd;
> > +		fd_num = 1;
> > +	}
> > +
> > +	ret = send_vhost_message(dev->slave_req_fd, &msg, fdp, fd_num);
> > +	if (ret < 0) {
> > +		RTE_LOG(ERR, VHOST_CONFIG,
> > +			"Failed to send slave message %u (%d)\n",
> > +			request, ret);
> > +		return ret;
> > +	}
> > +
> > +	return process_slave_message_reply(dev, &msg);
> 
> Maybe not needed right now, but we'll need a lock to avoid concurrent
> requests sending and waiting for reply.

Yeah, probably, we need a lock for each slave channel. I didn't
check the code of Linux. Maybe it will cause problems when two
threads send e.g. below messages at the same time:

thread A:
 IOTLB miss message

thread B:
 VFIO group message which has a file descriptor

Thanks for the comments! :)

Best regards,
Tiwei Bie