[dpdk-dev] [PATCH 01/32] net/ngbe: add packet type

[Ferruh Yigit]

On 9/8/2021 9:37 AM, Jiawen Wu wrote:
> Add packet type marco definition and convert ptype to ptid.
> 
> Signed-off-by: Jiawen Wu <jiawenwu at trustnetic.com>
> ---
>  doc/guides/nics/features/ngbe.ini |   1 +
>  doc/guides/nics/ngbe.rst          |   1 +
>  drivers/net/ngbe/meson.build      |   1 +
>  drivers/net/ngbe/ngbe_ethdev.c    |   9 +
>  drivers/net/ngbe/ngbe_ethdev.h    |   4 +
>  drivers/net/ngbe/ngbe_ptypes.c    | 300 ++++++++++++++++++++++++++++++
>  drivers/net/ngbe/ngbe_ptypes.h    | 240 ++++++++++++++++++++++++
>  drivers/net/ngbe/ngbe_rxtx.c      |  16 ++
>  drivers/net/ngbe/ngbe_rxtx.h      |   2 +
>  9 files changed, 574 insertions(+)
>  create mode 100644 drivers/net/ngbe/ngbe_ptypes.c
>  create mode 100644 drivers/net/ngbe/ngbe_ptypes.h
> 
> diff --git a/doc/guides/nics/features/ngbe.ini b/doc/guides/nics/features/ngbe.ini
> index 08d5f1b0dc..8b7588184a 100644
> --- a/doc/guides/nics/features/ngbe.ini
> +++ b/doc/guides/nics/features/ngbe.ini
> @@ -8,6 +8,7 @@ Speed capabilities   = Y
>  Link status          = Y
>  Link status event    = Y
>  Queue start/stop     = Y
> +Packet type parsing  = Y

"Packet type parsing" also requires to support
'rte_eth_dev_get_supported_ptypes()' & 'rte_eth_dev_set_ptypes()' APIs.

Current implementation seems parses the packet type and updates mbuf field for
it but doesn't support above APIs, can you please add them too? There is already
'ngbe_dev_supported_ptypes_get()' function but dev_ops seems not set.

<...>

> +++ b/drivers/net/ngbe/ngbe_ptypes.c
> @@ -0,0 +1,300 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018-2021 Beijing WangXun Technology Co., Ltd.
> + */
> +
> +#include <rte_mbuf.h>
> +#include <rte_memory.h>
> +
> +#include "base/ngbe_type.h"
> +#include "ngbe_ptypes.h"
> +
> +/* The ngbe_ptype_lookup is used to convert from the 8-bit ptid in the
> + * hardware to a bit-field that can be used by SW to more easily determine the
> + * packet type.
> + *
> + * Macros are used to shorten the table lines and make this table human
> + * readable.
> + *
> + * We store the PTYPE in the top byte of the bit field - this is just so that
> + * we can check that the table doesn't have a row missing, as the index into
> + * the table should be the PTYPE.
> + */
> +#define TPTE(ptid, l2, l3, l4, tun, el2, el3, el4) \
> +	[ptid] = (RTE_PTYPE_L2_##l2 | \
> +		RTE_PTYPE_L3_##l3 | \
> +		RTE_PTYPE_L4_##l4 | \
> +		RTE_PTYPE_TUNNEL_##tun | \
> +		RTE_PTYPE_INNER_L2_##el2 | \
> +		RTE_PTYPE_INNER_L3_##el3 | \
> +		RTE_PTYPE_INNER_L4_##el4)
> +
> +#define RTE_PTYPE_L2_NONE               0
> +#define RTE_PTYPE_L3_NONE               0
> +#define RTE_PTYPE_L4_NONE               0
> +#define RTE_PTYPE_TUNNEL_NONE           0
> +#define RTE_PTYPE_INNER_L2_NONE         0
> +#define RTE_PTYPE_INNER_L3_NONE         0
> +#define RTE_PTYPE_INNER_L4_NONE         0

Why you are defining new PTYPEs? If these are for driver internal you can drop
the 'RTE_' prefix.

<...>

> +
> +#ifndef RTE_PTYPE_UNKNOWN
> +#define RTE_PTYPE_UNKNOWN                   0x00000000
> +#define RTE_PTYPE_L2_ETHER                  0x00000001
> +#define RTE_PTYPE_L2_ETHER_TIMESYNC         0x00000002
> +#define RTE_PTYPE_L2_ETHER_ARP              0x00000003
> +#define RTE_PTYPE_L2_ETHER_LLDP             0x00000004
> +#define RTE_PTYPE_L2_ETHER_NSH              0x00000005
> +#define RTE_PTYPE_L2_ETHER_FCOE             0x00000009
> +#define RTE_PTYPE_L3_IPV4                   0x00000010
> +#define RTE_PTYPE_L3_IPV4_EXT               0x00000030
> +#define RTE_PTYPE_L3_IPV6                   0x00000040
> +#define RTE_PTYPE_L3_IPV4_EXT_UNKNOWN       0x00000090
> +#define RTE_PTYPE_L3_IPV6_EXT               0x000000c0
> +#define RTE_PTYPE_L3_IPV6_EXT_UNKNOWN       0x000000e0
> +#define RTE_PTYPE_L4_TCP                    0x00000100
> +#define RTE_PTYPE_L4_UDP                    0x00000200
> +#define RTE_PTYPE_L4_FRAG                   0x00000300
> +#define RTE_PTYPE_L4_SCTP                   0x00000400
> +#define RTE_PTYPE_L4_ICMP                   0x00000500
> +#define RTE_PTYPE_L4_NONFRAG                0x00000600
> +#define RTE_PTYPE_TUNNEL_IP                 0x00001000
> +#define RTE_PTYPE_TUNNEL_GRE                0x00002000
> +#define RTE_PTYPE_TUNNEL_VXLAN              0x00003000
> +#define RTE_PTYPE_TUNNEL_NVGRE              0x00004000
> +#define RTE_PTYPE_TUNNEL_GENEVE             0x00005000
> +#define RTE_PTYPE_TUNNEL_GRENAT             0x00006000
> +#define RTE_PTYPE_INNER_L2_ETHER            0x00010000
> +#define RTE_PTYPE_INNER_L2_ETHER_VLAN       0x00020000
> +#define RTE_PTYPE_INNER_L3_IPV4             0x00100000
> +#define RTE_PTYPE_INNER_L3_IPV4_EXT         0x00200000
> +#define RTE_PTYPE_INNER_L3_IPV6             0x00300000
> +#define RTE_PTYPE_INNER_L3_IPV4_EXT_UNKNOWN 0x00400000
> +#define RTE_PTYPE_INNER_L3_IPV6_EXT         0x00500000
> +#define RTE_PTYPE_INNER_L3_IPV6_EXT_UNKNOWN 0x00600000
> +#define RTE_PTYPE_INNER_L4_TCP              0x01000000
> +#define RTE_PTYPE_INNER_L4_UDP              0x02000000
> +#define RTE_PTYPE_INNER_L4_FRAG             0x03000000
> +#define RTE_PTYPE_INNER_L4_SCTP             0x04000000
> +#define RTE_PTYPE_INNER_L4_ICMP             0x05000000
> +#define RTE_PTYPE_INNER_L4_NONFRAG          0x06000000
> +#endif /* !RTE_PTYPE_UNKNOWN */

These are already defined in the mbuf public header, why there are defined again?

<...>

> @@ -378,6 +389,10 @@ ngbe_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts,
>  		rxm->data_len = pkt_len;
>  		rxm->port = rxq->port_id;
>  
> +		pkt_info = rte_le_to_cpu_32(rxd.qw0.dw0);
> +		rxm->packet_type = ngbe_rxd_pkt_info_to_pkt_type(pkt_info,
> +						       rxq->pkt_type_mask);
> +
>  		/*
>  		 * Store the mbuf address into the next entry of the array
>  		 * of returned packets.
> @@ -799,6 +814,7 @@ ngbe_dev_rx_queue_setup(struct rte_eth_dev *dev,
>  	rxq->port_id = dev->data->port_id;
>  	rxq->drop_en = rx_conf->rx_drop_en;
>  	rxq->rx_deferred_start = rx_conf->rx_deferred_start;
> +	rxq->pkt_type_mask = NGBE_PTID_MASK;

What is the use of the 'pkt_type_mask', it seems it is a fixed value, why
keeping it per queue?