[xdp-hints] Re: [PATCH bpf V7 4/7] mlx5: bpf_xdp_metadata_rx_hash add xdp rss hash type

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

On 10/04/2023 17.11, Simon Horman wrote:
> On Sat, Apr 08, 2023 at 09:24:56PM +0200, Jesper Dangaard Brouer wrote:
>> Update API for bpf_xdp_metadata_rx_hash() with arg for xdp rss hash type
>> via mapping table.
>>
>> The mlx5 hardware can also identify and RSS hash IPSEC.  This indicate
>> hash includes SPI (Security Parameters Index) as part of IPSEC hash.
>>
>> Extend xdp core enum xdp_rss_hash_type with IPSEC hash type.
>>
>> Fixes: bc8d405b1ba9 ("net/mlx5e: Support RX XDP metadata")
>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>> Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
>> Acked-by: Stanislav Fomichev <sdf@google.com>
>> ---
>>   drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c |   60 ++++++++++++++++++++++
>>   include/linux/mlx5/device.h                      |   14 ++++-
>>   include/net/xdp.h                                |    2 +
>>   3 files changed, 73 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
>> index efe609f8e3aa..97ef1df94d50 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
>> @@ -34,6 +34,7 @@
>>   #include <net/xdp_sock_drv.h>
>>   #include "en/xdp.h"
>>   #include "en/params.h"
>> +#include <linux/bitfield.h>
>>   
>>   int mlx5e_xdp_max_mtu(struct mlx5e_params *params, struct mlx5e_xsk_param *xsk)
>>   {
>> @@ -169,15 +170,72 @@ static int mlx5e_xdp_rx_timestamp(const struct xdp_md *ctx, u64 *timestamp)
>>   	return 0;
>>   }
>>   
>> +/* Mapping HW RSS Type bits CQE_RSS_HTYPE_IP + CQE_RSS_HTYPE_L4 into 4-bits*/
>> +#define RSS_TYPE_MAX_TABLE	16 /* 4-bits max 16 entries */
>> +#define RSS_L4		GENMASK(1, 0)
>> +#define RSS_L3		GENMASK(3, 2) /* Same as CQE_RSS_HTYPE_IP */
>> +
>> +/* Valid combinations of CQE_RSS_HTYPE_IP + CQE_RSS_HTYPE_L4 sorted numerical */
>> +enum mlx5_rss_hash_type {
>> +	RSS_TYPE_NO_HASH	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IP_NONE) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_NONE)),
>> +	RSS_TYPE_L3_IPV4	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV4) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_NONE)),
>> +	RSS_TYPE_L4_IPV4_TCP	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV4) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_TCP)),
>> +	RSS_TYPE_L4_IPV4_UDP	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV4) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_UDP)),
>> +	RSS_TYPE_L4_IPV4_IPSEC	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV4) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_IPSEC)),
>> +	RSS_TYPE_L3_IPV6	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV6) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_NONE)),
>> +	RSS_TYPE_L4_IPV6_TCP	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV6) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_TCP)),
>> +	RSS_TYPE_L4_IPV6_UDP	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV6) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_UDP)),
>> +	RSS_TYPE_L4_IPV6_IPSEC	= (FIELD_PREP_CONST(RSS_L3, CQE_RSS_IPV6) |
>> +				   FIELD_PREP_CONST(RSS_L4, CQE_RSS_L4_IPSEC)),
>> +} mlx5_rss_hash_type;
> 
> Hi Jesper,
> 
> Sparse seems confused about 'mlx5_rss_hash_type' on the line above.
> And I am too. Perhaps it can be removed?
> 

Yes, it can be removed (in V8).

The reason/trick for doing this was to get compiler to create the enum
symbol, which allowed me to inspect the type using pahole (see cmd
below).  (This will also expose this to BTF, but it isn't actually
useful to keep around for BTF, so I will remove it in V8.)


$ pahole -C mlx5_rss_hash_type 
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.o
enum mlx5_rss_hash_type {
	RSS_TYPE_NO_HASH       = 0,
	RSS_TYPE_L3_IPV4       = 4,
	RSS_TYPE_L4_IPV4_TCP   = 5,
	RSS_TYPE_L4_IPV4_UDP   = 6,
	RSS_TYPE_L4_IPV4_IPSEC = 7,
	RSS_TYPE_L3_IPV6       = 8,
	RSS_TYPE_L4_IPV6_TCP   = 9,
	RSS_TYPE_L4_IPV6_UDP   = 10,
	RSS_TYPE_L4_IPV6_IPSEC = 11,
};

This is practical to for reviewers to see if below code is correct:

 > +/* Invalid combinations will simply return zero, allows no boundary 
checks */
 > +static const enum xdp_rss_hash_type 
mlx5_xdp_rss_type[RSS_TYPE_MAX_TABLE] = {
 > +	[RSS_TYPE_NO_HASH]	 = XDP_RSS_TYPE_NONE,
 > +	[1]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[2]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[3]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[RSS_TYPE_L3_IPV4]	 = XDP_RSS_TYPE_L3_IPV4,
 > +	[RSS_TYPE_L4_IPV4_TCP]	 = XDP_RSS_TYPE_L4_IPV4_TCP,
 > +	[RSS_TYPE_L4_IPV4_UDP]	 = XDP_RSS_TYPE_L4_IPV4_UDP,
 > +	[RSS_TYPE_L4_IPV4_IPSEC] = XDP_RSS_TYPE_L4_IPV4_IPSEC,
 > +	[RSS_TYPE_L3_IPV6]	 = XDP_RSS_TYPE_L3_IPV6,
 > +	[RSS_TYPE_L4_IPV6_TCP]	 = XDP_RSS_TYPE_L4_IPV6_TCP,
 > +	[RSS_TYPE_L4_IPV6_UDP]   = XDP_RSS_TYPE_L4_IPV6_UDP,
 > +	[RSS_TYPE_L4_IPV6_IPSEC] = XDP_RSS_TYPE_L4_IPV6_IPSEC,
 > +	[12]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[13]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[14]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +	[15]			 = XDP_RSS_TYPE_NONE, /* Implicit zero */
 > +};

> drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c:198:3: warning: symbol 'mlx5_rss_hash_type' was not declared. Should it be static?
> 
> ...
>