diff mbox series

[3/3] RISC-V: Implement archrandom when Zkr is available

Message ID 20230627143747.1599218-4-sameo@rivosinc.com (mailing list archive)
State Superseded
Headers show
Series RISC-V: archrandom support | expand

Checks

Context Check Description
conchuod/cover_letter success Series has a cover letter
conchuod/tree_selection success Guessed tree name to be for-next at HEAD 488833ccdcac
conchuod/fixes_present success Fixes tag not required for -next series
conchuod/maintainers_pattern success MAINTAINERS pattern errors before the patch: 6 and now 6
conchuod/verify_signedoff success Signed-off-by tag matches author and committer
conchuod/kdoc success Errors and warnings before: 0 this patch: 0
conchuod/build_rv64_clang_allmodconfig success Errors and warnings before: 2857 this patch: 2857
conchuod/module_param success Was 0 now: 0
conchuod/build_rv64_gcc_allmodconfig success Errors and warnings before: 16720 this patch: 16720
conchuod/build_rv32_defconfig success Build OK
conchuod/dtb_warn_rv64 success Errors and warnings before: 20 this patch: 20
conchuod/header_inline success No static functions without inline keyword in header files
conchuod/checkpatch warning WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
conchuod/build_rv64_nommu_k210_defconfig success Build OK
conchuod/verify_fixes success No Fixes tag
conchuod/build_rv64_nommu_virt_defconfig success Build OK

Commit Message

Samuel Ortiz June 27, 2023, 2:37 p.m. UTC 
The Zkr extension is ratified and provides 16 bits of entropy seed when
reading the SEED CSR.

We can implement arch_get_random_seed_longs() by doing multiple csrrw to
that CSR and filling an unsigned long with valid entropy bits.

Signed-off-by: Samuel Ortiz <sameo@rivosinc.com>
---
 arch/riscv/include/asm/archrandom.h | 66 +++++++++++++++++++++++++++++
 arch/riscv/include/asm/csr.h        |  9 ++++
 2 files changed, 75 insertions(+)
 create mode 100644 arch/riscv/include/asm/archrandom.h

Comments

Conor Dooley June 27, 2023, 7:09 p.m. UTC | #1 
Hey Samuel,

On Tue, Jun 27, 2023 at 04:37:44PM +0200, Samuel Ortiz wrote:
> The Zkr extension is ratified and provides 16 bits of entropy seed when
> reading the SEED CSR.
> 
> We can implement arch_get_random_seed_longs() by doing multiple csrrw to
> that CSR and filling an unsigned long with valid entropy bits.
> 
> Signed-off-by: Samuel Ortiz <sameo@rivosinc.com>
> ---
>  arch/riscv/include/asm/archrandom.h | 66 +++++++++++++++++++++++++++++
>  arch/riscv/include/asm/csr.h        |  9 ++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 arch/riscv/include/asm/archrandom.h
> 
> diff --git a/arch/riscv/include/asm/archrandom.h b/arch/riscv/include/asm/archrandom.h
> new file mode 100644
> index 000000000000..3d01aab2800a
> --- /dev/null
> +++ b/arch/riscv/include/asm/archrandom.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Kernel interface for the RISCV arch_random_* functions
> + *
> + * Copyright (c) 2022 by Rivos Inc.
> + *
> + */
> +
> +#ifndef ASM_RISCV_ARCHRANDOM_H
> +#define ASM_RISCV_ARCHRANDOM_H
> +
> +#include <asm/csr.h>
> +
> +#define PR_PREFIX "Zkr Extension: "

Does pr_fmt() not work for you?
Also, "Zkr Extension" doesn't really seem super helpful to a punter if
they saw it in a log. Why not s/Zkr Extension/archrandom/, or similar?

> +#define SEED_RETRY_LOOPS 10
> +
> +static inline bool __must_check csr_seed_long(unsigned long *v)
> +{
> +	unsigned int retry = SEED_RETRY_LOOPS;
> +	unsigned int needed_seeds = sizeof(unsigned long) / 2, valid_seeds = 0;
> +	u16 *entropy = (u16 *)v;
> +
> +	do {
> +		/*
> +		 * The SEED CSR (0x015) must be accessed with a read-write
> +		 * instruction. Moreover, implementations must ignore the write
> +		 * value, its purpose is to signal polling for new seed.
> +		 */

What relevance does the second half of this comment have to the kernel?

> +		unsigned long csr_seed = csr_swap(CSR_SEED, 0);
> +
> +		switch (csr_seed & SEED_OPST_MASK) {
> +		case SEED_OPST_ES16:
> +			entropy[valid_seeds++] = csr_seed & SEED_ENTROPY_MASK;
> +			if (valid_seeds == needed_seeds)
> +				return true;
> +			break;
> +
> +		case SEED_OPST_DEAD:
> +			pr_err_once(PR_PREFIX "Unrecoverable error\n");
> +			return false;
> +
> +		case SEED_OPST_BIST:
> +			pr_info(PR_PREFIX "On going Built-in Self Test\n");

tiny nit, "On-going"? My OCD is bother by the capitalisation otherwise.

> +			fallthrough;
> +
> +		case SEED_OPST_WAIT:
> +		default:
> +			continue;
> +		}
> +
> +	} while (--retry);
> +
> +	return false;
> +}
> +
> +static inline size_t __must_check arch_get_random_longs(unsigned long *v, size_t max_longs)
> +{
> +	return 0;
> +}
> +
> +static inline size_t __must_check arch_get_random_seed_longs(unsigned long *v, size_t max_longs)
> +{
> +	return max_longs && riscv_isa_extension_available(NULL, ZKR) && csr_seed_long(v) ? 1 : 0;

Could you please write this in a more readable way, even if that is
going to be a lot more verbose? I know you copied it from x86, but
that's not really an excuse ;)

Also, is there a reason that you opted not to use the alternative-backed
riscv_has_extension_[un]likely() here? The ones backed by alternatives
are preferred if you are not checking on a per-cpu, or once-off basis.

Cheers,
Conor.

> +}
> +
> +#endif /* ASM_RISCV_ARCHRANDOM_H */
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index b98b3b6c9da2..7d0ca9082c66 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -389,6 +389,15 @@
>  #define CSR_VTYPE		0xc21
>  #define CSR_VLENB		0xc22
>  
> +/* Scalar Crypto Extension - Entropy */
> +#define CSR_SEED		0x015
> +#define SEED_OPST_MASK		_AC(0xC0000000, UL)
> +#define SEED_OPST_BIST		_AC(0x00000000, UL)
> +#define SEED_OPST_WAIT		_AC(0x40000000, UL)
> +#define SEED_OPST_ES16		_AC(0x80000000, UL)
> +#define SEED_OPST_DEAD		_AC(0xC0000000, UL)
> +#define SEED_ENTROPY_MASK	_AC(0xFFFF, UL)
> +
>  #ifdef CONFIG_RISCV_M_MODE
>  # define CSR_STATUS	CSR_MSTATUS
>  # define CSR_IE		CSR_MIE
> -- 
> 2.41.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Stefan O'Rear June 28, 2023, 1 a.m. UTC | #2 
On Tue, Jun 27, 2023, at 10:37 AM, Samuel Ortiz wrote:
> The Zkr extension is ratified and provides 16 bits of entropy seed when
> reading the SEED CSR.
>
> We can implement arch_get_random_seed_longs() by doing multiple csrrw to
> that CSR and filling an unsigned long with valid entropy bits.
>
> Signed-off-by: Samuel Ortiz <sameo@rivosinc.com>
> ---
>  arch/riscv/include/asm/archrandom.h | 66 +++++++++++++++++++++++++++++
>  arch/riscv/include/asm/csr.h        |  9 ++++
>  2 files changed, 75 insertions(+)
>  create mode 100644 arch/riscv/include/asm/archrandom.h
>
> diff --git a/arch/riscv/include/asm/archrandom.h 
> b/arch/riscv/include/asm/archrandom.h
> new file mode 100644
> index 000000000000..3d01aab2800a
> --- /dev/null
> +++ b/arch/riscv/include/asm/archrandom.h
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Kernel interface for the RISCV arch_random_* functions
> + *
> + * Copyright (c) 2022 by Rivos Inc.
> + *
> + */
> +
> +#ifndef ASM_RISCV_ARCHRANDOM_H
> +#define ASM_RISCV_ARCHRANDOM_H
> +
> +#include <asm/csr.h>
> +
> +#define PR_PREFIX "Zkr Extension: "
> +#define SEED_RETRY_LOOPS 10
> +
> +static inline bool __must_check csr_seed_long(unsigned long *v)
> +{
> +	unsigned int retry = SEED_RETRY_LOOPS;
> +	unsigned int needed_seeds = sizeof(unsigned long) / 2, valid_seeds = 
> 0;
> +	u16 *entropy = (u16 *)v;
> +
> +	do {
> +		/*
> +		 * The SEED CSR (0x015) must be accessed with a read-write
> +		 * instruction. Moreover, implementations must ignore the write
> +		 * value, its purpose is to signal polling for new seed.
> +		 */
> +		unsigned long csr_seed = csr_swap(CSR_SEED, 0);
> +
> +		switch (csr_seed & SEED_OPST_MASK) {
> +		case SEED_OPST_ES16:
> +			entropy[valid_seeds++] = csr_seed & SEED_ENTROPY_MASK;
> +			if (valid_seeds == needed_seeds)
> +				return true;
> +			break;
> +
> +		case SEED_OPST_DEAD:
> +			pr_err_once(PR_PREFIX "Unrecoverable error\n");
> +			return false;
> +
> +		case SEED_OPST_BIST:
> +			pr_info(PR_PREFIX "On going Built-in Self Test\n");
> +			fallthrough;
> +
> +		case SEED_OPST_WAIT:
> +		default:
> +			continue;
> +		}
> +
> +	} while (--retry);
> +
> +	return false;
> +}

The Entropy Source specification is annoyingly vague about expected retry
counts, only saying that "Without a polling-style mechanism, the entropy
source could hang for thousands of cycles under some circumstances."

Likewise no constraint is placed on the maximum runtime of a BIST or the
maximum number of times SEED_OPST_BIST is repeatedly returned (only that
it be returned at least once if the BIST starts and finishes between seed
reads).

With that, the limit of 10 reads seems suspiciously small.  Is there a
specific justification or is it known to work on some hardware?

-s

> +
> +static inline size_t __must_check arch_get_random_longs(unsigned long 
> *v, size_t max_longs)
> +{
> +	return 0;
> +}
> +
> +static inline size_t __must_check arch_get_random_seed_longs(unsigned 
> long *v, size_t max_longs)
> +{
> +	return max_longs && riscv_isa_extension_available(NULL, ZKR) && 
> csr_seed_long(v) ? 1 : 0;
> +}
> +
> +#endif /* ASM_RISCV_ARCHRANDOM_H */
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index b98b3b6c9da2..7d0ca9082c66 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -389,6 +389,15 @@
>  #define CSR_VTYPE		0xc21
>  #define CSR_VLENB		0xc22
> 
> +/* Scalar Crypto Extension - Entropy */
> +#define CSR_SEED		0x015
> +#define SEED_OPST_MASK		_AC(0xC0000000, UL)
> +#define SEED_OPST_BIST		_AC(0x00000000, UL)
> +#define SEED_OPST_WAIT		_AC(0x40000000, UL)
> +#define SEED_OPST_ES16		_AC(0x80000000, UL)
> +#define SEED_OPST_DEAD		_AC(0xC0000000, UL)
> +#define SEED_ENTROPY_MASK	_AC(0xFFFF, UL)
> +
>  #ifdef CONFIG_RISCV_M_MODE
>  # define CSR_STATUS	CSR_MSTATUS
>  # define CSR_IE		CSR_MIE
> -- 
> 2.41.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
Samuel Ortiz June 28, 2023, 12:28 p.m. UTC | #3 
Hi Conor,

On Tue, Jun 27, 2023 at 08:09:08PM +0100, Conor Dooley wrote:
> On Tue, Jun 27, 2023 at 04:37:44PM +0200, Samuel Ortiz wrote:
> > The Zkr extension is ratified and provides 16 bits of entropy seed when
> > reading the SEED CSR.
> > 
> > We can implement arch_get_random_seed_longs() by doing multiple csrrw to
> > that CSR and filling an unsigned long with valid entropy bits.
> > 
> > Signed-off-by: Samuel Ortiz <sameo@rivosinc.com>
> > ---
> >  arch/riscv/include/asm/archrandom.h | 66 +++++++++++++++++++++++++++++
> >  arch/riscv/include/asm/csr.h        |  9 ++++
> >  2 files changed, 75 insertions(+)
> >  create mode 100644 arch/riscv/include/asm/archrandom.h
> > 
> > diff --git a/arch/riscv/include/asm/archrandom.h b/arch/riscv/include/asm/archrandom.h
> > new file mode 100644
> > index 000000000000..3d01aab2800a
> > --- /dev/null
> > +++ b/arch/riscv/include/asm/archrandom.h
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Kernel interface for the RISCV arch_random_* functions
> > + *
> > + * Copyright (c) 2022 by Rivos Inc.
> > + *
> > + */
> > +
> > +#ifndef ASM_RISCV_ARCHRANDOM_H
> > +#define ASM_RISCV_ARCHRANDOM_H
> > +
> > +#include <asm/csr.h>
> > +
> > +#define PR_PREFIX "Zkr Extension: "
> 
> Does pr_fmt() not work for you?

It would, but since this is a header that e.g. gets included in
random.h, I would have to ifndef it first.
My v2 is less verbose and gets rid of this prefix anyways.

> Also, "Zkr Extension" doesn't really seem super helpful to a punter if
> they saw it in a log. Why not s/Zkr Extension/archrandom/, or similar?
> 
> > +#define SEED_RETRY_LOOPS 10
> > +
> > +static inline bool __must_check csr_seed_long(unsigned long *v)
> > +{
> > +	unsigned int retry = SEED_RETRY_LOOPS;
> > +	unsigned int needed_seeds = sizeof(unsigned long) / 2, valid_seeds = 0;
> > +	u16 *entropy = (u16 *)v;
> > +
> > +	do {
> > +		/*
> > +		 * The SEED CSR (0x015) must be accessed with a read-write
> > +		 * instruction. Moreover, implementations must ignore the write
> > +		 * value, its purpose is to signal polling for new seed.
> > +		 */
> 
> What relevance does the second half of this comment have to the kernel?

Not much, I will remove it with v2.

> > +		unsigned long csr_seed = csr_swap(CSR_SEED, 0);
> > +
> > +		switch (csr_seed & SEED_OPST_MASK) {
> > +		case SEED_OPST_ES16:
> > +			entropy[valid_seeds++] = csr_seed & SEED_ENTROPY_MASK;
> > +			if (valid_seeds == needed_seeds)
> > +				return true;
> > +			break;
> > +
> > +		case SEED_OPST_DEAD:
> > +			pr_err_once(PR_PREFIX "Unrecoverable error\n");
> > +			return false;
> > +
> > +		case SEED_OPST_BIST:
> > +			pr_info(PR_PREFIX "On going Built-in Self Test\n");
> 
> tiny nit, "On-going"? My OCD is bother by the capitalisation otherwise.

I removed that one with v2, as it may be a little too chatty.

> > +			fallthrough;
> > +
> > +		case SEED_OPST_WAIT:
> > +		default:
> > +			continue;
> > +		}
> > +
> > +	} while (--retry);
> > +
> > +	return false;
> > +}
> > +
> > +static inline size_t __must_check arch_get_random_longs(unsigned long *v, size_t max_longs)
> > +{
> > +	return 0;
> > +}
> > +
> > +static inline size_t __must_check arch_get_random_seed_longs(unsigned long *v, size_t max_longs)
> > +{
> > +	return max_longs && riscv_isa_extension_available(NULL, ZKR) && csr_seed_long(v) ? 1 : 0;
> 
> Could you please write this in a more readable way, even if that is
> going to be a lot more verbose? I know you copied it from x86, but
> that's not really an excuse ;)

Agreed :)
Fixed as well with v2.

> Also, is there a reason that you opted not to use the alternative-backed
> riscv_has_extension_[un]likely() here?

I did not know about them, I switched to it with v2 (Made it a likely
case).

Cheers,
Samuel.
diff mbox series

Patch

diff --git a/arch/riscv/include/asm/archrandom.h b/arch/riscv/include/asm/archrandom.h
new file mode 100644
index 000000000000..3d01aab2800a
--- /dev/null
+++ b/arch/riscv/include/asm/archrandom.h
@@ -0,0 +1,66 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Kernel interface for the RISCV arch_random_* functions
+ *
+ * Copyright (c) 2022 by Rivos Inc.
+ *
+ */
+
+#ifndef ASM_RISCV_ARCHRANDOM_H
+#define ASM_RISCV_ARCHRANDOM_H
+
+#include <asm/csr.h>
+
+#define PR_PREFIX "Zkr Extension: "
+#define SEED_RETRY_LOOPS 10
+
+static inline bool __must_check csr_seed_long(unsigned long *v)
+{
+	unsigned int retry = SEED_RETRY_LOOPS;
+	unsigned int needed_seeds = sizeof(unsigned long) / 2, valid_seeds = 0;
+	u16 *entropy = (u16 *)v;
+
+	do {
+		/*
+		 * The SEED CSR (0x015) must be accessed with a read-write
+		 * instruction. Moreover, implementations must ignore the write
+		 * value, its purpose is to signal polling for new seed.
+		 */
+		unsigned long csr_seed = csr_swap(CSR_SEED, 0);
+
+		switch (csr_seed & SEED_OPST_MASK) {
+		case SEED_OPST_ES16:
+			entropy[valid_seeds++] = csr_seed & SEED_ENTROPY_MASK;
+			if (valid_seeds == needed_seeds)
+				return true;
+			break;
+
+		case SEED_OPST_DEAD:
+			pr_err_once(PR_PREFIX "Unrecoverable error\n");
+			return false;
+
+		case SEED_OPST_BIST:
+			pr_info(PR_PREFIX "On going Built-in Self Test\n");
+			fallthrough;
+
+		case SEED_OPST_WAIT:
+		default:
+			continue;
+		}
+
+	} while (--retry);
+
+	return false;
+}
+
+static inline size_t __must_check arch_get_random_longs(unsigned long *v, size_t max_longs)
+{
+	return 0;
+}
+
+static inline size_t __must_check arch_get_random_seed_longs(unsigned long *v, size_t max_longs)
+{
+	return max_longs && riscv_isa_extension_available(NULL, ZKR) && csr_seed_long(v) ? 1 : 0;
+}
+
+#endif /* ASM_RISCV_ARCHRANDOM_H */
diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index b98b3b6c9da2..7d0ca9082c66 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -389,6 +389,15 @@ 
 #define CSR_VTYPE		0xc21
 #define CSR_VLENB		0xc22
 
+/* Scalar Crypto Extension - Entropy */
+#define CSR_SEED		0x015
+#define SEED_OPST_MASK		_AC(0xC0000000, UL)
+#define SEED_OPST_BIST		_AC(0x00000000, UL)
+#define SEED_OPST_WAIT		_AC(0x40000000, UL)
+#define SEED_OPST_ES16		_AC(0x80000000, UL)
+#define SEED_OPST_DEAD		_AC(0xC0000000, UL)
+#define SEED_ENTROPY_MASK	_AC(0xFFFF, UL)
+
 #ifdef CONFIG_RISCV_M_MODE
 # define CSR_STATUS	CSR_MSTATUS
 # define CSR_IE		CSR_MIE