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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <43F901BD926A4E43B106BF17856F0755019C9F2EFA@orsmsx508.amr.corp.intel.com>
Date:	Thu, 13 Oct 2011 10:04:26 -0700
From:	"Rose, Gregory V" <gregory.v.rose@...el.com>
To:	Ben Hutchings <bhutchings@...arflare.com>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [RFC PATCH V2 1/2] net/core/ethtool: New Commands to Configure
 IOV features

> -----Original Message-----
> From: Ben Hutchings [mailto:bhutchings@...arflare.com]
> Sent: Friday, October 07, 2011 5:38 PM
> To: Rose, Gregory V
> Cc: netdev@...r.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@...el.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.