if_bnxt: add kTLS (kernel TLS) TX offload

[sumit.saxena_broadcom.com]

Summary

Add kTLS TX crypto offload, layered on MPC (crypto add/delete
commands submitted over its TCE channel) and TX completion coalescing
(shared tx_bd_opaque encoding).

Adds kTLS session lifecycle management, a key-context ID allocator,
HWRM key-context alloc/free with partition-mode support, and the TX
submit path: in-order packets go out on the normal iflib path with
CSUM_SND_TAG set, while out-of-order or retransmitted TLS records get
a presync command plus a replayed copy of the record built from the
driver's own replay ring. Also adds kTLS/MPC completion-time counter
sysctls and the max_ktls_entries tunable.

[gallatin]

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

sys/dev/bnxt/bnxt_en/bnxt_hwrm.c

1260

If this can actually be called from here, it could result in sleeping while the hwrm lock is held. But I'm not sure this is even reachable

sys/dev/bnxt/bnxt_en/bnxt_ktls.c

380

This needs to match bnxt_crypto_del()'s condition to return early and not do async work. Eg, it needs to also include test_bit(BNXT_STATE_IN_FW_RESET, &softc->state). Or that check needs to be removed from bnxt_crypto_del(). Else you will submit a stack variable (tmp) for async work.

Then again, i'm not sure what the point of even calling bnxt_crypto_del() is in that case, since it does exactly nothing.

630

You should probably zero this.

678

Don't we need to free the partition allocated above in partition mode?

703

This should move down below the unlock to avoid sleeping while holding a mtx

1066

I think this should not be a pointer, but the actual struct

1432

Other error paths free the mbuf, but that is missing here. i think a defrag failure could leak an mbuf.

1649

I think we are missing the case where m_epg_npgs == 1 and the payload occupies [m_epg_1st_off, m_epg_last_len) inside a single page, so the length is

m_epg_last_len - m_epg_1st_off.

1859

This is reversed in bnxt_crypto_add() via bnxt_copy_tls_mp_data(). Does that need to be done here?

1919

I'd re-write as
if (ptag == NULL || ptag->sw == NULL || ptag->sw->type != IF_SND_TAG_TYPE_TLS)

Eg, the defensiveness here to guard against a null tag only causes you to not deref it here, and to then later deref it.

1976

don't you want a break here? Else a STATE_BAD will count as a STATE_FREED as well

sys/dev/bnxt/bnxt_en/if_bnxt.c

668

Ick. Why vzalloc? This is freebsd

Where is this freed?

[sumit.saxena_broadcom.com]

In D58606#1345673, @gallatin wrote:

Also, bnxt_ktls_pre_xmit() and bnxt_ktls_submit_mbuf() both write BDs and advance txr->prod directly, and neither

compares against the consumer.  Is that safe?

I think it should be safe. While adding Tx ktls support in iflib "43d7ee540efe iflib: support for transmit side nic KTLS offload" we had reserved padding BDs in the Tx ring for bnxt_ktls_prexmit() and bnxt_ktls_submit_mbuf(). bnxt driver sets padding tx BDs in this code:

#ifdef KTLS_IFLIB_SUPPORT

		scctx->isc_tx_pad = BNXT_MAX_NUM_SEGS + 5;

#endif

[sumit.saxena_broadcom.com]

sys/dev/bnxt/bnxt_en/bnxt_hwrm.c

1260

If start of the day, HW ktls offload in enabled in firmware, then the ktls is disabled in firmware followed by the firmware reset(not sure if this operation is allowed). In that case, interface re-init after FW reset may end up calling this code to free up ktls resources. We can hold ktls resources even if HW ktls is disabled on fly. Eventually driver unload can always free up the ktls resources. I think we can safely drop this "else" part.

sys/dev/bnxt/bnxt_en/bnxt_ktls.c

380

This path definitely needs rework. The objective is-

1. For non-fatal reset: Send down delete crypto MPC down to NIC to clear connection at hardware level and if MPC is successfully completed, clear software state of the connection. As the bnxt_crypto_del() is async and we call pci_disable_busmaster() after this async call, we need to make it synchronous for this use case. If the bnxt_crypto_del() is successfully completed by NIC, then clear the software state of connection.

2. For Fatal FW reset: Just clear the software state and need not send down the crypto delete MPC to NIC.

630

Ack, will do this in next revision

678

Yes, we need some clean up / reset here to undo the work done by bnxt_partition_alloc.

703

Ack

1066

Ack, this code is ported from Linux. and I don't think partition mode is ever tested on FreeBSD, so it never get caught.

1432

Ack

1649

Ack, will handle it in next revision.

1859

I think bnxt_copy_tls_mp_data() is not required in bnxt_crypto_add(). bnxt_copy_tls_mp_data() assumes the initial record sequence number passed by kernel is in Network byte-order(big-endian) and converts it into little endian as NIC crypto engine wants the seq number in little-endian. However the kernel passes down the record sequence number in CPU native format to the driver during offload initiation ( kernel code which sets initial seqno- "tls->initial_offload_seqno = be64dec(en->rec_seq)").

Since initial TLS sequence number is mostly 0 (infact I have never seen initial sequence number other than 0), this issue did not get caught in the testing.

1919

Ack

1976

Ack.

sys/dev/bnxt/bnxt_en/if_bnxt.c

668

Ack, will fix it in next revision.