[dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver

[Ferruh Yigit]

On 10/3/2017 12:51 PM, Tomasz Duszynski wrote:
> Add support for the Marvell PPv2 (Packet Processor v2) 1/10 Gbps adapter.
> Driver is based on external, publicly available, light-weight Marvell
> MUSDK library that provides access to network packet processor.
> 
> Driver comes with support for the following features:
> 
> * Speed capabilities
> * Link status
> * Queue start/stop
> * MTU update
> * Jumbo frame
> * Promiscuous mode
> * Allmulticast mode
> * Unicast MAC filter
> * Multicast MAC filter
> * RSS hash
> * VLAN filter
> * CRC offload
> * L3 checksum offload
> * L4 checksum offload
> * Packet type parsing
> * Basic stats
> * Stats per queue
> 
> Driver was engineered cooperatively by Semihalf and Marvell teams.
> 
> Semihalf:
> Jacek Siuda <jck at semihalf.com>
> Tomasz Duszynski <tdu at semihalf.com>
> 
> Marvell:
> Dmitri Epshtein <dima at marvell.com>
> Natalie Samsonov <nsamsono at marvell.com>
> 
> Signed-off-by: Jacek Siuda <jck at semihalf.com>
> Signed-off-by: Tomasz Duszynski <tdu at semihalf.com>

<...>

> +++ b/config/common_base
> @@ -262,6 +262,13 @@ CONFIG_RTE_LIBRTE_NFP_PMD=n
>  CONFIG_RTE_LIBRTE_NFP_DEBUG=n
> 
>  #
> +# Compile Marvell PMD driver
> +#
> +CONFIG_RTE_LIBRTE_MRVL_PMD=n
> +CONFIG_RTE_LIBRTE_MRVL_DEBUG=n
> +CONFIG_RTE_MRVL_MUSDK_DMA_MEMSIZE=41943040

Is dma memsize needs to be a configuration option?

<...>

> +include $(RTE_SDK)/mk/rte.vars.mk
> +
> +ifneq ($(MAKECMDGOALS),clean)
> +ifneq ($(MAKECMDGOALS),config)
> +ifeq ($(LIBMUSDK_PATH),)
> +$(error "Please define LIBMUSDK_PATH environment variable")

Not sure how to resolve this dependency.
What do you think adding this as configuration option?

Or DPDK just adds the -lmusdk external dependency and while compiling
for marvel EXTRA_LDFLAGS parameter should be pass with
"-L$(LIBMUSDK_PATH)" and this can be documented in marvel doc. What do
you think?

> +endif
> +ifeq ($(CONFIG_RTE_LIBRTE_CFGFILE),n)
> +$(error "RTE_LIBRTE_CFGFILE must be enabled in configuration!")

This can be also handled in drivers/net/Makefile, it can be possible to
add check there for LIBRTE_CFGFILE dependency.

> +endif
> +endif
> +endif
> +
> +# library name
> +LIB = librte_pmd_mrvl.a
> +
> +# library version
> +LIBABIVER := 1
> +
> +# versioning export map
> +EXPORT_MAP := rte_pmd_mrvl_version.map
> +
> +# external library dependencies
> +CFLAGS += -I$(LIBMUSDK_PATH)/include
> +CFLAGS += -DMVCONF_ARCH_DMA_ADDR_T_64BIT
> +CFLAGS += -DCONF_PP2_BPOOL_COOKIE_SIZE=32
> +CFLAGS += $(WERROR_FLAGS)
> +CFLAGS += -O3
> +LDLIBS += -L$(LIBMUSDK_PATH)/lib

This can be LDFLAGS instead of LDLIBS

> +LDLIBS += -lmusdk
> +
> +# library source files
> +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += mrvl_ethdev.c
> +SRCS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += mrvl_qos.c
> +
> +# library dependencies
> +DEPDIRS-$(CONFIG_RTE_LIBRTE_MRVL_PMD) += lib/librte_cfgfile

These variables no more used, you can drop this. drivers/net/Makefile
used for this, you are already updating that file, librte_cfgfile needs
to be added there.

> +
> +include $(RTE_SDK)/mk/rte.lib.mk

<...>

> +/*
> + * To use buffer harvesting based on loopback port shadow queue structure
> + * was introduced for buffers information bookkeeping.
> + *
> + * Before sending the packet, related buffer information (pp2_buff_inf) is
> + * stored in shadow queue. After packet is transmitted no longer used
> + * packet buffer is released back to it's original hardware pool,
> + * on condition it originated from interface.
> + * In case it  was generated by application itself i.e: mbuf->port field is
> + * 0xff then its released to software mempool.

You already explained here but can you please give more details why
shadow queue needed?

> + */
> +struct mrvl_shadow_txq {
> +	int head;           /* write index - used when sending buffers */
> +	int tail;           /* read index - used when releasing buffers */
> +	u16 size;           /* queue occupied size */
> +	u16 num_to_release; /* number of buffers sent, that can be released */
> +	struct buff_release_entry ent[MRVL_PP2_TX_SHADOWQ_SIZE]; /* q entries */
> +};
> +
> +struct mrvl_rxq {
> +	struct mrvl_priv *priv;
> +	struct rte_mempool *mp;
> +	int queue_id;
> +	int port_id;
> +	int cksum_enabled;
> +	uint64_t bytes_recv;
> +	uint64_t drop_mac;
> +};
> +
> +struct mrvl_txq {
> +	struct mrvl_priv *priv;
> +	int queue_id;
> +	int port_id;
> +	uint64_t bytes_sent;
> +};
> +

<...>

> +static int
> +mrvl_dev_start(struct rte_eth_dev *dev)
> +{
> +	struct mrvl_priv *priv = dev->data->dev_private;
> +	char match[MRVL_MATCH_LEN];
> +	int ret;
> +
> +	snprintf(match, sizeof(match), "ppio-%d:%d",
> +		 priv->pp_id, priv->ppio_id);
> +	priv->ppio_params.match = match;

Why this match is used, just a reminder that match is only valid for the
scope of this function, after this function it will be invalid.

<...>

> +
> +	if (rte_spinlock_trylock(&q->priv->lock) == 1) {

Why getting lock in Rx data path?

> +		num = mrvl_get_bpool_size(bpool->pp2_id, bpool->id);
> +
> +		if (unlikely(num <= q->priv->bpool_min_size ||
> +			     (!rx_done && num < q->priv->bpool_init_size))) {
> +			ret = mrvl_fill_bpool(q, MRVL_BURST_SIZE);
> +			if (ret)
> +				RTE_LOG(ERR, PMD, "Failed to fill bpool\n");
> +		} else if (unlikely(num > q->priv->bpool_max_size)) {
> +			int i;
> +			int pkt_to_remove = num - q->priv->bpool_init_size;
> +			struct rte_mbuf *mbuf;
> +			struct pp2_buff_inf buff;
> +
> +			RTE_LOG(DEBUG, PMD,
> +				"\nport-%d:%d: bpool %d oversize - remove %d buffers (pool size: %d -> %d)\n",
> +				bpool->pp2_id, q->priv->ppio->port_id,
> +				bpool->id, pkt_to_remove, num,
> +				q->priv->bpool_init_size);
> +
> +			for (i = 0; i < pkt_to_remove; i++) {
> +				pp2_bpool_get_buff(hifs[core_id], bpool, &buff);
> +				mbuf = (struct rte_mbuf *)
> +					(cookie_addr_high | buff.cookie);
> +				rte_pktmbuf_free(mbuf);
> +			}
> +			mrvl_port_bpool_size
> +				[bpool->pp2_id][bpool->id][core_id] -=
> +								pkt_to_remove;
> +		}
> +		rte_spinlock_unlock(&q->priv->lock);
> +	}
> +
> +	return rx_done;
> +}

<...>

> +	cfgnum = rte_kvargs_count(kvlist, MRVL_CFG_ARG);
> +	if (cfgnum > 1) {
> +		RTE_LOG(ERR, PMD, "Cannot handle more than one config file!\n");
> +		goto out_free_kvlist;
> +	} else if (cfgnum == 1) {
> +		rte_kvargs_process(kvlist, MRVL_CFG_ARG,
> +				   mrvl_get_qoscfg, &mrvl_qos_cfg);

Is the expected format/contect of the config file documented? How one
can know how to create a config file?

> +	}
> +
> +	/*
> +	 * ret == -EEXIST is correct, it means DMA
> +	 * has been already initialized (by another PMD).
> +	 */
> +	ret = mv_sys_dma_mem_init(RTE_MRVL_MUSDK_DMA_MEMSIZE);
> +	if (ret < 0 && ret != -EEXIST)
> +		goto out_free_kvlist;
> +
> +	ret = mrvl_init_pp2();
> +	if (ret) {
> +		RTE_LOG(ERR, PMD, "Failed to init PP!\n");
> +		goto out_deinit_dma;
> +	}
> +
> +	ret = mrvl_init_hifs();
> +	if (ret)
> +		goto out_deinit_hifs;
> +
> +	for (i = 0; i < ifnum; i++) {
> +		RTE_LOG(INFO, PMD, "Creating %s\n", ifnames[i]);
> +		ret = mrvl_eth_dev_create(vdev, ifnames[i]);

So you are supporting multiple ethdev devices created by single vdev
device, by providing multiple "iface" argument in device args.

This will cause eal create single virtual device but driver create
multiple ethdev devices. I don't see direct problem with this but lets
think about it.
This can be problem if you want to provide ethdev specific device
arguments. Perhaps that is why you need to provide a config file ?

It can be an option to define each ethdev with:
"--vdev net_mrvl0,iface=xx0,config=yy0 --vdev
net_mrlv1,iface=xx1,config=yy1 ..."

This may remove your dependecy to librte_cfgfile.

> +		if (ret)
> +			goto out_cleanup;
> +	}
> +
> +	rte_kvargs_free(kvlist);
> +
> +	memset(mrvl_port_bpool_size, 0, sizeof(mrvl_port_bpool_size));
> +
> +	mrvl_lcore_first = RTE_MAX_LCORE;
> +	mrvl_lcore_last = 0;
> +
> +	RTE_LCORE_FOREACH(core_id) {
> +		mrvl_set_first_last_cores(core_id);

This sets limits of core_id. Why you need to know this in PMD level?

<...>