Re: [PATCH v3 4/4] RISC-V: Implement archrandom when Zkr is available

From: Samuel Ortiz <sameo@rivosinc.com>
To: Heiko Stuebner <heiko.stuebner@vrull.eu>
Cc: "Paul Walmsley" <paul.walmsley@sifive.com>,
	"Palmer Dabbelt" <palmer@dabbelt.com>,
	"Albert Ou" <aou@eecs.berkeley.edu>,
	linux-riscv@lists.infradead.org, linux@rivosinc.com,
	"Conor Dooley" <conor.dooley@microchip.com>,
	"Andrew Jones" <ajones@ventanamicro.com>,
	"Anup Patel" <apatel@ventanamicro.com>,
	linux-kernel@vger.kernel.org,
	"Hongren (Zenithal) Zheng" <i@zenithal.me>,
	"Guo Ren" <guoren@kernel.org>,
	"Atish Patra" <atishp@rivosinc.com>,
	"Björn Töpel" <bjorn@rivosinc.com>,
	"Evan Green" <evan@rivosinc.com>,
	devicetree@vger.kernel.org
Subject: Re: [PATCH v3 4/4] RISC-V: Implement archrandom when Zkr is available
Date: Tue, 11 Jul 2023 19:17:02 +0200	[thread overview]
Message-ID: <ZK2OjvchhqzEKZWR@vermeer> (raw)
In-Reply-To: <3566075.R56niFO833@phil>

Hi Heiko,

On Sun, Jul 09, 2023 at 04:06:16PM +0200, Heiko Stuebner wrote:
> Am Sonntag, 9. Juli 2023, 13:55:46 CEST schrieb Samuel Ortiz:
> > 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.
> > 
> > Acked-by: Conor Dooley <conor.dooley@microchip.com>
> > Signed-off-by: Samuel Ortiz <sameo@rivosinc.com>
> > ---
> 
> > +static inline size_t __must_check arch_get_random_seed_longs(unsigned long *v, size_t max_longs)
> > +{
> > +	if (!max_longs)
> > +		return 0;
> > +
> > +	/*
> > +	 * If Zkr is supported and csr_seed_long succeeds, we return one long
> > +	 * worth of entropy.
> > +	 */
> > +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZKR) && csr_seed_long(v))
> 
> While this whole thing looks really nice, I don't think you can only
> check the ZKR existence though.
> 
> To access the seed csr from supervisor-mode, it looks like the SSEED
> bit in the mseccfg register also needs to be set by firmware.
> And in the kernel we will likely need to check this setting somehow
> before enabling access.

We can't check it as msseccfg is an M-mode only CSR. While reviewing v2
of this patchset, Stephen suggested to either document the SSEED
requirement with the dt-bindings documentation, use the SBI FWFEATURE
extension to ask firmware to set mseecfg properly, or trap seed access
and feed the caller with a virtual entropy source.
I'd like to go with the second proposed approach (FWFEATURE) but that
requires the corresponding pending patch to be merged first. So for now,
I will only document the SSEED requirement when passing the Zkr
extension, so that we at least have a contract definition for firmwares
that enable Zkr through DT. When they do, they're required to at least
set SSEED in MSSECFG.

I have a couple of pending patches ([1],[2]) related to that, so that an
OpenSBI+qemu+linux combination works as expected when enabling Zkr. I am
going to submit them upstream as well.

Cheers,
Samuel.

[1] https://github.com/qemu/qemu/commit/2a146057099ada946bf4a9c2e355a5a290c23c80
[2] https://github.com/riscv-software-src/opensbi/pull/315

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv