| Message ID | 20110727221759.8435.11589.stgit@gitlad.jf.intel.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Greg Rose <gregory.v.rose@intel.com> Date: Wed, 27 Jul 2011 15:17:59 -0700 > Add new set commands to configure the number of SR-IOV VFs, the > number of VM queues and spoof checking on/off switch. > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > --- > > include/linux/ethtool.h | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index c6e427a..c4972ba 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -36,12 +36,14 @@ struct ethtool_cmd { > __u8 mdio_support; > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > + __u32 num_vfs; /* Enable SR-IOV VFs */ > + __u32 num_vmqs; /* Set number of queues for VMDq */ You can't change the layout of this datastructure in this way without breaking every ethtool binary out there. You have to find another place to add these knobs. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Wednesday, July 27, 2011 10:28 PM > To: Rose, Gregory V > Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > From: Greg Rose <gregory.v.rose@intel.com> > Date: Wed, 27 Jul 2011 15:17:59 -0700 > > > Add new set commands to configure the number of SR-IOV VFs, the > > number of VM queues and spoof checking on/off switch. > > > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > --- > > > > include/linux/ethtool.h | 11 ++++++++++- > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index c6e427a..c4972ba 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -36,12 +36,14 @@ struct ethtool_cmd { > > __u8 mdio_support; > > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > > + __u32 num_vfs; /* Enable SR-IOV VFs */ > > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > You can't change the layout of this datastructure in this way without > breaking every ethtool binary out there. > > You have to find another place to add these knobs. Perhaps at the end of the ethtool_cmd structure? Something like this: /* This should work for both 32 and 64 bit userland. */ struct ethtool_cmd { __u32 cmd; __u32 supported; /* Features this interface supports */ __u32 advertising; /* Features this interface advertises */ __u16 speed; /* The forced speed (lower bits) in * Mbps. Please use * ethtool_cmd_speed()/_set() to * access it */ __u8 duplex; /* Duplex, half or full */ __u8 port; /* Which connector port */ __u8 phy_address; __u8 transceiver; /* Which transceiver to use */ __u8 autoneg; /* Enable or disable autonegotiation */ __u8 mdio_support; __u32 maxtxpkt; /* Tx pkts before generating tx int */ __u32 maxrxpkt; /* Rx pkts before generating rx int */ __u16 speed_hi; /* The forced speed (upper * bits) in Mbps. Please use * ethtool_cmd_speed()/_set() to * access it */ __u8 eth_tp_mdix; __u8 spoof_check; /* Enable/Disable anti-spoofing */ __u32 lp_advertising; /* Features the link partner advertises */ __u32 reserved[2]; __u32 num_vfs; /* Enable SR-IOV VFs */ __u32 num_vmqs; /* Set number of queues for VMDq */ }; - Greg -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: "Rose, Gregory V" <gregory.v.rose@intel.com> Date: Thu, 28 Jul 2011 08:51:05 -0700 >> -----Original Message----- >> From: David Miller [mailto:davem@davemloft.net] >> Sent: Wednesday, July 27, 2011 10:28 PM >> To: Rose, Gregory V >> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands >> >> From: Greg Rose <gregory.v.rose@intel.com> >> Date: Wed, 27 Jul 2011 15:17:59 -0700 >> >> > Add new set commands to configure the number of SR-IOV VFs, the >> > number of VM queues and spoof checking on/off switch. >> > >> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> >> > --- >> > >> > include/linux/ethtool.h | 11 ++++++++++- >> > 1 files changed, 10 insertions(+), 1 deletions(-) >> > >> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> > index c6e427a..c4972ba 100644 >> > --- a/include/linux/ethtool.h >> > +++ b/include/linux/ethtool.h >> > @@ -36,12 +36,14 @@ struct ethtool_cmd { >> > __u8 mdio_support; >> > __u32 maxtxpkt; /* Tx pkts before generating tx int */ >> > __u32 maxrxpkt; /* Rx pkts before generating rx int */ >> > + __u32 num_vfs; /* Enable SR-IOV VFs */ >> > + __u32 num_vmqs; /* Set number of queues for VMDq */ >> >> You can't change the layout of this datastructure in this way without >> breaking every ethtool binary out there. >> >> You have to find another place to add these knobs. > > Perhaps at the end of the ethtool_cmd structure? Something like this: Either use the two reserved u32's we have there, or create a new ethtool command and control structure. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Thursday, July 28, 2011 9:15 AM > To: Rose, Gregory V > Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > From: "Rose, Gregory V" <gregory.v.rose@intel.com> > Date: Thu, 28 Jul 2011 08:51:05 -0700 > > >> -----Original Message----- > >> From: David Miller [mailto:davem@davemloft.net] > >> Sent: Wednesday, July 27, 2011 10:28 PM > >> To: Rose, Gregory V > >> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey > T > >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > >> > >> From: Greg Rose <gregory.v.rose@intel.com> > >> Date: Wed, 27 Jul 2011 15:17:59 -0700 > >> > >> > Add new set commands to configure the number of SR-IOV VFs, the > >> > number of VM queues and spoof checking on/off switch. > >> > > >> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > >> > --- > >> > > >> > include/linux/ethtool.h | 11 ++++++++++- > >> > 1 files changed, 10 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > >> > index c6e427a..c4972ba 100644 > >> > --- a/include/linux/ethtool.h > >> > +++ b/include/linux/ethtool.h > >> > @@ -36,12 +36,14 @@ struct ethtool_cmd { > >> > __u8 mdio_support; > >> > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > >> > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > >> > + __u32 num_vfs; /* Enable SR-IOV VFs */ > >> > + __u32 num_vmqs; /* Set number of queues for VMDq */ > >> > >> You can't change the layout of this datastructure in this way without > >> breaking every ethtool binary out there. > >> > >> You have to find another place to add these knobs. > > > > Perhaps at the end of the ethtool_cmd structure? Something like this: > > Either use the two reserved u32's we have there, or create a new > ethtool command and control structure. OK, I see what you're saying. The size of the structure can never be changed. I'm slow sometimes but I do eventually get there. I didn't want to use the reserved u32's because I didn't know what they were reserved for. The num_vfs and num_vmqs values will actually never exceed 8 bits. I'll stuff them into the space used by one of the u32's and then add padding to align and keep the same structure size. Thanks for the feedback and help. I'll address this and the other feedback I've gotten and come back later with V2 of the RFC. - Greg -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> From: Anirban Chakraborty [mailto:anirban.chakraborty@qlogic.com] > Sent: Thursday, July 28, 2011 12:04 PM > To: Rose, Gregory V > Cc: David Miller; netdev; Ben Hutchings; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > > On Jul 28, 2011, at 8:51 AM, Rose, Gregory V wrote: > > > -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Wednesday, July 27, 2011 10:28 PM > To: Rose, Gregory V > Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > From: Greg Rose <gregory.v.rose@intel.com> > Date: Wed, 27 Jul 2011 15:17:59 -0700 > > Add new set commands to configure the number of SR-IOV VFs, the > number of VM queues and spoof checking on/off switch. > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > --- > > include/linux/ethtool.h | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index c6e427a..c4972ba 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -36,12 +36,14 @@ struct ethtool_cmd { > __u8 mdio_support; > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > + __u32 num_vfs; /* Enable SR-IOV VFs */ > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > You can't change the layout of this datastructure in this way without > breaking every ethtool binary out there. > > You have to find another place to add these knobs. > > Perhaps at the end of the ethtool_cmd structure? Something like this: > > /* This should work for both 32 and 64 bit userland. */ > struct ethtool_cmd { > __u32 cmd; > __u32 supported; /* Features this interface supports */ > __u32 advertising; /* Features this interface advertises */ > __u16 speed; /* The forced speed (lower bits) in > * Mbps. Please use > * ethtool_cmd_speed()/_set() to > * access it */ > __u8 duplex; /* Duplex, half or full */ > __u8 port; /* Which connector port */ > __u8 phy_address; > __u8 transceiver; /* Which transceiver to use */ > __u8 autoneg; /* Enable or disable autonegotiation */ > __u8 mdio_support; > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > __u16 speed_hi; /* The forced speed (upper > * bits) in Mbps. Please use > * ethtool_cmd_speed()/_set() to > * access it */ > __u8 eth_tp_mdix; > __u8 spoof_check; /* Enable/Disable anti-spoofing */ > __u32 lp_advertising; /* Features the link partner advertises */ > __u32 reserved[2]; > __u32 num_vfs; /* Enable SR-IOV VFs */ > __u32 num_vmqs; /* Set number of queues for VMDq */ > }; > > If I understood it correctly, you are trying to set/unset spoofing on per > eth interface, which could be a PF on the hypervisor or a pci passthru-ed > VF in the linux guest. There are other security features that one could set > for a port on the VF (lets call it vport), e.g. setting a port VLAN ID for > a VF and specifying if the VF (VM) is allowed to send tagged/untagged > packets, setting a vport in port mirroring mode so that the PF can monitor > the traffic on a VF, setting a vport in promiscuous mode etc. > > Does it make sense to try to use ip link util to specify all these parameters, > since ip link already does the job of setting VF properties and VF port > profile? > > Any thoughts? > Sure, that's a possibility too. I was considering ethtool for this since MAC addresses and VLANs are fairly specific to Ethernet whereas netlink might apply to other types of physical networks. At least that's my understanding. However, I have no strong feelings about it and if community consensus is to use ip link instead then that's fine by me. Of course, patches implementing such would be quite welcome also. ;^) - Greg -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-07-28 at 09:14 -0700, David Miller wrote: > From: "Rose, Gregory V" <gregory.v.rose@intel.com> > Date: Thu, 28 Jul 2011 08:51:05 -0700 > > >> -----Original Message----- > >> From: David Miller [mailto:davem@davemloft.net] > >> Sent: Wednesday, July 27, 2011 10:28 PM > >> To: Rose, Gregory V > >> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T > >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > >> > >> From: Greg Rose <gregory.v.rose@intel.com> > >> Date: Wed, 27 Jul 2011 15:17:59 -0700 > >> > >> > Add new set commands to configure the number of SR-IOV VFs, the > >> > number of VM queues and spoof checking on/off switch. > >> > > >> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > >> > --- > >> > > >> > include/linux/ethtool.h | 11 ++++++++++- > >> > 1 files changed, 10 insertions(+), 1 deletions(-) > >> > > >> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > >> > index c6e427a..c4972ba 100644 > >> > --- a/include/linux/ethtool.h > >> > +++ b/include/linux/ethtool.h > >> > @@ -36,12 +36,14 @@ struct ethtool_cmd { > >> > __u8 mdio_support; > >> > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > >> > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > >> > + __u32 num_vfs; /* Enable SR-IOV VFs */ > >> > + __u32 num_vmqs; /* Set number of queues for VMDq */ > >> > >> You can't change the layout of this datastructure in this way without > >> breaking every ethtool binary out there. > >> > >> You have to find another place to add these knobs. > > > > Perhaps at the end of the ethtool_cmd structure? Something like this: > > Either use the two reserved u32's we have there, or create a new > ethtool command and control structure. I'm going to insist on the latter. As I see it, struct ethtool_cmd is for Ethernet PHY settings. (The max{rx,tx}pkt fields weren't, but they're obsolete.) Ben.
> -----Original Message----- > From: Ben Hutchings [mailto:bhutchings@solarflare.com] > Sent: Thursday, July 28, 2011 2:15 PM > To: David Miller > Cc: Rose, Gregory V; netdev@vger.kernel.org; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > On Thu, 2011-07-28 at 09:14 -0700, David Miller wrote: > > From: "Rose, Gregory V" <gregory.v.rose@intel.com> > > Date: Thu, 28 Jul 2011 08:51:05 -0700 > > > > >> -----Original Message----- > > >> From: David Miller [mailto:davem@davemloft.net] > > >> Sent: Wednesday, July 27, 2011 10:28 PM > > >> To: Rose, Gregory V > > >> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, > Jeffrey T > > >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > >> > > >> From: Greg Rose <gregory.v.rose@intel.com> > > >> Date: Wed, 27 Jul 2011 15:17:59 -0700 > > >> > > >> > Add new set commands to configure the number of SR-IOV VFs, the > > >> > number of VM queues and spoof checking on/off switch. > > >> > > > >> > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > >> > --- > > >> > > > >> > include/linux/ethtool.h | 11 ++++++++++- > > >> > 1 files changed, 10 insertions(+), 1 deletions(-) > > >> > > > >> > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > >> > index c6e427a..c4972ba 100644 > > >> > --- a/include/linux/ethtool.h > > >> > +++ b/include/linux/ethtool.h > > >> > @@ -36,12 +36,14 @@ struct ethtool_cmd { > > >> > __u8 mdio_support; > > >> > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > > >> > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > > >> > + __u32 num_vfs; /* Enable SR-IOV VFs */ > > >> > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > >> > > >> You can't change the layout of this datastructure in this way without > > >> breaking every ethtool binary out there. > > >> > > >> You have to find another place to add these knobs. > > > > > > Perhaps at the end of the ethtool_cmd structure? Something like this: > > > > Either use the two reserved u32's we have there, or create a new > > ethtool command and control structure. > > I'm going to insist on the latter. As I see it, struct ethtool_cmd is > for Ethernet PHY settings. (The max{rx,tx}pkt fields weren't, but > they're obsolete.) OK, that's what I'll do. - Greg
On Wed, 2011-07-27 at 15:17 -0700, Greg Rose wrote: > Add new set commands to configure the number of SR-IOV VFs, the > number of VM queues and spoof checking on/off switch. > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > --- > > include/linux/ethtool.h | 11 ++++++++++- > 1 files changed, 10 insertions(+), 1 deletions(-) > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > index c6e427a..c4972ba 100644 > --- a/include/linux/ethtool.h > +++ b/include/linux/ethtool.h > @@ -36,12 +36,14 @@ struct ethtool_cmd { > __u8 mdio_support; > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > + __u32 num_vfs; /* Enable SR-IOV VFs */ What are the semantics of changing this after VFs have already been set up? > + __u32 num_vmqs; /* Set number of queues for VMDq */ VMDq is an Intel proprietary name. Please specify this in generic terms. > __u16 speed_hi; /* The forced speed (upper > * bits) in Mbps. Please use > * ethtool_cmd_speed()/_set() to > * access it */ > __u8 eth_tp_mdix; > - __u8 reserved2; > + __u8 spoof_check; /* Enable/Disable anti-spoofing */ > __u32 lp_advertising; /* Features the link partner advertises */ > __u32 reserved[2]; > }; > @@ -1121,6 +1123,13 @@ struct ethtool_ops { > #define AUTONEG_DISABLE 0x00 > #define AUTONEG_ENABLE 0x01 > > +/* Enable or disable MAC and/or VLAN spoofchecking.If this is > + * set to enable, then depending on the controller capabilities > + * MAC and/or VLAN spoofing will be turned on. > + */ > +#define SPOOFCHECK_DISABLE 0x00 > +#define SPOOFCHECK_ENABLE 0x01 I'm not sure it's necessary to continue defining macros for booleans. This should not 'depend on controller capabilities'. If the administrator tries to enable this feature on a device that does not support it, the request must return an error rather than failing silently. (This is another reason to define new commands.) Ben. > /* Mode MDI or MDI-X */ > #define ETH_TP_MDI_INVALID 0x00 > #define ETH_TP_MDI 0x01 >
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Ben Hutchings > Sent: Thursday, July 28, 2011 2:20 PM > To: Rose, Gregory V > Cc: netdev@vger.kernel.org; davem@davemloft.net; Kirsher, Jeffrey T > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > On Wed, 2011-07-27 at 15:17 -0700, Greg Rose wrote: > > Add new set commands to configure the number of SR-IOV VFs, the > > number of VM queues and spoof checking on/off switch. > > > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > --- > > > > include/linux/ethtool.h | 11 ++++++++++- > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > index c6e427a..c4972ba 100644 > > --- a/include/linux/ethtool.h > > +++ b/include/linux/ethtool.h > > @@ -36,12 +36,14 @@ struct ethtool_cmd { > > __u8 mdio_support; > > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > > + __u32 num_vfs; /* Enable SR-IOV VFs */ > > What are the semantics of changing this after VFs have already been set > up? There's an example of it patch 4/4 in this set. The PF driver checks if any VFs are assigned and active and if not then it will disable and destroy all the current VFs via a call to pci_disable_sriov() and then call pci_enable_sriov() with the new number of VFs. Or, if the number of new VFs is zero then SR-IOV is left disabled on that PF. Mostly this is to accommodate customer requests to be able to set a different number of VFs per PF or to only have specified PFs enable VFs. The current usage of the max_vfs module parameter is unwieldy in this sense as you must enable SR-IOV VFs on all physical functions found during the device probe with the same number of VFs. > > > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > VMDq is an Intel proprietary name. Please specify this in generic > terms. I'll use the more generic term VM queues. > > > __u16 speed_hi; /* The forced speed (upper > > * bits) in Mbps. Please use > > * ethtool_cmd_speed()/_set() to > > * access it */ > > __u8 eth_tp_mdix; > > - __u8 reserved2; > > + __u8 spoof_check; /* Enable/Disable anti-spoofing */ > > __u32 lp_advertising; /* Features the link partner advertises */ > > __u32 reserved[2]; > > }; > > @@ -1121,6 +1123,13 @@ struct ethtool_ops { > > #define AUTONEG_DISABLE 0x00 > > #define AUTONEG_ENABLE 0x01 > > > > +/* Enable or disable MAC and/or VLAN spoofchecking.If this is > > + * set to enable, then depending on the controller capabilities > > + * MAC and/or VLAN spoofing will be turned on. > > + */ > > +#define SPOOFCHECK_DISABLE 0x00 > > +#define SPOOFCHECK_ENABLE 0x01 > > I'm not sure it's necessary to continue defining macros for booleans. OK. I tend to just follow the conventions in the code I see when I'm not that familiar with it but in this case I'll just use true and false. > > This should not 'depend on controller capabilities'. If the > administrator tries to enable this feature on a device that does not > support it, the request must return an error rather than failing > silently. (This is another reason to define new commands.) Alright, I'll fix it up to work that way. - Greg
On Jul 28, 2011, at 1:38 PM, Rose, Gregory V wrote: > >> From: Anirban Chakraborty [mailto:anirban.chakraborty@qlogic.com] >> Sent: Thursday, July 28, 2011 12:04 PM >> To: Rose, Gregory V >> Cc: David Miller; netdev; Ben Hutchings; Kirsher, Jeffrey T >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands >> >> >> On Jul 28, 2011, at 8:51 AM, Rose, Gregory V wrote: >> >> >> -----Original Message----- >> From: David Miller [mailto:davem@davemloft.net] >> Sent: Wednesday, July 27, 2011 10:28 PM >> To: Rose, Gregory V >> Cc: netdev@vger.kernel.org; bhutchings@solarflare.com; Kirsher, Jeffrey T >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands >> >> From: Greg Rose <gregory.v.rose@intel.com> >> Date: Wed, 27 Jul 2011 15:17:59 -0700 >> >> Add new set commands to configure the number of SR-IOV VFs, the >> number of VM queues and spoof checking on/off switch. >> >> Signed-off-by: Greg Rose <gregory.v.rose@intel.com> >> --- >> >> include/linux/ethtool.h | 11 ++++++++++- >> 1 files changed, 10 insertions(+), 1 deletions(-) >> >> diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h >> index c6e427a..c4972ba 100644 >> --- a/include/linux/ethtool.h >> +++ b/include/linux/ethtool.h >> @@ -36,12 +36,14 @@ struct ethtool_cmd { >> __u8 mdio_support; >> __u32 maxtxpkt; /* Tx pkts before generating tx int */ >> __u32 maxrxpkt; /* Rx pkts before generating rx int */ >> + __u32 num_vfs; /* Enable SR-IOV VFs */ >> + __u32 num_vmqs; /* Set number of queues for VMDq */ >> >> You can't change the layout of this datastructure in this way without >> breaking every ethtool binary out there. >> >> You have to find another place to add these knobs. >> >> Perhaps at the end of the ethtool_cmd structure? Something like this: >> >> /* This should work for both 32 and 64 bit userland. */ >> struct ethtool_cmd { >> __u32 cmd; >> __u32 supported; /* Features this interface supports */ >> __u32 advertising; /* Features this interface advertises */ >> __u16 speed; /* The forced speed (lower bits) in >> * Mbps. Please use >> * ethtool_cmd_speed()/_set() to >> * access it */ >> __u8 duplex; /* Duplex, half or full */ >> __u8 port; /* Which connector port */ >> __u8 phy_address; >> __u8 transceiver; /* Which transceiver to use */ >> __u8 autoneg; /* Enable or disable autonegotiation */ >> __u8 mdio_support; >> __u32 maxtxpkt; /* Tx pkts before generating tx int */ >> __u32 maxrxpkt; /* Rx pkts before generating rx int */ >> __u16 speed_hi; /* The forced speed (upper >> * bits) in Mbps. Please use >> * ethtool_cmd_speed()/_set() to >> * access it */ >> __u8 eth_tp_mdix; >> __u8 spoof_check; /* Enable/Disable anti-spoofing */ >> __u32 lp_advertising; /* Features the link partner advertises */ >> __u32 reserved[2]; >> __u32 num_vfs; /* Enable SR-IOV VFs */ >> __u32 num_vmqs; /* Set number of queues for VMDq */ >> }; >> >> If I understood it correctly, you are trying to set/unset spoofing on per >> eth interface, which could be a PF on the hypervisor or a pci passthru-ed >> VF in the linux guest. There are other security features that one could set >> for a port on the VF (lets call it vport), e.g. setting a port VLAN ID for >> a VF and specifying if the VF (VM) is allowed to send tagged/untagged >> packets, setting a vport in port mirroring mode so that the PF can monitor >> the traffic on a VF, setting a vport in promiscuous mode etc. >> >> Does it make sense to try to use ip link util to specify all these parameters, >> since ip link already does the job of setting VF properties and VF port >> profile? >> >> Any thoughts? >> > > Sure, that's a possibility too. I was considering ethtool for this since MAC addresses and VLANs are fairly specific to Ethernet whereas netlink might apply to other types of physical networks. At least that's my understanding. You could specify VF MAC and VLANs using netlink today. e.g. ip link set ethX vf # mac, vlan etc. Adding spoofing as follows would do it. ip link set ethX vf # spoof on|off Having SR-IOV features scattered among ethtool and ip link may be inconvenient for the end users. CC-ing virtualization list. > > However, I have no strong feelings about it and if community consensus is to use ip link instead then that's fine by me. > > Of course, patches implementing such would be quite welcome also. I could take a stab at it at the netlink side, if there is a consensus. -Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Anirban Chakraborty > Sent: Thursday, July 28, 2011 3:01 PM > To: Rose, Gregory V > Cc: David Miller; netdev; Ben Hutchings; Kirsher, Jeffrey T; > virtualization@lists.linux-foundation.org > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > > On Jul 28, 2011, at 1:38 PM, Rose, Gregory V wrote: > > > > >> From: Anirban Chakraborty [mailto:anirban.chakraborty@qlogic.com] > >> Sent: Thursday, July 28, 2011 12:04 PM > >> To: Rose, Gregory V > >> Cc: David Miller; netdev; Ben Hutchings; Kirsher, Jeffrey T > >> Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > >> > >> > >> If I understood it correctly, you are trying to set/unset spoofing on > per > >> eth interface, which could be a PF on the hypervisor or a pci > passthru-ed > >> VF in the linux guest. There are other security features that one > could set > >> for a port on the VF (lets call it vport), e.g. setting a port VLAN ID > for > >> a VF and specifying if the VF (VM) is allowed to send tagged/untagged > >> packets, setting a vport in port mirroring mode so that the PF can > monitor > >> the traffic on a VF, setting a vport in promiscuous mode etc. > >> > >> Does it make sense to try to use ip link util to specify all these > parameters, > >> since ip link already does the job of setting VF properties and VF > port > >> profile? > >> > >> Any thoughts? > >> > > > > Sure, that's a possibility too. I was considering ethtool for this > since MAC addresses and VLANs are fairly specific to Ethernet whereas > netlink might apply to other types of physical networks. At least that's > my understanding. > > You could specify VF MAC and VLANs using netlink today. > e.g. ip link set ethX vf # mac, vlan etc. > Adding spoofing as follows would do it. > ip link set ethX vf # spoof on|off > > Having SR-IOV features scattered among ethtool and ip link may be > inconvenient for the end users. > CC-ing virtualization list. > > > > > However, I have no strong feelings about it and if community consensus > is to use ip link instead then that's fine by me. > > > > Of course, patches implementing such would be quite welcome also. > > I could take a stab at it at the netlink side, if there is a consensus. Now that I think about it I'm seeing it more your way. I'll drop the anti-spoofing stuff from my ethtool patches. If you get the time to provide patches to netlink for anti-spoofing then that's great, otherwise I'll do it when I get done with the SR-IOV reconfig stuff. Thanks, - Greg -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2011-07-28 at 14:34 -0700, Rose, Gregory V wrote: > > -----Original Message----- > > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > > On Behalf Of Ben Hutchings > > Sent: Thursday, July 28, 2011 2:20 PM > > To: Rose, Gregory V > > Cc: netdev@vger.kernel.org; davem@davemloft.net; Kirsher, Jeffrey T > > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > > > On Wed, 2011-07-27 at 15:17 -0700, Greg Rose wrote: > > > Add new set commands to configure the number of SR-IOV VFs, the > > > number of VM queues and spoof checking on/off switch. > > > > > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > > --- > > > > > > include/linux/ethtool.h | 11 ++++++++++- > > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > > index c6e427a..c4972ba 100644 > > > --- a/include/linux/ethtool.h > > > +++ b/include/linux/ethtool.h > > > @@ -36,12 +36,14 @@ struct ethtool_cmd { > > > __u8 mdio_support; > > > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > > > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > > > + __u32 num_vfs; /* Enable SR-IOV VFs */ > > > > What are the semantics of changing this after VFs have already been set > > up? > > There's an example of it patch 4/4 in this set. The PF driver checks > if any VFs are assigned and active and if not then it will disable and > destroy all the current VFs via a call to pci_disable_sriov() and then > call pci_enable_sriov() with the new number of VFs. Or, if the number > of new VFs is zero then SR-IOV is left disabled on that PF. And otherwise the request fails? > Mostly this is to accommodate customer requests to be able to set a > different number of VFs per PF or to only have specified PFs enable > VFs. The current usage of the max_vfs module parameter is unwieldy in > this sense as you must enable SR-IOV VFs on all physical functions > found during the device probe with the same number of VFs. Right, this makes a lot of sense. > > > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > > > VMDq is an Intel proprietary name. Please specify this in generic > > terms. > > I'll use the more generic term VM queues. [...] Still ambiguous. I think what you actually intend is that this will be the number of RX queues and the number of TX queues per VF. Right? You might want to allow for different numbers of RX and TX queues. Ben.
> -----Original Message----- > From: Ben Hutchings [mailto:bhutchings@solarflare.com] > Sent: Thursday, July 28, 2011 3:04 PM > To: Rose, Gregory V > Cc: netdev@vger.kernel.org; davem@davemloft.net; Kirsher, Jeffrey T > Subject: RE: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > On Thu, 2011-07-28 at 14:34 -0700, Rose, Gregory V wrote: > > > -----Original Message----- > > > From: netdev-owner@vger.kernel.org [mailto:netdev- > owner@vger.kernel.org] > > > On Behalf Of Ben Hutchings > > > Sent: Thursday, July 28, 2011 2:20 PM > > > To: Rose, Gregory V > > > Cc: netdev@vger.kernel.org; davem@davemloft.net; Kirsher, Jeffrey T > > > Subject: Re: [RFC net-next PATCH 3/4] ethtool: Add new set commands > > > > > > On Wed, 2011-07-27 at 15:17 -0700, Greg Rose wrote: > > > > Add new set commands to configure the number of SR-IOV VFs, the > > > > number of VM queues and spoof checking on/off switch. > > > > > > > > Signed-off-by: Greg Rose <gregory.v.rose@intel.com> > > > > --- > > > > > > > > include/linux/ethtool.h | 11 ++++++++++- > > > > 1 files changed, 10 insertions(+), 1 deletions(-) > > > > > > > > diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h > > > > index c6e427a..c4972ba 100644 > > > > --- a/include/linux/ethtool.h > > > > +++ b/include/linux/ethtool.h > > > > @@ -36,12 +36,14 @@ struct ethtool_cmd { > > > > __u8 mdio_support; > > > > __u32 maxtxpkt; /* Tx pkts before generating tx int */ > > > > __u32 maxrxpkt; /* Rx pkts before generating rx int */ > > > > + __u32 num_vfs; /* Enable SR-IOV VFs */ > > > > > > What are the semantics of changing this after VFs have already been > set > > > up? > > > > There's an example of it patch 4/4 in this set. The PF driver checks > > if any VFs are assigned and active and if not then it will disable and > > destroy all the current VFs via a call to pci_disable_sriov() and then > > call pci_enable_sriov() with the new number of VFs. Or, if the number > > of new VFs is zero then SR-IOV is left disabled on that PF. > > And otherwise the request fails? Yes, it returns -EBUSY if VFs are in use (assigned to guest VMs) or the PF driver is up and running. It can return other errors from the post configuration setup of interrupt resources. > > > Mostly this is to accommodate customer requests to be able to set a > > different number of VFs per PF or to only have specified PFs enable > > VFs. The current usage of the max_vfs module parameter is unwieldy in > > this sense as you must enable SR-IOV VFs on all physical functions > > found during the device probe with the same number of VFs. > > Right, this makes a lot of sense. > > > > > + __u32 num_vmqs; /* Set number of queues for VMDq */ > > > > > > VMDq is an Intel proprietary name. Please specify this in generic > > > terms. > > > > I'll use the more generic term VM queues. > [...] > > Still ambiguous. I think what you actually intend is that this will be > the number of RX queues and the number of TX queues per VF. Right? > You might want to allow for different numbers of RX and TX queues. OK, that's not a problem. Our devices only support matching numbers of queue pairs but I suppose other devices might have the ability to have different numbers of Rx/Tx queues per VF. I'll rework it that way while I'm adding the new command. The number of queues might also be used in a scenario similar to the VMware Net Queues (TM) feature. There is no support for that in Linux at the moment but it could be supported in the future. - Greg
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h index c6e427a..c4972ba 100644 --- a/include/linux/ethtool.h +++ b/include/linux/ethtool.h @@ -36,12 +36,14 @@ struct ethtool_cmd { __u8 mdio_support; __u32 maxtxpkt; /* Tx pkts before generating tx int */ __u32 maxrxpkt; /* Rx pkts before generating rx int */ + __u32 num_vfs; /* Enable SR-IOV VFs */ + __u32 num_vmqs; /* Set number of queues for VMDq */ __u16 speed_hi; /* The forced speed (upper * bits) in Mbps. Please use * ethtool_cmd_speed()/_set() to * access it */ __u8 eth_tp_mdix; - __u8 reserved2; + __u8 spoof_check; /* Enable/Disable anti-spoofing */ __u32 lp_advertising; /* Features the link partner advertises */ __u32 reserved[2]; }; @@ -1121,6 +1123,13 @@ struct ethtool_ops { #define AUTONEG_DISABLE 0x00 #define AUTONEG_ENABLE 0x01 +/* Enable or disable MAC and/or VLAN spoofchecking.If this is + * set to enable, then depending on the controller capabilities + * MAC and/or VLAN spoofing will be turned on. + */ +#define SPOOFCHECK_DISABLE 0x00 +#define SPOOFCHECK_ENABLE 0x01 + /* Mode MDI or MDI-X */ #define ETH_TP_MDI_INVALID 0x00 #define ETH_TP_MDI 0x01
Add new set commands to configure the number of SR-IOV VFs, the number of VM queues and spoof checking on/off switch. Signed-off-by: Greg Rose <gregory.v.rose@intel.com> --- include/linux/ethtool.h | 11 ++++++++++- 1 files changed, 10 insertions(+), 1 deletions(-) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html