Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-06 Thread Or Gerlitz 
On Fri, Mar 4, 2016 at 4:35 PM, Doug Ledford  wrote:

[...]
> So, at *best*, the solution you are suggesting for existing setups is a
> partial solution that leaves things only half done.
[...]

Doug,

Point/s taken, justifying the code re-route of libvirt attempting to
issue set_vf_mac on the PF to go through
set_vf_guid needs handling of more aspects. We will remove it from this patch.

Or.

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-04 Thread Doug Ledford 
On 03/02/2016 01:40 PM, Or Gerlitz wrote:
> On Wed, Mar 2, 2016 at 6:50 PM, Doug Ledford  wrote:
> 
>> Exactly *what* provisioning system tries to set the VF_MAC on an IPoIB
>> interface and expects it to set the GUID of an underlying IB device?
> 
> The provisioning system need not be fully aware in all their
> components this is IB here, there's PCI linkage that tells these are
> VFs of this PF and they have to be used for these VMs.

If I understand you correctly, then I don't think I agree.

From what I read, I gather you mean:

libvirt can be used to control guests today, and you can list a PCI
device as "managed" and specify a MAC address (which has libvirt
assuming the device is an ethernet device).  In that case, libvirt
automatically detaches the device from the host (if attached), figures
out if it's a PF or VF, sets the MAC address using either PF or VF MAC
setting methods in ethtool, attaches the device to the guest, then
starts the guest.  And you're saying we should put the MAC->GUID
transformation into this code for IB so that libvirt can be blissfully
ignorant and people can tell libvirt it's an ethernet device with a MAC
and libvirt will treat it as such and life will be grand.

Except it won't.  Along with setting the GUID, we also need to set the
P_Keys allowed list (at least using the alias GUIDs method of mlx4 you
do, so unless you add a patch to this series to switch mlx4 to this new
method, that's a valid concern).  And nothing in libvirt can do that as
long libvirt thinks this is ethernet because libvirt doesn't control the
vlans on a guest's ethernet device, the guest does.  In that sense, IB
and ethernet vary greatly.

So, at *best*, the solution you are suggesting for existing setups is a
partial solution that leaves things only half done.

I don't see the justification to clutter up upstream code with a
solution that isn't at least completely functional in its
implementation.  If the solution is only partial, then I would rather
leave it out and tell people to upgrade their libvirt to know about IB
devices.

This is actually further backed up, in my mind, by the fact that you can
have RoCE/iWARP Ethernet devices and regular Ethernet devices, and
libvirt needs to be taught the concept of an RDMA capable device,
whether Ethernet or IB, so that when trying to select a host for
migration it can make sure that the migration target has the same
capabilities as the hardware you are migrating from.  So, to me, doing
this right *requires* a libvirt upgrade, and there is no sense in this
middle ground GUID from MAC hack that you are suggesting.

>>> along with the fully IB aware solution where the
>>> upper level does provision IB GUIDs.
> 
>> There has never been upstream support for this MAC->GUID stuff you refer
>> to.  I'm not convinced we should add it now versus just doing things
>> right, period.
> 
> We **are** doing things right with the new ndo.
> 
> Using the small MAC->GUID addition, people could be using non-modified
> (or almost non-modified) provisioning systems that assign SRIOV VMs
> with a MACs --- just use these patches on their hosts and get DHCP
> server supplying IP addresses based on the derived GUIDs (this is
> supported today).
> 


-- 
Doug Ledford 
  GPG KeyID: 0E572FDD




signature.asc
Description: OpenPGP digital signature

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-02 Thread Or Gerlitz 
On Wed, Mar 2, 2016 at 6:50 PM, Doug Ledford  wrote:

> Exactly *what* provisioning system tries to set the VF_MAC on an IPoIB
> interface and expects it to set the GUID of an underlying IB device?

The provisioning system need not be fully aware in all their
components this is IB here, there's PCI linkage that tells these are
VFs of this PF and they have to be used for these VMs.

>> along with the fully IB aware solution where the
>> upper level does provision IB GUIDs.

> There has never been upstream support for this MAC->GUID stuff you refer
> to.  I'm not convinced we should add it now versus just doing things
> right, period.

We **are** doing things right with the new ndo.

Using the small MAC->GUID addition, people could be using non-modified
(or almost non-modified) provisioning systems that assign SRIOV VMs
with a MACs --- just use these patches on their hosts and get DHCP
server supplying IP addresses based on the derived GUIDs (this is
supported today).

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-02 Thread Doug Ledford 
On 3/1/2016 4:08 PM, Or Gerlitz wrote:
> On Tue, Mar 1, 2016 at 8:25 PM, Jason Gunthorpe
>  wrote:
>> On Tue, Mar 01, 2016 at 07:49:51PM +0200, Eli Cohen wrote:
>>> On Tue, Mar 01, 2016 at 10:37:51AM -0700, Jason Gunthorpe wrote:
> + return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);

 But is this emulation really necessary? It seems dangerous and
 continues the bad practice of assuming IFLA_VF_MAC is fixed to 6 bytes
 in size, and is not just LLADDR bytes. I'd rather see mac sets fail on
 IB.

>>> struct ifla_vf_mac  already defines mac as 32 bytes but the idea here
>>> is that applications that configure six byte Ethernet MACs to VFs will
>>> continue to work without any change.
>>
>> In my view it is incorrect for an application to try and set a 6 byte
>> mac on an *infiniband* interface, the kernel should refuse to do it.
> 
> As Eli wrote, there's a well defined way to extend MAC to GUID. With
> that in hand, the idea here is to allow staged/evolved support for IB
> Virtualization using un-touched provisioning systems which assign VMs
> with 6-byte MACs

Exactly *what* provisioning system tries to set the VF_MAC on an IPoIB
interface and expects it to set the GUID of an underlying IB device?

> along with the fully IB aware solution where the
> upper level does provision IB GUIDs.

There has never been upstream support for this MAC->GUID stuff you refer
to.  I'm not convinced we should add it now versus just doing things
right, period.




signature.asc
Description: OpenPGP digital signature

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-01 Thread Or Gerlitz 
On Tue, Mar 1, 2016 at 8:25 PM, Jason Gunthorpe
 wrote:
> On Tue, Mar 01, 2016 at 07:49:51PM +0200, Eli Cohen wrote:
>> On Tue, Mar 01, 2016 at 10:37:51AM -0700, Jason Gunthorpe wrote:
>> > > + return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);
>> >
>> > But is this emulation really necessary? It seems dangerous and
>> > continues the bad practice of assuming IFLA_VF_MAC is fixed to 6 bytes
>> > in size, and is not just LLADDR bytes. I'd rather see mac sets fail on
>> > IB.
>> >
>> struct ifla_vf_mac  already defines mac as 32 bytes but the idea here
>> is that applications that configure six byte Ethernet MACs to VFs will
>> continue to work without any change.
>
> In my view it is incorrect for an application to try and set a 6 byte
> mac on an *infiniband* interface, the kernel should refuse to do it.

As Eli wrote, there's a well defined way to extend MAC to GUID. With
that in hand, the idea here is to allow staged/evolved support for IB
Virtualization using un-touched provisioning systems which assign VMs
with 6-byte MACs along with the fully IB aware solution where the
upper level does provision IB GUIDs.

Or.

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-01 Thread Jason Gunthorpe 
On Tue, Mar 01, 2016 at 07:49:51PM +0200, Eli Cohen wrote:
> On Tue, Mar 01, 2016 at 10:37:51AM -0700, Jason Gunthorpe wrote:
> > > + return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);
> > 
> > But is this emulation really necessary? It seems dangerous and
> > continues the bad practice of assuming IFLA_VF_MAC is fixed to 6 bytes
> > in size, and is not just LLADDR bytes. I'd rather see mac sets fail on
> > IB.
> > 
> struct ifla_vf_mac  already defines mac as 32 bytes but the idea here
> is that applications that configure six byte Ethernet MACs to VFs will
> continue to work without any change. 

In my view it is incorrect for an application to try and set a 6 byte
mac on an *infiniband* interface, the kernel should refuse to do it.

Jason

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-01 Thread Eli Cohen 
On Tue, Mar 01, 2016 at 10:37:51AM -0700, Jason Gunthorpe wrote:
> > +   return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);
> 
> But is this emulation really necessary? It seems dangerous and
> continues the bad practice of assuming IFLA_VF_MAC is fixed to 6 bytes
> in size, and is not just LLADDR bytes. I'd rather see mac sets fail on
> IB.
> 
struct ifla_vf_mac  already defines mac as 32 bytes but the idea here
is that applications that configure six byte Ethernet MACs to VFs will
continue to work without any change.

Re: [PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-01 Thread Jason Gunthorpe 
On Tue, Mar 01, 2016 at 06:52:14PM +0200, Eli Cohen wrote:
> Add two new NLAs to support configuration of Infiniband node or port
> GUIDs. New applications can choose to use this interface to configure
> GUIDs with iproute2 with commands such as:
> 
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
> ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78

I like this idea better than the last version..

> +static int handle_vf_mac(struct net_device *dev, struct ifla_vf_mac *ivm)
> +{
[..]
> + return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);

But is this emulation really necessary? It seems dangerous and
continues the bad practice of assuming IFLA_VF_MAC is fixed to 6 bytes
in size, and is not just LLADDR bytes. I'd rather see mac sets fail on
IB.

Jason

[PATCH for-next 01/10] net/core: Add support for configuring VF GUIDs

2016-03-01 Thread Eli Cohen 
Add two new NLAs to support configuration of Infiniband node or port
GUIDs. New applications can choose to use this interface to configure
GUIDs with iproute2 with commands such as:

ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78

For backwards compatibility, old applications which set the MAC of a VF
may set the VF's port GUID for an infiniband port also via set MAC. The
GUID will be generated from the 6-byte MAC per IETF RFC 7042. Note that
when using set MAC to set a port GUID, the node GUID is set as well (to
the port guid value).

A new ndo, ndo_sef_vf_guid is introduced to notify the net device of the
request to change the GUID.

Signed-off-by: Eli Cohen 
---
 include/linux/netdevice.h|  3 ++
 include/uapi/linux/if_link.h |  7 
 net/core/rtnetlink.c | 79 
 3 files changed, 83 insertions(+), 6 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 5440b7b705eb..7b4ae218b90b 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1147,6 +1147,9 @@ struct net_device_ops {
   struct nlattr *port[]);
int (*ndo_get_vf_port)(struct net_device *dev,
   int vf, struct sk_buff *skb);
+   int (*ndo_set_vf_guid)(struct net_device *dev,
+  int vf, u64 guid,
+  int guid_type);
int (*ndo_set_vf_rss_query_en)(
   struct net_device *dev,
   int vf, bool setting);
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index a30b78090594..1d01e8a4e5dd 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -556,6 +556,8 @@ enum {
 */
IFLA_VF_STATS,  /* network device statistics */
IFLA_VF_TRUST,  /* Trust VF */
+   IFLA_VF_IB_NODE_GUID,   /* VF Infiniband node GUID */
+   IFLA_VF_IB_PORT_GUID,   /* VF Infiniband port GUID */
__IFLA_VF_MAX,
 };
 
@@ -588,6 +590,11 @@ struct ifla_vf_spoofchk {
__u32 setting;
 };
 
+struct ifla_vf_guid {
+   __u32 vf;
+   __u64 guid;
+};
+
 enum {
IFLA_VF_LINK_STATE_AUTO,/* link state of the uplink */
IFLA_VF_LINK_STATE_ENABLE,  /* link always up */
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index d735e854f916..9db6e5bde786 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -1387,6 +1387,8 @@ static const struct nla_policy 
ifla_vf_policy[IFLA_VF_MAX+1] = {
[IFLA_VF_RSS_QUERY_EN]  = { .len = sizeof(struct ifla_vf_rss_query_en) 
},
[IFLA_VF_STATS] = { .type = NLA_NESTED },
[IFLA_VF_TRUST] = { .len = sizeof(struct ifla_vf_trust) },
+   [IFLA_VF_IB_NODE_GUID]  = { .len = sizeof(struct ifla_vf_guid) },
+   [IFLA_VF_IB_PORT_GUID]  = { .len = sizeof(struct ifla_vf_guid) },
 };
 
 static const struct nla_policy ifla_vf_stats_policy[IFLA_VF_STATS_MAX + 1] = {
@@ -1534,6 +1536,58 @@ static int validate_linkmsg(struct net_device *dev, 
struct nlattr *tb[])
return 0;
 }
 
+static int handle_infiniband_guid(struct net_device *dev, struct ifla_vf_guid 
*ivt,
+ int guid_type)
+{
+   const struct net_device_ops *ops = dev->netdev_ops;
+
+   return ops->ndo_set_vf_guid(dev, ivt->vf, ivt->guid, guid_type);
+}
+
+static int handle_vf_guid(struct net_device *dev, struct ifla_vf_guid *ivt, 
int guid_type)
+{
+   if (dev->type != ARPHRD_INFINIBAND)
+   return -EOPNOTSUPP;
+
+   return handle_infiniband_guid(dev, ivt, guid_type);
+}
+
+static int handle_vf_mac(struct net_device *dev, struct ifla_vf_mac *ivm)
+{
+   const struct net_device_ops *ops = dev->netdev_ops;
+   struct ifla_vf_guid ivt;
+   u8 *s = ivm->mac;
+   u8 d[8];
+   int err;
+
+   if (dev->type != ARPHRD_INFINIBAND) {
+   if (!ops->ndo_set_vf_mac)
+   return -EOPNOTSUPP;
+
+   return ops->ndo_set_vf_mac(dev, ivm->vf, ivm->mac);
+   }
+
+   if (!ops->ndo_set_vf_guid)
+   return -EOPNOTSUPP;
+
+   d[0] = s[0];
+   d[1] = s[1];
+   d[2] = s[2];
+   d[3] = 0xff;
+   d[4] = 0xfe;
+   d[5] = s[3];
+   d[6] = s[4];
+   d[7] = s[5];
+
+   ivt.vf = ivm->vf;
+   ivt.guid = be64_to_cpu(*(__be64 *)d);
+   err = handle_infiniband_guid(dev, , IFLA_VF_IB_NODE_GUID);
+   if (err)
+   return err;
+
+   return handle_infiniband_guid(dev, , IFLA_VF_IB_PORT_GUID);
+}
+
 static int do_setvfinfo(struct net_device *dev, struct nlattr **tb)
 {