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

[Vlad-Andrei BĂDOIU (78692) <vlad_andrei.badoiu@xxxxxxxxxxxxxxx>]

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?
>
>>   +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)

>
>> +    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