Re: [net-next PATCH v1 02/15] octeontx2-af: Configure crypto hardware for inline ipsec

[Simon Horman <horms@kernel.org>]

On Fri, May 02, 2025 at 06:49:43PM +0530, Tanmay Jagdale wrote:
> From: Bharat Bhushan <bbhushan2@marvell.com>
> 
> Currently cpt_rx_inline_lf_cfg mailbox is handled by CPT PF
> driver to configures inbound inline ipsec. Ideally inbound
> inline ipsec configuration should be done by AF driver.
> 
> This patch adds support to allocate, attach and initialize
> a cptlf from AF. It also configures NIX to send CPT instruction
> if the packet needs inline ipsec processing and configures
> CPT LF to handle inline inbound instruction received from NIX.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> Signed-off-by: Tanmay Jagdale <tanmay@marvell.com>

Hi Bharat and Tanmay,

Some minor feedback from my side.

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> index 973ff5cf1a7d..8540a04a92f9 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
> @@ -1950,6 +1950,20 @@ enum otx2_cpt_eng_type {
>  	OTX2_CPT_MAX_ENG_TYPES,
>  };
>  
> +struct cpt_rx_inline_lf_cfg_msg {
> +	struct mbox_msghdr hdr;
> +	u16 sso_pf_func;
> +	u16 param1;
> +	u16 param2;
> +	u16 opcode;
> +	u32 credit;
> +	u32 credit_th;
> +	u16 bpid;

On arm64 (at least) there will be a 2 byte hole here. Is that intended?

And, not strictly related to this patch, struct mboxhdr also has
a 2 byte hole before it's rc member. Perhaps would be nice
if it was it filled by a reserved member?

> +	u32 reserved;
> +	u8 ctx_ilen_valid : 1;
> +	u8 ctx_ilen : 7;
> +};
> +
>  struct cpt_set_egrp_num {
>  	struct mbox_msghdr hdr;
>  	bool set;

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> index fa403da555ff..6923fd756b19 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> +++ b/drivers/net/ethernet/marvell/octeontx2/af/rvu.h
> @@ -525,8 +525,38 @@ struct rvu_cpt_eng_grp {
>  	u8 grp_num;
>  };
>  
> +struct rvu_cpt_rx_inline_lf_cfg {
> +	u16 sso_pf_func;
> +	u16 param1;
> +	u16 param2;
> +	u16 opcode;
> +	u32 credit;
> +	u32 credit_th;
> +	u16 bpid;

FWIIW, there is a hole here too.

> +	u32 reserved;
> +	u8 ctx_ilen_valid : 1;
> +	u8 ctx_ilen : 7;
> +};

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.c

...

> @@ -1087,6 +1115,72 @@ static void cpt_rxc_teardown(struct rvu *rvu, int blkaddr)
>  #define DQPTR      GENMASK_ULL(19, 0)
>  #define NQPTR      GENMASK_ULL(51, 32)
>  
> +static void cpt_rx_ipsec_lf_enable_iqueue(struct rvu *rvu, int blkaddr,
> +					  int slot)
> +{
> +	u64 val;
> +
> +	/* Set Execution Enable of instruction queue */
> +	val = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot, CPT_LF_INPROG);
> +	val |= BIT_ULL(16);

Bit 16 seems to have a meaning, it would be nice if a #define was used
I mean something like this (but probably not actually this :)

#define CPT_LF_INPROG_ENA_QUEUE BIT_ULL(16)

Perhaps defined near where CPT_LF_INPROG is defined.

> +	otx2_cpt_write64(rvu->pfreg_base, blkaddr, slot, CPT_LF_INPROG, val);
> +
> +	/* Set iqueue's enqueuing */
> +	val = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot, CPT_LF_CTL);
> +	val |= BIT_ULL(0);

Ditto.

> +	otx2_cpt_write64(rvu->pfreg_base, blkaddr, slot, CPT_LF_CTL, val);
> +}
> +
> +static void cpt_rx_ipsec_lf_disable_iqueue(struct rvu *rvu, int blkaddr,
> +					   int slot)
> +{
> +	int timeout = 1000000;
> +	u64 inprog, inst_ptr;
> +	u64 qsize, pending;
> +	int i = 0;
> +
> +	/* Disable instructions enqueuing */
> +	otx2_cpt_write64(rvu->pfreg_base, blkaddr, slot, CPT_LF_CTL, 0x0);
> +
> +	inprog = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot, CPT_LF_INPROG);
> +	inprog |= BIT_ULL(16);
> +	otx2_cpt_write64(rvu->pfreg_base, blkaddr, slot, CPT_LF_INPROG, inprog);
> +
> +	qsize = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot, CPT_LF_Q_SIZE)
> +		 & 0x7FFF;
> +	do {
> +		inst_ptr = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot,
> +					   CPT_LF_Q_INST_PTR);
> +		pending = (FIELD_GET(XQ_XOR, inst_ptr) * qsize * 40) +
> +			  FIELD_GET(NQPTR, inst_ptr) -
> +			  FIELD_GET(DQPTR, inst_ptr);

nit: I don't think you need the outer parentheses here.
     But if you do, the two lines above sould be indented by one more
     character.

> +		udelay(1);
> +		timeout--;
> +	} while ((pending != 0) && (timeout != 0));

nit: I don't think you need the inner parenthese here (x2).

> +
> +	if (timeout == 0)
> +		dev_warn(rvu->dev, "TIMEOUT: CPT poll on pending instructions\n");
> +
> +	timeout = 1000000;
> +	/* Wait for CPT queue to become execution-quiescent */
> +	do {
> +		inprog = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot,
> +					 CPT_LF_INPROG);
> +		if ((FIELD_GET(INFLIGHT, inprog) == 0) &&
> +		    (FIELD_GET(GRB_CNT, inprog) == 0)) {
> +			i++;
> +		} else {
> +			i = 0;
> +			timeout--;
> +		}
> +	} while ((timeout != 0) && (i < 10));
> +
> +	if (timeout == 0)
> +		dev_warn(rvu->dev, "TIMEOUT: CPT poll on inflight count\n");
> +	/* Wait for 2 us to flush all queue writes to memory */
> +	udelay(2);
> +}
> +
>  static void cpt_lf_disable_iqueue(struct rvu *rvu, int blkaddr, int slot)
>  {
>  	int timeout = 1000000;
> @@ -1310,6 +1404,474 @@ int rvu_cpt_ctx_flush(struct rvu *rvu, u16 pcifunc)
>  	return 0;
>  }
>  
> +static irqreturn_t rvu_cpt_rx_ipsec_misc_intr_handler(int irq, void *ptr)
> +{
> +	struct rvu_block *block = ptr;
> +	struct rvu *rvu = block->rvu;
> +	int blkaddr = block->addr;
> +	struct device *dev = rvu->dev;
> +	int slot = 0;
> +	u64 val;
> +
> +	val = otx2_cpt_read64(rvu->pfreg_base, blkaddr, slot, CPT_LF_MISC_INT);
> +
> +	if (val & (1 << 6)) {

Allong the lines of my earlier comment, bit 6 seems to have a meaning too.
Likewise for other bits below.

> +		dev_err(dev, "Memory error detected while executing CPT_INST_S, LF %d.\n",
> +			slot);
> +	} else if (val & (1 << 5)) {
> +		dev_err(dev, "HW error from an engine executing CPT_INST_S, LF %d.",
> +			slot);
> +	} else if (val & (1 << 3)) {
> +		dev_err(dev, "SMMU fault while writing CPT_RES_S to CPT_INST_S[RES_ADDR], LF %d.\n",
> +			slot);
> +	} else if (val & (1 << 2)) {
> +		dev_err(dev, "Memory error when accessing instruction memory queue CPT_LF_Q_BASE[ADDR].\n");
> +	} else if (val & (1 << 1)) {
> +		dev_err(dev, "Error enqueuing an instruction received at CPT_LF_NQ.\n");
> +	} else {
> +		dev_err(dev, "Unhandled interrupt in CPT LF %d\n", slot);
> +		return IRQ_NONE;
> +	}
> +
> +	/* Acknowledge interrupts */
> +	otx2_cpt_write64(rvu->pfreg_base, blkaddr, slot, CPT_LF_MISC_INT,
> +			 val & CPT_LF_MISC_INT_MASK);
> +
> +	return IRQ_HANDLED;
> +}

...

> +/* Allocate memory for CPT outbound Instruction queue.
> + * Instruction queue memory format is:
> + *      -----------------------------
> + *     | Instruction Group memory    |
> + *     |  (CPT_LF_Q_SIZE[SIZE_DIV40] |
> + *     |   x 16 Bytes)               |
> + *     |                             |
> + *      ----------------------------- <-- CPT_LF_Q_BASE[ADDR]
> + *     | Flow Control (128 Bytes)    |
> + *     |                             |
> + *      -----------------------------
> + *     |  Instruction Memory         |
> + *     |  (CPT_LF_Q_SIZE[SIZE_DIV40] |
> + *     |   × 40 × 64 bytes)          |
> + *     |                             |
> + *      -----------------------------
> + */

Nice diagram :)

...

> +static int rvu_rx_cpt_set_grp_pri_ilen(struct rvu *rvu, int blkaddr, int cptlf)
> +{
> +	u64 reg_val;
> +
> +	reg_val = rvu_read64(rvu, blkaddr, CPT_AF_LFX_CTL(cptlf));
> +	/* Set High priority */
> +	reg_val |= 1;
> +	/* Set engine group */
> +	reg_val |= ((1ULL << rvu->rvu_cpt.inline_ipsec_egrp) << 48);
> +	/* Set ilen if valid */
> +	if (rvu->rvu_cpt.rx_cfg.ctx_ilen_valid)
> +		reg_val |= rvu->rvu_cpt.rx_cfg.ctx_ilen  << 17;

Along the same lines. 48 and 17 seem to have meaning.
Perhaps define appropriate masks created using GENMASK_ULL
and use FIELD_PREP?

> +
> +	rvu_write64(rvu, blkaddr, CPT_AF_LFX_CTL(cptlf), reg_val);
> +	return 0;
> +}

...

> +static void rvu_rx_cptlf_cleanup(struct rvu *rvu, int blkaddr, int slot)
> +{
> +	/* IRQ cleanup */
> +	rvu_cpt_rx_inline_cleanup_irq(rvu, blkaddr, slot);
> +
> +	/* CPTLF cleanup */
> +	rvu_cpt_rx_inline_cptlf_clean(rvu, blkaddr, slot);
> +}
> +
> +int rvu_mbox_handler_cpt_rx_inline_lf_cfg(struct rvu *rvu,
> +					  struct cpt_rx_inline_lf_cfg_msg *req,
> +					  struct msg_rsp *rsp)

Compilers warn that rvu_mbox_handler_cpt_rx_inline_lf_cfg doesn't have
a prototype.

I think this can be resolved by squashing the following hunk,
which appears in a subsequent patch in this series, into this patch.

diff --git a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
index 8540a04a92f9..ad74a27888da 100644
--- a/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
+++ b/drivers/net/ethernet/marvell/octeontx2/af/mbox.h
@@ -213,6 +213,8 @@ M(CPT_FLT_ENG_INFO,     0xA09, cpt_flt_eng_info, cpt_flt_eng_info_req,	\
 			       cpt_flt_eng_info_rsp)			\
 M(CPT_SET_ENG_GRP_NUM,  0xA0A, cpt_set_eng_grp_num, cpt_set_egrp_num,   \
 				msg_rsp)				\
+M(CPT_RX_INLINE_LF_CFG, 0xBFE, cpt_rx_inline_lf_cfg, cpt_rx_inline_lf_cfg_msg, \
+				msg_rsp) \
 /* SDP mbox IDs (range 0x1000 - 0x11FF) */				\
 M(SET_SDP_CHAN_INFO, 0x1000, set_sdp_chan_info, sdp_chan_info_msg, msg_rsp) \
 M(GET_SDP_CHAN_INFO, 0x1001, get_sdp_chan_info, msg_req, sdp_get_chan_info_msg) \

...

> diff --git a/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.h b/drivers/net/ethernet/marvell/octeontx2/af/rvu_cpt.h

...

> +/* CPT instruction queue length in bytes */
> +#define RVU_CPT_INST_QLEN_BYTES                                               \
> +		((RVU_CPT_SIZE_DIV40 * 40 * RVU_CPT_INST_SIZE) +             \
> +		RVU_CPT_INST_QLEN_EXTRA_BYTES)

nit: I think the line above should be indented by one more character

> +
> +/* CPT instruction group queue length in bytes */
> +#define RVU_CPT_INST_GRP_QLEN_BYTES                                           \
> +		((RVU_CPT_SIZE_DIV40 + RVU_CPT_EXTRA_SIZE_DIV40) * 16)
> +
> +/* CPT FC length in bytes */
> +#define RVU_CPT_Q_FC_LEN 128
> +
> +/* CPT LF_Q_SIZE Register */
> +#define CPT_LF_Q_SIZE_DIV40 GENMASK_ULL(14, 0)
> +
> +/* CPT invalid engine group num */
> +#define OTX2_CPT_INVALID_CRYPTO_ENG_GRP 0xFF
> +
> +/* Fastpath ipsec opcode with inplace processing */
> +#define OTX2_CPT_INLINE_RX_OPCODE (0x26 | (1 << 6))
> +#define CN10K_CPT_INLINE_RX_OPCODE (0x29 | (1 << 6))

Along the lines of earlier comments, bit 6 seems to have a meaning here.

> +
> +/* Calculate CPT register offset */
> +#define CPT_RVU_FUNC_ADDR_S(blk, slot, offs) \
> +		(((blk) << 20) | ((slot) << 12) | (offs))

And perhaps this is another candidate for GENMASK + FIELD_PREP.

...