[PATCH net-next 0/2] Extend devlink for port trust setting

Currently a function (VF/SF) is always untrusted by default. Such a
function does not have the privilege to perform steering database update
as what a switchdev device can do.

In a use case where, a trusted application wants to modify/update the
device steering database through a VF or a SF, add a user knob through
which administrator can mark the function trusted; thereby it can update
the steering database.

This patchset introduces a knob to mark a function trusted. Function
restores to its untrusted state when either user marks it as untrusted
or the function is deleted (SR-IOV disablement or SF port deletion).

Patch Summary:
patch1: extends devlink to get/set trust state
patch2: extends mlx5 driver to get/set trust state setting

example config sequence:
Add SF Port:
$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 88
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted false

Set SF trust setting:
$ devlink port function set pci/0000:08:00.0/32768 trusted true

Query SF settings:
$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted true

Sunil Rani (2):
  devlink: Add support to set port function as trusted
  net/mlx5: SF/VF, Port function trust set support

 .../networking/devlink/devlink-port.rst       |   4 +
 .../net/ethernet/mellanox/mlx5/core/devlink.c |   2 +
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  24 ++++
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  11 +-
 .../mellanox/mlx5/core/eswitch_offloads.c     | 116 ++++++++++++++++++
 include/linux/mlx5/driver.h                   |   1 +
 include/linux/mlx5/mlx5_ifc.h                 |  10 +-
 include/net/devlink.h                         |  22 ++++
 include/uapi/linux/devlink.h                  |   1 +
 net/core/devlink.c                            |  55 +++++++++
 10 files changed, 244 insertions(+), 2 deletions(-)

-- 
2.26.2

Add support to mark a given PCI sub-function (SF) or
Virtual function (VF) as a trusted function. The device/firmware
decides how to define privileges and access to resources.
These functions by default are in untrusted mode.

Examples of add, set a function as trusted and show commands:
$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 88
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted false

$ devlink port function set pci/0000:08:00.0/32768 trusted true

$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted true

Signed-off-by: Sunil Rani <redacted>
Signed-off-by: Bodong Wang <redacted>
Reviewed-by: Parav Pandit <redacted>
Reviewed-by: Jiri Pirko <redacted>
---
 .../networking/devlink/devlink-port.rst       |  4 ++
 include/net/devlink.h                         | 22 ++++++++
 include/uapi/linux/devlink.h                  |  1 +
 net/core/devlink.c                            | 55 +++++++++++++++++++
 4 files changed, 82 insertions(+)

diff --git a/Documentation/networking/devlink/devlink-port.rst b/Documentation/networking/devlink/devlink-port.rst
index 7627b1da01f2..bedd9cd411be 100644
--- a/Documentation/networking/devlink/devlink-port.rst
+++ b/Documentation/networking/devlink/devlink-port.rst

@@ -122,6 +122,10 @@ A user may set the hardware address of the function using
 'devlink port function set hw_addr' command. For Ethernet port function
 this means a MAC address.
 
+A user can set a function as trusted so that a function has the additional
+privileges. One example is to allow trusted function to query and operate
+the steering database similar to the switchdev device.
+
 Subfunction
 ============
 

diff --git a/include/net/devlink.h b/include/net/devlink.h
index aab3d007c577..c82b2113d6fd 100644
--- a/include/net/devlink.h
+++ b/include/net/devlink.h

@@ -1461,6 +1461,28 @@ struct devlink_ops {
 				 enum devlink_port_fn_state state,
 				 struct netlink_ext_ack *extack);
 
+	/**
+	 * port_fn_trusted_get() - Get the trusted state of port function
+	 * @port: The devlink port
+	 * @trusted: Query privilege state
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: 0 on success, negative value otherwise.
+	 */
+	int (*port_fn_trusted_get)(struct devlink_port *port,
+				   bool *trusted,
+				   struct netlink_ext_ack *extack);
+	/**
+	 * port_fn_trusted_set() - Set the trusted state of port function
+	 * @port: The devlink port
+	 * @trusted: Set privilege state
+	 * @extack: extack for reporting error messages
+	 *
+	 * Return: 0 on success, negative value otherwise.
+	 */
+	int (*port_fn_trusted_set)(struct devlink_port *port,
+				   bool trusted,
+				   struct netlink_ext_ack *extack);
 	/**
 	 * Rate control callbacks.
 	 */

diff --git a/include/uapi/linux/devlink.h b/include/uapi/linux/devlink.h
index b897b80770f6..36624a356478 100644
--- a/include/uapi/linux/devlink.h
+++ b/include/uapi/linux/devlink.h

@@ -604,6 +604,7 @@ enum devlink_port_function_attr {
 	DEVLINK_PORT_FUNCTION_ATTR_HW_ADDR,	/* binary */
 	DEVLINK_PORT_FN_ATTR_STATE,	/* u8 */
 	DEVLINK_PORT_FN_ATTR_OPSTATE,	/* u8 */
+	DEVLINK_PORT_FN_ATTR_TRUSTED,	/* u8 */
 
 	__DEVLINK_PORT_FUNCTION_ATTR_MAX,
 	DEVLINK_PORT_FUNCTION_ATTR_MAX = __DEVLINK_PORT_FUNCTION_ATTR_MAX - 1

diff --git a/net/core/devlink.c b/net/core/devlink.c
index 5ba4f9434acd..6aaa3a67194a 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c

@@ -147,6 +147,7 @@ static const struct nla_policy devlink_function_nl_policy[DEVLINK_PORT_FUNCTION_
 	[DEVLINK_PORT_FN_ATTR_STATE] =
 		NLA_POLICY_RANGE(NLA_U8, DEVLINK_PORT_FN_STATE_INACTIVE,
 				 DEVLINK_PORT_FN_STATE_ACTIVE),
+	[DEVLINK_PORT_FN_ATTR_TRUSTED] = { .type = NLA_U8 },
 };
 
 static DEFINE_XARRAY_FLAGS(devlinks, XA_FLAGS_ALLOC);

@@ -986,6 +987,31 @@ devlink_port_fn_opstate_valid(enum devlink_port_fn_opstate opstate)
 	       opstate == DEVLINK_PORT_FN_OPSTATE_ATTACHED;
 }
 
+static int devlink_port_fn_trusted_fill(const struct devlink_ops *ops,
+					struct devlink_port *port,
+					struct sk_buff *msg,
+					struct netlink_ext_ack *extack,
+					bool *msg_updated)
+{
+	bool trusted;
+	int err;
+
+	if (!ops->port_fn_trusted_get)
+		return 0;
+
+	err = ops->port_fn_trusted_get(port, &trusted, extack);
+	if (err) {
+		if (err == -EOPNOTSUPP)
+			return 0;
+		return err;
+	}
+
+	if (nla_put_u8(msg, DEVLINK_PORT_FN_ATTR_TRUSTED, trusted))
+		return -EMSGSIZE;
+	*msg_updated = true;
+	return 0;
+}
+
 static int devlink_port_fn_state_fill(const struct devlink_ops *ops,
 				      struct devlink_port *port,
 				      struct sk_buff *msg,

@@ -1042,6 +1068,9 @@ devlink_nl_port_function_attrs_put(struct sk_buff *msg, struct devlink_port *por
 	if (err)
 		goto out;
 	err = devlink_port_fn_state_fill(ops, port, msg, extack, &msg_updated);
+	if (err)
+		goto out;
+	err = devlink_port_fn_trusted_fill(ops, port, msg, extack, &msg_updated);
 out:
 	if (err || !msg_updated)
 		nla_nest_cancel(msg, function_attr);

@@ -1434,6 +1463,25 @@ static int devlink_port_function_hw_addr_set(struct devlink_port *port,
 					      extack);
 }
 
+static int devlink_port_fn_trusted_set(struct devlink_port *port,
+				       const struct nlattr *attr,
+				       struct netlink_ext_ack *extack)
+{
+	const struct devlink_ops *ops;
+	bool trusted;
+
+	if (nla_get_u8(attr) > 1)
+		return -EINVAL;
+
+	trusted = nla_get_u8(attr);
+	ops = port->devlink->ops;
+	if (!ops->port_fn_trusted_set) {
+		NL_SET_ERR_MSG_MOD(extack, "Function does not support trust setting");
+		return -EOPNOTSUPP;
+	}
+	return ops->port_fn_trusted_set(port, trusted, extack);
+}
+
 static int devlink_port_fn_state_set(struct devlink_port *port,
 				     const struct nlattr *attr,
 				     struct netlink_ext_ack *extack)

@@ -1471,6 +1519,13 @@ static int devlink_port_function_set(struct devlink_port *port,
 		if (err)
 			return err;
 	}
+
+	attr = tb[DEVLINK_PORT_FN_ATTR_TRUSTED];
+	if (attr) {
+		err = devlink_port_fn_trusted_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.26.2

Add support to mark a given PCI sub-function (SF) or
Virtual function (VF) as a trusted function. The device/firmware
decides how to define privileges and access to resources. Trust
state is queried and cached during function creation. These functions
by default are in untrusted mode.

Function restores to its untrusted state when either user marks it as
untrusted or the function is deleted (SR-IOV disablement or
SF port deletion).

Examples of add, change privilege level and show commands:
$ devlink port add pci/0000:08:00.0 flavour pcisf pfnum 0 sfnum 88
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted false

$ devlink port function set pci/0000:08:00.0/32768 trusted true

$ devlink port show pci/0000:08:00.0/32768
pci/0000:08:00.0/32768: type eth netdev eth6 flavour pcisf controller 0 pfnum 0 sfnum 88 splittable false
  function:
    hw_addr 00:00:00:00:00:00 state inactive opstate detached trusted true

Signed-off-by: Sunil Rani <redacted>
Signed-off-by: Bodong Wang <redacted>
Reviewed-by: Parav Pandit <redacted>
Reviewed-by: Saeed Mahameed < saeedm@nvidia.com>
Reviewed-by: Mark Bloch <mbloch@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/devlink.c |   2 +
 .../net/ethernet/mellanox/mlx5/core/eswitch.c |  24 ++++
 .../net/ethernet/mellanox/mlx5/core/eswitch.h |  11 +-
 .../mellanox/mlx5/core/eswitch_offloads.c     | 116 ++++++++++++++++++
 include/linux/mlx5/driver.h                   |   1 +
 include/linux/mlx5/mlx5_ifc.h                 |  10 +-
 6 files changed, 162 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
index 1c98652b244a..cbf4288a777a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/devlink.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/devlink.c

@@ -298,6 +298,8 @@ static const struct devlink_ops mlx5_devlink_ops = {
 	.eswitch_encap_mode_get = mlx5_devlink_eswitch_encap_mode_get,
 	.port_function_hw_addr_get = mlx5_devlink_port_function_hw_addr_get,
 	.port_function_hw_addr_set = mlx5_devlink_port_function_hw_addr_set,
+	.port_fn_trusted_set = mlx5_devlink_port_function_trusted_set,
+	.port_fn_trusted_get = mlx5_devlink_port_function_trusted_get,
 	.rate_leaf_tx_share_set = mlx5_esw_devlink_rate_leaf_tx_share_set,
 	.rate_leaf_tx_max_set = mlx5_esw_devlink_rate_leaf_tx_max_set,
 	.rate_node_tx_share_set = mlx5_esw_devlink_rate_node_tx_share_set,

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
index 46532dd42b43..aca52a474af1 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.c

@@ -820,6 +820,24 @@ static void esw_vport_cleanup(struct mlx5_eswitch *esw, struct mlx5_vport *vport
 	esw_vport_cleanup_acl(esw, vport);
 }
 
+static int mlx5_esw_query_hca_trusted(struct mlx5_eswitch *esw,
+				      struct mlx5_vport *vport)
+{
+	bool trusted;
+	int err;
+
+	err = mlx5_esw_get_hca_trusted(esw, vport->vport, &trusted);
+	if (err == -EOPNOTSUPP)
+		return 0;
+
+	if (err)
+		return err;
+
+	vport->info.offloads_trusted = trusted;
+
+	return 0;
+}
+
 int mlx5_esw_vport_enable(struct mlx5_eswitch *esw, u16 vport_num,
 			  enum mlx5_eswitch_vport_event enabled_events)
 {

@@ -857,6 +875,12 @@ int mlx5_esw_vport_enable(struct mlx5_eswitch *esw, u16 vport_num,
 			goto err_vhca_mapping;
 	}
 
+	if (!mlx5_esw_is_manager_vport(esw, vport_num)) {
+		ret = mlx5_esw_query_hca_trusted(esw, vport);
+		if (ret)
+			goto err_vhca_mapping;
+	}
+
 	/* External controller host PF has factory programmed MAC.
 	 * Read it from the device.
 	 */

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
index 513f741d16c7..116cfa1c7ca8 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch.h

@@ -150,6 +150,7 @@ struct mlx5_vport_info {
 	u8                      qos;
 	u8                      spoofchk: 1;
 	u8                      trusted: 1;
+	u8                      offloads_trusted: 1;
 };
 
 /* Vport context events */

@@ -510,7 +511,15 @@ int mlx5_devlink_port_function_hw_addr_get(struct devlink_port *port,
 int mlx5_devlink_port_function_hw_addr_set(struct devlink_port *port,
 					   const u8 *hw_addr, int hw_addr_len,
 					   struct netlink_ext_ack *extack);
-
+int mlx5_devlink_port_function_trusted_get(struct devlink_port *port,
+					   bool *trusted,
+					   struct netlink_ext_ack *extack);
+int mlx5_devlink_port_function_trusted_set(struct devlink_port *port,
+					   bool trusted,
+					   struct netlink_ext_ack *extack);
+int mlx5_esw_get_hca_trusted(struct mlx5_eswitch *esw,
+			     u16 vport_num,
+			     bool *trusted);
 void *mlx5_eswitch_get_uplink_priv(struct mlx5_eswitch *esw, u8 rep_type);
 
 int mlx5_eswitch_add_vlan_action(struct mlx5_eswitch *esw,

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
index 4bd502ae82b6..0812cf826787 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c

@@ -3871,6 +3871,122 @@ is_port_function_supported(struct mlx5_eswitch *esw, u16 vport_num)
 	       mlx5_esw_is_sf_vport(esw, vport_num);
 }
 
+static struct mlx5_vport *
+mlx5_dlport_to_vport_get(struct devlink_port *port, struct mlx5_eswitch *esw,
+			 struct netlink_ext_ack *extack)
+{
+	u16 vport_num;
+
+	vport_num = mlx5_esw_devlink_port_index_to_vport_num(port->index);
+	if (!is_port_function_supported(esw, vport_num))
+		return ERR_PTR(-EOPNOTSUPP);
+
+	return mlx5_eswitch_get_vport(esw, vport_num);
+}
+
+static int mlx5_esw_set_hca_trusted(struct mlx5_eswitch *esw, u16 vport_num, bool trusted)
+{
+	u32 out[MLX5_ST_SZ_DW(vhca_trust_level)] = {};
+	u32 in[MLX5_ST_SZ_DW(vhca_trust_level)] = {};
+	int sz = MLX5_ST_SZ_BYTES(vhca_trust_level);
+	u16 vhca_id;
+	int err;
+
+	if (!MLX5_CAP_GEN(esw->dev, vhca_trust_level_reg))
+		return -EOPNOTSUPP;
+
+	err = mlx5_esw_query_vport_vhca_id(esw, vport_num, &vhca_id);
+	if (err) {
+		esw_warn(esw->dev, "Getting vhca_id for vport=%u failed err=%d\n",
+			 vport_num, err);
+		return err;
+	}
+
+	MLX5_SET(vhca_trust_level, in, vhca_id, vhca_id);
+	MLX5_SET(vhca_trust_level, in, trust_level, trusted);
+
+	return mlx5_core_access_reg(esw->dev, in, sz, out, sz, MLX5_REG_TRUST_LEVEL, 0, 1);
+}
+
+int mlx5_devlink_port_function_trusted_set(struct devlink_port *port,
+					   bool trusted,
+					   struct netlink_ext_ack *extack)
+{
+	struct mlx5_eswitch *esw;
+	struct mlx5_vport *vport;
+	int err;
+
+	esw = mlx5_devlink_eswitch_get(port->devlink);
+	if (IS_ERR(esw))
+		return PTR_ERR(esw);
+
+	vport = mlx5_dlport_to_vport_get(port, esw, extack);
+	if (IS_ERR(vport)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Failed to get vport");
+		return PTR_ERR(vport);
+	}
+
+	err = mlx5_esw_set_hca_trusted(esw, vport->vport, trusted);
+	if (!err)
+		vport->info.offloads_trusted = trusted;
+
+	return err;
+}
+
+int mlx5_esw_get_hca_trusted(struct mlx5_eswitch *esw, u16 vport_num, bool *trusted)
+{
+	u32 out[MLX5_ST_SZ_DW(vhca_trust_level)] = {};
+	u32 in[MLX5_ST_SZ_DW(vhca_trust_level)] = {};
+	int sz = MLX5_ST_SZ_BYTES(vhca_trust_level);
+	u32 trust_level;
+	u16 vhca_id;
+	int err;
+
+	if (!MLX5_CAP_GEN(esw->dev, vhca_trust_level_reg))
+		return -EOPNOTSUPP;
+
+	err = mlx5_esw_query_vport_vhca_id(esw, vport_num, &vhca_id);
+	if (err) {
+		esw_warn(esw->dev, "Query of vhca_id for vport %d failed, err %d\n",
+			 vport_num, err);
+		return err;
+	}
+
+	MLX5_SET(vhca_trust_level, in, vhca_id, vhca_id);
+	mlx5_core_access_reg(esw->dev, in, sz, out, sz, MLX5_REG_TRUST_LEVEL, 0, 0);
+	trust_level = MLX5_GET(vhca_trust_level, out, trust_level);
+	*trusted = trust_level & 0x1;
+
+	return 0;
+}
+
+int mlx5_devlink_port_function_trusted_get(struct devlink_port *port,
+					   bool *trusted,
+					   struct netlink_ext_ack *extack)
+{
+	struct mlx5_eswitch *esw;
+	struct mlx5_vport *vport;
+
+	esw = mlx5_devlink_eswitch_get(port->devlink);
+	if (IS_ERR(esw))
+		return PTR_ERR(esw);
+
+	if (!MLX5_CAP_GEN(esw->dev, vhca_trust_level_reg))
+		return -EOPNOTSUPP;
+
+	vport = mlx5_dlport_to_vport_get(port, esw, extack);
+	if (IS_ERR(vport)) {
+		NL_SET_ERR_MSG_MOD(extack,
+				   "Failed to get vport");
+		return PTR_ERR(vport);
+	}
+
+	*trusted = vport->info.offloads_trusted;
+
+	return 0;
+}
+
 int mlx5_devlink_port_function_hw_addr_get(struct devlink_port *port,
 					   u8 *hw_addr, int *hw_addr_len,
 					   struct netlink_ext_ack *extack)

diff --git a/include/linux/mlx5/driver.h b/include/linux/mlx5/driver.h
index a623ec635947..ce8e35f59851 100644
--- a/include/linux/mlx5/driver.h
+++ b/include/linux/mlx5/driver.h

@@ -153,6 +153,7 @@ enum {
 	MLX5_REG_MIRC		 = 0x9162,
 	MLX5_REG_SBCAM		 = 0xB01F,
 	MLX5_REG_RESOURCE_DUMP   = 0xC000,
+	MLX5_REG_TRUST_LEVEL     = 0xC007,
 	MLX5_REG_DTOR            = 0xC00E,
 };
 

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 3636df90899a..c9b39a08aae0 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h

@@ -1401,7 +1401,8 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 
 	u8         reserved_at_120[0xa];
 	u8         log_max_ra_req_dc[0x6];
-	u8         reserved_at_130[0xa];
+	u8         vhca_trust_level_reg[0x1];
+	u8         reserved_at_131[0x9];
 	u8         log_max_ra_res_dc[0x6];
 
 	u8         reserved_at_140[0x6];

@@ -11485,6 +11486,13 @@ struct mlx5_ifc_tls_progress_params_bits {
 	u8         hw_offset_record_number[0x18];
 };
 
+struct mlx5_ifc_vhca_trust_level_bits {
+	u8        all_vhca[0x1];
+	u8        reserved_0[0xf];
+	u8        vhca_id[0x10];
+	u8        trust_level[0x20];
+};
+
 enum {
 	MLX5_MTT_PERM_READ	= 1 << 0,
 	MLX5_MTT_PERM_WRITE	= 1 << 1,

-- 
2.26.2

On Mon, 22 Nov 2021 16:43:06 +0200 Sunil Rani wrote:

The device/firmware decides how to define privileges and access to resources.

Great API definition. nack

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>

External email: Use caution opening links or attachments


On Mon, 22 Nov 2021 16:43:06 +0200 Sunil Rani wrote:

quoted

The device/firmware decides how to define privileges and access to

resources.

Great API definition. Nack

Hi Jakub, 

Sorry for the late response. We agree that the current definition is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or restricted resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such as physical port counters.

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:

quoted

On Mon, 22 Nov 2021 16:43:06 +0200 Sunil Rani wrote:  

quoted

The device/firmware decides how to define privileges and access to resources.

Great API definition. Nack  

Sorry for the late response. We agree that the current definition is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or restricted resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such as physical port counters.

You need to say more about the use case, I don't understand 
what you're doing.

On Tue, 2021-11-30 at 19:12 -0800, Jakub Kicinski wrote:

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:

quoted

quoted

On Mon, 22 Nov 2021 16:43:06 +0200 Sunil Rani wrote:  

quoted

The device/firmware decides how to define privileges and access
to resources.

Great API definition. Nack  

Sorry for the late response. We agree that the current definition
is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or restricted
resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such as
physical port counters.

You need to say more about the use case, I don't understand 
what you're doing.

Some device features/registers/units are not available by default to
VFs/SFs (e.g restricted), examples are: physical port
registers/counters and similar global attributes.

Some customers want to use SF/VF in specialized VM/container for
management and monitoring, thus they want SF/VF to have similar
privileges to PF in terms of access to restricted resources.

Note: this doesn't break the sriov/sf model, trusted SF/VF will not be
allowed to alter device attributes, they will simply enjoy access to
more resources/features.

We would've pushed for a more fine-grained per "capability" API, but
where do we start/end? I think "trust" concept is the right approach.

On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:

On Tue, 2021-11-30 at 19:12 -0800, Jakub Kicinski wrote:

quoted

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:  

quoted

Sorry for the late response. We agree that the current definition
is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or restricted
resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such as
physical port counters.  

You need to say more about the use case, I don't understand 
what you're doing.  

Some device features/registers/units are not available by default to
VFs/SFs (e.g restricted), examples are: physical port
registers/counters and similar global attributes.

Some customers want to use SF/VF in specialized VM/container for
management and monitoring, thus they want SF/VF to have similar
privileges to PF in terms of access to restricted resources.

Note: this doesn't break the sriov/sf model, trusted SF/VF will not be
allowed to alter device attributes, they will simply enjoy access to
more resources/features.

None of this explains the use case. It's pretty much what Sunil already
stated. 

We would've pushed for a more fine-grained per "capability" API, but
where do we start/end? I think "trust" concept is the right approach.

On Thu, 2021-12-02 at 09:31 -0800, Jakub Kicinski wrote:

On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:

quoted

On Tue, 2021-11-30 at 19:12 -0800, Jakub Kicinski wrote:

quoted

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:  

quoted

Sorry for the late response. We agree that the current
definition
is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or
restricted
resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such
as
physical port counters.  

You need to say more about the use case, I don't understand 
what you're doing.  

Some device features/registers/units are not available by default
to
VFs/SFs (e.g restricted), examples are: physical port
registers/counters and similar global attributes.

Some customers want to use SF/VF in specialized VM/container for
management and monitoring, thus they want SF/VF to have similar
privileges to PF in terms of access to restricted resources.

Note: this doesn't break the sriov/sf model, trusted SF/VF will not
be
allowed to alter device attributes, they will simply enjoy access
to
more resources/features.

None of this explains the use case. It's pretty much what Sunil
already
stated. 

:) Fair enough.

On Thu, 2021-12-02 at 09:31 -0800, Jakub Kicinski wrote:

On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:

quoted

On Tue, 2021-11-30 at 19:12 -0800, Jakub Kicinski wrote:

quoted

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:  

quoted

Sorry for the late response. We agree that the current
definition
is vague.

What we meant is that the enforcement is done by device/FW.
We simply want to allow VF/SF to access privileged or
restricted
resource such as physical port counters.
So how about defining the api such that:
This knob allows the VF/SF to access restricted resource such
as
physical port counters.  

You need to say more about the use case, I don't understand 
what you're doing.  

Some device features/registers/units are not available by default
to
VFs/SFs (e.g restricted), examples are: physical port
registers/counters and similar global attributes.

Some customers want to use SF/VF in specialized VM/container for
management and monitoring, thus they want SF/VF to have similar
privileges to PF in terms of access to restricted resources.

Note: this doesn't break the sriov/sf model, trusted SF/VF will not
be
allowed to alter device attributes, they will simply enjoy access
to
more resources/features.

None of this explains the use case. It's pretty much what Sunil
already
stated. 

After some internal discussions, the plan is to not push new
interfaces, but to utilize the existing devlink params interface for
devlink port functions.

We will suggest a more fine grained parameters to control a port
function (SF/VF) well-defined capabilities.

devlink port function param set/get DEV/PORT_INDEX name PARAMETER value
VALUE cmode { runtime | driverinit | permanent }

Jiri is already on-board. Jakub I hope you are ok with this, let us
know if you have any concerns before we start implementation.

Thanks,
Saeed.

On Wed, 15 Dec 2021 18:19:16 +0000 Saeed Mahameed wrote:

After some internal discussions, the plan is to not push new
interfaces, but to utilize the existing devlink params interface for
devlink port functions.

We will suggest a more fine grained parameters to control a port
function (SF/VF) well-defined capabilities.

devlink port function param set/get DEV/PORT_INDEX name PARAMETER value
VALUE cmode { runtime | driverinit | permanent }

Jiri is already on-board. Jakub I hope you are ok with this, let us
know if you have any concerns before we start implementation.

You can use mail pigeon to configure this, my questions were about 
the feature itself not the interface.

On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:

On Wed, 15 Dec 2021 18:19:16 +0000 Saeed Mahameed wrote:

quoted

After some internal discussions, the plan is to not push new
interfaces, but to utilize the existing devlink params interface
for
devlink port functions.

We will suggest a more fine grained parameters to control a port
function (SF/VF) well-defined capabilities.

devlink port function param set/get DEV/PORT_INDEX name PARAMETER
value
VALUE cmode { runtime | driverinit | permanent }

Jiri is already on-board. Jakub I hope you are ok with this, let us
know if you have any concerns before we start implementation.

You can use mail pigeon to configure this, my questions were about 
the feature itself not the interface.

We will have a parameter per feature we want to enable/disable instead
of a global "trust" knob.

On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:

On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:

quoted

On Wed, 15 Dec 2021 18:19:16 +0000 Saeed Mahameed wrote:  

quoted

After some internal discussions, the plan is to not push new
interfaces, but to utilize the existing devlink params interface
for
devlink port functions.

We will suggest a more fine grained parameters to control a port
function (SF/VF) well-defined capabilities.

devlink port function param set/get DEV/PORT_INDEX name PARAMETER
value
VALUE cmode { runtime | driverinit | permanent }

Jiri is already on-board. Jakub I hope you are ok with this, let us
know if you have any concerns before we start implementation.  

You can use mail pigeon to configure this, my questions were about 
the feature itself not the interface.  

We will have a parameter per feature we want to enable/disable instead
of a global "trust" knob.

So you're just asking me if I'm okay with devlink params regardless if
I'm okay with what they control? Not really, I prefer an API as created
by this patches.

On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:

quoted

On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:

quoted

On Wed, 15 Dec 2021 18:19:16 +0000 Saeed Mahameed wrote:

quoted

After some internal discussions, the plan is to not push new
interfaces, but to utilize the existing devlink params interface
for devlink port functions.

We will suggest a more fine grained parameters to control a port
function (SF/VF) well-defined capabilities.

devlink port function param set/get DEV/PORT_INDEX name

PARAMETER

quoted

quoted

quoted

value VALUE cmode { runtime | driverinit | permanent }

Jiri is already on-board. Jakub I hope you are ok with this, let
us know if you have any concerns before we start implementation.

You can use mail pigeon to configure this, my questions were about
the feature itself not the interface.

We will have a parameter per feature we want to enable/disable instead
of a global "trust" knob.

So you're just asking me if I'm okay with devlink params regardless if I'm okay
with what they control? Not really, I prefer an API as created by this patches.

What shortcomings do you see in the finer granular approach we want to go to enable/disable
On a per feature basis instead of global knob?

On Thu, 16 Dec 2021 16:17:29 +0000 Sunil Sudhakar Rani wrote:

quoted

On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:  

quoted

We will have a parameter per feature we want to enable/disable instead
of a global "trust" knob.  

So you're just asking me if I'm okay with devlink params regardless if I'm okay
with what they control? Not really, I prefer an API as created by this patches.  

What shortcomings do you see in the finer granular approach we want
to go to enable/disable On a per feature basis instead of global knob?

I was replying to Saeed so I assumed some context which you probably
lack. Granular approach is indeed better, what I was referring to when
I said "prefer an API as created by this patch" was having an dedicated
devlink op, instead of the use of devlink params.

Hi Jakub,

From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, December 16, 2021 9:58 PM

On Thu, 16 Dec 2021 16:17:29 +0000 Sunil Sudhakar Rani wrote:

quoted

quoted

On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:

quoted

We will have a parameter per feature we want to enable/disable
instead of a global "trust" knob.

So you're just asking me if I'm okay with devlink params regardless
if I'm okay with what they control? Not really, I prefer an API as created by

this patches.

quoted

What shortcomings do you see in the finer granular approach we want to
go to enable/disable On a per feature basis instead of global knob?

I was replying to Saeed so I assumed some context which you probably lack.
Granular approach is indeed better, what I was referring to when I said "prefer
an API as created by this patch" was having an dedicated devlink op, instead of
the use of devlink params.

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before deploying such function.
As you suggested we discussed the granular approach and at present we have following features to on/off.

Generic features:
1. ipsec offload
2. ptp device

Device specific:
1. sw steering
2. physical port counters query

It was implicit that a driver API callback addition for both types of features is not good.
Devlink port function params enables to achieve both generic and device specific features.
Shall we proceed with port function params? What do you think?

On Tue, 11 Jan 2022 16:57:54 +0000 Parav Pandit wrote:

quoted

quoted

What shortcomings do you see in the finer granular approach we want to
go to enable/disable On a per feature basis instead of global knob?  

I was replying to Saeed so I assumed some context which you probably lack.
Granular approach is indeed better, what I was referring to when I said "prefer
an API as created by this patch" was having an dedicated devlink op, instead of
the use of devlink params.  

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before deploying such function.
As you suggested we discussed the granular approach and at present we have following features to on/off.

Generic features:
1. ipsec offload

Why is ipsec offload a trusted feature?

2. ptp device

Makes sense.

Device specific:
1. sw steering

No idea what that is/entails.

2. physical port counters query

Still don't know why VF needs to know phy counters.

It was implicit that a driver API callback addition for both types of features is not good.
Devlink port function params enables to achieve both generic and device specific features.
Shall we proceed with port function params? What do you think?

I already addressed this. I don't like devlink params. They muddy the
water between vendor specific gunk and bona fide Linux uAPI. Build a
normal dedicated API.

From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, January 11, 2022 11:50 PM

On Tue, 11 Jan 2022 16:57:54 +0000 Parav Pandit wrote:

quoted

quoted

quoted

What shortcomings do you see in the finer granular approach we
want to go to enable/disable On a per feature basis instead of global

knob?

quoted

quoted

I was replying to Saeed so I assumed some context which you probably

lack.

quoted

quoted

Granular approach is indeed better, what I was referring to when I
said "prefer an API as created by this patch" was having an
dedicated devlink op, instead of the use of devlink params.

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before deploying such

function.

quoted

As you suggested we discussed the granular approach and at present we

have following features to on/off.

quoted

Generic features:
1. ipsec offload

Why is ipsec offload a trusted feature?

It isn't trusted feature. The scope in few weeks got expanded from trusted to more granular at controlling capabilities.
One that came up was ipsec or other offloads that consumes more device resources.
 

quoted

2. ptp device

Makes sense.

quoted

Device specific:
1. sw steering

No idea what that is/entails.

:) it the device specific knob.

quoted

2. physical port counters query

Still don't know why VF needs to know phy counters.

A prometheous kind of monitoring software wants to monitor the physical port counters, running in a container.
Such container doesn't have direct access to the PF or physical representor.
Just for sake of monitoring counters, user doesn't want to run the monitoring container in root net ns.
 

quoted

It was implicit that a driver API callback addition for both types of features is

not good.

quoted

Devlink port function params enables to achieve both generic and device

specific features.

quoted

Shall we proceed with port function params? What do you think?

I already addressed this. I don't like devlink params. They muddy the water
between vendor specific gunk and bona fide Linux uAPI. Build a normal
dedicated API.

For sure we prefer the bona fide Linux uAPI for standard features.
But internal knobs of how to do steering etc, is something not generic enough.
May be only those quirks live in the port function params and rest in standard uAPIs?

On Tue, 11 Jan 2022 18:26:16 +0000 Parav Pandit wrote:

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, January 11, 2022 11:50 PM

quoted

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before deploying such  
function. As you suggested we discussed the granular approach and at present we  
have following features to on/off.  

Generic features:
1. ipsec offload  

Why is ipsec offload a trusted feature?

It isn't trusted feature. The scope in few weeks got expanded from
trusted to more granular at controlling capabilities. One that came
up was ipsec or other offloads that consumes more device resources. 

That's what I thought. Resource control is different than privileges,
and requires a different API.

quoted

quoted

2. ptp device

Makes sense.
  

quoted

Device specific:
1. sw steering  

No idea what that is/entails.
  

:) it the device specific knob.

quoted

quoted

2. physical port counters query  

Still don't know why VF needs to know phy counters.

A prometheous kind of monitoring software wants to monitor the
physical port counters, running in a container. Such container
doesn't have direct access to the PF or physical representor. Just
for sake of monitoring counters, user doesn't want to run the
monitoring container in root net ns.

Containerizing monitors seems very counter-intuitive to me.

quoted

quoted

It was implicit that a driver API callback addition for both
types of features is not good.  
Devlink port function params enables to achieve both generic and
device specific features.  
Shall we proceed with port function params? What do you think?  

I already addressed this. I don't like devlink params. They muddy
the water between vendor specific gunk and bona fide Linux uAPI.
Build a normal dedicated API.  

For sure we prefer the bona fide Linux uAPI for standard features.
But internal knobs of how to do steering etc, is something not
generic enough. May be only those quirks live in the port function
params and rest in standard uAPIs?

Something talks to that steering API, and it's not netdev. So please
don't push problems which are not ours onto us.

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 12, 2022 12:54 AM

On Tue, 11 Jan 2022 18:26:16 +0000 Parav Pandit wrote:

quoted

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Tuesday, January 11, 2022 11:50 PM

quoted

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before
deploying such function. As you suggested we discussed the
granular approach and at present we have following features to on/off.

Generic features:
1. ipsec offload

Why is ipsec offload a trusted feature?

It isn't trusted feature. The scope in few weeks got expanded from
trusted to more granular at controlling capabilities. One that came up
was ipsec or other offloads that consumes more device resources.

That's what I thought. Resource control is different than privileges, and
requires a different API.

It's the capability that is turned on/off.
A device is composed based on what is needed. ipsec offload is not always needed.
Its counter intuitive to expose some low level hardware resource to disable ipsec indirectly.
So it is better to do as capability/param rather than some resource.
It is capability is more than just resource.

quoted

quoted

quoted

2. ptp device

Makes sense.

quoted

Device specific:
1. sw steering

No idea what that is/entails.

:) it the device specific knob.

quoted

quoted

2. physical port counters query

Still don't know why VF needs to know phy counters.

A prometheous kind of monitoring software wants to monitor the
physical port counters, running in a container. Such container doesn't
have direct access to the PF or physical representor. Just for sake of
monitoring counters, user doesn't want to run the monitoring container
in root net ns.

Containerizing monitors seems very counter-intuitive to me.

May be. But it is in use at [1] for a long time now.

[1] docker run -p 9090:9090 prom/prometheus
 

quoted

quoted

quoted

It was implicit that a driver API callback addition for both types
of features is not good.
Devlink port function params enables to achieve both generic and
device specific features.
Shall we proceed with port function params? What do you think?

I already addressed this. I don't like devlink params. They muddy
the water between vendor specific gunk and bona fide Linux uAPI.
Build a normal dedicated API.

For sure we prefer the bona fide Linux uAPI for standard features.
But internal knobs of how to do steering etc, is something not generic
enough. May be only those quirks live in the port function params and
rest in standard uAPIs?

Something talks to that steering API, and it's not netdev. So please don't push
problems which are not ours onto us.

Not sure I follow you.
Netdev of a mlx5 function talks to the driver internal steering API in addition to other drivers operating this mlx5 function.

On Tue, 11 Jan 2022 19:39:37 +0000 Parav Pandit wrote:

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 12, 2022 12:54 AM

On Tue, 11 Jan 2022 18:26:16 +0000 Parav Pandit wrote:  

quoted

It isn't trusted feature. The scope in few weeks got expanded from
trusted to more granular at controlling capabilities. One that came up
was ipsec or other offloads that consumes more device resources.  

That's what I thought. Resource control is different than privileges, and
requires a different API.
 

It's the capability that is turned on/off.
A device is composed based on what is needed. ipsec offload is not always needed.
Its counter intuitive to expose some low level hardware resource to disable ipsec indirectly.
So it is better to do as capability/param rather than some resource.
It is capability is more than just resource.

Wouldn't there be some limitation on the number of SAs or max
throughput or such to limit on VF hogging the entire crypto path?

I was expecting such a knob, and then turning it to 0 would effectively
remove the capability (FW can completely hide it or driver ignore it).

quoted

quoted

A prometheous kind of monitoring software wants to monitor the
physical port counters, running in a container. Such container doesn't
have direct access to the PF or physical representor. Just for sake of
monitoring counters, user doesn't want to run the monitoring container
in root net ns.  

Containerizing monitors seems very counter-intuitive to me.
 

May be. But it is in use at [1] for a long time now.

[1] docker run -p 9090:9090 prom/prometheus

How is it "in use" if we haven't merged the patch to enable it? :)
What does it monitor? PHYs port does not include east-west traffic,
exposing just the PHYs stats seems like a half measure.

quoted

quoted

For sure we prefer the bona fide Linux uAPI for standard features.
But internal knobs of how to do steering etc, is something not generic
enough. May be only those quirks live in the port function params and
rest in standard uAPIs?  

Something talks to that steering API, and it's not netdev. So please don't push
problems which are not ours onto us.  

Not sure I follow you.
Netdev of a mlx5 function talks to the driver internal steering API
in addition to other drivers operating this mlx5 function.

But there is no such thing as "steering API" in netdev. We can expose
the functionality we do have, if say PTP requires some steering then
enabling PTP implies the required steering is enabled. "steering API"
as an entity is meaningless to a netdev user.

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 12, 2022 1:27 AM

On Tue, 11 Jan 2022 19:39:37 +0000 Parav Pandit wrote:

quoted

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 12, 2022 12:54 AM

On Tue, 11 Jan 2022 18:26:16 +0000 Parav Pandit wrote:

quoted

It isn't trusted feature. The scope in few weeks got expanded from
trusted to more granular at controlling capabilities. One that
came up was ipsec or other offloads that consumes more device

resources.

quoted

quoted

That's what I thought. Resource control is different than
privileges, and requires a different API.

It's the capability that is turned on/off.
A device is composed based on what is needed. ipsec offload is not always

needed.

quoted

Its counter intuitive to expose some low level hardware resource to disable

ipsec indirectly.

quoted

So it is better to do as capability/param rather than some resource.
It is capability is more than just resource.

Wouldn't there be some limitation on the number of SAs or max throughput or
such to limit on VF hogging the entire crypto path?

The fairness among VFs is present via the QoS knobs. Hence it doesn't hogg the entire crypto path.
 

I was expecting such a knob, and then turning it to 0 would effectively remove
the capability (FW can completely hide it or driver ignore it).

quoted

quoted

quoted

A prometheous kind of monitoring software wants to monitor the
physical port counters, running in a container. Such container
doesn't have direct access to the PF or physical representor. Just
for sake of monitoring counters, user doesn't want to run the
monitoring container in root net ns.

Containerizing monitors seems very counter-intuitive to me.

May be. But it is in use at [1] for a long time now.

[1] docker run -p 9090:9090 prom/prometheus

How is it "in use" if we haven't merged the patch to enable it? :) What does it
monitor? PHYs port does not include east-west traffic, exposing just the PHYs
stats seems like a half measure.

Containerized monitors are in use by running in monitor in same net ns of the PF having full access to the PF.
The monitor is interested in physical port counters related to link transitions, link errors, buffer overruns etc.

quoted

quoted

quoted

For sure we prefer the bona fide Linux uAPI for standard features.
But internal knobs of how to do steering etc, is something not
generic enough. May be only those quirks live in the port function
params and rest in standard uAPIs?

Something talks to that steering API, and it's not netdev. So please
don't push problems which are not ours onto us.

Not sure I follow you.
Netdev of a mlx5 function talks to the driver internal steering API in
addition to other drivers operating this mlx5 function.

But there is no such thing as "steering API" in netdev. We can expose the
functionality we do have, if say PTP requires some steering then enabling PTP
implies the required steering is enabled. "steering API"
as an entity is meaningless to a netdev user.

It is the internal mlx5 implementation of how to do steering, triggered by netdev ndo's and other devices callback.
There are multiple options on how steering is done.
Such as sw_steering or dev managed steering.
There is already a control knob to choose sw vs dev steering as devlink param on the PF at [1].
This [1] device specific param is only limited to PF. For VFs, HV need to enable/disable this capability on selected VF.
API wise nothing drastic is getting added here, it's only on different object. (instead of device, it is port function).

[1] https://www.kernel.org/doc/html/v5.8/networking/device_drivers/mellanox/mlx5.html#devlink-parameters

On Wed, 12 Jan 2022 04:40:01 +0000 Parav Pandit wrote:

quoted

quoted

It's the capability that is turned on/off.
A device is composed based on what is needed. ipsec offload is not always needed.  
Its counter intuitive to expose some low level hardware resource to disable ipsec indirectly.  
So it is better to do as capability/param rather than some resource.
It is capability is more than just resource.  

Wouldn't there be some limitation on the number of SAs or max throughput or
such to limit on VF hogging the entire crypto path?

The fairness among VFs is present via the QoS knobs. Hence it doesn't hogg the entire crypto path.

Why do you want to disable it, then?

quoted

I was expecting such a knob, and then turning it to 0 would effectively remove
the capability (FW can completely hide it or driver ignore it).

quoted

May be. But it is in use at [1] for a long time now.

[1] docker run -p 9090:9090 prom/prometheus  

How is it "in use" if we haven't merged the patch to enable it? :) What does it
monitor? PHYs port does not include east-west traffic, exposing just the PHYs
stats seems like a half measure.

Containerized monitors are in use by running in monitor in same net ns of the PF having full access to the PF.
The monitor is interested in physical port counters related to link transitions, link errors, buffer overruns etc.

I don't think we should support this use case. VFs and PFs are not
the same thing.

quoted

quoted

Not sure I follow you.
Netdev of a mlx5 function talks to the driver internal steering API in
addition to other drivers operating this mlx5 function.  

But there is no such thing as "steering API" in netdev. We can expose the
functionality we do have, if say PTP requires some steering then enabling PTP
implies the required steering is enabled. "steering API"
as an entity is meaningless to a netdev user.  

It is the internal mlx5 implementation of how to do steering, triggered by netdev ndo's and other devices callback.
There are multiple options on how steering is done.
Such as sw_steering or dev managed steering.
There is already a control knob to choose sw vs dev steering as devlink param on the PF at [1].
This [1] device specific param is only limited to PF. For VFs, HV need to enable/disable this capability on selected VF.
API wise nothing drastic is getting added here, it's only on different object. (instead of device, it is port function).

[1] https://www.kernel.org/doc/html/v5.8/networking/device_drivers/mellanox/mlx5.html#devlink-parameters

Ah, that thing. IIRC this was added for TC offloads, VFs don't own 
the eswitch so what rules are they inserting to require "high insertion
rate"? My suspicion is that since it's not TC it'd be mostly for the
"DR" feature you have hence my comment on it not being netdev.

From: Jakub Kicinski <kuba@kernel.org>
Sent: Thursday, January 13, 2022 6:06 AM

On Wed, 12 Jan 2022 04:40:01 +0000 Parav Pandit wrote:

quoted

quoted

quoted

It's the capability that is turned on/off.
A device is composed based on what is needed. ipsec offload is not

always needed.

quoted

quoted

quoted

Its counter intuitive to expose some low level hardware resource to

disable ipsec indirectly.

quoted

quoted

quoted

So it is better to do as capability/param rather than some resource.
It is capability is more than just resource.

Wouldn't there be some limitation on the number of SAs or max
throughput or such to limit on VF hogging the entire crypto path?

The fairness among VFs is present via the QoS knobs. Hence it doesn't hogg

the entire crypto path.

Why do you want to disable it, then?

Each enabled feature consumes 
(a) driver level memory resource such as querying ip sec capabilities and more later,
(b) time in querying those capabilities, 
(c) device level initialization in supporting this capability

So for light weight devices which doesn't need it we want to keep it disabled.

quoted

quoted

I was expecting such a knob, and then turning it to 0 would
effectively remove the capability (FW can completely hide it or driver

ignore it).

quoted

quoted

quoted

May be. But it is in use at [1] for a long time now.

[1] docker run -p 9090:9090 prom/prometheus

How is it "in use" if we haven't merged the patch to enable it? :)
What does it monitor? PHYs port does not include east-west traffic,
exposing just the PHYs stats seems like a half measure.

Containerized monitors are in use by running in monitor in same net ns of

the PF having full access to the PF.

quoted

The monitor is interested in physical port counters related to link transitions,

link errors, buffer overruns etc.

I don't think we should support this use case. VFs and PFs are not the same
thing.

quoted

quoted

quoted

Not sure I follow you.
Netdev of a mlx5 function talks to the driver internal steering
API in addition to other drivers operating this mlx5 function.

But there is no such thing as "steering API" in netdev. We can
expose the functionality we do have, if say PTP requires some
steering then enabling PTP implies the required steering is enabled.

"steering API"

quoted

quoted

as an entity is meaningless to a netdev user.

It is the internal mlx5 implementation of how to do steering, triggered by

netdev ndo's and other devices callback.

quoted

There are multiple options on how steering is done.
Such as sw_steering or dev managed steering.
There is already a control knob to choose sw vs dev steering as devlink

param on the PF at [1].

quoted

This [1] device specific param is only limited to PF. For VFs, HV need to

enable/disable this capability on selected VF.

quoted

API wise nothing drastic is getting added here, it's only on different object.

(instead of device, it is port function).

quoted

[1]
https://www.kernel.org/doc/html/v5.8/networking/device_drivers/mellano
x/mlx5.html#devlink-parameters

Ah, that thing. IIRC this was added for TC offloads, VFs don't own the eswitch
so what rules are they inserting to require "high insertion rate"? My suspicion
is that since it's not TC it'd be mostly for the "DR" feature you have hence my
comment on it not being netdev.

No it is limited to tc offloads.
A VF netdev inserts flow steering rss rules on nic rx table.
This also uses the same smfs/dmfs when a VF is capable to do so.

On Thu, 13 Jan 2022 03:37:47 +0000 Parav Pandit wrote:

quoted

quoted

The fairness among VFs is present via the QoS knobs. Hence it doesn't hogg  

the entire crypto path.

Could you please fix your email client? It's incorrectly wrapping the
quotes and at the same time not wrapping your replies at all. :( What
client is this? 

quoted

Why do you want to disable it, then?  

Each enabled feature consumes 
(a) driver level memory resource such as querying ip sec capabilities and more later,
(b) time in querying those capabilities, 

These are on the VM's side, it's not hypervisors responsibility to help
the client by stripping features.

(c) device level initialization in supporting this capability

So for light weight devices which doesn't need it we want to keep it disabled.

You need to explain this better. We are pretty far from "trust"
settings, which are about privilege and not breaking isolation.

"device level initialization" tells me nothing.

quoted

quoted

It is the internal mlx5 implementation of how to do steering, triggered by  

netdev ndo's and other devices callback.  

quoted

There are multiple options on how steering is done.
Such as sw_steering or dev managed steering.
There is already a control knob to choose sw vs dev steering as devlink  

param on the PF at [1].  

quoted

This [1] device specific param is only limited to PF. For VFs, HV need to  

enable/disable this capability on selected VF.  

quoted

API wise nothing drastic is getting added here, it's only on different object.  

(instead of device, it is port function).  

quoted

[1]
https://www.kernel.org/doc/html/v5.8/networking/device_drivers/mellano
x/mlx5.html#devlink-parameters  

Ah, that thing. IIRC this was added for TC offloads, VFs don't own the eswitch
so what rules are they inserting to require "high insertion rate"? My suspicion
is that since it's not TC it'd be mostly for the "DR" feature you have hence my
comment on it not being netdev.  

No it is limited to tc offloads.
A VF netdev inserts flow steering rss rules on nic rx table.
This also uses the same smfs/dmfs when a VF is capable to do so.

Given the above are you concerned about privilege or also just
resources use here? Do VFs have SMFS today?

From: Jakub Kicinski <kuba@kernel.org>
Sent: Friday, January 14, 2022 10:12 AM

On Thu, 13 Jan 2022 03:37:47 +0000 Parav Pandit wrote:

quoted

quoted

quoted

The fairness among VFs is present via the QoS knobs. Hence it
doesn't hogg

the entire crypto path.

Could you please fix your email client? It's incorrectly wrapping the quotes and
at the same time not wrapping your replies at all. :( What client is this?

I will fix the client.
 

quoted

quoted

Why do you want to disable it, then?

Each enabled feature consumes
(a) driver level memory resource such as querying ip sec capabilities
and more later,
(b) time in querying those capabilities,

These are on the VM's side, it's not hypervisors responsibility to help the client
by stripping features.

HV is composing the device before giving it to the VM.
VM can always disable certain feature if it doesn't want to use by ethtool or other means.
But here we are discussing about offering/not offering the feature to the VF from HV.
HV can choose to not offer certain features based on some instruction received from orchestration.

quoted

(c) device level initialization in supporting this capability

So for light weight devices which doesn't need it we want to keep it disabled.

You need to explain this better. We are pretty far from "trust"
settings, which are about privilege and not breaking isolation.

We split the abstract trust to more granular settings, some related to privilege and some to capabilities.
 

"device level initialization" tells me nothing.

Above one belongs to capabilities bucket. Sw_steering belongs to trust bucket.
 

quoted

quoted

quoted

It is the internal mlx5 implementation of how to do steering,
triggered by

netdev ndo's and other devices callback.

quoted

There are multiple options on how steering is done.
Such as sw_steering or dev managed steering.
There is already a control knob to choose sw vs dev steering as
devlink

param on the PF at [1].

quoted

This [1] device specific param is only limited to PF. For VFs, HV
need to

enable/disable this capability on selected VF.

quoted

API wise nothing drastic is getting added here, it's only on different

object.

quoted

quoted

(instead of device, it is port function).

quoted

[1]
https://www.kernel.org/doc/html/v5.8/networking/device_drivers/mel
lano
x/mlx5.html#devlink-parameters

Ah, that thing. IIRC this was added for TC offloads, VFs don't own
the eswitch so what rules are they inserting to require "high
insertion rate"? My suspicion is that since it's not TC it'd be
mostly for the "DR" feature you have hence my comment on it not being

netdev.

quoted

No it is limited to tc offloads.
A VF netdev inserts flow steering rss rules on nic rx table.
This also uses the same smfs/dmfs when a VF is capable to do so.

Given the above are you concerned about privilege or also just resources use
here? Do VFs have SMFS today?

Privilege.
VFs have SMFS today, but by default it is disabled. The proposed knob will enable it.

Tue, Jan 11, 2022 at 07:20:05PM CET, kuba@kernel.org wrote:

On Tue, 11 Jan 2022 16:57:54 +0000 Parav Pandit wrote:

quoted

quoted

quoted

What shortcomings do you see in the finer granular approach we want to
go to enable/disable On a per feature basis instead of global knob?  

I was replying to Saeed so I assumed some context which you probably lack.
Granular approach is indeed better, what I was referring to when I said "prefer
an API as created by this patch" was having an dedicated devlink op, instead of
the use of devlink params.  

This discussed got paused in yet another year-end holidays. :)
Resuming now and refreshing everyone's cache.

We need to set/clear the capabilities of the function before deploying such function.
As you suggested we discussed the granular approach and at present we have following features to on/off.

Generic features:
1. ipsec offload

Why is ipsec offload a trusted feature?

quoted

2. ptp device

Makes sense.

quoted

Device specific:
1. sw steering

No idea what that is/entails.

quoted

2. physical port counters query

Still don't know why VF needs to know phy counters.

quoted

It was implicit that a driver API callback addition for both types of features is not good.
Devlink port function params enables to achieve both generic and device specific features.
Shall we proceed with port function params? What do you think?

I already addressed this. I don't like devlink params. They muddy the
water between vendor specific gunk and bona fide Linux uAPI. Build a
normal dedicated API.

Well, that is indeed true. But on the other hand, what is the alternative
solution? There are still going to be things wich are generic and driver-
specific. Params or no params. Or do you say we need some new well
defined enum-based api for generic stuff and driver-speficic will just
go to params?

On Fri, 14 Jan 2022 10:15:49 +0100 Jiri Pirko wrote:

quoted

quoted

It was implicit that a driver API callback addition for both types of features is not good.
Devlink port function params enables to achieve both generic and device specific features.
Shall we proceed with port function params? What do you think?

I already addressed this. I don't like devlink params. They muddy the
water between vendor specific gunk and bona fide Linux uAPI. Build a
normal dedicated API.

Well, that is indeed true. But on the other hand, what is the alternative
solution? There are still going to be things wich are generic and driver-
specific. Params or no params. Or do you say we need some new well
defined enum-based api for generic stuff and driver-speficic will just
go to params?

The latter is where my thinking is right now. I think devlink params
are attracting too much vendor attention, when they should really be
more of control for quirks.

On Fri, 14 Jan 2022 04:52:24 +0000 Parav Pandit wrote:

quoted

quoted

Each enabled feature consumes
(a) driver level memory resource such as querying ip sec capabilities and more later,
(b) time in querying those capabilities,  

These are on the VM's side, it's not hypervisors responsibility to help the client
by stripping features.
  

HV is composing the device before giving it to the VM.
VM can always disable certain feature if it doesn't want to use by ethtool or other means.
But here we are discussing about offering/not offering the feature to the VF from HV.
HV can choose to not offer certain features based on some instruction received from orchestration.

I'm still missing why go thru orchestration and HV rather than making
the driver load more clever to avoid wasting time on initializing
unnecessary caps.

quoted

quoted

(c) device level initialization in supporting this capability

So for light weight devices which doesn't need it we want to keep it disabled.  

You need to explain this better. We are pretty far from "trust"
settings, which are about privilege and not breaking isolation.

We split the abstract trust to more granular settings, some related to privilege and some to capabilities.
 

quoted

"device level initialization" tells me nothing.
 

Above one belongs to capabilities bucket. Sw_steering belongs to trust bucket.

quoted

quoted

No it is limited to tc offloads.
A VF netdev inserts flow steering rss rules on nic rx table.
This also uses the same smfs/dmfs when a VF is capable to do so.  

Given the above are you concerned about privilege or also just resources use
here? Do VFs have SMFS today?  

Privilege.
VFs have SMFS today, but by default it is disabled. The proposed knob will enable it.

Could you rephrase? What does it mean that VFs have SMFS but it's
disabled? Again - privilege means security, I'd think that it can't have
security implications if you're freely admitting that it's exposed.

On 14 Jan 18:34, Jakub Kicinski wrote:

On Fri, 14 Jan 2022 04:52:24 +0000 Parav Pandit wrote:

quoted

quoted

quoted

Each enabled feature consumes
(a) driver level memory resource such as querying ip sec capabilities and more later,
(b) time in querying those capabilities,

These are on the VM's side, it's not hypervisors responsibility to help the client
by stripping features.

HV is composing the device before giving it to the VM.
VM can always disable certain feature if it doesn't want to use by ethtool or other means.
But here we are discussing about offering/not offering the feature to the VF from HV.
HV can choose to not offer certain features based on some instruction received from orchestration.

I'm still missing why go thru orchestration and HV rather than making
the driver load more clever to avoid wasting time on initializing
unnecessary caps.

unfortunately for "smartnics" of this era, many of these initilizations
and resources are only manged by FW and the details are hidden away from
drivers, we need the knobs to tell the FW, hey we don't need all of these
features for this particular vf, save the resources for something else.
After all VF users need only a small portion of all the features we offer
to them, but again unfortunately the FW pre-allocates precious HW
resources to allow such features per VFs.

I know in this case smartnic === dumb FW, and sometimes there is no way
around it, this is the hw arch we have currently, not everything is a
nice generic flexible resources, not when it has to be wrapped with FW
"__awesome__" logic ;), and for proper virtualization we need this FW.

But i totally agree with your point, when we can limit with resources, we
should limit with resources, otherwise we need a knob to communicate to FW
what is the user intention for this VF.

quoted

quoted

quoted

(c) device level initialization in supporting this capability

So for light weight devices which doesn't need it we want to keep it disabled.

You need to explain this better. We are pretty far from "trust"
settings, which are about privilege and not breaking isolation.

We split the abstract trust to more granular settings, some related to privilege and some to capabilities.

quoted

"device level initialization" tells me nothing.

Above one belongs to capabilities bucket. Sw_steering belongs to trust bucket.

quoted

quoted

No it is limited to tc offloads.
A VF netdev inserts flow steering rss rules on nic rx table.
This also uses the same smfs/dmfs when a VF is capable to do so.

Given the above are you concerned about privilege or also just resources use
here? Do VFs have SMFS today?

Privilege.
VFs have SMFS today, but by default it is disabled. The proposed knob will enable it.

Could you rephrase? What does it mean that VFs have SMFS but it's
disabled? Again - privilege means security, I'd think that it can't have
security implications if you're freely admitting that it's exposed.

I think the term privilege is misused here, due to the global knob proposed
initially. Anyway the issue is exactly as I explained above, SW steering requires
FW pre-allocated resources and initializations, for VFs it is disabled
since there was no demand for it and FW wanted to save on resources.

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule insertion rate
for use cases other than switchdev and TC, e.g TLS, connection tracking,
etc .. 

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:

On 14 Jan 18:34, Jakub Kicinski wrote:

quoted

quoted

HV is composing the device before giving it to the VM.
VM can always disable certain feature if it doesn't want to use by ethtool or other means.
But here we are discussing about offering/not offering the feature to the VF from HV.
HV can choose to not offer certain features based on some instruction received from orchestration.  

I'm still missing why go thru orchestration and HV rather than making
the driver load more clever to avoid wasting time on initializing
unnecessary caps.  

unfortunately for "smartnics" of this era, many of these initilizations
and resources are only manged by FW and the details are hidden away from
drivers, we need the knobs to tell the FW, hey we don't need all of these
features for this particular vf, save the resources for something else.
After all VF users need only a small portion of all the features we offer
to them, but again unfortunately the FW pre-allocates precious HW
resources to allow such features per VFs.

I know in this case smartnic === dumb FW, and sometimes there is no way
around it, this is the hw arch we have currently, not everything is a
nice generic flexible resources, not when it has to be wrapped with FW
"__awesome__" logic ;), and for proper virtualization we need this FW.

But i totally agree with your point, when we can limit with resources, we
should limit with resources, otherwise we need a knob to communicate to FW
what is the user intention for this VF.

quoted

quoted

Privilege.
VFs have SMFS today, but by default it is disabled. The proposed knob will enable it.  

Could you rephrase? What does it mean that VFs have SMFS but it's
disabled? Again - privilege means security, I'd think that it can't have
security implications if you're freely admitting that it's exposed.  

I think the term privilege is misused here, due to the global knob proposed
initially. Anyway the issue is exactly as I explained above, SW steering requires
FW pre-allocated resources and initializations, for VFs it is disabled
since there was no demand for it and FW wanted to save on resources.

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule insertion rate
for use cases other than switchdev and TC, e.g TLS, connection tracking,
etc .. 

Sorry long weekend here, thanks for the explanation!

Where do we stand? Are you okay with an explicit API for enabling /
disabling VF features? If SMFS really is about conntrack and TLS maybe
it can be implied by the delegation of appropriate bits meaningful to
netdev world?

On 18 Jan 10:02, Jakub Kicinski wrote:

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:

quoted

On 14 Jan 18:34, Jakub Kicinski wrote:

quoted

quoted

HV is composing the device before giving it to the VM.
VM can always disable certain feature if it doesn't want to use by ethtool or other means.
But here we are discussing about offering/not offering the feature to the VF from HV.
HV can choose to not offer certain features based on some instruction received from orchestration.

I'm still missing why go thru orchestration and HV rather than making
the driver load more clever to avoid wasting time on initializing
unnecessary caps.

unfortunately for "smartnics" of this era, many of these initilizations
and resources are only manged by FW and the details are hidden away from
drivers, we need the knobs to tell the FW, hey we don't need all of these
features for this particular vf, save the resources for something else.
After all VF users need only a small portion of all the features we offer
to them, but again unfortunately the FW pre-allocates precious HW
resources to allow such features per VFs.

I know in this case smartnic === dumb FW, and sometimes there is no way
around it, this is the hw arch we have currently, not everything is a
nice generic flexible resources, not when it has to be wrapped with FW
"__awesome__" logic ;), and for proper virtualization we need this FW.

But i totally agree with your point, when we can limit with resources, we
should limit with resources, otherwise we need a knob to communicate to FW
what is the user intention for this VF.

quoted

quoted

Privilege.
VFs have SMFS today, but by default it is disabled. The proposed knob will enable it.

Could you rephrase? What does it mean that VFs have SMFS but it's
disabled? Again - privilege means security, I'd think that it can't have
security implications if you're freely admitting that it's exposed.

I think the term privilege is misused here, due to the global knob proposed
initially. Anyway the issue is exactly as I explained above, SW steering requires
FW pre-allocated resources and initializations, for VFs it is disabled
since there was no demand for it and FW wanted to save on resources.

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule insertion rate
for use cases other than switchdev and TC, e.g TLS, connection tracking,
etc ..

Sorry long weekend here, thanks for the explanation!

Where do we stand? Are you okay with an explicit API for enabling /
disabling VF features? If SMFS really is about conntrack and TLS maybe

I am as skeptical as you are. But what other options do we have ? It's a
fact that "Smart" VFs have different use-cases and customization is
necessary to allow full scalability and better system resource
utilization.

As you already said, PTP for instance makes total sense as a VF feature
knob, for the same reason I would say any standard stateful
feature/offloads (e.g Crypto) also deserve own knobs.

If we agree on the need for a VF customization API, I would use one API
for all features. Having explicit enable/disable API for some then implicit
resources re-size API for other features is a bit confusing.

e.g.

# Enable ptp on specific vf
devlink port function <port idx> set feature PTP ON/OFF

# disable TLS on specific vf
devlink resource set <DEV> TLS size 0

And I am pretty sure resource API is not yet available for port functions (e.g
before VF instantiation, which is one of the main points of this RFC, so some
plumbing is necessary to expose resource API for port functions.

TBH, I actually like your resources idea, i would
like to explore that more with Parav, see what we can do about it .. 

it can be implied by the delegation of appropriate bits meaningful to
netdev world?

I don't get this point, netdev bits are known only after the VF has been fully
initialized.

And sometimes users want TLS without the optimization of SMFS, so as a vendor
driver maintainer i would prefer having control knobs per feature, instead of
maintaining some weird driver feature discovery and brokerage logic..

On Tue, 18 Jan 2022 14:33:28 -0800 Saeed Mahameed wrote:

On 18 Jan 10:02, Jakub Kicinski wrote:

quoted

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:  

quoted

I think the term privilege is misused here, due to the global knob proposed
initially. Anyway the issue is exactly as I explained above, SW steering requires
FW pre-allocated resources and initializations, for VFs it is disabled
since there was no demand for it and FW wanted to save on resources.

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule insertion rate
for use cases other than switchdev and TC, e.g TLS, connection tracking,
etc ..  

Sorry long weekend here, thanks for the explanation!

Where do we stand? Are you okay with an explicit API for enabling /
disabling VF features? If SMFS really is about conntrack and TLS maybe  

I am as skeptical as you are. But what other options do we have ? It's a
fact that "Smart" VFs have different use-cases and customization is
necessary to allow full scalability and better system resource
utilization.

As you already said, PTP for instance makes total sense as a VF feature
knob

To be clear when I was talking about PTP initially I was thinking
about real PTP clocks. "Modern" NICs sometimes do shenanigans in 
the FW to pretend they have more clocks that they really have.
There is a difference between delegating the PHC to the VF and
allowing the VF to use some SW pretend clock. I'm not sure which
camp your PTP falls into.

for the same reason I would say any standard stateful
feature/offloads (e.g Crypto) also deserve own knobs.

If we agree on the need for a VF customization API, I would use one API
for all features. Having explicit enable/disable API for some then implicit
resources re-size API for other features is a bit confusing.

e.g.

# Enable ptp on specific vf
devlink port function <port idx> set feature PTP ON/OFF

# disable TLS on specific vf
devlink resource set <DEV> TLS size 0

And I am pretty sure resource API is not yet available for port functions (e.g
before VF instantiation, which is one of the main points of this RFC, so some
plumbing is necessary to expose resource API for port functions.

TBH, I actually like your resources idea, i would
like to explore that more with Parav, see what we can do about it .. 

Right, that'd be great, although I'd imagine if the resource is very
flexible (e.g. memory) delegating N bytes to a function does not tell
the device how to perform the "diet". Obviously that's pure speculation
I don't know how things work on your SmartNIC :)

quoted

it can be implied by the delegation of appropriate bits meaningful to
netdev world?  

I don't get this point, netdev bits are known only after the VF has been fully
initialized.

I meant this as a simple starting point to enumerate the features.
It was an off-cuff suggestion, really. Reusing some approximation of
existing bits with clear code-driven semantics is simpler than defining
and documenting new ones.

We can start a new enum.

I hope you didn't mean "PTP" to be a string carried all the way to 
the driver in your example command?

And sometimes users want TLS without the optimization of SMFS, so as a vendor
driver maintainer i would prefer having control knobs per feature, instead of
maintaining some weird driver feature discovery and brokerage logic..

Shifting the "weird feature discovery" onto the user really does not
solve the problem. Enable SMFS is not a meaningful knob, the devops
engineer setting up the infra will have to guess. If we want something
like SMFS to be directly controlled it should be a clearly vendor
specific knob, IMO.

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 19, 2022 5:46 AM

On Tue, 18 Jan 2022 14:33:28 -0800 Saeed Mahameed wrote:

quoted

On 18 Jan 10:02, Jakub Kicinski wrote:

quoted

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:

quoted

I think the term privilege is misused here, due to the global knob
proposed initially. Anyway the issue is exactly as I explained
above, SW steering requires FW pre-allocated resources and
initializations, for VFs it is disabled since there was no demand for it and

FW wanted to save on resources.

quoted

quoted

quoted

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule
insertion rate for use cases other than switchdev and TC, e.g TLS,
connection tracking, etc ..

Sorry long weekend here, thanks for the explanation!

Where do we stand? Are you okay with an explicit API for enabling /
disabling VF features? If SMFS really is about conntrack and TLS
maybe

I am as skeptical as you are. But what other options do we have ? It's
a fact that "Smart" VFs have different use-cases and customization is
necessary to allow full scalability and better system resource
utilization.

As you already said, PTP for instance makes total sense as a VF
feature knob

To be clear when I was talking about PTP initially I was thinking about real PTP
clocks. "Modern" NICs sometimes do shenanigans in the FW to pretend they
have more clocks that they really have.
There is a difference between delegating the PHC to the VF and allowing the
VF to use some SW pretend clock. I'm not sure which camp your PTP falls into.

quoted

for the same reason I would say any standard stateful feature/offloads
(e.g Crypto) also deserve own knobs.

If we agree on the need for a VF customization API, I would use one
API for all features. Having explicit enable/disable API for some then
implicit resources re-size API for other features is a bit confusing.

e.g.

# Enable ptp on specific vf
devlink port function <port idx> set feature PTP ON/OFF

# disable TLS on specific vf
devlink resource set <DEV> TLS size 0

And I am pretty sure resource API is not yet available for port
functions (e.g before VF instantiation, which is one of the main
points of this RFC, so some plumbing is necessary to expose resource API for

port functions.

quoted

TBH, I actually like your resources idea, i would like to explore that
more with Parav, see what we can do about it ..

Right, that'd be great, although I'd imagine if the resource is very flexible (e.g.
memory) delegating N bytes to a function does not tell the device how to
perform the "diet". Obviously that's pure speculation I don't know how things
work on your SmartNIC :)

Right, we at least need to tell fw that only X bytes are allowed for sw_steering diet.
And _right_ amount of X bytes specific for sw_steering was not very clear.
Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port function resource is more suitable here even though its bool.
 

quoted

quoted

it can be implied by the delegation of appropriate bits meaningful to
netdev world?

I don't get this point, netdev bits are known only after the VF has
been fully initialized.

I meant this as a simple starting point to enumerate the features.
It was an off-cuff suggestion, really. Reusing some approximation of existing
bits with clear code-driven semantics is simpler than defining and
documenting new ones.

We can start a new enum.

I hope you didn't mean "PTP" to be a string carried all the way to the driver in
your example command?

Yet to sync with Saeed, but I think it will be a enum + string during resource registration time.
For generic features, enum and string are defined by devlink core.
For smfs kind of rare knob, enum and string is supplied by driver.

On 19 Jan 05:49, Parav Pandit wrote:

quoted

From: Jakub Kicinski <kuba@kernel.org>
Sent: Wednesday, January 19, 2022 5:46 AM

On Tue, 18 Jan 2022 14:33:28 -0800 Saeed Mahameed wrote:

quoted

On 18 Jan 10:02, Jakub Kicinski wrote:

quoted

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:

quoted

I think the term privilege is misused here, due to the global knob
proposed initially. Anyway the issue is exactly as I explained
above, SW steering requires FW pre-allocated resources and
initializations, for VFs it is disabled since there was no demand for it and

FW wanted to save on resources.

quoted

quoted

quoted

Now as SW steering is catching up with FW steering in terms of
functionality, people want it also on VFs to help with rule
insertion rate for use cases other than switchdev and TC, e.g TLS,
connection tracking, etc ..

Sorry long weekend here, thanks for the explanation!

Where do we stand? Are you okay with an explicit API for enabling /
disabling VF features? If SMFS really is about conntrack and TLS
maybe

I am as skeptical as you are. But what other options do we have ? It's
a fact that "Smart" VFs have different use-cases and customization is
necessary to allow full scalability and better system resource
utilization.

As you already said, PTP for instance makes total sense as a VF
feature knob

To be clear when I was talking about PTP initially I was thinking about real PTP
clocks. "Modern" NICs sometimes do shenanigans in the FW to pretend they
have more clocks that they really have.
There is a difference between delegating the PHC to the VF and allowing the
VF to use some SW pretend clock. I'm not sure which camp your PTP falls into.

delegating.

quoted

quoted

for the same reason I would say any standard stateful feature/offloads
(e.g Crypto) also deserve own knobs.

If we agree on the need for a VF customization API, I would use one
API for all features. Having explicit enable/disable API for some then
implicit resources re-size API for other features is a bit confusing.

e.g.

# Enable ptp on specific vf
devlink port function <port idx> set feature PTP ON/OFF

# disable TLS on specific vf
devlink resource set <DEV> TLS size 0

And I am pretty sure resource API is not yet available for port
functions (e.g before VF instantiation, which is one of the main
points of this RFC, so some plumbing is necessary to expose resource API for

port functions.

quoted

TBH, I actually like your resources idea, i would like to explore that
more with Parav, see what we can do about it ..

Right, that'd be great, although I'd imagine if the resource is very flexible (e.g.
memory) delegating N bytes to a function does not tell the device how to
perform the "diet". Obviously that's pure speculation I don't know how things
work on your SmartNIC :)

Right, we at least need to tell fw that only X bytes are allowed for sw_steering diet.
And _right_ amount of X bytes specific for sw_steering was not very clear.
Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port function resource is more suitable here even though its bool.

I believe flexibility can be achieved with some FW message? Parav can you
investigate ? To be clear here the knob must be specific to sw_steering
exposed as memory resource.

quoted

quoted

quoted

it can be implied by the delegation of appropriate bits meaningful to
netdev world?

I don't get this point, netdev bits are known only after the VF has
been fully initialized.

I meant this as a simple starting point to enumerate the features.
It was an off-cuff suggestion, really. Reusing some approximation of existing
bits with clear code-driven semantics is simpler than defining and
documenting new ones.

doable, although can be confusing. 

quoted

We can start a new enum.

I hope you didn't mean "PTP" to be a string carried all the way to the driver in
your example command?

No :), well defined enums, similar to devlink params. but yes we need a
clear cut of what is vendor specific and what's not.

Yet to sync with Saeed, but I think it will be a enum + string during resource registration time.
For generic features, enum and string are defined by devlink core.
For smfs kind of rare knob, enum and string is supplied by driver.

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

[..]

quoted

I do agree you and Saeed that instead of port function param, port function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can you
investigate ? To be clear here the knob must be specific to sw_steering
exposed as memory resource.

Sure.
I currently think of user interface something like below,
I will get back with more plumbing of netlink and enum/string.

# to enable
devlink port function resource set pci/0000:03:00.0/port_index device_memory/sw_steering 1

# to disable 
devlink port function resource set pci/0000:03:00.0/port_index device_memory/sw_steering 0 (current default)

Thanks Jakub, Saeed for the inputs and direction.

On 20 Jan 04:52, Parav Pandit wrote:

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

[..]

quoted

quoted

I do agree you and Saeed that instead of port function param, port function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can you
investigate ? To be clear here the knob must be specific to sw_steering
exposed as memory resource.

Sure.
I currently think of user interface something like below,
I will get back with more plumbing of netlink and enum/string.

# to enable
devlink port function resource set pci/0000:03:00.0/port_index device_memory/sw_steering 1

this looks like an abuse of the interface, I literally meant to control the
amount of ICM pages dedicated for SW steering per function, this requires
some FW support, but i think this is the correct direction.

# to disable
devlink port function resource set pci/0000:03:00.0/port_index device_memory/sw_steering 0 (current default)

Thanks Jakub, Saeed for the inputs and direction.

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 11:34 AM

On 20 Jan 04:52, Parav Pandit wrote:

quoted

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

[..]

quoted

quoted

I do agree you and Saeed that instead of port function param, port
function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can
you investigate ? To be clear here the knob must be specific to
sw_steering exposed as memory resource.

Sure.
I currently think of user interface something like below, I will get
back with more plumbing of netlink and enum/string.

# to enable
devlink port function resource set pci/0000:03:00.0/port_index
device_memory/sw_steering 1

this looks like an abuse of the interface, I literally meant to control the amount
of ICM pages dedicated for SW steering per function, this requires some FW
support, but i think this is the correct direction.

Ok. I will evaluate and update.

quoted

# to disable
devlink port function resource set pci/0000:03:00.0/port_index
device_memory/sw_steering 0 (current default)

Thanks Jakub, Saeed for the inputs and direction.

Hi Jakub, Saeed,

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

quoted

And _right_ amount of X bytes specific for sw_steering was not very clear.
Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can you
investigate ? To be clear here the knob must be specific to sw_steering
exposed as memory resource.

I investigated this further with hw and fw teams.
The memory resource allocator doesn't understand the resource type for page allocation.
And even if somehow it is extended, when the pages are freed, they are returned to the common pool cache instead of returning immediately to the driver. We will miss the efficiency gained with the caching and reusing these pages for other functions and for other resource types too.
This cache efficiency is far more important for speed of resource allocation.

And additionally, it is after all boolean feature to enable/disable a functionality.
So I suggest, how about we do something like below?
It is similar to ethtool -k option, but applicable at the HV PF side to enable/disable a feature for the functions.

$ devlink port function feature set ptp/ipsec/tlsoffload on/off
$ devlink port function feature set device_specific_feature1 on/off

$ devlink port show
pci/0000:06:00.0/1: type eth netdev eth0 flavour pcivf pfnum 0 vfnum 0
  function:
    hw_addr 00:00:00:00:00:00
    feature:
      tlsoffload <on/off>
      ipsec <on/off>
      ptp <on/off>
      device_specific_feature1 <on/off>

This enables having well defined features per function and odd device specific feature.
It also doesn't overload the device on doing accounting pages for boolean functionality.
Does it look reasonable?

On 03 Feb 18:35, Parav Pandit wrote:

Hi Jakub, Saeed,

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

quoted

quoted

And _right_ amount of X bytes specific for sw_steering was not very clear.
Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can you
investigate ? To be clear here the knob must be specific to sw_steering
exposed as memory resource.

I investigated this further with hw and fw teams.
The memory resource allocator doesn't understand the resource type for page allocation.
And even if somehow it is extended, when the pages are freed, they are returned to the common pool cache instead of returning immediately to the driver. We will miss the efficiency gained with the caching and reusing these pages for other functions and for other resource types too.
This cache efficiency is far more important for speed of resource allocation.

And additionally, it is after all boolean feature to enable/disable a functionality.
So I suggest, how about we do something like below?
It is similar to ethtool -k option, but applicable at the HV PF side to enable/disable a feature for the functions.

$ devlink port function feature set ptp/ipsec/tlsoffload on/off
$ devlink port function feature set device_specific_feature1 on/off

$ devlink port show
pci/0000:06:00.0/1: type eth netdev eth0 flavour pcivf pfnum 0 vfnum 0
 function:
   hw_addr 00:00:00:00:00:00
   feature:
     tlsoffload <on/off>
     ipsec <on/off>
     ptp <on/off>
     device_specific_feature1 <on/off>

Given the HW limitation of differentiating between memory allocated for
different resources, and after a second though about the fact that most of
ConnectX resources are mapped to ICM memory which is managed by FW,
although it would've been very useful to manager resources this way,
such architecture is very specific to ConnectX and might not suite other
vendors, so explicit API as the above sounds like a better compromise, 
but I would put device_specific_feature(s) into a separate category/list

basically you are looking for:

1) ethtool -k equivalent for devlink
2) ethtool --show-priv-flags equivalent for devlink

I think that's reasonable.

This enables having well defined features per function and odd device specific feature.
It also doesn't overload the device on doing accounting pages for boolean functionality.
Does it look reasonable?

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Friday, February 4, 2022 12:47 AM

On 03 Feb 18:35, Parav Pandit wrote:

quoted

Hi Jakub, Saeed,

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

quoted

quoted

And _right_ amount of X bytes specific for sw_steering was not very clear.
Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port
function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav can
you investigate ? To be clear here the knob must be specific to
sw_steering exposed as memory resource.

I investigated this further with hw and fw teams.
The memory resource allocator doesn't understand the resource type for page

allocation.

quoted

And even if somehow it is extended, when the pages are freed, they are

returned to the common pool cache instead of returning immediately to the
driver. We will miss the efficiency gained with the caching and reusing these
pages for other functions and for other resource types too.

quoted

This cache efficiency is far more important for speed of resource allocation.

And additionally, it is after all boolean feature to enable/disable a

functionality.

quoted

So I suggest, how about we do something like below?
It is similar to ethtool -k option, but applicable at the HV PF side to

enable/disable a feature for the functions.

quoted

$ devlink port function feature set ptp/ipsec/tlsoffload on/off $
devlink port function feature set device_specific_feature1 on/off

$ devlink port show
pci/0000:06:00.0/1: type eth netdev eth0 flavour pcivf pfnum 0 vfnum 0
 function:
   hw_addr 00:00:00:00:00:00
   feature:
     tlsoffload <on/off>
     ipsec <on/off>
     ptp <on/off>
     device_specific_feature1 <on/off>

Given the HW limitation of differentiating between memory allocated for
different resources, and after a second though about the fact that most of
ConnectX resources are mapped to ICM memory which is managed by FW,
although it would've been very useful to manager resources this way, such
architecture is very specific to ConnectX and might not suite other vendors, so
explicit API as the above sounds like a better compromise, but I would put
device_specific_feature(s) into a separate category/list

basically you are looking for:

1) ethtool -k equivalent for devlink
2) ethtool --show-priv-flags equivalent for devlink

I think that's reasonable.

Right. I was thinking to put under single "feature" bucket like above.
Shall we proceed with this UAPI?

quoted

This enables having well defined features per function and odd device specific

feature.

quoted

It also doesn't overload the device on doing accounting pages for boolean

functionality.

quoted

Does it look reasonable?

Hi Jakub,

From: Parav Pandit
Sent: Monday, February 7, 2022 8:15 PM

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Friday, February 4, 2022 12:47 AM

On 03 Feb 18:35, Parav Pandit wrote:

quoted

Hi Jakub, Saeed,

quoted

From: Saeed Mahameed <saeedm@nvidia.com>
Sent: Thursday, January 20, 2022 6:11 AM

quoted

quoted

And _right_ amount of X bytes specific for sw_steering was not very

clear.

quoted

quoted

quoted

quoted

Hence the on/off resource knob looked more doable and abtract.

I do agree you and Saeed that instead of port function param, port
function

resource is more suitable here even though its bool.

quoted

I believe flexibility can be achieved with some FW message? Parav
can you investigate ? To be clear here the knob must be specific to
sw_steering exposed as memory resource.

I investigated this further with hw and fw teams.
The memory resource allocator doesn't understand the resource type
for page

allocation.

quoted

And even if somehow it is extended, when the pages are freed, they
are

returned to the common pool cache instead of returning immediately to
the driver. We will miss the efficiency gained with the caching and
reusing these pages for other functions and for other resource types too.

quoted

This cache efficiency is far more important for speed of resource allocation.

And additionally, it is after all boolean feature to enable/disable a

functionality.

quoted

So I suggest, how about we do something like below?
It is similar to ethtool -k option, but applicable at the HV PF side
to

enable/disable a feature for the functions.

quoted

$ devlink port function feature set ptp/ipsec/tlsoffload on/off $
devlink port function feature set device_specific_feature1 on/off

$ devlink port show
pci/0000:06:00.0/1: type eth netdev eth0 flavour pcivf pfnum 0 vfnum
0
 function:
   hw_addr 00:00:00:00:00:00
   feature:
     tlsoffload <on/off>
     ipsec <on/off>
     ptp <on/off>
     device_specific_feature1 <on/off>

Given the HW limitation of differentiating between memory allocated
for different resources, and after a second though about the fact that
most of ConnectX resources are mapped to ICM memory which is managed
by FW, although it would've been very useful to manager resources this
way, such architecture is very specific to ConnectX and might not
suite other vendors, so explicit API as the above sounds like a better
compromise, but I would put
device_specific_feature(s) into a separate category/list

basically you are looking for:

1) ethtool -k equivalent for devlink
2) ethtool --show-priv-flags equivalent for devlink

I think that's reasonable.

Right. I was thinking to put under single "feature" bucket like above.
Shall we proceed with this UAPI?

Can you please review above interface? We would like to enable users.