Re: [dpdk-dev] [PATCH v2] eal: choose initial PRNG seed source at runtime

["Mattias Rönnblom" <mattias.ronnblom@ericsson.com>]

On 2020-04-16 01:59, Dan Gora wrote:
> Instead of choosing to use getentropy() or the rdseed instruction for
> the random number generator entropy source using compilation flags,
> determine the best source at run time.


I like this idea.


> This is accomplished by defining a weak symbol for getentropy(),
> checking that the compiler can generate the rdseed instruction even if
> the compilation platform does not natively support it, and checking for
> the availability of the rdseed instruction on the execution platform
> using rte_cpu_get_flag_enabled().
>
> If neither getentropy() or rdseed is available, rte_get_timer_cycles()
> will be continue to be used as the entropy source.
>
> This also allows non-meson builds to use getentropy().
>
> Signed-off-by: Dan Gora <dg@adax.com>
> ---
> v2:
> * Rebase to latest master.
> * Fix spelling of "meson".
>
>   config/x86/meson.build             |  7 +++++++
>   lib/librte_eal/common/rte_random.c | 29 ++++++++++++++++++++++++-----
>   lib/librte_eal/meson.build         |  3 ---
>   mk/rte.cpuflags.mk                 |  8 ++++++++
>   4 files changed, 39 insertions(+), 8 deletions(-)
>
> diff --git a/config/x86/meson.build b/config/x86/meson.build
> index adc857ba2..214b16f2a 100644
> --- a/config/x86/meson.build
> +++ b/config/x86/meson.build
> @@ -20,6 +20,13 @@ if cc.get_define('__SSE4_2__', args: machine_args) == ''
>   	machine_args += '-msse4'
>   endif
>   
> +# set -mrdseed if necessary so _rdseed32_step compiles if the
> +# compilation host does not support the RDSEED instruction.
> +if cc.get_define('__RDSEED__', args: machine_args) == '' and cc.has_argument('-mrdseed')
> +	machine_args += '-mrdseed'
> +	message('RDSEED not enabled by default, explicitly setting -mrdseed')
> +endif
> +
>   base_flags = ['SSE', 'SSE2', 'SSE3','SSSE3', 'SSE4_1', 'SSE4_2']
>   foreach f:base_flags
>   	dpdk_conf.set('RTE_MACHINE_CPUFLAG_' + f, 1)
> diff --git a/lib/librte_eal/common/rte_random.c b/lib/librte_eal/common/rte_random.c
> index 57ec8fb2b..40f8b5aab 100644
> --- a/lib/librte_eal/common/rte_random.c
> +++ b/lib/librte_eal/common/rte_random.c
> @@ -25,6 +25,8 @@ struct rte_rand_state {
>   
>   static struct rte_rand_state rand_states[RTE_MAX_LCORE];
>   
> +__rte_weak int getentropy(void *__buffer, size_t __length);
> +
>   static uint32_t
>   __rte_rand_lcg32(uint32_t *seed)
>   {
> @@ -176,10 +178,24 @@ rte_rand_max(uint64_t upper_bound)
>   	return res;
>   }
>   
> +/* Use rte_get_timer_cycles() if the system does not have
> + * genentropy() or the rdseed instruction.
> + */
> +__rte_weak int
> +getentropy(void *__buffer, size_t __length __rte_unused)
> +{


Just make the weak getentropy() to return -1, always.


> +	uint64_t *ge_seed = __buffer;
> +#ifdef RTE_MACHINE_CPUFLAG_RDSEED
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_RDSEED))
> +		return -1;
> +#endif
> +	*ge_seed = rte_get_timer_cycles();
> +	return 0;
> +}
> +
>   static uint64_t
>   __rte_random_initial_seed(void)
>   {
> -#ifdef RTE_LIBEAL_USE_GETENTROPY
>   	int ge_rc;
>   	uint64_t ge_seed;
>   
> @@ -187,15 +203,18 @@ __rte_random_initial_seed(void)
>   
>   	if (ge_rc == 0)
>   		return ge_seed;
> -#endif
> +
>   #ifdef RTE_MACHINE_CPUFLAG_RDSEED
>   	unsigned int rdseed_low;
>   	unsigned int rdseed_high;
>   
>   	/* first fallback: rdseed instruction, if available */
> -	if (_rdseed32_step(&rdseed_low) == 1 &&
> -	    _rdseed32_step(&rdseed_high) == 1)
> -		return (uint64_t)rdseed_low | ((uint64_t)rdseed_high << 32);
> +	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_RDSEED)) {


Move the variable declarations into this scope.


> +		if (_rdseed32_step(&rdseed_low) == 1 &&
> +		    _rdseed32_step(&rdseed_high) == 1)
> +			return (uint64_t)rdseed_low |
> +				((uint64_t)rdseed_high << 32);
> +	}
>   #endif
>   	/* second fallback: seed using rdtsc */
>   	return rte_get_timer_cycles();
> diff --git a/lib/librte_eal/meson.build b/lib/librte_eal/meson.build
> index 0267c3b9d..748359b8c 100644
> --- a/lib/librte_eal/meson.build
> +++ b/lib/librte_eal/meson.build
> @@ -15,9 +15,6 @@ deps += 'kvargs'
>   if dpdk_conf.has('RTE_USE_LIBBSD')
>   	ext_deps += libbsd
>   endif
> -if cc.has_function('getentropy', prefix : '#include <unistd.h>')
> -	cflags += '-DRTE_LIBEAL_USE_GETENTROPY'
> -endif
>   if cc.has_header('getopt.h')
>   	cflags += ['-DHAVE_GETOPT_H', '-DHAVE_GETOPT', '-DHAVE_GETOPT_LONG']
>   endif
> diff --git a/mk/rte.cpuflags.mk b/mk/rte.cpuflags.mk
> index fa8753531..fb7bf8a53 100644
> --- a/mk/rte.cpuflags.mk
> +++ b/mk/rte.cpuflags.mk
> @@ -53,6 +53,14 @@ endif
>   
>   ifneq ($(filter $(AUTO_CPUFLAGS),__RDSEED__),)
>   CPUFLAGS += RDSEED
> +else
> +# If the native environment doesn't define __RDSEED__, see
> +# if the compiler supports -mrdseed.


For which environments is this true?


> +RDSEED_CPUFLAGS := $(shell $(CC) $(MACHINE_CFLAGS) $(WERROR_FLAGS) $(EXTRA_CFLAGS) -mrdseed -dM -E - < /dev/null)
> +ifneq ($(filter $(RDSEED_CPUFLAGS),__RDSEED__),)


There are no better ways to achieve this? It seems like a bit of a hack.


> +CPUFLAGS += RDSEED
> +MACHINE_CFLAGS += -mrdseed
> +endif
>   endif
>   
>   ifneq ($(filter $(AUTO_CPUFLAGS),__FSGSBASE__),)