[PATCH net-next 0/2] Add ethtool set regs support

[PATCH net-next 0/2] Add ethtool set regs support

[Saeed Mahameed]

Hi Dave,

This series adds the support for setting device registers from user
space ethtool.

Currently ethtool only allows to get device registers,
we extend ethtool functionality to also set device registers, by
introducing set_regs to ethtool_ops which will be invoked when
user space requests "ETHTOOL_SREGS", for example via ethtool user app:

ethtool -D DEVNAME [ file FILENAME ] is used to set registers in
the device using vendor specific binary registers data provided via
stdin/file. Changes made by this option can be queried using get
regs -d flag.

This simple ethool change will give HW vendors the flexibility to set
pure HW configurations (not directly related to netdev resources states
and rings), without the need of vendor proprietary tools and hacks.

2nd patch adds the support for ethtool set/get_regs in mlx5e driver.

Important Note: With this extension we will allow HW vendors to access (set) their 
device register without the need for them to open their format, hence the binary
file passed on ethtool -D DEVNAME.

This means that the device driver MUST check for correctness/validity of the 
registers data sent to it and whether this register is permitted to be iset form user space
in order to prevent the user from accessing/setting registers/Device configurations
that already standardized by the kernel/stack user APIs, or not allowed to be seen/set by user.

mlx5 driver have registers allowed access list and will check the user 
Request validity before forwarding it to HW registers. Mlx5 will allow only mlx5 specific
configurations to be set (e.g. Device Diag Counters for HW performance debugging and analysis)
which has no standard API to access it.

Comments and redirections are more than welcome

This series was generated against commit:
b0da4f743db5 ("net: calxeda: xgmac: use new api ethtool_{get|set}_link_ksettings")

Thanks,
Saeed.

Gal Pressman (2):
  ethtool: Add set regs -D option support
  net/mlx5e: Add ethtool get/set reg support

 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   2 +-
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c      |  19 ----
 drivers/net/ethernet/mellanox/mlx5/core/en.h       |  12 +++
 .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   |  21 ++++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |   8 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_regs.c  | 116 +++++++++++++++++++++
 include/linux/ethtool.h                            |   1 +
 include/linux/mlx5/mlx5_ifc.h                      |  22 ++++
 include/uapi/linux/ethtool.h                       |   1 +
 net/core/ethtool.c                                 |  31 ++++++
 10 files changed, 213 insertions(+), 20 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_regs.c

-- 
2.7.4

[PATCH net-next 1/2] ethtool: Add set regs -D option support

[Saeed Mahameed]

From: Gal Pressman <galp@mellanox.com>

Currently ethtool only allows us to get device registers, in this patch
we extend this functionality to also set device registers.
ethtool -D DEVNAME [ file FILENAME ] is used to set registers in
the device using vendor specific binary registers data provided via
stdin/file. Changes made by this option can be queried using get
regs -d flag.

Example:
$ ethtool -D eth1 file /tmp/mlx5_regs

Signed-off-by: Gal Pressman <galp@mellanox.com>
Signed-off-by: Dmitry Teif <dimat@mellanox.com>
CC: John W. Linville <linville@tuxdriver.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 include/linux/ethtool.h      |  1 +
 include/uapi/linux/ethtool.h |  1 +
 net/core/ethtool.c           | 31 +++++++++++++++++++++++++++++++
 3 files changed, 33 insertions(+)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 9ded8c6..c9f5d37 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -305,6 +305,7 @@ struct ethtool_ops {
 	void	(*get_drvinfo)(struct net_device *, struct ethtool_drvinfo *);
 	int	(*get_regs_len)(struct net_device *);
 	void	(*get_regs)(struct net_device *, struct ethtool_regs *, void *);
+	int	(*set_regs)(struct net_device *, struct ethtool_regs *, u8 *);
 	void	(*get_wol)(struct net_device *, struct ethtool_wolinfo *);
 	int	(*set_wol)(struct net_device *, struct ethtool_wolinfo *);
 	u32	(*get_msglevel)(struct net_device *);
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
index f0db778..f81c6fd 100644
--- a/include/uapi/linux/ethtool.h
+++ b/include/uapi/linux/ethtool.h
@@ -1330,6 +1330,7 @@ struct ethtool_per_queue_op {
 #define ETHTOOL_SLINKSETTINGS	0x0000004d /* Set ethtool_link_settings */
 #define ETHTOOL_PHY_GTUNABLE	0x0000004e /* Get PHY tunable configuration */
 #define ETHTOOL_PHY_STUNABLE	0x0000004f /* Set PHY tunable configuration */
+#define ETHTOOL_SREGS		0x00000050 /* Set NIC registers */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index e23766c..5548565 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1424,6 +1424,34 @@ static int ethtool_get_regs(struct net_device *dev, char __user *useraddr)
 	return ret;
 }
 
+static int ethtool_set_regs(struct net_device *dev, char __user *useraddr)
+{
+	void __user *userbuf = useraddr + offsetof(struct ethtool_regs, data);
+	const struct ethtool_ops *ops = dev->ethtool_ops;
+	struct ethtool_regs regs;
+	int ret = 0;
+	u8 *data;
+
+	if (!ops->set_regs || !ops->get_regs_len)
+		return -EOPNOTSUPP;
+	if (copy_from_user(&regs, useraddr, sizeof(regs)))
+		return -EFAULT;
+
+	data = kmalloc(PAGE_SIZE, GFP_USER);
+	if (!data)
+		return -ENOMEM;
+
+	ret = -EFAULT;
+	if (copy_from_user(data, userbuf, regs.len))
+		goto out;
+
+	ret = ops->set_regs(dev, &regs, data);
+
+out:
+	kfree(data);
+	return ret;
+}
+
 static int ethtool_reset(struct net_device *dev, char __user *useraddr)
 {
 	struct ethtool_value reset;
@@ -2597,6 +2625,9 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GREGS:
 		rc = ethtool_get_regs(dev, useraddr);
 		break;
+	case ETHTOOL_SREGS:
+		rc = ethtool_set_regs(dev, useraddr);
+		break;
 	case ETHTOOL_GWOL:
 		rc = ethtool_get_wol(dev, useraddr);
 		break;
-- 
2.7.4

[PATCH net-next 2/2] net/mlx5e: Add ethtool get/set reg support

[Saeed Mahameed]

From: Gal Pressman <galp@mellanox.com>

Add ethtool -[dD] callbacks support for get and set registers.
This interface allows users to query and change device registers.

Add the support for set/get DIAGNOSTIC_PARAMS/COUNTERS registers.

Signed-off-by: Gal Pressman <galp@mellanox.com>
Signed-off-by: Dmitry Teif <dimat@mellanox.com>
Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/Makefile   |   2 +-
 drivers/net/ethernet/mellanox/mlx5/core/cmd.c      |  19 ----
 drivers/net/ethernet/mellanox/mlx5/core/en.h       |  12 +++
 .../net/ethernet/mellanox/mlx5/core/en_ethtool.c   |  21 ++++
 drivers/net/ethernet/mellanox/mlx5/core/en_main.c  |   8 ++
 drivers/net/ethernet/mellanox/mlx5/core/en_regs.c  | 116 +++++++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h                      |  22 ++++
 7 files changed, 180 insertions(+), 20 deletions(-)
 create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/en_regs.c

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
index 9f43beb..b24564c 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
+++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
@@ -8,6 +8,6 @@ mlx5_core-y :=	main.o cmd.o debugfs.o fw.o eq.o uar.o pagealloc.o \
 mlx5_core-$(CONFIG_MLX5_CORE_EN) += wq.o eswitch.o eswitch_offloads.o \
 		en_main.o en_common.o en_fs.o en_ethtool.o en_tx.o \
 		en_rx.o en_rx_am.o en_txrx.o en_clock.o vxlan.o \
-		en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o
+		en_tc.o en_arfs.o en_rep.o en_fs_ethtool.o en_selftest.o en_regs.o
 
 mlx5_core-$(CONFIG_MLX5_CORE_EN_DCB) +=  en_dcbnl.o
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
index b0448b5..f8b6c83 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/cmd.c
@@ -650,25 +650,6 @@ static int cmd_status_to_err(u8 status)
 	}
 }
 
-struct mlx5_ifc_mbox_out_bits {
-	u8         status[0x8];
-	u8         reserved_at_8[0x18];
-
-	u8         syndrome[0x20];
-
-	u8         reserved_at_40[0x40];
-};
-
-struct mlx5_ifc_mbox_in_bits {
-	u8         opcode[0x10];
-	u8         reserved_at_10[0x10];
-
-	u8         reserved_at_20[0x10];
-	u8         op_mod[0x10];
-
-	u8         reserved_at_40[0x40];
-};
-
 void mlx5_cmd_mbox_status(void *out, u8 *status, u32 *syndrome)
 {
 	*status = MLX5_GET(mbox_out, out, status);
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en.h b/drivers/net/ethernet/mellanox/mlx5/core/en.h
index 63dd639..fcc296b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en.h
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en.h
@@ -659,6 +659,11 @@ struct mlx5e_tir {
 	struct list_head  list;
 };
 
+struct mlx5e_reg {
+	u8  data_in[MLX5_ST_SZ_BYTES(mbox_in)];
+	u8 *data_out;
+};
+
 enum {
 	MLX5E_TC_PRIO = 0,
 	MLX5E_NIC_PRIO
@@ -713,6 +718,7 @@ struct mlx5e_priv {
 	struct mlx5e_stats         stats;
 	struct mlx5e_tstamp        tstamp;
 	u16 q_counter;
+	struct mlx5e_reg          *reg;
 #ifdef CONFIG_MLX5_CORE_EN_DCB
 	struct mlx5e_dcbx          dcbx;
 #endif
@@ -803,6 +809,12 @@ int mlx5e_get_max_linkspeed(struct mlx5_core_dev *mdev, u32 *speed);
 void mlx5e_set_rx_cq_mode_params(struct mlx5e_params *params,
 				 u8 cq_period_mode);
 
+struct mlx5e_reg *mlx5e_regs_init(void);
+int mlx5e_regs_set(struct net_device *dev, void *buff, int inlen);
+void mlx5e_regs_get(struct net_device *dev, void *buff);
+int mlx5e_regs_get_len(void);
+void mlx5e_regs_destroy(struct mlx5e_reg *reg);
+
 static inline void mlx5e_tx_notify_hw(struct mlx5e_sq *sq,
 				      struct mlx5_wqe_ctrl_seg *ctrl, int bf_sz)
 {
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
index 352462a..6adc9ea 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_ethtool.c
@@ -1552,6 +1552,23 @@ static u32 mlx5e_get_priv_flags(struct net_device *netdev)
 	return priv->params.pflags;
 }
 
+static int mlx5e_get_regs_len(struct net_device *dev)
+{
+	return mlx5e_regs_get_len();
+}
+
+static void mlx5e_get_regs(struct net_device *dev, struct ethtool_regs *regs,
+			   void *buff)
+{
+	mlx5e_regs_get(dev, buff);
+}
+
+static int mlx5e_set_regs(struct net_device *dev, struct ethtool_regs *regs,
+			  u8 *data)
+{
+	return mlx5e_regs_set(dev, data, regs->len);
+}
+
 static int mlx5e_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 {
 	int err = 0;
@@ -1605,4 +1622,8 @@ const struct ethtool_ops mlx5e_ethtool_ops = {
 	.get_priv_flags    = mlx5e_get_priv_flags,
 	.set_priv_flags    = mlx5e_set_priv_flags,
 	.self_test         = mlx5e_self_test,
+	.get_regs_len      = mlx5e_get_regs_len,
+	.get_regs          = mlx5e_get_regs,
+	.set_regs          = mlx5e_set_regs,
+
 };
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
index 9def5cc..e1905ba 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_main.c
@@ -3686,6 +3686,11 @@ static void mlx5e_nic_init(struct mlx5_core_dev *mdev,
 
 	mlx5e_build_nic_netdev_priv(mdev, netdev, profile, ppriv);
 	mlx5e_build_nic_netdev(netdev);
+
+	priv->reg = mlx5e_regs_init();
+	if (!priv->reg)
+		mlx5_core_warn(mdev, "Failed to allocate mlx5e_reg\n");
+
 	mlx5e_vxlan_init(priv);
 }
 
@@ -3696,6 +3701,9 @@ static void mlx5e_nic_cleanup(struct mlx5e_priv *priv)
 
 	mlx5e_vxlan_cleanup(priv);
 
+	if (priv->reg)
+		mlx5e_regs_destroy(priv->reg);
+
 	if (MLX5_CAP_GEN(mdev, vport_group_manager))
 		mlx5_eswitch_unregister_vport_rep(esw, 0);
 
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_regs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_regs.c
new file mode 100644
index 0000000..a83df1f
--- /dev/null
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_regs.c
@@ -0,0 +1,116 @@
+/*
+ * Copyright (c) 2016, Mellanox Technologies, Ltd.  All rights reserved.
+ *
+ * This software is available to you under a choice of one of two
+ * licenses.  You may choose to be licensed under the terms of the GNU
+ * General Public License (GPL) Version 2, available from the file
+ * COPYING in the main directory of this source tree, or the
+ * OpenIB.org BSD license below:
+ *
+ *     Redistribution and use in source and binary forms, with or
+ *     without modification, are permitted provided that the following
+ *     conditions are met:
+ *
+ *      - Redistributions of source code must retain the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer.
+ *
+ *      - Redistributions in binary form must reproduce the above
+ *        copyright notice, this list of conditions and the following
+ *        disclaimer in the documentation and/or other materials
+ *        provided with the distribution.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
+ * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ */
+
+#include <linux/mlx5/driver.h>
+#include "mlx5_core.h"
+#include "en.h"
+
+#define MLX5E_MAX_REG_LEN             4096
+#define MLX5E_MAX_CMD_OUT_LEN (MLX5E_MAX_REG_LEN - MLX5_ST_SZ_BYTES(mbox_in))
+
+static void reg_out_alloc(struct mlx5e_reg *reg)
+{
+	if (reg->data_out) {
+		memset(reg->data_out, 0, MLX5E_MAX_CMD_OUT_LEN);
+		return;
+	}
+
+	reg->data_out = mlx5_vzalloc(MLX5E_MAX_CMD_OUT_LEN);
+}
+
+struct mlx5e_reg *mlx5e_regs_init(void)
+{
+	return kzalloc(sizeof(struct mlx5e_reg), GFP_KERNEL);
+}
+
+void mlx5e_regs_destroy(struct mlx5e_reg *reg)
+{
+	kvfree(reg->data_out);
+	kfree(reg);
+}
+
+static bool opcode_valid(u16 opcode)
+{
+	switch (opcode) {
+	case MLX5_CMD_OP_QUERY_HCA_CAP:
+	case MLX5_CMD_OP_QUERY_DIAGNOSTIC_PARAMS:
+	case MLX5_CMD_OP_SET_DIAGNOSTIC_PARAMS:
+	case MLX5_CMD_OP_QUERY_DIAGNOSTICS_COUNTERS:
+		return true;
+	}
+
+	return false;
+}
+
+int mlx5e_regs_set(struct net_device *dev, void *buff, int inlen)
+{
+	struct mlx5e_priv *priv = netdev_priv(dev);
+	struct mlx5_core_dev *mdev = priv->mdev;
+	struct mlx5e_reg *reg = priv->reg;
+	u16 opcode;
+
+	if (!reg)
+		return -ENOMEM;
+
+	opcode = MLX5_GET(mbox_in, buff, opcode);
+	if (!opcode_valid(opcode))
+		return -EINVAL;
+
+	reg_out_alloc(reg);
+	if (!reg->data_out)
+		return -ENOMEM;
+
+	memcpy(reg->data_in, buff, sizeof(reg->data_in));
+
+	return mlx5_cmd_exec(mdev, buff, inlen, reg->data_out,
+			     MLX5E_MAX_CMD_OUT_LEN);
+}
+
+void mlx5e_regs_get(struct net_device *dev, void *buff)
+{
+	struct mlx5e_priv *priv = netdev_priv(dev);
+	struct mlx5e_reg *reg = priv->reg;
+
+	if (!reg)
+		return;
+
+	if (reg->data_out) {
+		memcpy(buff, reg->data_in, sizeof(reg->data_in));
+		memcpy(buff + sizeof(reg->data_in), reg->data_out,
+		       MLX5E_MAX_CMD_OUT_LEN);
+	}
+}
+
+int mlx5e_regs_get_len(void)
+{
+	return MLX5E_MAX_REG_LEN;
+}
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index a5f0fbe..9738b70 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -169,6 +169,9 @@ enum {
 	MLX5_CMD_OP_DEALLOC_XRCD                  = 0x80f,
 	MLX5_CMD_OP_ALLOC_TRANSPORT_DOMAIN        = 0x816,
 	MLX5_CMD_OP_DEALLOC_TRANSPORT_DOMAIN      = 0x817,
+	MLX5_CMD_OP_QUERY_DIAGNOSTIC_PARAMS       = 0x819,
+	MLX5_CMD_OP_SET_DIAGNOSTIC_PARAMS         = 0x820,
+	MLX5_CMD_OP_QUERY_DIAGNOSTICS_COUNTERS    = 0x821,
 	MLX5_CMD_OP_QUERY_CONG_STATUS             = 0x822,
 	MLX5_CMD_OP_MODIFY_CONG_STATUS            = 0x823,
 	MLX5_CMD_OP_QUERY_CONG_PARAMS             = 0x824,
@@ -230,6 +233,25 @@ enum {
 	MLX5_CMD_OP_MAX
 };
 
+struct mlx5_ifc_mbox_out_bits {
+	u8         status[0x8];
+	u8         reserved_at_8[0x18];
+
+	u8         syndrome[0x20];
+
+	u8         reserved_at_40[0x40];
+};
+
+struct mlx5_ifc_mbox_in_bits {
+	u8         opcode[0x10];
+	u8         reserved_at_10[0x10];
+
+	u8         reserved_at_20[0x10];
+	u8         op_mod[0x10];
+
+	u8         reserved_at_40[0x40];
+};
+
 struct mlx5_ifc_flow_table_fields_supported_bits {
 	u8         outer_dmac[0x1];
 	u8         outer_smac[0x1];
-- 
2.7.4

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Stephen Hemminger]

On Wed,  7 Dec 2016 00:33:08 +0200
Saeed Mahameed <saeedm@mellanox.com> wrote:

> This simple ethool change will give HW vendors the flexibility to set
> pure HW configurations (not directly related to netdev resources states
> and rings), without the need of vendor proprietary tools and hacks.


The danger is you need to restrict the kernel to only allow setting
safe registers (and this is HW dependent).  There are cases like secure
boot where it is expected that even root is not allowed to modify
all memory.

Also supporting closed format of device registers is not in the interest
of promoting open source.

I am not saying I fundamentally disagree with supporting this, but it
is a bigger step than you make it out to be.

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Andrew Lunn]

On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
> Hi Dave,
> 
> This series adds the support for setting device registers from user
> space ethtool.

Is this not the start of allowing binary only drivers in user space?

Do we want this?

> mlx5 driver have registers allowed access list and will check the user 
> Request validity before forwarding it to HW registers. Mlx5 will allow only mlx5 specific
> configurations to be set (e.g. Device Diag Counters for HW performance debugging and analysis)
> which has no standard API to access it.

Would it not be better to define an flexible API to do this? We have
lots of HW performance counters for CPUs. Why is it not possible to do
this for a network device?

      Andrew

Re: [PATCH net-next 0/2] Add ethtool set regs support

[David Miller]

From: Andrew Lunn <andrew@lunn.ch>
Date: Wed, 7 Dec 2016 03:41:43 +0100

> On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
>> Hi Dave,
>> 
>> This series adds the support for setting device registers from user
>> space ethtool.
> 
> Is this not the start of allowing binary only drivers in user space?
> 
> Do we want this?

I don't think we do.

> 
>> mlx5 driver have registers allowed access list and will check the user 
>> Request validity before forwarding it to HW registers. Mlx5 will allow only mlx5 specific
>> configurations to be set (e.g. Device Diag Counters for HW performance debugging and analysis)
>> which has no standard API to access it.
> 
> Would it not be better to define an flexible API to do this? We have
> lots of HW performance counters for CPUs. Why is it not possible to do
> this for a network device?

So if this isn't for raw PIO register access, then we should define
an appropriate interface for it.

The ethtool regs stuff is untyped, and arbitrary.

Please create something properly structured, and typed, which would
allow accessing the information you want the user to be able to
access.

That way the kernel can tell what the user is reading or writing,
and thus properly control access.

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Saeed Mahameed]

On Wed, Dec 7, 2016 at 12:45 AM, Stephen Hemminger
<stephen@networkplumber.org> wrote:
> On Wed,  7 Dec 2016 00:33:08 +0200
> Saeed Mahameed <saeedm@mellanox.com> wrote:
>
>> This simple ethool change will give HW vendors the flexibility to set
>> pure HW configurations (not directly related to netdev resources states
>> and rings), without the need of vendor proprietary tools and hacks.
>
>
> The danger is you need to restrict the kernel to only allow setting
> safe registers (and this is HW dependent).  There are cases like secure
> boot where it is expected that even root is not allowed to modify
> all memory.
>
> Also supporting closed format of device registers is not in the interest
> of promoting open source.
>

This is not totally close format, it is only a generic API to support
setting some of the HW registers.
the format is open in
1. http://www.mellanox.com/related-docs/user_manuals/Ethernet_Adapters_Programming_Manual.pdf
2. http://lxr.free-electrons.com/source/include/linux/mlx5/mlx5_ifc.h

We just want to let the user to configure HW dependent registers
without the need to add new defines/callback each time to the kernel
ABI/API.

> I am not saying I fundamentally disagree with supporting this, but it
> is a bigger step than you make it out to be.

I have to disagree, it is not as big as it seems, there are already at
least two places in ethtool API that do the same,

http://lxr.free-electrons.com/source/include/linux/ethtool.h
192  * @set_eeprom: Write data to the device EEPROM.
193  *      Should validate the magic field.  Don't need to check len for zero
194  *      or wraparound.  Update len to the amount written.  Returns an error
195  *      or zero.
http://lxr.free-electrons.com/source/net/core/ethtool.c#L1582

234  * @flash_device: Write a firmware image to device's flash memory.
235  *      Returns a negative error code or zero.

Both accept a binary array from user and forward to HW/FW.
Simply we want to to the same for HW registers, you already have a way
to get them (even parse them in user ethtool), but you can't set them.

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Saeed Mahameed]

On Wed, Dec 7, 2016 at 4:41 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
>> Hi Dave,
>>
>> This series adds the support for setting device registers from user
>> space ethtool.
>
> Is this not the start of allowing binary only drivers in user space?
>

It is not, we want to do same as set_eeprom already do,
Just set some HW registers, for analysis/debug/tweak/configure HW
dependent register for the NIC netdev sake.


> Do we want this?
>

Apparently yes, Our performance team will be happy to have such
feature upstream, so they can just jump in and start their performance
analysis/debugging
with ethtool directly without the need of extra proprietary FW tools
(which sometimes demand reboot and driver restart for each
configuration).

This is one usage and i believe other NIC vendors will be happy to have this.

>
>> mlx5 driver have registers allowed access list and will check the user
>> Request validity before forwarding it to HW registers. Mlx5 will allow only mlx5 specific
>> configurations to be set (e.g. Device Diag Counters for HW performance debugging and analysis)
>> which has no standard API to access it.
>
> Would it not be better to define an flexible API to do this? We have
> lots of HW performance counters for CPUs. Why is it not possible to do
> this for a network device?
>

What is flexible in this context ? Register Types/layout and purposes
differs from vendor to vendor and changes cross HW updates and new HW
generations.
We simply don't want to inform/update the stack and ethtool API about
each register layout, this is just not scalable, and those registers
are not precisely meant for performance/debug counters, sometimes they
have some other purposes. For example configuring an internal HW unit
to work in different ways, so we can have some taste of  "on the fly"
control for testing purposes.

where can i find some details about "HW performance counters for CPUs" ?
In light of my response, if you have any suggestion for more flexible
(strongly typed) API, I will be glad to hear about it.

Thanks for your response.
Saeed.

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Saeed Mahameed]

On Wed, Dec 7, 2016 at 4:57 AM, David Miller <davem@davemloft.net> wrote:
> From: Andrew Lunn <andrew@lunn.ch>
> Date: Wed, 7 Dec 2016 03:41:43 +0100
>
>> On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
>>> Hi Dave,
>>>
>>> This series adds the support for setting device registers from user
>>> space ethtool.
>>
>> Is this not the start of allowing binary only drivers in user space?
>>
>> Do we want this?
>
> I don't think we do.
>

Dave, i think we do have a case here, can you please check my response
to Stephen and Andrew,
I mention some examples and I think I made the purpose of this patches
more clear.

>>
>>> mlx5 driver have registers allowed access list and will check the user
>>> Request validity before forwarding it to HW registers. Mlx5 will allow only mlx5 specific
>>> configurations to be set (e.g. Device Diag Counters for HW performance debugging and analysis)
>>> which has no standard API to access it.
>>
>> Would it not be better to define an flexible API to do this? We have
>> lots of HW performance counters for CPUs. Why is it not possible to do
>> this for a network device?
>
> So if this isn't for raw PIO register access, then we should define
> an appropriate interface for it.
>

It is not, I hope i made it more clear in my responses to Stephen and Andrew.

>
> The ethtool regs stuff is untyped, and arbitrary.
>
> Please create something properly structured, and typed, which would
> allow accessing the information you want the user to be able to
> access.
>
> That way the kernel can tell what the user is reading or writing,
> and thus properly control access.

Thanks for your input,

As i explained to Stephen and Andrew, Those registers differs from
vendor to vendor and cross HW updates. Making those registers
strongly typed is wrong in my opinion, it is not scalable and as
flexible as you think it would be.  There are many HW vendors and each
has lots of registers.

Having a set_eeprom like API will do the job. you can think about this
as ethtool_flash_device kinda tool but more lightweight.

We simply would like to configure/tweak/monitor/debug HW internal
units activities for NIC netdev sake, like the usage we currently
have/suggest,
to have flexibility to enable/disable/query some HW internal
performance registers (should be off on production), both set and
query
will use ethtool_set/get_regs, obviously.

We also took into account future usages for other registers/formats
and to have flexibility across different NICs.

I will be glad if you can give this some thought and provide me with
more concrete direction/suggestion on how to make this less untyped.

The only thing i can think of right now is providing a list of write
allowed registers (address + type + layout length) to ethtool, but i
am not sure how
to handle the "type" information in the kernel, it will create more
restrictions than needed, and what types of register will we need to
add support for at first ?

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Andrew Lunn]

On Sun, Dec 11, 2016 at 02:18:00PM +0200, Saeed Mahameed wrote:
> On Wed, Dec 7, 2016 at 4:41 AM, Andrew Lunn <andrew@lunn.ch> wrote:
> > On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
> >> Hi Dave,
> >>
> >> This series adds the support for setting device registers from user
> >> space ethtool.
> >
> > Is this not the start of allowing binary only drivers in user space?
> >
> 
> It is not, we want to do same as set_eeprom already do,
> Just set some HW registers, for analysis/debug/tweak/configure HW
> dependent register for the NIC netdev sake.

Mellanox has a good reputation of open drivers. However, this API
sounds like it would be the first step towards user space
drivers. This is an API which can peek and poke registers, so it
probably could be mis-used to put part of a driver in user
space. Hence we should avoid this sort of API to start with.

> where can i find some details about "HW performance counters for CPUs" ?

https://perf.wiki.kernel.org/index.php/Tutorial

	Andrew

Re: [PATCH net-next 0/2] Add ethtool set regs support

[Florian Fainelli]

On 12/11/2016 07:22 AM, Andrew Lunn wrote:
> On Sun, Dec 11, 2016 at 02:18:00PM +0200, Saeed Mahameed wrote:
>> On Wed, Dec 7, 2016 at 4:41 AM, Andrew Lunn <andrew@lunn.ch> wrote:
>>> On Wed, Dec 07, 2016 at 12:33:08AM +0200, Saeed Mahameed wrote:
>>>> Hi Dave,
>>>>
>>>> This series adds the support for setting device registers from user
>>>> space ethtool.
>>>
>>> Is this not the start of allowing binary only drivers in user space?
>>>
>>
>> It is not, we want to do same as set_eeprom already do,
>> Just set some HW registers, for analysis/debug/tweak/configure HW
>> dependent register for the NIC netdev sake.
> 
> Mellanox has a good reputation of open drivers. However, this API
> sounds like it would be the first step towards user space
> drivers. This is an API which can peek and poke registers, so it
> probably could be mis-used to put part of a driver in user
> space. Hence we should avoid this sort of API to start with.

I don't necessarily share your concerns here on the proprietary vs. open
source driver, because this interface is limited to the register space,
not the data path, there is only a handful of things you can do here,
but getting a NIC to work, is not probably one of them.

My concern is more with the support/debugging aspect, if someone starts
tweaking a gazillion of registers through that interface, and I have no
way to tell, how am I going to support that? Of course, the first step
is: have you tried to turn it on and off again, and see if this is
reproducible, but what if I was asked/told to tweak this or that value
first, etc... it can be hard to collect the exact state in which a NIC
is at the time of the problem.

NB: on the proprietary driver side, you can already mmap() the PCI
device's space and write an entire user-space based driver (DPDK) and
bypass the kernel for most things, ethtool -D is not much worse here
since it just offers a subset (and a small one) of that.
-- 
Florian