Message-ID: <CAAeCc_kxyxNj47biCf=r2vZQXuC2g7XO524YHfKJQcqpfObpDw@mail.gmail.com>
Date: Fri, 15 Nov 2024 15:43:25 +0530
From: Bharat Bhushan <bharatb.linux@...il.com>
To: Paolo Abeni <pabeni@...hat.com>
Cc: Bharat Bhushan <bbhushan2@...vell.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org, sgoutham@...vell.com, gakula@...vell.com, 
	sbhatta@...vell.com, hkelam@...vell.com, davem@...emloft.net, 
	edumazet@...gle.com, kuba@...nel.org, jerinj@...vell.com, 
	lcherian@...vell.com, ndabilpuram@...vell.com, sd@...asysnail.net
Subject: Re: [net-next PATCH v9 5/8] cn10k-ipsec: Add SA add/del support for
 outb ipsec crypto offload

On Tue, Nov 12, 2024 at 7:14 PM Paolo Abeni <pabeni@...hat.com> wrote:
>
>
>
> On 11/8/24 05:57, Bharat Bhushan wrote:
> > This patch adds support to add and delete Security Association
> > (SA) xfrm ops. Hardware maintains SA context in memory allocated
> > by software. Each SA context is 128 byte aligned and size of
> > each context is multiple of 128-byte. Add support for transport
> > and tunnel ipsec mode, ESP protocol, aead aes-gcm-icv16, key size
> > 128/192/256-bits with 32bit salt.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@...vell.com>
> > ---
> > v8->v9:
> >  - Previous versions were supporting only 64 SAs and a bitmap was
> >    used for same. That limitation is removed from this version.
> >  - Replaced netdev_err with NL_SET_ERR_MSG_MOD in state add flow
> >    as per comment in previous version
> >  - Changes related to mutex lock removal
> >
> > v5->v6:
> >  - In ethtool flow, so not cleanup cptlf if SA are installed and
> >    call netdev_update_features() when all SA's are un-installed.
> >  - Description and comment re-word to replace "inline ipsec"
> >    with "ipsec crypto offload"
> >
> > v3->v4:
> >  - Added check for crypto offload (XFRM_DEV_OFFLOAD_CRYPTO)
> >    Thanks "Leon Romanovsky" for pointing out
> >
> > v2->v3:
> >  - Removed memset to zero wherever possible
> >   (comment from Kalesh Anakkur Purayil)
> >  - Corrected error handling when setting SA for inbound
> >    (comment from Kalesh Anakkur Purayil)
> >  - Move "netdev->xfrmdev_ops = &cn10k_ipsec_xfrmdev_ops;" to this patch
> >    This fix build error with W=1
> >
> >  .../marvell/octeontx2/nic/cn10k_ipsec.c       | 415 ++++++++++++++++++
> >  .../marvell/octeontx2/nic/cn10k_ipsec.h       | 113 +++++
> >  2 files changed, 528 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> > index e09ce42075c7..ccbcc5001431 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
> > @@ -375,6 +375,391 @@ static int cn10k_outb_cpt_clean(struct otx2_nic *pf)
> >       return ret;
> >  }
> >
> > +static void cn10k_cpt_inst_flush(struct otx2_nic *pf, struct cpt_inst_s *inst,
> > +                              u64 size)
> > +{
> > +     struct otx2_lmt_info *lmt_info;
> > +     u64 val = 0, tar_addr = 0;
> > +
> > +     lmt_info = per_cpu_ptr(pf->hw.lmt_info, smp_processor_id());
> > +     /* FIXME: val[0:10] LMT_ID.
> > +      * [12:15] no of LMTST - 1 in the burst.
> > +      * [19:63] data size of each LMTST in the burst except first.
> > +      */
> > +     val = (lmt_info->lmt_id & 0x7FF);
> > +     /* Target address for LMTST flush tells HW how many 128bit
> > +      * words are present.
> > +      * tar_addr[6:4] size of first LMTST - 1 in units of 128b.
> > +      */
> > +     tar_addr |= pf->ipsec.io_addr | (((size / 16) - 1) & 0x7) << 4;
> > +     dma_wmb();
> > +     memcpy((u64 *)lmt_info->lmt_addr, inst, size);
> > +     cn10k_lmt_flush(val, tar_addr);
> > +}
> > +
> > +static int cn10k_wait_for_cpt_respose(struct otx2_nic *pf,
> > +                                   struct cpt_res_s *res)
> > +{
> > +     unsigned long timeout = jiffies + msecs_to_jiffies(10000);
> > +
> > +     do {
> > +             if (time_after(jiffies, timeout)) {
> > +                     netdev_err(pf->netdev, "CPT response timeout\n");
> > +                     return -EBUSY;
> > +             }
> > +     } while (res->compcode == CN10K_CPT_COMP_E_NOTDONE);
>
> Why a READ_ONCE() annotation is not needed around the 'res->compcode'
> access?

Yes READ_ONCE() is required, thanks for pointing.
res->compcode is 7bit bit-field, so cannot  use
READ_ONCE((res->compcode). Will add below change:

Add bit-mask for compcode (CN10K_CPT_COMP_E_MASK 0x3F)

+       u64 *completion_ptr = (u64 *)res;

        do {
                if (time_after(jiffies, timeout)) {
                        netdev_err(pf->netdev, "CPT response timeout\n");
                        return -EBUSY;
                }
-       } while (res->compcode == CN10K_CPT_COMP_E_NOTDONE);
+       } while ((READ_ONCE(*completion_ptr) & CN10K_CPT_COMP_E_MASK) ==
+                CN10K_CPT_COMP_E_NOTDONE);


>
> Possibly more relevant: it looks like this code is busy polling the H/W
> for at most 10s, is that correct? If so that timeout is way too high my
> several order of magnitude. You should likely use usleep_range() or
> sleep_interruptible()

This is called from the atomic context (xdo_dev_state_delete()), so I
can't sleep.
Polling for 10 sec was added for some experimentation, Will change
polling time to 100ms. Is that okay?

Thanks
-Bharat

>
> [...]
> > +static int cn10k_ipsec_validate_state(struct xfrm_state *x,
> > +                                   struct netlink_ext_ack *extack)
> > +{
> > +     if (x->props.aalgo != SADB_AALG_NONE) {
> > +             NL_SET_ERR_MSG_MOD(extack,
> > +                                "Cannot offload authenticated xfrm states\n");
>
> No '\n' at the end of extack messages.
>
> (many cases below)
>
> Thanks,
>
> Paolo
>
>

Powered by blists - more mailing lists