[RFC PATCH V2 1/2] net/core/ethtool: New Commands to Configure IOV features

[RFC PATCH V2 1/2] net/core/ethtool: New Commands to Configure IOV features

[Greg Rose]

The only currently supported method of configuring the number of VFs
is through the max_vfs module parameter.  This method is inadequate to
support scenarios in which the user might wish to have varying numbers
of VFs per PF.  There is additional support for drivers that want to
partition some driver resources to additional net devices and for
configuring the number of Tx/Rx queue pairs per VF.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---

 include/linux/ethtool.h |   30 +++++++++++++++++++++++++++++-
 net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 1 deletions(-)

diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 45f00b6..448730f 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -720,6 +720,27 @@ enum ethtool_sfeatures_retval_bits {
 #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
 #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
 
+enum ethtool_iov_modes {
+	ETHTOOL_IOV_MODE_NONE,
+	ETHTOOL_IOV_MODE_SRIOV,
+	ETHTOOL_IOV_MODE_NETDEVS,
+};
+
+#define	ETHTOOL_IOV_CMD_CONFIGURE_SRIOV		1
+#define	ETHTOOL_IOV_CMD_CONFIGURE_NETDEVS	2
+#define	ETHTOOL_IOV_CMD_CONFIGURE_VF_QUEUES	3
+
+struct ethtool_iov_set_cmd {
+	__u32 cmd;
+	__u32 set_cmd;
+	__u32 cmd_param;
+};
+
+struct ethtool_iov_get_cmd {
+	__u32 cmd;
+	__u32 mode;
+};
+
 #ifdef __KERNEL__
 
 #include <linux/rculist.h>
@@ -879,6 +900,8 @@ bool ethtool_invalid_flags(struct net_device *dev, u32 data, u32 supported);
  * 		   and flag of the device.
  * @get_dump_data: Get dump data.
  * @set_dump: Set dump specific flags to the device.
+ * @iov_set_cmd: Set IOV parameters - number of VFs or VM Queues
+ * @iov_get_cmd: Get current IOV parameters and configuration
  *
  * All operations are optional (i.e. the function pointer may be set
  * to %NULL) and callers must take this into account.  Callers must
@@ -956,7 +979,10 @@ struct ethtool_ops {
 	int	(*get_dump_data)(struct net_device *,
 				 struct ethtool_dump *, void *);
 	int	(*set_dump)(struct net_device *, struct ethtool_dump *);
-
+	int	(*iov_set_cmd)(struct net_device *,
+			struct ethtool_iov_set_cmd *);
+	int	(*iov_get_cmd)(struct net_device *,
+			struct ethtool_iov_get_cmd *);
 };
 #endif /* __KERNEL__ */
 
@@ -1030,6 +1056,8 @@ struct ethtool_ops {
 #define ETHTOOL_SET_DUMP	0x0000003e /* Set dump settings */
 #define ETHTOOL_GET_DUMP_FLAG	0x0000003f /* Get dump settings */
 #define ETHTOOL_GET_DUMP_DATA	0x00000040 /* Get dump data */
+#define ETHTOOL_IOV_SET_CMD	0x00000041 /* Set IOV parameters */
+#define ETHTOOL_IOV_GET_CMD	0x00000042 /* Get IOV parameters */
 
 /* compatibility with older code */
 #define SPARC_ETH_GSET		ETHTOOL_GSET
diff --git a/net/core/ethtool.c b/net/core/ethtool.c
index f444817..a469e40 100644
--- a/net/core/ethtool.c
+++ b/net/core/ethtool.c
@@ -1633,6 +1633,38 @@ out:
 	return ret;
 }
 
+static int ethtool_iov_set_command(struct net_device *dev,
+				  void __user *useraddr)
+{
+	struct ethtool_iov_set_cmd iov_cmd;
+
+	if (!dev->ethtool_ops->iov_set_cmd)
+		return -EOPNOTSUPP;
+	if (copy_from_user(&iov_cmd, useraddr, sizeof(iov_cmd)))
+		return -EFAULT;
+
+	return dev->ethtool_ops->iov_set_cmd(dev, &iov_cmd);
+}
+
+static int ethtool_iov_get_command(struct net_device *dev,
+				  void __user *useraddr)
+{
+	int ret;
+	struct ethtool_iov_get_cmd iov_cmd;
+
+	if (!dev->ethtool_ops->iov_get_cmd)
+		return -EOPNOTSUPP;
+	if (copy_from_user(&iov_cmd, useraddr, sizeof(iov_cmd)))
+		return -EFAULT;
+
+	ret = dev->ethtool_ops->iov_get_cmd(dev, &iov_cmd);
+	if (!ret)
+		ret = copy_to_user(useraddr, &iov_cmd, sizeof(iov_cmd));
+
+	return ret;
+}
+
+
 /* The main entry point in this file.  Called from net/core/dev.c */
 
 int dev_ethtool(struct net *net, struct ifreq *ifr)
@@ -1855,6 +1887,12 @@ int dev_ethtool(struct net *net, struct ifreq *ifr)
 	case ETHTOOL_GET_DUMP_DATA:
 		rc = ethtool_get_dump_data(dev, useraddr);
 		break;
+	case ETHTOOL_IOV_SET_CMD:
+		rc = ethtool_iov_set_command(dev, useraddr);
+		break;
+	case ETHTOOL_IOV_GET_CMD:
+		rc = ethtool_iov_get_command(dev, useraddr);
+		break;
 	default:
 		rc = -EOPNOTSUPP;
 	}

[RFC PATCH V2 2/2] ixgbe: Add support for new ethtool IOV configuration commands

[Greg Rose]

Implement driver support for the new ethtool command to configure
the number of VFs per PF independently.  The framework is in place
for support of two other features that allow the user to partition
some of the I/O resources of the device to additional semi-independent
net devices and to set the number of queues per VF.

Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
---

 drivers/net/ethernet/intel/ixgbe/ixgbe.h         |    1 
 drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c |  110 ++++++++++++++++++++++
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c    |    3 +
 3 files changed, 113 insertions(+), 1 deletions(-)

diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 8fab322..cea2323 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -528,6 +528,7 @@ struct ixgbe_adapter {
 	int fdir_filter_count;
 	u32 timer_event_accumulator;
 	u32 vferr_refcount;
+	const struct ixgbe_info *saved_ii;
 };
 
 struct ixgbe_fdir_filter {
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 08c9994..c695362 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -38,7 +38,7 @@
 #include <linux/uaccess.h>
 
 #include "ixgbe.h"
-
+#include "ixgbe_sriov.h"
 
 #define IXGBE_ALL_RAR_ENTRIES 16
 
@@ -2577,6 +2577,112 @@ static int ixgbe_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd)
 	return ret;
 }
 
+static int ixgbe_reinit_sriov(struct net_device *netdev, int new_vfs)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(netdev);
+	struct pci_dev *pdev = adapter->pdev;
+	int err;
+	int i;
+
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+		if (ixgbe_check_vf_assignment(adapter)) {
+			netdev_warn(netdev, "%s ",
+				    "reconfigure of SR-IOV VFs "
+				    "not supported while VFs are "
+				    "assigned to guest VMs\n");
+			return -EBUSY;
+		}
+	}
+	if (netif_running(netdev)) {
+		netdev_warn(netdev, "%s",
+			    "Cannot reconfigure SR-IOV "
+			    "while interface is up\n"
+			    "Please bring the interface "
+			    "down first\n");
+		 return -EBUSY;
+	}
+
+	ixgbe_clear_interrupt_scheme(adapter);
+
+	if (adapter->num_vfs)
+		ixgbe_disable_sriov(adapter);
+
+	adapter->num_vfs = (new_vfs > 63) ? 63 : new_vfs;
+
+	if (adapter->num_vfs) {
+		ixgbe_enable_sriov(adapter, adapter->saved_ii);
+		for (i = 0; i < adapter->num_vfs; i++)
+			ixgbe_vf_configuration(pdev, (i | 0x10000000));
+	}
+
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED) {
+		adapter->flags &= ~(IXGBE_FLAG_RSS_ENABLED |
+				    IXGBE_FLAG_DCB_ENABLED);
+		netdev->features &= ~NETIF_F_RXHASH;
+	} else {
+		adapter->flags |= IXGBE_FLAG_RSS_ENABLED;
+		netdev->features |= NETIF_F_RXHASH;
+	}
+
+	err = ixgbe_init_interrupt_scheme(adapter);
+	/*
+	 * If we can't init some sort of interrupt scheme then the device
+	 * is hosed - just print a warning and bail.  Nothing will work
+	 * but at least we've put a message in the system log telling why.
+	 */
+	if (err)
+		e_dev_err("Cannot initialize interrupts for device\n");
+	else
+		ixgbe_reset(adapter);
+
+	return err;
+}
+
+static int ixgbe_iov_set_cmd(struct net_device *dev,
+			     struct ethtool_iov_set_cmd *iov_cmd)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+	struct ixgbe_hw *hw = &adapter->hw;
+	int err = 0;
+
+	if (iov_cmd->set_cmd == ETHTOOL_IOV_CMD_CONFIGURE_SRIOV &&
+	    hw->mac.type == ixgbe_mac_82598EB)
+		return -EOPNOTSUPP;
+
+	switch (iov_cmd->set_cmd) {
+	case ETHTOOL_IOV_CMD_CONFIGURE_SRIOV:
+		if (iov_cmd->cmd_param != adapter->num_vfs &&
+		    !(adapter->flags & IXGBE_FLAG_DCB_ENABLED))
+			err = ixgbe_reinit_sriov(dev, iov_cmd->cmd_param);
+		else
+			err = -EINVAL;
+		break;
+	case ETHTOOL_IOV_CMD_CONFIGURE_NETDEVS:
+		err = -ENOTSUPP;
+		break;
+	case ETHTOOL_IOV_CMD_CONFIGURE_VF_QUEUES:
+		err = -ENOTSUPP;
+		break;
+	default:
+		err = -EINVAL;
+		break;
+	}
+
+	return err;
+}
+
+static int ixgbe_iov_get_cmd(struct net_device *dev,
+			     struct ethtool_iov_get_cmd *iov_cmd)
+{
+	struct ixgbe_adapter *adapter = netdev_priv(dev);
+
+	iov_cmd->mode = ETHTOOL_IOV_MODE_NONE;
+	if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+		iov_cmd->mode = ETHTOOL_IOV_MODE_SRIOV;
+
+	return 0;
+}
+
 static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.get_settings           = ixgbe_get_settings,
 	.set_settings           = ixgbe_set_settings,
@@ -2605,6 +2711,8 @@ static const struct ethtool_ops ixgbe_ethtool_ops = {
 	.set_coalesce           = ixgbe_set_coalesce,
 	.get_rxnfc		= ixgbe_get_rxnfc,
 	.set_rxnfc		= ixgbe_set_rxnfc,
+	.iov_set_cmd		= ixgbe_iov_set_cmd,
+	.iov_get_cmd		= ixgbe_iov_get_cmd,
 };
 
 void ixgbe_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 99ff8b2..546f3ba 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -7591,6 +7591,9 @@ static void __devinit ixgbe_probe_vf(struct ixgbe_adapter *adapter,
 	if (hw->mac.type == ixgbe_mac_82598EB)
 		return;
 
+	/* need to save this away in case SR-IOV is reconfigured */
+	adapter->saved_ii = ii;
+
 	/* The 82599 supports up to 64 VFs per physical function
 	 * but this implementation limits allocation to 63 so that
 	 * basic networking resources are still available to the

Re: [RFC PATCH V2 1/2] net/core/ethtool: New Commands to Configure IOV features

[Ben Hutchings]

On Thu, 2011-09-22 at 14:35 -0700, Greg Rose wrote:
> The only currently supported method of configuring the number of VFs
> is through the max_vfs module parameter.  This method is inadequate to
> support scenarios in which the user might wish to have varying numbers
> of VFs per PF.  There is additional support for drivers that want to
> partition some driver resources to additional net devices and for
> configuring the number of Tx/Rx queue pairs per VF.
> 
> Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
> ---
> 
>  include/linux/ethtool.h |   30 +++++++++++++++++++++++++++++-
>  net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 67 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> index 45f00b6..448730f 100644
> --- a/include/linux/ethtool.h
> +++ b/include/linux/ethtool.h
> @@ -720,6 +720,27 @@ enum ethtool_sfeatures_retval_bits {
>  #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
>  #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
>  
> +enum ethtool_iov_modes {
> +	ETHTOOL_IOV_MODE_NONE,
> +	ETHTOOL_IOV_MODE_SRIOV,
> +	ETHTOOL_IOV_MODE_NETDEVS,
> +};
> +
> +#define	ETHTOOL_IOV_CMD_CONFIGURE_SRIOV		1
> +#define	ETHTOOL_IOV_CMD_CONFIGURE_NETDEVS	2
> +#define	ETHTOOL_IOV_CMD_CONFIGURE_VF_QUEUES	3
> +
> +struct ethtool_iov_set_cmd {
> +	__u32 cmd;
> +	__u32 set_cmd;
> +	__u32 cmd_param;
> +};

Why introduce sub-commands?

> +struct ethtool_iov_get_cmd {
> +	__u32 cmd;
> +	__u32 mode;
> +};

Why is it only possible to get the mode?

It really seems to make more sense to group these three parameters
together and get/set them all at once.  But if you can justify making
them separate then struct ethtool_value is the type to use.

In any case, any new types or macros need comments.

[...]
> +static int ethtool_iov_get_command(struct net_device *dev,
> +				  void __user *useraddr)
> +{
> +	int ret;
> +	struct ethtool_iov_get_cmd iov_cmd;
> +
> +	if (!dev->ethtool_ops->iov_get_cmd)
> +		return -EOPNOTSUPP;
> +	if (copy_from_user(&iov_cmd, useraddr, sizeof(iov_cmd)))
> +		return -EFAULT;
> +
> +	ret = dev->ethtool_ops->iov_get_cmd(dev, &iov_cmd);
> +	if (!ret)
> +		ret = copy_to_user(useraddr, &iov_cmd, sizeof(iov_cmd));

	if (!ret && copy_to_user(...))
		ret = -EFAULT;

> +	return ret;
> +}
[...]

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

RE: [RFC PATCH V2 1/2] net/core/ethtool: New Commands to Configure IOV features

[Rose, Gregory V]

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@solarflare.com]
> Sent: Friday, October 07, 2011 5:38 PM
> To: Rose, Gregory V
> Cc: netdev@vger.kernel.org
> Subject: Re: [RFC PATCH V2 1/2] net/core/ethtool: New Commands to
> Configure IOV features
> 
> On Thu, 2011-09-22 at 14:35 -0700, Greg Rose wrote:
> > The only currently supported method of configuring the number of VFs
> > is through the max_vfs module parameter.  This method is inadequate to
> > support scenarios in which the user might wish to have varying numbers
> > of VFs per PF.  There is additional support for drivers that want to
> > partition some driver resources to additional net devices and for
> > configuring the number of Tx/Rx queue pairs per VF.
> >
> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com>
> > ---
> >
> >  include/linux/ethtool.h |   30 +++++++++++++++++++++++++++++-
> >  net/core/ethtool.c      |   38 ++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 67 insertions(+), 1 deletions(-)
> >
> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
> > index 45f00b6..448730f 100644
> > --- a/include/linux/ethtool.h
> > +++ b/include/linux/ethtool.h
> > @@ -720,6 +720,27 @@ enum ethtool_sfeatures_retval_bits {
> >  #define ETHTOOL_F_WISH          (1 << ETHTOOL_F_WISH__BIT)
> >  #define ETHTOOL_F_COMPAT        (1 << ETHTOOL_F_COMPAT__BIT)
> >
> > +enum ethtool_iov_modes {
> > +	ETHTOOL_IOV_MODE_NONE,
> > +	ETHTOOL_IOV_MODE_SRIOV,
> > +	ETHTOOL_IOV_MODE_NETDEVS,
> > +};
> > +
> > +#define	ETHTOOL_IOV_CMD_CONFIGURE_SRIOV		1
> > +#define	ETHTOOL_IOV_CMD_CONFIGURE_NETDEVS	2
> > +#define	ETHTOOL_IOV_CMD_CONFIGURE_VF_QUEUES	3
> > +
> > +struct ethtool_iov_set_cmd {
> > +	__u32 cmd;
> > +	__u32 set_cmd;
> > +	__u32 cmd_param;
> > +};
> 
> Why introduce sub-commands?

For maximum flexibility.  It's difficult (to me anyway) to foresee all the things we might want to configure for I/O virtualization features.

> 
> > +struct ethtool_iov_get_cmd {
> > +	__u32 cmd;
> > +	__u32 mode;
> > +};
> 
> Why is it only possible to get the mode?

To my way of thinking it was the only interesting thing to query.  I suppose I could also return the number of VFs or net devices along with the mode.

> 
> It really seems to make more sense to group these three parameters
> together and get/set them all at once.  But if you can justify making
> them separate then struct ethtool_value is the type to use.

I can do that.

> 
> In any case, any new types or macros need comments.

OK.

> 
> [...]
> > +static int ethtool_iov_get_command(struct net_device *dev,
> > +				  void __user *useraddr)
> > +{
> > +	int ret;
> > +	struct ethtool_iov_get_cmd iov_cmd;
> > +
> > +	if (!dev->ethtool_ops->iov_get_cmd)
> > +		return -EOPNOTSUPP;
> > +	if (copy_from_user(&iov_cmd, useraddr, sizeof(iov_cmd)))
> > +		return -EFAULT;
> > +
> > +	ret = dev->ethtool_ops->iov_get_cmd(dev, &iov_cmd);
> > +	if (!ret)
> > +		ret = copy_to_user(useraddr, &iov_cmd, sizeof(iov_cmd));
> 
> 	if (!ret && copy_to_user(...))
> 		ret = -EFAULT;
> 
> > +	return ret;
> > +}
> [...]
> 
> --
> Ben Hutchings, Staff Engineer, Solarflare
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.