From mboxrd@z Thu Jan 1 00:00:00 1970 From: Shannon Nelson Subject: Re: [Intel-wired-lan] [next-queue 08/10] ixgbe: process the Tx ipsec offload Date: Wed, 6 Dec 2017 21:43:44 -0800 Message-ID: <1c5fddf2-4d93-d1cf-2ac0-d3b2bcb0c682@oracle.com> References: <1512452116-14795-1-git-send-email-shannon.nelson@oracle.com> <1512452116-14795-9-git-send-email-shannon.nelson@oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: intel-wired-lan , Jeff Kirsher , Steffen Klassert , Sowmini Varadhan , Netdev To: Alexander Duyck Return-path: Received: from userp2120.oracle.com ([156.151.31.85]:54070 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750937AbdLGFnz (ORCPT ); Thu, 7 Dec 2017 00:43:55 -0500 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 12/5/2017 10:13 AM, Alexander Duyck wrote: > On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson > wrote: >> If the skb has a security association referenced in the skb, then >> set up the Tx descriptor with the ipsec offload bits. While we're >> here, we fix an oddly named field in the context descriptor struct. >> >> Signed-off-by: Shannon Nelson >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe.h | 10 +++- >> drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c | 77 ++++++++++++++++++++++++++ >> drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c | 4 +- >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 38 ++++++++++--- >> drivers/net/ethernet/intel/ixgbe/ixgbe_type.h | 2 +- >> 5 files changed, 118 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> index 77f07dc..68097fe 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h >> @@ -171,10 +171,11 @@ enum ixgbe_tx_flags { >> IXGBE_TX_FLAGS_CC = 0x08, >> IXGBE_TX_FLAGS_IPV4 = 0x10, >> IXGBE_TX_FLAGS_CSUM = 0x20, >> + IXGBE_TX_FLAGS_IPSEC = 0x40, >> >> /* software defined flags */ >> - IXGBE_TX_FLAGS_SW_VLAN = 0x40, >> - IXGBE_TX_FLAGS_FCOE = 0x80, >> + IXGBE_TX_FLAGS_SW_VLAN = 0x80, >> + IXGBE_TX_FLAGS_FCOE = 0x100, >> }; >> >> /* VLAN info */ >> @@ -1012,12 +1013,17 @@ void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter); >> void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >> union ixgbe_adv_rx_desc *rx_desc, >> struct sk_buff *skb); >> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb, >> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd); >> void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter); >> #else >> static inline void ixgbe_init_ipsec_offload(struct ixgbe_adapter *adapter) { }; >> static inline void ixgbe_ipsec_rx(struct ixgbe_ring *rx_ring, >> union ixgbe_adv_rx_desc *rx_desc, >> struct sk_buff *skb) { }; >> +static inline int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, >> + struct sk_buff *skb, __be16 protocol, >> + struct ixgbe_ipsec_tx_data *itd) { return 0; }; >> static inline void ixgbe_ipsec_restore(struct ixgbe_adapter *adapter) { }; >> #endif /* CONFIG_XFRM_OFFLOAD */ >> #endif /* _IXGBE_H_ */ >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> index fd06d9b..2a0dd7a 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ipsec.c >> @@ -703,12 +703,89 @@ static void ixgbe_ipsec_del_sa(struct xfrm_state *xs) >> } >> } >> >> +/** >> + * ixgbe_ipsec_offload_ok - can this packet use the xfrm hw offload >> + * @skb: current data packet >> + * @xs: pointer to transformer state struct >> + **/ >> +static bool ixgbe_ipsec_offload_ok(struct sk_buff *skb, struct xfrm_state *xs) >> +{ >> + if (xs->props.family == AF_INET) { >> + /* Offload with IPv4 options is not supported yet */ >> + if (ip_hdr(skb)->ihl > 5) > > I would make this ihl != 5 instead of "> 5" since smaller values would > be invalid as well. Sure > >> + return false; >> + } else { >> + /* Offload with IPv6 extension headers is not support yet */ >> + if (ipv6_ext_hdr(ipv6_hdr(skb)->nexthdr)) >> + return false; >> + } >> + >> + return true; >> +} >> + >> static const struct xfrmdev_ops ixgbe_xfrmdev_ops = { >> .xdo_dev_state_add = ixgbe_ipsec_add_sa, >> .xdo_dev_state_delete = ixgbe_ipsec_del_sa, >> + .xdo_dev_offload_ok = ixgbe_ipsec_offload_ok, >> }; >> >> /** >> + * ixgbe_ipsec_tx - setup Tx flags for ipsec offload >> + * @tx_ring: outgoing context >> + * @skb: current data packet >> + * @protocol: network protocol >> + * @itd: ipsec Tx data for later use in building context descriptor >> + **/ >> +int ixgbe_ipsec_tx(struct ixgbe_ring *tx_ring, struct sk_buff *skb, >> + __be16 protocol, struct ixgbe_ipsec_tx_data *itd) >> +{ >> + struct ixgbe_adapter *adapter = netdev_priv(tx_ring->netdev); >> + struct ixgbe_ipsec *ipsec = adapter->ipsec; >> + struct xfrm_state *xs; >> + struct tx_sa *tsa; >> + >> + if (!skb->sp->len) { >> + netdev_err(tx_ring->netdev, "%s: no xfrm state len = %d\n", >> + __func__, skb->sp->len); >> + return 0; >> + } >> + >> + xs = xfrm_input_state(skb); >> + if (!xs) { >> + netdev_err(tx_ring->netdev, "%s: no xfrm_input_state() xs = %p\n", >> + __func__, xs); >> + return 0; >> + } >> + >> + itd->sa_idx = xs->xso.offload_handle - IXGBE_IPSEC_BASE_TX_INDEX; >> + if (itd->sa_idx > IXGBE_IPSEC_MAX_SA_COUNT) { >> + netdev_err(tx_ring->netdev, "%s: bad sa_idx=%d handle=%lu\n", >> + __func__, itd->sa_idx, xs->xso.offload_handle); >> + return 0; >> + } >> + >> + tsa = &ipsec->tx_tbl[itd->sa_idx]; >> + if (!tsa->used) { >> + netdev_err(tx_ring->netdev, "%s: unused sa_idx=%d\n", >> + __func__, itd->sa_idx); >> + return 0; >> + } >> + >> + itd->flags = 0; >> + if (xs->id.proto == IPPROTO_ESP) { >> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_TYPE_ESP | >> + IXGBE_ADVTXD_TUCMD_L4T_TCP; > > Why is the TCP value being set here? This doesn't seem correct either. > This implies TCP a TCP offload. It seems like this should only be > setting ESP. Honestly? Because when I was testing that, it didn't work without it. This was one of the things I was going to come back to when I started working on the csum and tso support. > >> + if (protocol == htons(ETH_P_IP)) >> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPV4; > > Does the IPsec offload need to know if the frame is v4 or v6? I'm just > wondering if it does or not. Yes, I believe this is how it knows how much header to skip to find the ESP header. However, I'll test that and see if it can come out. > If not then this probably isn't needed. > One thought on this line is you might look at moving it into > ixgbe_tx_csum. If setting the bit is harmless without setting IXSM we > might look at moving it into the end of ixgbe_tx_csum and just make it > compare against first->protocol there. > >> + itd->trailer_len = xs->props.trailer_len; >> + } >> + if (tsa->encrypt) >> + itd->flags |= IXGBE_ADVTXD_TUCMD_IPSEC_ENCRYPT_EN; >> + >> + return 1; >> +} >> + >> +/** >> * ixgbe_ipsec_rx - decode ipsec bits from Rx descriptor >> * @rx_ring: receiving ring >> * @rx_desc: receive data descriptor >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c >> index f1bfae0..d7875b3 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c >> @@ -1261,7 +1261,7 @@ void ixgbe_clear_interrupt_scheme(struct ixgbe_adapter *adapter) >> } >> >> void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring, u32 vlan_macip_lens, >> - u32 fcoe_sof_eof, u32 type_tucmd, u32 mss_l4len_idx) >> + u32 fceof_saidx, u32 type_tucmd, u32 mss_l4len_idx) >> { >> struct ixgbe_adv_tx_context_desc *context_desc; >> u16 i = tx_ring->next_to_use; >> @@ -1275,7 +1275,7 @@ void ixgbe_tx_ctxtdesc(struct ixgbe_ring *tx_ring, u32 vlan_macip_lens, >> type_tucmd |= IXGBE_TXD_CMD_DEXT | IXGBE_ADVTXD_DTYP_CTXT; >> >> context_desc->vlan_macip_lens = cpu_to_le32(vlan_macip_lens); >> - context_desc->seqnum_seed = cpu_to_le32(fcoe_sof_eof); >> + context_desc->fceof_saidx = cpu_to_le32(fceof_saidx); >> context_desc->type_tucmd_mlhl = cpu_to_le32(type_tucmd); >> context_desc->mss_l4len_idx = cpu_to_le32(mss_l4len_idx); >> } >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> index 60f9f2d..c857594 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> @@ -7659,9 +7659,10 @@ static void ixgbe_service_task(struct work_struct *work) >> >> static int ixgbe_tso(struct ixgbe_ring *tx_ring, >> struct ixgbe_tx_buffer *first, >> - u8 *hdr_len) >> + u8 *hdr_len, >> + struct ixgbe_ipsec_tx_data *itd) >> { >> - u32 vlan_macip_lens, type_tucmd, mss_l4len_idx; >> + u32 vlan_macip_lens, type_tucmd, mss_l4len_idx, fceof_saidx = 0; >> struct sk_buff *skb = first->skb; >> union { >> struct iphdr *v4; >> @@ -7740,7 +7741,12 @@ static int ixgbe_tso(struct ixgbe_ring *tx_ring, >> vlan_macip_lens |= (ip.hdr - skb->data) << IXGBE_ADVTXD_MACLEN_SHIFT; >> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK; >> >> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd, >> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) { >> + fceof_saidx |= itd->sa_idx; >> + type_tucmd |= itd->flags | itd->trailer_len; >> + } >> + >> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd, >> mss_l4len_idx); >> >> return 1; >> @@ -7756,10 +7762,12 @@ static inline bool ixgbe_ipv6_csum_is_sctp(struct sk_buff *skb) >> } >> >> static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, >> - struct ixgbe_tx_buffer *first) >> + struct ixgbe_tx_buffer *first, >> + struct ixgbe_ipsec_tx_data *itd) >> { >> struct sk_buff *skb = first->skb; >> u32 vlan_macip_lens = 0; >> + u32 fceof_saidx = 0; >> u32 type_tucmd = 0; >> >> if (skb->ip_summed != CHECKSUM_PARTIAL) { >> @@ -7800,7 +7808,12 @@ static void ixgbe_tx_csum(struct ixgbe_ring *tx_ring, >> vlan_macip_lens |= skb_network_offset(skb) << IXGBE_ADVTXD_MACLEN_SHIFT; >> vlan_macip_lens |= first->tx_flags & IXGBE_TX_FLAGS_VLAN_MASK; >> >> - ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, 0, type_tucmd, 0); >> + if (first->tx_flags & IXGBE_TX_FLAGS_IPSEC) { >> + fceof_saidx |= itd->sa_idx; >> + type_tucmd |= itd->flags | itd->trailer_len; >> + } >> + >> + ixgbe_tx_ctxtdesc(tx_ring, vlan_macip_lens, fceof_saidx, type_tucmd, 0); >> } >> >> #define IXGBE_SET_FLAG(_input, _flag, _result) \ >> @@ -7843,11 +7856,16 @@ static void ixgbe_tx_olinfo_status(union ixgbe_adv_tx_desc *tx_desc, >> IXGBE_TX_FLAGS_CSUM, >> IXGBE_ADVTXD_POPTS_TXSM); >> >> - /* enble IPv4 checksum for TSO */ >> + /* enable IPv4 checksum for TSO */ >> olinfo_status |= IXGBE_SET_FLAG(tx_flags, >> IXGBE_TX_FLAGS_IPV4, >> IXGBE_ADVTXD_POPTS_IXSM); >> >> + /* enable IPsec */ >> + olinfo_status |= IXGBE_SET_FLAG(tx_flags, >> + IXGBE_TX_FLAGS_IPSEC, >> + IXGBE_ADVTXD_POPTS_IPSEC); >> + >> /* >> * Check Context must be set if Tx switch is enabled, which it >> * always is for case where virtual functions are running >> @@ -8306,6 +8324,7 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb, >> u32 tx_flags = 0; >> unsigned short f; >> u16 count = TXD_USE_COUNT(skb_headlen(skb)); >> + struct ixgbe_ipsec_tx_data ipsec_tx = { 0 }; >> __be16 protocol = skb->protocol; >> u8 hdr_len = 0; >> >> @@ -8394,6 +8413,9 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb, >> } >> } >> >> + if (skb->sp && ixgbe_ipsec_tx(tx_ring, skb, protocol, &ipsec_tx)) >> + tx_flags |= IXGBE_TX_FLAGS_IPSEC | IXGBE_TX_FLAGS_CC; > > You might just want to pull the skb->sp check into ixgbe_ipsec_tx and > could pass tx_flags as a part of the first buffer. It doesn't really > matter anyway as most of this will just be inlined so it will all end > up a part of the same function anyway. Since the function is defined in a different .o file, are you sure it will get inlined? I put the skb->sp check here to make sure we don't do an unnecessary jump. > > Also I would move this down so that it is handled after the fields in > the first buffer_info structure are set. Then this can ll just fall > inline with the TSO block and get handled there. > >> + >> /* record initial flags and protocol */ >> first->tx_flags = tx_flags; >> first->protocol = protocol; >> @@ -8410,11 +8432,11 @@ netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb, >> } >> >> #endif /* IXGBE_FCOE */ > > So if you move the function down here it will help to avoid any other > complication. In addition you could follow the same logic that we do > for ixgbe_tso/fso so you could drop the frame instead of transmitting > it if it is requesting a bad offload. Sure sln > >> - tso = ixgbe_tso(tx_ring, first, &hdr_len); >> + tso = ixgbe_tso(tx_ring, first, &hdr_len, &ipsec_tx); >> if (tso < 0) >> goto out_drop; >> else if (!tso) >> - ixgbe_tx_csum(tx_ring, first); >> + ixgbe_tx_csum(tx_ring, first, &ipsec_tx); >> >> /* add the ATR filter if ATR is on */ >> if (test_bit(__IXGBE_TX_FDIR_INIT_DONE, &tx_ring->state)) >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h >> index 3df0763..0ac725fa 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_type.h >> @@ -2856,7 +2856,7 @@ union ixgbe_adv_rx_desc { >> /* Context descriptors */ >> struct ixgbe_adv_tx_context_desc { >> __le32 vlan_macip_lens; >> - __le32 seqnum_seed; >> + __le32 fceof_saidx; >> __le32 type_tucmd_mlhl; >> __le32 mss_l4len_idx; >> }; >> -- >> 2.7.4 >> >> _______________________________________________ >> Intel-wired-lan mailing list >> Intel-wired-lan@osuosl.org >> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan