Re: [Minios-devel] [UNIKRAFT PATCH v2 2/4] lib/ukswrand: Add seed generating function

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 18.10.19 15:13, Vlad-Andrei BĂDOIU (78692) wrote:
> Hey Simon,
>
> Please see my replies inline.
>
> Thanks,
>
> Vlad
>
> On 18.10.2019 15:59, Simon Kuenzer wrote:
> > Hey Vlad,
> >
> > thanks a lot for you work. I have a few comments inline.
> >
> > Thanks,
> >
> > Simon
> >
> > On 16.10.19 17:41, Vlad-Andrei BĂDOIU (78692) wrote:
> > > This patch adds a new function, _get_random_seed32 for seed
> > > generaton. If the
> > > pre-compiled config option is not selected, then rdrand is used on
> > > x86 and
> > > ukplat_wall_clock on ARM.
> > >
> > > Signed-off-by: Vlad-Andrei Badoiu <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>
> > > ---
> > >    lib/ukswrand/Config.uk           |  7 +++++++
> > >    lib/ukswrand/include/uk/swrand.h | 20 ++++++++++++++++++++
> > >    lib/ukswrand/mwc.c               |  2 +-
> > >    3 files changed, 28 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/lib/ukswrand/Config.uk b/lib/ukswrand/Config.uk
> > > index c58371bb..8db3af03 100644
> > > --- a/lib/ukswrand/Config.uk
> > > +++ b/lib/ukswrand/Config.uk
> > > @@ -14,10 +14,17 @@ config LIBUKSWRAND_MWC
> > >            Use multiply-with-carry algorithm
> > >    endchoice
> > >    +config LIBUKSWRAND_USE_INITIALSEED
> > > +    bool "Use a pre-compiled seed"
> > > +    default y
> > > +
> > > +if LIBUKSWRAND_USE_INITIALSEED
> > >    config LIBUKSWRAND_INITIALSEED
> > >        int "Initial random seed"
> > >        default 23
> > Hey, I would rather provide a drop-down menu. This is maybe a little
> > bit more intuitive to use. How about:
> Noted.
> > choice
> >      prompt "Initial seed"
> >      default LIBUKSWRANDR_INITIALSEED_TIME
> >
> > config LIBUKSWRAND_INITIALSEED_TIME
> >      bool "Platform timestamp"
> >
> > config LIBUKSWRAND_INITIALSEED_RDRAND
> >      depends on ARCH_X86_64 && (MARCH_X86_64_COREI7AVXI)
> >      bool "`rdrand` instruction"
> >
> > config LIBUKSWRAND_INITIALSEED_USECONSTANT
> >      bool "Compiled-in constant"
> > endchoice
> >
> > config LIBUKSWRAND_INITIALSEED_CONSTANT
> >      int "Initial seed constant"
> >      depends on LIBUKSWRAND_INITIALSEED_USECONSTANT
> >      default 23
> >
> > Also note that rdrand/rdseed is not available on all x86_64 CPUs. On
> > Intel it got introduced with Ivy-Bridge, afaik.
> Right, would adding a check for the existence of rdrand be all right in
> the case of x86?
Checking for the existence is probably the most right way to do it. I 
think you need to use the CPU ID instruction and traverse the 
information there until you have what you need to know.
I guess it is easier right now to do the check with the compiler option 
(as I suggested with the menu). We maybe should add a ukplat interface 
for CPU ID queries which we do not have yet.
> > >    +endif
> > > +
> > >    config LIBUKSWRAND_DEVFS
> > >        bool "Register random and urandom device to devfs"
> > >        select LIBDEVFS
> > > diff --git a/lib/ukswrand/include/uk/swrand.h
> > > b/lib/ukswrand/include/uk/swrand.h
> > > index 77912182..a3a1e227 100644
> > > --- a/lib/ukswrand/include/uk/swrand.h
> > > +++ b/lib/ukswrand/include/uk/swrand.h
> > > @@ -39,6 +39,7 @@
> > >    #include <uk/arch/types.h>
> > >    #include <uk/plat/lcpu.h>
> > >    #include <uk/config.h>
> > > +#include <uk/plat/time.h>
> > >      #ifdef __cplusplus
> > >    extern "C" {
> > > @@ -53,6 +54,25 @@ extern struct uk_swrand uk_swrand_def;
> > >    void uk_swrand_init_r(struct uk_swrand *r, __u32 seed);
> > >    __u32 uk_swrand_randr_r(struct uk_swrand *r);
> > >    +static inline __u32 _get_random_seed32(void)
> > > +{
> > > +    __u32 val;
> > > +
> > > +#ifdef CONFIG_LIBUKSWRAND_USE_INITIALSEED
> > > +    return CONFIG_LIBUKSWRAND_INITIALSEED;
> > > +#endif
> > > +
> > > +#ifdef CONFIG_ARCH_X86_64
> > > +    asm volatile ("rdrand %%eax;"
> > > +        : "=a" (val));
> > > +    return val;
> > > +#endif
> > > +
> > > +    /* TODO: Generate the seed randomly on ARM */
> > > +    val = (__u32)ukplat_wall_clock();
> > With this code, your implementation is now always using the timestamp.
> > I think this was not intended, right? ;-)
> Actually yes, I defaulted to ukplat_wall_clock when
> CONFIG_LIBUKSWRAND_USE_INITIALSEED is not selected on ARM.
>
> In the case of X86, we end up in one of the two guards. (both of them
> return a value)
Ah right, I see. Sorry.

> > > +    return val;
> > > +}
> > > +
> > >    /* Uses the pre-initialized default generator  */
> > >    /* TODO: Add assertion when we can test if we are in interrupt
> > > context */
> > >    /* TODO: Revisit with multi-CPU support */
> > > diff --git a/lib/ukswrand/mwc.c b/lib/ukswrand/mwc.c
> > > index 820aa2a2..6d785c07 100644
> > > --- a/lib/ukswrand/mwc.c
> > > +++ b/lib/ukswrand/mwc.c
> > > @@ -99,7 +99,7 @@ __u32 uk_swrand_randr_r(struct uk_swrand *r)
> > >    static void _uk_swrand_ctor(void)
> > >    {
> > >        uk_pr_info("Initialize random number generator...\n");
> > > -    uk_swrand_init_r(&uk_swrand_def, CONFIG_LIBUKSWRAND_INITIALSEED);
> > > +    uk_swrand_init_r(&uk_swrand_def, _get_random_seed32());
> > >    }
> > >      UK_CTOR_FUNC(UK_SWRAND_CTOR_PRIO, _uk_swrand_ctor);
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel