>-----Original Message-----
>From: Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>
>Sent: Monday, February 15, 2021 4:50 PM
>To: Xueming(Steven) Li <xuemi...@nvidia.com>
>Cc: dev@dpdk.org; Slava Ovsiienko <viachesl...@nvidia.com>; Asaf Penso
><as...@nvidia.com>; NBU-Contact-Thomas Monjalon
><tho...@monjalon.net>; Ferruh Yigit <ferruh.yi...@intel.com>; Ray Kinsella
><m...@ashroe.eu>; Neil Horman
><nhor...@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 <xuemi...@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.
>> *
