getentropy() implementation on BBB
Gedare Bloom
gedare at rtems.org
Tue Mar 6 17:35:09 UTC 2018
On Tue, Mar 6, 2018 at 9:43 AM, Udit agarwal <dev.madaari at gmail.com> wrote:
> Hi,
> Here's the updated code(I have also attached the patch PFA):
>
> From 96e6e1bfd8cffeef5d309eb0a07fe2bfd086ef0a Mon Sep 17 00:00:00 2001
> From: Udit agarwal <dev.madaari at gmail.com>
> Date: Tue, 6 Mar 2018 20:07:44 +0530
> Subject: [PATCH] Added getentropy support to BBB BSP
>
> ---
> bsps/arm/beagle/headers.am | 1 +
> bsps/arm/beagle/include/bsp/bbb_getentropy.h | 57 +++++++++++++++
> bsps/arm/include/libcpu/am335x.h | 16 +++++
> c/src/lib/libbsp/arm/beagle/Makefile.am | 4 ++
> .../libbsp/arm/beagle/getentropy/bbb_getentropy.c | 80
> ++++++++++++++++++++++
> 5 files changed, 158 insertions(+)
> create mode 100644 bsps/arm/beagle/include/bsp/bbb_getentropy.h
> create mode 100644 c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
>
> diff --git a/bsps/arm/beagle/headers.am b/bsps/arm/beagle/headers.am
> index 6692d0b..ac4ff7c 100644
> --- a/bsps/arm/beagle/headers.am
> +++ b/bsps/arm/beagle/headers.am
> @@ -12,3 +12,4 @@ include_bsp_HEADERS +=
> ../../../../../../bsps/arm/beagle/include/bsp/bbb-pwm.h
> include_bsp_HEADERS +=
> ../../../../../../bsps/arm/beagle/include/bsp/beagleboneblack.h
> include_bsp_HEADERS += ../../../../../../bsps/arm/beagle/include/bsp/i2c.h
> include_bsp_HEADERS += ../../../../../../bsps/arm/beagle/include/bsp/irq.h
> +include_bsp_HEADERS +=
> ../../../../../../bsps/arm/beagle/include/bsp/bbb_getentropy.h
> diff --git a/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> b/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> new file mode 100644
> index 0000000..5e46f89
> --- /dev/null
> +++ b/bsps/arm/beagle/include/bsp/bbb_getentropy.h
> @@ -0,0 +1,57 @@
> +/**
> + * @file
> + *
> + * @ingroup arm_beagle
> + *
> + * @brief TRNG Definations
> + */
> +
> +/*
> + * Copyright (c) 2018 Udit kumar agarwal <dev.madaari at gmail.com>
> + *
> + * 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.
> + */
> +
> +
> +#ifndef _TRNG_
> +#define _TRNG_
> +
> +/*------------------------------------------------------------------------------
Please look at similar files in RTEMS for how to add comments.
We do not use this style /* --------------------
And we don't put this kind of comment about the file sections.
> + * Headers
> +
> *----------------------------------------------------------------------------*/
> +
> +#include <libcpu/am335x.h>
> +
> +
> /*------------------------------------------------------------------------------
> + * Register structure
> +
> *----------------------------------------------------------------------------*/
> +
> +struct rng {
> + uint64_t output; /*00*/
> + uint32_t status; /*08*/
> + uint32_t irq_en; /*0c*/
> + uint32_t status_clr; /*10*/
> + uint32_t control; /*14*/
> + uint32_t config; /*18*/
> +};
for an exported header, the struct namespace needs to be user
friendly. See similar files at this layer for how to name things.
Probably, this can be defined in the am335x.h header?
> +
> +typedef volatile struct rng *rng_ref_t;
I'm not such a big fan of using the typedef to hide things like
volatile or pointer notation. I don't see a need for this usually.
> +
> +/*----------------------------------------------------------------------------*/
> +/* Exported functions
> */
> +/*----------------------------------------------------------------------------*/
> +
> +/* configure and enable RNG module */
> +static void am335x_rng_enable(rng_ref_t);
> +/* enable module clock for random number generator */
> +static void am335x_rng_clock_enable(void);
> +/* outputs random data */
> +static uint64_t trng_getranddata(rng_ref_t);
static keyword implies these are not exported.
> +/* Copy random data of a specified size to the pointer supplied */
> +int getentropy(void *ptr, size_t);
I guess getentropy() is already exported by a different header file
(unistd.h) in libc? This doesn't belong here.
I suspect this header file is not actually necessary.
> +
> +
> +
Don't use multiple blank lines in a row. Only one empty line is ever
needed by RTEMS style.
> +#endif /* #ifndef _TRNG_ */
> \ No newline at end of file
> diff --git a/bsps/arm/include/libcpu/am335x.h
> b/bsps/arm/include/libcpu/am335x.h
> index 367e97c..8b58f1a 100644
> --- a/bsps/arm/include/libcpu/am335x.h
> +++ b/bsps/arm/include/libcpu/am335x.h
> @@ -14,6 +14,9 @@
> * Modified by Ben Gras <beng at shrike-systems.com> to add lots
> * of beagleboard/beaglebone definitions, delete lpc32xx specific
> * ones, and merge with some other header files.
> + *
> + * Modified by Udit agarwal <dev.madaari at gmail.com> to add random
> + * number generating module definations
typo: definitions
add a period (full stop).
> */
>
> #if !defined(_AM335X_H_)
> @@ -701,4 +704,17 @@
> #define AM335X_CM_PER_OCPWP_L3_CLKSTCTRL_CLKACTIVITY_OCPWP_L4_GCLK
> (0x00000020u)
> #define AM335X_I2C_INT_STOP_CONDITION AM335X_I2C_IRQSTATUS_BF
>
> +/* TRNG Register */
> +
> +/* RNG base address */
> +#define RNG_BASE 0x48310000
> +/* RNG clock control */
> +#define CM_PER_RNG_CLKCTRL AM335X_CM_PER_ADDR | 9<<4
I'd put parentheses around this to avoid any problems with macro
expansions and order of operations.
> +/* Offset from RNG base for output ready flag */
> +#define RNG_STATUS_RDY (1u << 0)
> +/* Offset from RNG base for FRO related error*/
> +#define RNG_STATUS_ERR (1u << 1)
> +/* Offset from RNG base for clock status */
> +#define RNG_STATUS_CLK (1u << 31)
> +
> #endif
> diff --git a/c/src/lib/libbsp/arm/beagle/Makefile.am
> b/c/src/lib/libbsp/arm/beagle/Makefile.am
> index 8251660..bf92081 100644
> --- a/c/src/lib/libbsp/arm/beagle/Makefile.am
> +++ b/c/src/lib/libbsp/arm/beagle/Makefile.am
> @@ -88,9 +88,13 @@ libbsp_a_SOURCES += gpio/bbb-gpio.c
> #pwm
> libbsp_a_SOURCES += pwm/pwm.c
>
> +#getentropy
> +libbsp_a_SOURCES += getentropy/bbb_getentropy.c
> +
> #RTC
> libbsp_a_SOURCES += rtc.c
> libbsp_a_SOURCES += ../../shared/tod.c
> +
> # Clock
> libbsp_a_SOURCES += clock.c
> libbsp_a_SOURCES += ../../shared/clockdrv_shell.h
> diff --git a/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> new file mode 100644
> index 0000000..cab2102
> --- /dev/null
> +++ b/c/src/lib/libbsp/arm/beagle/getentropy/bbb_getentropy.c
> @@ -0,0 +1,80 @@
> +/**
> +* @file
> +*
> +* @ingroup arm_beagle
> +*
> +* @brief Getentropy implementation on BeagleBone Black BSP
> +*/
> +
> +/*
> +* Copyright (c) 2018 Udit kumar agarwal <dev.madaari at gmail.com>
> +*
> +* 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 <unistd.h>
> +#include <stdint.h>
> +#include <string.h>
> +#include <rtems/sysinit.h>
> +
> +
> +/* maximun and minimun refill cycle sets the number of samples to be taken
typo: minimum
> + from FRO to generate random number */
> +static void am335x_rng_enable(rng_ref_t rng){
put { on a new line.
> + rng->config = 0
> + | 34 << 16 /* max refill 34 * 256 cycles */
indent broken lines, but
> + | 33 << 0; /* min refill 33 * 64 cycles */
You can fit this on one line, remove the comments and put them above
the line, e.g.
rng->config = 34<<16 | 33;
I would prefer not to have these hard-coded values though it is
fragile. what means 34, 16, and 33?
#define AM335X_RNG_MAX_REFILL (34 << 16)
#define AM335X_RNG_MIN_REFILL (33)
#define AM335X_RNG_CONFIG_REFILL (AM335X_RNG_MAX_REFILL | AM335X_RNG_MIN_REFILL)
rng.config = rng.control = 0;
rng.config |= AM335X_RNG_CONFIG_REFILL_TIME;
> + rng->control = 0
> + | 33 << 16 /* startup 33 * 256 cycles */
> + | 1 << 10; /* enable module */
rng.control |= AM335X_RNG_CONTROL_STARTUP;
rng.control |= AM335X_RNG_CONTROL_ENABLE;
for example is more readable. look around for other examples.
> +}
> +
> +static void am335x_rng_clock_enable(void)
> +{
> + *(volatile uint8_t *) CM_PER_RNG_CLKCTRL = 2;
> + while( *(volatile uint32_t *) CM_PER_RNG_CLKCTRL & 0x30000 ) {}
> +}
> +
> +static uint64_t trng_getranddata(rng_ref_t rng){
> + uint64_t output = rng->output;
> + return output;
> +}
> +
> +int getentropy(void *ptr, size_t n)
> +{
> + rng_ref_t const rng = (rng_ref_t) RNG_BASE;
> + am335x_rng_enable(rng);
> + while (n > 0) {
> + uint64_t random;
indent blocks of code.
> + size_t copy;
> +
> + /* wait untill RNG becomes ready with next set of random data */
> + while( ( rng->status & RNG_STATUS_RDY ) == 0 );
> +
> + random = trng_getranddata(rng);
> +
> + /* Checking for error by masking all bits other then error bit in
> status register */
> + if( ((rng->status & RNG_STATUS_ERR)>>1) == 0){
> +
> + /* clear the status flag after reading to generate new random
> value*/
> + rng->status_clr = RNG_STATUS_RDY;
> + copy = sizeof(random);
> + if ( n < copy ) {
> + copy = n;
ditto.
> + }
> + memcpy(ptr, &random, copy);
> + n -= copy;
> + ptr += copy;
pointer arithmetic on void pointer is not legal. You can portably use
ptr = (char*)ptr + copy;
> + }
> + }
> +
> + return 0;
> +}
> +
> +RTEMS_SYSINIT_ITEM(
> + am335x_rng_clock_enable,
> + RTEMS_SYSINIT_DEVICE_DRIVERS,
> + RTEMS_SYSINIT_ORDER_LAST
> +);
> \ No newline at end of file
> --
> 1.9.1
>
> Regards,
> Udit Agarwal
>
> On Tue, Mar 6, 2018 at 12:50 AM, Christian Mauderer <list at c-mauderer.de>
> wrote:
>>
>> 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
>> >
>
>
>
> _______________________________________________
> devel mailing list
> devel at rtems.org
> http://lists.rtems.org/mailman/listinfo/devel
More information about the devel
mailing list