enable dual rng on VIA C7

enable dual rng on VIA C7

[Udo van den Heuvel]

[-- Attachment #1: Type: text/plain, Size: 91 bytes --]

Hello,

Any reason why the second rng on the VIA C7 CPU is not enabled?

Kind regards,
Udo

[-- Attachment #2: via-rng.patch --]
[-- Type: text/plain, Size: 633 bytes --]

--- old/drivers/char/hw_random/via-rng.c	2007-11-11 19:39:49.000000000 +0100
+++ new/drivers/char/hw_random/via-rng.c	2007-11-11 19:40:41.000000000 +0100
@@ -41,6 +41,7 @@
 	VIA_STRFILT_ENABLE	= (1 << 14),
 	VIA_RAWBITS_ENABLE	= (1 << 13),
 	VIA_RNG_ENABLE		= (1 << 6),
+	VIA_RNG_DUAL            = (1 << 9),
 	VIA_XSTORE_CNT_MASK	= 0x0F,
 
 	VIA_RNG_CHUNK_8		= 0x00,	/* 64 rand bits, 64 stored bits */
@@ -128,6 +129,7 @@
 	lo &= ~(0x7f << VIA_STRFILT_CNT_SHIFT);
 	lo &= ~VIA_XSTORE_CNT_MASK;
 	lo &= ~(VIA_STRFILT_ENABLE | VIA_STRFILT_FAIL | VIA_RAWBITS_ENABLE);
+	lo |= VIA_RNG_DUAL;
 	lo |= VIA_RNG_ENABLE;
 
 	if (lo != old_lo)

Re: enable dual rng on VIA C7

[Andrew Morton]

On Sun, 11 Nov 2007 19:49:08 +0100 Udo van den Heuvel <udovdh@xs4all.nl> wrote:

> Any reason why the second rng on the VIA C7 CPU is not enabled?
> 
> Kind regards,
> Udo
> 
> 
> [via-rng.patch  text/plain (634B)]
> --- old/drivers/char/hw_random/via-rng.c	2007-11-11 19:39:49.000000000 +0100
> +++ new/drivers/char/hw_random/via-rng.c	2007-11-11 19:40:41.000000000 +0100
> @@ -41,6 +41,7 @@
>  	VIA_STRFILT_ENABLE	= (1 << 14),
>  	VIA_RAWBITS_ENABLE	= (1 << 13),
>  	VIA_RNG_ENABLE		= (1 << 6),
> +	VIA_RNG_DUAL            = (1 << 9),
>  	VIA_XSTORE_CNT_MASK	= 0x0F,
>  
>  	VIA_RNG_CHUNK_8		= 0x00,	/* 64 rand bits, 64 stored bits */
> @@ -128,6 +129,7 @@
>  	lo &= ~(0x7f << VIA_STRFILT_CNT_SHIFT);
>  	lo &= ~VIA_XSTORE_CNT_MASK;
>  	lo &= ~(VIA_STRFILT_ENABLE | VIA_STRFILT_FAIL | VIA_RAWBITS_ENABLE);
> +	lo |= VIA_RNG_DUAL;
>  	lo |= VIA_RNG_ENABLE;
>  
>  	if (lo != old_lo)
> 

Does the patch work?

It's missing a signed-off-by:, btw

Re: enable dual rng on VIA C7

[Udo van den Heuvel]

Andrew Morton wrote:
> On Sun, 11 Nov 2007 19:49:08 +0100 Udo van den Heuvel <udovdh@xs4all.nl> wrote:
> 
>> Any reason why the second rng on the VIA C7 CPU is not enabled?
(...)
> Does the patch work?

Yes, at least:

dd if=/dev/hwrng of=/dev/null bs=1024 count=1024
shows a higher speed than without the patch.

> It's missing a signed-off-by:, btw

I did not know we are already that far ;-)
I mean: can this patch be aplied without hurting C3/C7 CPU's with just
one RNG? Maybe an expert needs to test/answer?
Maybe some logic needs to be applied around the extra bit?

Kind regards,
Udo

Re: enable dual rng on VIA C7

[Dave Jones]

On Mon, Nov 26, 2007 at 06:02:39PM +0100, Udo van den Heuvel wrote:

 > I did not know we are already that far ;-)
 > I mean: can this patch be aplied without hurting C3/C7 CPU's with just
 > one RNG? Maybe an expert needs to test/answer?
 > Maybe some logic needs to be applied around the extra bit?
 
>From the padlock spec..

"SRC Bits[9:8] Noise source select (I): These bits control the two noise
 sources on the processor that input bits to the accumulation buffers.
 On Nehemiah processors prior to stepping 8, these bits are reserved
 and undefined. The default RESET state is both bits = 0." 

Something like this perhaps ?

Signed-off-by: Dave Jones <davej@redhat.com>

diff --git a/drivers/char/hw_random/via-rng.c b/drivers/char/hw_random/via-rng.c
index ec435cb..95aac0f 100644
--- a/drivers/char/hw_random/via-rng.c
+++ b/drivers/char/hw_random/via-rng.c
@@ -41,6 +41,8 @@ enum {
 	VIA_STRFILT_ENABLE	= (1 << 14),
 	VIA_RAWBITS_ENABLE	= (1 << 13),
 	VIA_RNG_ENABLE		= (1 << 6),
+	VIA_NOISESRC1		= (1 << 8),
+	VIA_NOISESRC2		= (1 << 9),
 	VIA_XSTORE_CNT_MASK	= 0x0F,
 
 	VIA_RNG_CHUNK_8		= 0x00,	/* 64 rand bits, 64 stored bits */
@@ -114,6 +116,7 @@ static int via_rng_data_read(struct hwrng *rng, u32 *data)
 
 static int via_rng_init(struct hwrng *rng)
 {
+	struct cpuinfo_x86 *c = &cpu_data(0);
 	u32 lo, hi, old_lo;
 
 	/* Control the RNG via MSR.  Tread lightly and pay very close
@@ -129,6 +132,17 @@ static int via_rng_init(struct hwrng *rng)
 	lo &= ~VIA_XSTORE_CNT_MASK;
 	lo &= ~(VIA_STRFILT_ENABLE | VIA_STRFILT_FAIL | VIA_RAWBITS_ENABLE);
 	lo |= VIA_RNG_ENABLE;
+	lo |= VIA_NOISESRC1;
+
+	/* Enable secondary noise source on CPUs where it is present. */
+
+	/* Nehemiah stepping 8 and higher */
+	if ((c->x86_model == 9) && (c->x86_mask > 7))
+		lo |= VIA_NOISESRC2;
+
+	/* Esther */
+	if (c->x86_model >= 10)
+		lo |= VIA_NOISESRC2;
 
 	if (lo != old_lo)
 		wrmsr(MSR_VIA_RNG, lo, hi);
-- 
http://www.codemonkey.org.uk

Re: enable dual rng on VIA C7

[Udo van den Heuvel]

Dave Jones wrote:
> On Mon, Nov 26, 2007 at 06:02:39PM +0100, Udo van den Heuvel wrote:
> 
>  > I did not know we are already that far ;-)
>  > I mean: can this patch be aplied without hurting C3/C7 CPU's with just
>  > one RNG? Maybe an expert needs to test/answer?
>  > Maybe some logic needs to be applied around the extra bit?
>  
>>From the padlock spec..
> 
> "SRC Bits[9:8] Noise source select (I): These bits control the two noise
>  sources on the processor that input bits to the accumulation buffers.
>  On Nehemiah processors prior to stepping 8, these bits are reserved
>  and undefined. The default RESET state is both bits = 0." 
> 
> Something like this perhaps ?

Yes, I think that's a big step in the right direction!

But I am no expert and cannot really judge how necessary or correct the
implementation is w.r.t. the 'undefined' function bits for CPU's that
lack a certain feature.

Re: enable dual rng on VIA C7

[Dave Jones]

On Tue, Nov 27, 2007 at 05:08:26PM +0100, Udo van den Heuvel wrote:
 > Dave Jones wrote:
 > > On Mon, Nov 26, 2007 at 06:02:39PM +0100, Udo van den Heuvel wrote:
 > > 
 > >  > I did not know we are already that far ;-)
 > >  > I mean: can this patch be aplied without hurting C3/C7 CPU's with just
 > >  > one RNG? Maybe an expert needs to test/answer?
 > >  > Maybe some logic needs to be applied around the extra bit?
 > >  
 > >>From the padlock spec..
 > > 
 > > "SRC Bits[9:8] Noise source select (I): These bits control the two noise
 > >  sources on the processor that input bits to the accumulation buffers.
 > >  On Nehemiah processors prior to stepping 8, these bits are reserved
 > >  and undefined. The default RESET state is both bits = 0." 
 > > 
 > > Something like this perhaps ?
 > 
 > Yes, I think that's a big step in the right direction!
 > 
 > But I am no expert and cannot really judge how necessary or correct the
 > implementation is w.r.t. the 'undefined' function bits for CPU's that
 > lack a certain feature.

The checks at the end of the patch for the x86_mask/model ensure
we only enable the 2nd noise source on CPUs documented to have it,
so we should be safe.

Andrew, want to throw that in the -mm pile for a while?

	Dave

-- 
http://www.codemonkey.org.uk

Re: enable dual rng on VIA C7

[Udo van den Heuvel]

Dave Jones wrote:
>  > > Something like this perhaps ?
>  > 
>  > Yes, I think that's a big step in the right direction!
>  > 
>  > But I am no expert and cannot really judge how necessary or correct the
>  > implementation is w.r.t. the 'undefined' function bits for CPU's that
>  > lack a certain feature.
> 
> The checks at the end of the patch for the x86_mask/model ensure
> we only enable the 2nd noise source on CPUs documented to have it,
> so we should be safe.
> 
> Andrew, want to throw that in the -mm pile for a while?

Thanks for assuring we are 'safe'.
Sounds OK to me.
Thanks for picking up this tiny improvement.

Any ideas on the power consumption increase question I received w.r.t.
this patch? (or why VIA would have made the 2nd RNG switchable)

Re: enable dual rng on VIA C7

[Andrew Morton]

On Tue, 27 Nov 2007 13:50:53 -0500
Dave Jones <davej@redhat.com> wrote:

> On Tue, Nov 27, 2007 at 05:08:26PM +0100, Udo van den Heuvel wrote:
>  > Dave Jones wrote:
>  > > On Mon, Nov 26, 2007 at 06:02:39PM +0100, Udo van den Heuvel wrote:
>  > > 
>  > >  > I did not know we are already that far ;-)
>  > >  > I mean: can this patch be aplied without hurting C3/C7 CPU's with just
>  > >  > one RNG? Maybe an expert needs to test/answer?
>  > >  > Maybe some logic needs to be applied around the extra bit?
>  > >  
>  > >>From the padlock spec..
>  > > 
>  > > "SRC Bits[9:8] Noise source select (I): These bits control the two noise
>  > >  sources on the processor that input bits to the accumulation buffers.
>  > >  On Nehemiah processors prior to stepping 8, these bits are reserved
>  > >  and undefined. The default RESET state is both bits = 0." 
>  > > 
>  > > Something like this perhaps ?
>  > 
>  > Yes, I think that's a big step in the right direction!
>  > 
>  > But I am no expert and cannot really judge how necessary or correct the
>  > implementation is w.r.t. the 'undefined' function bits for CPU's that
>  > lack a certain feature.
> 
> The checks at the end of the patch for the x86_mask/model ensure
> we only enable the 2nd noise source on CPUs documented to have it,
> so we should be safe.
> 
> Andrew, want to throw that in the -mm pile for a while?
> 

Did that, renamed to "via-rng: enable secondary noise source on CPUs where
it is present".

Has anyone tested it?

Re: enable dual rng on VIA C7

[Udo van den Heuvel]

Andrew Morton wrote:
>> Andrew, want to throw that in the -mm pile for a while?
> 
> Did that, renamed to "via-rng: enable secondary noise source on CPUs where
> it is present".
> 
> Has anyone tested it?

I hope so; I did not yet get that far since the patche does not compile
on 2.6.23.8. I was assured by Dave that the patch should work on 2.6.24
(and mm-tree).

Re: enable dual rng on VIA C7

[Udo van den Heuvel]

Andrew Morton wrote:
> On Tue, 27 Nov 2007 13:50:53 -0500
> Dave Jones <davej@redhat.com> wrote:
> 
>> On Tue, Nov 27, 2007 at 05:08:26PM +0100, Udo van den Heuvel wrote:
(...)
>>  > Yes, I think that's a big step in the right direction!
(...)
>> Andrew, want to throw that in the -mm pile for a while?
>>
> 
> Did that, renamed to "via-rng: enable secondary noise source on CPUs where
> it is present".
> 
> Has anyone tested it?

Works for me.
Did anyone else have a look?