[dpdk-dev] [PATCH v3 2/4] net/mrvl: add mrvl net pmd driver
Ferruh Yigit
ferruh.yigit at intel.com
Thu Oct 5 19:37:26 CEST 2017
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
>
More information about the dev
mailing list