Re: [PATCH v6 4/8] ethdev: Add port representor device flag

[Shahaf Shuler <shahafs@mellanox.com>]

Thursday, March 29, 2018 5:53 PM, Doherty, Declan:
> On 29/03/2018 7:13 AM, Shahaf Shuler wrote:
> > Wednesday, March 28, 2018 4:54 PM, Declan Doherty:
> >> Subject: [dpdk-dev][PATCH v6 4/8] ethdev: Add port representor device
> >> flag
> >>
> >> Add new device flag to specify that ethdev port is a port representor.
> >> Extend rte_eth_dev_info structure to expose device flags to user
> >> which enable applications to discover if a port is a representor port.
> >>
> >> Signed-off-by: Declan Doherty <declan.doherty@intel.com>
> >> ---
> >>   lib/librte_ether/rte_ethdev.c             | 1 +
> >>   lib/librte_ether/rte_ethdev.h             | 9 ++++++---
> >>   lib/librte_ether/rte_ethdev_representor.h | 3 +++
> >>   3 files changed, 10 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/lib/librte_ether/rte_ethdev.c
> >> b/lib/librte_ether/rte_ethdev.c index c719f84a3..163246433 100644
> >> --- a/lib/librte_ether/rte_ethdev.c
> >> +++ b/lib/librte_ether/rte_ethdev.c
> >> @@ -2399,6 +2399,7 @@ rte_eth_dev_info_get(uint16_t port_id, struct
> >> rte_eth_dev_info *dev_info)
> >>   	dev_info->nb_rx_queues = dev->data->nb_rx_queues;
> >>   	dev_info->nb_tx_queues = dev->data->nb_tx_queues;
> >>   	dev_info->switch_id = dev->data->switch_id;
> >> +	dev_info->dev_flags = dev->data->dev_flags;
> >>   }
> >>
> >>   int
> >> diff --git a/lib/librte_ether/rte_ethdev.h
> >> b/lib/librte_ether/rte_ethdev.h index dced4fc41..226acc8b1 100644
> >> --- a/lib/librte_ether/rte_ethdev.h
> >> +++ b/lib/librte_ether/rte_ethdev.h
> >> @@ -996,6 +996,7 @@ struct rte_eth_dev_info {
> >>   	const char *driver_name; /**< Device Driver name. */
> >>   	unsigned int if_index; /**< Index to bound host interface, or 0 if
> >> none.
> >>   		Use if_indextoname() to translate into an interface name. */
> >> +	uint32_t dev_flags; /**< Device flags */
> >>   	uint32_t min_rx_bufsize; /**< Minimum size of RX buffer. */
> >>   	uint32_t max_rx_pktlen; /**< Maximum configurable length of RX
> >> pkt. */
> >>   	uint16_t max_rx_queues; /**< Maximum number of RX queues. */
> @@
> >> -1229,11 +1230,13 @@ struct rte_eth_dev_owner {  };
> >>
> >>   /** Device supports link state interrupt */
> >> -#define RTE_ETH_DEV_INTR_LSC     0x0002
> >> +#define RTE_ETH_DEV_INTR_LSC		0x0002
> >>   /** Device is a bonded slave */
> >> -#define RTE_ETH_DEV_BONDED_SLAVE 0x0004
> >> +#define RTE_ETH_DEV_BONDED_SLAVE	0x0004
> >>   /** Device supports device removal interrupt */
> >> -#define RTE_ETH_DEV_INTR_RMV     0x0008
> >> +#define RTE_ETH_DEV_INTR_RMV		0x0008
> >> +/** Device is port representor */
> >> +#define RTE_ETH_DEV_REPRESENTOR		0x0010
> >
> > Maybe it is a good time to make some order here.
> > I understand the decision to use flags instead of bit-field. It is better.
> >
> > However there is a mix here of device capabilities like :
> RTE_ETH_DEV_INTR_LSC   and RTE_ETH_DEV_INTR_RMV
> > And device attributes like : RTE_ETH_DEV_BONDED_SLAVE and
> RTE_ETH_DEV_REPRESENTOR.
> > I don't think they belong together under the genetic name of dev_flags.
> >
> > Moreover, I am not sure the fact device is bonded slave should be exposed
> to the application. It should be internal to ethdev and its port iterators.
> 
> That's a good point on the bonded slave flag, I'll look at fixing that for the
> next release. I don't think changing it should effect ABI but I'll need to have a
> closer look.
> 
> Do you think that we should have a separate device attributes field, which
> the representor flag is contained in.
> 
> >
> > Finally I think representor port may need more info now (and in the
> future), for example the associated vf id.
> > For that, I think it is better it to be exposed as a dedicated struct on device
> info.
> 
> I think a switch port id should suffice for that, for SR-IOV devices it would
> map to the vf_id.

I think we need both switch_domain and vf_id. 
Because for representors, the application should know which VFs can be reached from this representor and which VF it represent. 

> 
> >
> >>
> >>   /**
> >>    * @warning
> >> diff --git a/lib/librte_ether/rte_ethdev_representor.h
> >> b/lib/librte_ether/rte_ethdev_representor.h
> >> index cbc1f2855..f3726d0ba 100644
> >> --- a/lib/librte_ether/rte_ethdev_representor.h
> >> +++ b/lib/librte_ether/rte_ethdev_representor.h
> >> @@ -22,6 +22,9 @@ eth_dev_representor_port_init(struct rte_eth_dev
> >> *ethdev, void *init_params)
> >>   	/** representor inherits the switch id of it's base device */
> >>   	ethdev->data->switch_id = base_ethdev->data->switch_id;
> >>
> >> +	/** Set device flags to specify that device is a representor port */
> >> +	ethdev->data->dev_flags |= RTE_ETH_DEV_REPRESENTOR;
> >
> > Should be set in the PMD, not in ethdev layer
> 
> As in the previous patch this is just a generic port bus init function which
> meets the simplest use case of representor port with a single switch domain,
> a PMD doesn't need to use it but having it here saves duplicating the same
> code across multiple PMD which are only supporting the basic mode.
> 
> >
> >> +
> >>   	return 0;
> >>   }
> >>
> >> --
> >> 2.14.3
> >