[PATCH/RFC net-next 0/2] devlink: expose port function commands to assign VFs to multiple devlink

[PATCH/RFC net-next 0/2] devlink: expose port function commands to assign VFs to multiple devlink

[Simon Horman]

Hi,

this series proposed a new devlink port function to allow control over
how many VFs are associated with each physical port of a multi-port NIC.

This is to facilitate such configuration on NICs where this can
be supported.

Patch 1/2: Implements this new devlink feature
Patch 2/2: Uses it in the nfp driver


Fei Qin (2):
  devlink: expose port function commands to assign VFs to multiple
    netdevs
  nfp: add support for assigning VFs to different physical ports

 .../networking/devlink/devlink-port.rst       |  24 +++
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  71 +++++++++
 drivers/net/ethernet/netronome/nfp/nfp_main.c |   5 +
 drivers/net/ethernet/netronome/nfp/nfp_main.h |   4 +
 .../ethernet/netronome/nfp/nfp_net_sriov.c    | 147 +++++++++++++++++-
 .../ethernet/netronome/nfp/nfp_net_sriov.h    |   6 +
 include/net/devlink.h                         |  21 +++
 include/uapi/linux/devlink.h                  |   1 +
 net/devlink/leftover.c                        |  65 ++++++++
 9 files changed, 336 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

From: Fei Qin <fei.qin@corigine.com>

Multiple physical ports of the same NIC may share the single
PCI address. In some cases, assigning VFs to different physical
ports can be demanded, especially under high-traffic scenario.
Load balancing can be realized in virtualised use¬cases through
distributing packets between different physical ports with LAGs
of VFs which are assigned to those physical ports.

This patch adds new attribute "vf_count" to 'devlink port function'
API which only can be shown and configured under devlink ports
with flavor "DEVLINK_PORT_FLAVOUR_PHYSICAL".

e.g.
$ devlink port show pci/0000:82:00.0/0
pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
port 0 splittable true lanes 4
    function:
       vf_count 0

$ devlink port function set pci/0000:82:00.0/0 vf_count 3

$ devlink port show pci/0000:82:00.0/0
pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
port 0 splittable true lanes 4
    function:
       vf_count 3

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../networking/devlink/devlink-port.rst       | 24 +++++++
 include/net/devlink.h                         | 21 ++++++
 include/uapi/linux/devlink.h                  |  1 +
 net/devlink/leftover.c                        | 65 +++++++++++++++++++
 4 files changed, 111 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-port.rst b/Documentation/networking/devlink/devlink-port.rst
index 3da590953ce8..5c3996bce6d9 100644
--- a/Documentation/networking/devlink/devlink-port.rst
+++ b/Documentation/networking/devlink/devlink-port.rst
@@ -128,6 +128,9 @@ Users may also set the RoCE capability of the function using
 Users may also set the function as migratable using
 'devlink port function set migratable' command.
 
+Users may also assign VFs to physical ports using
+'devlink port function set vf_count' command.
+
 Function attributes
 ===================
 
@@ -240,6 +243,27 @@ Attach VF to the VM.
 Start the VM.
 Perform live migration.
 
+
+VF assignment setup
+---------------------------
+In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
+different ports.
+
+- Get count of VFs assigned to physical port::
+
+    $ devlink port show pci/0000:82:00.0/0
+    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
+        function:
+            vf_count 0
+
+- Set count of VFs assigned to physical port::
+    $ devlink port function set pci/0000:82:00.0/0 vf_count 3
+
+    $ devlink port show pci/0000:82:00.0/0
+    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
+        function:
+            vf_count 3
+
 Subfunction
 ============
 
diff --git a/include/net/devlink.h b/include/net/devlink.h
index 2e85a5970a32..3e98fa3d251f 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h
@@ -1491,6 +1491,27 @@ struct devlink_ops {
 	int (*port_fn_migratable_set)(struct devlink_port *devlink_port,
 				      bool enable,
 				      struct netlink_ext_ack *extack);
+
+	/**
+	 * @port_fn_vf_count_get: Port function's VF count get function
+	 *
+	 * Get assigned VF count of a function managed by the devlink port,
+	 * should only be used for DEVLINK_PORT_FLAVOUR_PHYSICAL.
+	 * Return -EOPNOTSUPP if port function vf_count setup is not supported.
+	 */
+	int (*port_fn_vf_count_get)(struct devlink_port *port, u16 *vf_count,
+				    struct netlink_ext_ack *extack);
+
+	/**
+	 * @port_fn_vf_count_set: Port function's VF count set function
+	 *
+	 * Set assigned VF count of a function managed by the devlink port,
+	 * should only be used for DEVLINK_PORT_FLAVOUR_PHYSICAL.
+	 * Return -EOPNOTSUPP if port function vf_count setup is not supported.
+	 */
+	int (*port_fn_vf_count_set)(struct devlink_port *port, u16 vf_count,
+				    struct netlink_ext_ack *extack);
+
 	/**
 	 * port_new() - Add a new port function of a specified flavor
 	 * @devlink: Devlink instance
diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index 3782d4219ac9..774e17f6100b 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h
@@ -676,6 +676,7 @@ enum devlink_port_function_attr {
 	DEVLINK_PORT_FN_ATTR_STATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_OPSTATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_CAPS,	/* bitfield32 */
+	DEVLINK_PORT_FN_ATTR_VF_COUNT,	/* u16 */
 
 	__DEVLINK_PORT_FUNCTION_ATTR_MAX,
 	DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1
diff --git a/net/devlink/leftover.c b/net/devlink/leftover.c
index 97d30ea98b00..6dac8b562232 100644
--- a/net/devlink/leftover.c
+++ b/net/devlink/leftover.c
@@ -141,6 +141,7 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
 				 DEVLINK_PORT_FN_STATE_ACTIVE),
 	[DEVLINK_PORT_FN_ATTR_CAPS] =
 		NLA_POLICY_BITFIELD32(DEVLINK_PORT_FN_CAPS_VALID_MASK),
+	[DEVLINK_PORT_FN_ATTR_VF_COUNT] = { .type = NLA_U16 },
 };
 
 #define ASSERT_DEVLINK_PORT_REGISTERED(devlink_port)				\
@@ -520,6 +521,35 @@ static int devlink_port_fn_caps_fill(const struct devlink_ops *ops,
 	return 0;
 }
 
+static int devlink_port_fn_vf_count_fill(const struct devlink_ops *ops,
+					 struct devlink_port *devlink_port,
+					 struct sk_buff *msg,
+					 struct netlink_ext_ack *extack,
+					 bool *msg_updated)
+{
+	u16 vf_count;
+	int err;
+
+	if (!ops->port_fn_vf_count_get ||
+	    devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL)
+		return 0;
+
+	err = ops->port_fn_vf_count_get(devlink_port, &vf_count, extack);
+	if (err) {
+		if (err == -EOPNOTSUPP)
+			return 0;
+		return err;
+	}
+
+	err = nla_put_u16(msg, DEVLINK_PORT_FN_ATTR_VF_COUNT, vf_count);
+	if (err)
+		return err;
+
+	*msg_updated = true;
+
+	return 0;
+}
+
 static int
 devlink_sb_tc_index_get_from_info(struct devlink_sb *devlink_sb,
 				  struct genl_info *info,
@@ -871,6 +901,16 @@ static int devlink_port_fn_caps_set(struct devlink_port *devlink_port,
 	return 0;
 }
 
+static int devlink_port_fn_vf_count_set(struct devlink_port *devlink_port,
+					const struct nlattr *attr,
+					struct netlink_ext_ack *extack)
+{
+	const struct devlink_ops *ops = devlink_port->devlink->ops;
+	u16 vf_count = nla_get_u16(attr);
+
+	return ops->port_fn_vf_count_set(devlink_port, vf_count, extack);
+}
+
 static int
 devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *port,
 				   struct netlink_ext_ack *extack)
@@ -893,6 +933,11 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 					&msg_updated);
 	if (err)
 		goto out;
+
+	err = devlink_port_fn_vf_count_fill(ops, port, msg, extack, &msg_updated);
+	if (err)
+		goto out;
+
 	err = devlink_port_fn_state_fill(ops, port, msg, extack, &msg_updated);
 out:
 	if (err || !msg_updated)
@@ -1219,6 +1264,19 @@ static int devlink_port_function_validate(struct devlink_port *devlink_port,
 				    "Function does not support state setting");
 		return -EOPNOTSUPP;
 	}
+	attr = tb[DEVLINK_PORT_FN_ATTR_VF_COUNT];
+	if (attr) {
+		if (!ops->port_fn_vf_count_set) {
+			NL_SET_ERR_MSG_ATTR(extack, attr,
+					    "Function doesn't support VF assignment");
+			return -EOPNOTSUPP;
+		}
+		if (devlink_port->attrs.flavour != DEVLINK_PORT_FLAVOUR_PHYSICAL) {
+			NL_SET_ERR_MSG_ATTR(extack, attr,
+					    "VFs assignment supported for physical ports only");
+			return -EOPNOTSUPP;
+		}
+	}
 	attr = tb[DEVLINK_PORT_FN_ATTR_CAPS];
 	if (attr) {
 		struct nla_bitfield32 caps;
@@ -1278,6 +1336,13 @@ static int devlink_port_function_set(struct devlink_port *port,
 			return err;
 	}
 
+	attr = tb[DEVLINK_PORT_FN_ATTR_VF_COUNT];
+	if (attr) {
+		err = devlink_port_fn_vf_count_set(port, attr, extack);
+		if (err)
+			return err;
+	}
+
 	/* Keep this as the last function attribute set, so that when
 	 * multiple port function attributes are set along with state,
 	 * Those can be applied first before activating the state.
-- 
2.30.2

[PATCH/RFC net-next 2/2] nfp: add support for assigning VFs to different physical ports

[Simon Horman]

From: Fei Qin <fei.qin@corigine.com>

Currently, all the VFs are bridged to port 0 for the device.
Assigning VFs to different ports is required to realize
traffic load balancing.

Add implement for "devlink port function set vf_count <VAL>"
command to support assigning vfs to different physical ports.

e.g.
$ devlink port show pci/0000:82:00.0/0
pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
port 0 splittable true lanes 4
    function:
       vf_count 0

$ devlink port function set pci/0000:82:00.0/0 vf_count 3

$ devlink port show pci/0000:82:00.0/0
pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
port 0 splittable true lanes 4
    function:
       vf_count 3

Signed-off-by: Fei Qin <fei.qin@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>
---
 .../net/ethernet/netronome/nfp/nfp_devlink.c  |  71 +++++++++
 drivers/net/ethernet/netronome/nfp/nfp_main.c |   5 +
 drivers/net/ethernet/netronome/nfp/nfp_main.h |   4 +
 .../ethernet/netronome/nfp/nfp_net_sriov.c    | 147 +++++++++++++++++-
 .../ethernet/netronome/nfp/nfp_net_sriov.h    |   6 +
 5 files changed, 225 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
index bf6bae557158..9a6b4d6c28ca 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_devlink.c
@@ -9,6 +9,7 @@
 #include "nfp_app.h"
 #include "nfp_main.h"
 #include "nfp_port.h"
+#include "nfp_net_sriov.h"
 
 static int
 nfp_devlink_fill_eth_port(struct nfp_port *port,
@@ -310,6 +311,74 @@ nfp_devlink_flash_update(struct devlink *devlink,
 	return nfp_flash_update_common(devlink_priv(devlink), params->fw, extack);
 }
 
+static int
+nfp_devlink_vf_count_get_port_id(struct devlink_port *dl_port)
+{
+	struct nfp_pf *pf = devlink_priv(dl_port->devlink);
+	struct nfp_eth_table_port *eth_port;
+	struct nfp_port *port;
+	int err;
+
+	err = nfp_net_sriov_check(pf->app, 0, NFP_NET_VF_CFG_MB_CAP_VF_ASSIGNMENT,
+				  "vf_assignment", false);
+	if (err)
+		return err;
+
+	port = container_of(dl_port, struct nfp_port, dl_port);
+	eth_port = __nfp_port_get_eth_port(port);
+	if (!eth_port)
+		return -EOPNOTSUPP;
+
+	return eth_port - pf->eth_tbl->ports;
+}
+
+static int
+nfp_devlink_vf_count_get(struct devlink_port *dl_port, u16 *vf_count,
+			 struct netlink_ext_ack *extack)
+{
+	int port_id = nfp_devlink_vf_count_get_port_id(dl_port);
+	struct nfp_pf *pf = devlink_priv(dl_port->devlink);
+
+	if (port_id < 0 || port_id >= NFP_VF_ASSIGNMENT_PORT_COUNT)
+		return -EOPNOTSUPP;
+
+	*vf_count = pf->num_assigned_vfs[port_id];
+
+	return 0;
+}
+
+static int
+nfp_devlink_vf_count_set(struct devlink_port *dl_port, u16 vf_count,
+			 struct netlink_ext_ack *extack)
+{
+	int port_id = nfp_devlink_vf_count_get_port_id(dl_port);
+	struct nfp_pf *pf = devlink_priv(dl_port->devlink);
+	int total_num_ports = pf->eth_tbl->count;
+	int total_num_vfs = 0;
+	unsigned int i;
+
+	if (port_id < 0 || port_id >= NFP_VF_ASSIGNMENT_PORT_COUNT)
+		return -EOPNOTSUPP;
+
+	for (i = 0; i < total_num_ports; i++) {
+		if (i != port_id)
+			total_num_vfs += pf->num_assigned_vfs[i];
+		else
+			total_num_vfs += vf_count;
+
+		if (total_num_vfs > pf->limit_vfs)
+			goto exit_out_of_range;
+	}
+
+	pf->num_assigned_vfs[port_id] = vf_count;
+
+	return 0;
+
+exit_out_of_range:
+	NL_SET_ERR_MSG_MOD(extack, "Parameter out of range");
+	return -EINVAL;
+}
+
 const struct devlink_ops nfp_devlink_ops = {
 	.port_split		= nfp_devlink_port_split,
 	.port_unsplit		= nfp_devlink_port_unsplit,
@@ -319,6 +388,8 @@ const struct devlink_ops nfp_devlink_ops = {
 	.eswitch_mode_set	= nfp_devlink_eswitch_mode_set,
 	.info_get		= nfp_devlink_info_get,
 	.flash_update		= nfp_devlink_flash_update,
+	.port_fn_vf_count_get	= nfp_devlink_vf_count_get,
+	.port_fn_vf_count_set	= nfp_devlink_vf_count_set,
 };
 
 int nfp_devlink_port_register(struct nfp_app *app, struct nfp_port *port)
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.c b/drivers/net/ethernet/netronome/nfp/nfp_main.c
index 71301dbd8fb5..29663d2562aa 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.c
@@ -29,6 +29,7 @@
 #include "nfp_app.h"
 #include "nfp_main.h"
 #include "nfp_net.h"
+#include "nfp_net_sriov.h"
 
 static const char nfp_driver_name[] = "nfp";
 
@@ -252,6 +253,10 @@ static int nfp_pcie_sriov_enable(struct pci_dev *pdev, int num_vfs)
 		return -EINVAL;
 	}
 
+	err = nfp_configure_assign_vf(pdev, num_vfs);
+	if (err)
+		return err;
+
 	err = pci_enable_sriov(pdev, num_vfs);
 	if (err) {
 		dev_warn(&pdev->dev, "Failed to enable PCI SR-IOV: %d\n", err);
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_main.h b/drivers/net/ethernet/netronome/nfp/nfp_main.h
index 14a751bfe1fe..67692fcf1201 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_main.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_main.h
@@ -16,6 +16,8 @@
 #include <linux/workqueue.h>
 #include <net/devlink.h>
 
+#define NFP_VF_ASSIGNMENT_PORT_COUNT 8
+
 struct dentry;
 struct device;
 struct pci_dev;
@@ -63,6 +65,7 @@ struct nfp_dumpspec {
  * @irq_entries:	Array of MSI-X entries for all vNICs
  * @limit_vfs:		Number of VFs supported by firmware (~0 for PCI limit)
  * @num_vfs:		Number of SR-IOV VFs enabled
+ * @num_assigned_vfs:	Number of VFs assigned to different physical ports
  * @fw_loaded:		Is the firmware loaded?
  * @unload_fw_on_remove:Do we need to unload firmware on driver removal?
  * @ctrl_vnic:		Pointer to the control vNIC if available
@@ -111,6 +114,7 @@ struct nfp_pf {
 
 	unsigned int limit_vfs;
 	unsigned int num_vfs;
+	u8 num_assigned_vfs[NFP_VF_ASSIGNMENT_PORT_COUNT];
 
 	bool fw_loaded;
 	bool unload_fw_on_remove;
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
index 6eeeb0fda91f..873c6d707c0e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.c
@@ -13,9 +13,13 @@
 #include "nfp_net_ctrl.h"
 #include "nfp_net.h"
 #include "nfp_net_sriov.h"
+#include "nfp_port.h"
+#include "nfpcore/nfp_nsp.h"
 
-static int
-nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg, bool warn)
+/* Capability of VF pre-configuration */
+#define NFP_NET_VF_PRE_CONFIG			NFP_NET_VF_CFG_MB_CAP_VF_ASSIGNMENT
+
+int nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg, bool warn)
 {
 	u16 cap_vf;
 
@@ -29,6 +33,9 @@ nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg, bool
 		return -EOPNOTSUPP;
 	}
 
+	if (cap & NFP_NET_VF_PRE_CONFIG)
+		return 0;
+
 	if (vf < 0 || vf >= app->pf->num_vfs) {
 		if (warn)
 			nfp_warn(app->pf->cpp, "invalid VF id %d\n", vf);
@@ -38,17 +45,125 @@ nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg, bool
 	return 0;
 }
 
+/* VFs can be shown and configured through each physical port even with VF
+ * assignment enabled. FW requires configurations to be sent through the port
+ * that the VF assigned to. Driver may need to get the port id and judge if the
+ * current netdev is the one that the VF assigned to.
+ */
+static int nfp_vf_assignment_get_port_id(struct nfp_app *app, int vf)
+{
+	struct nfp_pf *pf = app->pf;
+	unsigned int i, start_vf;
+
+	for (start_vf = 0, i = 0; i < ARRAY_SIZE(pf->num_assigned_vfs); i++) {
+		if (vf >= start_vf && vf < (start_vf + pf->num_assigned_vfs[i]))
+			return i;
+		start_vf += pf->num_assigned_vfs[i];
+	}
+
+	/* If VF assignment is disabled, all the VFs are assigned to port 0 */
+	return 0;
+}
+
+static bool nfp_vf_assignment_assigned_to_cur_port(struct net_device *netdev, int vf)
+{
+	struct nfp_app *app = nfp_app_from_netdev(netdev);
+	struct nfp_eth_table_port *eth_port;
+	struct nfp_pf *pf = app->pf;
+	int assigned_port_id, err;
+	struct nfp_port *port;
+
+	/* If firmware doesn't support vf_assignment, each VF should be shown under
+	 * each physical port or PF normally.
+	 */
+	err = nfp_net_sriov_check(pf->app, 0, NFP_NET_VF_CFG_MB_CAP_VF_ASSIGNMENT,
+				  "vf_assignment", false);
+	if (err < 0)
+		return true;
+
+	port = nfp_port_from_netdev(netdev);
+	eth_port = nfp_port_get_eth_port(port);
+	if (!eth_port)
+		return true;
+
+	assigned_port_id = nfp_vf_assignment_get_port_id(app, vf);
+	return eth_port == &pf->eth_tbl->ports[assigned_port_id];
+}
+
+int nfp_configure_assign_vf(struct pci_dev *pdev, int num_vfs)
+{
+	struct nfp_pf *pf = pci_get_drvdata(pdev);
+	int err, total_num_vfs = 0, idx = 0;
+	struct nfp_net *nn;
+	unsigned int i;
+
+	err = nfp_net_sriov_check(pf->app, 0, NFP_NET_VF_CFG_MB_CAP_VF_ASSIGNMENT,
+				  "vf_assignment", false);
+	if (err)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(pf->num_assigned_vfs); i++)
+		total_num_vfs += pf->num_assigned_vfs[i];
+
+	/* When the total_num_vfs is nonzero, the VF assignment is enabled.
+	 * The total number of created VFs is required to be consistent with
+	 * the one set in VF assignment.
+	 */
+	if (total_num_vfs && num_vfs != total_num_vfs) {
+		dev_err(&pdev->dev,
+			"Trying to create %d VFs not satisfy the configuration of VF assignment\n",
+			num_vfs);
+		return -EINVAL;
+	}
+
+	/* When VF assignment is disabled, all the VFs are allocated to port 0 */
+	list_for_each_entry(nn, &pf->vnics, vnic_list) {
+		u8 num_assigned_vfs = ((idx == 0) && !total_num_vfs) ?
+				  pf->limit_vfs : pf->num_assigned_vfs[idx];
+		idx++;
+
+		writeb(num_assigned_vfs, pf->vfcfg_tbl2 + NFP_NET_VF_CFG_VF_ASSIGNMENT);
+		writew(NFP_NET_VF_CFG_MB_UPD_VF_ASSIGNMENT, pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_UPD);
+
+		err = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_VF);
+		if (err)
+			return err;
+
+		err = readw(pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_RET);
+		if (err) {
+			dev_info(&pdev->dev,
+				 "FW refused VF assignment update with errno: %d\n", err);
+			return -err;
+		}
+	}
+
+	return 0;
+}
+
 static int
 nfp_net_sriov_update(struct nfp_app *app, int vf, u16 update, const char *msg)
 {
+	int ret, assigned_port_id, cnt = 0;
+	struct nfp_net *nn_iter;
 	struct nfp_net *nn;
-	int ret;
 
 	/* Write update info to mailbox in VF config symbol */
 	writeb(vf, app->pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_VF_NUM);
 	writew(update, app->pf->vfcfg_tbl2 + NFP_NET_VF_CFG_MB_UPD);
 
 	nn = list_first_entry(&app->pf->vnics, struct nfp_net, vnic_list);
+
+	/* If VF assignment is enabled, reconfig of VF should be set through "nn"
+	 * of corresponding physical port
+	 */
+	assigned_port_id = nfp_vf_assignment_get_port_id(app, vf);
+	list_for_each_entry(nn_iter, &app->pf->vnics, vnic_list) {
+		if (cnt++ == assigned_port_id) {
+			nn = nn_iter;
+			break;
+		}
+	}
+
 	/* Signal VF reconfiguration */
 	ret = nfp_net_reconfig(nn, NFP_NET_CFG_UPDATE_VF);
 	if (ret)
@@ -257,11 +372,18 @@ int nfp_app_set_vf_link_state(struct net_device *netdev, int vf,
 				    "link state");
 }
 
+/* VFs can be shown under each physical port. When the VF is
+ * not assigned to the physical port, hardcode its mac to
+ * ff:ff:ff:ff:ff:ff to distinguish. The changes of VFs'
+ * configurations can be only seen under the corresponding
+ * physical ports.
+ */
 int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 			  struct ifla_vf_info *ivi)
 {
 	struct nfp_app *app = nfp_app_from_netdev(netdev);
 	u32 vf_offset, mac_hi, rate;
+	bool is_assigned = true;
 	u32 vlan_tag;
 	u16 mac_lo;
 	u8 flags;
@@ -271,13 +393,19 @@ int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 	if (err)
 		return err;
 
-	vf_offset = NFP_NET_VF_CFG_MB_SZ + vf * NFP_NET_VF_CFG_SZ;
+	is_assigned = nfp_vf_assignment_assigned_to_cur_port(netdev, vf);
 
-	mac_hi = readl(app->pf->vfcfg_tbl2 + vf_offset);
-	mac_lo = readw(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_MAC_LO);
+	vf_offset = NFP_NET_VF_CFG_MB_SZ + vf * NFP_NET_VF_CFG_SZ;
 
-	flags = readb(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_CTRL);
-	vlan_tag = readl(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_VLAN);
+	if (is_assigned) {
+		mac_hi = readl(app->pf->vfcfg_tbl2 + vf_offset);
+		mac_lo = readw(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_MAC_LO);
+		flags = readb(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_CTRL);
+		vlan_tag = readl(app->pf->vfcfg_tbl2 + vf_offset + NFP_NET_VF_CFG_VLAN);
+	} else {
+		mac_hi = 0xffffffff;
+		mac_lo = 0xffff;
+	}
 
 	memset(ivi, 0, sizeof(*ivi));
 	ivi->vf = vf;
@@ -285,6 +413,9 @@ int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 	put_unaligned_be32(mac_hi, &ivi->mac[0]);
 	put_unaligned_be16(mac_lo, &ivi->mac[4]);
 
+	if (!is_assigned)
+		return 0;
+
 	ivi->vlan = FIELD_GET(NFP_NET_VF_CFG_VLAN_VID, vlan_tag);
 	ivi->qos = FIELD_GET(NFP_NET_VF_CFG_VLAN_QOS, vlan_tag);
 	if (!nfp_net_sriov_check(app, vf, NFP_NET_VF_CFG_MB_CAP_VLAN_PROTO, "vlan_proto", false))
diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
index 2d445fa199dc..fdf429f60d34 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_sriov.h
@@ -21,6 +21,7 @@
 #define   NFP_NET_VF_CFG_MB_CAP_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_CAP_VLAN_PROTO		  (0x1 << 5)
 #define   NFP_NET_VF_CFG_MB_CAP_RATE			  (0x1 << 6)
+#define   NFP_NET_VF_CFG_MB_CAP_VF_ASSIGNMENT		  (0x1 << 8)
 #define NFP_NET_VF_CFG_MB_RET				0x2
 #define NFP_NET_VF_CFG_MB_UPD				0x4
 #define   NFP_NET_VF_CFG_MB_UPD_MAC			  (0x1 << 0)
@@ -30,6 +31,8 @@
 #define   NFP_NET_VF_CFG_MB_UPD_TRUST			  (0x1 << 4)
 #define   NFP_NET_VF_CFG_MB_UPD_VLAN_PROTO		  (0x1 << 5)
 #define   NFP_NET_VF_CFG_MB_UPD_RATE			  (0x1 << 6)
+#define   NFP_NET_VF_CFG_MB_UPD_VF_ASSIGNMENT		  (0x1 << 8)
+#define NFP_NET_VF_CFG_VF_ASSIGNMENT			0x6
 #define NFP_NET_VF_CFG_MB_VF_NUM			0x7
 
 /* VF config entry
@@ -67,5 +70,8 @@ int nfp_app_set_vf_link_state(struct net_device *netdev, int vf,
 			      int link_state);
 int nfp_app_get_vf_config(struct net_device *netdev, int vf,
 			  struct ifla_vf_info *ivi);
+int nfp_configure_assign_vf(struct pci_dev *pdev, int num_vfs);
+int nfp_net_sriov_check(struct nfp_app *app, int vf, u16 cap, const char *msg,
+			bool warn);
 
 #endif /* _NFP_NET_SRIOV_H_ */
-- 
2.30.2

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jakub Kicinski]

On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> +VF assignment setup
> +---------------------------
> +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> +different ports.

Please make sure you run make htmldocs when changing docs,
this will warn.

> +- Get count of VFs assigned to physical port::
> +
> +    $ devlink port show pci/0000:82:00.0/0
> +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4

Physical port has VFs? My knee jerk reaction is that allocating
resources via devlink is fine but this seems to lean a bit into
forwarding. How do other vendors do it? What's the mapping of VFs
to ports?

What do you suggest should happen when user enables switchdev mode?

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> > +VF assignment setup
> > +---------------------------
> > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> > +different ports.
> 
> Please make sure you run make htmldocs when changing docs,
> this will warn.

Thanks, will do.

> > +- Get count of VFs assigned to physical port::
> > +
> > +    $ devlink port show pci/0000:82:00.0/0
> > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> 
> Physical port has VFs? My knee jerk reaction is that allocating
> resources via devlink is fine but this seems to lean a bit into
> forwarding. How do other vendors do it? What's the mapping of VFs
> to ports?

We are not aware of any non-vendor-specific mechanism.
If there is one we'd gladly consider it.

> What do you suggest should happen when user enables switchdev mode?

Thanks for pointing this out. It ought to be documented.

Prior to this patch-set, for all NFP application firmwares supported by
upstream (and I'm not implying there is anything sneaky elsewhere - I am
honestly not aware of any such things), the embedded switch on the NIC
(which as you know is software running on the NIC), allows forwarding of
packets between VFs and physical ports without any partitioning - other
than what policy may implement.

This remains the behaviour if the feature proposed by this patch-set is not
enabled, either because it is unsupported by the application firmware, or
has not been enabled, i.e. using devlink.

If the feature is enabled, then in effect there is a partition between
VFs on one physical port and those on another. And some sort of forwarding
in software (on the host) is required. It is my understanding that
this is the dominant behaviour amongst multi-port NICs from other vendors
which, by default (and perhaps can only), associate VFs with specific
physical ports.

This is my understanding of things.
And I believe this applies in both switchdev and legacy mode.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Leon Romanovsky]

On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> > +VF assignment setup
> > +---------------------------
> > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> > +different ports.
> 
> Please make sure you run make htmldocs when changing docs,
> this will warn.
> 
> > +- Get count of VFs assigned to physical port::
> > +
> > +    $ devlink port show pci/0000:82:00.0/0
> > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> 
> Physical port has VFs? My knee jerk reaction is that allocating
> resources via devlink is fine but this seems to lean a bit into
> forwarding. How do other vendors do it? What's the mapping of VFs
> to ports?

I don't understand the meaning of VFs here. If we are talking about PCI
VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
talks about having one bit to enable all VFs at once. All these VFs will
have separate netdevs.

> 
> What do you suggest should happen when user enables switchdev mode?
> 

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
> On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> > > +VF assignment setup
> > > +---------------------------
> > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> > > +different ports.
> > 
> > Please make sure you run make htmldocs when changing docs,
> > this will warn.
> > 
> > > +- Get count of VFs assigned to physical port::
> > > +
> > > +    $ devlink port show pci/0000:82:00.0/0
> > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> > 
> > Physical port has VFs? My knee jerk reaction is that allocating
> > resources via devlink is fine but this seems to lean a bit into
> > forwarding. How do other vendors do it? What's the mapping of VFs
> > to ports?
> 
> I don't understand the meaning of VFs here. If we are talking about PCI
> VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
> talks about having one bit to enable all VFs at once. All these VFs will
> have separate netdevs.

Yes, that is the case here too (before and after).

What we are talking about is the association of VFs to physical ports
(in the case where a NIC has more than one physical port).

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jiri Pirko]

Mon, Feb 06, 2023 at 04:36:02PM CET, simon.horman@corigine.com wrote:
>From: Fei Qin <fei.qin@corigine.com>
>
>Multiple physical ports of the same NIC may share the single
>PCI address. In some cases, assigning VFs to different physical
>ports can be demanded, especially under high-traffic scenario.
>Load balancing can be realized in virtualised use¬cases through
>distributing packets between different physical ports with LAGs
>of VFs which are assigned to those physical ports.
>
>This patch adds new attribute "vf_count" to 'devlink port function'
>API which only can be shown and configured under devlink ports
>with flavor "DEVLINK_PORT_FLAVOUR_PHYSICAL".

I have to be missing something. That is the meaning of "assigning VF"
to a physical port? Why there should be any relationship between
physical port and VF other than configured forwarding (using TC for
example)?

This seems very wrong. Preliminary NAK.


>
>e.g.
>$ devlink port show pci/0000:82:00.0/0
>pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
>port 0 splittable true lanes 4
>    function:
>       vf_count 0
>
>$ devlink port function set pci/0000:82:00.0/0 vf_count 3
>
>$ devlink port show pci/0000:82:00.0/0
>pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical
>port 0 splittable true lanes 4
>    function:
>       vf_count 3
>
>Signed-off-by: Fei Qin <fei.qin@corigine.com>
>Signed-off-by: Simon Horman <simon.horman@corigine.com>

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jiri Pirko]

Wed, Feb 08, 2023 at 12:36:53PM CET, simon.horman@corigine.com wrote:
>On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
>> On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
>> > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
>> > > +VF assignment setup
>> > > +---------------------------
>> > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
>> > > +different ports.
>> > 
>> > Please make sure you run make htmldocs when changing docs,
>> > this will warn.
>> > 
>> > > +- Get count of VFs assigned to physical port::
>> > > +
>> > > +    $ devlink port show pci/0000:82:00.0/0
>> > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
>> > 
>> > Physical port has VFs? My knee jerk reaction is that allocating
>> > resources via devlink is fine but this seems to lean a bit into
>> > forwarding. How do other vendors do it? What's the mapping of VFs
>> > to ports?
>> 
>> I don't understand the meaning of VFs here. If we are talking about PCI
>> VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
>> talks about having one bit to enable all VFs at once. All these VFs will
>> have separate netdevs.
>
>Yes, that is the case here too (before and after).
>
>What we are talking about is the association of VFs to physical ports
>(in the case where a NIC has more than one physical port).

What is "the association"?

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Leon Romanovsky]

On Wed, Feb 08, 2023 at 12:36:53PM +0100, Simon Horman wrote:
> On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
> > On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> > > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> > > > +VF assignment setup
> > > > +---------------------------
> > > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> > > > +different ports.
> > > 
> > > Please make sure you run make htmldocs when changing docs,
> > > this will warn.
> > > 
> > > > +- Get count of VFs assigned to physical port::
> > > > +
> > > > +    $ devlink port show pci/0000:82:00.0/0
> > > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> > > 
> > > Physical port has VFs? My knee jerk reaction is that allocating
> > > resources via devlink is fine but this seems to lean a bit into
> > > forwarding. How do other vendors do it? What's the mapping of VFs
> > > to ports?
> > 
> > I don't understand the meaning of VFs here. If we are talking about PCI
> > VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
> > talks about having one bit to enable all VFs at once. All these VFs will
> > have separate netdevs.
> 
> Yes, that is the case here too (before and after).
> 
> What we are talking about is the association of VFs to physical ports
> (in the case where a NIC has more than one physical port).

We have devices with multiple ports too, but don't have such issues.
So it will help if you can provide more context here.

I'm failing to see connection between physical ports and physical VFs.

Are you saying that physical ports are actual PCI VFs, which spans L2 VFs,
which you want to assign to another port (PF)?

Thanks

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Wed, Feb 08, 2023 at 01:53:48PM +0200, Leon Romanovsky wrote:
> On Wed, Feb 08, 2023 at 12:36:53PM +0100, Simon Horman wrote:
> > On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
> > > On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> > > > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> > > > > +VF assignment setup
> > > > > +---------------------------
> > > > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> > > > > +different ports.
> > > > 
> > > > Please make sure you run make htmldocs when changing docs,
> > > > this will warn.
> > > > 
> > > > > +- Get count of VFs assigned to physical port::
> > > > > +
> > > > > +    $ devlink port show pci/0000:82:00.0/0
> > > > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> > > > 
> > > > Physical port has VFs? My knee jerk reaction is that allocating
> > > > resources via devlink is fine but this seems to lean a bit into
> > > > forwarding. How do other vendors do it? What's the mapping of VFs
> > > > to ports?
> > > 
> > > I don't understand the meaning of VFs here. If we are talking about PCI
> > > VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
> > > talks about having one bit to enable all VFs at once. All these VFs will
> > > have separate netdevs.
> > 
> > Yes, that is the case here too (before and after).
> > 
> > What we are talking about is the association of VFs to physical ports
> > (in the case where a NIC has more than one physical port).
> 
> We have devices with multiple ports too, but don't have such issues.
> So it will help if you can provide more context here.
> 
> I'm failing to see connection between physical ports and physical VFs.
> 
> Are you saying that physical ports are actual PCI VFs, which spans L2 VFs,
> which you want to assign to another port (PF)?

No, a physical port is not a VF (nor a PF, FWIIW).

The topic here is about two modes of behaviour.

One, where VFs are associated with physical ports - conceptually one might
think of this as some VFs and one phys port being plugged into one VEB,
while other VFs and another phys port are plugged into another VEB.

I believe this is the mode on most multi-port NICs.

And another mode where all VFs are associated with one physical port,
even if more than one is present. The NFP currently implements this model.

This patch is about allowing NICs, in particular the NFP based NICs,
to switch modes.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Wed, Feb 08, 2023 at 12:40:45PM +0100, Jiri Pirko wrote:
> Mon, Feb 06, 2023 at 04:36:02PM CET, simon.horman@corigine.com wrote:
> >From: Fei Qin <fei.qin@corigine.com>
> >
> >Multiple physical ports of the same NIC may share the single
> >PCI address. In some cases, assigning VFs to different physical
> >ports can be demanded, especially under high-traffic scenario.
> >Load balancing can be realized in virtualised use¬cases through
> >distributing packets between different physical ports with LAGs
> >of VFs which are assigned to those physical ports.
> >
> >This patch adds new attribute "vf_count" to 'devlink port function'
> >API which only can be shown and configured under devlink ports
> >with flavor "DEVLINK_PORT_FLAVOUR_PHYSICAL".
> 
> I have to be missing something. That is the meaning of "assigning VF"
> to a physical port? Why there should be any relationship between
> physical port and VF other than configured forwarding (using TC for
> example)?
> 
> This seems very wrong. Preliminary NAK.

Of course if TC is involved, then we have flexibility.

What we are talking about here is primarily legacy mode.
And the behaviour described would, when enabled allow NFP based NICs
to behave more like most other multi-port NICs.

That is, we can envisage a VEB with some VFs and one physical port.
And anther with other VFs and another physical port.

This is as opposed to a single VEB with all VFs, as is currently
the case on NFP based NICs (but not most other multi-port NICs).

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Wed, Feb 08, 2023 at 12:41:45PM +0100, Jiri Pirko wrote:
> Wed, Feb 08, 2023 at 12:36:53PM CET, simon.horman@corigine.com wrote:
> >On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
> >> On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
> >> > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
> >> > > +VF assignment setup
> >> > > +---------------------------
> >> > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
> >> > > +different ports.
> >> > 
> >> > Please make sure you run make htmldocs when changing docs,
> >> > this will warn.
> >> > 
> >> > > +- Get count of VFs assigned to physical port::
> >> > > +
> >> > > +    $ devlink port show pci/0000:82:00.0/0
> >> > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
> >> > 
> >> > Physical port has VFs? My knee jerk reaction is that allocating
> >> > resources via devlink is fine but this seems to lean a bit into
> >> > forwarding. How do other vendors do it? What's the mapping of VFs
> >> > to ports?
> >> 
> >> I don't understand the meaning of VFs here. If we are talking about PCI
> >> VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
> >> talks about having one bit to enable all VFs at once. All these VFs will
> >> have separate netdevs.
> >
> >Yes, that is the case here too (before and after).
> >
> >What we are talking about is the association of VFs to physical ports
> >(in the case where a NIC has more than one physical port).
> 
> What is "the association"?

My current explanation - and I'm sure I can dig and find others - is
to association == plugged into same VEB. But I feel that description
will lead to further confusion :(

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jiri Pirko]

Wed, Feb 08, 2023 at 01:07:54PM CET, simon.horman@corigine.com wrote:
>On Wed, Feb 08, 2023 at 12:40:45PM +0100, Jiri Pirko wrote:
>> Mon, Feb 06, 2023 at 04:36:02PM CET, simon.horman@corigine.com wrote:
>> >From: Fei Qin <fei.qin@corigine.com>
>> >
>> >Multiple physical ports of the same NIC may share the single
>> >PCI address. In some cases, assigning VFs to different physical
>> >ports can be demanded, especially under high-traffic scenario.
>> >Load balancing can be realized in virtualised use¬cases through
>> >distributing packets between different physical ports with LAGs
>> >of VFs which are assigned to those physical ports.
>> >
>> >This patch adds new attribute "vf_count" to 'devlink port function'
>> >API which only can be shown and configured under devlink ports
>> >with flavor "DEVLINK_PORT_FLAVOUR_PHYSICAL".
>> 
>> I have to be missing something. That is the meaning of "assigning VF"
>> to a physical port? Why there should be any relationship between
>> physical port and VF other than configured forwarding (using TC for
>> example)?
>> 
>> This seems very wrong. Preliminary NAK.
>
>Of course if TC is involved, then we have flexibility.
>
>What we are talking about here is primarily legacy mode.

I don't see any reason to add knobs for purpose of supporting the legacy
mode, sorry.

If you need this functionality, use TC.



>And the behaviour described would, when enabled allow NFP based NICs
>to behave more like most other multi-port NICs.
>
>That is, we can envisage a VEB with some VFs and one physical port.
>And anther with other VFs and another physical port.
>
>This is as opposed to a single VEB with all VFs, as is currently
>the case on NFP based NICs (but not most other multi-port NICs).
>

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Simon Horman]

On Wed, Feb 08, 2023 at 01:34:19PM +0100, Jiri Pirko wrote:
> Wed, Feb 08, 2023 at 01:07:54PM CET, simon.horman@corigine.com wrote:
> >On Wed, Feb 08, 2023 at 12:40:45PM +0100, Jiri Pirko wrote:
> >> Mon, Feb 06, 2023 at 04:36:02PM CET, simon.horman@corigine.com wrote:
> >> >From: Fei Qin <fei.qin@corigine.com>
> >> >
> >> >Multiple physical ports of the same NIC may share the single
> >> >PCI address. In some cases, assigning VFs to different physical
> >> >ports can be demanded, especially under high-traffic scenario.
> >> >Load balancing can be realized in virtualised use¬cases through
> >> >distributing packets between different physical ports with LAGs
> >> >of VFs which are assigned to those physical ports.
> >> >
> >> >This patch adds new attribute "vf_count" to 'devlink port function'
> >> >API which only can be shown and configured under devlink ports
> >> >with flavor "DEVLINK_PORT_FLAVOUR_PHYSICAL".
> >> 
> >> I have to be missing something. That is the meaning of "assigning VF"
> >> to a physical port? Why there should be any relationship between
> >> physical port and VF other than configured forwarding (using TC for
> >> example)?
> >> 
> >> This seems very wrong. Preliminary NAK.
> >
> >Of course if TC is involved, then we have flexibility.
> >
> >What we are talking about here is primarily legacy mode.
> 
> I don't see any reason to add knobs for purpose of supporting the legacy
> mode, sorry.
> 
> If you need this functionality, use TC.

I understand your point, even if I don't agree in this case.

> >And the behaviour described would, when enabled allow NFP based NICs
> >to behave more like most other multi-port NICs.
> >
> >That is, we can envisage a VEB with some VFs and one physical port.
> >And anther with other VFs and another physical port.
> >
> >This is as opposed to a single VEB with all VFs, as is currently
> >the case on NFP based NICs (but not most other multi-port NICs).
> >

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Saeed Mahameed]

On 08 Feb 13:05, Simon Horman wrote:
>On Wed, Feb 08, 2023 at 01:53:48PM +0200, Leon Romanovsky wrote:
>> On Wed, Feb 08, 2023 at 12:36:53PM +0100, Simon Horman wrote:
>> > On Wed, Feb 08, 2023 at 01:21:22PM +0200, Leon Romanovsky wrote:
>> > > On Mon, Feb 06, 2023 at 06:42:27PM -0800, Jakub Kicinski wrote:
>> > > > On Mon,  6 Feb 2023 16:36:02 +0100 Simon Horman wrote:
>> > > > > +VF assignment setup
>> > > > > +---------------------------
>> > > > > +In some cases, NICs could have multiple physical ports per PF. Users can assign VFs to
>> > > > > +different ports.
>> > > >
>> > > > Please make sure you run make htmldocs when changing docs,
>> > > > this will warn.
>> > > >
>> > > > > +- Get count of VFs assigned to physical port::
>> > > > > +
>> > > > > +    $ devlink port show pci/0000:82:00.0/0
>> > > > > +    pci/0000:82:00.0/0: type eth netdev enp130s0np0 flavour physical port 0 splittable true lanes 4
>> > > >
>> > > > Physical port has VFs? My knee jerk reaction is that allocating
>> > > > resources via devlink is fine but this seems to lean a bit into
>> > > > forwarding. How do other vendors do it? What's the mapping of VFs
>> > > > to ports?
>> > >
>> > > I don't understand the meaning of VFs here. If we are talking about PCI
>> > > VFs, other vendors follow PCI spec "9.3.3.3.1 VF Enable" section, which
>> > > talks about having one bit to enable all VFs at once. All these VFs will
>> > > have separate netdevs.
>> >
>> > Yes, that is the case here too (before and after).
>> >
>> > What we are talking about is the association of VFs to physical ports
>> > (in the case where a NIC has more than one physical port).
>>
>> We have devices with multiple ports too, but don't have such issues.
>> So it will help if you can provide more context here.
>>
>> I'm failing to see connection between physical ports and physical VFs.
>>
>> Are you saying that physical ports are actual PCI VFs, which spans L2 VFs,
>> which you want to assign to another port (PF)?
>
>No, a physical port is not a VF (nor a PF, FWIIW).
>
>The topic here is about two modes of behaviour.
>
>One, where VFs are associated with physical ports - conceptually one might
>think of this as some VFs and one phys port being plugged into one VEB,
>while other VFs and another phys port are plugged into another VEB.
>
>I believe this is the mode on most multi-port NICs.
>
>And another mode where all VFs are associated with one physical port,
>even if more than one is present. The NFP currently implements this model.
>
>This patch is about allowing NICs, in particular the NFP based NICs,
>to switch modes.

I don't understand the difference between the two modes, 
1) "where VFs are associated with physical ports"
2) "another mode where all VFs are associated with one physical port"

anyway here how it works for ConnectX devices, and i think the model should
be generalized to others as it simplifies the user life in my opinion.

Each physical port is represented by a PCI PF function.

devlink dev show
pci/0000:81:00.0 #port 1
pci/0000:81:00.1 #port 2

when you enable sriov, you enable it on a specific PF, eventually port,
meaning those vfs are only associated with that port.

# enable vfs on port 1
echo 4 > /sys/bus/pci/devices/0000:81:00.0/sriov_numvfs

# enable vfs on port 2
echo 4 > /sys/bus/pci/devices/0000:81:00.1/sriov_numvfs

$ devlink dev show
# port 1 PF
pci/0000:81:00.0

# port 2 PF
pci/0000:81:00.1

# port 1 VFs
pci/0000:81:00.2
pci/0000:81:00.3
pci/0000:81:00.4
pci/0000:81:00.5

# port 2 VFs
pci/0000:81:01.2
pci/0000:81:01.3
pci/0000:81:01.4
pci/0000:81:01.5

The pci address enumeration is device specific, but i don't think user
should care.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jakub Kicinski]

On Wed, 8 Feb 2023 13:37:08 -0800 Saeed Mahameed wrote:
> I don't understand the difference between the two modes, 
> 1) "where VFs are associated with physical ports"
> 2) "another mode where all VFs are associated with one physical port"
> 
> anyway here how it works for ConnectX devices, and i think the model should
> be generalized to others as it simplifies the user life in my opinion.

I'm guessing the version of the NFP Simon posted this for behaves 
much like CX3 / mlx4. One PF, multiple Ethernet ports.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jakub Kicinski]

On Wed, 8 Feb 2023 13:34:19 +0100 Jiri Pirko wrote:
>> Of course if TC is involved, then we have flexibility.
>>
>> What we are talking about here is primarily legacy mode.  
> 
> I don't see any reason to add knobs for purpose of supporting the legacy
> mode, sorry.
> 
> If you need this functionality, use TC.

Agreed, I seem to remember that mlx4 had some custom module param 
to do exactly the same thing. But this is a new addition so we should
just say no.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Saeed Mahameed]

On 08 Feb 15:35, Jakub Kicinski wrote:
>On Wed, 8 Feb 2023 13:37:08 -0800 Saeed Mahameed wrote:
>> I don't understand the difference between the two modes,
>> 1) "where VFs are associated with physical ports"
>> 2) "another mode where all VFs are associated with one physical port"
>>
>> anyway here how it works for ConnectX devices, and i think the model should
>> be generalized to others as it simplifies the user life in my opinion.
>
>I'm guessing the version of the NFP Simon posted this for behaves
>much like CX3 / mlx4. One PF, multiple Ethernet ports.

Then the question is, can they do PF per port and avoid such complex APIs ?

RE: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Yinjun Zhang]

On Wed, 8 Feb 2023 16:55:12 -0800, Saeed Mahameed wrote:
> On 08 Feb 15:35, Jakub Kicinski wrote:
> >On Wed, 8 Feb 2023 13:37:08 -0800 Saeed Mahameed wrote:
> >> I don't understand the difference between the two modes,
> >> 1) "where VFs are associated with physical ports"
> >> 2) "another mode where all VFs are associated with one physical port"
> >>
> >> anyway here how it works for ConnectX devices, and i think the model
> should
> >> be generalized to others as it simplifies the user life in my opinion.
> >
> >I'm guessing the version of the NFP Simon posted this for behaves
> >much like CX3 / mlx4. One PF, multiple Ethernet ports.
> 
> Then the question is, can they do PF per port and avoid such complex APIs ?
> 

To answer your last question, it needs silicon support, so we can't for some old products.

Then let me clarify something more for this patch-set's purpose. 
Indeed, one port per PF is current mainstream. In this case, all the VFs created from PF0
use physical port 0 as the uplink port(outlet to external world), and all the VFs from PF1
use p1 as the uplink port. Let me call them two switch-sets. And they're isolated, you can't 
make the traffic input from VFs of PF0 output to p1 or VFs of PF1, right? Even with TC in
switchdev mode, the two switch-sets are still isolated, right? Correct me if I'm wrong here.
And the posted configuration in this patch-set is useless in this case, it's for one PF with
multi ports.

Let me take NFP implementation for example here, all the VFs created from the single PF
use p0 as the uplink port by default. In legacy mode, by no means we can choose other
ports as outlet. So what we're doing here is try to simulate one-port-per-PF case, to split
one switch-set to several switch-sets with every physical port as the uplink port respectively,
by grouping the VFs and assigning them to physical ports.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jiri Pirko]

Thu, Feb 09, 2023 at 03:20:48AM CET, yinjun.zhang@corigine.com wrote:
>On Wed, 8 Feb 2023 16:55:12 -0800, Saeed Mahameed wrote:
>> On 08 Feb 15:35, Jakub Kicinski wrote:
>> >On Wed, 8 Feb 2023 13:37:08 -0800 Saeed Mahameed wrote:
>> >> I don't understand the difference between the two modes,
>> >> 1) "where VFs are associated with physical ports"
>> >> 2) "another mode where all VFs are associated with one physical port"
>> >>
>> >> anyway here how it works for ConnectX devices, and i think the model
>> should
>> >> be generalized to others as it simplifies the user life in my opinion.
>> >
>> >I'm guessing the version of the NFP Simon posted this for behaves
>> >much like CX3 / mlx4. One PF, multiple Ethernet ports.
>> 
>> Then the question is, can they do PF per port and avoid such complex APIs ?
>> 
>
>To answer your last question, it needs silicon support, so we can't for some old products.
>
>Then let me clarify something more for this patch-set's purpose. 
>Indeed, one port per PF is current mainstream. In this case, all the VFs created from PF0
>use physical port 0 as the uplink port(outlet to external world), and all the VFs from PF1
>use p1 as the uplink port. Let me call them two switch-sets. And they're isolated, you can't 
>make the traffic input from VFs of PF0 output to p1 or VFs of PF1, right? Even with TC in
>switchdev mode, the two switch-sets are still isolated, right? Correct me if I'm wrong here.
>And the posted configuration in this patch-set is useless in this case, it's for one PF with
>multi ports.
>
>Let me take NFP implementation for example here, all the VFs created from the single PF
>use p0 as the uplink port by default. In legacy mode, by no means we can choose other

Legacy is legacy. I believe it is like 5 years already no knobs for
legacy mode are accepted. You should not use it for new features.
Why this is any different?

Implement TC offloading and then you can ballance the hell out of the
thing :)


>ports as outlet. So what we're doing here is try to simulate one-port-per-PF case, to split
>one switch-set to several switch-sets with every physical port as the uplink port respectively,
>by grouping the VFs and assigning them to physical ports.

RE: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Yinjun Zhang]

On Thu, 9 Feb 2023 16:15:58 +0100, Jiri Pirko wrote:
> Thu, Feb 09, 2023 at 03:20:48AM CET, yinjun.zhang@corigine.com wrote:
> >
> >Let me take NFP implementation for example here, all the VFs created from the single PF
> >use p0 as the uplink port by default. In legacy mode, by no means we can choose other
> 
> Legacy is legacy. I believe it is like 5 years already no knobs for
> legacy mode are accepted. You should not use it for new features.
> Why this is any different?
> 
> Implement TC offloading and then you can ballance the hell out of the
> thing :)

I understand in switchdev mode, the fine-grained manipulation by TC can do it.
While legacy has fixed forwarding rule, and we hope it can be implemented without
too much involved configuration from user if they only want legacy forwarding.

As multi-port mapping to one PF NIC is scarce, maybe we should implement is as
vendor specific configuration, make sense?

> 
> 
> >ports as outlet. So what we're doing here is try to simulate one-port-per-PF case, to split
> >one switch-set to several switch-sets with every physical port as the uplink port respectively,
> >by grouping the VFs and assigning them to physical ports.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jakub Kicinski]

On Fri, 10 Feb 2023 02:14:27 +0000 Yinjun Zhang wrote:
> I understand in switchdev mode, the fine-grained manipulation by TC can do it.
> While legacy has fixed forwarding rule, and we hope it can be implemented without
> too much involved configuration from user if they only want legacy forwarding.
> 
> As multi-port mapping to one PF NIC is scarce, maybe we should implement is as
> vendor specific configuration, make sense?

Vendor extension or not we are disallowing adding configuration 
for legacy SR-IOV mode. We want people to move to switchdev mode,
otherwise we'll have to keep extending both for ever.

Re: [PATCH/RFC net-next 1/2] devlink: expose port function commands to assign VFs to multiple netdevs

[Jiri Pirko]

Fri, Feb 10, 2023 at 03:14:27AM CET, yinjun.zhang@corigine.com wrote:
>On Thu, 9 Feb 2023 16:15:58 +0100, Jiri Pirko wrote:
>> Thu, Feb 09, 2023 at 03:20:48AM CET, yinjun.zhang@corigine.com wrote:
>> >
>> >Let me take NFP implementation for example here, all the VFs created from the single PF
>> >use p0 as the uplink port by default. In legacy mode, by no means we can choose other
>> 
>> Legacy is legacy. I believe it is like 5 years already no knobs for
>> legacy mode are accepted. You should not use it for new features.
>> Why this is any different?
>> 
>> Implement TC offloading and then you can ballance the hell out of the
>> thing :)
>
>I understand in switchdev mode, the fine-grained manipulation by TC can do it.
>While legacy has fixed forwarding rule, and we hope it can be implemented without
>too much involved configuration from user if they only want legacy forwarding.
>
>As multi-port mapping to one PF NIC is scarce, maybe we should implement is as
>vendor specific configuration, make sense?

No, it does not make sense what so ever.

You want to extend legacy, which is no longer an option (for many years).

If you need this feature, implement switchdev mode for your device.
Simple as that. I think this was clearly stated in multiple emails in
this thread, I don't follow why it needs to be repeated.


>
>> 
>> 
>> >ports as outlet. So what we're doing here is try to simulate one-port-per-PF case, to split
>> >one switch-set to several switch-sets with every physical port as the uplink port respectively,
>> >by grouping the VFs and assigning them to physical ports.