getentropy() implementation on BBB

[Christian Mauderer]

Am 05.03.2018 um 14:51 schrieb Udit agarwal:
> Hi,
> I tried implementing getentropy on BBB, below is the patch. Please have
> a look.
> I followed these(1
> <https://e2e.ti.com/support/arm/sitara_arm/f/791/t/355064> & 2
> <http://ftp.netbsd.org/pub/NetBSD/NetBSD-current/src/sys/arch/arm/omap/am335x_trng.c>)
> links for code reference. and this
> <https://docs.google.com/spreadsheets/d/1VpghMlLtrWQIrcvCsRg3ueRZkX0eGSQkFnzjfsqrd8A/view#gid=0>
> for register reference. Moreover, what further configuration in RTEMS is
> needed to execute and test this code along with getentropy() sample
> testcode?

Hello Udit,

regarding the patch: Please use 'git format-patch' for generating your
patches and send them unchanged to the mailing list (for example with
'git send-email'. If you follow this workflow, it's easier to apply and
test the patches. The preferred workflow for RTEMS is described here:
https://devel.rtems.org/wiki/Developer/Git/Users#CreatingaPatch

Regarding documentation: It seems that there is really only few
documentation regarding the crypto part available. The FreeBSD driver is
most likely your best source.

I added further comments down in your code.

Best regards

Christian

> 
> 
> + /*
> +  * Copyright (c) 2018 embedded brains GmbH.  All rights reserved.
> +  *
> +  *  embedded brains GmbH
> +  *  Dornierstr. 4
> +  *  82178 Puchheim
> +  *  Germany
> +  *  <rtems at embedded-brains.de <mailto:rtems at embedded-brains.de>>
> +  *

Also it's nice if you add an embedded brains header, I don't think that
I've seen you when I have been at work today. So please add your own
name to the copyright line instead. With that you can even point to that
file and tell everyone that it's from you.

An address isn't really necessary except if you want it for some reason
(like PR in case of a company).

> +  * The license and distribution terms for this file may be
> +  * found in the file LICENSE in this distribution or at
> +  * http://www.rtems.org/license/LICENSE.
> +  */
> +
> + #include <libcpu/am335x.h>
> + #include <unistd.h>
> + #include <string.h>
> + #include <rtems/sysinit.h>
> +
> + #define RNG_BASE    0x48310000
> + #define CM_PER_RNG_CLKCTRL    0x44e00090
> + #define RNG_STATUS_RDY    (1u <<  0)  // output ready for reading
> + #define RNG_STATUS_ERR    (1u <<  1)  // FRO shutdown alarm
> + #define RNG_STATUS_CLK    (1u << 31)  // module functional clock
> active (no irq)

Please add address definitions and the bit masks to the
bsps/arm/include/libcpu/am335x.h instead of defining them here locally.
Note that for the CM_PER_RNG_CLKCTRL, there is already a
AM335X_CM_PER_ADDR that should be used as a base.

> +
> + typedef volatile struct rng *rng_ref_t;
> +
> + struct rng {
> + /*00*/    uint64_t output;    //r-
> + /*08*/    uint32_t status;    //r-
> + /*0c*/    uint32_t irq_en;    //rw
> + /*10*/    uint32_t status_clr;    //-c
> + /*14*/    uint32_t control;    //rw
> + /*18*/    uint32_t config;    //rw
> + };

Same is true here: It's a structure describing the registers. So it
should be in the header instead.

It seems that you copied that from the forum post. Please always check
the license when you copy code. We have to avoid that we get any code
into RTEMS that isn't compatible with RTEMS license.

In this case the code should be obvious enough that - if you just
rewrite it in RTEMS style, there shouldn't be a problem. But just
remember that licensing is always a difficult topic.

> +
> + static void TRNG_Enable(rng_ref_t rng){
> +     rng->config = 0
> +     | 34 << 16  // max refill 34 * 256 cycles
> +     | 33 <<  0;  // min refill 33 * 64 cycles
> +     rng->control = 0
> +     | 33 << 16  // startup 33 * 256 cycles
> +     |  1 << 10;  // enable module
> + }

Please use C-Style comments /* */ in all pure C code (most of RTEMS).

> +
> + static void beagle_trng_clock_enable(void)
> + {
> +     // enable module clock
> +     *(volatile uint8_t *) CM_PER_RNG_CLKCTRL = 2;
> +     while( *(volatile uint32_t *) CM_PER_RNG_CLKCTRL & 0x30000 ) {}
> + }
> +
> + static uint64_t TRNG_GetRandData(){
> +     uint64_t output = rng->output;

'rng' isn't defined here.

> +     return output;
> + }
> +
> + int getentropy(void *ptr, size_t n)
> + {
> +     rng_ref_t const rng = (rng_ref_t) RNG_BASE;
> +     TRNG_Enable(rng);
> +     while (n > 0) {
> +         uint64_t random;
> +         size_t copy;
> +
> +         while( ( rng->status & RNG_STATUS_RDY ) == 0 ); //wait

You should check specifically for the TRNG_STATUS_READY flag here like
done in FreeBSD. I'm not sure what the other flags are but it's quite
possible that there is also some error flag or similar. In that case it
would not be a good idea to use the random number.

> +
> +         random = TRNG_GetRandData(rng);

Note that you don't have to copy the function names from the atsam
exactly. In the atsam BSP, the library has provided these names.
Otherwise it's quite unusual to start functions with upper case in RTEMS.

> +
> +         /*
> +          * Read TRNG status one more time to avoid race condition.
> +          * Otherwise we can read (and clear) an old ready status but get
> +          * a new value. The ready status for this value wouldn't be
> +          * reset.
> +          */

That comment seems like it doesn't fit here any longer. You acknowledge
the status instead in your next line. Just remove the comment.

> +         rng->status_clr = RNG_STATUS_RDY;
> +
> +         copy = sizeof(random);
> +         if (n < copy ) {
> +             copy = n;
> +         }
> +         memcpy(ptr, &random, copy);
> +         n -= copy;
> +         ptr += copy;
> +     }
> +
> +     return 0;
> + }
> +
> + RTEMS_SYSINIT_ITEM(
> +   beagle_trng_clock_enable,
> +   RTEMS_SYSINIT_DEVICE_DRIVERS,
> +   RTEMS_SYSINIT_ORDER_LAST
> + );
> 
> Regards,
> Udit agarwal
> 
> 
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
>