Re: [Intel-wired-lan] [PATCH iwl-next v10 04/14] iavf: add support for negotiating flexible RXDID format

[Wojciech Drewek <wojciech.drewek@intel.com>]

On 21.08.2024 15:52, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.drewek@intel.com>
> Date: Wed, 21 Aug 2024 14:15:29 +0200
> 
>> From: Jacob Keller <jacob.e.keller@intel.com>
>>
>> Enable support for VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, to enable the VF
>> driver the ability to determine what Rx descriptor formats are
>> available. This requires sending an additional message during
>> initialization and reset, the VIRTCHNL_OP_GET_SUPPORTED_RXDIDS. This
>> operation requests the supported Rx descriptor IDs available from the
>> PF.
>>
>> This is treated the same way that VLAN V2 capabilities are handled. Add
>> a new set of extended capability flags, used to process send and receipt
>> of the VIRTCHNL_OP_GET_SUPPORTED_RXDIDS message.
>>
>> This ensures we finish negotiating for the supported descriptor formats
>> prior to beginning configuration of receive queues.
>>
>> This change stores the supported format bitmap into the iavf_adapter
>> structure. Additionally, if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is enabled
>> by the PF, we need to make sure that the Rx queue configuration
>> specifies the format.
>>
>> Signed-off-by: Jacob Keller <jacob.e.keller@intel.com>
>> Reviewed-by: Wojciech Drewek <wojciech.drewek@intel.com>
>> Reviewed-by: Simon Horman <horms@kernel.org>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlopek@intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.drewek@intel.com>
> 
> [...]
> 
>> +/**
>> + * iavf_select_rx_desc_format - Select Rx descriptor format
>> + * @adapter: adapter private structure
>> + *
>> + * Select what Rx descriptor format based on availability and enabled
>> + * features.
>> + *
>> + * Return: the desired RXDID to select for a given Rx queue, as defined by
>> + *         enum virtchnl_rxdid_format.
>> + */
>> +static u8 iavf_select_rx_desc_format(const struct iavf_adapter *adapter)
>> +{
>> +	u64 rxdids = adapter->supp_rxdids;
>> +
>> +	/* If we did not negotiate VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC, we must
>> +	 * stick with the default value of the legacy 32 byte format.
>> +	 */
>> +	if (!IAVF_RXDID_ALLOWED(adapter))
>> +		return VIRTCHNL_RXDID_1_32B_BASE;
>> +
>> +	/* Warn if the PF does not list support for the default legacy
>> +	 * descriptor format. This shouldn't happen, as this is the format
>> +	 * used if VIRTCHNL_VF_OFFLOAD_RX_FLEX_DESC is not supported. It is
>> +	 * likely caused by a bug in the PF implementation failing to indicate
>> +	 * support for the format.
>> +	 */
>> +	if (!(rxdids & VIRTCHNL_RXDID_1_32B_BASE_M))
>> +		dev_warn(&adapter->pdev->dev, "PF does not list support for default Rx descriptor format\n");
> 
> pci_warn() or netdev_warn() if netdev is available here, sorry if I
> didn't mention this earlier =\

sure

> 
>> +
>> +	return VIRTCHNL_RXDID_1_32B_BASE;
>> +}
>> +
>>  /**
>>   * iavf_configure_rx - Configure Receive Unit after Reset
>>   * @adapter: board private structure
> 
> [...]
> 
>> @@ -262,6 +276,37 @@ int iavf_get_vf_vlan_v2_caps(struct iavf_adapter *adapter)
>>  	return err;
>>  }
>>  
>> +int iavf_get_vf_supported_rxdids(struct iavf_adapter *adapter)
>> +{
>> +	struct iavf_hw *hw = &adapter->hw;
>> +	struct iavf_arq_event_info event;
>> +	u32 len = sizeof(u64);
>> +	enum virtchnl_ops op;
>> +	enum iavf_status err;
>> +	u8 rxdids;
>> +
>> +	event.msg_buf = &rxdids;
>> +	event.buf_len = len;
> 
> This looks suspicious. @rxdids is u8, while @len is sizeof(u64), i.e 8
> bytes, not 1. Is this intended? Or maybe @rxdids should be u64 here as
> well, just like adapter->supported_rxdids?
> 
>> +
>> +	while (1) {
> 
> @op can be declared right here.
> @err can be also declared right here if you address the comment below.
> 
>> +		/* When the AQ is empty, iavf_clean_arq_element will return
>> +		 * nonzero and this loop will terminate.
>> +		 */
>> +		err = iavf_clean_arq_element(hw, &event, NULL);
>> +		if (err != IAVF_SUCCESS)
>> +			return err;
>> +		op = le32_to_cpu(event.desc.cookie_high);
>> +		if (op == VIRTCHNL_OP_GET_SUPPORTED_RXDIDS)
> 
> When one of the elements you want to compare is a compile-time constant,
> you will get more optimized code if you do
> 
> 		__le32 op;
> 
> 		op = event.desc.cookie_high;
> 		if (op == cpu_to_le32(VIRTCHNL_OP_GET_SUPPORTED_RXDIDS))
> 
> because then you won't need to byteswap a variable and constants get
> byteswapped at compilation time.
> 
> But given that iavf runs on LE 99% of time and it's not hotpath, it's up
> to you whether to do it like that here or just leave as it is.
> 
>> +			break;
>> +	}
>> +
>> +	err = le32_to_cpu(event.desc.cookie_low);
>> +	if (!err)
> 
> 	if (!event.desc.cookie_low)
> 
> Because 0 == le32_to_cpu(0), it's always 0.
> So you don't need @err here and it can be declared inside the loop above.

I'll refactor this function to use iavf_poll_virtchnl_msg, so
this loop will disappear from hear.

> 
>> +		adapter->supp_rxdids = rxdids;
>> +
>> +	return 0;
>> +}
>> +
>>  /**
>>   * iavf_configure_queues
>>   * @adapter: adapter structure
>> @@ -308,6 +353,8 @@ void iavf_configure_queues(struct iavf_adapter *adapter)
>>  		vqpi->rxq.dma_ring_addr = adapter->rx_rings[i].dma;
>>  		vqpi->rxq.max_pkt_size = max_frame;
>>  		vqpi->rxq.databuffer_size = adapter->rx_rings[i].rx_buf_len;
>> +		if (IAVF_RXDID_ALLOWED(adapter))
>> +			vqpi->rxq.rxdid = adapter->rxdid;
>>  		if (CRC_OFFLOAD_ALLOWED(adapter))
>>  			vqpi->rxq.crc_disable = !!(adapter->netdev->features &
>>  						   NETIF_F_RXFCS);
>> @@ -2372,6 +2419,10 @@ void iavf_virtchnl_completion(struct iavf_adapter *adapter,
>>  			aq_required;
>>  		}
>>  		break;
>> +	case VIRTCHNL_OP_GET_SUPPORTED_RXDIDS:
>> +		memcpy(&adapter->supp_rxdids, msg,
>> +		       min_t(u16, msglen, sizeof(adapter->supp_rxdids)));
> 
> Why is this needed if you assign ->supp_rxdids in
> iavf_get_vf_supported_rxdids()? Or is this something different?

I think that iavf_get_vf_supported_rxdids is used only during init.
This part here is used later during reset e.g.

> 
> I'd also say this memcpy() is not safe. ->supp_rxdids is u64. If somehow
> @msglen is less than 8 bytes, you'd probably get a corrupted u64 value.
> I think you should compare @msglen to sizeof(u64) and bail out if it's
> different. If it's the same, you should just do

Sure, I can make it like that.

> 
> 		adapter->supp_rxdids = *(u64 *)msg;
> 
>> +		break;
>>  	case VIRTCHNL_OP_ENABLE_QUEUES:
>>  		/* enable transmits */
>>  		iavf_irq_enable(adapter, true);
>> diff --git a/drivers/net/ethernet/intel/ice/ice_virtchnl.c b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> index b60df6e9b3e7..3c2d6a504aa0 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_virtchnl.c
>> @@ -2709,12 +2709,12 @@ static int ice_vc_set_rss_hena(struct ice_vf *vf, u8 *msg)
>>  static int ice_vc_query_rxdid(struct ice_vf *vf)
>>  {
>>  	enum virtchnl_status_code v_ret = VIRTCHNL_STATUS_SUCCESS;
>> -	struct virtchnl_supported_rxdids *rxdid = NULL;
>>  	struct ice_hw *hw = &vf->pf->hw;
>>  	struct ice_pf *pf = vf->pf;
>> -	int len = 0;
>> -	int ret, i;
>> +	u32 len = sizeof(u64);
>>  	u32 regval;
>> +	u64 rxdid;
>> +	int ret, i;
> 
> RCT broke here =\

sure

> 
>>  
>>  	if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) {
>>  		v_ret = VIRTCHNL_STATUS_ERR_PARAM;
> 
> Thanks,
> Olek