[dpdk-dev,v2] net/vhost: fix segfault when creating vdev dynamically

  when creating vdev dynamically, vhost pmd driver start directly without
checking TX/RX queues ready or not, and thus cause segmentation fault when
vhost library accessing queues. This patch add flag to check whether queues
setup or not, and add driver start call into dev_start to allow user start
it after setting up queue.

Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
---
Changes in v2:
- check queue status in new_device, create queue in dev_start if not setup yet
 drivers/net/vhost/rte_eth_vhost.c | 73 ++++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 20 deletions(-)
  

Hi Junjie,

Thanks for the patch, it looks like the right thing to do.
Please find my comments below:

On 03/29/2018 05:35 PM, Junjie Chen wrote:
> when creating vdev dynamically, vhost pmd driver start directly without
s/when/When/

s/start/starts/
> checking TX/RX queues ready or not, and thus cause segmentation fault when
s/ready/are ready/
s/cause/causes/
> vhost library accessing queues. This patch add flag to check whether queues
s/accessing/accesses/
s/add flag/adds a flag/
> setup or not, and add driver start call into dev_start to allow user start
s/setup/are setup/
s/add/adds/
s/start/ to start/
> it after setting up queue.
s/queue/queues/
> 
> Signed-off-by: Junjie Chen <junjie.j.chen@intel.com>
> ---
> Changes in v2:
> - check queue status in new_device, create queue in dev_start if not setup yet
>   drivers/net/vhost/rte_eth_vhost.c | 73 ++++++++++++++++++++++++++++-----------
>   1 file changed, 53 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/vhost/rte_eth_vhost.c b/drivers/net/vhost/rte_eth_vhost.c
> index 3aae01c39..41410fa5a 100644
> --- a/drivers/net/vhost/rte_eth_vhost.c
> +++ b/drivers/net/vhost/rte_eth_vhost.c
> @@ -117,7 +117,9 @@ struct pmd_internal {
>   	char *dev_name;
>   	char *iface_name;
>   	uint16_t max_queues;
> +	uint16_t vid;
>   	rte_atomic32_t started;
> +	rte_atomic32_t once;

The name is a bit vague.
Also, I wonder if we need a new variable.
Couldn't we rely on dev_attached? In new_device(), only set
dev_attached=1 if queues are allocated. And in eth_dev_start(),
attach queues if !dev_attached and then set dev_attached to 1.

>   };
>   
>   struct internal_list {
> @@ -580,21 +582,27 @@ new_device(int vid)
>   		eth_dev->data->numa_node = newnode;
>   #endif
>   
> -	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> -		vq = eth_dev->data->rx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = vid;
> -		vq->internal = internal;
> -		vq->port = eth_dev->data->port_id;
> -	}
> -	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> -		vq = eth_dev->data->tx_queues[i];
> -		if (vq == NULL)
> -			continue;
> -		vq->vid = vid;
> -		vq->internal = internal;
> -		vq->port = eth_dev->data->port_id;
> +	if (eth_dev->data->rx_queues && eth_dev->data->tx_queues) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +	} else {
> +		RTE_LOG(INFO, PMD, "RX/TX queues have not setup yet\n");
> +		internal->vid = vid;
> +		rte_atomic32_set(&internal->once, 0);
>   	}
>   
>   	for (i = 0; i < rte_vhost_get_vring_num(vid); i++)
> @@ -605,7 +613,8 @@ new_device(int vid)
>   	eth_dev->data->dev_link.link_status = ETH_LINK_UP;
>   
>   	rte_atomic32_set(&internal->dev_attached, 1);
> -	update_queuing_status(eth_dev);
> +	if (likely(rte_atomic32_read(&internal->once) == 1))
> +		update_queuing_status(eth_dev);
>   
>   	RTE_LOG(INFO, PMD, "Vhost device %d created\n", vid);
>   
> @@ -770,12 +779,34 @@ rte_eth_vhost_get_vid_from_port_id(uint16_t port_id)
>   }
>   
>   static int
> -eth_dev_start(struct rte_eth_dev *dev)
> +eth_dev_start(struct rte_eth_dev *eth_dev)
>   {
> -	struct pmd_internal *internal = dev->data->dev_private;
> +	struct pmd_internal *internal = eth_dev->data->dev_private;
> +	struct vhost_queue *vq;
> +	int i;
> +
> +	if (unlikely(rte_atomic32_read(&internal->once) == 0)) {
> +		for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
> +			vq = eth_dev->data->rx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = internal->vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}
> +		for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
> +			vq = eth_dev->data->tx_queues[i];
> +			if (!vq)
> +				continue;
> +			vq->vid = internal->vid;
> +			vq->internal = internal;
> +			vq->port = eth_dev->data->port_id;
> +		}

I think you could avoid the code duplication by creating a new function 
to setup queues:

void queue_setup(struct rte_eth_dev *eth_dev,
		 struct pmd_internal *internal)
{
	int i;

	for (i = 0; i < eth_dev->data->nb_rx_queues; i++) {
		vq = eth_dev->data->rx_queues[i];
		if (!vq)
			continue;
		vq->vid = internal->vid;
		vq->internal = internal;
		vq->port = eth_dev->data->port_id;
	}
	for (i = 0; i < eth_dev->data->nb_tx_queues; i++) {
		vq = eth_dev->data->tx_queues[i];
		if (!vq)
			continue;
		vq->vid = internal->vid;
		vq->internal = internal;
		vq->port = eth_dev->data->port_id;
	}
}

> +			rte_atomic32_set(&internal->once, 1);

Indentation looks wrong here.

> +	}
>   
>   	rte_atomic32_set(&internal->started, 1);
> -	update_queuing_status(dev);
> +	update_queuing_status(eth_dev);
>   
>   	return 0;
>   }
> @@ -786,7 +817,9 @@ eth_dev_stop(struct rte_eth_dev *dev)
>   	struct pmd_internal *internal = dev->data->dev_private;
>   
>   	rte_atomic32_set(&internal->started, 0);
> -	update_queuing_status(dev);
> +
> +	if (likely(rte_atomic32_read(&internal->once) == 1))
> +		update_queuing_status(dev);
>   }
>   
>   static void
>