[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.