Re: [dpdk-dev] [PATCH v6 7/9] ethdev: new API to get representor info

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>Sent: Monday, February 15, 2021 4:50 PM
>To: Xueming(Steven) Li <xuemingl@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viacheslavo@nvidia.com>; Asaf Penso <asafp@nvidia.com>; NBU-Contact-Thomas Monjalon
><thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
><nhorman@tuxdriver.com>
>Subject: Re: [PATCH v6 7/9] ethdev: new API to get representor info
>
>On 2/14/21 6:21 AM, Xueming Li wrote:
>> The NIC can have multiple PCIe links and can be attached to multiple
>> hosts, for example the same single NIC can be shared for multiple
>> server units in the rack. On each PCIe link NIC can provide multiple
>> PFs and VFs/SFs based on these ones. The full representor identifier
>> consists of three indices - controller index, PF index, and VF or SF index (if any).
>>
>> This patch introduces a new API rte_eth_representor_info_get() to
>> retrieve representor corresponding info mapping:
>>  - caller controller index and pf index.
>>  - supported representor ID ranges.
>>  - type, controller, pf and start vf/sf ID of each range.
>> The API is useful to convert representor from devargs to representor ID.
>>
>> New ethdev callback representor_info_get() is added to retrieve info
>> from PMD driver, optional for PMD that doesn't support new devargs
>> representor syntax.
>>
>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>
>LGTM, except minor notes below.
>
>> ---
>>  lib/librte_ethdev/ethdev_driver.h |  6 +++++
>>  lib/librte_ethdev/rte_ethdev.c    | 14 ++++++++++
>>  lib/librte_ethdev/rte_ethdev.h    | 43 +++++++++++++++++++++++++++++++
>>  lib/librte_ethdev/version.map     |  3 +++
>>  4 files changed, 66 insertions(+)
>>
>> diff --git a/lib/librte_ethdev/ethdev_driver.h
>> b/lib/librte_ethdev/ethdev_driver.h
>> index 06ff35266f..abcbc3112d 100644
>> --- a/lib/librte_ethdev/ethdev_driver.h
>> +++ b/lib/librte_ethdev/ethdev_driver.h
>> @@ -289,6 +289,10 @@ typedef int (*eth_fw_version_get_t)(struct rte_eth_dev *dev,
>>  				     char *fw_version, size_t fw_size);  /**< @internal Get
>> firmware information of an Ethernet device. */
>>
>> +typedef int (*eth_representor_info_get_t)(struct rte_eth_dev *dev,
>> +	struct rte_eth_representor_info *info); /**< @internal Get
>> +representor type and ID range. */
>> +
>>  typedef int (*eth_tx_done_cleanup_t)(void *txq, uint32_t free_cnt);
>> /**< @internal Force mbufs to be from TX ring. */
>>
>> @@ -823,6 +827,8 @@ struct eth_dev_ops {
>>  	eth_burst_mode_get_t       rx_burst_mode_get; /**< Get RX burst mode */
>>  	eth_burst_mode_get_t       tx_burst_mode_get; /**< Get TX burst mode */
>>  	eth_fw_version_get_t       fw_version_get; /**< Get firmware version. */
>> +	eth_representor_info_get_t representor_info_get;
>> +	/**< Get representor info. */
>
>Why is it added in the middle of the ops structure?

Looks like here group of function that getting information. Will move to end.

>
>>  	eth_dev_supported_ptypes_get_t dev_supported_ptypes_get;
>>  	/**< Get packet types supported and identified by device. */
>>  	eth_dev_ptypes_set_t dev_ptypes_set; diff --git
>> a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
>> index fe9466a03e..07c6debb58 100644
>> --- a/lib/librte_ethdev/rte_ethdev.c
>> +++ b/lib/librte_ethdev/rte_ethdev.c
>> @@ -3265,6 +3265,20 @@ rte_eth_dev_fw_version_get(uint16_t port_id, char *fw_version, size_t fw_size)
>>  							fw_version, fw_size));
>>  }
>>
>> +int
>> +rte_eth_representor_info_get(uint16_t port_id,
>> +			     struct rte_eth_representor_info *info) {
>> +	struct rte_eth_dev *dev;
>> +
>> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
>> +	dev = &rte_eth_devices[port_id];
>> +
>> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->fw_version_get, -ENOTSUP);
>
>I guess you mean to check representor_info_get here.

Thanks, my bad.

>
>> +	return eth_err(port_id, (*dev->dev_ops->representor_info_get)(dev,
>> +								      info));
>> +}
>> +
>>  int
>>  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info
>> *dev_info)  { diff --git a/lib/librte_ethdev/rte_ethdev.h
>> b/lib/librte_ethdev/rte_ethdev.h index 9cd519bf59..35eb0a5721 100644
>> --- a/lib/librte_ethdev/rte_ethdev.h
>> +++ b/lib/librte_ethdev/rte_ethdev.h
>> @@ -1581,6 +1581,30 @@ struct rte_eth_dev_info {
>>  	void *reserved_ptrs[2];   /**< Reserved for future fields */
>>  };
>>
>> +/**
>> + * @warning
>> + * @b EXPERIMENTAL: this structure may change without prior notice.
>> + *
>> + * Ethernet device representor information  */ struct
>> +rte_eth_representor_info {
>> +	uint16_t controller; /**< Controller ID of caller device. */
>> +	uint16_t pf; /**< Physical function ID of caller device. */
>> +	struct {
>> +		enum rte_eth_representor_type type; /**< Representor type */
>> +		int controller; /**< Controller ID, -1 to ignore */
>> +		int pf; /**< Physical function ID, -1 to ignore */
>> +		__extension__
>> +		union {
>> +			int vf; /**< VF start index */
>> +			int sf; /**< SF start index */
>> +		};
>> +		uint16_t id_base; /**< Representor ID start index */
>> +		uint16_t id_end;  /**< Representor ID end index */
>> +		char name[RTE_DEV_NAME_MAX_LEN];	/**< Representor name */
>> +	} ranges[]; /**< Representor ID range by type */
>
>I'm pretty sure that you need separate type for the structure when you add support, since you need to allocate memory and calculate
>required size.

Sizeof(info.ranges[0]) works, but splitting looks a good idea :)

>
>> +};
>> +
>>  /**
>>   * Ethernet device RX queue information structure.
>>   * Used to retrieve information about configured queue.
>> @@ -3038,6 +3062,25 @@ int rte_eth_macaddr_get(uint16_t port_id, struct rte_ether_addr *mac_addr);
>>   */
>>  int rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info
>> *dev_info);
>>
>> +/**
>> + * Retrieve the representor info of the device.
>> + *
>> + * @param port_id
>> + *   The port identifier of the device.
>> + * @param info
>> + *   A pointer to a representor info structure.
>> + *   NULL to return number of range entries and allocate memory
>> + *   for next call to store detail.
>> + * @return
>> + *   - (-ENOTSUP) if operation is not supported.
>> + *   - (-ENODEV) if *port_id* invalid.
>> + *   - (-EIO) if device is removed.
>> + *   - (>=0) number of representor range entries supported by device.
>> + */
>> +__rte_experimental
>> +int rte_eth_representor_info_get(uint16_t port_id,
>> +				 struct rte_eth_representor_info *info);
>> +
>>  /**
>>   * Retrieve the firmware version of a device.
>>   *