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 =\ > + > + 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. > + 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'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 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 =\ > > if (!test_bit(ICE_VF_STATE_ACTIVE, vf->vf_states)) { > v_ret = VIRTCHNL_STATUS_ERR_PARAM; Thanks, Olek