Re: [Intel-wired-lan] [PATCH iwl-next v10 11/14] iavf: refactor iavf_clean_rx_irq to support legacy and flex descriptors

[Alexander Lobakin <aleksander.lobakin@intel.com>]

From: Wojciech Drewek <wojciech.drewek@intel.com>
Date: Wed, 21 Aug 2024 14:15:36 +0200

> From: Jacob Keller <jacob.e.keller@intel.com>
> 
> Using VIRTCHNL_VF_OFFLOAD_FLEX_DESC, the iAVF driver is capable of
> negotiating to enable the advanced flexible descriptor layout. Add the
> flexible NIC layout (RXDID=2) as a member of the Rx descriptor union.

[...]

> +static struct libeth_rx_csum
> +iavf_legacy_rx_csum(const struct iavf_vsi *vsi,
> +		    const struct iavf_rx_desc *rx_desc,
> +		    const struct libeth_rx_pt decoded)
> +{
> +	struct libeth_rx_csum csum_bits;
> +	u64 qw1 = le64_to_cpu(rx_desc->qw1);

Oops, RCT broke here.

> +
> +	if (!libeth_rx_pt_has_checksum(vsi->netdev, decoded))
> +		return csum_bits;
> +
> +	csum_bits.ipe = FIELD_GET(IAVF_RXD_LEGACY_IPE_M, qw1);
> +	csum_bits.eipe = FIELD_GET(IAVF_RXD_LEGACY_EIPE_M, qw1);
> +	csum_bits.l4e = FIELD_GET(IAVF_RXD_LEGACY_L4E_M, qw1);
> +	csum_bits.pprs = FIELD_GET(IAVF_RXD_LEGACY_PPRS_M, qw1);
> +	csum_bits.l3l4p = FIELD_GET(IAVF_RXD_LEGACY_L3L4P_M, qw1);
> +	csum_bits.ipv6exadd = FIELD_GET(IAVF_RXD_LEGACY_IPV6EXADD_M, qw1);
> +
> +	return csum_bits;
> +}

[...]

> +static struct libeth_rqe_info
> +iavf_extract_legacy_rx_fields(const struct iavf_ring *rx_ring,
> +			      const struct iavf_rx_desc *rx_desc, u32 *ptype)
> +{
> +	struct libeth_rqe_info fields = {};
> +	u64 qw0 = le64_to_cpu(rx_desc->qw0);
> +	u64 qw1 = le64_to_cpu(rx_desc->qw1);
> +	u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +	bool l2tag1p;
> +	bool l2tag2p;
> +
> +	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
> +	fields.len = FIELD_GET(IAVF_RXD_LEGACY_LENGTH_M, qw1);

And here you can do

	if (!fields.eop)
		return;

because the rest of the fields are not read for non-EOP descriptors.

> +	fields.rxe = FIELD_GET(IAVF_RXD_LEGACY_RXE_M, qw1);
> +	*ptype = FIELD_GET(IAVF_RXD_LEGACY_PTYPE_M, qw1);
> +
> +	l2tag1p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1P_M, qw1);
> +	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> +		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG1_M, qw0);
> +
> +	l2tag2p = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2P_M, qw2);
> +	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> +		fields.vlan_tag = FIELD_GET(IAVF_RXD_LEGACY_L2TAG2_M, qw2);

So vlan_tag can be set by both these ifs. They should be mutually
exclusive I think, via an `else`? Which of them has a higher priority?
can rx_ring->flags contain both these bits? I think no? Then it should be

	if (flags & L2TAG1) {
		l2tag1p = FIELD_GET(L2TAG1P_M);
		if (l2tag1p)
			vlan_tag = FIELD_GET(...
	} else if (flags & L2TAG2_2) {
		l2tag2p = FIELD_GET(L2TAG2P_M);
		if (l2tag2p)
			vlan_tag = FIELD_GET(...
	}

> +
> +	return fields;

I think you unconditionally initialize all the fields except vlan_tag,
why `= { }` then?
Just don't initialize it with empty struct and set .vlan_tag to 0 right
before `l2tag1p = ...`.

> +}
> +
> +/**
> + * iavf_extract_flex_rx_fields - Extract fields from the Rx descriptor
> + * @rx_ring: rx descriptor ring
> + * @rx_desc: the descriptor to process
> + * @ptype: pointer that will store packet type
> + *
> + * Decode the Rx descriptor and extract relevant information including the
> + * size, VLAN tag, Rx packet type, end of packet field and RXE field value.
> + *
> + * This function only operates on the VIRTCHNL_RXDID_2_FLEX_SQ_NIC flexible
> + * descriptor writeback format.
> + *
> + * Return: fields extracted from the Rx descriptor.
> + */
> +static struct libeth_rqe_info
> +iavf_extract_flex_rx_fields(const struct iavf_ring *rx_ring,
> +			    const struct iavf_rx_desc *rx_desc, u32 *ptype)
> +{
> +	struct libeth_rqe_info fields = {};
> +	u64 qw0 = le64_to_cpu(rx_desc->qw0);
> +	u64 qw1 = le64_to_cpu(rx_desc->qw1);
> +	u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +	bool l2tag1p, l2tag2p;
> +
> +	fields.len = FIELD_GET(IAVF_RXD_FLEX_PKT_LEN_M, qw0);
> +	fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
> +	fields.eop = FIELD_GET(IAVF_RXD_FLEX_EOP_M, qw1);
> +	*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
> +
> +	l2tag1p = FIELD_GET(IAVF_RXD_FLEX_L2TAG1P_M, qw1);
> +	if (l2tag1p && (rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1))
> +		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG1_M, qw1);
> +
> +	l2tag2p = FIELD_GET(IAVF_RXD_FLEX_L2TAG2P_M, qw2);
> +	if (l2tag2p && (rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2))
> +		fields.vlan_tag = FIELD_GET(IAVF_RXD_FLEX_L2TAG2_2_M, qw2);
> +
> +	return fields;

Same for the whole function as above.

> +}
> +
> +static struct libeth_rqe_info
> +iavf_extract_rx_fields(const struct iavf_ring *rx_ring,
> +		       const struct iavf_rx_desc *rx_desc, u32 *ptype)
> +{
> +	if (rx_ring->rxdid == VIRTCHNL_RXDID_1_32B_BASE)
> +		return iavf_extract_legacy_rx_fields(rx_ring, rx_desc, ptype);
> +	else
> +		return iavf_extract_flex_rx_fields(rx_ring, rx_desc, ptype);
> +}
> +
>  /**
>   * iavf_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf
>   * @rx_ring: rx descriptor ring to transact packets on
> @@ -1141,13 +1315,10 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  	bool failure = false;
>  
>  	while (likely(total_rx_packets < (unsigned int)budget)) {
> +		struct libeth_rqe_info fields = {};

Also redundant init since you assign it later unconditionally.

>  		struct libeth_fqe *rx_buffer;
>  		struct iavf_rx_desc *rx_desc;
> -		u16 ext_status = 0;
> -		unsigned int size;
> -		u16 vlan_tag = 0;
> -		u8 rx_ptype;
> -		u64 qword;
> +		u32 ptype;
>  
>  		/* return some buffers to hardware, one at a time is too slow */
>  		if (cleaned_count >= IAVF_RX_BUFFER_WRITE) {

[...]

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_type.h b/drivers/net/ethernet/intel/iavf/iavf_type.h
> index c1a4506fbaba..7c94e4c7f544 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_type.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_type.h
> @@ -191,45 +191,140 @@ struct iavf_rx_desc {
>  #define IAVF_RXD_LEGACY_RSS_M			GENMASK_ULL(63, 32)
>  /* Stripped L2 Tag from the receive packet. */
>  #define IAVF_RXD_LEGACY_L2TAG1_M		GENMASK_ULL(33, 16)
> +/* Packet type. */
> +#define IAVF_RXD_FLEX_PTYPE_M			GENMASK_ULL(25, 16)
> +/* Packet length. */
> +#define IAVF_RXD_FLEX_PKT_LEN_M			GENMASK_ULL(45, 32)
>  
>  	aligned_le64 qw1;
> +/* Status field stores information about end of packet, descriptor done etc. */
> +#define IAVF_RXD_LEGACY_STATUS_M		GENMASK(18, 0)
> +/* Descriptor done indication flag. */
> +#define IAVF_RXD_LEGACY_DD_M			BIT(0)
> +/* End of packet. Set to 1 if this descriptor is the last one of the packet. */
> +#define IAVF_RXD_LEGACY_EOP_M			BIT(1)
> +/* L2 TAG 1 presence indication. */
> +#define IAVF_RXD_LEGACY_L2TAG1P_M		BIT(2)
> +/* Detectable L3 and L4 integrity check is processed by the HW. */
> +#define IAVF_RXD_LEGACY_L3L4P_M			BIT(3)
> +/* The Ethernet CRC is posted with data to the host buffer. */
> +#define IAVF_RXD_LEGACY_CRCP_M			BIT(4)
> +/* Note: Bit 8 is reserved in X710 and XL710. */
> +#define IAVF_RXD_LEGACY_EXT_UDP_0_M		BIT(8)
> +/* Flow director filter match indication. */
> +#define IAVF_RXD_LEGACY_FLM_M			BIT(11)
> +/* Loop back indication means that the packet is originated from this system. */
> +#define IAVF_RXD_LEGACY_LPBK_M			BIT(14)
> +/* Set when an IPv6 packet contains a Destination Options Header or a Routing
> + * Header
> + */
> +#define IAVF_RXD_LEGACY_IPV6EXADD_M		BIT(15)
> +/* Note: For non-tunnel packets INT_UDP_0 is the right status for
> + * UDP header.
> + */
> +#define IAVF_RXD_LEGACY_INT_UDP_0_M		BIT(18)
> +/* Receive MAC Errors: CRC; Alignment; Oversize; Undersizes; Length error. */
> +#define IAVF_RXD_LEGACY_RXE_M			BIT(19)
> +/* Set by the Rx data processing unit if it could not complete properly
> + * the packet processing.
> + */
> +#define IAVF_RXD_LEGACY_RECIPE_M		BIT(20)
> +/* Header Buffer overflow. */
> +#define IAVF_RXD_LEGACY_HBO_M			BIT(21)
> +/* Checksum reports:
> + * - IPE: IP checksum error
> + * - L4E: L4 integrity error
> + * - EIPE: External IP header (tunneled packets)
> + */
> +#define IAVF_RXD_LEGACY_IPE_M			BIT(22)
> +#define IAVF_RXD_LEGACY_L4E_M			BIT(23)
> +#define IAVF_RXD_LEGACY_EIPE_M			BIT(24)
> +/* Oversize packet error. */
> +#define IAVF_RXD_LEGACY_OVERSIZE_M		BIT(25)
> +/* Set for packets that skip checksum calculation in pre-parser. */
> +#define IAVF_RXD_LEGACY_PPRS_M			BIT(26)
> +/* Destination Address type (unicast/multicast/broadcast/mirrored packet). */
> +#define IAVF_RXD_LEGACY_UMBCAST_M		GENMASK_ULL(10, 9)
> +/* Indicates the content in the Filter Status field . */
> +#define IAVF_RXD_LEGACY_FLTSTAT_M		GENMASK_ULL(13, 12)
> +/* Reserved. */
> +#define IAVF_RXD_LEGACY_RESERVED_M		GENMASK_ULL(17, 16)
> +/* Encoded errors of IP packets */
> +#define IAVF_RXD_LEGACY_L3L4E_M			GENMASK_ULL(24, 22)
> +/* Packet type. */
> +#define IAVF_RXD_LEGACY_PTYPE_M			GENMASK_ULL(37, 30)
> +/* Packet length. */
> +#define IAVF_RXD_LEGACY_LENGTH_M		GENMASK_ULL(51, 38)
> +/* Descriptor done indication flag. */
> +#define IAVF_RXD_FLEX_DD_M			BIT(0)
> +/* End of packet. Set to 1 if this descriptor is the last one of the packet. */
> +#define IAVF_RXD_FLEX_EOP_M			BIT(1)
> +/* Header Buffer overflow. */
> +#define IAVF_RXD_FLEX_HBO_M			BIT(2)
> +/* Detectable L3 and L4 integrity check is processed by the HW. */
> +#define IAVF_RXD_FLEX_L3L4P_M			BIT(3)
> +/* Checksum reports:
> + * - IPE: IP checksum error
> + * - L4E: L4 integrity error
> + * - EIPE: External IP header (tunneled packets)
> + * - EUDPE: External UDP checksum error (tunneled packets)
> + */
> +#define IAVF_RXD_FLEX_XSUM_IPE_M		BIT(4)
> +#define IAVF_RXD_FLEX_XSUM_L4E_M		BIT(5)
> +#define IAVF_RXD_FLEX_XSUM_EIPE_M		BIT(6)
> +#define IAVF_RXD_FLEX_XSUM_EUDPE_M		BIT(7)
> +/* Loop back indication means that the packet is originated from this system. */
> +#define IAVF_RXD_FLEX_LPBK_M			BIT(8)
> +/* Set when an IPv6 packet contains a Destination Options Header or a Routing
> + * Header
> + */
> +#define IAVF_RXD_FLEX_IPV6EXADD_M		BIT(9)
> +/* Receive MAC Errors: CRC; Alignment; Oversize; Undersizes; Length error. */
> +#define IAVF_RXD_FLEX_RXE_M			BIT(10)
> +/* The Ethernet CRC is posted with data to the host buffer. */
> +#define IAVF_RXD_FLEX_CRCP_M			BIT(11)
> +/* Indicates that the RSS/HASH result is valid. */
> +#define IAVF_RXD_FLEX_RSS_VALID_M		BIT(12)
> +/* L2 TAG 1 presence indication. */
> +#define IAVF_RXD_FLEX_L2TAG1P_M			BIT(13)
> +/* Indicates that extracted data from the packet is valid in specific Metadata
> + * Container
> + */
> +#define IAVF_RXD_FLEX_XTRMD0_VALID_M		BIT(14)
> +#define IAVF_RXD_FLEX_XTRMD1_VALID_M		BIT(15)
> +/* Stripped L2 Tag from the receive packet. */
> +#define IAVF_RXD_FLEX_L2TAG1_M			GENMASK_ULL(31, 16)
> +/* The hash signature (RSS). */
> +#define IAVF_RXD_FLEX_RSS_HASH_M		GENMASK_ULL(63, 32)
> +
>  	aligned_le64 qw2;
>  /* Extracted second word of the L2 Tag 2. */
>  #define IAVF_RXD_LEGACY_L2TAG2_2_M		GENMASK_ULL(63, 48)
>  /* Extended status bits. */
>  #define IAVF_RXD_LEGACY_EXT_STATUS_M		GENMASK_ULL(11, 0)
> +/* L2 Tag 2 Presence. */
> +#define IAVF_RXD_LEGACY_L2TAG2P_M		BIT(0)
> +/* Stripped L2 Tag from the receive packet. */
> +#define IAVF_RXD_LEGACY_L2TAG2_M		GENMASK_ULL(63, 32)
> +/* Stripped L2 Tag from the receive packet. */
> +#define IAVF_RXD_LEGACY_L2TAG2_1_M		GENMASK_ULL(47, 32)
> +/* Stripped L2 Tag from the receive packet. */
> +#define IAVF_RXD_FLEX_L2TAG2_2_M		GENMASK_ULL(63, 48)
> +/* The packet is a UDP tunneled packet. */
> +#define IAVF_RXD_FLEX_NAT_M			BIT(4)
> +/* L2 Tag 2 Presence. */
> +#define IAVF_RXD_FLEX_L2TAG2P_M			BIT(11)
> +/* Indicates that extracted data from the packet is valid in specific Metadata
> + * Container

Multi-line comments should end with a period ('.'), oneline shouldn't.

> + */
> +#define IAVF_RXD_FLEX_XTRMD2_VALID_M		BIT(12)
> +#define IAVF_RXD_FLEX_XTRMD3_VALID_M		BIT(13)
> +#define IAVF_RXD_FLEX_XTRMD4_VALID_M		BIT(14)
> +#define IAVF_RXD_FLEX_XTRMD5_VALID_M		BIT(15)

I don't think you use every definition from that structure. Please leave
only the used ones.

>  
>  	aligned_le64 qw3;
>  } __aligned(4 * sizeof(__le64));
>  
> -enum iavf_rx_desc_status_bits {
> -	/* Note: These are predefined bit offsets */
> -	IAVF_RX_DESC_STATUS_DD_SHIFT		= 0,
> -	IAVF_RX_DESC_STATUS_EOF_SHIFT		= 1,
> -	IAVF_RX_DESC_STATUS_L2TAG1P_SHIFT	= 2,
> -	IAVF_RX_DESC_STATUS_L3L4P_SHIFT		= 3,
> -	IAVF_RX_DESC_STATUS_CRCP_SHIFT		= 4,
> -	IAVF_RX_DESC_STATUS_TSYNINDX_SHIFT	= 5, /* 2 BITS */
> -	IAVF_RX_DESC_STATUS_TSYNVALID_SHIFT	= 7,
> -	/* Note: Bit 8 is reserved in X710 and XL710 */
> -	IAVF_RX_DESC_STATUS_EXT_UDP_0_SHIFT	= 8,
> -	IAVF_RX_DESC_STATUS_UMBCAST_SHIFT	= 9, /* 2 BITS */
> -	IAVF_RX_DESC_STATUS_FLM_SHIFT		= 11,
> -	IAVF_RX_DESC_STATUS_FLTSTAT_SHIFT	= 12, /* 2 BITS */
> -	IAVF_RX_DESC_STATUS_LPBK_SHIFT		= 14,
> -	IAVF_RX_DESC_STATUS_IPV6EXADD_SHIFT	= 15,
> -	IAVF_RX_DESC_STATUS_RESERVED_SHIFT	= 16, /* 2 BITS */
> -	/* Note: For non-tunnel packets INT_UDP_0 is the right status for
> -	 * UDP header
> -	 */
> -	IAVF_RX_DESC_STATUS_INT_UDP_0_SHIFT	= 18,
> -	IAVF_RX_DESC_STATUS_LAST /* this entry must be last!!! */
> -};
> -
> -#define IAVF_RXD_QW1_STATUS_SHIFT	0
> -#define IAVF_RXD_QW1_STATUS_MASK	((BIT(IAVF_RX_DESC_STATUS_LAST) - 1) \
> -					 << IAVF_RXD_QW1_STATUS_SHIFT)
> -
>  #define IAVF_RXD_QW1_STATUS_TSYNINDX_SHIFT IAVF_RX_DESC_STATUS_TSYNINDX_SHIFT
>  #define IAVF_RXD_QW1_STATUS_TSYNINDX_MASK  (0x3UL << \
>  					    IAVF_RXD_QW1_STATUS_TSYNINDX_SHIFT)
> @@ -245,22 +340,6 @@ enum iavf_rx_desc_fltstat_values {
>  	IAVF_RX_DESC_FLTSTAT_RSS_HASH	= 3,
>  };
>  
> -#define IAVF_RXD_QW1_ERROR_SHIFT	19
> -#define IAVF_RXD_QW1_ERROR_MASK		(0xFFUL << IAVF_RXD_QW1_ERROR_SHIFT)
> -
> -enum iavf_rx_desc_error_bits {
> -	/* Note: These are predefined bit offsets */
> -	IAVF_RX_DESC_ERROR_RXE_SHIFT		= 0,
> -	IAVF_RX_DESC_ERROR_RECIPE_SHIFT		= 1,
> -	IAVF_RX_DESC_ERROR_HBO_SHIFT		= 2,
> -	IAVF_RX_DESC_ERROR_L3L4E_SHIFT		= 3, /* 3 BITS */
> -	IAVF_RX_DESC_ERROR_IPE_SHIFT		= 3,
> -	IAVF_RX_DESC_ERROR_L4E_SHIFT		= 4,
> -	IAVF_RX_DESC_ERROR_EIPE_SHIFT		= 5,
> -	IAVF_RX_DESC_ERROR_OVERSIZE_SHIFT	= 6,
> -	IAVF_RX_DESC_ERROR_PPRS_SHIFT		= 7
> -};
> -
>  enum iavf_rx_desc_error_l3l4e_fcoe_masks {
>  	IAVF_RX_DESC_ERROR_L3L4E_NONE		= 0,
>  	IAVF_RX_DESC_ERROR_L3L4E_PROT		= 1,
> @@ -269,13 +348,6 @@ enum iavf_rx_desc_error_l3l4e_fcoe_masks {
>  	IAVF_RX_DESC_ERROR_L3L4E_DMAC_WARN	= 4
>  };
>  
> -#define IAVF_RXD_QW1_PTYPE_SHIFT	30
> -#define IAVF_RXD_QW1_PTYPE_MASK		(0xFFULL << IAVF_RXD_QW1_PTYPE_SHIFT)
> -
> -#define IAVF_RXD_QW1_LENGTH_PBUF_SHIFT	38
> -#define IAVF_RXD_QW1_LENGTH_PBUF_MASK	(0x3FFFULL << \
> -					 IAVF_RXD_QW1_LENGTH_PBUF_SHIFT)
> -
>  #define IAVF_RXD_QW1_LENGTH_HBUF_SHIFT	52
>  #define IAVF_RXD_QW1_LENGTH_HBUF_MASK	(0x7FFULL << \
>  					 IAVF_RXD_QW1_LENGTH_HBUF_SHIFT)

Thanks,
Olek