Re: [PATCH rdma-next 04/23] net/mlx5: Add macros for reading log-based caps

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The concern says MLX5_GET_LOGSZ_CLAMP(), MLX5_CAP_GEN_LOGSZ_U32()
  and MLX5_CAP_GEN_LOGSZ_U64() in include/linux/mlx5/device.h are added…

--

net/mlx5: Add macros for reading log-based caps

This commit adds MLX5_GET_LOGSZ_CLAMP() and the MLX5_CAP_GEN_LOGSZ_U32()
and MLX5_CAP_GEN_LOGSZ_U64() accessors to include/linux/mlx5/device.h.
They compute 2^cap for log2-encoded capability fields, with the shift
clamped to the largest shift that is defined for the result type.

> diff --git a/include/linux/mlx5/device.h b/include/linux/mlx5/device.h
> index 37727b5ab78f2..f45454f65d5c0 100644
> --- a/include/linux/mlx5/device.h
> +++ b/include/linux/mlx5/device.h

[ ... ]

> @@ -47,6 +48,16 @@
>  #error Host endianness not defined
>  #endif
>  
> +/* Largest shift count that is defined behaviour for a value of type @T. */
> +#define __mlx5_max_shift(T) (BITS_PER_TYPE(T) - 1 - is_signed_type(T))
> +
> +/* Calculate 2^@fld from a log2 cap field. Exponents that don't fit in @T
> + * saturate to the largest representable power of two (not to the type
> + * maximum), avoiding undefined shifts.
> + */
> +#define MLX5_GET_LOGSZ_CLAMP(typ, p, fld, T) \
> +	((T)((T)1 << min_t(u32, MLX5_GET(typ, p, fld), __mlx5_max_shift(T))))
> +

[ ... ]

> @@ -1096,6 +1107,15 @@ enum mlx5_qcam_feature_groups {
>  #define MLX5_CAP_GEN_MAX(mdev, cap) \
>  	MLX5_GET(cmd_hca_cap, mdev->caps.hca[MLX5_CAP_GENERAL]->max, cap)
>  
> +/* 2^cap for log2-encoded general caps */
> +#define MLX5_CAP_GEN_LOGSZ_U32(mdev, cap) \
> +	MLX5_GET_LOGSZ_CLAMP(cmd_hca_cap, \
> +			     mdev->caps.hca[MLX5_CAP_GENERAL]->cur, cap, u32)
> +
> +#define MLX5_CAP_GEN_LOGSZ_U64(mdev, cap) \
> +	MLX5_GET_LOGSZ_CLAMP(cmd_hca_cap, \
> +			     mdev->caps.hca[MLX5_CAP_GENERAL]->cur, cap, u64)
> +

[Severity: Low]
This isn't a bug, but MLX5_GET_LOGSZ_CLAMP(), MLX5_CAP_GEN_LOGSZ_U32() and
MLX5_CAP_GEN_LOGSZ_U64() have no callers in this commit.

Their users show up later in the series, in "RDMA/mlx5: Clamp log_max_cq_sz
shift in CQ size checks", "RDMA/mlx5: Clamp log_max_* shifts in QP and WQ
limit checks" and "RDMA/mlx5: Clamp log_max_* shifts in query_device and
ucontext paths". Those patches convert call sites in cq.c, qp.c, main.c,
mad.c and srq.c under drivers/infiniband/hw/mlx5/. For example, in
mlx5_ib_query_device():

    props->max_mr = MLX5_CAP_GEN_LOGSZ_U32(mdev, log_max_mkey);

Could the commit message mention these follow-up users and the reason for
the clamping?

The macro itself looks correct. min_t() limits the shift to 31 for u32 and
to 63 for u64.

The next point is a pre-existing issue that this patch did not introduce.
At the end of the series, net/mlx5 core still open-codes some of these
shifts. One is in get_init_bulk_query_len() and get_max_bulk_query_len()
in fs_counters.c:

    return min_t(int, MLX5_INIT_COUNTERS_BULK,
                 (1 << MLX5_CAP_GEN(dev, log_max_flow_counter_bulk)));

Another is mlx5e_mpwrq_max_num_entries() in en/params.c:

    u32 klm_limit = min(MLX5E_MAX_RQ_NUM_KSMS,
                        1 << MLX5_CAP_GEN(mdev, log_max_klm_list_size));

These shifts are exposed to untrusted firmware values by the earlier patch
"net/mlx5: Add capability filtering in adversarial mode". There,
mlx5_adversarial_filter_general() lets both caps through
MLX5_HCA_CAP_ALLOW without checking their range.

Would it make sense to convert these sites to the new helpers too?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103163949.1554-1-yishaih%40nvidia.com