Dectect the Zkr extension and use it to seed the kernel base address.
Detection of the extension can not be done in the typical fashion, as
this is very early in the boot process. Instead, add a trap handler
and run it to see if the extension is present.
Signed-off-by: Jesse Taube <[email protected]>
---
arch/riscv/kernel/pi/Makefile | 2 +-
arch/riscv/kernel/pi/archrandom_early.c | 71 +++++++++++++++++++++++++
arch/riscv/mm/init.c | 3 ++
3 files changed, 75 insertions(+), 1 deletion(-)
create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
index 50bc5ef7dd2f..9025eb52945a 100644
--- a/arch/riscv/kernel/pi/Makefile
+++ b/arch/riscv/kernel/pi/Makefile
@@ -32,5 +32,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
$(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
$(call if_changed_rule,cc_o_c)
-obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
+obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
extra-y := $(patsubst %.pi.o,%.o,$(obj-y))
diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
new file mode 100644
index 000000000000..311be9388b5c
--- /dev/null
+++ b/arch/riscv/kernel/pi/archrandom_early.c
@@ -0,0 +1,71 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+/*
+ * To avoid rewriteing code include asm/archrandom.h and create macros
+ * for the functions that won't be included.
+ */
+
+#define riscv_has_extension_likely(...) false
+#define pr_err_once(...)
+
+#include <linux/types.h>
+#include <asm/hwcap.h>
+#include <asm/archrandom.h>
+
+/*
+ * Asm goto is needed so that the compiler does not remove the label.
+ */
+
+#define csr_goto_swap(csr, val) \
+({ \
+ unsigned long __v; \
+ __asm__ __volatile__ goto("csrrw %0, " __ASM_STR(csr) ", %1" \
+ : "=r" (__v) : "rK" (&&val) \
+ : "memory" : val); \
+ __v; \
+})
+
+/*
+ * Declare the functions that are exported (but prefixed) here so that LLVM
+ * does not complain it lacks the 'static' keyword (which, if added, makes
+ * LLVM complain because the function is actually unused in this file).
+ */
+
+u64 get_kaslr_seed_zkr(void);
+
+/*
+ * This function is called by setup_vm to check if the kernel has the ZKR.
+ * Traps haven't been set up yet, but save and restore the TVEC to avoid
+ * any side effects.
+ */
+
+static inline bool __must_check riscv_has_zkr(void)
+{
+ unsigned long tvec;
+
+ tvec = csr_goto_swap(CSR_TVEC, not_zkr);
+ csr_swap(CSR_SEED, 0);
+ csr_write(CSR_TVEC, tvec);
+ return true;
+not_zkr:
+ csr_write(CSR_TVEC, tvec);
+ return false;
+}
+
+u64 get_kaslr_seed_zkr(void)
+{
+ const int needed_seeds = sizeof(u64) / sizeof(long);
+ int i = 0;
+ u64 seed = 0;
+ long *entropy = (long *)(&seed);
+
+ if (!riscv_has_zkr())
+ return 0;
+
+ for (i = 0; i < needed_seeds; i++) {
+ if (!csr_seed_long(&entropy[i]))
+ return 0;
+ }
+
+ return seed;
+}
diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
index 9940171c79f0..8ef1edd2cddd 100644
--- a/arch/riscv/mm/init.c
+++ b/arch/riscv/mm/init.c
@@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
#ifdef CONFIG_RANDOMIZE_BASE
extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
+extern u64 __init __pi_get_kaslr_seed_zkr(void);
static int __init print_nokaslr(char *p)
{
@@ -1049,6 +1050,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
u32 nr_pos;
+ if (kaslr_seed == 0)
+ kaslr_seed = __pi_get_kaslr_seed_zkr();
/*
* Compute the number of positions available: we are limited
* by the early page table that only has one PUD and we must
--
2.43.0
On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> Dectect the Zkr extension and use it to seed the kernel base address.
>
> Detection of the extension can not be done in the typical fashion, as
> this is very early in the boot process. Instead, add a trap handler
> and run it to see if the extension is present.
You can't rely on the lack of a trap meaning that Zkr is present unless
you know that the platform implements Ssstrict. The CSR with that number
could do anything if not Ssstrict compliant, so this approach gets a
nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
it, so you're stuck with getting that information from firmware.
For DT systems, you can actually parse the DT in the pi, we do it to get
the kaslr seed if present, so you can actually check for Zkr. With ACPI
I have no idea how you can get that information, I amn't an ACPI-ist.
Thanks,
Conor.
Hi Jesse,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc1 next-20240531]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jesse-Taube/RISC-V-Use-Zkr-to-seed-KASLR-base-address/20240601-002545
base: linus/master
patch link: https://lore.kernel.org/r/20240531162327.2436962-1-jesse%40rivosinc.com
patch subject: [PATCH v0] RISC-V: Use Zkr to seed KASLR base address
config: riscv-allnoconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All error/warnings (new ones prefixed by >>):
riscv64-linux-ld: arch/riscv/kernel/pi/archrandom_early.pi.o: in function `__pi_.L0 ':
>> __pi_archrandom_early.c:(.init.pi.text+0x90): undefined reference to `__pi__printk'
--
In file included from include/linux/kernel.h:31,
from include/linux/cpumask.h:11,
from arch/riscv/include/asm/processor.h:71,
from arch/riscv/include/asm/archrandom.h:13,
from arch/riscv/kernel/pi/archrandom_early.c:13:
>> include/linux/printk.h:619: warning: "pr_err_once" redefined
619 | #define pr_err_once(fmt, ...) \
|
arch/riscv/kernel/pi/archrandom_early.c:9: note: this is the location of the previous definition
9 | #define pr_err_once(...)
|
vim +/pr_err_once +619 include/linux/printk.h
16cb839f133249 Joe Perches 2011-01-12 612
16cb839f133249 Joe Perches 2011-01-12 613 #define pr_emerg_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 614 printk_once(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 615 #define pr_alert_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 616 printk_once(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 617 #define pr_crit_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 618 printk_once(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 @619 #define pr_err_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 620 printk_once(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 621 #define pr_warn_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 622 printk_once(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 623 #define pr_notice_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 624 printk_once(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
16cb839f133249 Joe Perches 2011-01-12 625 #define pr_info_once(fmt, ...) \
16cb839f133249 Joe Perches 2011-01-12 626 printk_once(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
eb012d125a2419 Tetsuo Handa 2020-05-25 627 /* no pr_cont_once, don't do that... */
36d308d8b547ee Mikhail Gruzdev 2013-02-21 628
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Jesse,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linus/master]
[also build test WARNING on v6.10-rc1 next-20240531]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jesse-Taube/RISC-V-Use-Zkr-to-seed-KASLR-base-address/20240601-002545
base: linus/master
patch link: https://lore.kernel.org/r/20240531162327.2436962-1-jesse%40rivosinc.com
patch subject: [PATCH v0] RISC-V: Use Zkr to seed KASLR base address
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
In file included from arch/riscv/kernel/pi/archrandom_early.c:13:
In file included from arch/riscv/include/asm/archrandom.h:13:
In file included from arch/riscv/include/asm/processor.h:71:
In file included from include/linux/cpumask.h:11:
In file included from include/linux/kernel.h:31:
>> include/linux/printk.h:619:9: warning: 'pr_err_once' macro redefined [-Wmacro-redefined]
619 | #define pr_err_once(fmt, ...) \
| ^
arch/riscv/kernel/pi/archrandom_early.c:9:9: note: previous definition is here
9 | #define pr_err_once(...)
| ^
>> arch/riscv/kernel/pi/archrandom_early.c:51:22: warning: variable 'tvec' is uninitialized when used here [-Wuninitialized]
51 | csr_write(CSR_TVEC, tvec);
| ^~~~
arch/riscv/include/asm/csr.h:506:38: note: expanded from macro 'csr_write'
506 | unsigned long __v = (unsigned long)(val); \
| ^~~
arch/riscv/kernel/pi/archrandom_early.c:44:20: note: initialize the variable 'tvec' to silence this warning
44 | unsigned long tvec;
| ^
| = 0
2 warnings generated.
vim +/pr_err_once +619 include/linux/printk.h
16cb839f1332497 Joe Perches 2011-01-12 612
16cb839f1332497 Joe Perches 2011-01-12 613 #define pr_emerg_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 614 printk_once(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 615 #define pr_alert_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 616 printk_once(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 617 #define pr_crit_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 618 printk_once(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 @619 #define pr_err_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 620 printk_once(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 621 #define pr_warn_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 622 printk_once(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 623 #define pr_notice_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 624 printk_once(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
16cb839f1332497 Joe Perches 2011-01-12 625 #define pr_info_once(fmt, ...) \
16cb839f1332497 Joe Perches 2011-01-12 626 printk_once(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
eb012d125a24197 Tetsuo Handa 2020-05-25 627 /* no pr_cont_once, don't do that... */
36d308d8b547ee1 Mikhail Gruzdev 2013-02-21 628
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, May 31, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > Dectect the Zkr extension and use it to seed the kernel base address.
> >
> > Detection of the extension can not be done in the typical fashion, as
> > this is very early in the boot process. Instead, add a trap handler
> > and run it to see if the extension is present.
>
> You can't rely on the lack of a trap meaning that Zkr is present unless
> you know that the platform implements Ssstrict. The CSR with that number
> could do anything if not Ssstrict compliant, so this approach gets a
> nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> it, so you're stuck with getting that information from firmware.
The Scalar Cryptography v1.0.1 spec says "If Zkr is not implemented, the
[s,u]seed bits must be hardwired to zero". It also says "Without the
corresponding access control bit set to 1, any attempted access to seed
from U, S, or HS modes will raise an illegal instruction exception."
There is a slight nuance here as the definition of Ssstrict is:
"No non-conforming extensions are present. Attempts to execute
unimplemented opcodes or access unimplemented CSRs in the standard or
reserved encoding spaces raises an illegal instruction exception that
results in a contained trap to the supervisor-mode trap handler."
The trap that Jesse is relying on in the code here is related to access
bits and not related to the CSR being unimplemented. Since the access
bits are required to be 0 on an implementation without Zkr, it is
required to trap if seed is accessed, regardless of Ssstrict.
The situation here is slightly odd because the spec is defining behavior
for what to do if the extension is not supported, and requires
implementations to follow this aspect of the Scalar Cryptography spec
even though they may not implement any of the instructions in the spec.
>
> For DT systems, you can actually parse the DT in the pi, we do it to get
> the kaslr seed if present, so you can actually check for Zkr. With ACPI
> I have no idea how you can get that information, I amn't an ACPI-ist.
It is feasible to check if Zkr is present in the device tree at this
stage in boot, but at first glance does not seem feasible to read the
ACPI tables this early.
The CSR being read is just for entropy so even if the seed CSR doesn't
trap and provides an arbitrary value on an implementation that does not
support Zkr, it can still be used as a source of entropy. If the
implementation does trap, the entropy will be set to 0 which is just a
different hard-coded arbitrary value.
- Charlie
>
> Thanks,
> Conor.
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Fri, May 31, 2024 at 10:36:46PM +0100, Conor Dooley wrote:
> On Fri, May 31, 2024 at 01:19:14PM -0700, Charlie Jenkins wrote:
> > On Fri, May 31, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> > > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > > > Dectect the Zkr extension and use it to seed the kernel base address.
> > > >
> > > > Detection of the extension can not be done in the typical fashion, as
> > > > this is very early in the boot process. Instead, add a trap handler
> > > > and run it to see if the extension is present.
> > >
> > > You can't rely on the lack of a trap meaning that Zkr is present unless
> > > you know that the platform implements Ssstrict. The CSR with that number
> > > could do anything if not Ssstrict compliant, so this approach gets a
> > > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> > > it, so you're stuck with getting that information from firmware.
> >
> > The Scalar Cryptography v1.0.1 spec says "If Zkr is not implemented, the
> > [s,u]seed bits must be hardwired to zero". It also says "Without the
> > corresponding access control bit set to 1, any attempted access to seed
> > from U, S, or HS modes will raise an illegal instruction exception."
> >
> > There is a slight nuance here as the definition of Ssstrict is:
> >
> > "No non-conforming extensions are present. Attempts to execute
> > unimplemented opcodes or access unimplemented CSRs in the standard or
> > reserved encoding spaces raises an illegal instruction exception that
> > results in a contained trap to the supervisor-mode trap handler."
> >
> > The trap that Jesse is relying on in the code here is related to access
> > bits and not related to the CSR being unimplemented. Since the access
> > bits are required to be 0 on an implementation without Zkr, it is
> > required to trap if seed is accessed, regardless of Ssstrict.
> >
> > The situation here is slightly odd because the spec is defining behavior
> > for what to do if the extension is not supported, and requires
> > implementations to follow this aspect of the Scalar Cryptography spec
> > even though they may not implement any of the instructions in the spec.
>
> Firstly, you absolutely cannot rely on the behaviour defined by a new
> extension by systems that implement a version of the ISA that predates
> it. Secondly, we're talking about non-conforming implementations that
> use a reserved CSR number for other purposes, you cannot rely on the
> behaviour that the Scalar Crypto spec prescribes for access to the
> register.
Yes that is definitely a slippery slope.
>
> "Non-conforming" is also a moving target btw - the Andes PMU (I think
> it's that) uses an interrupt number that was moved from "platform
> specific use" to "reserved" by the AIA spec. If you only looked the
> current specs, the Andes PMU is a "non-conforming extension" but at the
> time that it was created it was compliant. I think we're gonna have a
> fun conversation defining what "Ssstrict" actually means when someone
> actually tries to document that.
Sounds fun ;)
>
> > > For DT systems, you can actually parse the DT in the pi, we do it to get
> > > the kaslr seed if present, so you can actually check for Zkr. With ACPI
> > > I have no idea how you can get that information, I amn't an ACPI-ist.
> >
> > It is feasible to check if Zkr is present in the device tree at this
> > stage in boot, but at first glance does not seem feasible to read the
> > ACPI tables this early.
> >
> > The CSR being read is just for entropy so even if the seed CSR doesn't
> > trap and provides an arbitrary value on an implementation that does not
> > support Zkr, it can still be used as a source of entropy. If the
> > implementation does trap, the entropy will be set to 0 which is just a
> > different hard-coded arbitrary value.
>
> Right. I can see value in doing something that may contain entropy, and
> is at worst no better than the 0 we can currently get. But the patch
> we're talking about here mentions nothing of the sort, it presents itself
> as detection of Zkr and an actually random number - but all it actually
> detects is whether or not the CSR at CSR_SEED traps.
>
> To be acceptable, the patch would need to stop claiming that it is a valid
> way to detect Zkr. The commit message, and a comment, must also explain
> what may happen on systems that do not implement Zkr as you have done
> here.
> For example, `if (!riscv_has_zkr()) return 0` would have to be something
> like `if (riscv_csr_seed_traps()) return 0`.
That is reasonable, thank you for your input!
- Charlie
>
> Thanks,
> Conor.
On Fri, May 31, 2024 at 01:19:14PM -0700, Charlie Jenkins wrote:
> On Fri, May 31, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > > Dectect the Zkr extension and use it to seed the kernel base address.
> > >
> > > Detection of the extension can not be done in the typical fashion, as
> > > this is very early in the boot process. Instead, add a trap handler
> > > and run it to see if the extension is present.
> >
> > You can't rely on the lack of a trap meaning that Zkr is present unless
> > you know that the platform implements Ssstrict. The CSR with that number
> > could do anything if not Ssstrict compliant, so this approach gets a
> > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> > it, so you're stuck with getting that information from firmware.
>
> The Scalar Cryptography v1.0.1 spec says "If Zkr is not implemented, the
> [s,u]seed bits must be hardwired to zero". It also says "Without the
> corresponding access control bit set to 1, any attempted access to seed
> from U, S, or HS modes will raise an illegal instruction exception."
>
> There is a slight nuance here as the definition of Ssstrict is:
>
> "No non-conforming extensions are present. Attempts to execute
> unimplemented opcodes or access unimplemented CSRs in the standard or
> reserved encoding spaces raises an illegal instruction exception that
> results in a contained trap to the supervisor-mode trap handler."
>
> The trap that Jesse is relying on in the code here is related to access
> bits and not related to the CSR being unimplemented. Since the access
> bits are required to be 0 on an implementation without Zkr, it is
> required to trap if seed is accessed, regardless of Ssstrict.
>
> The situation here is slightly odd because the spec is defining behavior
> for what to do if the extension is not supported, and requires
> implementations to follow this aspect of the Scalar Cryptography spec
> even though they may not implement any of the instructions in the spec.
Firstly, you absolutely cannot rely on the behaviour defined by a new
extension by systems that implement a version of the ISA that predates
it. Secondly, we're talking about non-conforming implementations that
use a reserved CSR number for other purposes, you cannot rely on the
behaviour that the Scalar Crypto spec prescribes for access to the
register.
"Non-conforming" is also a moving target btw - the Andes PMU (I think
it's that) uses an interrupt number that was moved from "platform
specific use" to "reserved" by the AIA spec. If you only looked the
current specs, the Andes PMU is a "non-conforming extension" but at the
time that it was created it was compliant. I think we're gonna have a
fun conversation defining what "Ssstrict" actually means when someone
actually tries to document that.
> > For DT systems, you can actually parse the DT in the pi, we do it to get
> > the kaslr seed if present, so you can actually check for Zkr. With ACPI
> > I have no idea how you can get that information, I amn't an ACPI-ist.
>
> It is feasible to check if Zkr is present in the device tree at this
> stage in boot, but at first glance does not seem feasible to read the
> ACPI tables this early.
>
> The CSR being read is just for entropy so even if the seed CSR doesn't
> trap and provides an arbitrary value on an implementation that does not
> support Zkr, it can still be used as a source of entropy. If the
> implementation does trap, the entropy will be set to 0 which is just a
> different hard-coded arbitrary value.
Right. I can see value in doing something that may contain entropy, and
is at worst no better than the 0 we can currently get. But the patch
we're talking about here mentions nothing of the sort, it presents itself
as detection of Zkr and an actually random number - but all it actually
detects is whether or not the CSR at CSR_SEED traps.
To be acceptable, the patch would need to stop claiming that it is a valid
way to detect Zkr. The commit message, and a comment, must also explain
what may happen on systems that do not implement Zkr as you have done
here.
For example, `if (!riscv_has_zkr()) return 0` would have to be something
like `if (riscv_csr_seed_traps()) return 0`.
Thanks,
Conor.
Hi Jesse,
kernel test robot noticed the following build errors:
[auto build test ERROR on linus/master]
[also build test ERROR on v6.10-rc1 next-20240531]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jesse-Taube/RISC-V-Use-Zkr-to-seed-KASLR-base-address/20240601-002545
base: linus/master
patch link: https://lore.kernel.org/r/20240531162327.2436962-1-jesse%40rivosinc.com
patch subject: [PATCH v0] RISC-V: Use Zkr to seed KASLR base address
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240601/[email protected]/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project bafda89a0944d947fc4b3b5663185e07a397ac30)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240601/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All errors (new ones prefixed by >>):
>> ld.lld: error: undefined symbol: __pi__printk
>>> referenced by __pi_archrandom_early.c
>>> arch/riscv/kernel/pi/archrandom_early.pi.o:(__pi_get_kaslr_seed_zkr) in archive vmlinux.a
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
On Fri, May 31, 2024 at 02:40:05PM -0700, Charlie Jenkins wrote:
> On Fri, May 31, 2024 at 10:36:46PM +0100, Conor Dooley wrote:
> > On Fri, May 31, 2024 at 01:19:14PM -0700, Charlie Jenkins wrote:
> > > On Fri, May 31, 2024 at 06:31:09PM +0100, Conor Dooley wrote:
> > > > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > > > > Dectect the Zkr extension and use it to seed the kernel base address.
> > > > >
> > > > > Detection of the extension can not be done in the typical fashion, as
> > > > > this is very early in the boot process. Instead, add a trap handler
> > > > > and run it to see if the extension is present.
> > > >
> > > > You can't rely on the lack of a trap meaning that Zkr is present unless
> > > > you know that the platform implements Ssstrict. The CSR with that number
> > > > could do anything if not Ssstrict compliant, so this approach gets a
> > > > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> > > > it, so you're stuck with getting that information from firmware.
> > >
> > > The Scalar Cryptography v1.0.1 spec says "If Zkr is not implemented, the
> > > [s,u]seed bits must be hardwired to zero". It also says "Without the
> > > corresponding access control bit set to 1, any attempted access to seed
> > > from U, S, or HS modes will raise an illegal instruction exception."
> > >
> > > There is a slight nuance here as the definition of Ssstrict is:
> > >
> > > "No non-conforming extensions are present. Attempts to execute
> > > unimplemented opcodes or access unimplemented CSRs in the standard or
> > > reserved encoding spaces raises an illegal instruction exception that
> > > results in a contained trap to the supervisor-mode trap handler."
> > >
> > > The trap that Jesse is relying on in the code here is related to access
> > > bits and not related to the CSR being unimplemented. Since the access
> > > bits are required to be 0 on an implementation without Zkr, it is
> > > required to trap if seed is accessed, regardless of Ssstrict.
> > >
> > > The situation here is slightly odd because the spec is defining behavior
> > > for what to do if the extension is not supported, and requires
> > > implementations to follow this aspect of the Scalar Cryptography spec
> > > even though they may not implement any of the instructions in the spec.
> >
> > Firstly, you absolutely cannot rely on the behaviour defined by a new
> > extension by systems that implement a version of the ISA that predates
> > it. Secondly, we're talking about non-conforming implementations that
> > use a reserved CSR number for other purposes, you cannot rely on the
> > behaviour that the Scalar Crypto spec prescribes for access to the
> > register.
>
> Yes that is definitely a slippery slope.
>
> >
> > "Non-conforming" is also a moving target btw - the Andes PMU (I think
> > it's that) uses an interrupt number that was moved from "platform
> > specific use" to "reserved" by the AIA spec. If you only looked the
> > current specs, the Andes PMU is a "non-conforming extension" but at the
> > time that it was created it was compliant. I think we're gonna have a
> > fun conversation defining what "Ssstrict" actually means when someone
> > actually tries to document that.
>
> Sounds fun ;)
>
> >
> > > > For DT systems, you can actually parse the DT in the pi, we do it to get
> > > > the kaslr seed if present, so you can actually check for Zkr. With ACPI
> > > > I have no idea how you can get that information, I amn't an ACPI-ist.
> > >
> > > It is feasible to check if Zkr is present in the device tree at this
> > > stage in boot, but at first glance does not seem feasible to read the
> > > ACPI tables this early.
> > >
> > > The CSR being read is just for entropy so even if the seed CSR doesn't
> > > trap and provides an arbitrary value on an implementation that does not
> > > support Zkr, it can still be used as a source of entropy. If the
> > > implementation does trap, the entropy will be set to 0 which is just a
> > > different hard-coded arbitrary value.
> >
> > Right. I can see value in doing something that may contain entropy, and
> > is at worst no better than the 0 we can currently get. But the patch
> > we're talking about here mentions nothing of the sort, it presents itself
> > as detection of Zkr and an actually random number - but all it actually
> > detects is whether or not the CSR at CSR_SEED traps.
> >
> > To be acceptable, the patch would need to stop claiming that it is a valid
> > way to detect Zkr. The commit message, and a comment, must also explain
> > what may happen on systems that do not implement Zkr as you have done
> > here.
> > For example, `if (!riscv_has_zkr()) return 0` would have to be something
> > like `if (riscv_csr_seed_traps()) return 0`.
>
> That is reasonable, thank you for your input!
Another thing to consider is that writing a zero to CSR_SEED may have
side effects on a non-conforming implementation.
Hi Conor,
On 31/05/2024 19:31, Conor Dooley wrote:
> On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
>> Dectect the Zkr extension and use it to seed the kernel base address.
>>
>> Detection of the extension can not be done in the typical fashion, as
>> this is very early in the boot process. Instead, add a trap handler
>> and run it to see if the extension is present.
> You can't rely on the lack of a trap meaning that Zkr is present unless
> you know that the platform implements Ssstrict. The CSR with that number
> could do anything if not Ssstrict compliant, so this approach gets a
> nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> it, so you're stuck with getting that information from firmware.
FYI, this patch is my idea, so I'm the one to blame here :)
>
> For DT systems, you can actually parse the DT in the pi, we do it to get
> the kaslr seed if present, so you can actually check for Zkr. With ACPI
> I have no idea how you can get that information, I amn't an ACPI-ist.
I took a look at how to access ACPI tables this early when implementing
the Zabha/Zacas patches, but it seems not possible.
But I'll look into this more, this is not the first time we need the
extensions list very early and since we have no way to detect the
presence of an extension at runtime, something needs to be done.
Thanks,
Alex
>
> Thanks,
> Conor.
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
> Hi Conor,
>
> On 31/05/2024 19:31, Conor Dooley wrote:
> > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> > > Dectect the Zkr extension and use it to seed the kernel base address.
> > >
> > > Detection of the extension can not be done in the typical fashion, as
> > > this is very early in the boot process. Instead, add a trap handler
> > > and run it to see if the extension is present.
> > You can't rely on the lack of a trap meaning that Zkr is present unless
> > you know that the platform implements Ssstrict. The CSR with that number
> > could do anything if not Ssstrict compliant, so this approach gets a
> > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> > it, so you're stuck with getting that information from firmware.
>
>
> FYI, this patch is my idea, so I'm the one to blame here :)
>
>
> >
> > For DT systems, you can actually parse the DT in the pi, we do it to get
> > the kaslr seed if present, so you can actually check for Zkr. With ACPI
> > I have no idea how you can get that information, I amn't an ACPI-ist.
>
>
> I took a look at how to access ACPI tables this early when implementing the
> Zabha/Zacas patches, but it seems not possible.
>
> But I'll look into this more, this is not the first time we need the
> extensions list very early and since we have no way to detect the presence
> of an extension at runtime, something needs to be done.
Aye, having remembered that reading CSR_SEED could have side-effects on a
system with non-conforming extensions, it'd be good to see if we can
actually do this via detection on ACPI - especially for some other
extensions that we may need to turn on very early (I forget which ones we
talked about this before for). I didn't arm64 do anything with ACPI in the
pi code, is the code arch/x86/boot/compressed run at an equivilent-ish point
in boot?
On Sat, Jun 1, 2024 at 12:23 AM Jesse Taube <[email protected]> wrote:
>
> Dectect the Zkr extension and use it to seed the kernel base address.
>
> Detection of the extension can not be done in the typical fashion, as
> this is very early in the boot process. Instead, add a trap handler
> and run it to see if the extension is present.
>
> Signed-off-by: Jesse Taube <[email protected]>
> ---
> arch/riscv/kernel/pi/Makefile | 2 +-
> arch/riscv/kernel/pi/archrandom_early.c | 71 +++++++++++++++++++++++++
> arch/riscv/mm/init.c | 3 ++
> 3 files changed, 75 insertions(+), 1 deletion(-)
> create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
>
> diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
> index 50bc5ef7dd2f..9025eb52945a 100644
> --- a/arch/riscv/kernel/pi/Makefile
> +++ b/arch/riscv/kernel/pi/Makefile
> @@ -32,5 +32,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
> $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
> $(call if_changed_rule,cc_o_c)
>
> -obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
> +obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
> extra-y := $(patsubst %.pi.o,%.o,$(obj-y))
> diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
> new file mode 100644
> index 000000000000..311be9388b5c
> --- /dev/null
> +++ b/arch/riscv/kernel/pi/archrandom_early.c
> @@ -0,0 +1,71 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +/*
> + * To avoid rewriteing code include asm/archrandom.h and create macros
> + * for the functions that won't be included.
> + */
> +
> +#define riscv_has_extension_likely(...) false
> +#define pr_err_once(...)
> +
> +#include <linux/types.h>
> +#include <asm/hwcap.h>
> +#include <asm/archrandom.h>
> +
> +/*
> + * Asm goto is needed so that the compiler does not remove the label.
> + */
> +
> +#define csr_goto_swap(csr, val) \
> +({ \
> + unsigned long __v; \
> + __asm__ __volatile__ goto("csrrw %0, " __ASM_STR(csr) ", %1" \
> + : "=r" (__v) : "rK" (&&val) \
> + : "memory" : val); \
> + __v; \
> +})
> +
> +/*
> + * Declare the functions that are exported (but prefixed) here so that LLVM
> + * does not complain it lacks the 'static' keyword (which, if added, makes
> + * LLVM complain because the function is actually unused in this file).
> + */
> +
> +u64 get_kaslr_seed_zkr(void);
> +
> +/*
> + * This function is called by setup_vm to check if the kernel has the ZKR.
> + * Traps haven't been set up yet, but save and restore the TVEC to avoid
> + * any side effects.
> + */
> +
> +static inline bool __must_check riscv_has_zkr(void)
> +{
> + unsigned long tvec;
> +
> + tvec = csr_goto_swap(CSR_TVEC, not_zkr);
> + csr_swap(CSR_SEED, 0);
> + csr_write(CSR_TVEC, tvec);
> + return true;
> +not_zkr:
> + csr_write(CSR_TVEC, tvec);
> + return false;
> +}
> +
> +u64 get_kaslr_seed_zkr(void)
> +{
> + const int needed_seeds = sizeof(u64) / sizeof(long);
> + int i = 0;
> + u64 seed = 0;
> + long *entropy = (long *)(&seed);
> +
> + if (!riscv_has_zkr())
> + return 0;
> +
> + for (i = 0; i < needed_seeds; i++) {
> + if (!csr_seed_long(&entropy[i]))
> + return 0;
> + }
> +
> + return seed;
> +}
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 9940171c79f0..8ef1edd2cddd 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
> #ifdef CONFIG_RANDOMIZE_BASE
> extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
> extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
> +extern u64 __init __pi_get_kaslr_seed_zkr(void);
>
> static int __init print_nokaslr(char *p)
> {
> @@ -1049,6 +1050,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
> u32 nr_pos;
>
> + if (kaslr_seed == 0)
> + kaslr_seed = __pi_get_kaslr_seed_zkr();
Could you elaborate on why we try to get the seed from DT first,
rather than from ZKR? Also, I was wondering if, by any chance, it can
leverage arch_get_random_seed_longs() to get the seed instead of
__pi_get_kasler_seed_zkr()? Thanks
> /*
> * Compute the number of positions available: we are limited
> * by the early page table that only has one PUD and we must
> --
> 2.43.0
>
>
> _______________________________________________
> linux-riscv mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On 6/5/24 00:51, Zong Li wrote:
> On Sat, Jun 1, 2024 at 12:23 AM Jesse Taube <[email protected]> wrote:
>>
>> Dectect the Zkr extension and use it to seed the kernel base address.
>>
>> Detection of the extension can not be done in the typical fashion, as
>> this is very early in the boot process. Instead, add a trap handler
>> and run it to see if the extension is present.
>>
>> Signed-off-by: Jesse Taube <[email protected]>
>> ---
>> arch/riscv/kernel/pi/Makefile | 2 +-
>> arch/riscv/kernel/pi/archrandom_early.c | 71 +++++++++++++++++++++++++
>> arch/riscv/mm/init.c | 3 ++
>> 3 files changed, 75 insertions(+), 1 deletion(-)
>> create mode 100644 arch/riscv/kernel/pi/archrandom_early.c
>>
>> diff --git a/arch/riscv/kernel/pi/Makefile b/arch/riscv/kernel/pi/Makefile
>> index 50bc5ef7dd2f..9025eb52945a 100644
>> --- a/arch/riscv/kernel/pi/Makefile
>> +++ b/arch/riscv/kernel/pi/Makefile
>> @@ -32,5 +32,5 @@ $(obj)/string.o: $(srctree)/lib/string.c FORCE
>> $(obj)/ctype.o: $(srctree)/lib/ctype.c FORCE
>> $(call if_changed_rule,cc_o_c)
>>
>> -obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o
>> +obj-y := cmdline_early.pi.o fdt_early.pi.o string.pi.o ctype.pi.o lib-fdt.pi.o lib-fdt_ro.pi.o archrandom_early.pi.o
>> extra-y := $(patsubst %.pi.o,%.o,$(obj-y))
>> diff --git a/arch/riscv/kernel/pi/archrandom_early.c b/arch/riscv/kernel/pi/archrandom_early.c
>> new file mode 100644
>> index 000000000000..311be9388b5c
>> --- /dev/null
>> +++ b/arch/riscv/kernel/pi/archrandom_early.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
>> +/*
>> + * To avoid rewriteing code include asm/archrandom.h and create macros
>> + * for the functions that won't be included.
>> + */
>> +
>> +#define riscv_has_extension_likely(...) false
>> +#define pr_err_once(...)
>> +
>> +#include <linux/types.h>
>> +#include <asm/hwcap.h>
>> +#include <asm/archrandom.h>
>> +
>> +/*
>> + * Asm goto is needed so that the compiler does not remove the label.
>> + */
>> +
>> +#define csr_goto_swap(csr, val) \
>> +({ \
>> + unsigned long __v; \
>> + __asm__ __volatile__ goto("csrrw %0, " __ASM_STR(csr) ", %1" \
>> + : "=r" (__v) : "rK" (&&val) \
>> + : "memory" : val); \
>> + __v; \
>> +})
>> +
>> +/*
>> + * Declare the functions that are exported (but prefixed) here so that LLVM
>> + * does not complain it lacks the 'static' keyword (which, if added, makes
>> + * LLVM complain because the function is actually unused in this file).
>> + */
>> +
>> +u64 get_kaslr_seed_zkr(void);
>> +
>> +/*
>> + * This function is called by setup_vm to check if the kernel has the ZKR.
>> + * Traps haven't been set up yet, but save and restore the TVEC to avoid
>> + * any side effects.
>> + */
>> +
>> +static inline bool __must_check riscv_has_zkr(void)
>> +{
>> + unsigned long tvec;
>> +
>> + tvec = csr_goto_swap(CSR_TVEC, not_zkr);
>> + csr_swap(CSR_SEED, 0);
>> + csr_write(CSR_TVEC, tvec);
>> + return true;
>> +not_zkr:
>> + csr_write(CSR_TVEC, tvec);
>> + return false;
>> +}
>> +
>> +u64 get_kaslr_seed_zkr(void)
>> +{
>> + const int needed_seeds = sizeof(u64) / sizeof(long);
>> + int i = 0;
>> + u64 seed = 0;
>> + long *entropy = (long *)(&seed);
>> +
>> + if (!riscv_has_zkr())
>> + return 0;
>> +
>> + for (i = 0; i < needed_seeds; i++) {
>> + if (!csr_seed_long(&entropy[i]))
>> + return 0;
>> + }
>> +
>> + return seed;
>> +}
>> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
>> index 9940171c79f0..8ef1edd2cddd 100644
>> --- a/arch/riscv/mm/init.c
>> +++ b/arch/riscv/mm/init.c
>> @@ -1025,6 +1025,7 @@ static void __init pt_ops_set_late(void)
>> #ifdef CONFIG_RANDOMIZE_BASE
>> extern bool __init __pi_set_nokaslr_from_cmdline(uintptr_t dtb_pa);
>> extern u64 __init __pi_get_kaslr_seed(uintptr_t dtb_pa);
>> +extern u64 __init __pi_get_kaslr_seed_zkr(void);
>>
>> static int __init print_nokaslr(char *p)
>> {
>> @@ -1049,6 +1050,8 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
>> u32 kernel_size = (uintptr_t)(&_end) - (uintptr_t)(&_start);
>> u32 nr_pos;
>>
>> + if (kaslr_seed == 0)
>> + kaslr_seed = __pi_get_kaslr_seed_zkr();
>
> Could you elaborate on why we try to get the seed from DT first,
> rather than from ZKR?
The ordering preference doesn't matter to much,
but my thought was if someone wanted to set a seed they could override
the Zkr provided seed. Alexandre said privately to use the DT as a
fallback rather than the way we have it now.
> Also, I was wondering if, by any chance, it can
> leverage arch_get_random_seed_longs() to get the seed instead of
> __pi_get_kasler_seed_zkr()? Thanks
arch_get_random_seed_longs() is almost equivalent to
__pi_get_kasler_seed_zkr()
in the sense they do they will both use Zkr for entropy
if its accessible.
The problem is that we are very early in the boot process so the check
arch_get_random_seed_longs uses to detect if Zkr is present
(__riscv_isa_extension_available) is not set up yet. This is why
riscv_has_zkr is used to check if Zkr is present. Unfortunately, which I
didn't realize is that how riscv_has_zkr works isn't a valid way to
check if Zkr is present.
The only solution seems to be parsing the ISA string from DT and ACPI.
The hard part is parsing ACPI this early in boot.
Thanks,
Jesse Taube
>
>> /*
>> * Compute the number of positions available: we are limited
>> * by the early page table that only has one PUD and we must
>> --
>> 2.43.0
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> [email protected]
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
>> Hi Conor,
>>
>> On 31/05/2024 19:31, Conor Dooley wrote:
>> > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
>> > > Dectect the Zkr extension and use it to seed the kernel base address.
>> > >
>> > > Detection of the extension can not be done in the typical fashion, as
>> > > this is very early in the boot process. Instead, add a trap handler
>> > > and run it to see if the extension is present.
>> > You can't rely on the lack of a trap meaning that Zkr is present unless
>> > you know that the platform implements Ssstrict. The CSR with that number
>> > could do anything if not Ssstrict compliant, so this approach gets a
>> > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
>> > it, so you're stuck with getting that information from firmware.
>>
>>
>> FYI, this patch is my idea, so I'm the one to blame here :)
>>
>>
>> >
>> > For DT systems, you can actually parse the DT in the pi, we do it to get
>> > the kaslr seed if present, so you can actually check for Zkr. With ACPI
>> > I have no idea how you can get that information, I amn't an ACPI-ist.
>>
>>
>> I took a look at how to access ACPI tables this early when implementing the
>> Zabha/Zacas patches, but it seems not possible.
>>
>> But I'll look into this more, this is not the first time we need the
>> extensions list very early and since we have no way to detect the presence
>> of an extension at runtime, something needs to be done.
>
>Aye, having remembered that reading CSR_SEED could have side-effects on a
>system with non-conforming extensions, it'd be good to see if we can
>actually do this via detection on ACPI - especially for some other
>extensions that we may need to turn on very early (I forget which ones we
>talked about this before for). I didn't arm64 do anything with ACPI in the
>pi code, is the code arch/x86/boot/compressed run at an equivilent-ish point
>in boot?
cc: +Clement and Atish
I don't know all the details but on first glance it seems like instead of ACPI,
may be FWFT is a better place for discovery ?
https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
Supervisor could query if Sstrict is implemented and then it can check for
lack of trap on CSR_SEED or straight-away check for presence of Zkr.
On 07/06/2024 20:51, Deepak Gupta wrote:
> On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>> On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
>>> Hi Conor,
>>>
>>> On 31/05/2024 19:31, Conor Dooley wrote:
>>> > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
>>> > > Dectect the Zkr extension and use it to seed the kernel base
>>> address.
>>> > >
>>> > > Detection of the extension can not be done in the typical
>>> fashion, as
>>> > > this is very early in the boot process. Instead, add a trap handler
>>> > > and run it to see if the extension is present.
>>> > You can't rely on the lack of a trap meaning that Zkr is present
>>> unless
>>> > you know that the platform implements Ssstrict. The CSR with that
>>> number
>>> > could do anything if not Ssstrict compliant, so this approach gets a
>>> > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
>>> > it, so you're stuck with getting that information from firmware.
>>>
>>>
>>> FYI, this patch is my idea, so I'm the one to blame here :)
>>>
>>>
>>> >
>>> > For DT systems, you can actually parse the DT in the pi, we do it
>>> to get
>>> > the kaslr seed if present, so you can actually check for Zkr. With
>>> ACPI
>>> > I have no idea how you can get that information, I amn't an ACPI-ist.
>>>
>>>
>>> I took a look at how to access ACPI tables this early when
>>> implementing the
>>> Zabha/Zacas patches, but it seems not possible.
>>>
>>> But I'll look into this more, this is not the first time we need the
>>> extensions list very early and since we have no way to detect the
>>> presence
>>> of an extension at runtime, something needs to be done.
>>
>> Aye, having remembered that reading CSR_SEED could have side-effects on a
>> system with non-conforming extensions, it'd be good to see if we can
>> actually do this via detection on ACPI - especially for some other
>> extensions that we may need to turn on very early (I forget which ones we
>> talked about this before for). I didn't arm64 do anything with ACPI in
>> the
>> pi code, is the code arch/x86/boot/compressed run at an equivilent-ish
>> point
>> in boot?
>
> cc: +Clement and Atish
>
> I don't know all the details but on first glance it seems like instead
> of ACPI,
> may be FWFT is a better place for discovery ?
> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
IMHO, doing discovery in FWFT is not the goal of this extension. I think
the "real" solution would be to wait for the unified discovery task
group to come up with something for that (which is their goal I think) [1]
Clément,
Link: https://github.com/riscv/configuration-structure [1]
>
> Supervisor could query if Sstrict is implemented and then it can check for
> lack of trap on CSR_SEED or straight-away check for presence of Zkr.
>
>
On Mon, Jun 10, 2024 at 10:33:34AM +0200, Cl?ment L?ger wrote:
>
>
> On 07/06/2024 20:51, Deepak Gupta wrote:
> > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
> >> On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
> >>> Hi Conor,
> >>>
> >>> On 31/05/2024 19:31, Conor Dooley wrote:
> >>> > On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
> >>> > > Dectect the Zkr extension and use it to seed the kernel base
> >>> address.
> >>> > >
> >>> > > Detection of the extension can not be done in the typical
> >>> fashion, as
> >>> > > this is very early in the boot process. Instead, add a trap handler
> >>> > > and run it to see if the extension is present.
> >>> > You can't rely on the lack of a trap meaning that Zkr is present
> >>> unless
> >>> > you know that the platform implements Ssstrict. The CSR with that
> >>> number
> >>> > could do anything if not Ssstrict compliant, so this approach gets a
> >>> > nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
> >>> > it, so you're stuck with getting that information from firmware.
> >>>
> >>>
> >>> FYI, this patch is my idea, so I'm the one to blame here :)
> >>>
> >>>
> >>> >
> >>> > For DT systems, you can actually parse the DT in the pi, we do it
> >>> to get
> >>> > the kaslr seed if present, so you can actually check for Zkr. With
> >>> ACPI
> >>> > I have no idea how you can get that information, I amn't an ACPI-ist.
> >>>
> >>>
> >>> I took a look at how to access ACPI tables this early when
> >>> implementing the
> >>> Zabha/Zacas patches, but it seems not possible.
> >>>
> >>> But I'll look into this more, this is not the first time we need the
> >>> extensions list very early and since we have no way to detect the
> >>> presence
> >>> of an extension at runtime, something needs to be done.
> >>
> >> Aye, having remembered that reading CSR_SEED could have side-effects on a
> >> system with non-conforming extensions, it'd be good to see if we can
> >> actually do this via detection on ACPI - especially for some other
> >> extensions that we may need to turn on very early (I forget which ones we
> >> talked about this before for). I didn't arm64 do anything with ACPI in
> >> the
> >> pi code, is the code arch/x86/boot/compressed run at an equivilent-ish
> >> point
> >> in boot?
> >
> > cc: +Clement and Atish
> >
> > I don't know all the details but on first glance it seems like instead
> > of ACPI,
> > may be FWFT is a better place for discovery ?
> > https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>
> IMHO, doing discovery in FWFT is not the goal of this extension. I think
> the "real" solution would be to wait for the unified discovery task
> group to come up with something for that (which is their goal I think) [1]
I'm curious to see how that works out. The proposal documents an m-mode
csr, so we'd have to smuggle the information to s-mode somehow...
> Link: https://github.com/riscv/configuration-structure [1]
On 10/06/2024 11:02, Conor Dooley wrote:
> On Mon, Jun 10, 2024 at 10:33:34AM +0200, Clément Léger wrote:
>>
>>
>> On 07/06/2024 20:51, Deepak Gupta wrote:
>>> On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>>>> On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
>>>>> Hi Conor,
>>>>>
>>>>> On 31/05/2024 19:31, Conor Dooley wrote:
>>>>>> On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
>>>>>>> Dectect the Zkr extension and use it to seed the kernel base
>>>>> address.
>>>>>>>
>>>>>>> Detection of the extension can not be done in the typical
>>>>> fashion, as
>>>>>>> this is very early in the boot process. Instead, add a trap handler
>>>>>>> and run it to see if the extension is present.
>>>>>> You can't rely on the lack of a trap meaning that Zkr is present
>>>>> unless
>>>>>> you know that the platform implements Ssstrict. The CSR with that
>>>>> number
>>>>>> could do anything if not Ssstrict compliant, so this approach gets a
>>>>>> nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
>>>>>> it, so you're stuck with getting that information from firmware.
>>>>>
>>>>>
>>>>> FYI, this patch is my idea, so I'm the one to blame here :)
>>>>>
>>>>>
>>>>>>
>>>>>> For DT systems, you can actually parse the DT in the pi, we do it
>>>>> to get
>>>>>> the kaslr seed if present, so you can actually check for Zkr. With
>>>>> ACPI
>>>>>> I have no idea how you can get that information, I amn't an ACPI-ist.
>>>>>
>>>>>
>>>>> I took a look at how to access ACPI tables this early when
>>>>> implementing the
>>>>> Zabha/Zacas patches, but it seems not possible.
>>>>>
>>>>> But I'll look into this more, this is not the first time we need the
>>>>> extensions list very early and since we have no way to detect the
>>>>> presence
>>>>> of an extension at runtime, something needs to be done.
>>>>
>>>> Aye, having remembered that reading CSR_SEED could have side-effects on a
>>>> system with non-conforming extensions, it'd be good to see if we can
>>>> actually do this via detection on ACPI - especially for some other
>>>> extensions that we may need to turn on very early (I forget which ones we
>>>> talked about this before for). I didn't arm64 do anything with ACPI in
>>>> the
>>>> pi code, is the code arch/x86/boot/compressed run at an equivilent-ish
>>>> point
>>>> in boot?
>>>
>>> cc: +Clement and Atish
>>>
>>> I don't know all the details but on first glance it seems like instead
>>> of ACPI,
>>> may be FWFT is a better place for discovery ?
>>> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>>
>> IMHO, doing discovery in FWFT is not the goal of this extension. I think
>> the "real" solution would be to wait for the unified discovery task
>> group to come up with something for that (which is their goal I think) [1]
>
> I'm curious to see how that works out. The proposal documents an m-mode
> csr, so we'd have to smuggle the information to s-mode somehow...
Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
mconfigptr CSR will be accessible by M-mode only so I guess we will have
to find another way...
>
>> Link: https://github.com/riscv/configuration-structure [1]
>
On Mon, Jun 10, 2024 at 11:16:42AM +0200, Cl?ment L?ger wrote:
>
>
>On 10/06/2024 11:02, Conor Dooley wrote:
>> On Mon, Jun 10, 2024 at 10:33:34AM +0200, Cl?ment L?ger wrote:
>>>
>>>
>>> On 07/06/2024 20:51, Deepak Gupta wrote:
>>>> On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>>>>> On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
>>>>>> Hi Conor,
>>>>>>
>>>>>> On 31/05/2024 19:31, Conor Dooley wrote:
>>>>>>> On Fri, May 31, 2024 at 12:23:27PM -0400, Jesse Taube wrote:
>>>>>>>> Dectect the Zkr extension and use it to seed the kernel base
>>>>>> address.
>>>>>>>>
>>>>>>>> Detection of the extension can not be done in the typical
>>>>>> fashion, as
>>>>>>>> this is very early in the boot process. Instead, add a trap handler
>>>>>>>> and run it to see if the extension is present.
>>>>>>> You can't rely on the lack of a trap meaning that Zkr is present
>>>>>> unless
>>>>>>> you know that the platform implements Ssstrict. The CSR with that
>>>>>> number
>>>>>>> could do anything if not Ssstrict compliant, so this approach gets a
>>>>>>> nak from me. Unfortunately, Ssstrict doesn't provide a way to detect
>>>>>>> it, so you're stuck with getting that information from firmware.
>>>>>>
>>>>>>
>>>>>> FYI, this patch is my idea, so I'm the one to blame here :)
>>>>>>
>>>>>>
>>>>>>>
>>>>>>> For DT systems, you can actually parse the DT in the pi, we do it
>>>>>> to get
>>>>>>> the kaslr seed if present, so you can actually check for Zkr. With
>>>>>> ACPI
>>>>>>> I have no idea how you can get that information, I amn't an ACPI-ist.
>>>>>>
>>>>>>
>>>>>> I took a look at how to access ACPI tables this early when
>>>>>> implementing the
>>>>>> Zabha/Zacas patches, but it seems not possible.
>>>>>>
>>>>>> But I'll look into this more, this is not the first time we need the
>>>>>> extensions list very early and since we have no way to detect the
>>>>>> presence
>>>>>> of an extension at runtime, something needs to be done.
>>>>>
>>>>> Aye, having remembered that reading CSR_SEED could have side-effects on a
>>>>> system with non-conforming extensions, it'd be good to see if we can
>>>>> actually do this via detection on ACPI - especially for some other
>>>>> extensions that we may need to turn on very early (I forget which ones we
>>>>> talked about this before for). I didn't arm64 do anything with ACPI in
>>>>> the
>>>>> pi code, is the code arch/x86/boot/compressed run at an equivilent-ish
>>>>> point
>>>>> in boot?
>>>>
>>>> cc: +Clement and Atish
>>>>
>>>> I don't know all the details but on first glance it seems like instead
>>>> of ACPI,
>>>> may be FWFT is a better place for discovery ?
>>>> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>>>
>>> IMHO, doing discovery in FWFT is not the goal of this extension. I think
>>> the "real" solution would be to wait for the unified discovery task
>>> group to come up with something for that (which is their goal I think) [1]
Yeah I understand the conundrum here.
>>
>> I'm curious to see how that works out. The proposal documents an m-mode
>> csr, so we'd have to smuggle the information to s-mode somehow...
>
>Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
>mconfigptr CSR will be accessible by M-mode only so I guess we will have
>to find another way...
That's not the only problem. Even if you get mconfigptr access, parsing the format
is another thing that has to be done. This is early in boot. Although its strictly (pun
intended) not a firmware feature extension, I think it's much easier to ask underlying
firmware if platform is `Sstrict`. or may be expose something like below
`ENABLE_SSTRICT`.
Platforms which support `Sstrict` can return success for this while platforms which don't
have `Sstrict` can return error code (not supported or not implemented).
This way its not feature discovery.
It seems like arm64 parses fdt and it reads certain CSRs too
(`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like it has to do any
discovery for them.
>
>>
>>> Link: https://github.com/riscv/configuration-structure [1]
>>
On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
> On Mon, Jun 10, 2024 at 11:16:42AM +0200, Cl?ment L?ger wrote:
> > On 10/06/2024 11:02, Conor Dooley wrote:
> > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Cl?ment L?ger wrote:
> > > > On 07/06/2024 20:51, Deepak Gupta wrote:
> > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
> > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
> > > > > I don't know all the details but on first glance it seems like instead
> > > > > of ACPI,
> > > > > may be FWFT is a better place for discovery ?
> > > > > https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
> > > >
> > > > IMHO, doing discovery in FWFT is not the goal of this extension. I think
> > > > the "real" solution would be to wait for the unified discovery task
> > > > group to come up with something for that (which is their goal I think) [1]
>
> Yeah I understand the conundrum here.
>
> > >
> > > I'm curious to see how that works out. The proposal documents an m-mode
> > > csr, so we'd have to smuggle the information to s-mode somehow...
> >
> > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
> > mconfigptr CSR will be accessible by M-mode only so I guess we will have
> > to find another way...
>
> That's not the only problem. Even if you get mconfigptr access, parsing the format
> is another thing that has to be done. This is early in boot. Although its strictly (pun
> intended) not a firmware feature extension, I think it's much easier to ask underlying
> firmware if platform is `Sstrict`. or may be expose something like below
>
> `ENABLE_SSTRICT`.
> Platforms which support `Sstrict` can return success for this while platforms which don't
> have `Sstrict` can return error code (not supported or not implemented).
> This way its not feature discovery.
I mean, it's feature discovery in all but name. You're calling it
enable, but the behaviour you describe is feature discovery - not that I
am against this being feature discovery since it gets us out of an
annoying bind.
I forget which extension Alex and I discussed previously, but there's
some mm-related things that you're gonna have to probe super early and
we need to figure out if we can get that info from ACPI or not. I know
we discussed it w.r.t. one of the T-Head vendor extensions, but I think
another standard one got mentioned too.
> It seems like arm64 parses fdt and it reads certain CSRs too
> (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like it has to do any
> discovery for them.
A decree from the Palmer that we don't support things that do not conform
in this manner would allow us to do what arm64 does. I brought this up
originally because it's been discussed before that we cannot rely on
conformance because we want to support people's platforms, whether they
comply or not. I'd be wary of making this an exception now, and then
later on someone makes a platform we want to support that doesn't
conform and hey presto, we regress KASLR support - even if I think it is
pretty unlikely that someone is going to repurpose the Zkr CSRs.
One of the problems with only supporting conforming platforms is that
the definition of conforming changes over time! This has happened with
the Andes PMU for example, which I believe uses an interrupt number that
was later co-opted by AIA spec. That conformed at the time, but doesn't
anymore - do they get to mark themselves as Sstrict?
Maybe we can do this on a case-by-case basis, but it's up to Palmer
whether or not we can do that.
On Mon, Jun 10, 2024 at 10:56:35PM +0100, Conor Dooley wrote:
>On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
>> On Mon, Jun 10, 2024 at 11:16:42AM +0200, Cl?ment L?ger wrote:
>> > On 10/06/2024 11:02, Conor Dooley wrote:
>> > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Cl?ment L?ger wrote:
>> > > > On 07/06/2024 20:51, Deepak Gupta wrote:
>> > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>> > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti wrote:
>> > > > > I don't know all the details but on first glance it seems like instead
>> > > > > of ACPI,
>> > > > > may be FWFT is a better place for discovery ?
>> > > > > https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>> > > >
>> > > > IMHO, doing discovery in FWFT is not the goal of this extension. I think
>> > > > the "real" solution would be to wait for the unified discovery task
>> > > > group to come up with something for that (which is their goal I think) [1]
>>
>> Yeah I understand the conundrum here.
>>
>> > >
>> > > I'm curious to see how that works out. The proposal documents an m-mode
>> > > csr, so we'd have to smuggle the information to s-mode somehow...
>> >
>> > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
>> > mconfigptr CSR will be accessible by M-mode only so I guess we will have
>> > to find another way...
>>
>> That's not the only problem. Even if you get mconfigptr access, parsing the format
>> is another thing that has to be done. This is early in boot. Although its strictly (pun
>> intended) not a firmware feature extension, I think it's much easier to ask underlying
>> firmware if platform is `Sstrict`. or may be expose something like below
>>
>> `ENABLE_SSTRICT`.
>> Platforms which support `Sstrict` can return success for this while platforms which don't
>> have `Sstrict` can return error code (not supported or not implemented).
>> This way its not feature discovery.
>
>I mean, it's feature discovery in all but name. You're calling it
>enable, but the behaviour you describe is feature discovery - not that I
>am against this being feature discovery since it gets us out of an
>annoying bind.
Yes I know it's cheating but at least for this case, it seems like easy solution which
doesn't break anything. Neither I see it creating any future problems (except FWFT becoming
to look like discovery mechanism :-) and Clement/Atish hating me for that)
Another solution to this could be introducing a riscv config `CONFIG_RISCV_SSTRICT`.
By default always select `CONFIG_RISCV_SSTRICT` and any platform owner who are not
sstrict, they can build their own.
I expect distro (ubuntu, red hat, etc) would want by default `CONFIG_RISCV_SSTRICT`.
>
>I forget which extension Alex and I discussed previously, but there's
>some mm-related things that you're gonna have to probe super early and
>we need to figure out if we can get that info from ACPI or not. I know
>we discussed it w.r.t. one of the T-Head vendor extensions, but I think
>another standard one got mentioned too.
>
>> It seems like arm64 parses fdt and it reads certain CSRs too
>> (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like it has to do any
>> discovery for them.
>
>A decree from the Palmer that we don't support things that do not conform
>in this manner would allow us to do what arm64 does. I brought this up
>originally because it's been discussed before that we cannot rely on
>conformance because we want to support people's platforms, whether they
>comply or not. I'd be wary of making this an exception now, and then
>later on someone makes a platform we want to support that doesn't
>conform and hey presto, we regress KASLR support - even if I think it is
>pretty unlikely that someone is going to repurpose the Zkr CSRs.
>
>One of the problems with only supporting conforming platforms is that
>the definition of conforming changes over time! This has happened with
>the Andes PMU for example, which I believe uses an interrupt number that
>was later co-opted by AIA spec. That conformed at the time, but doesn't
>anymore - do they get to mark themselves as Sstrict?
>
>Maybe we can do this on a case-by-case basis, but it's up to Palmer
>whether or not we can do that.
On 11/06/2024 17:32, Deepak Gupta wrote:
> On Mon, Jun 10, 2024 at 10:56:35PM +0100, Conor Dooley wrote:
>> On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
>>> On Mon, Jun 10, 2024 at 11:16:42AM +0200, Clément Léger wrote:
>>> > On 10/06/2024 11:02, Conor Dooley wrote:
>>> > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Clément Léger wrote:
>>> > > > On 07/06/2024 20:51, Deepak Gupta wrote:
>>> > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>>> > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti
>>> wrote:
>>> > > > > I don't know all the details but on first glance it seems
>>> like instead
>>> > > > > of ACPI,
>>> > > > > may be FWFT is a better place for discovery ?
>>> > > > >
>>> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>>> > > >
>>> > > > IMHO, doing discovery in FWFT is not the goal of this
>>> extension. I think
>>> > > > the "real" solution would be to wait for the unified discovery
>>> task
>>> > > > group to come up with something for that (which is their goal I
>>> think) [1]
>>>
>>> Yeah I understand the conundrum here.
>>>
>>> > >
>>> > > I'm curious to see how that works out. The proposal documents an
>>> m-mode
>>> > > csr, so we'd have to smuggle the information to s-mode somehow...
>>> >
>>> > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
>>> > mconfigptr CSR will be accessible by M-mode only so I guess we will
>>> have
>>> > to find another way...
>>>
>>> That's not the only problem. Even if you get mconfigptr access,
>>> parsing the format
>>> is another thing that has to be done. This is early in boot. Although
>>> its strictly (pun
>>> intended) not a firmware feature extension, I think it's much easier
>>> to ask underlying
>>> firmware if platform is `Sstrict`. or may be expose something like below
>>>
>>> `ENABLE_SSTRICT`.
>>> Platforms which support `Sstrict` can return success for this while
>>> platforms which don't
>>> have `Sstrict` can return error code (not supported or not implemented).
>>> This way its not feature discovery.
>>
>> I mean, it's feature discovery in all but name. You're calling it
>> enable, but the behaviour you describe is feature discovery - not that I
>> am against this being feature discovery since it gets us out of an
>> annoying bind.
>
> Yes I know it's cheating but at least for this case, it seems like easy
> solution which
> doesn't break anything. Neither I see it creating any future problems
> (except FWFT becoming
> to look like discovery mechanism :-) and Clement/Atish hating me for that)
Ahah no worries;) Thinking a bit more about it, if we need only a few
extensions to be discoverable, it seems manageable (ie add a "locked"
feature that report the extension availability itself, get only will
work, set will return SBI_EDENIED). But if need more of them, then a
dedicated mechanism should probably be designed.
Clément
>
> Another solution to this could be introducing a riscv config
> `CONFIG_RISCV_SSTRICT`.
> By default always select `CONFIG_RISCV_SSTRICT` and any platform owner
> who are not
> sstrict, they can build their own.
> I expect distro (ubuntu, red hat, etc) would want by default
> `CONFIG_RISCV_SSTRICT`.
>
>>
>> I forget which extension Alex and I discussed previously, but there's
>> some mm-related things that you're gonna have to probe super early and
>> we need to figure out if we can get that info from ACPI or not. I know
>> we discussed it w.r.t. one of the T-Head vendor extensions, but I think
>> another standard one got mentioned too.
>>
>>> It seems like arm64 parses fdt and it reads certain CSRs too
>>> (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like
>>> it has to do any
>>> discovery for them.
>>
>> A decree from the Palmer that we don't support things that do not conform
>> in this manner would allow us to do what arm64 does. I brought this up
>> originally because it's been discussed before that we cannot rely on
>> conformance because we want to support people's platforms, whether they
>> comply or not. I'd be wary of making this an exception now, and then
>> later on someone makes a platform we want to support that doesn't
>> conform and hey presto, we regress KASLR support - even if I think it is
>> pretty unlikely that someone is going to repurpose the Zkr CSRs.
>>
>> One of the problems with only supporting conforming platforms is that
>> the definition of conforming changes over time! This has happened with
>> the Andes PMU for example, which I believe uses an interrupt number that
>> was later co-opted by AIA spec. That conformed at the time, but doesn't
>> anymore - do they get to mark themselves as Sstrict?
>>
>> Maybe we can do this on a case-by-case basis, but it's up to Palmer
>> whether or not we can do that.
On Wed, Jun 12, 2024 at 12:15 AM Clément Léger <[email protected]> wrote:
>
>
>
> On 11/06/2024 17:32, Deepak Gupta wrote:
> > On Mon, Jun 10, 2024 at 10:56:35PM +0100, Conor Dooley wrote:
> >> On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
> >>> On Mon, Jun 10, 2024 at 11:16:42AM +0200, Clément Léger wrote:
> >>> > On 10/06/2024 11:02, Conor Dooley wrote:
> >>> > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Clément Léger wrote:
> >>> > > > On 07/06/2024 20:51, Deepak Gupta wrote:
> >>> > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
> >>> > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti
> >>> wrote:
> >>> > > > > I don't know all the details but on first glance it seems
> >>> like instead
> >>> > > > > of ACPI,
> >>> > > > > may be FWFT is a better place for discovery ?
> >>> > > > >
> >>> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
> >>> > > >
> >>> > > > IMHO, doing discovery in FWFT is not the goal of this
> >>> extension. I think
> >>> > > > the "real" solution would be to wait for the unified discovery
> >>> task
> >>> > > > group to come up with something for that (which is their goal I
> >>> think) [1]
> >>>
> >>> Yeah I understand the conundrum here.
> >>>
> >>> > >
> >>> > > I'm curious to see how that works out. The proposal documents an
> >>> m-mode
> >>> > > csr, so we'd have to smuggle the information to s-mode somehow...
> >>> >
> >>> > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
> >>> > mconfigptr CSR will be accessible by M-mode only so I guess we will
> >>> have
> >>> > to find another way...
> >>>
> >>> That's not the only problem. Even if you get mconfigptr access,
> >>> parsing the format
> >>> is another thing that has to be done. This is early in boot. Although
> >>> its strictly (pun
> >>> intended) not a firmware feature extension, I think it's much easier
> >>> to ask underlying
> >>> firmware if platform is `Sstrict`. or may be expose something like below
> >>>
> >>> `ENABLE_SSTRICT`.
> >>> Platforms which support `Sstrict` can return success for this while
> >>> platforms which don't
> >>> have `Sstrict` can return error code (not supported or not implemented).
> >>> This way its not feature discovery.
> >>
> >> I mean, it's feature discovery in all but name. You're calling it
> >> enable, but the behaviour you describe is feature discovery - not that I
> >> am against this being feature discovery since it gets us out of an
> >> annoying bind.
> >
> > Yes I know it's cheating but at least for this case, it seems like easy
> > solution which
> > doesn't break anything. Neither I see it creating any future problems
> > (except FWFT becoming
> > to look like discovery mechanism :-) and Clement/Atish hating me for that)
>
> Ahah no worries;) Thinking a bit more about it, if we need only a few
> extensions to be discoverable, it seems manageable (ie add a "locked"
> feature that report the extension availability itself, get only will
> work, set will return SBI_EDENIED). But if need more of them, then a
> dedicated mechanism should probably be designed.
>
Retrying again as gmail switched my default to html again :(. Sorry
for the spam.
Yes. Once it is allowed, there will be many more in the future :).
Reading this thread, it seems we need early detection for these 3 now.
Zabha/Zacas/Zkr
Is there any use case for obtaining additional information related to
an ISA or just extension presence is enough ?
+Sunil (in case he has some tricks in ACPI land for early parsing).
Otherwise, we may have to come up with a separate mechanism for early detection.
> Clément
>
> >
> > Another solution to this could be introducing a riscv config
> > `CONFIG_RISCV_SSTRICT`.
> > By default always select `CONFIG_RISCV_SSTRICT` and any platform owner
> > who are not
> > sstrict, they can build their own.
> > I expect distro (ubuntu, red hat, etc) would want by default
> > `CONFIG_RISCV_SSTRICT`.
> >
> >>
> >> I forget which extension Alex and I discussed previously, but there's
> >> some mm-related things that you're gonna have to probe super early and
> >> we need to figure out if we can get that info from ACPI or not. I know
> >> we discussed it w.r.t. one of the T-Head vendor extensions, but I think
> >> another standard one got mentioned too.
> >>
> >>> It seems like arm64 parses fdt and it reads certain CSRs too
> >>> (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like
> >>> it has to do any
> >>> discovery for them.
> >>
> >> A decree from the Palmer that we don't support things that do not conform
> >> in this manner would allow us to do what arm64 does. I brought this up
> >> originally because it's been discussed before that we cannot rely on
> >> conformance because we want to support people's platforms, whether they
> >> comply or not. I'd be wary of making this an exception now, and then
> >> later on someone makes a platform we want to support that doesn't
> >> conform and hey presto, we regress KASLR support - even if I think it is
> >> pretty unlikely that someone is going to repurpose the Zkr CSRs.
> >>
> >> One of the problems with only supporting conforming platforms is that
> >> the definition of conforming changes over time! This has happened with
> >> the Andes PMU for example, which I believe uses an interrupt number that
> >> was later co-opted by AIA spec. That conformed at the time, but doesn't
> >> anymore - do they get to mark themselves as Sstrict?
> >>
> >> Maybe we can do this on a case-by-case basis, but it's up to Palmer
> >> whether or not we can do that.
On Wed, 12 Jun 2024 00:15:26 PDT (-0700), [email protected] wrote:
>
>
> On 11/06/2024 17:32, Deepak Gupta wrote:
>> On Mon, Jun 10, 2024 at 10:56:35PM +0100, Conor Dooley wrote:
>>> On Mon, Jun 10, 2024 at 02:06:50PM -0700, Deepak Gupta wrote:
>>>> On Mon, Jun 10, 2024 at 11:16:42AM +0200, Clément Léger wrote:
>>>> > On 10/06/2024 11:02, Conor Dooley wrote:
>>>> > > On Mon, Jun 10, 2024 at 10:33:34AM +0200, Clément Léger wrote:
>>>> > > > On 07/06/2024 20:51, Deepak Gupta wrote:
>>>> > > > > On Mon, Jun 03, 2024 at 01:47:40PM +0100, Conor Dooley wrote:
>>>> > > > > > On Mon, Jun 03, 2024 at 11:14:49AM +0200, Alexandre Ghiti
>>>> wrote:
>>>> > > > > I don't know all the details but on first glance it seems
>>>> like instead
>>>> > > > > of ACPI,
>>>> > > > > may be FWFT is a better place for discovery ?
>>>> > > > >
>>>> https://lists.riscv.org/g/tech-prs/topic/patch_v12_add_firmware/106479571
>>>> > > >
>>>> > > > IMHO, doing discovery in FWFT is not the goal of this
>>>> extension. I think
>>>> > > > the "real" solution would be to wait for the unified discovery
>>>> task
>>>> > > > group to come up with something for that (which is their goal I
>>>> think) [1]
>>>>
>>>> Yeah I understand the conundrum here.
>>>>
>>>> > >
>>>> > > I'm curious to see how that works out. The proposal documents an
>>>> m-mode
>>>> > > csr, so we'd have to smuggle the information to s-mode somehow...
>>>> >
>>>> > Ahem, yeah, I spoke a bit too fast. Looked at the proposal and the
>>>> > mconfigptr CSR will be accessible by M-mode only so I guess we will
>>>> have
>>>> > to find another way...
>>>>
>>>> That's not the only problem. Even if you get mconfigptr access,
>>>> parsing the format
>>>> is another thing that has to be done. This is early in boot. Although
>>>> its strictly (pun
>>>> intended) not a firmware feature extension, I think it's much easier
>>>> to ask underlying
>>>> firmware if platform is `Sstrict`. or may be expose something like below
>>>>
>>>> `ENABLE_SSTRICT`.
>>>> Platforms which support `Sstrict` can return success for this while
>>>> platforms which don't
>>>> have `Sstrict` can return error code (not supported or not implemented).
>>>> This way its not feature discovery.
>>>
>>> I mean, it's feature discovery in all but name. You're calling it
>>> enable, but the behaviour you describe is feature discovery - not that I
>>> am against this being feature discovery since it gets us out of an
>>> annoying bind.
>>
>> Yes I know it's cheating but at least for this case, it seems like easy
>> solution which
>> doesn't break anything. Neither I see it creating any future problems
>> (except FWFT becoming
>> to look like discovery mechanism :-) and Clement/Atish hating me for that)
>
> Ahah no worries;) Thinking a bit more about it, if we need only a few
> extensions to be discoverable, it seems manageable (ie add a "locked"
> feature that report the extension availability itself, get only will
> work, set will return SBI_EDENIED). But if need more of them, then a
> dedicated mechanism should probably be designed.
>
> Clément
>
>>
>> Another solution to this could be introducing a riscv config
>> `CONFIG_RISCV_SSTRICT`.
>> By default always select `CONFIG_RISCV_SSTRICT` and any platform owner
>> who are not
>> sstrict, they can build their own.
>> I expect distro (ubuntu, red hat, etc) would want by default
>> `CONFIG_RISCV_SSTRICT`.
IMO things like Sstrict is going to end up being useless. It's just
another way to kick the can down the road on actually having
compatibility. We've already seen this with the extensions and the
profiles, I don't see how Sstrict is any different.
The RISC-V foundation needs to just actually do compatibility, making up
more names isn't going to change anything.
>>> I forget which extension Alex and I discussed previously, but there's
>>> some mm-related things that you're gonna have to probe super early and
>>> we need to figure out if we can get that info from ACPI or not. I know
>>> we discussed it w.r.t. one of the T-Head vendor extensions, but I think
>>> another standard one got mentioned too.
>>>
>>>> It seems like arm64 parses fdt and it reads certain CSRs too
>>>> (`arch/arm64/kernel/pi/kaslr_early.c`). Although it doesn't look like
>>>> it has to do any
>>>> discovery for them.
>>>
>>> A decree from the Palmer that we don't support things that do not conform
>>> in this manner would allow us to do what arm64 does. I brought this up
>>> originally because it's been discussed before that we cannot rely on
>>> conformance because we want to support people's platforms, whether they
>>> comply or not. I'd be wary of making this an exception now, and then
>>> later on someone makes a platform we want to support that doesn't
>>> conform and hey presto, we regress KASLR support - even if I think it is
>>> pretty unlikely that someone is going to repurpose the Zkr CSRs.
Ya, I think that's fine. Let's just blindly read the CSR without
probing, if some hardware has destructive side effects on the CSR read
then we can figure out what to do with it later.
>>> One of the problems with only supporting conforming platforms is that
>>> the definition of conforming changes over time! This has happened with
>>> the Andes PMU for example, which I believe uses an interrupt number that
>>> was later co-opted by AIA spec. That conformed at the time, but doesn't
>>> anymore - do they get to mark themselves as Sstrict?
>>>
>>> Maybe we can do this on a case-by-case basis, but it's up to Palmer
>>> whether or not we can do that.
Yep. Trying to stick to only supporting what counts as conforming today
isn't going to work, RISC-V just isn't managed that way. That's why we
have all these strict rules about avoiding breaking what used to work in
software land, if we didn't have those then we be breaking users every
release.