Re: [dpdk-dev] [PATCH v5 12/24] net/ngbe: add info get operation

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 6/2/21 12:40 PM, Jiawen Wu wrote:
> Add device information get operation.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@trustnetic.com>
> ---
>   doc/guides/nics/features/ngbe.ini |  1 +
>   drivers/net/ngbe/meson.build      |  1 +
>   drivers/net/ngbe/ngbe_ethdev.c    | 86 +++++++++++++++++++++++++++++++
>   drivers/net/ngbe/ngbe_ethdev.h    | 26 ++++++++++
>   drivers/net/ngbe/ngbe_rxtx.c      | 67 ++++++++++++++++++++++++
>   drivers/net/ngbe/ngbe_rxtx.h      | 15 ++++++
>   6 files changed, 196 insertions(+)
>   create mode 100644 drivers/net/ngbe/ngbe_rxtx.c
>   create mode 100644 drivers/net/ngbe/ngbe_rxtx.h
> 
> diff --git a/doc/guides/nics/features/ngbe.ini b/doc/guides/nics/features/ngbe.ini
> index 977286ac04..ca03a255de 100644
> --- a/doc/guides/nics/features/ngbe.ini
> +++ b/doc/guides/nics/features/ngbe.ini
> @@ -4,6 +4,7 @@
>   ; Refer to default.ini for the full list of available PMD features.
>   ;
>   [Features]
> +Speed capabilities   = Y
>   Multiprocess aware   = Y
>   Linux                = Y
>   ARMv8                = Y
> diff --git a/drivers/net/ngbe/meson.build b/drivers/net/ngbe/meson.build
> index 81173fa7f0..9e75b82f1c 100644
> --- a/drivers/net/ngbe/meson.build
> +++ b/drivers/net/ngbe/meson.build
> @@ -12,6 +12,7 @@ objs = [base_objs]
>   
>   sources = files(
>   	'ngbe_ethdev.c',
> +	'ngbe_rxtx.c',
>   )
>   
>   includes += include_directories('base')
> diff --git a/drivers/net/ngbe/ngbe_ethdev.c b/drivers/net/ngbe/ngbe_ethdev.c
> index e9ddbe9753..07df677b64 100644
> --- a/drivers/net/ngbe/ngbe_ethdev.c
> +++ b/drivers/net/ngbe/ngbe_ethdev.c
> @@ -9,6 +9,7 @@
>   #include "ngbe_logs.h"
>   #include "base/ngbe.h"
>   #include "ngbe_ethdev.h"
> +#include "ngbe_rxtx.h"
>   
>   static int ngbe_dev_close(struct rte_eth_dev *dev);
>   
> @@ -31,6 +32,22 @@ static const struct rte_pci_id pci_id_ngbe_map[] = {
>   	{ .vendor_id = 0, /* sentinel */ },
>   };
>   
> +static const struct rte_eth_desc_lim rx_desc_lim = {
> +	.nb_max = NGBE_RING_DESC_MAX,
> +	.nb_min = NGBE_RING_DESC_MIN,
> +	.nb_align = NGBE_RXD_ALIGN,
> +};
> +
> +static const struct rte_eth_desc_lim tx_desc_lim = {
> +	.nb_max = NGBE_RING_DESC_MAX,
> +	.nb_min = NGBE_RING_DESC_MIN,
> +	.nb_align = NGBE_TXD_ALIGN,
> +	.nb_seg_max = NGBE_TX_MAX_SEG,
> +	.nb_mtu_seg_max = NGBE_TX_MAX_SEG,
> +};
> +
> +static const struct eth_dev_ops ngbe_eth_dev_ops;
> +
>   /*
>    * Ensure that all locks are released before first NVM or PHY access
>    */
> @@ -64,6 +81,8 @@ eth_ngbe_dev_init(struct rte_eth_dev *eth_dev, void *init_params __rte_unused)
>   
>   	PMD_INIT_FUNC_TRACE();
>   
> +	eth_dev->dev_ops = &ngbe_eth_dev_ops;
> +
>   	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>   		return 0;
>   
> @@ -206,6 +225,73 @@ ngbe_dev_close(struct rte_eth_dev *dev)
>   	return 0;
>   }
>   
> +static int
> +ngbe_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
> +{
> +	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> +	struct ngbe_hw *hw = NGBE_DEV_HW(dev);
> +
> +	dev_info->max_rx_queues = (uint16_t)hw->mac.max_rx_queues;
> +	dev_info->max_tx_queues = (uint16_t)hw->mac.max_tx_queues;
> +	dev_info->min_rx_bufsize = 1024;
> +	dev_info->max_rx_pktlen = 15872;
> +	dev_info->max_mac_addrs = hw->mac.num_rar_entries;

Is it 1 or something else? If something else, it should be reported
when you actually support it.

> +	dev_info->max_hash_mac_addrs = NGBE_VMDQ_NUM_UC_MAC;

It should be set when actually supported.

> +	dev_info->max_vfs = pci_dev->max_vfs;

Again it should be reported when you can actually
enable and support VFs.

> +	dev_info->max_vmdq_pools = ETH_64_POOLS;

Same, when you implement supoort in dev_configure

> +	dev_info->vmdq_queue_num = dev_info->max_rx_queues;

Same

> +	dev_info->rx_queue_offload_capa = ngbe_get_rx_queue_offloads(dev);
> +	dev_info->rx_offload_capa = (ngbe_get_rx_port_offloads(dev) |
> +				     dev_info->rx_queue_offload_capa);
> +	dev_info->tx_queue_offload_capa = 0;
> +	dev_info->tx_offload_capa = ngbe_get_tx_port_offloads(dev);

Offloads must be reported when actually supported.
I.e. when PMD user can really request the offload, use it and
it will work.

> +
> +	dev_info->default_rxconf = (struct rte_eth_rxconf) {
> +		.rx_thresh = {
> +			.pthresh = NGBE_DEFAULT_RX_PTHRESH,
> +			.hthresh = NGBE_DEFAULT_RX_HTHRESH,
> +			.wthresh = NGBE_DEFAULT_RX_WTHRESH,
> +		},
> +		.rx_free_thresh = NGBE_DEFAULT_RX_FREE_THRESH,
> +		.rx_drop_en = 0,
> +		.offloads = 0,
> +	};
> +
> +	dev_info->default_txconf = (struct rte_eth_txconf) {
> +		.tx_thresh = {
> +			.pthresh = NGBE_DEFAULT_TX_PTHRESH,
> +			.hthresh = NGBE_DEFAULT_TX_HTHRESH,
> +			.wthresh = NGBE_DEFAULT_TX_WTHRESH,
> +		},
> +		.tx_free_thresh = NGBE_DEFAULT_TX_FREE_THRESH,
> +		.offloads = 0,
> +	};

It makes sense to report some values to above fields when
you actually take them into account on configure stage.

> +
> +	dev_info->rx_desc_lim = rx_desc_lim;

It belongs to the patch which implements Rx queue setup.

> +	dev_info->tx_desc_lim = tx_desc_lim;

It belongs to the patch which implements Tx queue setup.

> +
> +	dev_info->hash_key_size = NGBE_HKEY_MAX_INDEX * sizeof(uint32_t);
> +	dev_info->reta_size = ETH_RSS_RETA_SIZE_128;
> +	dev_info->flow_type_rss_offloads = NGBE_RSS_OFFLOAD_ALL;
> +
> +	dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
> +	dev_info->speed_capa |= ETH_LINK_SPEED_100M;
> +
> +	/* Driver-preferred Rx/Tx parameters */
> +	dev_info->default_rxportconf.burst_size = 32;
> +	dev_info->default_txportconf.burst_size = 32;
> +	dev_info->default_rxportconf.nb_queues = 1;
> +	dev_info->default_txportconf.nb_queues = 1;
> +	dev_info->default_rxportconf.ring_size = 256;
> +	dev_info->default_txportconf.ring_size = 256;

Basically it is misleading to report any kind of information
which is not actually supported. So, all above lines belong
to patches which actually support it.

> +
> +	return 0;
> +}
> +
> +static const struct eth_dev_ops ngbe_eth_dev_ops = {
> +	.dev_infos_get              = ngbe_dev_info_get,
> +};
> +
>   RTE_PMD_REGISTER_PCI(net_ngbe, rte_ngbe_pmd);
>   RTE_PMD_REGISTER_PCI_TABLE(net_ngbe, pci_id_ngbe_map);
>   RTE_PMD_REGISTER_KMOD_DEP(net_ngbe, "* igb_uio | uio_pci_generic | vfio-pci");
> diff --git a/drivers/net/ngbe/ngbe_ethdev.h b/drivers/net/ngbe/ngbe_ethdev.h
> index 5917ff02aa..b4e2000dd3 100644
> --- a/drivers/net/ngbe/ngbe_ethdev.h
> +++ b/drivers/net/ngbe/ngbe_ethdev.h
> @@ -6,6 +6,19 @@
>   #ifndef _NGBE_ETHDEV_H_
>   #define _NGBE_ETHDEV_H_
>   
> +#define NGBE_HKEY_MAX_INDEX 10
> +
> +#define NGBE_RSS_OFFLOAD_ALL ( \
> +	ETH_RSS_IPV4 | \
> +	ETH_RSS_NONFRAG_IPV4_TCP | \
> +	ETH_RSS_NONFRAG_IPV4_UDP | \
> +	ETH_RSS_IPV6 | \
> +	ETH_RSS_NONFRAG_IPV6_TCP | \
> +	ETH_RSS_NONFRAG_IPV6_UDP | \
> +	ETH_RSS_IPV6_EX | \
> +	ETH_RSS_IPV6_TCP_EX | \
> +	ETH_RSS_IPV6_UDP_EX)
> +
>   /*
>    * Structure to store private data for each driver instance (for each port).
>    */
> @@ -21,4 +34,17 @@ struct ngbe_adapter {
>   
>   #define NGBE_VMDQ_NUM_UC_MAC         4096 /* Maximum nb. of UC MAC addr. */
>   
> +/*
> + *  Default values for RX/TX configuration
> + */
> +#define NGBE_DEFAULT_RX_FREE_THRESH  32
> +#define NGBE_DEFAULT_RX_PTHRESH      8
> +#define NGBE_DEFAULT_RX_HTHRESH      8
> +#define NGBE_DEFAULT_RX_WTHRESH      0
> +
> +#define NGBE_DEFAULT_TX_FREE_THRESH  32
> +#define NGBE_DEFAULT_TX_PTHRESH      32
> +#define NGBE_DEFAULT_TX_HTHRESH      0
> +#define NGBE_DEFAULT_TX_WTHRESH      0
> +
>   #endif /* _NGBE_ETHDEV_H_ */
> diff --git a/drivers/net/ngbe/ngbe_rxtx.c b/drivers/net/ngbe/ngbe_rxtx.c
> new file mode 100644
> index 0000000000..ae24367b18
> --- /dev/null
> +++ b/drivers/net/ngbe/ngbe_rxtx.c
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018-2020 Beijing WangXun Technology Co., Ltd.
> + * Copyright(c) 2010-2017 Intel Corporation
> + */
> +
> +#include <stdint.h>
> +#include <rte_ethdev.h>
> +
> +#include "base/ngbe.h"
> +#include "ngbe_ethdev.h"
> +#include "ngbe_rxtx.h"
> +
> +uint64_t
> +ngbe_get_tx_port_offloads(struct rte_eth_dev *dev)
> +{
> +	uint64_t tx_offload_capa;
> +	struct ngbe_hw *hw = NGBE_DEV_HW(dev);
> +
> +	tx_offload_capa =
> +		DEV_TX_OFFLOAD_VLAN_INSERT |
> +		DEV_TX_OFFLOAD_IPV4_CKSUM  |
> +		DEV_TX_OFFLOAD_UDP_CKSUM   |
> +		DEV_TX_OFFLOAD_TCP_CKSUM   |
> +		DEV_TX_OFFLOAD_SCTP_CKSUM  |
> +		DEV_TX_OFFLOAD_TCP_TSO     |
> +		DEV_TX_OFFLOAD_UDP_TSO	   |
> +		DEV_TX_OFFLOAD_UDP_TNL_TSO	|
> +		DEV_TX_OFFLOAD_IP_TNL_TSO	|
> +		DEV_TX_OFFLOAD_IPIP_TNL_TSO	|
> +		DEV_TX_OFFLOAD_MULTI_SEGS;
> +
> +	if (hw->is_pf)
> +		tx_offload_capa |= DEV_TX_OFFLOAD_QINQ_INSERT;
> +
> +	tx_offload_capa |= DEV_TX_OFFLOAD_OUTER_IPV4_CKSUM;
> +
> +	return tx_offload_capa;
> +}
> +
> +uint64_t
> +ngbe_get_rx_queue_offloads(struct rte_eth_dev *dev __rte_unused)
> +{
> +	return DEV_RX_OFFLOAD_VLAN_STRIP;
> +}
> +
> +uint64_t
> +ngbe_get_rx_port_offloads(struct rte_eth_dev *dev)
> +{
> +	uint64_t offloads;
> +	struct ngbe_hw *hw = NGBE_DEV_HW(dev);
> +
> +	offloads = DEV_RX_OFFLOAD_IPV4_CKSUM  |
> +		   DEV_RX_OFFLOAD_UDP_CKSUM   |
> +		   DEV_RX_OFFLOAD_TCP_CKSUM   |
> +		   DEV_RX_OFFLOAD_KEEP_CRC    |
> +		   DEV_RX_OFFLOAD_JUMBO_FRAME |
> +		   DEV_RX_OFFLOAD_VLAN_FILTER |
> +		   DEV_RX_OFFLOAD_SCATTER;
> +
> +	if (hw->is_pf)
> +		offloads |= (DEV_RX_OFFLOAD_VLAN_FILTER |
> +			     DEV_RX_OFFLOAD_QINQ_STRIP |
> +			     DEV_RX_OFFLOAD_VLAN_EXTEND);
> +
> +	return offloads;
> +}
> +
> diff --git a/drivers/net/ngbe/ngbe_rxtx.h b/drivers/net/ngbe/ngbe_rxtx.h
> new file mode 100644
> index 0000000000..39011ee286
> --- /dev/null
> +++ b/drivers/net/ngbe/ngbe_rxtx.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright(c) 2018-2020 Beijing WangXun Technology Co., Ltd.
> + * Copyright(c) 2010-2017 Intel Corporation
> + */
> +
> +#ifndef _NGBE_RXTX_H_
> +#define _NGBE_RXTX_H_
> +
> +#define NGBE_TX_MAX_SEG                    40
> +
> +uint64_t ngbe_get_tx_port_offloads(struct rte_eth_dev *dev);
> +uint64_t ngbe_get_rx_queue_offloads(struct rte_eth_dev *dev);
> +uint64_t ngbe_get_rx_port_offloads(struct rte_eth_dev *dev);
> +
> +#endif /* _NGBE_RXTX_H_ */
>