On 8/30/2024 6:30 AM, Alexander Lobakin wrote:
> From: Paul Greenwalt <[email protected]>
> Date: Mon, 26 Aug 2024 13:39:16 -0400
>
>> E830 supports generic receive and HW_CSUM transmit checksumming.
>>
>> Generic receive checksum support is provided by hardware calculating the
>> checksum over the whole packet and providing it to the driver in the Rx
>> flex descriptor. Then the driver assigns the checksum to skb-->csum and
>> sets skb->ip_summed to CHECKSUM_COMPLETE.
>>
>> E830 HW_CSUM transmit checksum does not support TCP Segmentation Offload
>
> Why is it so?
>
>> (TSO) inner packet modification, so TSO and HW_CSUM are mutually exclusive.
>
> What is HW_CSUM in your opinion here?
>
>> Therefore NETIF_F_HW_CSUM hardware feature support is indicated but is not
>> enabled by default. Instead, IP checksum and TSO are the defaults.
>> Enforcement of mutual exclusivity of TSO and HW_CSUM is done in
>> ice_fix_features(). Mutual exclusivity of IP checksum and HW_CSUM is
>> handled by netdev_fix_features().
>>
>> When NETIF_F_HW_CSUM is requested the provide skb->csum_start and
>
> "the provided"?
>
>> skb->csum_offset are passed to hardware in the Tx context descriptor
>> generic checksum (GCS) parameters. Hardware calculates the 1's complement
>> from skb->csum_start to the end of the packet, and inserts the result in
>> the packet at skb->csum_offset.
>>
>> Co-developed-by: Alice Michael <[email protected]>
>> Signed-off-by: Alice Michael <[email protected]>
>> Co-developed-by: Eric Joyner <[email protected]>
>> Signed-off-by: Eric Joyner <[email protected]>
>> Signed-off-by: Paul Greenwalt <[email protected]>
>
> [...]
>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
>> b/drivers/net/ethernet/intel/ice/ice_lib.c
>> index f559e60992fa..118ac34f89e9 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_lib.c
>> @@ -1380,6 +1380,9 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
>> ring->flags |= ICE_TX_FLAGS_RING_VLAN_L2TAG2;
>> else
>> ring->flags |= ICE_TX_FLAGS_RING_VLAN_L2TAG1;
>> +
>> + if (ice_is_feature_supported(pf, ICE_F_GCS))
>> + ring->flags |= ICE_TXRX_FLAGS_GCS_ENA;
>
> An empty newline here, since WRITE_ONCE() is not related to this one?
>
>> WRITE_ONCE(vsi->tx_rings[i], ring);
>> }
>>
>> @@ -1399,6 +1402,9 @@ static int ice_vsi_alloc_rings(struct ice_vsi *vsi)
>> ring->dev = dev;
>> ring->count = vsi->num_rx_desc;
>> ring->cached_phctime = pf->ptp.cached_phc_time;
>> +
>> + if (ice_is_feature_supported(pf, ICE_F_GCS))
>> + ring->flags |= ICE_TXRX_FLAGS_GCS_ENA;
>
> Same here.
>
>> WRITE_ONCE(vsi->rx_rings[i], ring);
>> }
>>
>> @@ -3896,6 +3902,9 @@ void ice_init_feature_support(struct ice_pf *pf)
>> default:
>> break;
>> }
>> +
>> + if (pf->hw.mac_type == ICE_MAC_E830)
>> + ice_set_feature_support(pf, ICE_F_GCS);
>> }
>>
>> /**
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>> b/drivers/net/ethernet/intel/ice/ice_main.c
>> index 6f97ed471fe9..67e32ac962df 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -3678,6 +3678,12 @@ static void ice_set_netdev_features(struct net_device
>> *netdev)
>> */
>> netdev->hw_features |= NETIF_F_RXFCS;
>>
>> + /* Mutual exclusivity for TSO and GCS is enforced by the
>> + * ice_fix_features() ndo callback.
>> + */
>> + if (ice_is_feature_supported(pf, ICE_F_GCS))
>> + netdev->hw_features |= NETIF_F_HW_CSUM;
>> +
>> netif_set_tso_max_size(netdev, ICE_MAX_TSO_SIZE);
>> }
>>
>> @@ -6237,6 +6243,7 @@ ice_fix_features(struct net_device *netdev,
>> netdev_features_t features)
>> struct ice_netdev_priv *np = netdev_priv(netdev);
>> netdev_features_t req_vlan_fltr, cur_vlan_fltr;
>> bool cur_ctag, cur_stag, req_ctag, req_stag;
>> + netdev_features_t changed;
>>
>> cur_vlan_fltr = netdev->features & NETIF_VLAN_FILTERING_FEATURES;
>> cur_ctag = cur_vlan_fltr & NETIF_F_HW_VLAN_CTAG_FILTER;
>> @@ -6285,6 +6292,29 @@ ice_fix_features(struct net_device *netdev,
>> netdev_features_t features)
>> features &= ~NETIF_VLAN_STRIPPING_FEATURES;
>> }
>>
>> + if (!ice_is_feature_supported(np->vsi->back, ICE_F_GCS) ||
>> + !(features & NETIF_F_HW_CSUM))
>> + return features;
>> +
>> + changed = netdev->features ^ features;
>> +
>> + /* HW checksum feature is supported and set, so enforce mutual
>> + * exclusivity of TSO and GCS.
>> + */
>> + if (features & NETIF_F_ALL_TSO) {
>
> if (!(features & ALL_TSO))
> return features;
>
> to reduce indent level and avoid huge `if` blocks.
>
>> + if (changed & NETIF_F_HW_CSUM && changed & NETIF_F_ALL_TSO) {
>> + netdev_warn(netdev, "Dropping TSO and HW checksum. TSO
>> and HW checksum are mutually exclusive.\n");
>> + features &= ~NETIF_F_HW_CSUM;
>> + features &= ~((features & changed) & NETIF_F_ALL_TSO);
>> + } else if (changed & NETIF_F_HW_CSUM) {
>> + netdev_warn(netdev, "Dropping HW checksum. TSO and HW
>> checksum are mutually exclusive.\n");
>> + features &= ~NETIF_F_HW_CSUM;
>> + } else {
>> + netdev_warn(netdev, "Dropping TSO. TSO and HW checksum
>> are mutually exclusive.\n");
>> + features &= ~NETIF_F_ALL_TSO;
>> + }
>> + }
>> +
>> return features;
>> }
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c
>> b/drivers/net/ethernet/intel/ice/ice_txrx.c
>> index 8d25b6981269..bfcce1eab243 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
>> @@ -1792,6 +1792,7 @@ ice_tx_map(struct ice_tx_ring *tx_ring, struct
>> ice_tx_buf *first,
>> static
>> int ice_tx_csum(struct ice_tx_buf *first, struct ice_tx_offload_params *off)
>> {
>> + const struct ice_tx_ring *tx_ring = off->tx_ring;
>> u32 l4_len = 0, l3_len = 0, l2_len = 0;
>> struct sk_buff *skb = first->skb;
>> union {
>> @@ -1941,6 +1942,29 @@ int ice_tx_csum(struct ice_tx_buf *first, struct
>> ice_tx_offload_params *off)
>> l3_len = l4.hdr - ip.hdr;
>> offset |= (l3_len / 4) << ICE_TX_DESC_LEN_IPLEN_S;
>>
>> +#define TX_GCS_ENABLED 1
>
> This must be somewhere where the offload params or descriptor values are
> described, i.e. next to the related definitions.
>
>> + if (tx_ring->netdev->features & NETIF_F_HW_CSUM &&
>
> Please give bitops a separate set of parenthesis
>
> if ((features & HW_CSUM) &&
>
> etc., below as well.
>
>> + tx_ring->flags & ICE_TXRX_FLAGS_GCS_ENA &&
>> + !(first->tx_flags & ICE_TX_FLAGS_TSO) &&
>> + !skb_csum_is_sctp(skb)) {
>> + /* Set GCS */
>> + u16 gcs_params = ((skb->csum_start - skb->mac_header) / 2) <<
>> + ICE_TX_GCS_DESC_START;
>
> This must be
>
> gcs_params = FIELD_PREP(GCS_DESC_MASK, (skb...) / 2)
>
> 2 places below as well.
>
>> + gcs_params |= (skb->csum_offset / 2) << ICE_TX_GCS_DESC_OFFSET;
>> + /* Type is 1 for 16-bit TCP/UDP checksums w/ pseudo */
>> + gcs_params |= TX_GCS_ENABLED << ICE_TX_GCS_DESC_TYPE;
>> +
>> + /* Unlike legacy HW checksums, GCS requires a context
>> + * descriptor.
>> + */
>> + off->cd_qw1 |= (u64)(ICE_TX_DESC_DTYPE_CTX);
>
> Redundant cast.
>
>> + off->cd_gcs_params = gcs_params;
>> + /* Fill out CSO info in data descriptors */
>> + off->td_offset |= offset;
>> + off->td_cmd |= cmd;
>> + return 1;
>> + }
>
> [...]
>
>> @@ -366,6 +367,7 @@ struct ice_rx_ring {
>> #define ICE_RX_FLAGS_RING_BUILD_SKB BIT(1)
>> #define ICE_RX_FLAGS_CRC_STRIP_DIS BIT(2)
>> #define ICE_RX_FLAGS_MULTIDEV BIT(3)
>> +#define ICE_TXRX_FLAGS_GCS_ENA BIT(4)
>
> As Tony said, RX and TX features are now separated, just like Rx and Tx
> ring structures. Please define them separately for Rx and Tx.
>
>> u8 flags;
>> /* CL5 - 5th cacheline starts here */
>> struct xdp_rxq_info xdp_rxq;
>> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> index 2719f0e20933..a344886d2129 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_txrx_lib.c
>> @@ -80,6 +80,24 @@ ice_rx_hash_to_skb(const struct ice_rx_ring *rx_ring,
>> libeth_rx_pt_set_hash(skb, hash, decoded);
>> }
>>
>> +/**
>> + * ice_rx_gcs - Set generic checksum in skd
>
> "skb"
>
>> + * @skb: skb currently being received and modified
>> + * @rx_desc: Receive descriptor
>
> Lowercase for "Receive" I'd say.
>
>> + * Returns hash, if present, 0 otherwise.
>
> The function returns void.
>
>> + */
>> +static void ice_rx_gcs(struct sk_buff *skb, union ice_32b_rx_flex_desc
>> *rx_desc)
>
> const union.
>
>> +{
>> + struct ice_32b_rx_flex_desc_nic *nic_csum;
>
> It's a descriptor, not a csum. + also const.
>
>> +
>> + if (rx_desc->wb.rxdid != ICE_RXDID_FLEX_NIC)
>> + return;
>
> Redundant check since you're checking this below.
>
>> +
>> + nic_csum = (struct ice_32b_rx_flex_desc_nic *)rx_desc;
>> + skb->ip_summed = CHECKSUM_COMPLETE;
>> + skb->csum = (__force __wsum)htons(le16_to_cpu(nic_csum->raw_csum));
>
> htons(le16_to_cpu(x)) is the same as swab16(x), have you tried it?
>
>> +}
>> +
>> /**
>> * ice_rx_csum - Indicate in skb if checksum is good
>> * @ring: the ring we care about
>> @@ -107,6 +125,21 @@ ice_rx_csum(struct ice_rx_ring *ring, struct sk_buff
>> *skb,
>> rx_status0 = le16_to_cpu(rx_desc->wb.status_error0);
>> rx_status1 = le16_to_cpu(rx_desc->wb.status_error1);
>>
>> + if (rx_desc->wb.rxdid == ICE_RXDID_FLEX_NIC &&
>> + ring->flags & ICE_TXRX_FLAGS_GCS_ENA &&
>
> I'd reorder these. 1 flags, 2 flex.
>
>> + (decoded.inner_prot == LIBETH_RX_PT_INNER_TCP ||
>> + decoded.inner_prot == LIBETH_RX_PT_INNER_UDP ||
>> + decoded.inner_prot == LIBETH_RX_PT_INNER_ICMP)) {
>> + /* ICE_RX_FLEX_DESC_STATUS1_L2TAG2P_S is overloaded in the
>> + * rx_status1 layout to indicate that the extracted data from
>> + * the packet is valid.
>> + */
>> + if (rx_status1 & BIT(ICE_RX_FLEX_DESC_STATUS1_L2TAG2P_S))
>> + return ice_rx_gcs(skb, rx_desc);
>
> ice_rx_csum() is void. Please separate the call and the return.
>
>> +
>> + goto checksum_fail;
>
> Why fail? If there's no STATUS1_L2TAG2P_S bit, can't there be a usual
> checksum status?
>
Hi Olek,
I'm preparing a v2 based on yours and Tony's feedback.
I will need to check into the conditions that can lead to Rx flex
descriptor valid bit for Rx checksum not being set
(ICE_RX_FLEX_DESC_STATUS1_L2TAG2P_S), and if it is valid to check the
protocal checksum.
Thanks,
Paul
>> + }
>> +
>> /* check if HW has decoded the packet and checksum */
>> if (!(rx_status0 & BIT(ICE_RX_FLEX_DESC_STATUS0_L3L4P_S)))
>> return;
>
> Thanks,
> Olek
