Re: commit ab118da4c10a70b8437f5c90ab77adae1835963e causes ib_srpt to fail connections served by target LIO

[Leon Romanovsky <leonro@mellanox.com>]

On Sat, Mar 14, 2020 at 05:30:00PM -0400, Laurence Oberman wrote:
> Hello Bart, Leon and Max
>
> Max had reached out to me to test a new set of patches for SRQ.
> I had not tested upstream ib_srpt on an LIO target for quite a while,
> only ib_srp client tests had been run of late.
> During a baseline test before applying Max's patches it was apparent
> that something had broken ib_srpt connections within LIO target since
> 5.5.
>
> Note thet ib_srp client connectivity with the commit functions fine,
> it's just the target that breaks with this commit.
>
> After a long bisect this is the commit that seems to break it.
> While it's not directly code in ib_srpt, its code in mlx5 vport
> ethernet connectivity that then breaks ib_srpt connectivity over mlx5
> IB RDMA with LIO.
>
> I will let Leon and others decide but reverting the below commit allows
> SRP connectivity to an LIO target to work again.

Thanks Laurence,
It is very strange, according to our HW specification (PRM), there are
limited number of fields which can be overwritten and it is determined
by the field_select field.

From the PRM:
" A bitmask to each field in the context which informs the following
• In QUERY if fields are RO or RW
• In MODIFY indicates which fields to modify
Each bit in field_select has this information for a specific field in
context. Value 0 - means RO. Value 1 - means RW. ‘
field_select bitmask
Bit 0: port_guid
Bit 1: node_guid
Bit 2: vport_state_policy
Bit 3: min_wqe_inline_mode - used only by vport_group_manager
to configure other ports (not its own vport)
Bit 4: grh_required - Allows PFs to determine the VF VPort type.
Bit 5: system_image_guid - allows vport group manager to modify
the system_image_guid of its VFs "

Bits 0, 1 and 3 are handled in the code.
Bits 4 and 5 were not set before too.

I'll take offline with Max to see which extra field was RW without
corresponding field_select field.

Thanks

>
> Max, I will test your new patches once we have a decision on this.
>
> Client
> Linux ibclient.lab.eng.bos.redhat.com 5.6.0-rc5+ #1 SMP Thu Mar 12
> 16:58:19 EDT 2020 x86_64 x86_64 x86_64 GNU/Linux
>
> Server with reverted commit
> Linux fedstorage.bos.redhat.com 5.6.0-rc5+ #1 SMP Sat Mar 14 16:39:35
> EDT 2020 x86_64 x86_64 x86_64 GNU/Linux
>
> commit ab118da4c10a70b8437f5c90ab77adae1835963e
> Author: Leon Romanovsky <leonro@mellanox.com>
> Date:   Wed Nov 13 12:03:47 2019 +0200
>
>     net/mlx5: Don't write read-only fields in MODIFY_HCA_VPORT_CONTEXT
> command
>
>     The MODIFY_HCA_VPORT_CONTEXT uses field_selector to mask fields
> needed
>     to be written, other fields are required to be zero according to
> the
>     HW specification. The supported fields are controlled by bitfield
>     and limited to vport state, node and port GUIDs.
>
>     Signed-off-by: Leon Romanovsky <leonro@mellanox.com>
>     Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> b/drivers/net/ethernet/mellanox/mlx5
> index 30f7848..1faac31f 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/vport.c
> @@ -1064,26 +1064,13 @@ int mlx5_core_modify_hca_vport_context(struct
> mlx5_core_dev *dev,
>
>         ctx = MLX5_ADDR_OF(modify_hca_vport_context_in, in,
> hca_vport_context);
>         MLX5_SET(hca_vport_context, ctx, field_select, req-
> >field_select);
> -       MLX5_SET(hca_vport_context, ctx, sm_virt_aware, req-
> >sm_virt_aware);
> -       MLX5_SET(hca_vport_context, ctx, has_smi, req->has_smi);
> -       MLX5_SET(hca_vport_context, ctx, has_raw, req->has_raw);
> -       MLX5_SET(hca_vport_context, ctx, vport_state_policy, req-
> >policy);
> -       MLX5_SET(hca_vport_context, ctx, port_physical_state, req-
> >phys_state);
> -       MLX5_SET(hca_vport_context, ctx, vport_state, req-
> >vport_state);
> -       MLX5_SET64(hca_vport_context, ctx, port_guid, req->port_guid);
> -       MLX5_SET64(hca_vport_context, ctx, node_guid, req->node_guid);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask1, req->cap_mask1);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask1_field_select, req-
> >cap_mask1_perm);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask2, req->cap_mask2);
> -       MLX5_SET(hca_vport_context, ctx, cap_mask2_field_select, req-
> >cap_mask2_perm);
> -       MLX5_SET(hca_vport_context, ctx, lid, req->lid);
> -       MLX5_SET(hca_vport_context, ctx, init_type_reply, req-
> >init_type_reply);
> -       MLX5_SET(hca_vport_context, ctx, lmc, req->lmc);
> -       MLX5_SET(hca_vport_context, ctx, subnet_timeout, req-
> >subnet_timeout);
> -       MLX5_SET(hca_vport_context, ctx, sm_lid, req->sm_lid);
> -       MLX5_SET(hca_vport_context, ctx, sm_sl, req->sm_sl);
> -       MLX5_SET(hca_vport_context, ctx, qkey_violation_counter, req-
> >qkey_violation_counter);
> -       MLX5_SET(hca_vport_context, ctx, pkey_violation_counter, req-
> >pkey_violation_counter);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_STATE_POLICY)
> +               MLX5_SET(hca_vport_context, ctx, vport_state_policy,
> +                        req->policy);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_PORT_GUID)
> +               MLX5_SET64(hca_vport_context, ctx, port_guid, req-
> >port_guid);
> +       if (req->field_select & MLX5_HCA_VPORT_SEL_NODE_GUID)
> +               MLX5_SET64(hca_vport_context, ctx, node_guid, req-
> >node_guid);
>         err = mlx5_cmd_exec(dev, in, in_sz, out, sizeof(out));
>  ex:
>         kfree(in);
>
>