Re: [PATCH RFC v2 net-next 09/28] p4tc: add P4 data types

[Simon Horman <simon.horman@corigine.com>]

On Wed, May 17, 2023 at 07:02:13AM -0400, Jamal Hadi Salim wrote:
> Introduce abstraction that represents P4 data types.
> This also introduces the Kconfig and Makefile which later patches use.
> Types could be little, host or big endian definitions. The abstraction also
> supports defining:
> 
> a) bitstrings using annotations in control that look like "bitX" where X
>    is the number of bits defined in a type
> 
> b) bitslices such that one can define in control bit8[0-3] and
>    bit16[0-9]. A 4-bit slice from bits 0-3 and a 10-bit slice from bits
>    0-9 respectively.
> 
> Each type has a bitsize, a name (for debugging purposes), an ID and
> methods/ops. The P4 types will be used by metadata, headers, dynamic
> actions and other part of P4TC.
> 
> Each type has four ops:
> 
> - validate_p4t: Which validates if a given value of a specific type
>   meets valid boundary conditions.
> 
> - create_bitops: Which, given a bitsize, bitstart and bitend allocates and
>   returns a mask and a shift value. For example, if we have type bit8[3-3]
>   meaning bitstart = 3 and bitend = 3, we'll create a mask which would only
>   give us the fourth bit of a bit8 value, that is, 0x08. Since we are
>   interested in the fourth bit, the bit shift value will be 3.
> 
> - host_read : Which reads the value of a given type and transforms it to
>   host order
> 
> - host_write : Which writes a provided host order value and transforms it
>   to the type's native order
> 
> Co-developed-by: Victor Nogueira <victor@mojatatu.com>
> Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>

Hi Victor, Pedro and Jamal,

some minor feedback from my side.

> diff --git a/net/sched/p4tc/p4tc_types.c b/net/sched/p4tc/p4tc_types.c

...

> +static struct p4tc_type *p4type_find_byname(const char *name)
> +{
> +	struct p4tc_type *type;
> +	unsigned long tmp, typeid;

As per my comment on another patch in this series,
please use reverse xmas tree - longest line to shortest -
for local variable declarations in networking code.

The following tool can help:
https://github.com/ecree-solarflare/xmastree

> +
> +	idr_for_each_entry_ul(&p4tc_types_idr, type, tmp, typeid) {
> +		if (!strncmp(type->name, name, P4T_MAX_STR_SZ))
> +			return type;
> +	}
> +
> +	return NULL;
> +}

...

> +static int p4t_be32_validate(struct p4tc_type *container, void *value,
> +			     u16 bitstart, u16 bitend,
> +			     struct netlink_ext_ack *extack)
> +{
> +	size_t container_maxsz = U32_MAX;
> +	__u32 *val_u32 = value;
> +	__be32 val = 0;
> +	size_t maxval;
> +	int ret;
> +
> +	ret = p4t_validate_bitpos(bitstart, bitend, 31, 31, extack);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (value)
> +		val = (__be32)(be32_to_cpu(*val_u32));

From a type annotation point of view, a value can be either CPU byte order
or big endian. It can't be both. What was the byte order of val_u32?  What is
the desired byte order of val?

Sparse, invoked using make C=1, flags this and
sever other issues with endineness handling.

> +
> +	maxval = GENMASK(bitend, 0);
> +	if (val && (val > container_maxsz || val > maxval)) {
> +		NL_SET_ERR_MSG_MOD(extack, "BE32 value out of range");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...