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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

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:
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.
>   
> +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? ;-)
> +       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