[PATCH rdma-next 0/6] Set flow_label and RoCEv2 UDP source port for datagram QP

[PATCH rdma-next 0/6] Set flow_label and RoCEv2 UDP source port for datagram QP

[Leon Romanovsky]

From: Leon Romanovsky <leonro@mellanox.com>

From Mark:

This series provide flow label and UDP source port definition in RoCE v2.
Those fields are used to create entropy for network routes (ECMP), load
balancers and 802.3ad link aggregation switching that are not aware of
RoCE headers.

Thanks.

Mark Zhang (6):
  net/mlx5: Enable SW-defined RoCEv2 UDP source port
  RDMA/core: Add hash functions to calculate RoCEv2 flowlabel and UDP
    source port
  RDMA/mlx5: Define RoCEv2 udp source port when set path
  RDMA/cma: Initialize the flow label of CM's route path record
  RDMA/cm: Set flow label of recv_wc based on primary flow label
  RDMA/mlx5: Set UDP source port based on the grh.flow_label

 drivers/infiniband/core/cm.c                  |  7 +++
 drivers/infiniband/core/cma.c                 | 23 ++++++++++
 drivers/infiniband/hw/mlx5/ah.c               | 21 ++++++++-
 drivers/infiniband/hw/mlx5/main.c             |  4 +-
 drivers/infiniband/hw/mlx5/mlx5_ib.h          |  4 +-
 drivers/infiniband/hw/mlx5/qp.c               | 30 ++++++++++---
 .../net/ethernet/mellanox/mlx5/core/main.c    | 39 ++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h                 |  5 ++-
 include/rdma/ib_verbs.h                       | 44 +++++++++++++++++++
 9 files changed, 164 insertions(+), 13 deletions(-)

--
2.24.1

[PATCH mlx5-next 1/6] net/mlx5: Enable SW-defined RoCEv2 UDP source port

[Leon Romanovsky]

From: Mark Zhang <markz@mellanox.com>

When this is enabled, UDP source port for RoCEv2 packets are defined
by software instead of firmware.

Signed-off-by: Mark Zhang <markz@mellanox.com>
Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
---
 .../net/ethernet/mellanox/mlx5/core/main.c    | 39 +++++++++++++++++++
 include/linux/mlx5/mlx5_ifc.h                 |  5 ++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c b/drivers/net/ethernet/mellanox/mlx5/core/main.c
index 6b38ec72215a..bdc73370297b 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
@@ -585,6 +585,39 @@ static int handle_hca_cap(struct mlx5_core_dev *dev)
 	return err;
 }
 
+static int handle_hca_cap_roce(struct mlx5_core_dev *dev)
+{
+	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
+	void *set_hca_cap;
+	void *set_ctx;
+	int err;
+
+	if (!MLX5_CAP_GEN(dev, roce))
+		return 0;
+
+	err = mlx5_core_get_caps(dev, MLX5_CAP_ROCE);
+	if (err)
+		return err;
+
+	if (MLX5_CAP_ROCE(dev, sw_r_roce_src_udp_port) ||
+	    !MLX5_CAP_ROCE_MAX(dev, sw_r_roce_src_udp_port))
+		return 0;
+
+	set_ctx = kzalloc(set_sz, GFP_KERNEL);
+	if (!set_ctx)
+		return -ENOMEM;
+
+	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx, capability);
+	memcpy(set_hca_cap, dev->caps.hca_cur[MLX5_CAP_ROCE],
+	       MLX5_ST_SZ_BYTES(roce_cap));
+	MLX5_SET(roce_cap, set_hca_cap, sw_r_roce_src_udp_port, 1);
+
+	err = set_caps(dev, set_ctx, set_sz, MLX5_SET_HCA_CAP_OP_MOD_ROCE);
+
+	kfree(set_ctx);
+	return err;
+}
+
 static int set_hca_cap(struct mlx5_core_dev *dev)
 {
 	int err;
@@ -607,6 +640,12 @@ static int set_hca_cap(struct mlx5_core_dev *dev)
 		goto out;
 	}
 
+	err = handle_hca_cap_roce(dev);
+	if (err) {
+		mlx5_core_err(dev, "handle_hca_cap_roce failed\n");
+		goto out;
+	}
+
 out:
 	return err;
 }
diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 208bf1127be7..bb217c3f30da 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -74,6 +74,7 @@ enum {
 	MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE        = 0x0,
 	MLX5_SET_HCA_CAP_OP_MOD_ODP                   = 0x2,
 	MLX5_SET_HCA_CAP_OP_MOD_ATOMIC                = 0x3,
+	MLX5_SET_HCA_CAP_OP_MOD_ROCE                  = 0x4,
 };
 
 enum {
@@ -902,7 +903,9 @@ struct mlx5_ifc_per_protocol_networking_offload_caps_bits {
 
 struct mlx5_ifc_roce_cap_bits {
 	u8         roce_apm[0x1];
-	u8         reserved_at_1[0x1f];
+	u8         reserved_at_1[0x3];
+	u8         sw_r_roce_src_udp_port[0x1];
+	u8         reserved_at_5[0x1b];
 
 	u8         reserved_at_20[0x60];
 
-- 
2.24.1

Re: [PATCH mlx5-next 1/6] net/mlx5: Enable SW-defined RoCEv2 UDP source port

[Saeed Mahameed]

On Wed, 2020-03-18 at 11:52 +0200, Leon Romanovsky wrote:
> From: Mark Zhang <markz@mellanox.com>
> 
> When this is enabled, UDP source port for RoCEv2 packets are defined
> by software instead of firmware.
> 
> Signed-off-by: Mark Zhang <markz@mellanox.com>
> Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> ---
>  .../net/ethernet/mellanox/mlx5/core/main.c    | 39
> +++++++++++++++++++
>  include/linux/mlx5/mlx5_ifc.h                 |  5 ++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> index 6b38ec72215a..bdc73370297b 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> @@ -585,6 +585,39 @@ static int handle_hca_cap(struct mlx5_core_dev
> *dev)
>  	return err;
>  }
>  
> +static int handle_hca_cap_roce(struct mlx5_core_dev *dev)
> +{
> +	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> +	void *set_hca_cap;
> +	void *set_ctx;
> +	int err;
> +
> +	if (!MLX5_CAP_GEN(dev, roce))
> +		return 0;
> +
> +	err = mlx5_core_get_caps(dev, MLX5_CAP_ROCE);
> +	if (err)
> +		return err;
> +
> +	if (MLX5_CAP_ROCE(dev, sw_r_roce_src_udp_port) ||
> +	    !MLX5_CAP_ROCE_MAX(dev, sw_r_roce_src_udp_port))
> +		return 0;
> +
> +	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> +	if (!set_ctx)
> +		return -ENOMEM;
> +

all the sisters of this function allocate this and free it
consecutively, why not allocate it from outside once, pass it to all
handle_hca_cap_xyz functions, each one will memset it and reuse it.
see below.

> +	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> capability);
> +	memcpy(set_hca_cap, dev->caps.hca_cur[MLX5_CAP_ROCE],
> +	       MLX5_ST_SZ_BYTES(roce_cap));
> +	MLX5_SET(roce_cap, set_hca_cap, sw_r_roce_src_udp_port, 1);
> +
> +	err = set_caps(dev, set_ctx, set_sz,
> MLX5_SET_HCA_CAP_OP_MOD_ROCE);
> +

Do we really need to fail the whole driver if we just try to set a non
mandatory cap ?

> +	kfree(set_ctx);
> +	return err;
> +}
> +
>  static int set_hca_cap(struct mlx5_core_dev *dev)
>  {
>  	int err;

let's allocate the set_ctx in this parent function and pass it to all
hca cap handlers;

set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
set_ctx = kzalloc(set_sz, GFP_KERNEL);

> @@ -607,6 +640,12 @@ static int set_hca_cap(struct mlx5_core_dev
> *dev)
>  		goto out;
>  	}
>  
> +	err = handle_hca_cap_roce(dev);
> +	if (err) {
> +		mlx5_core_err(dev, "handle_hca_cap_roce failed\n");
> +		goto out;
> +	}
> +
>  out:
>  	return err;
>  }
> diff --git a/include/linux/mlx5/mlx5_ifc.h
> b/include/linux/mlx5/mlx5_ifc.h
> index 208bf1127be7..bb217c3f30da 100644
> --- a/include/linux/mlx5/mlx5_ifc.h
> +++ b/include/linux/mlx5/mlx5_ifc.h
> @@ -74,6 +74,7 @@ enum {
>  	MLX5_SET_HCA_CAP_OP_MOD_GENERAL_DEVICE        = 0x0,
>  	MLX5_SET_HCA_CAP_OP_MOD_ODP                   = 0x2,
>  	MLX5_SET_HCA_CAP_OP_MOD_ATOMIC                = 0x3,
> +	MLX5_SET_HCA_CAP_OP_MOD_ROCE                  = 0x4,
>  };
>  
>  enum {
> @@ -902,7 +903,9 @@ struct
> mlx5_ifc_per_protocol_networking_offload_caps_bits {
>  
>  struct mlx5_ifc_roce_cap_bits {
>  	u8         roce_apm[0x1];
> -	u8         reserved_at_1[0x1f];
> +	u8         reserved_at_1[0x3];
> +	u8         sw_r_roce_src_udp_port[0x1];
> +	u8         reserved_at_5[0x1b];
>  
>  	u8         reserved_at_20[0x60];
>  

Re: [PATCH mlx5-next 1/6] net/mlx5: Enable SW-defined RoCEv2 UDP source port

[Leon Romanovsky]

On Wed, Mar 18, 2020 at 11:33:46PM +0000, Saeed Mahameed wrote:
> On Wed, 2020-03-18 at 11:52 +0200, Leon Romanovsky wrote:
> > From: Mark Zhang <markz@mellanox.com>
> >
> > When this is enabled, UDP source port for RoCEv2 packets are defined
> > by software instead of firmware.
> >
> > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > ---
> >  .../net/ethernet/mellanox/mlx5/core/main.c    | 39
> > +++++++++++++++++++
> >  include/linux/mlx5/mlx5_ifc.h                 |  5 ++-
> >  2 files changed, 43 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > index 6b38ec72215a..bdc73370297b 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > @@ -585,6 +585,39 @@ static int handle_hca_cap(struct mlx5_core_dev
> > *dev)
> >  	return err;
> >  }
> >
> > +static int handle_hca_cap_roce(struct mlx5_core_dev *dev)
> > +{
> > +	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> > +	void *set_hca_cap;
> > +	void *set_ctx;
> > +	int err;
> > +
> > +	if (!MLX5_CAP_GEN(dev, roce))
> > +		return 0;
> > +
> > +	err = mlx5_core_get_caps(dev, MLX5_CAP_ROCE);
> > +	if (err)
> > +		return err;
> > +
> > +	if (MLX5_CAP_ROCE(dev, sw_r_roce_src_udp_port) ||
> > +	    !MLX5_CAP_ROCE_MAX(dev, sw_r_roce_src_udp_port))
> > +		return 0;
> > +
> > +	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> > +	if (!set_ctx)
> > +		return -ENOMEM;
> > +
>
> all the sisters of this function allocate this and free it
> consecutively, why not allocate it from outside once, pass it to all
> handle_hca_cap_xyz functions, each one will memset it and reuse it.
> see below.

Agree, I'll do it.

>
> > +	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> > capability);
> > +	memcpy(set_hca_cap, dev->caps.hca_cur[MLX5_CAP_ROCE],
> > +	       MLX5_ST_SZ_BYTES(roce_cap));
> > +	MLX5_SET(roce_cap, set_hca_cap, sw_r_roce_src_udp_port, 1);
> > +
> > +	err = set_caps(dev, set_ctx, set_sz,
> > MLX5_SET_HCA_CAP_OP_MOD_ROCE);
> > +
>
> Do we really need to fail the whole driver if we just try to set a non
> mandatory cap ?

It is less important what caused to failure, but the fact that basic
mlx5_cmd_exec() failed during initialization flow. I think that it
is bad enough to stop the driver, because its operation is going to
be unreliable.

Please share your end-result decision on that and I'll align to it.

>
> > +	kfree(set_ctx);
> > +	return err;
> > +}
> > +
> >  static int set_hca_cap(struct mlx5_core_dev *dev)
> >  {
> >  	int err;
>
> let's allocate the set_ctx in this parent function and pass it to all
> hca cap handlers;
>
> set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> set_ctx = kzalloc(set_sz, GFP_KERNEL);

I'm doing it now.

Thanks

Re: [PATCH mlx5-next 1/6] net/mlx5: Enable SW-defined RoCEv2 UDP source port

[Saeed Mahameed]

On Thu, 2020-03-19 at 08:05 +0200, Leon Romanovsky wrote:
> On Wed, Mar 18, 2020 at 11:33:46PM +0000, Saeed Mahameed wrote:
> > On Wed, 2020-03-18 at 11:52 +0200, Leon Romanovsky wrote:
> > > From: Mark Zhang <markz@mellanox.com>
> > > 
> > > When this is enabled, UDP source port for RoCEv2 packets are
> > > defined
> > > by software instead of firmware.
> > > 
> > > Signed-off-by: Mark Zhang <markz@mellanox.com>
> > > Reviewed-by: Maor Gottlieb <maorg@mellanox.com>
> > > Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
> > > ---
> > >  .../net/ethernet/mellanox/mlx5/core/main.c    | 39
> > > +++++++++++++++++++
> > >  include/linux/mlx5/mlx5_ifc.h                 |  5 ++-
> > >  2 files changed, 43 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > > b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > > index 6b38ec72215a..bdc73370297b 100644
> > > --- a/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > > +++ b/drivers/net/ethernet/mellanox/mlx5/core/main.c
> > > @@ -585,6 +585,39 @@ static int handle_hca_cap(struct
> > > mlx5_core_dev
> > > *dev)
> > >  	return err;
> > >  }
> > > 
> > > +static int handle_hca_cap_roce(struct mlx5_core_dev *dev)
> > > +{
> > > +	int set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> > > +	void *set_hca_cap;
> > > +	void *set_ctx;
> > > +	int err;
> > > +
> > > +	if (!MLX5_CAP_GEN(dev, roce))
> > > +		return 0;
> > > +
> > > +	err = mlx5_core_get_caps(dev, MLX5_CAP_ROCE);
> > > +	if (err)
> > > +		return err;
> > > +
> > > +	if (MLX5_CAP_ROCE(dev, sw_r_roce_src_udp_port) ||
> > > +	    !MLX5_CAP_ROCE_MAX(dev, sw_r_roce_src_udp_port))
> > > +		return 0;
> > > +
> > > +	set_ctx = kzalloc(set_sz, GFP_KERNEL);
> > > +	if (!set_ctx)
> > > +		return -ENOMEM;
> > > +
> > 
> > all the sisters of this function allocate this and free it
> > consecutively, why not allocate it from outside once, pass it to
> > all
> > handle_hca_cap_xyz functions, each one will memset it and reuse it.
> > see below.
> 
> Agree, I'll do it.
> 
> > > +	set_hca_cap = MLX5_ADDR_OF(set_hca_cap_in, set_ctx,
> > > capability);
> > > +	memcpy(set_hca_cap, dev->caps.hca_cur[MLX5_CAP_ROCE],
> > > +	       MLX5_ST_SZ_BYTES(roce_cap));
> > > +	MLX5_SET(roce_cap, set_hca_cap, sw_r_roce_src_udp_port, 1);
> > > +
> > > +	err = set_caps(dev, set_ctx, set_sz,
> > > MLX5_SET_HCA_CAP_OP_MOD_ROCE);
> > > +
> > 
> > Do we really need to fail the whole driver if we just try to set a
> > non
> > mandatory cap ?
> 
> It is less important what caused to failure, but the fact that basic
> mlx5_cmd_exec() failed during initialization flow. I think that it
> is bad enough to stop the driver, because its operation is going to
> be unreliable.
> 
> Please share your end-result decision on that and I'll align to it.
> 

driver stability and reliability is not affected by this failing, since
design-wise we don't count on setting the caps on this stage, we query
them anyway in the next stages of the driver load.

Many reason this could fail, old FW that doesn't handle this new CAP
properly, new FW which has a bug only in the new feature flow.
The driver should be resilient and provide basic functionality or in
this case just drop this feature, since next cap query of this feature
will return 0, and driver will not try to enable this feature anyway.

if it is something really fundamental that caused the issue, then just
let it be, if we fail in a more advanced mandatory stage then we will
fail on that stage, if we didn't, then it is a win win.


> > > +	kfree(set_ctx);
> > > +	return err;
> > > +}
> > > +
> > >  static int set_hca_cap(struct mlx5_core_dev *dev)
> > >  {
> > >  	int err;
> > 
> > let's allocate the set_ctx in this parent function and pass it to
> > all
> > hca cap handlers;
> > 
> > set_sz = MLX5_ST_SZ_BYTES(set_hca_cap_in);
> > set_ctx = kzalloc(set_sz, GFP_KERNEL);
> 
> I'm doing it now.
> 

Awesome, Thanks !