[EXT] RE: [PATCH v8 10/15] crypto/fsl: Improve hwrng performance in kernel

Gaurav Jain gaurav.jain at nxp.com
Tue Jan 11 06:44:54 CET 2022


Hi Andrey

For now I will remove this particular patch from my series and will send a separate patch addressing your comments.

Regards
Gaurav Jain

> -----Original Message-----
> From: ZHIZHIKIN Andrey <andrey.zhizhikin at leica-geosystems.com>
> Sent: Monday, January 10, 2022 7:31 PM
> To: Gaurav Jain <gaurav.jain at nxp.com>; u-boot at lists.denx.de
> Cc: Stefano Babic <sbabic at denx.de>; Fabio Estevam <festevam at gmail.com>;
> Peng Fan <peng.fan at nxp.com>; Simon Glass <sjg at chromium.org>; Priyanka
> Jain <priyanka.jain at nxp.com>; Ye Li <ye.li at nxp.com>; Horia Geanta
> <horia.geanta at nxp.com>; Ji Luo <ji.luo at nxp.com>; Franck Lenormand
> <franck.lenormand at nxp.com>; Silvano Di Ninno <silvano.dininno at nxp.com>;
> Sahil Malhotra <sahil.malhotra at nxp.com>; Pankaj Gupta
> <pankaj.gupta at nxp.com>; Varun Sethi <V.Sethi at nxp.com>; dl-uboot-imx
> <uboot-imx at nxp.com>; Shengzhou Liu <shengzhou.liu at nxp.com>; Mingkai Hu
> <mingkai.hu at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>; Meenakshi
> Aggarwal <meenakshi.aggarwal at nxp.com>; Wasim Khan
> <wasim.khan at nxp.com>; Alison Wang <alison.wang at nxp.com>; Pramod
> Kumar <pramod.kumar_1 at nxp.com>; Andy Tang <andy.tang at nxp.com>;
> Adrian Alonso <adrian.alonso at nxp.com>; Vladimir Oltean
> <olteanv at gmail.com>; michael at walle.cc
> Subject: [EXT] RE: [PATCH v8 10/15] crypto/fsl: Improve hwrng performance in
> kernel
> 
> Caution: EXT Email
> 
> Hello Gaurav,
> 
> Cc: Michael Walle
> 
> > -----Original Message-----
> > From: U-Boot <u-boot-bounces at lists.denx.de> On Behalf Of Gaurav Jain
> > Sent: Monday, January 10, 2022 1:27 PM
> > To: u-boot at lists.denx.de
> > Cc: Stefano Babic <sbabic at denx.de>; Fabio Estevam
> > <festevam at gmail.com>; Peng Fan <peng.fan at nxp.com>; Simon Glass
> > <sjg at chromium.org>; Priyanka Jain <priyanka.jain at nxp.com>; Ye Li
> > <ye.li at nxp.com>; Horia Geanta <horia.geanta at nxp.com>; Ji Luo
> > <ji.luo at nxp.com>; Franck Lenormand <franck.lenormand at nxp.com>; Silvano
> > Di Ninno <silvano.dininno at nxp.com>; Sahil malhotra
> > <sahil.malhotra at nxp.com>; Pankaj Gupta <pankaj.gupta at nxp.com>; Varun
> > Sethi <V.Sethi at nxp.com>; NXP i . MX U-Boot Team <uboot-imx at nxp.com>;
> > Shengzhou Liu <Shengzhou.Liu at nxp.com>; Mingkai Hu
> > <mingkai.hu at nxp.com>; Rajesh Bhagat <rajesh.bhagat at nxp.com>;
> Meenakshi
> > Aggarwal <meenakshi.aggarwal at nxp.com>; Wasim Khan
> > <wasim.khan at nxp.com>; Alison Wang <alison.wang at nxp.com>; Pramod
> Kumar
> > <pramod.kumar_1 at nxp.com>; Tang Yuantian <andy.tang at nxp.com>; Adrian
> > Alonso <adrian.alonso at nxp.com>; Vladimir Oltean <olteanv at gmail.com>
> > Subject: [PATCH v8 10/15] crypto/fsl: Improve hwrng performance in
> > kernel
> >
> > From: Ye Li <ye.li at nxp.com>
> >
> > RNG parameters are reconfigured.
> > - For TRNG to generate 256 bits of entropy, RNG TRNG Seed Control register
> >   is configured to have reduced SAMP_SIZE from default 2500 to 512. it is
> >   number of entropy samples that will be taken during Entropy generation.
> > - self-test registers(Monobit Limit, Poker Range, Run Length Limit)
> >   are synchronized with new RTSDCTL[SAMP_SIZE] of 512.
> >
> > TRNG time is caluculated based on sample size.
> 
> Typo: caluculated -> calculated
> 
> > time required to generate entropy is reduced and hwrng performance
> > improved from 0.3 kB/s to 1.3 kB/s.
> 
> Is there any degradation in passed/failed FIPS 140-2 test count? Can you provide
> some results from at least rngtest run?
> 
> >
> > Signed-off-by: Ye Li <ye.li at nxp.com>
> > Acked-by: Gaurav Jain <gaurav.jain at nxp.com>>
> > ---
> >  drivers/crypto/fsl/jr.c | 102 +++++++++++++++++++++++++++++++++-------
> >  include/fsl_sec.h       |   1 +
> >  2 files changed, 87 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/crypto/fsl/jr.c b/drivers/crypto/fsl/jr.c index
> > a84440ab10..e5346a84a4 100644
> > --- a/drivers/crypto/fsl/jr.c
> > +++ b/drivers/crypto/fsl/jr.c
> > @@ -603,30 +603,100 @@ static u8 get_rng_vid(ccsr_sec_t *sec)
> >   */
> >  static void kick_trng(int ent_delay, ccsr_sec_t *sec)  {
> > +     u32 samples  = 512; /* number of bits to generate and test */
> > +     u32 mono_min = 195;
> > +     u32 mono_max = 317;
> > +     u32 mono_range  = mono_max - mono_min;
> > +     u32 poker_min = 1031;
> > +     u32 poker_max = 1600;
> > +     u32 poker_range = poker_max - poker_min + 1;
> > +     u32 retries    = 2;
> > +     u32 lrun_max   = 32;
> > +     s32 run_1_min   = 27;
> > +     s32 run_1_max   = 107;
> > +     s32 run_1_range = run_1_max - run_1_min;
> > +     s32 run_2_min   = 7;
> > +     s32 run_2_max   = 62;
> > +     s32 run_2_range = run_2_max - run_2_min;
> > +     s32 run_3_min   = 0;
> > +     s32 run_3_max   = 39;
> > +     s32 run_3_range = run_3_max - run_3_min;
> > +     s32 run_4_min   = -1;
> > +     s32 run_4_max   = 26;
> > +     s32 run_4_range = run_4_max - run_4_min;
> > +     s32 run_5_min   = -1;
> > +     s32 run_5_max   = 18;
> > +     s32 run_5_range = run_5_max - run_5_min;
> > +     s32 run_6_min   = -1;
> > +     s32 run_6_max   = 17;
> > +     s32 run_6_range = run_6_max - run_6_min;
> 
> I have a feeling that this whole block of local variables can be simplified. I'm not
> sure it is required to list this so detailed.
> 
> You can attempt to define those values in header file and use macros to
> compute bound conditions, rather than allocating this on the stack here.
> 
> > +     u32 val;
> > +
> >       struct rng4tst __iomem *rng =
> >                       (struct rng4tst __iomem *)&sec->rng;
> > -     u32 val;
> >
> > -     /* put RNG4 into program mode */
> > -     sec_setbits32(&rng->rtmctl, RTMCTL_PRGM);
> > -     /* rtsdctl bits 0-15 contain "Entropy Delay, which defines the
> > -      * length (in system clocks) of each Entropy sample taken
> > -      * */
> > +     /* Put RNG in program mode */
> > +     /* Setting both RTMCTL:PRGM and RTMCTL:TRNG_ACC causes TRNG to
> > +      * properly invalidate the entropy in the entropy register and
> > +      * force re-generation.
> > +      */
> > +     sec_setbits32(&rng->rtmctl, RTMCTL_PRGM | RTMCTL_ACC);
> > +
> > +     /* Configure the RNG Entropy Delay
> > +      * Performance-wise, it does not make sense to
> > +      * set the delay to a value that is lower
> > +      * than the last one that worked (i.e. the state handles
> > +      * were instantiated properly. Thus, instead of wasting
> > +      * time trying to set the values controlling the sample
> > +      * frequency, the function simply returns.
> > +      */
> >       val = sec_in32(&rng->rtsdctl);
> > -     val = (val & ~RTSDCTL_ENT_DLY_MASK) |
> > -           (ent_delay << RTSDCTL_ENT_DLY_SHIFT);
> > +     val &= RTSDCTL_ENT_DLY_MASK;
> > +     val >>= RTSDCTL_ENT_DLY_SHIFT;
> > +     if (ent_delay < val) {
> > +             /* Put RNG4 into run mode */
> > +             sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM | RTMCTL_ACC);
> > +             return;
> > +     }
> > +
> > +     val = (ent_delay << RTSDCTL_ENT_DLY_SHIFT) | samples;
> >       sec_out32(&rng->rtsdctl, val);
> > -     /* min. freq. count, equal to 1/4 of the entropy sample length */
> > -     sec_out32(&rng->rtfreqmin, ent_delay >> 2);
> > -     /* disable maximum frequency count */
> > -     sec_out32(&rng->rtfreqmax, RTFRQMAX_DISABLE);
> > +
> > +     /*
> > +      * Recommended margins (min,max) for freq. count:
> > +      *   freq_mul = RO_freq / TRNG_clk_freq
> > +      *   rtfrqmin = (ent_delay x freq_mul) >> 1;
> > +      *   rtfrqmax = (ent_delay x freq_mul) << 3;
> > +      * Given current deployments of CAAM in i.MX SoCs, and to simplify
> > +      * the configuration, we consider [1,16] to be a safe interval
> 
> Statement "... we consider ..." should be perhaps extended with justification on
> how this conclusion was made. Some test results (either here in the comment or
> in commit message) would be beneficial to see.
> 
> > +      * for the freq_mul and the limits of the interval are used to compute
> > +      * rtfrqmin, rtfrqmax
> > +      */
> > +     sec_out32(&rng->rtfreqmin, ent_delay >> 1);
> > +     sec_out32(&rng->rtfreqmax, ent_delay << 7);
> > +
> > +     sec_out32(&rng->rtscmisc, (retries << 16) | lrun_max);
> > +     sec_out32(&rng->rtpkrmax, poker_max);
> > +     sec_out32(&rng->rtpkrrng, poker_range);
> > +     sec_out32(&rng->rsvd1[0], (mono_range << 16) | mono_max);
> > +     sec_out32(&rng->rsvd1[1], (run_1_range << 16) | run_1_max);
> > +     sec_out32(&rng->rsvd1[2], (run_2_range << 16) | run_2_max);
> > +     sec_out32(&rng->rsvd1[3], (run_3_range << 16) | run_3_max);
> > +     sec_out32(&rng->rsvd1[4], (run_4_range << 16) | run_4_max);
> > +     sec_out32(&rng->rsvd1[5], (run_5_range << 16) | run_5_max);
> > +     sec_out32(&rng->rsvd1[6], (run_6_range << 16) | run_6_max);
> 
> Writing in reserved area is not a good idea. Those registers are defined in HW,
> can you please define them also in the header file?
> 
> > +
> > +     val = sec_in32(&rng->rtmctl);
> >       /*
> > -      * select raw sampling in both entropy shifter
> > +      * Select raw sampling in both entropy shifter
> 
> This change is not needed, otherwise please adjust all comments in original file,
> as for example comment below also starts with small letter...
> 
> >        * and statistical checker
> >        */
> > -     sec_setbits32(&rng->rtmctl, RTMCTL_SAMP_MODE_RAW_ES_SC);
> 
> The only usage of RTMCTL_SAMP_MODE_RAW_ES_SC is dropped here, please
> remove it from include/fsl_sec.h as well.
> 
> > -     /* put RNG4 into run mode */
> > -     sec_clrbits32(&rng->rtmctl, RTMCTL_PRGM);
> > +     val &= ~RTMCTL_SAMP_MODE_INVALID;
> > +     val |= RTMCTL_SAMP_MODE_RAW_ES_SC;
> > +     /* Put RNG4 into run mode */
> > +     val &= ~(RTMCTL_PRGM | RTMCTL_ACC);
> 
> BIT() macro for both new and existing defines?
> 
> > +     /*test with sample mode only */
> > +     sec_out32(&rng->rtmctl, val);
> >  }
> >
> >  static int rng_init(uint8_t sec_idx, ccsr_sec_t *sec) diff --git
> > a/include/fsl_sec.h b/include/fsl_sec.h index 7b6e3e2c20..2b3239414a
> > 100644
> > --- a/include/fsl_sec.h
> > +++ b/include/fsl_sec.h
> > @@ -34,6 +34,7 @@
> >  #if CONFIG_SYS_FSL_SEC_COMPAT >= 4
> >  /* RNG4 TRNG test registers */
> >  struct rng4tst {
> > +#define RTMCTL_ACC  0x20
> >  #define RTMCTL_PRGM 0x00010000       /* 1 -> program mode, 0 -> run mode
> */
> >  #define RTMCTL_SAMP_MODE_VON_NEUMANN_ES_SC     0 /* use von
> Neumann data in
> >                                                   both entropy shifter
> > and
> > --
> > 2.17.1
> 
> -- andrey


More information about the U-Boot mailing list