Re: [net-next PATCH v1 09/15] octeontx2-pf: ipsec: Allocate Ingress SA table

Tanmay Jagdale
2025-05-22T09:22:18Z

Hi Simon,

On 2025-05-07 at 18:26:25, Simon Horman (horms@kernel.org) wrote:
> On Fri, May 02, 2025 at 06:49:50PM +0530, Tanmay Jagdale wrote:
> > Every NIX LF has the facility to maintain a contiguous SA table that
> > is used by NIX RX to find the exact SA context pointer associated with
> > a particular flow. Allocate a 128-entry SA table where each entry is of
> > 2048 bytes which is enough to hold the complete inbound SA context.
> >
> > Add the structure definitions for SA context (cn10k_rx_sa_s) and
> > SA bookkeeping information (ctx_inb_ctx_info).
> >
> > Also, initialize the inb_sw_ctx_list to track all the SA's and their
> > associated NPC rules and hash table related data.
> >
> > Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>
>
> ...
>
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h
>
> ...
>
> > @@ -146,6 +169,76 @@ struct cn10k_tx_sa_s {
> > u64 hw_ctx[6]; /* W31 - W36 */
> > };
> >
> > +struct cn10k_rx_sa_s {
> > + u64 inb_ar_win_sz : 3; /* W0 */
> > + u64 hard_life_dec : 1;
> > + u64 soft_life_dec : 1;
> > + u64 count_glb_octets : 1;
> > + u64 count_glb_pkts : 1;
> > + u64 count_mib_bytes : 1;
> > + u64 count_mib_pkts : 1;
> > + u64 hw_ctx_off : 7;
> > + u64 ctx_id : 16;
> > + u64 orig_pkt_fabs : 1;
> > + u64 orig_pkt_free : 1;
> > + u64 pkind : 6;
> > + u64 rsvd_w0_40 : 1;
> > + u64 eth_ovrwr : 1;
> > + u64 pkt_output : 2;
> > + u64 pkt_format : 1;
> > + u64 defrag_opt : 2;
> > + u64 x2p_dst : 1;
> > + u64 ctx_push_size : 7;
> > + u64 rsvd_w0_55 : 1;
> > + u64 ctx_hdr_size : 2;
> > + u64 aop_valid : 1;
> > + u64 rsvd_w0_59 : 1;
> > + u64 ctx_size : 4;
> > +
> > + u64 rsvd_w1_31_0 : 32; /* W1 */
> > + u64 cookie : 32;
> > +
> > + u64 sa_valid : 1; /* W2 Control Word */
> > + u64 sa_dir : 1;
> > + u64 rsvd_w2_2_3 : 2;
> > + u64 ipsec_mode : 1;
> > + u64 ipsec_protocol : 1;
> > + u64 aes_key_len : 2;
> > + u64 enc_type : 3;
> > + u64 life_unit : 1;
> > + u64 auth_type : 4;
> > + u64 encap_type : 2;
> > + u64 et_ovrwr_ddr_en : 1;
> > + u64 esn_en : 1;
> > + u64 tport_l4_incr_csum : 1;
> > + u64 iphdr_verify : 2;
> > + u64 udp_ports_verify : 1;
> > + u64 l2_l3_hdr_on_error : 1;
> > + u64 rsvd_w25_31 : 7;
> > + u64 spi : 32;
>
> As I understand it, this driver is only intended to run on arm64 systems.
> While it is also possible, with COMPILE_TEST test, to compile the driver
> on for 64-bit systems.
Yes, this driver works only on Marvell CN10K SoC. I have COMPILE_TESTed
on x86 and ARM64 platforms.

>
> So, given the first point above, this may be moot. But the above
> assumes that the byte order of the host is the same as the device.
> Or perhaps more to the point, it has been written for a little-endian
> host and the device is expecting the data in that byte order.
>
> But u64 is supposed to represent host byte order. And, in my understanding
> of things, this is the kind of problem that FIELD_PREP and FIELD_GET are
> intended to avoid, when combined on endian-specific integer types (in this
> case __le64 seems appropriate).
>
> I do hesitate in bringing this up, as the above very likely works on
> all systems on which this code is intended to run. But I do so
> because it is not correct on all systems for which this code can be
> compiled. And thus seems somehow misleading.
Okay. Are you referring to a case where we compile on BE machine
and then run on LE platform?

With Regards,
Tanmay
>
> > +
> > + u64 w3; /* W3 */
> > +
> > + u8 cipher_key[32]; /* W4 - W7 */
> > + u32 rsvd_w8_0_31; /* W8 : IV */
> > + u32 iv_gcm_salt;
> > + u64 rsvd_w9; /* W9 */
> > + u64 rsvd_w10; /* W10 : UDP Encap */
> > + u32 dest_ipaddr; /* W11 - Tunnel mode: outer src and dest ipaddr */
> > + u32 src_ipaddr;
> > + u64 rsvd_w12_w30[19]; /* W12 - W30 */
> > +
> > + u64 ar_base; /* W31 */
> > + u64 ar_valid_mask; /* W32 */
> > + u64 hard_sa_life; /* W33 */
> > + u64 soft_sa_life; /* W34 */
> > + u64 mib_octs; /* W35 */
> > + u64 mib_pkts; /* W36 */
> > + u64 ar_winbits; /* W37 */
> > +
> > + u64 rsvd_w38_w100[63];
> > +};
> > +
> > /* CPT instruction parameter-1 */
> > #define CN10K_IPSEC_INST_PARAM1_DIS_L4_CSUM 0x1
> > #define CN10K_IPSEC_INST_PARAM1_DIS_L3_CSUM 0x2