netdev - Re: [PATCH iwl-next v10 12/14] iavf: Implement checking DD desc field

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e122969f-8954-4cdf-9495-c0a166494cda@intel.com>
Date: Thu, 22 Aug 2024 18:46:46 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Wojciech Drewek <wojciech.drewek@...el.com>
CC: <netdev@...r.kernel.org>, <intel-wired-lan@...ts.osuosl.org>,
	<horms@...nel.org>, <anthony.l.nguyen@...el.com>, <kuba@...nel.org>,
	<alexandr.lobakin@...el.com>
Subject: Re: [PATCH iwl-next v10 12/14] iavf: Implement checking DD desc field

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

> From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
> 
> Rx timestamping introduced in PF driver caused the need of refactoring
> the VF driver mechanism to check packet fields.

[...]

> +static bool iavf_is_descriptor_done(struct iavf_ring *rx_ring,

const

> +				    struct iavf_rx_desc *rx_desc)

Just pass qw1 here directly instead of @rx_desc.

> +{
> +	__le64 qw1 = rx_desc->qw1;
> +
> +	if (rx_ring->rxdid == VIRTCHNL_RXDID_1_32B_BASE)

I would also pass `bool flex` here instead of @rx_ring.
At the beginning of iavf_clean_rx_irq(), do

	bool flex = rx_ring->rxdid == RXDID_FLEX

and then pass this @flex everywhere instead of checking for rx_ring->rxdid.

iavf_is_descriptor_done(u64 qw1, bool flex)
{
	if (flex)
		return le64_get_bits(qw1, FLEX_DD);
	else
		return le64_get_bits(qw1, LEGACY_DD);
}

That's it.

> +		return !!le64_get_bits(qw1, IAVF_RXD_LEGACY_DD_M);
> +
> +	return !!le64_get_bits(qw1, IAVF_RXD_FLEX_DD_M);
> +}
> +
>  static __le64 build_ctob(u32 td_cmd, u32 td_offset, unsigned int size,
>  			 u32 td_tag)
>  {
> @@ -1223,24 +1245,31 @@ 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);
> -	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);
> +	fields.eop = FIELD_GET(IAVF_RXD_LEGACY_EOP_M, qw1);
>  
> -	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);
> +	if (fields.eop) {
> +		bool l2tag1p, l2tag2p;
> +		u64 qw0 = le64_to_cpu(rx_desc->qw0);
> +		u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +
> +		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);
> +	}

Just do that right where you introduce this function, this change is not
related to this patch.

Also, `if (!fields.eop) return fields` instead of this big if.

>  
>  	return fields;
>  }
> @@ -1266,21 +1295,29 @@ iavf_extract_flex_rx_fields(const struct iavf_ring *rx_ring,
>  	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);
> +	if (fields.len) {
> +		bool l2tag1p, l2tag2p;
> +		u64 qw2 = le64_to_cpu(rx_desc->qw2);
> +
> +		fields.rxe = FIELD_GET(IAVF_RXD_FLEX_RXE_M, qw1);
> +		*ptype = FIELD_GET(IAVF_RXD_FLEX_PTYPE_M, qw0);
>  
> -	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);
> +		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);
> +	}

Same.

>  
>  	return fields;
>  }
> @@ -1335,7 +1372,10 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>  		 */
>  		dma_rmb();
>  
> -		if (!iavf_test_staterr(rx_desc, IAVF_RXD_FLEX_DD_M))
> +		/* If DD field (descriptor done) is unset then other fields are
> +		 * not valid
> +		 */
> +		if (!iavf_is_descriptor_done(rx_ring, rx_desc))
>  			break;
>  
>  		fields = iavf_extract_rx_fields(rx_ring, rx_desc, &ptype);

Thanks,
Olek