Message-ID: <d4a7fe72-1e44-466a-9fa3-87e34394a239@intel.com>
Date: Wed, 5 Mar 2025 15:04:14 -0800
From: Tony Nguyen <anthony.l.nguyen@...el.com>
To: Paul Greenwalt <paul.greenwalt@...el.com>,
	<intel-wired-lan@...ts.osuosl.org>
CC: <netdev@...r.kernel.org>, Aleksandr Loktionov
	<aleksandr.loktionov@...el.com>, Alice Michael <alice.michael@...el.com>
Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1] ice: add E830 Earliest
 TxTime First Offload support



On 2/27/2025 3:13 AM, Paul Greenwalt wrote:
> E830 supports Earliest TxTime First (ETF) hardware offload, which is
> configured via the ETF Qdisc (see tc-etf(8)). ETF introduces a new Tx flow
> mechanism that utilizes a timestamp ring (tstamp_ring) alongside the
> standard Tx ring. This timestamp ring is used to indicate when hardware
> will transmit a packet.
> 
> The allocation and initialization of the timestamp ring occur when the
> feature is enabled via tc-etf. Since the timestamp ring and Tx ring are
> tightly coupled, both must be configured simultaneously.
> 
> To support ETF, the following flags are introduced:
> 
>   - ICE_F_TXTIME: Device feature flag set for E830 NICs, indicating ETF
>     support.
>   - ICE_FLAG_TXTIME: PF-level flag indicating whether ETF is enabled on any
>     Tx queue. It is checked during ring allocation to determine if timestamp
>     rings should be allocated and is also referenced when modifying queue
>     count via ethtool -G.
>   - ICE_TX_FLAGS_TXTIME: Per-ring flag set when ETF is enabled and cleared
>     when disabled for a specific Tx queue. It helps determine ETF status
>     when transmitting timestamped packets and is used by ice_is_txtime_ena()
>     to check if ETF is enabled on any Tx queue.
> 
> Due to a hardware issue that can result in a malicious driver detection
> event, additional timestamp descriptors are required when wrapping the
> timestamp ring. Up to 64 additional timestamp descriptors are reserved,
> reducing the available Tx descriptors.
> 
> To accommodate this, ICE_MAX_NUM_DESC_BY_MAC is introduced, defining:
> 
>   - E830: Maximum Tx descriptor length of 8096 (8K - 32 - 64 for timestamp
>     fetch descriptors).
>   - E810 and E82X: Maximum Tx descriptor length of 8160 (8K - 32) .

This doesn't apply.

Also, some comments from a glance over this.

...

> +ice_setup_tx_ctx(struct ice_tx_ring *ring, struct ice_tlan_ctx *tlan_ctx, u16 pf_q)
> +{
> +	struct ice_vsi *vsi = ring->vsi;
> +	struct ice_hw *hw = &vsi->back->hw;

RCT

...

>   static int
> -ice_vsi_cfg_txq(struct ice_vsi *vsi, struct ice_tx_ring *ring,
> -		struct ice_aqc_add_tx_qgrp *qg_buf)
> +ice_vsi_cfg_txq(const struct ice_vsi *vsi, struct ice_tx_ring *ring,
> +		struct ice_tx_ring *tstamp_ring,
> +		struct ice_aqc_add_tx_qgrp *qg_buf,
> +		struct ice_aqc_set_txtime_qgrp *txtime_qg_buf)
>   {
>   	u8 buf_len = struct_size(qg_buf, txqs, 1);
>   	struct ice_tlan_ctx tlan_ctx = { 0 };
> @@ -947,6 +1047,27 @@ ice_vsi_cfg_txq(struct ice_vsi *vsi, struct ice_tx_ring *ring,
>   	if (pf_q == le16_to_cpu(txq->txq_id))
>   		ring->txq_teid = le32_to_cpu(txq->q_teid);
>   
> +	if (tstamp_ring) {
> +		u8 txtime_buf_len = struct_size(txtime_qg_buf, txtimeqs, 1);
> +		struct ice_txtime_ctx txtime_ctx = { 0 };

IIRC, preference is to initialize without the '0', {}

> +
> +		ice_setup_txtime_ctx(tstamp_ring, &txtime_ctx,
> +				     !!(ring->flags & ICE_TX_FLAGS_TXTIME));
> +		ice_pack_txtime_ctx(&txtime_ctx,
> +				    &txtime_qg_buf->txtimeqs[0].txtime_ctx);
> +
> +		tstamp_ring->tail =
> +			 hw->hw_addr + E830_GLQTX_TXTIME_DBELL_LSB(pf_q);
> +
> +		status = ice_aq_set_txtimeq(hw, pf_q, 1, txtime_qg_buf,
> +					    txtime_buf_len, NULL);
> +		if (status) {
> +			dev_err(ice_pf_to_dev(pf), "Failed to set Tx Time queue context, error: %d\n",
> +				status);
> +			return status;
> +		}
> +	}
> +
>   	return 0;
>   }
>   

...

> +int
> +ice_aq_set_txtimeq(struct ice_hw *hw, u16 txtimeq, u8 q_count,
> +		   struct ice_aqc_set_txtime_qgrp *txtime_qg, u16 buf_size,
> +		   struct ice_sq_cd *cd)
> +{
> +	struct ice_aqc_set_txtimeqs *cmd;
> +	struct ice_aq_desc desc;
> +	u16 size;
> +
> +	cmd = &desc.params.set_txtimeqs;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_set_txtimeqs);
> +
> +	if (!txtime_qg)
> +		return -EINVAL;
> +
> +	if (txtimeq > ICE_TXTIME_MAX_QUEUE || q_count < 1 ||
> +	    q_count > ICE_SET_TXTIME_MAX_Q_AMOUNT)
> +		return -EINVAL;

Should the bailout conditions be checked first?

> +
> +	size = struct_size(txtime_qg, txtimeqs, q_count);
> +

No newline here
> +	if (buf_size != size)
> +		return -EINVAL;
> +
> +	desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +
> +	cmd->q_id = cpu_to_le16(txtimeq);
> +	cmd->q_amount = cpu_to_le16(q_count);
> +	return ice_aq_send_cmd(hw, &desc, txtime_qg, buf_size, cd);
> +}
> +
> +/**
> + * ice_aq_ena_dis_txtimeq - enable/disable Tx time queue
> + * @hw: pointer to the hardware structure
> + * @txtimeq: first Tx time queue id to configure
> + * @q_count: number of queues to configure
> + * @q_ena: enable/disable Tx time queue
> + * @txtime_qg: holds the first Tx time queue that failed enable/disable on
> + * response
> + * @cd: pointer to command details structure or NULL
> + *
> + * Enable/disable Tx Time queue (0x0C37)
> + * Return: 0 on success or negative value on failure.
> + */
> +int
> +ice_aq_ena_dis_txtimeq(struct ice_hw *hw, u16 txtimeq, u16 q_count, bool q_ena,
> +		       struct ice_aqc_ena_dis_txtime_qgrp *txtime_qg,
> +		       struct ice_sq_cd *cd)
> +{
> +	struct ice_aqc_ena_dis_txtimeqs *cmd;
> +	struct ice_aq_desc desc;
> +
> +	cmd = &desc.params.operate_txtimeqs;
> +
> +	ice_fill_dflt_direct_cmd_desc(&desc, ice_aqc_opc_ena_dis_txtimeqs);
> +
> +	if (!txtime_qg)
> +		return -EINVAL;
> +
> +	if (txtimeq > ICE_TXTIME_MAX_QUEUE || q_count < 1 ||
> +	    q_count > ICE_OP_TXTIME_MAX_Q_AMOUNT)
> +		return -EINVAL;

Same bailout question

> +
> +	desc.flags |= cpu_to_le16(ICE_AQ_FLAG_RD);
> +
> +	cmd->q_id = cpu_to_le16(txtimeq);
> +	cmd->q_amount = cpu_to_le16(q_count);
> +
> +	if (q_ena)
> +		cmd->cmd_type |= ICE_AQC_TXTIME_CMD_TYPE_Q_ENA;
> +
> +	return ice_aq_send_cmd(hw, &desc, txtime_qg, sizeof(*txtime_qg), cd);
> +}
> +
>   /* End of FW Admin Queue command wrappers */
>   

...

> +/**
> + * ice_vsi_cfg_txtime - configure TxTime for the VSI
> + * @vsi: VSI to reconfigure
> + * @enable: enable or disable TxTime
> + * @queue: Tx queue to configure TxTime on
> + *
> + * Return: 0 on success, negative value on failure.
> + */
> +static int ice_vsi_cfg_txtime(struct ice_vsi *vsi, bool enable,
> +			      int queue)
> +{
> +	bool if_running = netif_running(vsi->netdev), locked = false;
> +	struct ice_pf *pf = vsi->back;
> +	int ret, timeout = 50;
> +
> +	while (test_and_set_bit(ICE_CFG_BUSY, vsi->back->state)) {
> +		timeout--;
> +		if (!timeout)
> +			return -EBUSY;
> +		usleep_range(1000, 2000);
> +	}
> +
> +	if (pf->adev) {
> +		mutex_lock(&pf->adev_mutex);
> +		device_lock(&pf->adev->dev);
> +		locked = true;
> +		if (pf->adev->dev.driver) {
> +			dev_err(ice_pf_to_dev(pf), "Cannot change TxTime when RDMA is active\n");
> +			ret = -EBUSY;
> +			goto adev_unlock;
> +		}
> +	}
> +
> +	/* If rnnning, close and open VSI to clear and reconfigure all rings. */
> +	if (if_running)
> +		ice_vsi_close(vsi);
> +
> +	/* Enable or disable PF TxTime flag which is checked during VSI rebuild
> +	 * for allocating the timestamp rings.
> +	 */
> +	if (enable)
> +		set_bit(ICE_FLAG_TXTIME, pf->flags);
> +	else
> +		clear_bit(ICE_FLAG_TXTIME, pf->flags);
> +
> +	/* Rebuild VSI to allocate or free timestamp rings */
> +	ret = ice_vsi_rebuild(vsi, ICE_VSI_FLAG_NO_INIT);
> +	if (ret) {
> +		dev_err(ice_pf_to_dev(pf), "Unhandled error during VSI rebuild. Unload and reload the dri     ver.\n");

extra spaces in 'driver'

> +		goto adev_unlock;
> +	}
> +
> +	if (enable)
> +		vsi->tx_rings[queue]->flags |= ICE_TX_FLAGS_TXTIME;
> +
> +	if (!if_running)
> +		goto adev_unlock;
> +	ice_vsi_open(vsi);
> +
> +adev_unlock:
> +	if (locked) {
> +		device_unlock(&pf->adev->dev);
> +		mutex_unlock(&pf->adev_mutex);
> +	}
> +	clear_bit(ICE_CFG_BUSY, vsi->back->state);
> +	return ret;
> +}
> +
> +/**
> + * ice_offload_txtime - set earliest TxTime first
> + * @netdev: network interface device structure
> + * @qopt_off: etf queue option offload from the skb to set
> + *
> + * Return: 0 on success, negative value on failure.
> + */
> +static int ice_offload_txtime(struct net_device *netdev,
> +			      void *qopt_off)
> +{
> +	struct ice_netdev_priv *np = netdev_priv(netdev);
> +	struct ice_pf *pf = np->vsi->back;
> +	struct tc_etf_qopt_offload *qopt;
> +	struct ice_vsi *vsi = np->vsi;
> +	struct ice_tx_ring *tx_ring;
> +	int ret = 0;
> +
> +	if (!ice_is_feature_supported(pf, ICE_F_TXTIME))
> +		return -EOPNOTSUPP;
> +
> +	qopt = qopt_off;
> +	if (!qopt_off || qopt->queue < 0 || qopt->queue >= vsi->num_txq)
> +		return -EINVAL;
> +
> +	tx_ring = vsi->tx_rings[qopt->queue];
> +
> +	/* Enable or disable TxTime on the specified Tx queue. */
> +	if (qopt->enable)
> +		tx_ring->flags |= ICE_TX_FLAGS_TXTIME;
> +	else
> +		tx_ring->flags &= ~ICE_TX_FLAGS_TXTIME;
> +
> +	/* When TxTime is first enabled on any Tx queue or is disabled on all
> +	 * Tx queues, then configure TxTime to allocate or free resources.
> +	 */
> +	if (!test_bit(ICE_FLAG_TXTIME, pf->flags) || !ice_is_txtime_ena(vsi)) {
> +		ret = ice_vsi_cfg_txtime(vsi, qopt->enable, qopt->queue);
> +		if (ret)
> +			goto err;
> +	} else if (netif_running(netdev)) {
> +		struct ice_aqc_ena_dis_txtime_qgrp txtime_pg;
> +		struct ice_hw *hw = &pf->hw;
> +
> +		/* If queues are allocated and configured (running), then enable
> +		 * or disable TxTime on the specified queue.
> +		 */
> +		ret = ice_aq_ena_dis_txtimeq(hw, qopt->queue, 1, qopt->enable,
> +					     &txtime_pg, NULL);
> +		if (ret)
> +			goto err;
> +	}
> +	netdev_info(netdev, "%s TxTime on queue: %i\n",
> +		    qopt->enable ? "enable" : "disable", qopt->queue);

string_choices helper can be used here

> +
> +	return 0;
> +
> +err:
> +	netdev_err(netdev, "Failed to %s TxTime on queue: %i\n",
> +		   qopt->enable ? "enable" : "disable", qopt->queue);

and here

Thanks,
Tony

Powered by blists - more mailing lists