Re: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer

[Leon Romanovsky <leon@kernel.org>]

On Mon, Nov 07, 2022 at 09:46:46AM +0000, Yinjun Zhang wrote:
> On Mon, 7 Nov 2022 08:14:12 +0200, Leon Romanovsky wrote:
>  <...>
> > > +	struct sa_ctrl_word {
> > > +		uint32_t hash   :4;	  /* From nfp_ipsec_sa_hash_type */
> > > +		uint32_t cimode :4;	  /* From nfp_ipsec_sa_cipher_mode */
> > > +		uint32_t cipher :4;	  /* From nfp_ipsec_sa_cipher */
> > > +		uint32_t mode   :2;	  /* From nfp_ipsec_sa_mode */
> > > +		uint32_t proto  :2;	  /* From nfp_ipsec_sa_prot */
> > > +		uint32_t dir :1;	  /* SA direction */
> > > +		uint32_t ena_arw:1;	  /* Anti-Replay Window */
> > > +		uint32_t ext_seq:1;	  /* 64-bit Sequence Num */
> > > +		uint32_t ext_arw:1;	  /* 64b Anti-Replay Window */
> > > +		uint32_t spare2 :9;	  /* Must be set to 0 */
> > > +		uint32_t encap_dsbl:1;	  /* Encap/Decap disable */
> > > +		uint32_t gen_seq:1;	  /* Firmware Generate Seq */
> > > +		uint32_t spare8 :1;	  /* Must be set to 0 */
> > > +	} ctrl_word;
> > > +	u32 spi;			  /* SPI Value */
> > > +	uint32_t pmtu_limit :16;	  /* PMTU Limit */
> > > +	uint32_t spare3     :1;
> > > +	uint32_t frag_check :1;		  /* Stateful fragment checking flag */
> > > +	uint32_t bypass_DSCP:1;		  /* Bypass DSCP Flag */
> > > +	uint32_t df_ctrl    :2;		  /* DF Control bits */
> > > +	uint32_t ipv6       :1;		  /* Outbound IPv6 addr format */
> > > +	uint32_t udp_enable :1;		  /* Add/Remove UDP header for NAT */
> > > +	uint32_t tfc_enable :1;		  /* Traffic Flow Confidentiality */
> > > +	uint32_t spare4	 :8;
> > > +	u32 soft_lifetime_byte_count;
> > > +	u32 hard_lifetime_byte_count;
> > 
> > These fields are not relevant for IPsec crypto offload. I would be more
> > than happy to see only used fields here.
> 
> They are not used currently in kernel indeed. However the HW is not designed for 
> crypto-offloading only, not for kernel only, some extensive features are supported. 
> So they're reserved here.

So mark them as reserved. If it is not supported by kernel, it shouldn't
be in the kernel.

> 
> <...>
> > > +
> > > +	/* General */
> > > +	switch (x->props.mode) {
> > > +	case XFRM_MODE_TUNNEL:
> > > +		cfg->ctrl_word.mode = NFP_IPSEC_PROTMODE_TUNNEL;
> > > +		break;
> > > +	case XFRM_MODE_TRANSPORT:
> > > +		cfg->ctrl_word.mode = NFP_IPSEC_PROTMODE_TRANSPORT;
> > > +		break;
> > 
> > Why is it important for IPsec crypto? The HW logic must be the same for
> > all modes. There are no differences between transport and tunnel.
> 
> As I mentioned above, it's differentiated in HW to support more features.

You are adding crypto offload, so please don't try to sneak "more" features.

> 
> > 
> > > +	default:
> > > +		nn_err(nn, "Unsupported mode for xfrm offload\n");
> > 
> > There are no other modes.
> > 
> 
> <...>
> > 
> > > +	err = xa_alloc(&nn->xa_ipsec, &saidx, x,
> > > +		       XA_LIMIT(0, NFP_NET_IPSEC_MAX_SA_CNT - 1),
> > GFP_KERNEL);
> > 
> > Create XArray with XA_FLAGS_ALLOC1, it will cause to xarray skip 0.
> > See DEFINE_XARRAY_ALLOC1() for more info.
> 
> Actually 0 is a valid SA id in HW/driver, we don't want to skip 0.

NP, thanks