| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20250506202413.GY3339421@horms.kernel.org>
Date: Tue, 6 May 2025 21:24:13 +0100
From: Simon Horman <horms@...nel.org>
To: Tanmay Jagdale <tanmay@...vell.com>
Cc: bbrezillon@...nel.org, arno@...isbad.org, schalla@...vell.com,
herbert@...dor.apana.org.au, davem@...emloft.net,
sgoutham@...vell.com, lcherian@...vell.com, gakula@...vell.com,
jerinj@...vell.com, hkelam@...vell.com, sbhatta@...vell.com,
andrew+netdev@...n.ch, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, bbhushan2@...vell.com, bhelgaas@...gle.com,
pstanner@...hat.com, gregkh@...uxfoundation.org,
peterz@...radead.org, linux@...blig.org,
krzysztof.kozlowski@...aro.org, giovanni.cabiddu@...el.com,
linux-crypto@...r.kernel.org, linux-kernel@...r.kernel.org,
netdev@...r.kernel.org, rkannoth@...vell.com, sumang@...vell.com,
gcherian@...vell.com
Subject: Re: [net-next PATCH v1 02/15] octeontx2-af: Configure crypto
hardware for inline ipsec
On Fri, May 02, 2025 at 06:49:43PM +0530, Tanmay Jagdale wrote:
> From: Bharat Bhushan <bbhushan2@...vell.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@...vell.com>
> Signed-off-by: Tanmay Jagdale <tanmay@...vell.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.
...
Powered by blists - more mailing lists