RE: [PATCH v5 07/12] net/nfp: add flower ctrl VNIC related logics

[Chaoyong He <chaoyong.he@corigine.com>]

> On 8/5/22 09:32, Chaoyong He wrote:
> > This commit adds the setup/start logic for the ctrl vNIC. This vNIC
> 
> "This commit adds" -> "Add"
> 
> > is used by the PMD and flower firmware as a communication channel
> > between driver and firmware. In the case of OVS it is also used to
> > communicate flow statistics from hardware to the driver.
> >
> > A rte_eth device is not exposed to DPDK for this vNIC as it is
> > strictly used internally by flower logic. Rx and Tx logic will be
> > added later for this vNIC.
> >
> > Signed-off-by: Chaoyong He <chaoyong.he@corigine.com>
> > Reviewed-by: Niklas Söderlund <niklas.soderlund@corigine.com>
> > ---
> >   drivers/net/nfp/flower/nfp_flower.c | 385
> +++++++++++++++++++++++++++++++++++-
> >   drivers/net/nfp/flower/nfp_flower.h |   6 +
> >   2 files changed, 388 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/nfp/flower/nfp_flower.c
> > b/drivers/net/nfp/flower/nfp_flower.c
> > index 2498020..51df504 100644
> > --- a/drivers/net/nfp/flower/nfp_flower.c
> > +++ b/drivers/net/nfp/flower/nfp_flower.c
> > @@ -26,6 +26,10 @@
> >   #define MEMPOOL_CACHE_SIZE 512
> >   #define DEFAULT_FLBUF_SIZE 9216
> >
> > +#define CTRL_VNIC_NB_DESC 64
> > +#define CTRL_VNIC_RX_FREE_THRESH 32
> > +#define CTRL_VNIC_TX_FREE_THRESH 32
> > +
> >   /*
> >    * Simple dev ops functions for the flower PF. Because a rte_device is
> exposed
> >    * to DPDK the flower logic also makes use of helper functions like
> > @@ -543,6 +547,302 @@
> >   	return ret;
> >   }
> >
> > +static void
> > +nfp_flower_cleanup_ctrl_vnic(struct nfp_net_hw *hw) {
> > +	uint32_t i;
> > +	struct nfp_net_rxq *rxq;
> > +	struct nfp_net_txq *txq;
> > +	struct rte_eth_dev *eth_dev;
> > +
> > +	eth_dev = hw->eth_dev;
> > +
> > +	for (i = 0; i < hw->max_tx_queues; i++) {
> > +		txq = eth_dev->data->tx_queues[i];
> > +		if (txq) {
> 
> Compare vs NULL as you do in other cases and as DPDK coding style
> recommends.
> 
> > +			rte_free(txq->txbufs);
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_tx_ring", i);
> > +			rte_free(txq);
> > +		}
> > +	}
> > +
> > +	for (i = 0; i < hw->max_rx_queues; i++) {
> > +		rxq = eth_dev->data->rx_queues[i];
> > +		if (rxq) {
> 
> Compare vs NULL
> 
> > +			rte_free(rxq->rxbufs);
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_rx_ring", i);
> > +			rte_free(rxq);
> > +		}
> > +	}
> > +
> > +	rte_free(eth_dev->data->tx_queues);
> > +	rte_free(eth_dev->data->rx_queues);
> > +	rte_free(eth_dev->data);
> > +	rte_free(eth_dev);
> > +}
> > +
> > +static int
> > +nfp_flower_init_ctrl_vnic(struct nfp_net_hw *hw) {
> > +	uint32_t i;
> > +	int ret = 0;
> > +	uint16_t nb_desc;
> > +	unsigned int numa_node;
> > +	struct rte_mempool *mp;
> > +	uint16_t rx_free_thresh;
> > +	uint16_t tx_free_thresh;
> > +	struct nfp_net_rxq *rxq;
> > +	struct nfp_net_txq *txq;
> > +	struct nfp_pf_dev *pf_dev;
> > +	struct rte_eth_dev *eth_dev;
> > +	const struct rte_memzone *tz;
> > +	struct nfp_app_flower *app_flower;
> > +
> > +	/* Hardcoded values for now */
> > +	nb_desc = CTRL_VNIC_NB_DESC;
> > +	rx_free_thresh = CTRL_VNIC_RX_FREE_THRESH;
> 
> What's the point to introduce the variable and use it only once below?
> 
> > +	tx_free_thresh = CTRL_VNIC_TX_FREE_THRESH;
> 
> Same here.
> 
> > +	numa_node = rte_socket_id();
> > +
> > +	/* Set up some pointers here for ease of use */
> > +	pf_dev = hw->pf_dev;
> > +	app_flower = NFP_APP_PRIV_TO_APP_FLOWER(pf_dev->app_priv);
> > +
> > +	ret = nfp_flower_init_vnic_common(hw, "ctrl_vnic");
> > +	if (ret)
> 
> Compare vs 0
> 
> > +		goto done;
> > +
> > +	/* Allocate memory for the eth_dev of the vNIC */
> > +	hw->eth_dev = rte_zmalloc("ctrl_vnic_eth_dev",
> 
> Why not rte_eth_dev_allocate()? Isn't an ethdev?
> Why do you bypsss ethdev layer in this case completely and do everything
> yourself?

Here we created an ethdev locally to nfp PMD, we want the user totally won't be aware of it.
If we use rte_eth_dev_allocate() to create it, it will be in array 'rte_ethdev_devices[]', that's not we want.

> 
> > +		sizeof(struct rte_eth_dev), RTE_CACHE_LINE_SIZE);
> > +	if (hw->eth_dev == NULL) {
> > +		ret = -ENOMEM;
> > +		goto done;
> > +	}
> > +
> > +	/* Grab the pointer to the newly created rte_eth_dev here */
> > +	eth_dev = hw->eth_dev;
> > +
> > +	/* Also allocate memory for the data part of the eth_dev */
> > +	eth_dev->data = rte_zmalloc("ctrl_vnic_eth_dev_data",
> > +		sizeof(struct rte_eth_dev_data), RTE_CACHE_LINE_SIZE);
> > +	if (eth_dev->data == NULL) {
> > +		ret = -ENOMEM;
> > +		goto eth_dev_cleanup;
> > +	}
> > +
> > +	eth_dev->data->rx_queues = rte_zmalloc("ethdev->rx_queues",
> > +		sizeof(eth_dev->data->rx_queues[0]) * hw-
> >max_rx_queues,
> > +		RTE_CACHE_LINE_SIZE);
> > +	if (eth_dev->data->rx_queues == NULL) {
> > +		PMD_INIT_LOG(ERR, "rte_zmalloc failed for ctrl vnic rx
> queues");
> > +		ret = -ENOMEM;
> > +		goto dev_data_cleanup;
> > +	}
> > +
> > +	eth_dev->data->tx_queues = rte_zmalloc("ethdev->tx_queues",
> > +		sizeof(eth_dev->data->tx_queues[0]) * hw-
> >max_tx_queues,
> > +		RTE_CACHE_LINE_SIZE);
> > +	if (eth_dev->data->tx_queues == NULL) {
> > +		PMD_INIT_LOG(ERR, "rte_zmalloc failed for ctrl vnic tx
> queues");
> > +		ret = -ENOMEM;
> > +		goto rx_queue_cleanup;
> > +	}
> > +
> > +	eth_dev->device = &pf_dev->pci_dev->device;
> > +	eth_dev->data->nb_tx_queues = hw->max_tx_queues;
> > +	eth_dev->data->nb_rx_queues = hw->max_rx_queues;
> > +	eth_dev->data->dev_private = hw;
> > +
> > +	/* Create a mbuf pool for the vNIC */
> > +	app_flower->ctrl_pktmbuf_pool =
> rte_pktmbuf_pool_create("ctrl_mbuf_pool",
> > +		4 * nb_desc, 64, 0, 9216, numa_node);
> > +	if (app_flower->ctrl_pktmbuf_pool == NULL) {
> > +		PMD_INIT_LOG(ERR, "create mbuf pool for ctrl vnic failed");
> > +		ret = -ENOMEM;
> > +		goto tx_queue_cleanup;
> > +	}
> > +
> > +	mp = app_flower->ctrl_pktmbuf_pool;
> > +
> > +	/* Set up the Rx queues */
> > +	PMD_INIT_LOG(INFO, "Configuring flower ctrl vNIC Rx queue");
> > +	for (i = 0; i < hw->max_rx_queues; i++) {
> > +		/* Hardcoded number of desc to 64 */
> > +		rxq = rte_zmalloc_socket("ethdev RX queue",
> > +			sizeof(struct nfp_net_rxq), RTE_CACHE_LINE_SIZE,
> > +			numa_node);
> > +		if (rxq == NULL) {
> > +			PMD_DRV_LOG(ERR, "Error allocating rxq");
> > +			ret = -ENOMEM;
> > +			goto rx_queue_setup_cleanup;
> > +		}
> > +
> > +		eth_dev->data->rx_queues[i] = rxq;
> > +
> > +		/* Hw queues mapping based on firmware configuration */
> > +		rxq->qidx = i;
> > +		rxq->fl_qcidx = i * hw->stride_rx;
> > +		rxq->rx_qcidx = rxq->fl_qcidx + (hw->stride_rx - 1);
> > +		rxq->qcp_fl = hw->rx_bar + NFP_QCP_QUEUE_OFF(rxq-
> >fl_qcidx);
> > +		rxq->qcp_rx = hw->rx_bar + NFP_QCP_QUEUE_OFF(rxq-
> >rx_qcidx);
> > +
> > +		/*
> > +		 * Tracking mbuf size for detecting a potential mbuf overflow
> due to
> > +		 * RX offset
> > +		 */
> > +		rxq->mem_pool = mp;
> > +		rxq->mbuf_size = rxq->mem_pool->elt_size;
> > +		rxq->mbuf_size -= (sizeof(struct rte_mbuf) +
> RTE_PKTMBUF_HEADROOM);
> > +		hw->flbufsz = rxq->mbuf_size;
> > +
> > +		rxq->rx_count = nb_desc;
> > +		rxq->rx_free_thresh = rx_free_thresh;
> > +		rxq->drop_en = 1;
> > +
> > +		/*
> > +		 * Allocate RX ring hardware descriptors. A memzone large
> enough to
> > +		 * handle the maximum ring size is allocated in order to allow
> for
> > +		 * resizing in later calls to the queue setup function.
> > +		 */
> > +		tz = rte_eth_dma_zone_reserve(eth_dev, "ctrl_rx_ring", i,
> > +			sizeof(struct nfp_net_rx_desc) *
> NFP_NET_MAX_RX_DESC,
> > +			NFP_MEMZONE_ALIGN, numa_node);
> > +		if (tz == NULL) {
> > +			PMD_DRV_LOG(ERR, "Error allocating rx dma");
> > +			rte_free(rxq);
> > +			ret = -ENOMEM;
> > +			goto rx_queue_setup_cleanup;
> > +		}
> > +
> > +		/* Saving physical and virtual addresses for the RX ring */
> > +		rxq->dma = (uint64_t)tz->iova;
> > +		rxq->rxds = (struct nfp_net_rx_desc *)tz->addr;
> > +
> > +		/* mbuf pointers array for referencing mbufs linked to RX
> descriptors */
> > +		rxq->rxbufs = rte_zmalloc_socket("rxq->rxbufs",
> > +			sizeof(*rxq->rxbufs) * nb_desc,
> RTE_CACHE_LINE_SIZE,
> > +			numa_node);
> > +		if (rxq->rxbufs == NULL) {
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_rx_ring", i);
> > +			rte_free(rxq);
> > +			ret = -ENOMEM;
> > +			goto rx_queue_setup_cleanup;
> > +		}
> > +
> > +		nfp_net_reset_rx_queue(rxq);
> > +
> > +		rxq->hw = hw;
> > +
> > +		/*
> > +		 * Telling the HW about the physical address of the RX ring
> and number
> > +		 * of descriptors in log2 format
> > +		 */
> > +		nn_cfg_writeq(hw, NFP_NET_CFG_RXR_ADDR(i), rxq->dma);
> > +		nn_cfg_writeb(hw, NFP_NET_CFG_RXR_SZ(i),
> rte_log2_u32(nb_desc));
> > +	}
> > +
> > +	/* Now the Tx queues */
> > +	PMD_INIT_LOG(INFO, "Configuring flower ctrl vNIC Tx queue");
> > +	for (i = 0; i < hw->max_tx_queues; i++) {
> > +		/* Hardcoded number of desc to 64 */
> > +		/* Allocating tx queue data structure */
> > +		txq = rte_zmalloc_socket("ethdev TX queue",
> > +			sizeof(struct nfp_net_txq), RTE_CACHE_LINE_SIZE,
> > +			numa_node);
> > +		if (txq == NULL) {
> > +			PMD_DRV_LOG(ERR, "Error allocating txq");
> > +			ret = -ENOMEM;
> > +			goto tx_queue_setup_cleanup;
> > +		}
> > +
> > +		eth_dev->data->tx_queues[i] = txq;
> > +
> > +		/*
> > +		 * Allocate TX ring hardware descriptors. A memzone large
> enough to
> > +		 * handle the maximum ring size is allocated in order to allow
> for
> > +		 * resizing in later calls to the queue setup function.
> > +		 */
> > +		tz = rte_eth_dma_zone_reserve(eth_dev, "ctrl_tx_ring", i,
> > +			sizeof(struct nfp_net_nfd3_tx_desc) *
> NFP_NET_MAX_TX_DESC,
> > +			NFP_MEMZONE_ALIGN, numa_node);
> > +		if (tz == NULL) {
> > +			PMD_DRV_LOG(ERR, "Error allocating tx dma");
> > +			rte_free(txq);
> > +			ret = -ENOMEM;
> > +			goto tx_queue_setup_cleanup;
> > +		}
> > +
> > +		txq->tx_count = nb_desc;
> > +		txq->tx_free_thresh = tx_free_thresh;
> > +		txq->tx_pthresh = DEFAULT_TX_PTHRESH;
> > +		txq->tx_hthresh = DEFAULT_TX_HTHRESH;
> > +		txq->tx_wthresh = DEFAULT_TX_WTHRESH;
> > +
> > +		/* queue mapping based on firmware configuration */
> > +		txq->qidx = i;
> > +		txq->tx_qcidx = i * hw->stride_tx;
> > +		txq->qcp_q = hw->tx_bar + NFP_QCP_QUEUE_OFF(txq-
> >tx_qcidx);
> > +
> > +		/* Saving physical and virtual addresses for the TX ring */
> > +		txq->dma = (uint64_t)tz->iova;
> > +		txq->txds = (struct nfp_net_nfd3_tx_desc *)tz->addr;
> > +
> > +		/* mbuf pointers array for referencing mbufs linked to TX
> descriptors */
> > +		txq->txbufs = rte_zmalloc_socket("txq->txbufs",
> > +			sizeof(*txq->txbufs) * nb_desc,
> RTE_CACHE_LINE_SIZE,
> > +			numa_node);
> > +		if (txq->txbufs == NULL) {
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_tx_ring", i);
> > +			rte_free(txq);
> > +			ret = -ENOMEM;
> > +			goto tx_queue_setup_cleanup;
> > +		}
> > +
> > +		nfp_net_reset_tx_queue(txq);
> > +
> > +		txq->hw = hw;
> > +
> > +		/*
> > +		 * Telling the HW about the physical address of the TX ring
> and number
> > +		 * of descriptors in log2 format
> > +		 */
> > +		nn_cfg_writeq(hw, NFP_NET_CFG_TXR_ADDR(i), txq->dma);
> > +		nn_cfg_writeb(hw, NFP_NET_CFG_TXR_SZ(i),
> rte_log2_u32(nb_desc));
> > +	}
> > +
> > +	return 0;
> > +
> > +tx_queue_setup_cleanup:
> > +	for (i = 0; i < hw->max_tx_queues; i++) {
> > +		txq = eth_dev->data->tx_queues[i];
> > +		if (txq) {
> 
> Compare vs NULL
> 
> > +			rte_free(txq->txbufs);
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_tx_ring", i);
> > +			rte_free(txq);
> > +		}
> > +	}
> > +rx_queue_setup_cleanup:
> > +	for (i = 0; i < hw->max_rx_queues; i++) {
> > +		rxq = eth_dev->data->rx_queues[i];
> > +		if (rxq) {
> 
> Compare vs NULL
> 
> > +			rte_free(rxq->rxbufs);
> > +			rte_eth_dma_zone_free(eth_dev, "ctrl_rx_ring", i);
> > +			rte_free(rxq);
> > +		}
> > +	}
> > +tx_queue_cleanup:
> > +	rte_free(eth_dev->data->tx_queues);
> > +rx_queue_cleanup:
> > +	rte_free(eth_dev->data->rx_queues);
> > +dev_data_cleanup:
> > +	rte_free(eth_dev->data);
> > +eth_dev_cleanup:
> > +	rte_free(eth_dev);
> > +done:
> > +	return ret;
> > +}
> > +
> >   static int
> >   nfp_flower_start_pf_vnic(struct nfp_net_hw *hw)
> >   {
> > @@ -561,12 +861,57 @@
> >   	return 0;
> >   }
> >
> > +static int
> > +nfp_flower_start_ctrl_vnic(struct nfp_net_hw *hw) {
> > +	int ret;
> > +	uint32_t update;
> > +	uint32_t new_ctrl;
> > +	struct rte_eth_dev *dev;
> > +
> > +	dev = hw->eth_dev;
> > +
> > +	/* Disabling queues just in case... */
> > +	nfp_net_disable_queues(dev);
> > +
> > +	/* Enabling the required queues in the device */
> > +	nfp_net_enable_queues(dev);
> > +
> > +	/* Writing configuration parameters in the device */
> > +	nfp_net_params_setup(hw);
> > +
> > +	new_ctrl = NFP_NET_CFG_CTRL_ENABLE;
> > +	update = NFP_NET_CFG_UPDATE_GEN |
> NFP_NET_CFG_UPDATE_RING |
> > +		 NFP_NET_CFG_UPDATE_MSIX;
> > +
> > +	rte_wmb();
> > +
> > +	/* If an error when reconfig we avoid to change hw state */
> > +	ret = nfp_net_reconfig(hw, new_ctrl, update);
> > +	if (ret) {
> 
> Compare vs 0
> 
> > +		PMD_INIT_LOG(ERR, "Failed to reconfig ctrl vnic");
> > +		return -EIO;
> > +	}
> > +
> > +	hw->ctrl = new_ctrl;
> > +
> > +	/* Setup the freelist ring */
> > +	ret = nfp_net_rx_freelist_setup(dev);
> > +	if (ret) {
> 
> Compare vs 0
> 
> > +		PMD_INIT_LOG(ERR, "Error with flower ctrl vNIC freelist
> setup");
> > +		return -EIO;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >   int
> >   nfp_init_app_flower(struct nfp_pf_dev *pf_dev)
> >   {
> >   	int ret;
> >   	unsigned int numa_node;
> >   	struct nfp_net_hw *pf_hw;
> > +	struct nfp_net_hw *ctrl_hw;
> >   	struct nfp_app_flower *app_flower;
> >
> >   	numa_node = rte_socket_id();
> > @@ -612,29 +957,63 @@
> >   	pf_hw->pf_dev = pf_dev;
> >   	pf_hw->cpp = pf_dev->cpp;
> >
> > +	/* The ctrl vNIC struct comes directly after the PF one */
> > +	app_flower->ctrl_hw = pf_hw + 1;
> > +	ctrl_hw = app_flower->ctrl_hw;
> > +
> > +	/* Map the ctrl vNIC ctrl bar */
> > +	ctrl_hw->ctrl_bar = nfp_rtsym_map(pf_dev->sym_tbl,
> "_pf0_net_ctrl_bar",
> > +		32768, &ctrl_hw->ctrl_area);
> > +	if (ctrl_hw->ctrl_bar == NULL) {
> > +		PMD_INIT_LOG(ERR, "Cloud not map the ctrl vNIC ctrl bar");
> > +		ret = -ENODEV;
> > +		goto pf_cpp_area_cleanup;
> > +	}
> > +
> > +	/* Now populate the ctrl vNIC */
> > +	ctrl_hw->pf_dev = pf_dev;
> > +	ctrl_hw->cpp = pf_dev->cpp;
> > +
> >   	ret = nfp_flower_init_pf_vnic(app_flower->pf_hw);
> >   	if (ret) {
> >   		PMD_INIT_LOG(ERR, "Could not initialize flower PF vNIC");
> > -		goto pf_cpp_area_cleanup;
> > +		goto ctrl_cpp_area_cleanup;
> > +	}
> > +
> > +	ret = nfp_flower_init_ctrl_vnic(app_flower->ctrl_hw);
> > +	if (ret) {
> 
> Compare vs 0
> 
> > +		PMD_INIT_LOG(ERR, "Could not initialize flower ctrl vNIC");
> > +		goto pf_vnic_cleanup;
> >   	}
> >
> >   	/* Start the PF vNIC */
> >   	ret = nfp_flower_start_pf_vnic(app_flower->pf_hw);
> >   	if (ret) {
> >   		PMD_INIT_LOG(ERR, "Could not start flower PF vNIC");
> > -		goto pf_vnic_cleanup;
> > +		goto ctrl_vnic_cleanup;
> > +	}
> > +
> > +	/* Start the ctrl vNIC */
> > +	ret = nfp_flower_start_ctrl_vnic(app_flower->ctrl_hw);
> > +	if (ret) {
> 
> Compare vs 0
> 
> > +		PMD_INIT_LOG(ERR, "Could not start flower ctrl vNIC");
> > +		goto ctrl_vnic_cleanup;
> >   	}
> >
> >   	/* Start up flower services */
> >   	if (nfp_flower_enable_services(app_flower)) {
> >   		ret = -ESRCH;
> > -		goto pf_vnic_cleanup;
> > +		goto ctrl_vnic_cleanup;
> >   	}
> >
> >   	return 0;
> >
> > +ctrl_vnic_cleanup:
> > +	nfp_flower_cleanup_ctrl_vnic(app_flower->ctrl_hw);
> >   pf_vnic_cleanup:
> >   	nfp_flower_cleanup_pf_vnic(app_flower->pf_hw);
> > +ctrl_cpp_area_cleanup:
> > +	nfp_cpp_area_free(ctrl_hw->ctrl_area);
> >   pf_cpp_area_cleanup:
> >   	nfp_cpp_area_free(pf_dev->ctrl_area);
> >   eth_tbl_cleanup:
> > diff --git a/drivers/net/nfp/flower/nfp_flower.h
> > b/drivers/net/nfp/flower/nfp_flower.h
> > index f6fd4eb..f11ef6d 100644
> > --- a/drivers/net/nfp/flower/nfp_flower.h
> > +++ b/drivers/net/nfp/flower/nfp_flower.h
> > @@ -21,6 +21,12 @@ struct nfp_app_flower {
> >   	/* Pointer to the PF vNIC */
> >   	struct nfp_net_hw *pf_hw;
> >
> > +	/* Pointer to a mempool for the ctrlvNIC */
> > +	struct rte_mempool *ctrl_pktmbuf_pool;
> > +
> > +	/* Pointer to the ctrl vNIC */
> > +	struct nfp_net_hw *ctrl_hw;
> > +
> >   	/* the eth table as reported by firmware */
> >   	struct nfp_eth_table *nfp_eth_table;
> >   };