[dpdk-dev] [PATCH 04/17] vhost: make notify ops per vhost driver

[Maxime Coquelin]

On 03/03/2017 10:51 AM, Yuanhan Liu wrote:
> Assume there is an application both support vhost-user net and
> vhost-user scsi, the callback should be different. Making notify
> ops per vhost driver allow application define different set of
> callbacks for different driver.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu at linux.intel.com>
> ---
>  drivers/net/vhost/rte_eth_vhost.c | 20 +++++++++++---------
>  examples/tep_termination/main.c   |  3 ++-
>  examples/vhost/main.c             |  5 +++--
>  lib/librte_vhost/rte_virtio_net.h |  3 ++-
>  lib/librte_vhost/socket.c         | 32 ++++++++++++++++++++++++++++++++
>  lib/librte_vhost/vhost.c          | 16 +---------------
>  lib/librte_vhost/vhost.h          |  5 ++++-
>  lib/librte_vhost/vhost_user.c     | 15 +++++++++------
>  8 files changed, 64 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 0ebaa4a..816a9a0 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -667,6 +667,12 @@ struct vhost_xstats_name_off {
>  	return 0;
>  }
>
> +static struct virtio_net_device_ops vhost_ops = {
> +	.new_device          = new_device,
> +	.destroy_device      = destroy_device,
> +	.vring_state_changed = vring_state_changed,
> +};
> +
>  int
>  rte_eth_vhost_get_queue_event(uint8_t port_id,
>  		struct rte_eth_vhost_queue_event *event)
> @@ -736,15 +742,6 @@ struct vhost_xstats_name_off {
>  static void *
>  vhost_driver_session(void *param __rte_unused)
>  {
> -	static struct virtio_net_device_ops vhost_ops;
> -
> -	/* set vhost arguments */
> -	vhost_ops.new_device = new_device;
> -	vhost_ops.destroy_device = destroy_device;
> -	vhost_ops.vring_state_changed = vring_state_changed;
> -	if (rte_vhost_driver_callback_register(&vhost_ops) < 0)
> -		RTE_LOG(ERR, PMD, "Can't register callbacks\n");
> -
>  	/* start event handling */
>  	rte_vhost_driver_session_start();
>
> @@ -1077,6 +1074,11 @@ struct vhost_xstats_name_off {
>  	if (rte_vhost_driver_register(iface_name, flags))
>  		goto error;
>
> +	if (rte_vhost_driver_callback_register(iface_name, &vhost_ops) < 0) {
> +		RTE_LOG(ERR, PMD, "Can't register callbacks\n");
> +		goto error;
> +	}
> +
>  	/* We need only one message handling thread */
>  	if (rte_atomic16_add_return(&nb_started_ports, 1) == 1) {
>  		if (vhost_driver_session_start())
> diff --git a/examples/tep_termination/main.c b/examples/tep_termination/main.c
> index 8c45128..03c0fbe 100644
> --- a/examples/tep_termination/main.c
> +++ b/examples/tep_termination/main.c
> @@ -1258,7 +1258,8 @@ static inline void __attribute__((always_inline))
>  	rte_vhost_driver_disable_features(dev_basename,
>  		1ULL << VIRTIO_NET_F_MRG_RXBUF);
>
> -	rte_vhost_driver_callback_register(&virtio_net_device_ops);
> +	rte_vhost_driver_callback_register(dev_basename,
> +		&virtio_net_device_ops);

Return should be checked here, as this function can now return -1.

>
>  	rte_vhost_driver_session_start();
>
> diff --git a/examples/vhost/main.c b/examples/vhost/main.c
> index 972a6a8..867efc6 100644
> --- a/examples/vhost/main.c
> +++ b/examples/vhost/main.c
> @@ -1538,9 +1538,10 @@ static inline void __attribute__((always_inline))
>  			rte_vhost_driver_enable_features(file,
>  				1ULL << VIRTIO_NET_F_CTRL_RX);
>  		}
> -	}
>
> -	rte_vhost_driver_callback_register(&virtio_net_device_ops);
> +		rte_vhost_driver_callback_register(file,
> +			&virtio_net_device_ops);
> +	}

Ditto.

>
>  	rte_vhost_driver_session_start();
>  	return 0;
> diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h
> index 51f5166..3bfd0b7 100644
> --- a/lib/librte_vhost/rte_virtio_net.h
> +++ b/lib/librte_vhost/rte_virtio_net.h
> @@ -91,7 +91,8 @@ struct virtio_net_device_ops {
>  int rte_vhost_driver_disable_features(const char *path, uint64_t features);
>
>  /* Register callbacks. */
> -int rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const);
> +int rte_vhost_driver_callback_register(const char *path,
> +	struct virtio_net_device_ops const * const);
>  /* Start vhost driver session blocking loop. */
>  int rte_vhost_driver_session_start(void);
>
> diff --git a/lib/librte_vhost/socket.c b/lib/librte_vhost/socket.c
> index 9e0ec05..550af64 100644
> --- a/lib/librte_vhost/socket.c
> +++ b/lib/librte_vhost/socket.c
> @@ -73,6 +73,8 @@ struct vhost_user_socket {
>  	 */
>  	uint64_t supported_features;
>  	uint64_t features;
> +
> +	struct virtio_net_device_ops const *notify_ops;
>  };
>
>  struct vhost_user_connection {
> @@ -718,6 +720,36 @@ struct vhost_user_reconnect_list {
>  	return -1;
>  }
>
> +/*
> + * Register ops so that we can add/remove device to data core.
> + */
> +int
> +rte_vhost_driver_callback_register(const char *path,
> +	struct virtio_net_device_ops const * const ops)
> +{
> +	struct vhost_user_socket *vsocket;
> +
> +	pthread_mutex_lock(&vhost_user.mutex);
> +	vsocket = find_vhost_user_socket(path);
> +	if (vsocket)
> +		vsocket->notify_ops = ops;
> +	pthread_mutex_unlock(&vhost_user.mutex);
> +
> +	return vsocket ? 0 : -1;
> +}
> +
> +struct virtio_net_device_ops const *
> +vhost_driver_callback_get(const char *path)
> +{
> +	struct vhost_user_socket *vsocket;
> +
> +	pthread_mutex_lock(&vhost_user.mutex);
> +	vsocket = find_vhost_user_socket(path);
> +	pthread_mutex_unlock(&vhost_user.mutex);
> +
> +	return vsocket->notify_ops;

There should be a check against vsocket to avoid NULL pointer
dereferencing.

> +}
> +
>  int
>  rte_vhost_driver_session_start(void)
>  {
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index 2790f17..0088f87 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -51,9 +51,6 @@
>
>  struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
>
> -/* device ops to add/remove device to/from data core. */
> -struct virtio_net_device_ops const *notify_ops;
> -
>  struct virtio_net *
>  get_device(int vid)
>  {
> @@ -253,7 +250,7 @@ struct virtio_net *
>
>  	if (dev->flags & VIRTIO_DEV_RUNNING) {
>  		dev->flags &= ~VIRTIO_DEV_RUNNING;
> -		notify_ops->destroy_device(vid);
> +		dev->notify_ops->destroy_device(vid);
>  	}
>
>  	cleanup_device(dev, 1);
> @@ -377,14 +374,3 @@ struct virtio_net *
>  	dev->virtqueue[queue_id]->used->flags = VRING_USED_F_NO_NOTIFY;
>  	return 0;
>  }
> -
> -/*
> - * Register ops so that we can add/remove device to data core.
> - */
> -int
> -rte_vhost_driver_callback_register(struct virtio_net_device_ops const * const ops)
> -{
> -	notify_ops = ops;
> -
> -	return 0;
> -}
> diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h
> index 61e7448..bc03e09 100644
> --- a/lib/librte_vhost/vhost.h
> +++ b/lib/librte_vhost/vhost.h
> @@ -177,6 +177,8 @@ struct virtio_net {
>  	uint64_t		log_addr;
>  	struct ether_addr	mac;
>
> +	struct virtio_net_device_ops const *notify_ops;
> +
>  	uint32_t		nr_guest_pages;
>  	uint32_t		max_guest_pages;
>  	struct guest_page       *guest_pages;
> @@ -280,7 +282,6 @@ static inline phys_addr_t __attribute__((always_inline))
>  	return 0;
>  }
>
> -struct virtio_net_device_ops const *notify_ops;
>  struct virtio_net *get_device(int vid);
>
>  int vhost_new_device(void);
> @@ -293,6 +294,8 @@ static inline phys_addr_t __attribute__((always_inline))
>  void vhost_set_ifname(int, const char *if_name, unsigned int if_len);
>  void vhost_enable_dequeue_zero_copy(int vid);
>
> +struct virtio_net_device_ops const *vhost_driver_callback_get(const char *path);
> +
>  /*
>   * Backend-specific cleanup.
>   *
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index f7227bf..c101fbc 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -131,7 +131,7 @@
>  {
>  	if (dev->flags & VIRTIO_DEV_RUNNING) {
>  		dev->flags &= ~VIRTIO_DEV_RUNNING;
> -		notify_ops->destroy_device(dev->vid);
> +		dev->notify_ops->destroy_device(dev->vid);
>  	}
>
>  	cleanup_device(dev, 0);
> @@ -499,7 +499,7 @@
>  	/* Remove from the data plane. */
>  	if (dev->flags & VIRTIO_DEV_RUNNING) {
>  		dev->flags &= ~VIRTIO_DEV_RUNNING;
> -		notify_ops->destroy_device(dev->vid);
> +		dev->notify_ops->destroy_device(dev->vid);
>  	}
>
>  	if (dev->mem) {
> @@ -680,7 +680,7 @@
>  				"dequeue zero copy is enabled\n");
>  		}
>
> -		if (notify_ops->new_device(dev->vid) == 0)
> +		if (dev->notify_ops->new_device(dev->vid) == 0)
>  			dev->flags |= VIRTIO_DEV_RUNNING;
>  	}
>  }
> @@ -713,7 +713,7 @@
>  	/* We have to stop the queue (virtio) if it is running. */
>  	if (dev->flags & VIRTIO_DEV_RUNNING) {
>  		dev->flags &= ~VIRTIO_DEV_RUNNING;
> -		notify_ops->destroy_device(dev->vid);
> +		dev->notify_ops->destroy_device(dev->vid);
>  	}
>
>  	/* Here we are safe to get the last used index */
> @@ -753,8 +753,8 @@
>  		"set queue enable: %d to qp idx: %d\n",
>  		enable, state->index);
>
> -	if (notify_ops->vring_state_changed)
> -		notify_ops->vring_state_changed(dev->vid, state->index, enable);
> +	if (dev->notify_ops->vring_state_changed)
> +		dev->notify_ops->vring_state_changed(dev->vid, state->index, enable);
>
>  	dev->virtqueue[state->index]->enabled = enable;
>
> @@ -952,6 +952,9 @@
>  	if (dev == NULL)
>  		return -1;
>
> +	if (!dev->notify_ops)
> +		dev->notify_ops = vhost_driver_callback_get(dev->ifname);

Once vhost_driver_callback_get() fixed, notify_ops can be NULL, and it
seems to be dereferenced without being checked later on.

> +
>  	ret = read_vhost_message(fd, &msg);
>  	if (ret <= 0 || msg.request >= VHOST_USER_MAX) {
>  		if (ret < 0)
>