[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