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

[Ferruh Yigit]

On 10/4/2017 2:19 PM, Tomasz Duszynski wrote:
> On Wed, Oct 04, 2017 at 01:28:47AM +0100, Ferruh Yigit wrote:
>> 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?
> 
> That config option is used both by NET and CRYPTO drivers. In case NET
> and CRYPTO are used together i.e ipsec-secgw then DMA_MEMSIZE must be
> the set to the same size. Putting this configuration option in .config
> makes sure DMA_MEMSIZE stays synchronized.

OK.

> 
>>
>> <...>
>>
>>> +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?
> 
> All other drivers with external dependencies follow the same approach.
> 
>>
>> 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?
> 
> Both solutions are reasonable. The former was chosen because that's what the
> other drivers do.

OK.

> 
>>
>>> +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.
>>
> 
> ACK
> 
>>> +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
> 
> Moving that to LDFLAGS will break compilation in case
> CONFIG_RTE_BUILD_SHARED_LIB is set as -L... does not show up on command
> line thus linker does not know where to look extra library up.
> I may be wrong but it looks as if specifying LDFLAGS in driver's
> Makefile is no-op.

I would expect LDFLAGS will work, but if it is breaking the build,
please keep as it is, we can check and fix this later.

> 
> On the other hand, if we are building static libraries both
> -lmusdk and -L$(LIBMUSDK_PATH)/lib are added to specific _LDLIBS which in turn
> ends up in 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.
>>
> 
> ACK
> 
>>> +
>>> +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?
> 
> It's used for mbuf harvesting in tx-path. Instead of releasing pushed
> out mbuf to mempool and allocating it once again later on, mbuf is
> stored in the shadow queue and returned back to hardware buffer manager
> after being sent.
> 
>>
>>> + */
>>> +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.
>>
> 
> Keeping match locally is fine. That's used to tell MUSDK which physical
> port to configure, i.e ppio-0:1 means to configure port 1 on packet
> processor 0. Armada 8k has to such packet processor, while armada 7k
> only one.

Ok, thanks for clarification.

> 
>> <...>
>>
>>> +
>>> +	if (rte_spinlock_trylock(&q->priv->lock) == 1) {
>>
>> Why getting lock in Rx data path?
>>
> 
> In multi-core and multi-queue case some kind of protection is
> necessary so that several cores cannot modify bpool at
> the same time.
> 
>>> +		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?
>>
> 
> Right, documentation is missing for that. Will add in v4.
> 
>>> +	}
>>> +
>>> +	/*
>>> +	 * 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.
>>
> 
> Currently there's not need to passing separate options to each created
> device. As for configuration file it handles all devices at once.

Ok, that was an option...

> 
>>> +		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?
> 
> It's just to limit number of entries in mrvl_port_bpool_size we iterate
> over every time we want to count the total number of buffers in the
> hardware buffer pool.
> 
>>
>> <...>
>>
> 
> --
> - Tomasz Duszyński
>