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

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

[ZELEZNIAK, ALEX]

----------------------------------------

From: "az5157@att.com<mailto:az5157@att.com>" <az5157@att.com<mailto:az5157@att.com>>
Date: Wednesday, September 28, 2016 at 12:23:06 PM
To: "Thomas Monjalon" <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>>, "Iremonger, Bernard" <bernard.iremonger@intel.com<mailto:bernard.iremonger@intel.com>>, "Richardson, Bruce" <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>, "dev@dpdk.org<mailto:dev@dpdk.org>" <dev@dpdk.org<mailto:dev@dpdk.org>>, "Jerin Jacob" <jerin.jacob@caviumnetworks.com<mailto:jerin.jacob@caviumnetworks.com>>, "Shah, Rahul R" <rahul.r.shah@intel.com<mailto:rahul.r.shah@intel.com>>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com<mailto:wenzhuo.lu@intel.com>>
Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF management

>
> 2016-09-28 16:52, Ananyev, Konstantin:
> >
> > >
> > > 2016-09-28 14:30, Ananyev, Konstantin:
> > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>]
> > > > > 2016-09-28 13:26, Ananyev, Konstantin:
> > > > > > From: Thomas Monjalon [mailto:thomas.monjalon@6wind.com<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.
>

Looks good to me.
Thanks,
Alex Z.

________________________________

From: "az5157@att.com" <az5157@att.com<mailto:az5157@att.com>>
Date: Wednesday, September 28, 2016 at 12:23:06 PM
To: "Thomas Monjalon" <thomas.monjalon@6wind.com<mailto:thomas.monjalon@6wind.com>>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com<mailto:konstantin.ananyev@intel.com>>, "Iremonger, Bernard" <bernard.iremonger@intel.com<mailto:bernard.iremonger@intel.com>>, "Richardson, Bruce" <bruce.richardson@intel.com<mailto:bruce.richardson@intel.com>>, "dev@dpdk.org" <dev@dpdk.org<mailto:dev@dpdk.org>>, "Jerin Jacob" <jerin.jacob@caviumnetworks.com<mailto:jerin.jacob@caviumnetworks.com>>, "Shah, Rahul R" <rahul.r.shah@intel.com<mailto:rahul.r.shah@intel.com>>, "Lu, Wenzhuo" <wenzhuo.lu@intel.com<mailto:wenzhuo.lu@intel.com>>
Subject: Re: [dpdk-dev] [RFC PATCH v2 3/5] librte_ether: add API's for VF management

[dpdk-dev] [RFC PATCH 0/5] add API's for VF management

[Bernard Iremonger]

This RFC patchset contains new DPDK API's requested by AT&T for use
with the Virtual Function Daemon (VFD).

The need to configure and manage VF's on a NIC has grown to the
point where AT&T have devloped a DPDK based tool, VFD, to do this.

This RFC proposes to add the following API extensions to DPDK:
  mailbox communication callback support
  VF configuration

Nine new functions have been added to the eth_dev_ops structure.
Corresponding functions have been added to the ixgbe PMD for the
Niantic NIC.

Two new callback functions have been added.
Changes have been made to the ixgbe_rcv_msg_from_vf function to
use the callback functions.

Changes have been made to testpmd to facilitate testing of the new API's.
The testpmd documentation has been updated to document the testpmd changes.

Note:
Adding new functions to the eth_dev_ops structure will cause an
ABI breakage.

Bernard Iremonger (5):
  librte_ether: add internal callback functions
  net/ixgbe: add callback to user app on VF to PF mbox msg
  librte_ether: add API's for VF management
  net/ixgbe: add functions for VF management
  app/test_pmd: add tests for new API's

 app/test-pmd/cmdline.c                      | 700 ++++++++++++++++++++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  68 ++-
 drivers/net/ixgbe/ixgbe_ethdev.c            | 179 +++++++
 drivers/net/ixgbe/ixgbe_pf.c                |  39 +-
 lib/librte_ether/rte_ethdev.c               | 176 +++++++
 lib/librte_ether/rte_ethdev.h               | 284 +++++++++++
 lib/librte_ether/rte_ether_version.map      |  16 +
 7 files changed, 1455 insertions(+), 7 deletions(-)

-- 
2.9.0

[dpdk-dev] [RFC PATCH v2 0/5] add API's for VF management

[Bernard Iremonger]

This RFC patchset contains new DPDK API's requested by AT&T for use
with the Virtual Function Daemon (VFD).

The need to configure and manage VF's on a NIC has grown to the
point where AT&T have devloped a DPDK based tool, VFD, to do this.

This RFC proposes to add the following API extensions to DPDK:
  mailbox communication callback support
  VF configuration

Nine new functions have been added to the eth_dev_ops structure.
Corresponding functions have been added to the ixgbe PMD for the
Niantic NIC.

Two new callback functions have been added.
Changes have been made to the ixgbe_rcv_msg_from_vf function to
use the callback functions.

Changes have been made to testpmd to facilitate testing of the new API's.
The testpmd documentation has been updated to document the testpmd changes.

Note:
Adding new functions to the eth_dev_ops structure will cause an
ABI breakage.

Changes in V2:
rebase to latest master branch.
fix compile  error with clang.

Bernard Iremonger (5):
  librte_ether: add internal callback functions
  net/ixgbe: add callback to user app on VF to PF mbox msg
  librte_ether: add API's for VF management
  net/ixgbe: add functions for VF management
  app/test_pmd: add tests for new API's

 app/test-pmd/cmdline.c                      | 700 ++++++++++++++++++++++++++++
 doc/guides/testpmd_app_ug/testpmd_funcs.rst |  68 ++-
 drivers/net/ixgbe/ixgbe_ethdev.c            | 179 +++++++
 drivers/net/ixgbe/ixgbe_pf.c                |  39 +-
 lib/librte_ether/rte_ethdev.c               | 176 +++++++
 lib/librte_ether/rte_ethdev.h               | 284 +++++++++++
 lib/librte_ether/rte_ether_version.map      |  16 +
 7 files changed, 1455 insertions(+), 7 deletions(-)

-- 
2.9.0

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

[Bernard Iremonger]

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(+)

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) {
+		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;
+}
+
+int
+rte_eth_dev_set_vf_mac_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) {
+		RTE_PMD_DEBUG_TRACE("VF MAC anti spoof:VF %d > 63\n", vf);
+		return -EINVAL;
+	}
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_mac_anti_spoof, -ENOTSUP);
+	(*dev->dev_ops->set_vf_mac_anti_spoof)(dev, vf, on);
+	return 0;
+}
+
+int
+rte_eth_dev_vf_ping(uint8_t port_id, int32_t vf)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+	if (vf > 63) {
+		RTE_PMD_DEBUG_TRACE("VF ping: VF %d > 64\n", vf);
+		return -EINVAL;
+	}
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->vf_ping, -ENOTSUP);
+	return (*dev->dev_ops->vf_ping)(dev, vf);
+}
+
+int
+rte_eth_dev_set_vf_vlan_strip(uint8_t port_id, uint16_t vf, int on)
+{
+	struct rte_eth_dev *dev;
+	struct rte_eth_dev_info dev_info;
+	uint16_t queues_per_pool;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+	if (vf > 63) {
+		RTE_PMD_DEBUG_TRACE("VF vlan strip set VF %d > 63\n", vf);
+		return -EINVAL;
+	}
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_vlan_strip, -ENOTSUP);
+
+	rte_eth_dev_info_get(port_id, &dev_info);
+	queues_per_pool = dev_info.vmdq_queue_num/dev_info.max_vmdq_pools;
+
+	(*dev->dev_ops->set_vf_vlan_strip)(dev, on, queues_per_pool);
+	return 0;
+}
+
+int
+rte_eth_dev_set_vf_vlan_insert(uint8_t port_id, uint16_t vf, int 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) {
+		RTE_PMD_DEBUG_TRACE("VF vlan insert set VF %d > 63\n", vf);
+		return -EINVAL;
+	}
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_vlan_insert, -ENOTSUP);
+
+	(*dev->dev_ops->set_vf_vlan_insert)(dev, vf, on);
+	return 0;
+}
+
+int
+rte_eth_dev_set_tx_loopback(uint8_t port_id, int on)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_tx_loopback, -ENOTSUP);
+
+	(*dev->dev_ops->set_tx_loopback)(dev, on);
+	return 0;
+}
+
+int
+rte_eth_dev_set_all_queues_drop_en(uint8_t port_id, int state)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_all_queues_drop_en, -ENOTSUP);
+
+	(*dev->dev_ops->set_all_queues_drop_en)(dev, state);
+	return 0;
+}
+
+int
+rte_eth_dev_set_vf_split_drop_en(uint8_t port_id, uint16_t vf, int state)
+{
+	struct rte_eth_dev *dev;
+
+	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
+
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->set_vf_split_drop_en, -ENOTSUP);
+
+	(*dev->dev_ops->set_vf_split_drop_en)(dev, vf, state);
+	return 0;
+}
+
 int rte_eth_set_queue_rate_limit(uint8_t port_id, uint16_t queue_idx,
 					uint16_t tx_rate)
 {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 4fb0b9c..ed3d61b 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -1230,6 +1230,11 @@ typedef void (*eth_mac_addr_set_t)(struct rte_eth_dev *dev,
 				  struct ether_addr *mac_addr);
 /**< @internal Set a MAC address into Receive Address Address Register */
 
+typedef int (*eth_set_vf_mac_addr_t)(struct rte_eth_dev *dev,
+				  uint16_t vf,
+				  struct ether_addr *mac_addr);
+/**< @internal Set VF address into Receive Address Address Register */
+
 typedef int (*eth_uc_hash_table_set_t)(struct rte_eth_dev *dev,
 				  struct ether_addr *mac_addr,
 				  uint8_t on);
@@ -1261,6 +1266,43 @@ typedef int (*eth_set_vf_vlan_filter_t)(struct rte_eth_dev *dev,
 				  uint8_t vlan_on);
 /**< @internal Set VF VLAN pool filter */
 
+typedef void (*eth_set_vf_vlan_anti_spoof_t)(struct rte_eth_dev *dev,
+				  uint16_t vf,
+				  uint8_t on);
+/**< @internal Set VF VLAN anti spoof */
+
+typedef void (*eth_set_vf_mac_anti_spoof_t)(struct rte_eth_dev *dev,
+				  uint16_t vf,
+				  uint8_t on);
+/**< @internal Set VF MAC anti spoof */
+
+typedef int (*eth_vf_ping_t)(struct rte_eth_dev *dev,
+				int32_t vf);
+/**< @internal ping one or all vf's */
+
+typedef void (*eth_set_vf_vlan_strip_t)(struct rte_eth_dev *dev,
+				  int on,
+				  uint16_t queues_per_pool);
+/**< @internal Set VF vlan strip */
+
+typedef void (*eth_set_vf_vlan_insert_t)(struct rte_eth_dev *dev,
+				  uint16_t vf,
+				  int vlan);
+/**< @internal Set VF vlan insert */
+
+typedef void (*eth_set_tx_loopback_t)(struct rte_eth_dev *dev,
+				  int on);
+/**< @internal Set tx loopback */
+
+typedef void (*eth_set_all_queues_drop_en_t)(struct rte_eth_dev *dev,
+				  int state);
+/**< @internal Set all queues drop */
+
+typedef void (*eth_set_vf_split_drop_en_t)(struct rte_eth_dev *dev,
+				  uint16_t vf,
+				  int state);
+/**< @internal Set the enable drop bit in the VF split rx control register */
+
 typedef int (*eth_set_queue_rate_limit_t)(struct rte_eth_dev *dev,
 				uint16_t queue_idx,
 				uint16_t tx_rate);
@@ -1465,6 +1507,7 @@ struct eth_dev_ops {
 	eth_mac_addr_remove_t      mac_addr_remove; /**< Remove MAC address */
 	eth_mac_addr_add_t         mac_addr_add;  /**< Add a MAC address */
 	eth_mac_addr_set_t         mac_addr_set;  /**< Set a MAC address */
+	eth_set_vf_mac_addr_t      set_vf_mac_addr;  /**< Set a VF MAC address */
 	eth_uc_hash_table_set_t    uc_hash_table_set;  /**< Set Unicast Table Array */
 	eth_uc_all_hash_table_set_t uc_all_hash_table_set;  /**< Set Unicast hash bitmap */
 	eth_mirror_rule_set_t	   mirror_rule_set;  /**< Add a traffic mirror rule.*/
@@ -1473,6 +1516,14 @@ struct eth_dev_ops {
 	eth_set_vf_rx_t            set_vf_rx;  /**< enable/disable a VF receive */
 	eth_set_vf_tx_t            set_vf_tx;  /**< enable/disable a VF transmit */
 	eth_set_vf_vlan_filter_t   set_vf_vlan_filter;  /**< Set VF VLAN filter */
+	eth_set_vf_vlan_anti_spoof_t  set_vf_vlan_anti_spoof; /**< Set VF VLAN anti spoof */
+	eth_set_vf_mac_anti_spoof_t   set_vf_mac_anti_spoof;  /**< Set VF MAC anti spoof */
+	eth_vf_ping_t             vf_ping;  /**< Ping one or all VF's */
+	eth_set_vf_vlan_strip_t   set_vf_vlan_strip; /** <Set VF VLAN strip */
+	eth_set_vf_vlan_insert_t  set_vf_vlan_insert; /** <Set VF VLAN insert */
+	eth_set_tx_loopback_t  set_tx_loopback; /** <Set tx loopback */
+	eth_set_all_queues_drop_en_t  set_all_queues_drop_en; /** <Set queue drop enable bit */
+	eth_set_vf_split_drop_en_t  set_vf_split_drop_en; /** <Set split drop enable bit.*/
 	/** Add UDP tunnel port. */
 	eth_udp_tunnel_port_add_t udp_tunnel_port_add;
 	/** Del UDP tunnel port. */
@@ -3389,7 +3440,25 @@ int rte_eth_dev_mac_addr_remove(uint8_t port, struct ether_addr *mac_addr);
  */
 int rte_eth_dev_default_mac_addr_set(uint8_t port, struct ether_addr *mac_addr);
 
+/**
+ * Set the VF MAC address.
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @param vf
+ *   VF id.
+ * @param mac_addr
+ *   VF MAC address.
 
+ * @return
+ *   - (0) if successful, or *mac_addr* didn't exist.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if MAC address is invalid.
+ */
+
+int rte_eth_dev_set_vf_mac_addr(uint8_t port, uint16_t vf,
+				struct ether_addr *mac_addr);
 /**
  * Update Redirection Table(RETA) of Receive Side Scaling of Ethernet device.
  *
@@ -3556,6 +3625,160 @@ rte_eth_dev_set_vf_vlan_filter(uint8_t port, uint16_t vlan_id,
 				uint8_t vlan_on);
 
 /**
+ * Enable/Disable VF VLAN anti spoofing.
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf
+ *    VF on which to set VLAN anti spoofing.
+ * @param vlan_on
+ *    1 - Enable VFs VLAN anti spoofing.
+ *    0 - Disable VFs VLAN anti spoofing.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_vf_vlan_anti_spoof(uint8_t port,
+				uint16_t vf,
+				uint8_t on);
+
+/**
+ * Enable/Disable VF MAC anti spoofing.
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf
+ *    VF on which to set MAC anti spoofing.
+ * @param vlan_on
+ *    1 - Enable VFs MAC anti spoofing.
+ *    0 - Disable VFs MAC anti spoofing.
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_vf_mac_anti_spoof(uint8_t port,
+				uint16_t vf,
+				uint8_t on);
+
+/**
+ * Ping all or specified VF
+ *
+ * @param port
+ *   The port identifier of the Ethernet device.
+ * @param vf
+ *   specify VF to ping or all if -1.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_vf_ping(uint8_t port, int32_t vf);
+
+/**
+ * Enable/Disable vf vlan strip
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf
+ *    ID specifying VF.
+ * @param on
+ *    1 - Enable VF's vlan strip.
+ *    0 - Disable VF's vlan strip
+ * @param queues_per_pool
+ *    The number of queues per pool.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_vf_vlan_strip(uint8_t port, uint16_t vf, int on);
+
+/**
+ * Enable/Disable vf vlan insert
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf
+ *    ID specifying VF.
+ * @param on
+ *    1 - Enable VF's vlan insert.
+ *    0 - Disable VF's vlan insert
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_vf_vlan_insert(uint8_t port, uint16_t vf, int on);
+
+/**
+ * Enable/Disable tx loopback
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param on
+ *    1 - Enable tx loopback.
+ *    0 - Disable tx loopback.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_tx_loopback(uint8_t port, int on);
+/**
+ * set all queues drop enable bit
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param state
+ *    1 - set the queue drop enable bit for all pools.
+ *    0 - reset the queue drop enable bit for all pools.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_all_queues_drop_en(uint8_t port, int state);
+/**
+ * set drop enable bit in the VF split rx control register
+ *
+ * @param port
+ *    The port identifier of the Ethernet device.
+ * @param vf
+ *    ID specifying VF.
+ * @param state
+ *    1 - set the drop enable bit in the split rx control register.
+ *    0 - reset the drop enable bit in the split rx control register.
+ *
+ * @return
+ *   - (0) if successful.
+ *   - (-ENOTSUP) if hardware doesn't support this feature.
+ *   - (-ENODEV) if *port* invalid.
+ *   - (-EINVAL) if bad parameter.
+ */
+int
+rte_eth_dev_set_vf_split_drop_en(uint8_t port, uint16_t vf, int state);
+/**
  * Set a traffic mirroring rule on an Ethernet device
  *
  * @param port_id
diff --git a/lib/librte_ether/rte_ether_version.map b/lib/librte_ether/rte_ether_version.map
index cb7ef15..c9649ee 100644
--- a/lib/librte_ether/rte_ether_version.map
+++ b/lib/librte_ether/rte_ether_version.map
@@ -145,4 +145,13 @@ DPDK_16.11 {
 
 	_rte_eth_dev_callback_process_generic;
 	_rte_eth_dev_callback_process_vf;
+	rte_eth_dev_set_all_queues_drop_en;
+	rte_eth_dev_set_tx_loopback;
+	rte_eth_dev_set_vf_mac_addr;
+	rte_eth_dev_set_vf_mac_anti_spoof;
+	rte_eth_dev_set_vf_split_drop_en;
+	rte_eth_dev_set_vf_vlan_anti_spoof;
+	rte_eth_dev_set_vf_vlan_insert;
+	rte_eth_dev_set_vf_vlan_strip;
+	rte_eth_dev_vf_ping;
 } DPDK_16.07;
-- 
2.9.0

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

[Jerin Jacob]

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;
> +}
> +

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

[Iremonger, Bernard]

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.

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

[Thomas Monjalon]

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.

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

[Iremonger, Bernard]

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.

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

[Thomas Monjalon]

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?

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

[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.

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

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

[Thomas Monjalon]

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.

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

[Richardson, Bruce]

> -----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

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

[Thomas Monjalon]

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

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

[Iremonger, Bernard]

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.

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

[Thomas Monjalon]

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?

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

[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

Regards,

Bernard.

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

[Thomas Monjalon]

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?

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

[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-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.

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

[Bruce Richardson]

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

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

[Iremonger, Bernard]

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.

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

[Ananyev, Konstantin]

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.

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

[Iremonger, 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.


 

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

[Richardson, Bruce]

> -----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

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

[Thomas Monjalon]

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.

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

[Ananyev, Konstantin]

> -----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
 

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

[Thomas Monjalon]

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.

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

[Ananyev, Konstantin]

> -----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

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

[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?

Regards,

Bernard.

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

[Thomas Monjalon]

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?

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

[Iremonger, Bernard]

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.

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

[Thomas Monjalon]

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?

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

[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.
Konstantin

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

[Thomas Monjalon]

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.

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

[Bruce Richardson]

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

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

[Bruce Richardson]

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