[dpdk-dev,RFC,v2,3/5] librte_ether: add API's for VF management

  Add new API functions to configure and manage VF's on a NIC.

add rte_eth_dev_vf_ping function.
add rte_eth_dev_set_vf_vlan_anti_spoof function.
add rte_eth_dev_set_vf_mac_anti_spoof function.

Signed-off-by: azelezniak <alexz@att.com>

add rte_eth_dev_set_vf_vlan_strip function.
add rte_eth_dev_set_vf_vlan_insert function.
add rte_eth_dev_set_loopback function.
add rte_eth_dev_set_all_queues_drop function.
add rte_eth_dev_set_vf_split_drop_en function
add rte_eth_dev_set_vf_mac_addr function.
increment LIBABIVER to 5.

Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_ethdev.c          | 159 +++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.h          | 223 +++++++++++++++++++++++++++++++++
 lib/librte_ether/rte_ether_version.map |   9 ++
 3 files changed, 391 insertions(+)
  

On Fri, Aug 26, 2016 at 10:10:18AM +0100, Bernard Iremonger wrote:
> Add new API functions to configure and manage VF's on a NIC.
> 
> add rte_eth_dev_vf_ping function.
> add rte_eth_dev_set_vf_vlan_anti_spoof function.
> add rte_eth_dev_set_vf_mac_anti_spoof function.
> 
> Signed-off-by: azelezniak <alexz@att.com>
> 
> add rte_eth_dev_set_vf_vlan_strip function.
> add rte_eth_dev_set_vf_vlan_insert function.
> add rte_eth_dev_set_loopback function.
> add rte_eth_dev_set_all_queues_drop function.
> add rte_eth_dev_set_vf_split_drop_en function
> add rte_eth_dev_set_vf_mac_addr function.

Do we really need to expose VF specific functions here?
It can be generic(PF/VF) function indexed only through port_id.
(example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id, uint8_t on))
For instance, In Thunderx PMD, We are not exposing a separate port_id
for PF. We only enumerate 0..N VFs as 0..N ethdev port_id

> increment LIBABIVER to 5.
> 
> Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c          | 159 +++++++++++++++++++++++
>  lib/librte_ether/rte_ethdev.h          | 223 +++++++++++++++++++++++++++++++++
>  lib/librte_ether/rte_ether_version.map |   9 ++
>  3 files changed, 391 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 1388ea3..2a3d2ae 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -2306,6 +2306,22 @@ rte_eth_dev_default_mac_addr_set(uint8_t port_id, struct ether_addr *addr)
>  }
>  
>  int
> +rte_eth_dev_set_vf_mac_addr(uint8_t port_id, uint16_t vf, struct ether_addr *addr)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	if (!is_valid_assigned_ether_addr(addr))
> +		return -EINVAL;
> +
> +	dev = &rte_eth_devices[port_id];
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_mac_addr, -ENOTSUP);
> +
> +	return (*dev->dev_ops->set_vf_mac_addr)(dev, vf, addr);
> +}
> +
> +int
>  rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
>  				uint16_t rx_mode, uint8_t on)
>  {
> @@ -2490,6 +2506,149 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port_id, uint16_t vlan_id,
>  						   vf_mask, vlan_on);
>  }
>  
> +int
> +rte_eth_dev_set_vf_vlan_anti_spoof(uint8_t port_id,
> +			       uint16_t vf, uint8_t on)
> +{
> +	struct rte_eth_dev *dev;
> +
> +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> +
> +	dev = &rte_eth_devices[port_id];
> +	if (vf > 63) {

PMD may have more than 64 VFs.


> +		RTE_PMD_DEBUG_TRACE("VF VLAN anti spoof:VF %d > 63\n", vf);
> +		return -EINVAL;
> +	}
> +
> +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_vlan_anti_spoof, -ENOTSUP);
> +	(*dev->dev_ops->set_vf_vlan_anti_spoof)(dev, vf, on);
> +	return 0;
> +}
> +
  

Hi Jerin,

<snip>

> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> On Fri, Aug 26, 2016 at 10:10:18AM +0100, Bernard Iremonger wrote:
> > Add new API functions to configure and manage VF's on a NIC.
> >
> > add rte_eth_dev_vf_ping function.
> > add rte_eth_dev_set_vf_vlan_anti_spoof function.
> > add rte_eth_dev_set_vf_mac_anti_spoof function.
> >
> > Signed-off-by: azelezniak <alexz@att.com>
> >
> > add rte_eth_dev_set_vf_vlan_strip function.
> > add rte_eth_dev_set_vf_vlan_insert function.
> > add rte_eth_dev_set_loopback function.
> > add rte_eth_dev_set_all_queues_drop function.
> > add rte_eth_dev_set_vf_split_drop_en function add
> > rte_eth_dev_set_vf_mac_addr function.
> 
> Do we really need to expose VF specific functions here?
> It can be generic(PF/VF) function indexed only through port_id.
> (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id, uint8_t on))
> For instance, In Thunderx PMD, We are not exposing a separate port_id for
> PF. We only enumerate 0..N VFs as 0..N ethdev port_id

Our intention with this patch is to control the VF from the PF.

The following librte_ether functions already work in a similar way:

rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t rx_mode, uint8_t on)

rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)

rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)

int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t tx_rate, uint64_t q_msk)

> 
> > increment LIBABIVER to 5.
> >
> > Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
> > ---
> >  lib/librte_ether/rte_ethdev.c          | 159 +++++++++++++++++++++++
> >  lib/librte_ether/rte_ethdev.h          | 223
> +++++++++++++++++++++++++++++++++
> >  lib/librte_ether/rte_ether_version.map |   9 ++
> >  3 files changed, 391 insertions(+)
> >
> > diff --git a/lib/librte_ether/rte_ethdev.c
> > b/lib/librte_ether/rte_ethdev.c index 1388ea3..2a3d2ae 100644
> > --- a/lib/librte_ether/rte_ethdev.c
> > +++ b/lib/librte_ether/rte_ethdev.c
> > @@ -2306,6 +2306,22 @@ rte_eth_dev_default_mac_addr_set(uint8_t
> > port_id, struct ether_addr *addr)  }
> >
> >  int
> > +rte_eth_dev_set_vf_mac_addr(uint8_t port_id, uint16_t vf, struct
> > +ether_addr *addr) {
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> > +	if (!is_valid_assigned_ether_addr(addr))
> > +		return -EINVAL;
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_mac_addr, -
> ENOTSUP);
> > +
> > +	return (*dev->dev_ops->set_vf_mac_addr)(dev, vf, addr); }
> > +
> > +int
> >  rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> >  				uint16_t rx_mode, uint8_t on)
> >  {
> > @@ -2490,6 +2506,149 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t
> port_id, uint16_t vlan_id,
> >  						   vf_mask, vlan_on);
> >  }
> >
> > +int
> > +rte_eth_dev_set_vf_vlan_anti_spoof(uint8_t port_id,
> > +			       uint16_t vf, uint8_t on)
> > +{
> > +	struct rte_eth_dev *dev;
> > +
> > +	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > +
> > +	dev = &rte_eth_devices[port_id];
> > +	if (vf > 63) {
> 
> PMD may have more than 64 VFs.

Yes, it would be better to check on max_vfs,  the same way as the already implemented functions mentioned above.
 
> 
> 
> > +		RTE_PMD_DEBUG_TRACE("VF VLAN anti spoof:VF %d >
> 63\n", vf);
> > +		return -EINVAL;
> > +	}
> > +
> > +	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops-
> >set_vf_vlan_anti_spoof, -ENOTSUP);
> > +	(*dev->dev_ops->set_vf_vlan_anti_spoof)(dev, vf, on);
> > +	return 0;
> > +}
> > +

Thanks for reviewing.
Regards,

Bernard.
  

2016-09-12 16:28, Iremonger, Bernard:
> > On Fri, Aug 26, 2016 at 10:10:18AM +0100, Bernard Iremonger wrote:
> > > Add new API functions to configure and manage VF's on a NIC.
> > >
> > > add rte_eth_dev_vf_ping function.
> > > add rte_eth_dev_set_vf_vlan_anti_spoof function.
> > > add rte_eth_dev_set_vf_mac_anti_spoof function.
> > >
> > > Signed-off-by: azelezniak <alexz@att.com>
> > >
> > > add rte_eth_dev_set_vf_vlan_strip function.
> > > add rte_eth_dev_set_vf_vlan_insert function.
> > > add rte_eth_dev_set_loopback function.
> > > add rte_eth_dev_set_all_queues_drop function.
> > > add rte_eth_dev_set_vf_split_drop_en function add
> > > rte_eth_dev_set_vf_mac_addr function.
> > 
> > Do we really need to expose VF specific functions here?
> > It can be generic(PF/VF) function indexed only through port_id.
> > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id, uint8_t on))
> > For instance, In Thunderx PMD, We are not exposing a separate port_id for
> > PF. We only enumerate 0..N VFs as 0..N ethdev port_id
> 
> Our intention with this patch is to control the VF from the PF.
> 
> The following librte_ether functions already work in a similar way:
> 
> rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t rx_mode, uint8_t on)
> 
> rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> 
> rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> 
> int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t tx_rate, uint64_t q_msk)

I have a bad feeling with these functions dedicated to VF from PF.
Are we sure there is no other way?
I mean we just need to know the VF with a port ID.
  

Hi Thomas,

<snip>

> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-12 16:28, Iremonger, Bernard:
> > > On Fri, Aug 26, 2016 at 10:10:18AM +0100, Bernard Iremonger wrote:
> > > > Add new API functions to configure and manage VF's on a NIC.
> > > >
> > > > add rte_eth_dev_vf_ping function.
> > > > add rte_eth_dev_set_vf_vlan_anti_spoof function.
> > > > add rte_eth_dev_set_vf_mac_anti_spoof function.
> > > >
> > > > Signed-off-by: azelezniak <alexz@att.com>
> > > >
> > > > add rte_eth_dev_set_vf_vlan_strip function.
> > > > add rte_eth_dev_set_vf_vlan_insert function.
> > > > add rte_eth_dev_set_loopback function.
> > > > add rte_eth_dev_set_all_queues_drop function.
> > > > add rte_eth_dev_set_vf_split_drop_en function add
> > > > rte_eth_dev_set_vf_mac_addr function.
> > >
> > > Do we really need to expose VF specific functions here?
> > > It can be generic(PF/VF) function indexed only through port_id.
> > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id,
> > > uint8_t on)) For instance, In Thunderx PMD, We are not exposing a
> > > separate port_id for PF. We only enumerate 0..N VFs as 0..N ethdev
> > > port_id
> >
> > Our intention with this patch is to control the VF from the PF.
> >
> > The following librte_ether functions already work in a similar way:
> >
> > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t
> > rx_mode, uint8_t on)
> >
> > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> >
> > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> >
> > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t
> > tx_rate, uint64_t q_msk)
> 
> I have a bad feeling with these functions dedicated to VF from PF.
> Are we sure there is no other way?
> I mean we just need to know the VF with a port ID.

When the VF is used in a VM the port ID of the VF is not visible to the PF.
I don't think there is another way to do this.

Regards,

Bernard.
  

2016-09-15 16:46, Iremonger, Bernard:
> > > > Do we really need to expose VF specific functions here?
> > > > It can be generic(PF/VF) function indexed only through port_id.
> > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id,
> > > > uint8_t on)) For instance, In Thunderx PMD, We are not exposing a
> > > > separate port_id for PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > port_id
> > >
> > > Our intention with this patch is to control the VF from the PF.
> > >
> > > The following librte_ether functions already work in a similar way:
> > >
> > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t
> > > rx_mode, uint8_t on)
> > >
> > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> > >
> > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> > >
> > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t
> > > tx_rate, uint64_t q_msk)
> > 
> > I have a bad feeling with these functions dedicated to VF from PF.
> > Are we sure there is no other way?
> > I mean we just need to know the VF with a port ID.
> 
> When the VF is used in a VM the port ID of the VF is not visible to the PF.
> I don't think there is another way to do this.

I don't understand why we could not assign a port id to the VF from the
host instead of having the couple PF port id / VF id.
Can we enumerate all the VFs associated to a PF?
Then can we allocate them a port id in the array rte_eth_devices?
  

On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> 2016-09-15 16:46, Iremonger, Bernard:
> > > > > Do we really need to expose VF specific functions here?
> > > > > It can be generic(PF/VF) function indexed only through port_id.
> > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id,
> > > > > uint8_t on)) For instance, In Thunderx PMD, We are not exposing a
> > > > > separate port_id for PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > > port_id
> > > >
> > > > Our intention with this patch is to control the VF from the PF.
> > > >
> > > > The following librte_ether functions already work in a similar way:
> > > >
> > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t
> > > > rx_mode, uint8_t on)
> > > >
> > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > >
> > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > >
> > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t
> > > > tx_rate, uint64_t q_msk)
> > > 
> > > I have a bad feeling with these functions dedicated to VF from PF.
> > > Are we sure there is no other way?
> > > I mean we just need to know the VF with a port ID.
> > 
> > When the VF is used in a VM the port ID of the VF is not visible to the PF.
> > I don't think there is another way to do this.
> 
> I don't understand why we could not assign a port id to the VF from the
> host instead of having the couple PF port id / VF id.
> Can we enumerate all the VFs associated to a PF?
> Then can we allocate them a port id in the array rte_eth_devices?

Hi Thomas,

The VF is not a port visible to DPDK, though, so it shouldn't have a port id
IMHO. DPDK can't actually do anything with it.

The PCI device for the VF is likely passed through to a different VM and being
used there. Unfortunately, the VF still needs certain things done for it by the
PF, so if the PF is under DPDK control, it needs to provide the functionality
to assist the VF.

/Bruce
  

2016-09-23 10:20, Bruce Richardson:
> On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > Do we really need to expose VF specific functions here?
> > > > > > It can be generic(PF/VF) function indexed only through port_id.
> > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id,
> > > > > > uint8_t on)) For instance, In Thunderx PMD, We are not exposing a
> > > > > > separate port_id for PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > > > port_id
> > > > >
> > > > > Our intention with this patch is to control the VF from the PF.
> > > > >
> > > > > The following librte_ether functions already work in a similar way:
> > > > >
> > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t
> > > > > rx_mode, uint8_t on)
> > > > >
> > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > > >
> > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > > >
> > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t
> > > > > tx_rate, uint64_t q_msk)
> > > > 
> > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > Are we sure there is no other way?
> > > > I mean we just need to know the VF with a port ID.
> > > 
> > > When the VF is used in a VM the port ID of the VF is not visible to the PF.
> > > I don't think there is another way to do this.
> > 
> > I don't understand why we could not assign a port id to the VF from the
> > host instead of having the couple PF port id / VF id.
> > Can we enumerate all the VFs associated to a PF?
> > Then can we allocate them a port id in the array rte_eth_devices?
> 
> Hi Thomas,
> 
> The VF is not a port visible to DPDK, though, so it shouldn't have a port id
> IMHO. DPDK can't actually do anything with it.

You say the contrary below.

> The PCI device for the VF is likely passed through to a different VM and being
> used there. Unfortunately, the VF still needs certain things done for it by the
> PF, so if the PF is under DPDK control, it needs to provide the functionality
> to assist the VF.

Why not have a VF_from_PF driver which does the mailbox things?
So you can manage the VF from the PF with a simple port id.
It really seems to be the cleanest design to me.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, September 23, 2016 10:36 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; dev@dpdk.org; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul R
> <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; azelezniak
> <alexz@att.com>
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-23 10:20, Bruce Richardson:
> > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > It can be generic(PF/VF) function indexed only through
> port_id.
> > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD, We are
> > > > > > > not exposing a separate port_id for PF. We only enumerate
> > > > > > > 0..N VFs as 0..N ethdev port_id
> > > > > >
> > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > >
> > > > > > The following librte_ether functions already work in a similar
> way:
> > > > > >
> > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> > > > > > uint16_t rx_mode, uint8_t on)
> > > > > >
> > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t
> > > > > > on)
> > > > > >
> > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t
> > > > > > on)
> > > > > >
> > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf,
> > > > > > uint16_t tx_rate, uint64_t q_msk)
> > > > >
> > > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > > Are we sure there is no other way?
> > > > > I mean we just need to know the VF with a port ID.
> > > >
> > > > When the VF is used in a VM the port ID of the VF is not visible to
> the PF.
> > > > I don't think there is another way to do this.
> > >
> > > I don't understand why we could not assign a port id to the VF from
> > > the host instead of having the couple PF port id / VF id.
> > > Can we enumerate all the VFs associated to a PF?
> > > Then can we allocate them a port id in the array rte_eth_devices?
> >
> > Hi Thomas,
> >
> > The VF is not a port visible to DPDK, though, so it shouldn't have a
> > port id IMHO. DPDK can't actually do anything with it.
> 
> You say the contrary below.

Well, yes and no. The driver can manipulate things for the VF, but DPDK doesn't actually have a device that corresponds to the VF. There are no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?
> 
> > The PCI device for the VF is likely passed through to a different VM
> > and being used there. Unfortunately, the VF still needs certain things
> > done for it by the PF, so if the PF is under DPDK control, it needs to
> > provide the functionality to assist the VF.
> 
> Why not have a VF_from_PF driver which does the mailbox things?
> So you can manage the VF from the PF with a simple port id.
> It really seems to be the cleanest design to me.

While I see your point, and it could work, I just want to be sure that we are ok with the results of that. Suppose we do create ethdevs for the VFs controlled by the PF. Does the new VF get counted in the rte_eth_dev_count() value (I assume yes)? How are apps meant to use the port? Do they have to put in a special case when iterating through all the port ids to check that it's not a pseudo port that can't do anything. None of the standard ethdev calls from an app will work on it, you can't configure nb rx/tx queues on it, you can't start or stop it, you can't do rx or tx on it, etc, etc.

/Bruce
  

On Fri, Sep 23, 2016 at 11:36:21AM +0200, Thomas Monjalon wrote:
> 2016-09-23 10:20, Bruce Richardson:
> > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > It can be generic(PF/VF) function indexed only through port_id.
> > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t port_id,
> > > > > > > uint8_t on)) For instance, In Thunderx PMD, We are not exposing a
> > > > > > > separate port_id for PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > > > > port_id
> > > > > >
> > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > >
> > > > > > The following librte_ether functions already work in a similar way:
> > > > > >
> > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf, uint16_t
> > > > > > rx_mode, uint8_t on)
> > > > > >
> > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > > > >
> > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t on)
> > > > > >
> > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf, uint16_t
> > > > > > tx_rate, uint64_t q_msk)
> > > > > 
> > > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > > Are we sure there is no other way?
> > > > > I mean we just need to know the VF with a port ID.
> > > > 
> > > > When the VF is used in a VM the port ID of the VF is not visible to the PF.
> > > > I don't think there is another way to do this.
> > > 
> > > I don't understand why we could not assign a port id to the VF from the
> > > host instead of having the couple PF port id / VF id.
> > > Can we enumerate all the VFs associated to a PF?
> > > Then can we allocate them a port id in the array rte_eth_devices?
> > 
> > Hi Thomas,
> > 
> > The VF is not a port visible to DPDK, though, so it shouldn't have a port id
> > IMHO. DPDK can't actually do anything with it.
> 
> You say the contrary below.
> 
> > The PCI device for the VF is likely passed through to a different VM and being
> > used there. Unfortunately, the VF still needs certain things done for it by the
> > PF, so if the PF is under DPDK control, it needs to provide the functionality
> > to assist the VF.
> 
> Why not have a VF_from_PF driver which does the mailbox things?
> So you can manage the VF from the PF with a simple port id.
> It really seems to be the cleanest design to me.

Just to confirm I am understanding your suggestion correctly:

* for a normal app, eg. testpmd or l2fwd, things stay exactly as they are
* for an app that wants to control VFs from PFs, then we provide a new 
  ethdev driver, and a call in the PF to create an new instance of it 
  e.g. rte_eth_vf_from_pf(pf_port_id, vf_number)
* then to control the VF, you make regular rte_ethdev calls using the new
  ethdev port id you got from the vf_from_pf call.
* it's up to that app to know what ports are regular ports that can do IO,
  and what ports are VF control ports, though we may add in an ethdev flag value
  somewhere to assist in this.

Is that basically it?

/Bruce
  

2016-09-23 09:53, Richardson, Bruce:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-09-23 10:20, Bruce Richardson:
> > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > > It can be generic(PF/VF) function indexed only through
> > port_id.
> > > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD, We are
> > > > > > > > not exposing a separate port_id for PF. We only enumerate
> > > > > > > > 0..N VFs as 0..N ethdev port_id
> > > > > > >
> > > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > > >
> > > > > > > The following librte_ether functions already work in a similar
> > way:
> > > > > > >
> > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> > > > > > > uint16_t rx_mode, uint8_t on)
> > > > > > >
> > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf, uint8_t
> > > > > > > on)
> > > > > > >
> > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf, uint8_t
> > > > > > > on)
> > > > > > >
> > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t vf,
> > > > > > > uint16_t tx_rate, uint64_t q_msk)
> > > > > >
> > > > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > > > Are we sure there is no other way?
> > > > > > I mean we just need to know the VF with a port ID.
> > > > >
> > > > > When the VF is used in a VM the port ID of the VF is not visible to
> > the PF.
> > > > > I don't think there is another way to do this.
> > > >
> > > > I don't understand why we could not assign a port id to the VF from
> > > > the host instead of having the couple PF port id / VF id.
> > > > Can we enumerate all the VFs associated to a PF?
> > > > Then can we allocate them a port id in the array rte_eth_devices?
> > >
> > > Hi Thomas,
> > >
> > > The VF is not a port visible to DPDK, though, so it shouldn't have a
> > > port id IMHO. DPDK can't actually do anything with it.
> > 
> > You say the contrary below.
> 
> Well, yes and no. The driver can manipulate things for the VF, but DPDK doesn't actually have a device that corresponds to the VF. There are no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?

Very good point.
There are only few ethdev functions which are supported by every drivers,
like Rx/Tx and would not be available for VF from PF interface.

> > > The PCI device for the VF is likely passed through to a different VM
> > > and being used there. Unfortunately, the VF still needs certain things
> > > done for it by the PF, so if the PF is under DPDK control, it needs to
> > > provide the functionality to assist the VF.
> > 
> > Why not have a VF_from_PF driver which does the mailbox things?
> > So you can manage the VF from the PF with a simple port id.
> > It really seems to be the cleanest design to me.
> 
> While I see your point, and it could work, I just want to be sure that we are ok with the results of that. Suppose we do create ethdevs for the VFs controlled by the PF. Does the new VF get counted in the rte_eth_dev_count() value (I assume yes)? How are apps meant to use the port? Do they have to put in a special case when iterating through all the port ids to check that it's not a pseudo port that can't do anything. None of the standard ethdev calls from an app will work on it, you can't configure nb rx/tx queues on it, you can't start or stop it, you can't do rx or tx on it, etc, etc.

Yes these devices would be special because their supported API would be
quite different. I was thinking that in the future you could add most
of the configuration functions through the VF mailbox.
But the Intel mailbox currently support only some special configurations
which are not supported by other devices even its own VF device (except
setting MAC address).
And when I read "set drop enable bit in the VF split rx control register",
it becomes clear it is really specific and has nothing to do in the
generic ethdev API.
That's why it is a NACK.

When we want to use these very specific features we are aware of the
underlying device and driver. So we can directly include a header from
the driver. I suggest to retrieve a handler for the device which is not
a port id and will allow to call ixgbe functions directly.
It could be achieved by adding an ethdev function like discussed here:
	http://dpdk.org/ml/archives/dev/2016-September/047392.html
  

Hi Thoms

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Friday, September 23, 2016 2:15 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Iremonger, Bernard
> <bernard.iremonger@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Shah,
> Rahul R <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> azelezniak <alexz@att.com>
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-23 09:53, Richardson, Bruce:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-09-23 10:20, Bruce Richardson:
> > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > > > It can be generic(PF/VF) function indexed only through
> > > port_id.
> > > > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD, We
> > > > > > > > > are not exposing a separate port_id for PF. We only
> > > > > > > > > enumerate 0..N VFs as 0..N ethdev port_id
> > > > > > > >
> > > > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > > > >
> > > > > > > > The following librte_ether functions already work in a
> > > > > > > > similar
> > > way:
> > > > > > > >
> > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> > > > > > > > uint16_t rx_mode, uint8_t on)
> > > > > > > >
> > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf,
> > > > > > > > uint8_t
> > > > > > > > on)
> > > > > > > >
> > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf,
> > > > > > > > uint8_t
> > > > > > > > on)
> > > > > > > >
> > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t
> > > > > > > > vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > >
> > > > > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > > > > Are we sure there is no other way?
> > > > > > > I mean we just need to know the VF with a port ID.
> > > > > >
> > > > > > When the VF is used in a VM the port ID of the VF is not
> > > > > > visible to
> > > the PF.
> > > > > > I don't think there is another way to do this.
> > > > >
> > > > > I don't understand why we could not assign a port id to the VF
> > > > > from the host instead of having the couple PF port id / VF id.
> > > > > Can we enumerate all the VFs associated to a PF?
> > > > > Then can we allocate them a port id in the array rte_eth_devices?
> > > >
> > > > Hi Thomas,
> > > >
> > > > The VF is not a port visible to DPDK, though, so it shouldn't have
> > > > a port id IMHO. DPDK can't actually do anything with it.
> > >
> > > You say the contrary below.
> >
> > Well, yes and no. The driver can manipulate things for the VF, but DPDK
> doesn't actually have a device that corresponds to the VF. There are no PCI
> bar mappings for it, DPDK can't do RX and TX with it etc.?
> 
> Very good point.
> There are only few ethdev functions which are supported by every drivers,
> like Rx/Tx and would not be available for VF from PF interface.
> 
> > > > The PCI device for the VF is likely passed through to a different
> > > > VM and being used there. Unfortunately, the VF still needs certain
> > > > things done for it by the PF, so if the PF is under DPDK control,
> > > > it needs to provide the functionality to assist the VF.
> > >
> > > Why not have a VF_from_PF driver which does the mailbox things?
> > > So you can manage the VF from the PF with a simple port id.
> > > It really seems to be the cleanest design to me.
> >
> > While I see your point, and it could work, I just want to be sure that we are
> ok with the results of that. Suppose we do create ethdevs for the VFs
> controlled by the PF. Does the new VF get counted in the
> rte_eth_dev_count() value (I assume yes)? How are apps meant to use the
> port? Do they have to put in a special case when iterating through all the port
> ids to check that it's not a pseudo port that can't do anything. None of the
> standard ethdev calls from an app will work on it, you can't configure nb rx/tx
> queues on it, you can't start or stop it, you can't do rx or tx on it, etc, etc.
> 
> Yes these devices would be special because their supported API would be
> quite different. I was thinking that in the future you could add most of the
> configuration functions through the VF mailbox.
> But the Intel mailbox currently support only some special configurations
> which are not supported by other devices even its own VF device (except
> setting MAC address).
> And when I read "set drop enable bit in the VF split rx control register", it
> becomes clear it is really specific and has nothing to do in the generic ethdev
> API.
> That's why it is a NACK.
> 
> When we want to use these very specific features we are aware of the
> underlying device and driver. So we can directly include a header from the
> driver. I suggest to retrieve a handler for the device which is not a port id and
> will allow to call ixgbe functions directly.
> It could be achieved by adding an ethdev function like discussed here:
> 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> 

I have been reading the net/vhost mail thread above. The following quote is from this thread.

"It means I would be in favor of introducing API in drivers for very specific features."

At present all the PMD functions are accessed through the eth_dev_ops structure, there are no PMD API's.

Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a driver ioctl API) which can be accessed through a generic API in the ethdev?

What will this generic API look like?

Regards,

Bernard.
  

2016-09-23 17:02, Iremonger, Bernard:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-09-23 09:53, Richardson, Bruce:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > > > > It can be generic(PF/VF) function indexed only through
> > > > port_id.
> > > > > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD, We
> > > > > > > > > > are not exposing a separate port_id for PF. We only
> > > > > > > > > > enumerate 0..N VFs as 0..N ethdev port_id
> > > > > > > > >
> > > > > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > > > > >
> > > > > > > > > The following librte_ether functions already work in a
> > > > > > > > > similar
> > > > way:
> > > > > > > > >
> > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t vf,
> > > > > > > > > uint16_t rx_mode, uint8_t on)
> > > > > > > > >
> > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf,
> > > > > > > > > uint8_t
> > > > > > > > > on)
> > > > > > > > >
> > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf,
> > > > > > > > > uint8_t
> > > > > > > > > on)
> > > > > > > > >
> > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id, uint16_t
> > > > > > > > > vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > >
> > > > > > > > I have a bad feeling with these functions dedicated to VF from PF.
> > > > > > > > Are we sure there is no other way?
> > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > >
> > > > > > > When the VF is used in a VM the port ID of the VF is not
> > > > > > > visible to
> > > > the PF.
> > > > > > > I don't think there is another way to do this.
> > > > > >
> > > > > > I don't understand why we could not assign a port id to the VF
> > > > > > from the host instead of having the couple PF port id / VF id.
> > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > Then can we allocate them a port id in the array rte_eth_devices?
> > > > >
> > > > > Hi Thomas,
> > > > >
> > > > > The VF is not a port visible to DPDK, though, so it shouldn't have
> > > > > a port id IMHO. DPDK can't actually do anything with it.
> > > >
> > > > You say the contrary below.
> > >
> > > Well, yes and no. The driver can manipulate things for the VF, but DPDK
> > doesn't actually have a device that corresponds to the VF. There are no PCI
> > bar mappings for it, DPDK can't do RX and TX with it etc.?
> > 
> > Very good point.
> > There are only few ethdev functions which are supported by every drivers,
> > like Rx/Tx and would not be available for VF from PF interface.
> > 
> > > > > The PCI device for the VF is likely passed through to a different
> > > > > VM and being used there. Unfortunately, the VF still needs certain
> > > > > things done for it by the PF, so if the PF is under DPDK control,
> > > > > it needs to provide the functionality to assist the VF.
> > > >
> > > > Why not have a VF_from_PF driver which does the mailbox things?
> > > > So you can manage the VF from the PF with a simple port id.
> > > > It really seems to be the cleanest design to me.
> > >
> > > While I see your point, and it could work, I just want to be sure that we are
> > ok with the results of that. Suppose we do create ethdevs for the VFs
> > controlled by the PF. Does the new VF get counted in the
> > rte_eth_dev_count() value (I assume yes)? How are apps meant to use the
> > port? Do they have to put in a special case when iterating through all the port
> > ids to check that it's not a pseudo port that can't do anything. None of the
> > standard ethdev calls from an app will work on it, you can't configure nb rx/tx
> > queues on it, you can't start or stop it, you can't do rx or tx on it, etc, etc.
> > 
> > Yes these devices would be special because their supported API would be
> > quite different. I was thinking that in the future you could add most of the
> > configuration functions through the VF mailbox.
> > But the Intel mailbox currently support only some special configurations
> > which are not supported by other devices even its own VF device (except
> > setting MAC address).
> > And when I read "set drop enable bit in the VF split rx control register", it
> > becomes clear it is really specific and has nothing to do in the generic ethdev
> > API.
> > That's why it is a NACK.
> > 
> > When we want to use these very specific features we are aware of the
> > underlying device and driver. So we can directly include a header from the
> > driver. I suggest to retrieve a handler for the device which is not a port id and
> > will allow to call ixgbe functions directly.
> > It could be achieved by adding an ethdev function like discussed here:
> > 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> > 
> 
> I have been reading the net/vhost mail thread above. The following quote is from this thread.
> 
> "It means I would be in favor of introducing API in drivers for very specific features."
> 
> At present all the PMD functions are accessed through the eth_dev_ops structure, there are no PMD API's.
> 
> Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a driver ioctl API) which can be accessed through a generic API in the ethdev?

Not exactly. I'm thinking about a PMD specific API.
The only ethdev API you need would be a function to retrieve a handler
(an opaque pointer on the device struct) from the port id.
Then you can include rte_ixgbe.h and directly call the specific ixgbe
function, passing the device handler.
How does it sound?
  

Hi Thomas, Bruce,

<snip>

> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-23 17:02, Iremonger, Bernard:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-09-23 09:53, Richardson, Bruce:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > > > > > It can be generic(PF/VF) function indexed only
> > > > > > > > > > > through
> > > > > port_id.
> > > > > > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD,
> > > > > > > > > > > We are not exposing a separate port_id for PF. We
> > > > > > > > > > > only enumerate 0..N VFs as 0..N ethdev port_id
> > > > > > > > > >
> > > > > > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > > > > > >
> > > > > > > > > > The following librte_ether functions already work in a
> > > > > > > > > > similar
> > > > > way:
> > > > > > > > > >
> > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t
> > > > > > > > > > vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > >
> > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf,
> > > > > > > > > > uint8_t
> > > > > > > > > > on)
> > > > > > > > > >
> > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf,
> > > > > > > > > > uint8_t
> > > > > > > > > > on)
> > > > > > > > > >
> > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > > > > > > > > > uint16_t vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > > >
> > > > > > > > > I have a bad feeling with these functions dedicated to VF from
> PF.
> > > > > > > > > Are we sure there is no other way?
> > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > >
> > > > > > > > When the VF is used in a VM the port ID of the VF is not
> > > > > > > > visible to
> > > > > the PF.
> > > > > > > > I don't think there is another way to do this.
> > > > > > >
> > > > > > > I don't understand why we could not assign a port id to the
> > > > > > > VF from the host instead of having the couple PF port id / VF id.
> > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > Then can we allocate them a port id in the array rte_eth_devices?
> > > > > >
> > > > > > Hi Thomas,
> > > > > >
> > > > > > The VF is not a port visible to DPDK, though, so it shouldn't
> > > > > > have a port id IMHO. DPDK can't actually do anything with it.
> > > > >
> > > > > You say the contrary below.
> > > >
> > > > Well, yes and no. The driver can manipulate things for the VF, but
> > > > DPDK
> > > doesn't actually have a device that corresponds to the VF. There are
> > > no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?
> > >
> > > Very good point.
> > > There are only few ethdev functions which are supported by every
> > > drivers, like Rx/Tx and would not be available for VF from PF interface.
> > >
> > > > > > The PCI device for the VF is likely passed through to a
> > > > > > different VM and being used there. Unfortunately, the VF still
> > > > > > needs certain things done for it by the PF, so if the PF is
> > > > > > under DPDK control, it needs to provide the functionality to assist
> the VF.
> > > > >
> > > > > Why not have a VF_from_PF driver which does the mailbox things?
> > > > > So you can manage the VF from the PF with a simple port id.
> > > > > It really seems to be the cleanest design to me.
> > > >
> > > > While I see your point, and it could work, I just want to be sure
> > > > that we are
> > > ok with the results of that. Suppose we do create ethdevs for the
> > > VFs controlled by the PF. Does the new VF get counted in the
> > > rte_eth_dev_count() value (I assume yes)? How are apps meant to use
> > > the port? Do they have to put in a special case when iterating
> > > through all the port ids to check that it's not a pseudo port that
> > > can't do anything. None of the standard ethdev calls from an app
> > > will work on it, you can't configure nb rx/tx queues on it, you can't start or
> stop it, you can't do rx or tx on it, etc, etc.
> > >
> > > Yes these devices would be special because their supported API would
> > > be quite different. I was thinking that in the future you could add
> > > most of the configuration functions through the VF mailbox.
> > > But the Intel mailbox currently support only some special
> > > configurations which are not supported by other devices even its own
> > > VF device (except setting MAC address).
> > > And when I read "set drop enable bit in the VF split rx control
> > > register", it becomes clear it is really specific and has nothing to
> > > do in the generic ethdev API.
> > > That's why it is a NACK.
> > >
> > > When we want to use these very specific features we are aware of the
> > > underlying device and driver. So we can directly include a header
> > > from the driver. I suggest to retrieve a handler for the device
> > > which is not a port id and will allow to call ixgbe functions directly.
> > > It could be achieved by adding an ethdev function like discussed here:
> > > 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> > >
> >
> > I have been reading the net/vhost mail thread above. The following quote
> is from this thread.
> >
> > "It means I would be in favor of introducing API in drivers for very specific
> features."
> >
> > At present all the PMD functions are accessed through the eth_dev_ops
> structure, there are no PMD API's.
> >
> > Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a driver
> ioctl API) which can be accessed through a generic API in the ethdev?
> 
> Not exactly. I'm thinking about a PMD specific API.
> The only ethdev API you need would be a function to retrieve a handler (an
> opaque pointer on the device struct) from the port id.
> Then you can include rte_ixgbe.h and directly call the specific ixgbe function,
> passing the device handler.
> How does it sound?

I have been prototyping this proposed solution, it appears to work.

I have added the following function:

int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void** pmd_handle);

The pmd_handle is a pointer to a dev_ops structure containing driver specific functions.

Using the pmd_handle the driver specific functions can be called (without having them in struct eth_dev_ops)

Has this proposal been superseded by the discussion on the following patch?

[PATCH] net/vhost: Add function to retreive the 'vid' for a given port id

Regards,

Bernard.
  

2016-09-26 15:37, Iremonger, Bernard:
> Hi Thomas, Bruce,
> 
> <snip>
> 
> > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> > management
> > 
> > 2016-09-23 17:02, Iremonger, Bernard:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon wrote:
> > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > Do we really need to expose VF specific functions here?
> > > > > > > > > > > > It can be generic(PF/VF) function indexed only
> > > > > > > > > > > > through
> > > > > > port_id.
> > > > > > > > > > > > (example: as rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx PMD,
> > > > > > > > > > > > We are not exposing a separate port_id for PF. We
> > > > > > > > > > > > only enumerate 0..N VFs as 0..N ethdev port_id
> > > > > > > > > > >
> > > > > > > > > > > Our intention with this patch is to control the VF from the PF.
> > > > > > > > > > >
> > > > > > > > > > > The following librte_ether functions already work in a
> > > > > > > > > > > similar
> > > > > > way:
> > > > > > > > > > >
> > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,  uint16_t
> > > > > > > > > > > vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > >
> > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t vf,
> > > > > > > > > > > uint8_t
> > > > > > > > > > > on)
> > > > > > > > > > >
> > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t vf,
> > > > > > > > > > > uint8_t
> > > > > > > > > > > on)
> > > > > > > > > > >
> > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > > > > > > > > > > uint16_t vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > > > >
> > > > > > > > > > I have a bad feeling with these functions dedicated to VF from
> > PF.
> > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > >
> > > > > > > > > When the VF is used in a VM the port ID of the VF is not
> > > > > > > > > visible to
> > > > > > the PF.
> > > > > > > > > I don't think there is another way to do this.
> > > > > > > >
> > > > > > > > I don't understand why we could not assign a port id to the
> > > > > > > > VF from the host instead of having the couple PF port id / VF id.
> > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > Then can we allocate them a port id in the array rte_eth_devices?
> > > > > > >
> > > > > > > Hi Thomas,
> > > > > > >
> > > > > > > The VF is not a port visible to DPDK, though, so it shouldn't
> > > > > > > have a port id IMHO. DPDK can't actually do anything with it.
> > > > > >
> > > > > > You say the contrary below.
> > > > >
> > > > > Well, yes and no. The driver can manipulate things for the VF, but
> > > > > DPDK
> > > > doesn't actually have a device that corresponds to the VF. There are
> > > > no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?
> > > >
> > > > Very good point.
> > > > There are only few ethdev functions which are supported by every
> > > > drivers, like Rx/Tx and would not be available for VF from PF interface.
> > > >
> > > > > > > The PCI device for the VF is likely passed through to a
> > > > > > > different VM and being used there. Unfortunately, the VF still
> > > > > > > needs certain things done for it by the PF, so if the PF is
> > > > > > > under DPDK control, it needs to provide the functionality to assist
> > the VF.
> > > > > >
> > > > > > Why not have a VF_from_PF driver which does the mailbox things?
> > > > > > So you can manage the VF from the PF with a simple port id.
> > > > > > It really seems to be the cleanest design to me.
> > > > >
> > > > > While I see your point, and it could work, I just want to be sure
> > > > > that we are
> > > > ok with the results of that. Suppose we do create ethdevs for the
> > > > VFs controlled by the PF. Does the new VF get counted in the
> > > > rte_eth_dev_count() value (I assume yes)? How are apps meant to use
> > > > the port? Do they have to put in a special case when iterating
> > > > through all the port ids to check that it's not a pseudo port that
> > > > can't do anything. None of the standard ethdev calls from an app
> > > > will work on it, you can't configure nb rx/tx queues on it, you can't start or
> > stop it, you can't do rx or tx on it, etc, etc.
> > > >
> > > > Yes these devices would be special because their supported API would
> > > > be quite different. I was thinking that in the future you could add
> > > > most of the configuration functions through the VF mailbox.
> > > > But the Intel mailbox currently support only some special
> > > > configurations which are not supported by other devices even its own
> > > > VF device (except setting MAC address).
> > > > And when I read "set drop enable bit in the VF split rx control
> > > > register", it becomes clear it is really specific and has nothing to
> > > > do in the generic ethdev API.
> > > > That's why it is a NACK.
> > > >
> > > > When we want to use these very specific features we are aware of the
> > > > underlying device and driver. So we can directly include a header
> > > > from the driver. I suggest to retrieve a handler for the device
> > > > which is not a port id and will allow to call ixgbe functions directly.
> > > > It could be achieved by adding an ethdev function like discussed here:
> > > > 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> > > >
> > >
> > > I have been reading the net/vhost mail thread above. The following quote
> > is from this thread.
> > >
> > > "It means I would be in favor of introducing API in drivers for very specific
> > features."
> > >
> > > At present all the PMD functions are accessed through the eth_dev_ops
> > structure, there are no PMD API's.
> > >
> > > Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a driver
> > ioctl API) which can be accessed through a generic API in the ethdev?
> > 
> > Not exactly. I'm thinking about a PMD specific API.
> > The only ethdev API you need would be a function to retrieve a handler (an
> > opaque pointer on the device struct) from the port id.
> > Then you can include rte_ixgbe.h and directly call the specific ixgbe function,
> > passing the device handler.
> > How does it sound?
> 
> I have been prototyping this proposed solution, it appears to work.
> 
> I have added the following function:
> 
> int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void** pmd_handle);
> 
> The pmd_handle is a pointer to a dev_ops structure containing driver specific functions.
> 
> Using the pmd_handle the driver specific functions can be called (without having them in struct eth_dev_ops)
> 
> Has this proposal been superseded by the discussion on the following patch?
> 
> [PATCH] net/vhost: Add function to retreive the 'vid' for a given port id

Maybe, it can be superseded by this discussion, yes.
Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
What is your opinion about using port_id directly and retrieving the
structs from the driver via rte_eth_devices?
  

Hi Thomas, Bruce,

<snip>

> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-26 15:37, Iremonger, Bernard:
> > Hi Thomas, Bruce,
> >
> > <snip>
> >
> > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's
> > > for VF management
> > >
> > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon
> wrote:
> > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > Do we really need to expose VF specific functions
> here?
> > > > > > > > > > > > > It can be generic(PF/VF) function indexed only
> > > > > > > > > > > > > through
> > > > > > > port_id.
> > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx
> > > > > > > > > > > > > PMD, We are not exposing a separate port_id for
> > > > > > > > > > > > > PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > > > > > > > > > > port_id
> > > > > > > > > > > >
> > > > > > > > > > > > Our intention with this patch is to control the VF from the
> PF.
> > > > > > > > > > > >
> > > > > > > > > > > > The following librte_ether functions already work
> > > > > > > > > > > > in a similar
> > > > > > > way:
> > > > > > > > > > > >
> > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > >
> > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t
> > > > > > > > > > > > vf, uint8_t
> > > > > > > > > > > > on)
> > > > > > > > > > > >
> > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t
> > > > > > > > > > > > vf, uint8_t
> > > > > > > > > > > > on)
> > > > > > > > > > > >
> > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > > > > > > > > > > > uint16_t vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > > > > >
> > > > > > > > > > > I have a bad feeling with these functions dedicated
> > > > > > > > > > > to VF from
> > > PF.
> > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > > >
> > > > > > > > > > When the VF is used in a VM the port ID of the VF is
> > > > > > > > > > not visible to
> > > > > > > the PF.
> > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > >
> > > > > > > > > I don't understand why we could not assign a port id to
> > > > > > > > > the VF from the host instead of having the couple PF port id /
> VF id.
> > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > Then can we allocate them a port id in the array
> rte_eth_devices?
> > > > > > > >
> > > > > > > > Hi Thomas,
> > > > > > > >
> > > > > > > > The VF is not a port visible to DPDK, though, so it
> > > > > > > > shouldn't have a port id IMHO. DPDK can't actually do anything
> with it.
> > > > > > >
> > > > > > > You say the contrary below.
> > > > > >
> > > > > > Well, yes and no. The driver can manipulate things for the VF,
> > > > > > but DPDK
> > > > > doesn't actually have a device that corresponds to the VF. There
> > > > > are no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?
> > > > >
> > > > > Very good point.
> > > > > There are only few ethdev functions which are supported by every
> > > > > drivers, like Rx/Tx and would not be available for VF from PF
> interface.
> > > > >
> > > > > > > > The PCI device for the VF is likely passed through to a
> > > > > > > > different VM and being used there. Unfortunately, the VF
> > > > > > > > still needs certain things done for it by the PF, so if
> > > > > > > > the PF is under DPDK control, it needs to provide the
> > > > > > > > functionality to assist
> > > the VF.
> > > > > > >
> > > > > > > Why not have a VF_from_PF driver which does the mailbox
> things?
> > > > > > > So you can manage the VF from the PF with a simple port id.
> > > > > > > It really seems to be the cleanest design to me.
> > > > > >
> > > > > > While I see your point, and it could work, I just want to be
> > > > > > sure that we are
> > > > > ok with the results of that. Suppose we do create ethdevs for
> > > > > the VFs controlled by the PF. Does the new VF get counted in the
> > > > > rte_eth_dev_count() value (I assume yes)? How are apps meant to
> > > > > use the port? Do they have to put in a special case when
> > > > > iterating through all the port ids to check that it's not a
> > > > > pseudo port that can't do anything. None of the standard ethdev
> > > > > calls from an app will work on it, you can't configure nb rx/tx
> > > > > queues on it, you can't start or
> > > stop it, you can't do rx or tx on it, etc, etc.
> > > > >
> > > > > Yes these devices would be special because their supported API
> > > > > would be quite different. I was thinking that in the future you
> > > > > could add most of the configuration functions through the VF
> mailbox.
> > > > > But the Intel mailbox currently support only some special
> > > > > configurations which are not supported by other devices even its
> > > > > own VF device (except setting MAC address).
> > > > > And when I read "set drop enable bit in the VF split rx control
> > > > > register", it becomes clear it is really specific and has
> > > > > nothing to do in the generic ethdev API.
> > > > > That's why it is a NACK.
> > > > >
> > > > > When we want to use these very specific features we are aware of
> > > > > the underlying device and driver. So we can directly include a
> > > > > header from the driver. I suggest to retrieve a handler for the
> > > > > device which is not a port id and will allow to call ixgbe functions
> directly.
> > > > > It could be achieved by adding an ethdev function like discussed here:
> > > > > 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> > > > >
> > > >
> > > > I have been reading the net/vhost mail thread above. The following
> > > > quote
> > > is from this thread.
> > > >
> > > > "It means I would be in favor of introducing API in drivers for
> > > > very specific
> > > features."
> > > >
> > > > At present all the PMD functions are accessed through the
> > > > eth_dev_ops
> > > structure, there are no PMD API's.
> > > >
> > > > Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a
> > > > driver
> > > ioctl API) which can be accessed through a generic API in the ethdev?
> > >
> > > Not exactly. I'm thinking about a PMD specific API.
> > > The only ethdev API you need would be a function to retrieve a
> > > handler (an opaque pointer on the device struct) from the port id.
> > > Then you can include rte_ixgbe.h and directly call the specific
> > > ixgbe function, passing the device handler.
> > > How does it sound?
> >
> > I have been prototyping this proposed solution, it appears to work.
> >
> > I have added the following function:
> >
> > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void** pmd_handle);
> >
> > The pmd_handle is a pointer to a dev_ops structure containing driver
> specific functions.
> >
> > Using the pmd_handle the driver specific functions can be called
> > (without having them in struct eth_dev_ops)
> >
> > Has this proposal been superseded by the discussion on the following
> patch?
> >
> > [PATCH] net/vhost: Add function to retreive the 'vid' for a given port
> > id
> 
> Maybe, it can be superseded by this discussion, yes.
> Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> What is your opinion about using port_id directly and retrieving the structs
> from the driver via rte_eth_devices?

Looking at the code in rte_eth_devices[]

struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];

struct rte_eth_dev {

...

const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ 

...

 void *pmd_ops;  /** < exported PMD specific functions */ 
  
}

The PMD functions are only accessible at present if they are in struct eth_dev_ops.

Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD functions accessible and is a simpler solution than using rte_eth_dev_get_pmd_handle() to get access to the PMD functions.

Regards,

Bernard.
  

On Tue, Sep 27, 2016 at 11:31:06AM +0100, Iremonger, Bernard wrote:
> Hi Thomas, Bruce,
> 
> <snip>
> 
> > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> > management
> > 
> > 2016-09-26 15:37, Iremonger, Bernard:
> > > Hi Thomas, Bruce,
> > >
> > > <snip>
> > >
> > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's
> > > > for VF management
> > > >
> > > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas Monjalon
> > wrote:
> > > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > > Do we really need to expose VF specific functions
> > here?
> > > > > > > > > > > > > > It can be generic(PF/VF) function indexed only
> > > > > > > > > > > > > > through
> > > > > > > > port_id.
> > > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > > port_id, uint8_t on)) For instance, In Thunderx
> > > > > > > > > > > > > > PMD, We are not exposing a separate port_id for
> > > > > > > > > > > > > > PF. We only enumerate 0..N VFs as 0..N ethdev
> > > > > > > > > > > > > > port_id
> > > > > > > > > > > > >
> > > > > > > > > > > > > Our intention with this patch is to control the VF from the
> > PF.
> > > > > > > > > > > > >
> > > > > > > > > > > > > The following librte_ether functions already work
> > > > > > > > > > > > > in a similar
> > > > > > > > way:
> > > > > > > > > > > > >
> > > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > > >
> > > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id, uint16_t
> > > > > > > > > > > > > vf, uint8_t
> > > > > > > > > > > > > on)
> > > > > > > > > > > > >
> > > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id, uint16_t
> > > > > > > > > > > > > vf, uint8_t
> > > > > > > > > > > > > on)
> > > > > > > > > > > > >
> > > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > > > > > > > > > > > > uint16_t vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > > > > > >
> > > > > > > > > > > > I have a bad feeling with these functions dedicated
> > > > > > > > > > > > to VF from
> > > > PF.
> > > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > > > >
> > > > > > > > > > > When the VF is used in a VM the port ID of the VF is
> > > > > > > > > > > not visible to
> > > > > > > > the PF.
> > > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > > >
> > > > > > > > > > I don't understand why we could not assign a port id to
> > > > > > > > > > the VF from the host instead of having the couple PF port id /
> > VF id.
> > > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > > Then can we allocate them a port id in the array
> > rte_eth_devices?
> > > > > > > > >
> > > > > > > > > Hi Thomas,
> > > > > > > > >
> > > > > > > > > The VF is not a port visible to DPDK, though, so it
> > > > > > > > > shouldn't have a port id IMHO. DPDK can't actually do anything
> > with it.
> > > > > > > >
> > > > > > > > You say the contrary below.
> > > > > > >
> > > > > > > Well, yes and no. The driver can manipulate things for the VF,
> > > > > > > but DPDK
> > > > > > doesn't actually have a device that corresponds to the VF. There
> > > > > > are no PCI bar mappings for it, DPDK can't do RX and TX with it etc.?
> > > > > >
> > > > > > Very good point.
> > > > > > There are only few ethdev functions which are supported by every
> > > > > > drivers, like Rx/Tx and would not be available for VF from PF
> > interface.
> > > > > >
> > > > > > > > > The PCI device for the VF is likely passed through to a
> > > > > > > > > different VM and being used there. Unfortunately, the VF
> > > > > > > > > still needs certain things done for it by the PF, so if
> > > > > > > > > the PF is under DPDK control, it needs to provide the
> > > > > > > > > functionality to assist
> > > > the VF.
> > > > > > > >
> > > > > > > > Why not have a VF_from_PF driver which does the mailbox
> > things?
> > > > > > > > So you can manage the VF from the PF with a simple port id.
> > > > > > > > It really seems to be the cleanest design to me.
> > > > > > >
> > > > > > > While I see your point, and it could work, I just want to be
> > > > > > > sure that we are
> > > > > > ok with the results of that. Suppose we do create ethdevs for
> > > > > > the VFs controlled by the PF. Does the new VF get counted in the
> > > > > > rte_eth_dev_count() value (I assume yes)? How are apps meant to
> > > > > > use the port? Do they have to put in a special case when
> > > > > > iterating through all the port ids to check that it's not a
> > > > > > pseudo port that can't do anything. None of the standard ethdev
> > > > > > calls from an app will work on it, you can't configure nb rx/tx
> > > > > > queues on it, you can't start or
> > > > stop it, you can't do rx or tx on it, etc, etc.
> > > > > >
> > > > > > Yes these devices would be special because their supported API
> > > > > > would be quite different. I was thinking that in the future you
> > > > > > could add most of the configuration functions through the VF
> > mailbox.
> > > > > > But the Intel mailbox currently support only some special
> > > > > > configurations which are not supported by other devices even its
> > > > > > own VF device (except setting MAC address).
> > > > > > And when I read "set drop enable bit in the VF split rx control
> > > > > > register", it becomes clear it is really specific and has
> > > > > > nothing to do in the generic ethdev API.
> > > > > > That's why it is a NACK.
> > > > > >
> > > > > > When we want to use these very specific features we are aware of
> > > > > > the underlying device and driver. So we can directly include a
> > > > > > header from the driver. I suggest to retrieve a handler for the
> > > > > > device which is not a port id and will allow to call ixgbe functions
> > directly.
> > > > > > It could be achieved by adding an ethdev function like discussed here:
> > > > > > 	http://dpdk.org/ml/archives/dev/2016-September/047392.html
> > > > > >
> > > > >
> > > > > I have been reading the net/vhost mail thread above. The following
> > > > > quote
> > > > is from this thread.
> > > > >
> > > > > "It means I would be in favor of introducing API in drivers for
> > > > > very specific
> > > > features."
> > > > >
> > > > > At present all the PMD functions are accessed through the
> > > > > eth_dev_ops
> > > > structure, there are no PMD API's.
> > > > >
> > > > > Is your proposal to add API(s) to the DPDK ixgbe PMD (similar to a
> > > > > driver
> > > > ioctl API) which can be accessed through a generic API in the ethdev?
> > > >
> > > > Not exactly. I'm thinking about a PMD specific API.
> > > > The only ethdev API you need would be a function to retrieve a
> > > > handler (an opaque pointer on the device struct) from the port id.
> > > > Then you can include rte_ixgbe.h and directly call the specific
> > > > ixgbe function, passing the device handler.
> > > > How does it sound?
> > >
> > > I have been prototyping this proposed solution, it appears to work.
> > >
> > > I have added the following function:
> > >
> > > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void** pmd_handle);
> > >
> > > The pmd_handle is a pointer to a dev_ops structure containing driver
> > specific functions.
> > >
> > > Using the pmd_handle the driver specific functions can be called
> > > (without having them in struct eth_dev_ops)
> > >
> > > Has this proposal been superseded by the discussion on the following
> > patch?
> > >
> > > [PATCH] net/vhost: Add function to retreive the 'vid' for a given port
> > > id
> > 
> > Maybe, it can be superseded by this discussion, yes.
> > Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> > What is your opinion about using port_id directly and retrieving the structs
> > from the driver via rte_eth_devices?
> 
> Looking at the code in rte_eth_devices[]
> 
> struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];
> 
> struct rte_eth_dev {
> 
> ...
> 
> const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */ 
> 
> ...
> 
>  void *pmd_ops;  /** < exported PMD specific functions */ 
>   
> }
> 
> The PMD functions are only accessible at present if they are in struct eth_dev_ops.
> 
> Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD functions accessible and is a simpler solution than using rte_eth_dev_get_pmd_handle() to get access to the PMD functions.
> 
> Regards,
> 

Why would an ops structure be needed? If it's a private API for a driver, there
should be no need for function pointers, and instead the driver can define
regular functions in it's header file, no?

/Bruce
  

Hi Bruce,

<snip>

> > > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add
> > > > > API's for VF management
> > > > >
> > > > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas
> > > > > > > > > > Monjalon
> > > wrote:
> > > > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > > > Do we really need to expose VF specific
> > > > > > > > > > > > > > > functions
> > > here?
> > > > > > > > > > > > > > > It can be generic(PF/VF) function indexed
> > > > > > > > > > > > > > > only through
> > > > > > > > > port_id.
> > > > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > > > port_id, uint8_t on)) For instance, In
> > > > > > > > > > > > > > > Thunderx PMD, We are not exposing a separate
> > > > > > > > > > > > > > > port_id for PF. We only enumerate 0..N VFs
> > > > > > > > > > > > > > > as 0..N ethdev port_id
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > Our intention with this patch is to control
> > > > > > > > > > > > > > the VF from the
> > > PF.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > The following librte_ether functions already
> > > > > > > > > > > > > > work in a similar
> > > > > > > > > way:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id,
> > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > on)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id,
> > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > on)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t port_id,
> > > > > > > > > > > > > > uint16_t vf, uint16_t tx_rate, uint64_t q_msk)
> > > > > > > > > > > > >
> > > > > > > > > > > > > I have a bad feeling with these functions
> > > > > > > > > > > > > dedicated to VF from
> > > > > PF.
> > > > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > > > > >
> > > > > > > > > > > > When the VF is used in a VM the port ID of the VF
> > > > > > > > > > > > is not visible to
> > > > > > > > > the PF.
> > > > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > > > >
> > > > > > > > > > > I don't understand why we could not assign a port id
> > > > > > > > > > > to the VF from the host instead of having the couple
> > > > > > > > > > > PF port id /
> > > VF id.
> > > > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > > > Then can we allocate them a port id in the array
> > > rte_eth_devices?
> > > > > > > > > >
> > > > > > > > > > Hi Thomas,
> > > > > > > > > >
> > > > > > > > > > The VF is not a port visible to DPDK, though, so it
> > > > > > > > > > shouldn't have a port id IMHO. DPDK can't actually do
> > > > > > > > > > anything
> > > with it.
> > > > > > > > >
> > > > > > > > > You say the contrary below.
> > > > > > > >
> > > > > > > > Well, yes and no. The driver can manipulate things for the
> > > > > > > > VF, but DPDK
> > > > > > > doesn't actually have a device that corresponds to the VF.
> > > > > > > There are no PCI bar mappings for it, DPDK can't do RX and TX with
> it etc.?
> > > > > > >
> > > > > > > Very good point.
> > > > > > > There are only few ethdev functions which are supported by
> > > > > > > every drivers, like Rx/Tx and would not be available for VF
> > > > > > > from PF
> > > interface.
> > > > > > >
> > > > > > > > > > The PCI device for the VF is likely passed through to
> > > > > > > > > > a different VM and being used there. Unfortunately,
> > > > > > > > > > the VF still needs certain things done for it by the
> > > > > > > > > > PF, so if the PF is under DPDK control, it needs to
> > > > > > > > > > provide the functionality to assist
> > > > > the VF.
> > > > > > > > >
> > > > > > > > > Why not have a VF_from_PF driver which does the mailbox
> > > things?
> > > > > > > > > So you can manage the VF from the PF with a simple port id.
> > > > > > > > > It really seems to be the cleanest design to me.
> > > > > > > >
> > > > > > > > While I see your point, and it could work, I just want to
> > > > > > > > be sure that we are
> > > > > > > ok with the results of that. Suppose we do create ethdevs
> > > > > > > for the VFs controlled by the PF. Does the new VF get
> > > > > > > counted in the
> > > > > > > rte_eth_dev_count() value (I assume yes)? How are apps meant
> > > > > > > to use the port? Do they have to put in a special case when
> > > > > > > iterating through all the port ids to check that it's not a
> > > > > > > pseudo port that can't do anything. None of the standard
> > > > > > > ethdev calls from an app will work on it, you can't
> > > > > > > configure nb rx/tx queues on it, you can't start or
> > > > > stop it, you can't do rx or tx on it, etc, etc.
> > > > > > >
> > > > > > > Yes these devices would be special because their supported
> > > > > > > API would be quite different. I was thinking that in the
> > > > > > > future you could add most of the configuration functions
> > > > > > > through the VF
> > > mailbox.
> > > > > > > But the Intel mailbox currently support only some special
> > > > > > > configurations which are not supported by other devices even
> > > > > > > its own VF device (except setting MAC address).
> > > > > > > And when I read "set drop enable bit in the VF split rx
> > > > > > > control register", it becomes clear it is really specific
> > > > > > > and has nothing to do in the generic ethdev API.
> > > > > > > That's why it is a NACK.
> > > > > > >
> > > > > > > When we want to use these very specific features we are
> > > > > > > aware of the underlying device and driver. So we can
> > > > > > > directly include a header from the driver. I suggest to
> > > > > > > retrieve a handler for the device which is not a port id and
> > > > > > > will allow to call ixgbe functions
> > > directly.
> > > > > > > It could be achieved by adding an ethdev function like discussed
> here:
> > > > > > > 	http://dpdk.org/ml/archives/dev/2016-
> September/047392.html
> > > > > > >
> > > > > >
> > > > > > I have been reading the net/vhost mail thread above. The
> > > > > > following quote
> > > > > is from this thread.
> > > > > >
> > > > > > "It means I would be in favor of introducing API in drivers
> > > > > > for very specific
> > > > > features."
> > > > > >
> > > > > > At present all the PMD functions are accessed through the
> > > > > > eth_dev_ops
> > > > > structure, there are no PMD API's.
> > > > > >
> > > > > > Is your proposal to add API(s) to the DPDK ixgbe PMD (similar
> > > > > > to a driver
> > > > > ioctl API) which can be accessed through a generic API in the ethdev?
> > > > >
> > > > > Not exactly. I'm thinking about a PMD specific API.
> > > > > The only ethdev API you need would be a function to retrieve a
> > > > > handler (an opaque pointer on the device struct) from the port id.
> > > > > Then you can include rte_ixgbe.h and directly call the specific
> > > > > ixgbe function, passing the device handler.
> > > > > How does it sound?
> > > >
> > > > I have been prototyping this proposed solution, it appears to work.
> > > >
> > > > I have added the following function:
> > > >
> > > > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void**
> > > > pmd_handle);
> > > >
> > > > The pmd_handle is a pointer to a dev_ops structure containing
> > > > driver
> > > specific functions.
> > > >
> > > > Using the pmd_handle the driver specific functions can be called
> > > > (without having them in struct eth_dev_ops)
> > > >
> > > > Has this proposal been superseded by the discussion on the
> > > > following
> > > patch?
> > > >
> > > > [PATCH] net/vhost: Add function to retreive the 'vid' for a given
> > > > port id
> > >
> > > Maybe, it can be superseded by this discussion, yes.
> > > Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> > > What is your opinion about using port_id directly and retrieving the
> > > structs from the driver via rte_eth_devices?
> >
> > Looking at the code in rte_eth_devices[]
> >
> > struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];
> >
> > struct rte_eth_dev {
> >
> > ...
> >
> > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> >
> > ...
> >
> >  void *pmd_ops;  /** < exported PMD specific functions */
> >
> > }
> >
> > The PMD functions are only accessible at present if they are in struct
> eth_dev_ops.
> >
> > Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD functions
> accessible and is a simpler solution than using
> rte_eth_dev_get_pmd_handle() to get access to the PMD functions.
> >
> > Regards,
> >
> 
> Why would an ops structure be needed? If it's a private API for a driver,
> there should be no need for function pointers, and instead the driver can
> define regular functions in it's header file, no?
> 
> /Bruce

The driver functions were static, I have made them public and added them to the rte_pmd_ixgbe.h file, and it works.  These functions will also need to be added to the rte_pmd_ixgbe_version.map file, previously there were no public functions.

Regards,

Bernard.
  

Hi lads,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger, Bernard
> Sent: Tuesday, September 27, 2016 3:13 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org; Jerin Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul
> R <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; azelezniak <alexz@att.com>
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF management
> 
> Hi Bruce,
> 
> <snip>
> 
> > > > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add
> > > > > > API's for VF management
> > > > > >
> > > > > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas
> > > > > > > > > > > Monjalon
> > > > wrote:
> > > > > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > > > > Do we really need to expose VF specific
> > > > > > > > > > > > > > > > functions
> > > > here?
> > > > > > > > > > > > > > > > It can be generic(PF/VF) function indexed
> > > > > > > > > > > > > > > > only through
> > > > > > > > > > port_id.
> > > > > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > > > > port_id, uint8_t on)) For instance, In
> > > > > > > > > > > > > > > > Thunderx PMD, We are not exposing a
> > > > > > > > > > > > > > > > separate port_id for PF. We only enumerate
> > > > > > > > > > > > > > > > 0..N VFs as 0..N ethdev port_id
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Our intention with this patch is to control
> > > > > > > > > > > > > > > the VF from the
> > > > PF.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > The following librte_ether functions already
> > > > > > > > > > > > > > > work in a similar
> > > > > > > > > > way:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id,
> > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id,
> > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t
> > > > > > > > > > > > > > > port_id, uint16_t vf, uint16_t tx_rate,
> > > > > > > > > > > > > > > uint64_t q_msk)
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I have a bad feeling with these functions
> > > > > > > > > > > > > > dedicated to VF from
> > > > > > PF.
> > > > > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > > > > > >
> > > > > > > > > > > > > When the VF is used in a VM the port ID of the
> > > > > > > > > > > > > VF is not visible to
> > > > > > > > > > the PF.
> > > > > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > > > > >
> > > > > > > > > > > > I don't understand why we could not assign a port
> > > > > > > > > > > > id to the VF from the host instead of having the
> > > > > > > > > > > > couple PF port id /
> > > > VF id.
> > > > > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > > > > Then can we allocate them a port id in the array
> > > > rte_eth_devices?
> > > > > > > > > > >
> > > > > > > > > > > Hi Thomas,
> > > > > > > > > > >
> > > > > > > > > > > The VF is not a port visible to DPDK, though, so it
> > > > > > > > > > > shouldn't have a port id IMHO. DPDK can't actually
> > > > > > > > > > > do anything
> > > > with it.
> > > > > > > > > >
> > > > > > > > > > You say the contrary below.
> > > > > > > > >
> > > > > > > > > Well, yes and no. The driver can manipulate things for
> > > > > > > > > the VF, but DPDK
> > > > > > > > doesn't actually have a device that corresponds to the VF.
> > > > > > > > There are no PCI bar mappings for it, DPDK can't do RX and
> > > > > > > > TX with
> > it etc.?
> > > > > > > >
> > > > > > > > Very good point.
> > > > > > > > There are only few ethdev functions which are supported by
> > > > > > > > every drivers, like Rx/Tx and would not be available for
> > > > > > > > VF from PF
> > > > interface.
> > > > > > > >
> > > > > > > > > > > The PCI device for the VF is likely passed through
> > > > > > > > > > > to a different VM and being used there.
> > > > > > > > > > > Unfortunately, the VF still needs certain things
> > > > > > > > > > > done for it by the PF, so if the PF is under DPDK
> > > > > > > > > > > control, it needs to provide the functionality to
> > > > > > > > > > > assist
> > > > > > the VF.
> > > > > > > > > >
> > > > > > > > > > Why not have a VF_from_PF driver which does the
> > > > > > > > > > mailbox
> > > > things?
> > > > > > > > > > So you can manage the VF from the PF with a simple port id.
> > > > > > > > > > It really seems to be the cleanest design to me.
> > > > > > > > >
> > > > > > > > > While I see your point, and it could work, I just want
> > > > > > > > > to be sure that we are
> > > > > > > > ok with the results of that. Suppose we do create ethdevs
> > > > > > > > for the VFs controlled by the PF. Does the new VF get
> > > > > > > > counted in the
> > > > > > > > rte_eth_dev_count() value (I assume yes)? How are apps
> > > > > > > > meant to use the port? Do they have to put in a special
> > > > > > > > case when iterating through all the port ids to check that
> > > > > > > > it's not a pseudo port that can't do anything. None of the
> > > > > > > > standard ethdev calls from an app will work on it, you
> > > > > > > > can't configure nb rx/tx queues on it, you can't start or
> > > > > > stop it, you can't do rx or tx on it, etc, etc.
> > > > > > > >
> > > > > > > > Yes these devices would be special because their supported
> > > > > > > > API would be quite different. I was thinking that in the
> > > > > > > > future you could add most of the configuration functions
> > > > > > > > through the VF
> > > > mailbox.
> > > > > > > > But the Intel mailbox currently support only some special
> > > > > > > > configurations which are not supported by other devices
> > > > > > > > even its own VF device (except setting MAC address).
> > > > > > > > And when I read "set drop enable bit in the VF split rx
> > > > > > > > control register", it becomes clear it is really specific
> > > > > > > > and has nothing to do in the generic ethdev API.
> > > > > > > > That's why it is a NACK.
> > > > > > > >
> > > > > > > > When we want to use these very specific features we are
> > > > > > > > aware of the underlying device and driver. So we can
> > > > > > > > directly include a header from the driver. I suggest to
> > > > > > > > retrieve a handler for the device which is not a port id
> > > > > > > > and will allow to call ixgbe functions
> > > > directly.
> > > > > > > > It could be achieved by adding an ethdev function like
> > > > > > > > discussed
> > here:
> > > > > > > > 	http://dpdk.org/ml/archives/dev/2016-
> > September/047392.html
> > > > > > > >
> > > > > > >
> > > > > > > I have been reading the net/vhost mail thread above. The
> > > > > > > following quote
> > > > > > is from this thread.
> > > > > > >
> > > > > > > "It means I would be in favor of introducing API in drivers
> > > > > > > for very specific
> > > > > > features."
> > > > > > >
> > > > > > > At present all the PMD functions are accessed through the
> > > > > > > eth_dev_ops
> > > > > > structure, there are no PMD API's.
> > > > > > >
> > > > > > > Is your proposal to add API(s) to the DPDK ixgbe PMD
> > > > > > > (similar to a driver
> > > > > > ioctl API) which can be accessed through a generic API in the ethdev?
> > > > > >
> > > > > > Not exactly. I'm thinking about a PMD specific API.
> > > > > > The only ethdev API you need would be a function to retrieve a
> > > > > > handler (an opaque pointer on the device struct) from the port id.
> > > > > > Then you can include rte_ixgbe.h and directly call the
> > > > > > specific ixgbe function, passing the device handler.
> > > > > > How does it sound?
> > > > >
> > > > > I have been prototyping this proposed solution, it appears to work.
> > > > >
> > > > > I have added the following function:
> > > > >
> > > > > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void**
> > > > > pmd_handle);
> > > > >
> > > > > The pmd_handle is a pointer to a dev_ops structure containing
> > > > > driver
> > > > specific functions.
> > > > >
> > > > > Using the pmd_handle the driver specific functions can be called
> > > > > (without having them in struct eth_dev_ops)
> > > > >
> > > > > Has this proposal been superseded by the discussion on the
> > > > > following
> > > > patch?
> > > > >
> > > > > [PATCH] net/vhost: Add function to retreive the 'vid' for a
> > > > > given port id
> > > >
> > > > Maybe, it can be superseded by this discussion, yes.
> > > > Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> > > > What is your opinion about using port_id directly and retrieving
> > > > the structs from the driver via rte_eth_devices?
> > >
> > > Looking at the code in rte_eth_devices[]
> > >
> > > struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];
> > >
> > > struct rte_eth_dev {
> > >
> > > ...
> > >
> > > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD */
> > >
> > > ...
> > >
> > >  void *pmd_ops;  /** < exported PMD specific functions */
> > >
> > > }
> > >
> > > The PMD functions are only accessible at present if they are in
> > > struct
> > eth_dev_ops.
> > >
> > > Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD
> > > functions
> > accessible and is a simpler solution than using
> > rte_eth_dev_get_pmd_handle() to get access to the PMD functions.
> > >
> > > Regards,
> > >
> >
> > Why would an ops structure be needed? If it's a private API for a
> > driver, there should be no need for function pointers, and instead the
> > driver can define regular functions in it's header file, no?
> >
> > /Bruce
> 
> The driver functions were static, I have made them public and added them to the rte_pmd_ixgbe.h file, and it works.  These functions
> will also need to be added to the rte_pmd_ixgbe_version.map file, previously there were no public functions.

Sorry for being late in the discussion, but I have a question:
If we  this way (force user to include driver specific headers and call driver specific functions),
how you guys plan to make this functionality available for multiple driver types.
From discussion with Bernard  understand that customers would need similar functionality for i40e.
Does it mean that they'll have to re-implement this part of their code again?
Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
Basically their own version of rte_ethdev?

Konstantin

> 
> Regards,
> 
> Bernard.
  

Hi Bruce, Konstantin,

<snip>

> Subject: RE: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> Hi lads,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger,
> > Bernard
> > Sent: Tuesday, September 27, 2016 3:13 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org;
> Jerin
> > Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul R
> > <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > azelezniak <alexz@att.com>
> > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for
> > VF management
> >
> > Hi Bruce,
> >
> > <snip>
> >
> > > > > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add
> > > > > > > API's for VF management
> > > > > > >
> > > > > > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > > > > > From: Thomas Monjalon
> > > > > > > > > > [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas
> > > > > > > > > > > > Monjalon
> > > > > wrote:
> > > > > > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > > > > > Do we really need to expose VF specific
> > > > > > > > > > > > > > > > > functions
> > > > > here?
> > > > > > > > > > > > > > > > > It can be generic(PF/VF) function
> > > > > > > > > > > > > > > > > indexed only through
> > > > > > > > > > > port_id.
> > > > > > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > > > > > port_id, uint8_t on)) For instance, In
> > > > > > > > > > > > > > > > > Thunderx PMD, We are not exposing a
> > > > > > > > > > > > > > > > > separate port_id for PF. We only
> > > > > > > > > > > > > > > > > enumerate 0..N VFs as 0..N ethdev
> > > > > > > > > > > > > > > > > port_id
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Our intention with this patch is to
> > > > > > > > > > > > > > > > control the VF from the
> > > > > PF.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > The following librte_ether functions
> > > > > > > > > > > > > > > > already work in a similar
> > > > > > > > > > > way:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t
> > > > > > > > > > > > > > > > port_id, uint16_t vf, uint16_t tx_rate,
> > > > > > > > > > > > > > > > uint64_t q_msk)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I have a bad feeling with these functions
> > > > > > > > > > > > > > > dedicated to VF from
> > > > > > > PF.
> > > > > > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > > > > > I mean we just need to know the VF with a port ID.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When the VF is used in a VM the port ID of the
> > > > > > > > > > > > > > VF is not visible to
> > > > > > > > > > > the PF.
> > > > > > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't understand why we could not assign a
> > > > > > > > > > > > > port id to the VF from the host instead of
> > > > > > > > > > > > > having the couple PF port id /
> > > > > VF id.
> > > > > > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > > > > > Then can we allocate them a port id in the array
> > > > > rte_eth_devices?
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Thomas,
> > > > > > > > > > > >
> > > > > > > > > > > > The VF is not a port visible to DPDK, though, so
> > > > > > > > > > > > it shouldn't have a port id IMHO. DPDK can't
> > > > > > > > > > > > actually do anything
> > > > > with it.
> > > > > > > > > > >
> > > > > > > > > > > You say the contrary below.
> > > > > > > > > >
> > > > > > > > > > Well, yes and no. The driver can manipulate things for
> > > > > > > > > > the VF, but DPDK
> > > > > > > > > doesn't actually have a device that corresponds to the VF.
> > > > > > > > > There are no PCI bar mappings for it, DPDK can't do RX
> > > > > > > > > and TX with
> > > it etc.?
> > > > > > > > >
> > > > > > > > > Very good point.
> > > > > > > > > There are only few ethdev functions which are supported
> > > > > > > > > by every drivers, like Rx/Tx and would not be available
> > > > > > > > > for VF from PF
> > > > > interface.
> > > > > > > > >
> > > > > > > > > > > > The PCI device for the VF is likely passed through
> > > > > > > > > > > > to a different VM and being used there.
> > > > > > > > > > > > Unfortunately, the VF still needs certain things
> > > > > > > > > > > > done for it by the PF, so if the PF is under DPDK
> > > > > > > > > > > > control, it needs to provide the functionality to
> > > > > > > > > > > > assist
> > > > > > > the VF.
> > > > > > > > > > >
> > > > > > > > > > > Why not have a VF_from_PF driver which does the
> > > > > > > > > > > mailbox
> > > > > things?
> > > > > > > > > > > So you can manage the VF from the PF with a simple port
> id.
> > > > > > > > > > > It really seems to be the cleanest design to me.
> > > > > > > > > >
> > > > > > > > > > While I see your point, and it could work, I just want
> > > > > > > > > > to be sure that we are
> > > > > > > > > ok with the results of that. Suppose we do create
> > > > > > > > > ethdevs for the VFs controlled by the PF. Does the new
> > > > > > > > > VF get counted in the
> > > > > > > > > rte_eth_dev_count() value (I assume yes)? How are apps
> > > > > > > > > meant to use the port? Do they have to put in a special
> > > > > > > > > case when iterating through all the port ids to check
> > > > > > > > > that it's not a pseudo port that can't do anything. None
> > > > > > > > > of the standard ethdev calls from an app will work on
> > > > > > > > > it, you can't configure nb rx/tx queues on it, you can't
> > > > > > > > > start or
> > > > > > > stop it, you can't do rx or tx on it, etc, etc.
> > > > > > > > >
> > > > > > > > > Yes these devices would be special because their
> > > > > > > > > supported API would be quite different. I was thinking
> > > > > > > > > that in the future you could add most of the
> > > > > > > > > configuration functions through the VF
> > > > > mailbox.
> > > > > > > > > But the Intel mailbox currently support only some
> > > > > > > > > special configurations which are not supported by other
> > > > > > > > > devices even its own VF device (except setting MAC address).
> > > > > > > > > And when I read "set drop enable bit in the VF split rx
> > > > > > > > > control register", it becomes clear it is really
> > > > > > > > > specific and has nothing to do in the generic ethdev API.
> > > > > > > > > That's why it is a NACK.
> > > > > > > > >
> > > > > > > > > When we want to use these very specific features we are
> > > > > > > > > aware of the underlying device and driver. So we can
> > > > > > > > > directly include a header from the driver. I suggest to
> > > > > > > > > retrieve a handler for the device which is not a port id
> > > > > > > > > and will allow to call ixgbe functions
> > > > > directly.
> > > > > > > > > It could be achieved by adding an ethdev function like
> > > > > > > > > discussed
> > > here:
> > > > > > > > > 	http://dpdk.org/ml/archives/dev/2016-
> > > September/047392.html
> > > > > > > > >
> > > > > > > >
> > > > > > > > I have been reading the net/vhost mail thread above. The
> > > > > > > > following quote
> > > > > > > is from this thread.
> > > > > > > >
> > > > > > > > "It means I would be in favor of introducing API in
> > > > > > > > drivers for very specific
> > > > > > > features."
> > > > > > > >
> > > > > > > > At present all the PMD functions are accessed through the
> > > > > > > > eth_dev_ops
> > > > > > > structure, there are no PMD API's.
> > > > > > > >
> > > > > > > > Is your proposal to add API(s) to the DPDK ixgbe PMD
> > > > > > > > (similar to a driver
> > > > > > > ioctl API) which can be accessed through a generic API in the
> ethdev?
> > > > > > >
> > > > > > > Not exactly. I'm thinking about a PMD specific API.
> > > > > > > The only ethdev API you need would be a function to retrieve
> > > > > > > a handler (an opaque pointer on the device struct) from the port
> id.
> > > > > > > Then you can include rte_ixgbe.h and directly call the
> > > > > > > specific ixgbe function, passing the device handler.
> > > > > > > How does it sound?
> > > > > >
> > > > > > I have been prototyping this proposed solution, it appears to work.
> > > > > >
> > > > > > I have added the following function:
> > > > > >
> > > > > > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void**
> > > > > > pmd_handle);
> > > > > >
> > > > > > The pmd_handle is a pointer to a dev_ops structure containing
> > > > > > driver
> > > > > specific functions.
> > > > > >
> > > > > > Using the pmd_handle the driver specific functions can be
> > > > > > called (without having them in struct eth_dev_ops)
> > > > > >
> > > > > > Has this proposal been superseded by the discussion on the
> > > > > > following
> > > > > patch?
> > > > > >
> > > > > > [PATCH] net/vhost: Add function to retreive the 'vid' for a
> > > > > > given port id
> > > > >
> > > > > Maybe, it can be superseded by this discussion, yes.
> > > > > Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> > > > > What is your opinion about using port_id directly and retrieving
> > > > > the structs from the driver via rte_eth_devices?
> > > >
> > > > Looking at the code in rte_eth_devices[]
> > > >
> > > > struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];
> > > >
> > > > struct rte_eth_dev {
> > > >
> > > > ...
> > > >
> > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD
> > > > */
> > > >
> > > > ...
> > > >
> > > >  void *pmd_ops;  /** < exported PMD specific functions */
> > > >
> > > > }
> > > >
> > > > The PMD functions are only accessible at present if they are in
> > > > struct
> > > eth_dev_ops.
> > > >
> > > > Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD
> > > > functions
> > > accessible and is a simpler solution than using
> > > rte_eth_dev_get_pmd_handle() to get access to the PMD functions.
> > > >
> > > > Regards,
> > > >
> > >
> > > Why would an ops structure be needed? If it's a private API for a
> > > driver, there should be no need for function pointers, and instead
> > > the driver can define regular functions in it's header file, no?
> > >
> > > /Bruce
> >
> > The driver functions were static, I have made them public and added
> > them to the rte_pmd_ixgbe.h file, and it works.  These functions will also
> need to be added to the rte_pmd_ixgbe_version.map file, previously there
> were no public functions.
> 
> Sorry for being late in the discussion, but I have a question:
> If we  this way (force user to include driver specific headers and call driver
> specific functions), how you guys plan to make this functionality available for
> multiple driver types.
> From discussion with Bernard  understand that customers would need similar
> functionality for i40e.
> Does it mean that they'll have to re-implement this part of their code again?
> Or would have to create (and maintain) their own shim layer that would
> provide some s of abstraction?
> Basically their own version of rte_ethdev?
> 
> Konstantin

Adding the pmd_ops field to struct eth_devops {} discussed previously in this email thread will allow driver specific functions for multiple drivers and will get rid of the driver specific header file rte_pmd_driver.h. 

Regards,

Bernard.
  

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Wednesday, September 28, 2016 12:24 PM
> To: Iremonger, Bernard <bernard.iremonger@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org; Jerin Jacob
> <jerin.jacob@caviumnetworks.com>; Shah, Rahul R <rahul.r.shah@intel.com>;
> Lu, Wenzhuo <wenzhuo.lu@intel.com>; azelezniak <alexz@att.com>
> Subject: RE: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> Hi lads,
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Iremonger,
> > Bernard
> > Sent: Tuesday, September 27, 2016 3:13 PM
> > To: Richardson, Bruce <bruce.richardson@intel.com>
> > Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org; Jerin
> > Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul R
> > <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> > azelezniak <alexz@att.com>
> > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for
> > VF management
> >
> > Hi Bruce,
> >
> > <snip>
> >
> > > > > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add
> > > > > > > API's for VF management
> > > > > > >
> > > > > > > 2016-09-23 17:02, Iremonger, Bernard:
> > > > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > 2016-09-23 09:53, Richardson, Bruce:
> > > > > > > > > > From: Thomas Monjalon
> > > > > > > > > > [mailto:thomas.monjalon@6wind.com]
> > > > > > > > > > > 2016-09-23 10:20, Bruce Richardson:
> > > > > > > > > > > > On Thu, Sep 22, 2016 at 07:04:37PM +0200, Thomas
> > > > > > > > > > > > Monjalon
> > > > > wrote:
> > > > > > > > > > > > > 2016-09-15 16:46, Iremonger, Bernard:
> > > > > > > > > > > > > > > > > Do we really need to expose VF specific
> > > > > > > > > > > > > > > > > functions
> > > > > here?
> > > > > > > > > > > > > > > > > It can be generic(PF/VF) function
> > > > > > > > > > > > > > > > > indexed only through
> > > > > > > > > > > port_id.
> > > > > > > > > > > > > > > > > (example: as
> > > > > > > > > > > > > > > > > rte_eth_dev_set_vlan_anti_spoof(uint8_t
> > > > > > > > > > > > > > > > > port_id, uint8_t on)) For instance, In
> > > > > > > > > > > > > > > > > Thunderx PMD, We are not exposing a
> > > > > > > > > > > > > > > > > separate port_id for PF. We only
> > > > > > > > > > > > > > > > > enumerate 0..N VFs as 0..N ethdev
> > > > > > > > > > > > > > > > > port_id
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > Our intention with this patch is to
> > > > > > > > > > > > > > > > control the VF from the
> > > > > PF.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > The following librte_ether functions
> > > > > > > > > > > > > > > > already work in a similar
> > > > > > > > > > > way:
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_rxmode(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint16_t rx_mode, uint8_t on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_rx(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > rte_eth_dev_set_vf_tx(uint8_t port_id,
> > > > > > > > > > > > > > > > uint16_t vf, uint8_t
> > > > > > > > > > > > > > > > on)
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > int rte_eth_set_vf_rate_limit(uint8_t
> > > > > > > > > > > > > > > > port_id, uint16_t vf, uint16_t tx_rate,
> > > > > > > > > > > > > > > > uint64_t q_msk)
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > I have a bad feeling with these functions
> > > > > > > > > > > > > > > dedicated to VF from
> > > > > > > PF.
> > > > > > > > > > > > > > > Are we sure there is no other way?
> > > > > > > > > > > > > > > I mean we just need to know the VF with a port
> ID.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > When the VF is used in a VM the port ID of the
> > > > > > > > > > > > > > VF is not visible to
> > > > > > > > > > > the PF.
> > > > > > > > > > > > > > I don't think there is another way to do this.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I don't understand why we could not assign a
> > > > > > > > > > > > > port id to the VF from the host instead of
> > > > > > > > > > > > > having the couple PF port id /
> > > > > VF id.
> > > > > > > > > > > > > Can we enumerate all the VFs associated to a PF?
> > > > > > > > > > > > > Then can we allocate them a port id in the array
> > > > > rte_eth_devices?
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Thomas,
> > > > > > > > > > > >
> > > > > > > > > > > > The VF is not a port visible to DPDK, though, so
> > > > > > > > > > > > it shouldn't have a port id IMHO. DPDK can't
> > > > > > > > > > > > actually do anything
> > > > > with it.
> > > > > > > > > > >
> > > > > > > > > > > You say the contrary below.
> > > > > > > > > >
> > > > > > > > > > Well, yes and no. The driver can manipulate things for
> > > > > > > > > > the VF, but DPDK
> > > > > > > > > doesn't actually have a device that corresponds to the VF.
> > > > > > > > > There are no PCI bar mappings for it, DPDK can't do RX
> > > > > > > > > and TX with
> > > it etc.?
> > > > > > > > >
> > > > > > > > > Very good point.
> > > > > > > > > There are only few ethdev functions which are supported
> > > > > > > > > by every drivers, like Rx/Tx and would not be available
> > > > > > > > > for VF from PF
> > > > > interface.
> > > > > > > > >
> > > > > > > > > > > > The PCI device for the VF is likely passed through
> > > > > > > > > > > > to a different VM and being used there.
> > > > > > > > > > > > Unfortunately, the VF still needs certain things
> > > > > > > > > > > > done for it by the PF, so if the PF is under DPDK
> > > > > > > > > > > > control, it needs to provide the functionality to
> > > > > > > > > > > > assist
> > > > > > > the VF.
> > > > > > > > > > >
> > > > > > > > > > > Why not have a VF_from_PF driver which does the
> > > > > > > > > > > mailbox
> > > > > things?
> > > > > > > > > > > So you can manage the VF from the PF with a simple
> port id.
> > > > > > > > > > > It really seems to be the cleanest design to me.
> > > > > > > > > >
> > > > > > > > > > While I see your point, and it could work, I just want
> > > > > > > > > > to be sure that we are
> > > > > > > > > ok with the results of that. Suppose we do create
> > > > > > > > > ethdevs for the VFs controlled by the PF. Does the new
> > > > > > > > > VF get counted in the
> > > > > > > > > rte_eth_dev_count() value (I assume yes)? How are apps
> > > > > > > > > meant to use the port? Do they have to put in a special
> > > > > > > > > case when iterating through all the port ids to check
> > > > > > > > > that it's not a pseudo port that can't do anything. None
> > > > > > > > > of the standard ethdev calls from an app will work on
> > > > > > > > > it, you can't configure nb rx/tx queues on it, you can't
> > > > > > > > > start or
> > > > > > > stop it, you can't do rx or tx on it, etc, etc.
> > > > > > > > >
> > > > > > > > > Yes these devices would be special because their
> > > > > > > > > supported API would be quite different. I was thinking
> > > > > > > > > that in the future you could add most of the
> > > > > > > > > configuration functions through the VF
> > > > > mailbox.
> > > > > > > > > But the Intel mailbox currently support only some
> > > > > > > > > special configurations which are not supported by other
> > > > > > > > > devices even its own VF device (except setting MAC
> address).
> > > > > > > > > And when I read "set drop enable bit in the VF split rx
> > > > > > > > > control register", it becomes clear it is really
> > > > > > > > > specific and has nothing to do in the generic ethdev API.
> > > > > > > > > That's why it is a NACK.
> > > > > > > > >
> > > > > > > > > When we want to use these very specific features we are
> > > > > > > > > aware of the underlying device and driver. So we can
> > > > > > > > > directly include a header from the driver. I suggest to
> > > > > > > > > retrieve a handler for the device which is not a port id
> > > > > > > > > and will allow to call ixgbe functions
> > > > > directly.
> > > > > > > > > It could be achieved by adding an ethdev function like
> > > > > > > > > discussed
> > > here:
> > > > > > > > > 	http://dpdk.org/ml/archives/dev/2016-
> > > September/047392.html
> > > > > > > > >
> > > > > > > >
> > > > > > > > I have been reading the net/vhost mail thread above. The
> > > > > > > > following quote
> > > > > > > is from this thread.
> > > > > > > >
> > > > > > > > "It means I would be in favor of introducing API in
> > > > > > > > drivers for very specific
> > > > > > > features."
> > > > > > > >
> > > > > > > > At present all the PMD functions are accessed through the
> > > > > > > > eth_dev_ops
> > > > > > > structure, there are no PMD API's.
> > > > > > > >
> > > > > > > > Is your proposal to add API(s) to the DPDK ixgbe PMD
> > > > > > > > (similar to a driver
> > > > > > > ioctl API) which can be accessed through a generic API in the
> ethdev?
> > > > > > >
> > > > > > > Not exactly. I'm thinking about a PMD specific API.
> > > > > > > The only ethdev API you need would be a function to retrieve
> > > > > > > a handler (an opaque pointer on the device struct) from the
> port id.
> > > > > > > Then you can include rte_ixgbe.h and directly call the
> > > > > > > specific ixgbe function, passing the device handler.
> > > > > > > How does it sound?
> > > > > >
> > > > > > I have been prototyping this proposed solution, it appears to
> work.
> > > > > >
> > > > > > I have added the following function:
> > > > > >
> > > > > > int  rte_eth_dev_get_pmd_handle(uint8_t port_id, void**
> > > > > > pmd_handle);
> > > > > >
> > > > > > The pmd_handle is a pointer to a dev_ops structure containing
> > > > > > driver
> > > > > specific functions.
> > > > > >
> > > > > > Using the pmd_handle the driver specific functions can be
> > > > > > called (without having them in struct eth_dev_ops)
> > > > > >
> > > > > > Has this proposal been superseded by the discussion on the
> > > > > > following
> > > > > patch?
> > > > > >
> > > > > > [PATCH] net/vhost: Add function to retreive the 'vid' for a
> > > > > > given port id
> > > > >
> > > > > Maybe, it can be superseded by this discussion, yes.
> > > > > Bruce thinks we do not need rte_eth_dev_get_pmd_handle().
> > > > > What is your opinion about using port_id directly and retrieving
> > > > > the structs from the driver via rte_eth_devices?
> > > >
> > > > Looking at the code in rte_eth_devices[]
> > > >
> > > > struct rte_eth_dev  rte_eth_devices[RTE_MAX_ETHPORTS];
> > > >
> > > > struct rte_eth_dev {
> > > >
> > > > ...
> > > >
> > > > const struct eth_dev_ops *dev_ops; /**< Functions exported by PMD
> > > > */
> > > >
> > > > ...
> > > >
> > > >  void *pmd_ops;  /** < exported PMD specific functions */
> > > >
> > > > }
> > > >
> > > > The PMD functions are only accessible at present if they are in
> > > > struct
> > > eth_dev_ops.
> > > >
> > > > Adding a pmd_ops field to struct rte_eth_dev {} makes the PMD
> > > > functions
> > > accessible and is a simpler solution than using
> > > rte_eth_dev_get_pmd_handle() to get access to the PMD functions.
> > > >
> > > > Regards,
> > > >
> > >
> > > Why would an ops structure be needed? If it's a private API for a
> > > driver, there should be no need for function pointers, and instead
> > > the driver can define regular functions in it's header file, no?
> > >
> > > /Bruce
> >
> > The driver functions were static, I have made them public and added
> > them to the rte_pmd_ixgbe.h file, and it works.  These functions will
> also need to be added to the rte_pmd_ixgbe_version.map file, previously
> there were no public functions.
> 
> Sorry for being late in the discussion, but I have a question:
> If we  this way (force user to include driver specific headers and call
> driver specific functions), how you guys plan to make this functionality
> available for multiple driver types.
> From discussion with Bernard  understand that customers would need similar
> functionality for i40e.
> Does it mean that they'll have to re-implement this part of their code
> again?
> Or would have to create (and maintain) their own shim layer that would
> provide some s of abstraction?
> Basically their own version of rte_ethdev?
> 
> Konstantin

Yes, it's a problem. However, the concern right now is that this functionality is not generic across NICs, and will only be implemented for ixgbe and i40e. If it turns out to be implemented for a significant subset of DPDK PMDs, e.g. 4 or 5 drivers from at least two vendors, then I would expect the case to be made to "promote" this functionality to ethdev.

Does this sound reasonable. Any other thoughts?

/Bruce
  

2016-09-28 11:23, Ananyev, Konstantin:
> If we  this way (force user to include driver specific headers and call driver specific functions),
> how you guys plan to make this functionality available for multiple driver types.

Multiple drivers won't have exactly the same specific features.
But yes, there are some things common to several Intel NICs.

> From discussion with Bernard  understand that customers would need similar functionality for i40e.
> Does it mean that they'll have to re-implement this part of their code again?
> Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> Basically their own version of rte_ethdev?

No definitive answer.
But we can argue the contrary: how to handle a generic API which is implemented
only in 1 or 2 drivers? If the application tries to use it, we can imagine
that a specific range of hardware is expected.

I think it is an important question.
Previously we had the issue of having some API which are too specific
and need a rework to be used with other NICs. In order to avoid such
rework and API break, we can try to make them available in a driver-specific
or vendor-specific staging area, waiting for a later generalization.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, September 28, 2016 2:03 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul R <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> azelezniak <alexz@att.com>
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF management
> 
> 2016-09-28 11:23, Ananyev, Konstantin:
> > If we  this way (force user to include driver specific headers and
> > call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> 
> Multiple drivers won't have exactly the same specific features.
> But yes, there are some things common to several Intel NICs.
> 
> > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > Does it mean that they'll have to re-implement this part of their code again?
> > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > Basically their own version of rte_ethdev?
> 
> No definitive answer.
> But we can argue the contrary: how to handle a generic API which is implemented only in 1 or 2 drivers? If the application tries to use
> it, we can imagine that a specific range of hardware is expected.

Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the app.

> 
> I think it is an important question.
> Previously we had the issue of having some API which are too specific and need a rework to be used with other NICs. In order to avoid
> such rework and API break, we can try to make them available in a driver-specific or vendor-specific staging area, waiting for a later
> generalization.

Could you remind me why you guys were that opposed to ioctl style approach?
It is not my favorite thing either, but it seems pretty generic way to handle such situations.

Konstantin
  

2016-09-28 13:26, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-09-28 11:23, Ananyev, Konstantin:
> > > If we  this way (force user to include driver specific headers and
> > > call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > 
> > Multiple drivers won't have exactly the same specific features.
> > But yes, there are some things common to several Intel NICs.
> > 
> > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > Does it mean that they'll have to re-implement this part of their code again?
> > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > Basically their own version of rte_ethdev?
> > 
> > No definitive answer.
> > But we can argue the contrary: how to handle a generic API which is implemented only in 1 or 2 drivers? If the application tries to use
> > it, we can imagine that a specific range of hardware is expected.
> 
> Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the app.
> 
> > 
> > I think it is an important question.
> > Previously we had the issue of having some API which are too specific and need a rework to be used with other NICs. In order to avoid
> > such rework and API break, we can try to make them available in a driver-specific or vendor-specific staging area, waiting for a later
> > generalization.
> 
> Could you remind me why you guys were that opposed to ioctl style approach?
> It is not my favorite thing either, but it seems pretty generic way to handle such situations.

We prefer having well-defined functions instead of opaque ioctl-style encoding.
And it was not clear what is the benefit of ioctl.
Now I think I understand you would like to have a common ioctl service for
features available on 2 drivers. Right? Example (trying to read your mind):
	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>);
instead of
	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
	rte_pmd_i40e_vf_ping(port_id, vf_id);
Please confirm I understand what you are thinking about.
  

> -----Original Message-----
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> Sent: Wednesday, September 28, 2016 3:24 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Cc: Iremonger, Bernard <bernard.iremonger@intel.com>; Richardson, Bruce <bruce.richardson@intel.com>; dev@dpdk.org; Jerin
> Jacob <jerin.jacob@caviumnetworks.com>; Shah, Rahul R <rahul.r.shah@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>;
> azelezniak <alexz@att.com>
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF management
> 
> 2016-09-28 13:26, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > If we  this way (force user to include driver specific headers and
> > > > call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > >
> > > Multiple drivers won't have exactly the same specific features.
> > > But yes, there are some things common to several Intel NICs.
> > >
> > > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > > Does it mean that they'll have to re-implement this part of their code again?
> > > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > > Basically their own version of rte_ethdev?
> > >
> > > No definitive answer.
> > > But we can argue the contrary: how to handle a generic API which is
> > > implemented only in 1 or 2 drivers? If the application tries to use it, we can imagine that a specific range of hardware is expected.
> >
> > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> > Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the app.
> >
> > >
> > > I think it is an important question.
> > > Previously we had the issue of having some API which are too
> > > specific and need a rework to be used with other NICs. In order to
> > > avoid such rework and API break, we can try to make them available in a driver-specific or vendor-specific staging area, waiting for
> a later generalization.
> >
> > Could you remind me why you guys were that opposed to ioctl style approach?
> > It is not my favorite thing either, but it seems pretty generic way to handle such situations.
> 
> We prefer having well-defined functions instead of opaque ioctl-style encoding.
> And it was not clear what is the benefit of ioctl.
> Now I think I understand you would like to have a common ioctl service for features available on 2 drivers. Right?

Yes.

> Example (trying to  read your mind):
> 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>); instead of
> 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> 	rte_pmd_i40e_vf_ping(port_id, vf_id);
> Please confirm I understand what you are thinking about.

Yep, you read my mind correctly :)
Konstantin
  

<snip>

> > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for
> > VF management
> >
> > 2016-09-28 13:26, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > If we  this way (force user to include driver specific headers
> > > > > and call driver specific functions), how you guys plan to make this
> functionality available for multiple driver types.
> > > >
> > > > Multiple drivers won't have exactly the same specific features.
> > > > But yes, there are some things common to several Intel NICs.
> > > >
> > > > > From discussion with Bernard  understand that customers would
> need similar functionality for i40e.
> > > > > Does it mean that they'll have to re-implement this part of their code
> again?
> > > > > Or would have to create (and maintain) their own shim layer that
> would provide some s of abstraction?
> > > > > Basically their own version of rte_ethdev?
> > > >
> > > > No definitive answer.
> > > > But we can argue the contrary: how to handle a generic API which
> > > > is implemented only in 1 or 2 drivers? If the application tries to use it,
> we can imagine that a specific range of hardware is expected.
> > >
> > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs
> for now, but different models/drivers).
> > > Obviously users would like to have an ability to run their app on all HW
> from this subset without rebuilding/implementing the app.
> > >
> > > >
> > > > I think it is an important question.
> > > > Previously we had the issue of having some API which are too
> > > > specific and need a rework to be used with other NICs. In order to
> > > > avoid such rework and API break, we can try to make them available
> > > > in a driver-specific or vendor-specific staging area, waiting for
> > a later generalization.
> > >
> > > Could you remind me why you guys were that opposed to ioctl style
> approach?
> > > It is not my favorite thing either, but it seems pretty generic way to
> handle such situations.
> >
> > We prefer having well-defined functions instead of opaque ioctl-style
> encoding.
> > And it was not clear what is the benefit of ioctl.
> > Now I think I understand you would like to have a common ioctl service for
> features available on 2 drivers. Right?
> 
> Yes.
> 
> > Example (trying to  read your mind):
> > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF
> id>); instead of
> > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > what you are thinking about.
> 
> Yep, you read my mind correctly :)
> Konstantin
> 
Adding the pmd_ops field to struct eth_devops {} discussed previously in this email thread will allow driver specific functions for multiple drivers and will get rid of the driver specific header file rte_pmd_driver.h.
Would this be an acceptable solution?

Regards,

Bernard.
  

2016-09-28 14:30, Ananyev, Konstantin:
> From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > 2016-09-28 13:26, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > If we  this way (force user to include driver specific headers and
> > > > > call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > > >
> > > > Multiple drivers won't have exactly the same specific features.
> > > > But yes, there are some things common to several Intel NICs.
> > > >
> > > > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > > > Does it mean that they'll have to re-implement this part of their code again?
> > > > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > > > Basically their own version of rte_ethdev?
> > > >
> > > > No definitive answer.
> > > > But we can argue the contrary: how to handle a generic API which is
> > > > implemented only in 1 or 2 drivers? If the application tries to use it, we can imagine that a specific range of hardware is expected.
> > >
> > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> > > Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the app.
> > >
> > > >
> > > > I think it is an important question.
> > > > Previously we had the issue of having some API which are too
> > > > specific and need a rework to be used with other NICs. In order to
> > > > avoid such rework and API break, we can try to make them available in a driver-specific or vendor-specific staging area, waiting for
> > a later generalization.
> > >
> > > Could you remind me why you guys were that opposed to ioctl style approach?
> > > It is not my favorite thing either, but it seems pretty generic way to handle such situations.
> > 
> > We prefer having well-defined functions instead of opaque ioctl-style encoding.
> > And it was not clear what is the benefit of ioctl.
> > Now I think I understand you would like to have a common ioctl service for features available on 2 drivers. Right?
> 
> Yes.
> 
> > Example (trying to  read your mind):
> > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>); instead of
> > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > 	rte_pmd_i40e_vf_ping(port_id, vf_id);
> > Please confirm I understand what you are thinking about.
> 
> Yep, you read my mind correctly :)

Both could coexist (if ioctl was accepted by community).
What about starting to implement the PMD functions and postpone
ioctl to later with a dedicated thread?
  

2016-09-28 14:48, Iremonger, Bernard:
> <snip>
> 
> > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for
> > > VF management
> > >
> > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > > If we  this way (force user to include driver specific headers
> > > > > > and call driver specific functions), how you guys plan to make this
> > functionality available for multiple driver types.
> > > > >
> > > > > Multiple drivers won't have exactly the same specific features.
> > > > > But yes, there are some things common to several Intel NICs.
> > > > >
> > > > > > From discussion with Bernard  understand that customers would
> > need similar functionality for i40e.
> > > > > > Does it mean that they'll have to re-implement this part of their code
> > again?
> > > > > > Or would have to create (and maintain) their own shim layer that
> > would provide some s of abstraction?
> > > > > > Basically their own version of rte_ethdev?
> > > > >
> > > > > No definitive answer.
> > > > > But we can argue the contrary: how to handle a generic API which
> > > > > is implemented only in 1 or 2 drivers? If the application tries to use it,
> > we can imagine that a specific range of hardware is expected.
> > > >
> > > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs
> > for now, but different models/drivers).
> > > > Obviously users would like to have an ability to run their app on all HW
> > from this subset without rebuilding/implementing the app.
> > > >
> > > > >
> > > > > I think it is an important question.
> > > > > Previously we had the issue of having some API which are too
> > > > > specific and need a rework to be used with other NICs. In order to
> > > > > avoid such rework and API break, we can try to make them available
> > > > > in a driver-specific or vendor-specific staging area, waiting for
> > > a later generalization.
> > > >
> > > > Could you remind me why you guys were that opposed to ioctl style
> > approach?
> > > > It is not my favorite thing either, but it seems pretty generic way to
> > handle such situations.
> > >
> > > We prefer having well-defined functions instead of opaque ioctl-style
> > encoding.
> > > And it was not clear what is the benefit of ioctl.
> > > Now I think I understand you would like to have a common ioctl service for
> > features available on 2 drivers. Right?
> > 
> > Yes.
> > 
> > > Example (trying to  read your mind):
> > > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF
> > id>); instead of
> > > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > > what you are thinking about.
> > 
> > Yep, you read my mind correctly :)
> > Konstantin
> > 
> Adding the pmd_ops field to struct eth_devops {} discussed previously in this email thread will allow driver specific functions for multiple drivers and will get rid of the driver specific header file rte_pmd_driver.h.
> Would this be an acceptable solution?

How pmd_ops would be different of eth_devops?
  

Hi Thomas,

<snip>

> -----Original Message-----
> Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF
> management
> 
> 2016-09-28 14:48, Iremonger, Bernard:
> > <snip>
> >
> > > > Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's
> > > > for VF management
> > > >
> > > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > > > If we  this way (force user to include driver specific
> > > > > > > headers and call driver specific functions), how you guys
> > > > > > > plan to make this
> > > functionality available for multiple driver types.
> > > > > >
> > > > > > Multiple drivers won't have exactly the same specific features.
> > > > > > But yes, there are some things common to several Intel NICs.
> > > > > >
> > > > > > > From discussion with Bernard  understand that customers
> > > > > > > would
> > > need similar functionality for i40e.
> > > > > > > Does it mean that they'll have to re-implement this part of
> > > > > > > their code
> > > again?
> > > > > > > Or would have to create (and maintain) their own shim layer
> > > > > > > that
> > > would provide some s of abstraction?
> > > > > > > Basically their own version of rte_ethdev?
> > > > > >
> > > > > > No definitive answer.
> > > > > > But we can argue the contrary: how to handle a generic API
> > > > > > which is implemented only in 1 or 2 drivers? If the
> > > > > > application tries to use it,
> > > we can imagine that a specific range of hardware is expected.
> > > > >
> > > > > Yes, as I understand, it is a specific subset of supported HW
> > > > > (just Inel NICs
> > > for now, but different models/drivers).
> > > > > Obviously users would like to have an ability to run their app
> > > > > on all HW
> > > from this subset without rebuilding/implementing the app.
> > > > >
> > > > > >
> > > > > > I think it is an important question.
> > > > > > Previously we had the issue of having some API which are too
> > > > > > specific and need a rework to be used with other NICs. In
> > > > > > order to avoid such rework and API break, we can try to make
> > > > > > them available in a driver-specific or vendor-specific staging
> > > > > > area, waiting for
> > > > a later generalization.
> > > > >
> > > > > Could you remind me why you guys were that opposed to ioctl
> > > > > style
> > > approach?
> > > > > It is not my favorite thing either, but it seems pretty generic
> > > > > way to
> > > handle such situations.
> > > >
> > > > We prefer having well-defined functions instead of opaque
> > > > ioctl-style
> > > encoding.
> > > > And it was not clear what is the benefit of ioctl.
> > > > Now I think I understand you would like to have a common ioctl
> > > > service for
> > > features available on 2 drivers. Right?
> > >
> > > Yes.
> > >
> > > > Example (trying to  read your mind):
> > > > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF
> > > id>); instead of
> > > > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > > > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > > > what you are thinking about.
> > >
> > > Yep, you read my mind correctly :)
> > > Konstantin
> > >
> > Adding the pmd_ops field to struct eth_devops {} discussed previously in
> this email thread will allow driver specific functions for multiple drivers and
> will get rid of the driver specific header file rte_pmd_driver.h.
> > Would this be an acceptable solution?
> 
> How pmd_ops would be different of eth_devops?

There is not a lot of difference, however it would separate generic ethdev functions from driver specific functions.

Regards,

Bernard.
  

> 
> 2016-09-28 14:30, Ananyev, Konstantin:
> > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > > If we  this way (force user to include driver specific headers
> > > > > > and call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > > > >
> > > > > Multiple drivers won't have exactly the same specific features.
> > > > > But yes, there are some things common to several Intel NICs.
> > > > >
> > > > > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > > > > Does it mean that they'll have to re-implement this part of their code again?
> > > > > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > > > > Basically their own version of rte_ethdev?
> > > > >
> > > > > No definitive answer.
> > > > > But we can argue the contrary: how to handle a generic API which
> > > > > is implemented only in 1 or 2 drivers? If the application tries to use it, we can imagine that a specific range of hardware is
> expected.
> > > >
> > > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> > > > Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the
> app.
> > > >
> > > > >
> > > > > I think it is an important question.
> > > > > Previously we had the issue of having some API which are too
> > > > > specific and need a rework to be used with other NICs. In order
> > > > > to avoid such rework and API break, we can try to make them
> > > > > available in a driver-specific or vendor-specific staging area,
> > > > > waiting for
> > > a later generalization.
> > > >
> > > > Could you remind me why you guys were that opposed to ioctl style approach?
> > > > It is not my favorite thing either, but it seems pretty generic way to handle such situations.
> > >
> > > We prefer having well-defined functions instead of opaque ioctl-style encoding.
> > > And it was not clear what is the benefit of ioctl.
> > > Now I think I understand you would like to have a common ioctl service for features available on 2 drivers. Right?
> >
> > Yes.
> >
> > > Example (trying to  read your mind):
> > > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>); instead of
> > > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > > what you are thinking about.
> >
> > Yep, you read my mind correctly :)
> 
> Both could coexist (if ioctl was accepted by community).

True.

> What about starting to implement the PMD functions and postpone ioctl to later with a dedicated thread?

You mean something like:
- 16.11: implement rte_pmd_ixgbe_vf_ping()
- 17.02:
	a) implement rte_pmd_i40e_vf_ping()
	b) introduce ioctl PMD API
	c) make possible to vf_ping via ioctl API
?
If so, then it sounds like reasonable approach to me.
Though would be inserting to hear what other guys think.
Konstantin
  

2016-09-28 16:52, Ananyev, Konstantin:
> 
> > 
> > 2016-09-28 14:30, Ananyev, Konstantin:
> > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > > > If we  this way (force user to include driver specific headers
> > > > > > > and call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > > > > >
> > > > > > Multiple drivers won't have exactly the same specific features.
> > > > > > But yes, there are some things common to several Intel NICs.
> > > > > >
> > > > > > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > > > > > Does it mean that they'll have to re-implement this part of their code again?
> > > > > > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > > > > > Basically their own version of rte_ethdev?
> > > > > >
> > > > > > No definitive answer.
> > > > > > But we can argue the contrary: how to handle a generic API which
> > > > > > is implemented only in 1 or 2 drivers? If the application tries to use it, we can imagine that a specific range of hardware is
> > expected.
> > > > >
> > > > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> > > > > Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the
> > app.
> > > > >
> > > > > >
> > > > > > I think it is an important question.
> > > > > > Previously we had the issue of having some API which are too
> > > > > > specific and need a rework to be used with other NICs. In order
> > > > > > to avoid such rework and API break, we can try to make them
> > > > > > available in a driver-specific or vendor-specific staging area,
> > > > > > waiting for
> > > > a later generalization.
> > > > >
> > > > > Could you remind me why you guys were that opposed to ioctl style approach?
> > > > > It is not my favorite thing either, but it seems pretty generic way to handle such situations.
> > > >
> > > > We prefer having well-defined functions instead of opaque ioctl-style encoding.
> > > > And it was not clear what is the benefit of ioctl.
> > > > Now I think I understand you would like to have a common ioctl service for features available on 2 drivers. Right?
> > >
> > > Yes.
> > >
> > > > Example (trying to  read your mind):
> > > > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>); instead of
> > > > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > > > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > > > what you are thinking about.
> > >
> > > Yep, you read my mind correctly :)
> > 
> > Both could coexist (if ioctl was accepted by community).
> 
> True.
> 
> > What about starting to implement the PMD functions and postpone ioctl to later with a dedicated thread?
> 
> You mean something like:
> - 16.11: implement rte_pmd_ixgbe_vf_ping()
> - 17.02:
> 	a) implement rte_pmd_i40e_vf_ping()
> 	b) introduce ioctl PMD API
> 	c) make possible to vf_ping via ioctl API
> ?
> If so, then it sounds like reasonable approach to me.
> Though would be inserting to hear what other guys think.

Yes.
I would just add that we have to start a discussion thread to decide
wether we'll add an ioctl call in 17.02 or not.
  

On Wed, Sep 28, 2016 at 08:02:21PM +0200, Thomas Monjalon wrote:
> 2016-09-28 16:52, Ananyev, Konstantin:
> > 
> > > 
> > > 2016-09-28 14:30, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com]
> > > > > > > 2016-09-28 11:23, Ananyev, Konstantin:
> > > > > > > > If we  this way (force user to include driver specific headers
> > > > > > > > and call driver specific functions), how you guys plan to make this functionality available for multiple driver types.
> > > > > > >
> > > > > > > Multiple drivers won't have exactly the same specific features.
> > > > > > > But yes, there are some things common to several Intel NICs.
> > > > > > >
> > > > > > > > From discussion with Bernard  understand that customers would need similar functionality for i40e.
> > > > > > > > Does it mean that they'll have to re-implement this part of their code again?
> > > > > > > > Or would have to create (and maintain) their own shim layer that would provide some s of abstraction?
> > > > > > > > Basically their own version of rte_ethdev?
> > > > > > >
> > > > > > > No definitive answer.
> > > > > > > But we can argue the contrary: how to handle a generic API which
> > > > > > > is implemented only in 1 or 2 drivers? If the application tries to use it, we can imagine that a specific range of hardware is
> > > expected.
> > > > > >
> > > > > > Yes, as I understand, it is a specific subset of supported HW (just Inel NICs for now, but different models/drivers).
> > > > > > Obviously users would like to have an ability to run their app on all HW from this subset without rebuilding/implementing the
> > > app.
> > > > > >
> > > > > > >
> > > > > > > I think it is an important question.
> > > > > > > Previously we had the issue of having some API which are too
> > > > > > > specific and need a rework to be used with other NICs. In order
> > > > > > > to avoid such rework and API break, we can try to make them
> > > > > > > available in a driver-specific or vendor-specific staging area,
> > > > > > > waiting for
> > > > > a later generalization.
> > > > > >
> > > > > > Could you remind me why you guys were that opposed to ioctl style approach?
> > > > > > It is not my favorite thing either, but it seems pretty generic way to handle such situations.
> > > > >
> > > > > We prefer having well-defined functions instead of opaque ioctl-style encoding.
> > > > > And it was not clear what is the benefit of ioctl.
> > > > > Now I think I understand you would like to have a common ioctl service for features available on 2 drivers. Right?
> > > >
> > > > Yes.
> > > >
> > > > > Example (trying to  read your mind):
> > > > > 	rte_ethdev_ioctl(port_id, <TLV encoding VF_PING service and VF id>); instead of
> > > > > 	rte_pmd_ixgbe_vf_ping(port_id, vf_id);
> > > > > 	rte_pmd_i40e_vf_ping(port_id, vf_id); Please confirm I understand
> > > > > what you are thinking about.
> > > >
> > > > Yep, you read my mind correctly :)
> > > 
> > > Both could coexist (if ioctl was accepted by community).
> > 
> > True.
> > 
> > > What about starting to implement the PMD functions and postpone ioctl to later with a dedicated thread?
> > 
> > You mean something like:
> > - 16.11: implement rte_pmd_ixgbe_vf_ping()
> > - 17.02:
> > 	a) implement rte_pmd_i40e_vf_ping()
> > 	b) introduce ioctl PMD API
> > 	c) make possible to vf_ping via ioctl API
> > ?
> > If so, then it sounds like reasonable approach to me.
> > Though would be inserting to hear what other guys think.
> 
> Yes.
> I would just add that we have to start a discussion thread to decide
> wether we'll add an ioctl call in 17.02 or not.

I still don't like IOCTLs as an option, I still think that they are awkward to
use and hinder code readability. Here is an addition suggestion to get people
thinking.

How about allowing a set of "vendor-specific" operations for each device. We
could do this using Bernard's suggestion of adding a set of additional 
function pointers to the device struct. The reason for doing this is that it
would allow us to group functionality into families of functionality, rather
than having to things either fully generically or specifically to each NIC. If
we look at the capabilities of most NICs, the other NICs which support similar
functions are generally other NICs from the same vendor. We could therefore
have ops common across ixgbe and i40e and other ops mlx4 and mlx5 drivers.

Could such a scheme work? Would it be worth doing?

/Bruce