[PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Matt Brown]

Currently ppc_md.get_random_seed uses the powernv_get_random_long function.
A guest calling this function would have to go through the hypervisor. The
'darn' instruction, introduced in POWER9, allows us to bypass this by
directly obtaining a value from the mmio region.

This patch adds a function for ppc_md.get_random_seed on p9,
utilising the darn instruction.

Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
---
v2:
	- remove repeat darn attempts
	- move hook to rng_init
---
 arch/powerpc/include/asm/ppc-opcode.h |  4 ++++
 arch/powerpc/platforms/powernv/rng.c  | 22 ++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index c4ced1d..d5f7082 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -134,6 +134,7 @@
 #define PPC_INST_COPY			0x7c00060c
 #define PPC_INST_COPY_FIRST		0x7c20060c
 #define PPC_INST_CP_ABORT		0x7c00068c
+#define PPC_INST_DARN			0x7c0005e6
 #define PPC_INST_DCBA			0x7c0005ec
 #define PPC_INST_DCBA_MASK		0xfc0007fe
 #define PPC_INST_DCBAL			0x7c2005ec
@@ -325,6 +326,9 @@
 
 /* Deal with instructions that older assemblers aren't aware of */
 #define	PPC_CP_ABORT		stringify_in_c(.long PPC_INST_CP_ABORT)
+#define PPC_DARN(t, l)		stringify_in_c(.long PPC_INST_DARN |  \
+						___PPC_RT(t)	   |  \
+						___PPC_RA(l))
 #define	PPC_DCBAL(a, b)		stringify_in_c(.long PPC_INST_DCBAL | \
 					__PPC_RA(a) | __PPC_RB(b))
 #define	PPC_DCBZL(a, b)		stringify_in_c(.long PPC_INST_DCBZL | \
diff --git a/arch/powerpc/platforms/powernv/rng.c b/arch/powerpc/platforms/powernv/rng.c
index 5dcbdea..ab6f411 100644
--- a/arch/powerpc/platforms/powernv/rng.c
+++ b/arch/powerpc/platforms/powernv/rng.c
@@ -8,6 +8,7 @@
  */
 
 #define pr_fmt(fmt)	"powernv-rng: " fmt
+#define DARN_ERR 0xFFFFFFFFFFFFFFFFul
 
 #include <linux/kernel.h>
 #include <linux/of.h>
@@ -16,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/smp.h>
 #include <asm/archrandom.h>
+#include <asm/cputable.h>
 #include <asm/io.h>
 #include <asm/prom.h>
 #include <asm/machdep.h>
@@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
 	return 1;
 }
 
+int powernv_get_random_darn(unsigned long *v)
+{
+	unsigned long val;
+
+	/* Using DARN with L=1 - conditioned random number */
+	asm (PPC_DARN(%0, 1)"\n" : "=r"(val) :);
+
+	if (val == DARN_ERR)
+		return 0;
+
+	*v = val;
+
+	return 1;
+}
+
 int powernv_get_random_long(unsigned long *v)
 {
 	struct powernv_rng *rng;
@@ -136,6 +153,7 @@ static __init int rng_create(struct device_node *dn)
 static __init int rng_init(void)
 {
 	struct device_node *dn;
+	unsigned long drn_test;
 	int rc;
 
 	for_each_compatible_node(dn, NULL, "ibm,power-rng") {
@@ -150,6 +168,10 @@ static __init int rng_init(void)
 		of_platform_device_create(dn, NULL, NULL);
 	}
 
+	if (cpu_has_feature(CPU_FTR_ARCH_300) &&
+	    powernv_get_random_darn(&drn_test))
+		ppc_md.get_random_seed = powernv_get_random_darn;
+
 	return 0;
 }
 machine_subsys_initcall(powernv, rng_init);
-- 
2.9.3

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Daniel Axtens]

Hi Matt,

> Currently ppc_md.get_random_seed uses the powernv_get_random_long function.
> A guest calling this function would have to go through the hypervisor. The
> 'darn' instruction, introduced in POWER9, allows us to bypass this by
> directly obtaining a value from the mmio region.
>
> This patch adds a function for ppc_md.get_random_seed on p9,
> utilising the darn instruction.

This patch looks pretty good - I'm not set up to test it but I have one
code-style nit:

> diff --git a/arch/powerpc/platforms/powernv/rng.c b/arch/powerpc/platforms/powernv/rng.c
> index 5dcbdea..ab6f411 100644
> --- a/arch/powerpc/platforms/powernv/rng.c
> +++ b/arch/powerpc/platforms/powernv/rng.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #define pr_fmt(fmt)	"powernv-rng: " fmt
> +#define DARN_ERR 0xFFFFFFFFFFFFFFFFul
>  
>  #include <linux/kernel.h>
>  #include <linux/of.h>
> @@ -16,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/smp.h>
>  #include <asm/archrandom.h>
> +#include <asm/cputable.h>
>  #include <asm/io.h>
>  #include <asm/prom.h>
>  #include <asm/machdep.h>
> @@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
>  	return 1;
>  }
>  
> +int powernv_get_random_darn(unsigned long *v)

This is only referenced in this file so it should probably be labelled
as 'static'.

> +{
> +	unsigned long val;
> +
> +	/* Using DARN with L=1 - conditioned random number */
> +	asm (PPC_DARN(%0, 1)"\n" : "=r"(val) :);
> +
> +	if (val == DARN_ERR)
> +		return 0;
> +
> +	*v = val;
> +
> +	return 1;

I was a bit confused to see 1 representing success - I think I have been
in userspace too long. But I checked against pseries_get_random_long and
it is in fact correct, so good for you!

An excellent followup patch would be changing the type of this function
to be bool rather than int, but no pressure :)

Regards,
Daniel

> +}
> +
>  int powernv_get_random_long(unsigned long *v)
>  {
>  	struct powernv_rng *rng;
> @@ -136,6 +153,7 @@ static __init int rng_create(struct device_node *dn)
>  static __init int rng_init(void)
>  {
>  	struct device_node *dn;
> +	unsigned long drn_test;
>  	int rc;
>  
>  	for_each_compatible_node(dn, NULL, "ibm,power-rng") {
> @@ -150,6 +168,10 @@ static __init int rng_init(void)
>  		of_platform_device_create(dn, NULL, NULL);
>  	}
>  
> +	if (cpu_has_feature(CPU_FTR_ARCH_300) &&
> +	    powernv_get_random_darn(&drn_test))
> +		ppc_md.get_random_seed = powernv_get_random_darn;
> +
>  	return 0;
>  }
>  machine_subsys_initcall(powernv, rng_init);
> -- 
> 2.9.3

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Matt Brown]

On Tue, Jul 11, 2017 at 7:34 PM, Daniel Axtens <dja@axtens.net> wrote:
> Hi Matt,
>
>> Currently ppc_md.get_random_seed uses the powernv_get_random_long function.
>> A guest calling this function would have to go through the hypervisor. The
>> 'darn' instruction, introduced in POWER9, allows us to bypass this by
>> directly obtaining a value from the mmio region.
>>
>> This patch adds a function for ppc_md.get_random_seed on p9,
>> utilising the darn instruction.
>
> This patch looks pretty good - I'm not set up to test it but I have one
> code-style nit:
>
>> diff --git a/arch/powerpc/platforms/powernv/rng.c b/arch/powerpc/platforms/powernv/rng.c
>> index 5dcbdea..ab6f411 100644
>> --- a/arch/powerpc/platforms/powernv/rng.c
>> +++ b/arch/powerpc/platforms/powernv/rng.c
>> @@ -8,6 +8,7 @@
>>   */
>>
>>  #define pr_fmt(fmt)  "powernv-rng: " fmt
>> +#define DARN_ERR 0xFFFFFFFFFFFFFFFFul
>>
>>  #include <linux/kernel.h>
>>  #include <linux/of.h>
>> @@ -16,6 +17,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/smp.h>
>>  #include <asm/archrandom.h>
>> +#include <asm/cputable.h>
>>  #include <asm/io.h>
>>  #include <asm/prom.h>
>>  #include <asm/machdep.h>
>> @@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
>>       return 1;
>>  }
>>
>> +int powernv_get_random_darn(unsigned long *v)
>
> This is only referenced in this file so it should probably be labelled
> as 'static'.
>

That's true, my thinking was to then use the get_random_darn where the
get_random_long was being used.
Looks like there is only one other caller of it at the moment,
kvmppc_h_random (in arch/powerpc/kvm/book3s_hv_builtin.c).
We may want to change that to get_random_darn.

>> +{
>> +     unsigned long val;
>> +
>> +     /* Using DARN with L=1 - conditioned random number */
>> +     asm (PPC_DARN(%0, 1)"\n" : "=r"(val) :);
>> +
>> +     if (val == DARN_ERR)
>> +             return 0;
>> +
>> +     *v = val;
>> +
>> +     return 1;
>
> I was a bit confused to see 1 representing success - I think I have been
> in userspace too long. But I checked against pseries_get_random_long and
> it is in fact correct, so good for you!
>
> An excellent followup patch would be changing the type of this function
> to be bool rather than int, but no pressure :)

That's probably a good idea!

Thanks,
Matt

>
> Regards,
> Daniel
>
>> +}
>> +
>>  int powernv_get_random_long(unsigned long *v)
>>  {
>>       struct powernv_rng *rng;
>> @@ -136,6 +153,7 @@ static __init int rng_create(struct device_node *dn)
>>  static __init int rng_init(void)
>>  {
>>       struct device_node *dn;
>> +     unsigned long drn_test;
>>       int rc;
>>
>>       for_each_compatible_node(dn, NULL, "ibm,power-rng") {
>> @@ -150,6 +168,10 @@ static __init int rng_init(void)
>>               of_platform_device_create(dn, NULL, NULL);
>>       }
>>
>> +     if (cpu_has_feature(CPU_FTR_ARCH_300) &&
>> +         powernv_get_random_darn(&drn_test))
>> +             ppc_md.get_random_seed = powernv_get_random_darn;
>> +
>>       return 0;
>>  }
>>  machine_subsys_initcall(powernv, rng_init);
>> --
>> 2.9.3

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Michael Ellerman]

Matt Brown <matthew.brown.dev@gmail.com> writes:
> On Tue, Jul 11, 2017 at 7:34 PM, Daniel Axtens <dja@axtens.net> wrote:
>>> @@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
>>>       return 1;
>>>  }
>>>
>>> +int powernv_get_random_darn(unsigned long *v)
>>
>> This is only referenced in this file so it should probably be labelled
>> as 'static'.
>>
>
> That's true, my thinking was to then use the get_random_darn where the
> get_random_long was being used.
> Looks like there is only one other caller of it at the moment,
> kvmppc_h_random (in arch/powerpc/kvm/book3s_hv_builtin.c).
> We may want to change that to get_random_darn.

It needs to use the right one depending on whether it's running on P8 or
P9.

It should really just use ppc_md.get_random_seed(), except that it can
be called in real mode, which means it needs to be more careful. So as
usual it's complicated :)

cheers

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Michael Ellerman]

Hi Matt,

A few comments inline ...

Matt Brown <matthew.brown.dev@gmail.com> writes:
> Currently ppc_md.get_random_seed uses the powernv_get_random_long function.
> A guest calling this function would have to go through the hypervisor. The

This is not quite right. The powernv routine is only ever used on bare
metal. In a guest we use pseries_get_random_long(), which does go via
they hypervisor.

On both Power8 and Power9 there is a hardware RNG per chip. The
difference is that on Power8 we had to go and find it in the device tree
and read from it via MMIO. On Power9 that is all done transparently for
us by the DARN instruction.

> 'darn' instruction, introduced in POWER9, allows us to bypass this by
> directly obtaining a value from the mmio region.
>
> This patch adds a function for ppc_md.get_random_seed on p9,
> utilising the darn instruction.
>
> Signed-off-by: Matt Brown <matthew.brown.dev@gmail.com>
> ---
> v2:
> 	- remove repeat darn attempts
> 	- move hook to rng_init
> ---
>  arch/powerpc/include/asm/ppc-opcode.h |  4 ++++
>  arch/powerpc/platforms/powernv/rng.c  | 22 ++++++++++++++++++++++
>  2 files changed, 26 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
> index c4ced1d..d5f7082 100644
> --- a/arch/powerpc/include/asm/ppc-opcode.h
> +++ b/arch/powerpc/include/asm/ppc-opcode.h
> @@ -134,6 +134,7 @@
>  #define PPC_INST_COPY			0x7c00060c
>  #define PPC_INST_COPY_FIRST		0x7c20060c
>  #define PPC_INST_CP_ABORT		0x7c00068c
> +#define PPC_INST_DARN			0x7c0005e6

That looks right to me.

> @@ -325,6 +326,9 @@
>  
>  /* Deal with instructions that older assemblers aren't aware of */
>  #define	PPC_CP_ABORT		stringify_in_c(.long PPC_INST_CP_ABORT)
> +#define PPC_DARN(t, l)		stringify_in_c(.long PPC_INST_DARN |  \
> +						___PPC_RT(t)	   |  \
> +						___PPC_RA(l))

But this is not quite.

The macros are:

#define ___PPC_RA(a)	(((a) & 0x1f) << 16)
#define ___PPC_RS(s)	(((s) & 0x1f) << 21)
#define ___PPC_RT(t)	___PPC_RS(t)

But the definition of darn is:

 +---------+---------+---------+---------+---------+---------+----+
 |    31   |    RT   |    /    |    L    |    /    |   755   | /  |
 | 31 - 26 | 25 - 21 | 20 - 18 | 17 - 16 | 15 - 11 |  10 - 1 | 0  |
 +---------+---------+---------+---------+---------+---------+----+

Using ___PPC_RT() gets you the right shift and mask, but because it's
the triple underscore verison, it doesn't check that you pass a register
number to it. You should use __PPC_RT() instead.

And ___PPC_RA() is not quite right. The L field is only 2 bits wide, not
the 5 that ___PPC_RA() allows.

We don't have a __PPC_L() macro, because L fields vary in size and
location. So I think you're best of open coding it, eg:

+#define PPC_DARN(t, l)		stringify_in_c(.long PPC_INST_DARN |  \
+						__PPC_RT(t)	   |  \
+						(((l) & 0x3) << 16))

> diff --git a/arch/powerpc/platforms/powernv/rng.c b/arch/powerpc/platforms/powernv/rng.c
> index 5dcbdea..ab6f411 100644
> --- a/arch/powerpc/platforms/powernv/rng.c
> +++ b/arch/powerpc/platforms/powernv/rng.c
> @@ -8,6 +8,7 @@
>   */
>  
>  #define pr_fmt(fmt)	"powernv-rng: " fmt
> +#define DARN_ERR 0xFFFFFFFFFFFFFFFFul

Usual place for constants is after all the includes, before the first
code or variables.

>  #include <linux/kernel.h>
>  #include <linux/of.h>
> @@ -16,6 +17,7 @@
>  #include <linux/slab.h>
>  #include <linux/smp.h>
>  #include <asm/archrandom.h>
> +#include <asm/cputable.h>
>  #include <asm/io.h>
>  #include <asm/prom.h>
>  #include <asm/machdep.h>
> @@ -67,6 +69,21 @@ int powernv_get_random_real_mode(unsigned long *v)
>  	return 1;
>  }
>  
> +int powernv_get_random_darn(unsigned long *v)
> +{
> +	unsigned long val;
> +
> +	/* Using DARN with L=1 - conditioned random number */

Actually L=1 is a 64-bit conditioned random number, vs L=0 for 32-bit.

> +	asm (PPC_DARN(%0, 1)"\n" : "=r"(val) :);
> +
> +	if (val == DARN_ERR)
> +		return 0;
> +
> +	*v = val;
> +
> +	return 1;
> +}
> +
>  int powernv_get_random_long(unsigned long *v)
>  {
>  	struct powernv_rng *rng;
> @@ -136,6 +153,7 @@ static __init int rng_create(struct device_node *dn)
>  static __init int rng_init(void)
>  {
>  	struct device_node *dn;
> +	unsigned long drn_test;

Buy yourself a vowel! ;)

>  	int rc;
>  
>  	for_each_compatible_node(dn, NULL, "ibm,power-rng") {
> @@ -150,6 +168,10 @@ static __init int rng_init(void)
>  		of_platform_device_create(dn, NULL, NULL);
>  	}
>  
> +	if (cpu_has_feature(CPU_FTR_ARCH_300) &&
> +	    powernv_get_random_darn(&drn_test))
> +		ppc_md.get_random_seed = powernv_get_random_darn;

I know we took the loop out of powernv_get_random_darn(), but we might
want to put it back in this case. ie. we don't want to skip registering
it just because we happened to get one error.

So perhaps 10 iterations and if they all fail then you don't register.
Also it's probably worth a pr_warn() if you can't get any good values,
because that shouldn't happen and we shouldn't silently continue.

cheers

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Segher Boessenkool]

On Mon, Jul 31, 2017 at 07:10:15PM +1000, Michael Ellerman wrote:
> And ___PPC_RA() is not quite right. The L field is only 2 bits wide, not
> the 5 that ___PPC_RA() allows.
> 
> We don't have a __PPC_L() macro, because L fields vary in size and
> location. So I think you're best of open coding it, eg:
> 
> +#define PPC_DARN(t, l)		stringify_in_c(.long PPC_INST_DARN |  \
> +						__PPC_RT(t)	   |  \
> +						(((l) & 0x3) << 16))

It would be better if you could do a compile-time error if the L value
is out of range.  Hrm, nothing else does such checking either?


Segher

Re: [PATCH v2] powerpc/powernv: Use darn instr for random_seed on p9

[Matt Brown]

On Tue, Aug 1, 2017 at 10:57 PM, Segher Boessenkool
<segher@kernel.crashing.org> wrote:
> On Mon, Jul 31, 2017 at 07:10:15PM +1000, Michael Ellerman wrote:
>> And ___PPC_RA() is not quite right. The L field is only 2 bits wide, not
>> the 5 that ___PPC_RA() allows.
>>
>> We don't have a __PPC_L() macro, because L fields vary in size and
>> location. So I think you're best of open coding it, eg:
>>
>> +#define PPC_DARN(t, l)               stringify_in_c(.long PPC_INST_DARN |  \
>> +                                             __PPC_RT(t)        |  \
>> +                                             (((l) & 0x3) << 16))
>
> It would be better if you could do a compile-time error if the L value
> is out of range.  Hrm, nothing else does such checking either?
>

Yeah currently the only checks are whether the register value is
valid, using the __PPC_R{A,B,S,T} macros.
However, we can't use these macros for inline asm because we're
passing a variable into it
so the pre-processor attempts to look for register %0 which breaks it.
(Have to use triple underscore versions)

We could add more checking to validate the L value, but I don't know
how much of an issue it currently is.
A question for mpe I guess.

Thanks,
Matt


>
> Segher