netdev - Re: [Intel-wired-lan] [PATCH iwl-next v5 09/12] iavf: refactor iavf_clean_rx_irq to support legacy and flex descriptors

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: <7be58f82-77de-4097-973e-36085b63a576@intel.com>
Date: Mon, 22 Apr 2024 11:22:10 +0200
From: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
To: Rahul Rameshbabu <rrameshbabu@...dia.com>
CC: <intel-wired-lan@...ts.osuosl.org>, <netdev@...r.kernel.org>,
	<horms@...nel.org>, <anthony.l.nguyen@...el.com>, Jacob Keller
	<jacob.e.keller@...el.com>, Wojciech Drewek <wojciech.drewek@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v5 09/12] iavf: refactor
 iavf_clean_rx_irq to support legacy and flex descriptors



On 4/18/2024 10:00 PM, Rahul Rameshbabu wrote:
> 
> On Thu, 18 Apr, 2024 01:24:57 -0400 Mateusz Polchlopek <mateusz.polchlopek@...el.com> wrote:
>> From: Jacob Keller <jacob.e.keller@...el.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.
>>
>> Also add bit position definitions for the status and error indications
>> that are needed.
>>
>> The iavf_clean_rx_irq function needs to extract a few fields from the Rx
>> descriptor, including the size, rx_ptype, and vlan_tag.
>> Move the extraction to a separate function that decodes the fields into
>> a structure. This will reduce the burden for handling multiple
>> descriptor types by keeping the relevant extraction logic in one place.
>>
>> To support handling an additional descriptor format with minimal code
>> duplication, refactor Rx checksum handling so that the general logic
>> is separated from the bit calculations. Introduce an iavf_rx_desc_decoded
>> structure which holds the relevant bits decoded from the Rx descriptor.
>> This will enable implementing flexible descriptor handling without
>> duplicating the general logic twice.
>>
>> Introduce an iavf_extract_flex_rx_fields, iavf_flex_rx_hash, and
>> iavf_flex_rx_csum functions which operate on the flexible NIC descriptor
>> format instead of the legacy 32 byte format. Based on the negotiated
>> RXDID, select the correct function for processing the Rx descriptors.
>>
>> With this change, the Rx hot path should be functional when using either
>> the default legacy 32byte format or when we switch to the flexible NIC
>> layout.
>>
>> Modify the Rx hot path to add support for the flexible descriptor
>> format and add request enabling Rx timestamps for all queues.
>>
>> As in ice, make sure we bump the checksum level if the hardware detected
>> a packet type which could have an outer checksum. This is important
>> because hardware only verifies the inner checksum.
>>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@...el.com>
>> Signed-off-by: Jacob Keller <jacob.e.keller@...el.com>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@...el.com>
>> ---
>>   drivers/net/ethernet/intel/iavf/iavf_txrx.c   | 354 +++++++++++++-----
>>   drivers/net/ethernet/intel/iavf/iavf_txrx.h   |   8 +
>>   drivers/net/ethernet/intel/iavf/iavf_type.h   | 149 ++++++--
>>   .../net/ethernet/intel/iavf/iavf_virtchnl.c   |   5 +
>>   4 files changed, 390 insertions(+), 126 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> <snip>
>> +/**
>> + * iavf_flex_rx_hash - set the hash value in the skb
>> + * @ring: descriptor ring
>> + * @rx_desc: specific descriptor
>> + * @skb: skb currently being received and modified
>> + * @rx_ptype: Rx packet type
>> + *
>> + * This function only operates on the VIRTCHNL_RXDID_2_FLEX_SQ_NIC flexible
>> + * descriptor writeback format.
>> + **/
>> +static void iavf_flex_rx_hash(struct iavf_ring *ring,
>> +			      union iavf_rx_desc *rx_desc,
>> +			      struct sk_buff *skb, u16 rx_ptype)
>> +{
>> +	__le16 status0;
>> +
>> +	if (!(ring->netdev->features & NETIF_F_RXHASH))
>> +		return;
>> +
>> +	status0 = rx_desc->flex_wb.status_error0;
> 
> Any reason to not convert rx_desc->flex_wb.status_error0 to
> CPU-endianness for the bit check?
> 
>> +	if (status0 & cpu_to_le16(IAVF_RX_FLEX_DESC_STATUS0_RSS_VALID_M)) {
>> +		u32 hash = le32_to_cpu(rx_desc->flex_wb.rss_hash);
>> +
>> +		skb_set_hash(skb, hash, iavf_ptype_to_htype(rx_ptype));
>> +	}
>> +}
> <snip>
>> +/**
>> + * iavf_extract_flex_rx_fields - Extract fields from the Rx descriptor
>> + * @rx_ring: rx descriptor ring
>> + * @rx_desc: the descriptor to process
>> + * @fields: storage for extracted values
>> + *
>> + * 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.
>> + */
>> +static void iavf_extract_flex_rx_fields(struct iavf_ring *rx_ring,
>> +					union iavf_rx_desc *rx_desc,
>> +					struct iavf_rx_extracted *fields)
>> +{
>> +	__le16 status0, status1, flexi_flags0;
>> +
>> +	fields->size = FIELD_GET(IAVF_RX_FLEX_DESC_PKT_LEN_M,
>> +				 le16_to_cpu(rx_desc->flex_wb.pkt_len));
>> +
>> +	flexi_flags0 = rx_desc->flex_wb.ptype_flexi_flags0;
>> +
>> +	fields->rx_ptype = FIELD_GET(IAVF_RX_FLEX_DESC_PTYPE_M,
>> +				     le16_to_cpu(flexi_flags0));
>> +
>> +	status0 = rx_desc->flex_wb.status_error0;
>> +	if (status0 & cpu_to_le16(IAVF_RX_FLEX_DESC_STATUS0_L2TAG1P_M) &&
>> +	    rx_ring->flags & IAVF_TXRX_FLAGS_VLAN_TAG_LOC_L2TAG1)
>> +		fields->vlan_tag = le16_to_cpu(rx_desc->flex_wb.l2tag1);
>> +
>> +	status1 = rx_desc->flex_wb.status_error1;
> 
> Similar comment to previous in this function.
> 

Thanks for that comment, I will take a look on that and probably will
change in next version.

>> +	if (status1 & cpu_to_le16(IAVF_RX_FLEX_DESC_STATUS1_L2TAG2P_M) &&
>> +	    rx_ring->flags & IAVF_RXR_FLAGS_VLAN_TAG_LOC_L2TAG2_2)
>> +		fields->vlan_tag = le16_to_cpu(rx_desc->flex_wb.l2tag2_2nd);
>> +
>> +	fields->end_of_packet = FIELD_GET(IAVF_RX_FLEX_DESC_STATUS_ERR0_EOP_BIT,
>> +					  le16_to_cpu(status0));
>> +	fields->rxe = FIELD_GET(IAVF_RX_FLEX_DESC_STATUS_ERR0_RXE_BIT,
>> +				le16_to_cpu(status0));
>> +}
>> +
>> +static void iavf_extract_rx_fields(struct iavf_ring *rx_ring,
>> +				   union iavf_rx_desc *rx_desc,
>> +				   struct iavf_rx_extracted *fields)
>> +{
>> +	if (rx_ring->rxdid == VIRTCHNL_RXDID_1_32B_BASE)
>> +		iavf_extract_legacy_rx_fields(rx_ring, rx_desc, fields);
>> +	else
>> +		iavf_extract_flex_rx_fields(rx_ring, rx_desc, fields);
>> +}
>> +
> <snip>
> 
> --
> Thanks,
> 
> Rahul Rameshbabu