On 21.08.2024 15:52, Alexander Lobakin wrote:
> From: Wojciech Drewek <wojciech.dre...@intel.com>
> Date: Wed, 21 Aug 2024 14:15:29 +0200
>
>> From: Jacob Keller <jacob.e.kel...@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.kel...@intel.com>
>> Reviewed-by: Wojciech Drewek <wojciech.dre...@intel.com>
>> Reviewed-by: Simon Horman <ho...@kernel.org>
>> Co-developed-by: Mateusz Polchlopek <mateusz.polchlo...@intel.com>
>> Signed-off-by: Mateusz Polchlopek <mateusz.polchlo...@intel.com>
>> Signed-off-by: Wojciech Drewek <wojciech.dre...@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
