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

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

[Sunil Rani]

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

[PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Sunil Rani]

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 <sunrani@nvidia.com>
Signed-off-by: Bodong Wang <bodong@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 .../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

[PATCH net-next 2/2] net/mlx5: SF/VF, Port function trust set support

[Sunil Rani]

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 <sunrani@nvidia.com>
Signed-off-by: Bodong Wang <bodong@nvidia.com>
Reviewed-by: Parav Pandit <parav@nvidia.com>
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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Sunil Sudhakar Rani]

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Tue, 30 Nov 2021 22:17:29 +0000 Sunil Sudhakar Rani wrote:
> > 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  
>
> 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:
> 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:  
> > > 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On Thu, 2021-12-02 at 09:31 -0800, Jakub Kicinski wrote:
> On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:
> > 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:  
> > > > 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On Thu, 2021-12-02 at 09:31 -0800, Jakub Kicinski wrote:
> On Wed, 1 Dec 2021 07:07:05 +0000 Saeed Mahameed wrote:
> > 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:  
> > > > 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:
> 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.

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:
> On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:
> > 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.  
> 
> 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.

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Sunil Sudhakar Rani]

> On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:
> > On Wed, 2021-12-15 at 11:22 -0800, Jakub Kicinski wrote:
> > > 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.
> >
> > 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?

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Thu, 16 Dec 2021 16:17:29 +0000 Sunil Sudhakar Rani wrote:
> > On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:  
> > > 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.

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

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:
> > > On Wed, 15 Dec 2021 22:15:10 +0000 Saeed Mahameed wrote:
> > > > 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.
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?

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Tue, 11 Jan 2022 16:57:54 +0000 Parav Pandit wrote:
> > > 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.

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> 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:
> > > > 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?
>
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.
 
> > 2. ptp device
> 
> Makes sense.
> 
> > Device specific:
> > 1. sw steering
> 
> No idea what that is/entails.
> 
:) it the device specific knob.

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Tue, 11 Jan 2022 18:26:16 +0000 Parav Pandit wrote:
> > From: Jakub Kicinski <kuba@kernel.org>
> > Sent: Tuesday, January 11, 2022 11:50 PM
> > > 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.

> > > 2. ptp device
> > 
> > Makes sense.
> >   
> > > Device specific:
> > > 1. sw steering  
> > 
> > No idea what that is/entails.
> >   
> :) it the device specific knob.
> 
> > > 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.

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> 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:
> > > From: Jakub Kicinski <kuba@kernel.org>
> > > Sent: Tuesday, January 11, 2022 11:50 PM
> > > > 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.

> > > > 2. ptp device
> > >
> > > Makes sense.
> > >
> > > > Device specific:
> > > > 1. sw steering
> > >
> > > No idea what that is/entails.
> > >
> > :) it the device specific knob.
> >
> > > > 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
 
> > > > 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Tue, 11 Jan 2022 19:39:37 +0000 Parav Pandit wrote:
> > 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:  
> > > 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).



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

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> 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:
> > > 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:
> > > > 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?
>
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).
> 
> 
> 
> > > > 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.


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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Wed, 12 Jan 2022 04:40:01 +0000 Parav Pandit wrote:
> > > 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?

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

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> 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:
> > > > 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?
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.

> 
> > > 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).
> > >
> > > > 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.
> 
> > > > 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/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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Thu, 13 Jan 2022 03:37:47 +0000 Parav Pandit wrote:
> > > 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? 

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

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

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

> > (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.
 
> > > > 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/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.
> > 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jiri Pirko]

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

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?

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Fri, 14 Jan 2022 10:15:49 +0100 Jiri Pirko wrote:
>>> 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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Fri, 14 Jan 2022 04:52:24 +0000 Parav Pandit wrote:
> > > 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.

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On 14 Jan 18:34, Jakub Kicinski wrote:
>On Fri, 14 Jan 2022 04:52:24 +0000 Parav Pandit wrote:
>> > > 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.

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:
> On 14 Jan 18:34, Jakub Kicinski wrote:
> >> 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.
> 
> >> 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?

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On 18 Jan 10:02, Jakub Kicinski wrote:
>On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:
>> On 14 Jan 18:34, Jakub Kicinski wrote:
>> >> 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.
>>
>> >> 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..

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Jakub Kicinski]

On Tue, 18 Jan 2022 14:33:28 -0800 Saeed Mahameed wrote:
> On 18 Jan 10:02, Jakub Kicinski wrote:
> >On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:  
> >> 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 :)

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> 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:
> > On 18 Jan 10:02, Jakub Kicinski wrote:
> > >On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:
> > >> 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 :)
>
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.
 
> > >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.

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On 19 Jan 05:49, Parav Pandit wrote:
>
>> 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:
>> > On 18 Jan 10:02, Jakub Kicinski wrote:
>> > >On Fri, 14 Jan 2022 22:15:48 -0800 Saeed Mahameed wrote:
>> > >> 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.
>>

delegating.

>> > 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 :)
>>
>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.

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

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

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

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On 20 Jan 04:52, Parav Pandit wrote:
>
>> From: Saeed Mahameed <saeedm@nvidia.com>
>> Sent: Thursday, January 20, 2022 6:11 AM
>
>[..]
>> >
>> >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.
>>
>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.

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> From: Saeed Mahameed <saeedm@nvidia.com>
> Sent: Thursday, January 20, 2022 11:34 AM
> 
> On 20 Jan 04:52, Parav Pandit wrote:
> >
> >> From: Saeed Mahameed <saeedm@nvidia.com>
> >> Sent: Thursday, January 20, 2022 6:11 AM
> >
> >[..]
> >> >
> >> >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.
> >>
> >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.

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

Hi Jakub, Saeed,

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

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

Re: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Saeed Mahameed]

On 03 Feb 18:35, Parav Pandit wrote:
>Hi Jakub, Saeed,
>
>> From: Saeed Mahameed <saeedm@nvidia.com>
>> Sent: Thursday, January 20, 2022 6:11 AM
>
>> >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.
>>
>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?

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

> From: Saeed Mahameed <saeedm@nvidia.com>
> Sent: Friday, February 4, 2022 12:47 AM
> 
> On 03 Feb 18:35, Parav Pandit wrote:
> >Hi Jakub, Saeed,
> >
> >> From: Saeed Mahameed <saeedm@nvidia.com>
> >> Sent: Thursday, January 20, 2022 6:11 AM
> >
> >> >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.
> >>
> >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.
> 

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

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

RE: [PATCH net-next 1/2] devlink: Add support to set port function as trusted

[Parav Pandit]

Hi Jakub,

> From: Parav Pandit
> Sent: Monday, February 7, 2022 8:15 PM
> 
> > From: Saeed Mahameed <saeedm@nvidia.com>
> > Sent: Friday, February 4, 2022 12:47 AM
> >
> > On 03 Feb 18:35, Parav Pandit wrote:
> > >Hi Jakub, Saeed,
> > >
> > >> From: Saeed Mahameed <saeedm@nvidia.com>
> > >> Sent: Thursday, January 20, 2022 6:11 AM
> > >
> > >> >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.
> > >>
> > >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.
> >
> 
> 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.