Generally, riscv ISA extensions are fixed for any specific hardware
platform, so a hart's features won't change after booting, this
chacteristic makes it straightforward to use a static branch to check
a specific ISA extension is supported or not to optimize performance.
However, some ISA extensions such as SVPBMT and ZICBOM are handled
via. the alternative sequences.
Basically, for ease of maintenance, we prefer to use static branches
in C code, but recently, Samuel found that the static branch usage in
cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
Samuel pointed out, "Having a static branch in cpu_relax() is
problematic because that function is widely inlined, including in some
quite complex functions like in the VDSO. A quick measurement shows
this static branch is responsible by itself for around 40% of the jump
table."
Samuel's findings pointed out one of a few downsides of static branches
usage in C code to handle ISA extensions detected at boot time:
static branch's metadata in the __jump_table section, which is not
discarded after ISA extensions are finalized, wastes some space.
I want to try to solve the issue for all possible dynamic handling of
ISA extensions at boot time. Inspired by Mark[2], this patch introduces
riscv_has_extension_*() helpers, which work like static branches but
are patched using alternatives, thus the metadata can be freed after
patching.
Since v4
- collect Reviewed-by and Acked-by tag
- rebase on the latest riscv for-next
- add Andrew's patch to add ADD16 and SUB16 rela types
- adopt Conor's nit comment to patch9
Since v3
- collect Reviewed-by tag and remove Heiko's reviewed-by from patch5
- address Conor and Andrew comments
- fix two building errors of !MMU and RV32
Since v2
- rebase on riscv-next
- collect Reviewed-by tag
- fix jal imm construction
- combine Heiko's code and my code for jal patching, thus add
Co-developed-by tag
- address comments from Conor
Since v1
- rebase on v6.1-rc7 + Heiko's alternative improvements[3]
- collect Reviewed-by tag
- add one patch to update jal offsets in patched alternatives
- add one patch to switch to relative alternative entries
- add patches to patch vdso
[1]https://lore.kernel.org/linux-riscv/[email protected]/
[2]https://lore.kernel.org/linux-arm-kernel/[email protected]/
[3]https://lore.kernel.org/linux-riscv/[email protected]/
Andrew Jones (2):
riscv: module: Add ADD16 and SUB16 rela types
riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
Jisheng Zhang (11):
riscv: move riscv_noncoherent_supported() out of ZICBOM probe
riscv: cpufeature: detect RISCV_ALTERNATIVES_EARLY_BOOT earlier
riscv: hwcap: make ISA extension ids can be used in asm
riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA
extensions
riscv: introduce riscv_has_extension_[un]likely()
riscv: fpu: switch has_fpu() to riscv_has_extension_likely()
riscv: module: move find_section to module.h
riscv: switch to relative alternative entries
riscv: alternative: patch alternatives in the vDSO
riscv: cpu_relax: switch to riscv_has_extension_likely()
riscv: remove riscv_isa_ext_keys[] array and related usage
arch/riscv/errata/sifive/errata.c | 3 +-
arch/riscv/errata/thead/errata.c | 11 ++-
arch/riscv/include/asm/alternative-macros.h | 20 ++---
arch/riscv/include/asm/alternative.h | 17 ++--
arch/riscv/include/asm/errata_list.h | 9 +-
arch/riscv/include/asm/hwcap.h | 98 +++++++++++----------
arch/riscv/include/asm/module.h | 16 ++++
arch/riscv/include/asm/switch_to.h | 3 +-
arch/riscv/include/asm/vdso.h | 4 +
arch/riscv/include/asm/vdso/processor.h | 2 +-
arch/riscv/kernel/alternative.c | 29 ++++++
arch/riscv/kernel/cpufeature.c | 79 +++--------------
arch/riscv/kernel/module.c | 31 +++----
arch/riscv/kernel/setup.c | 3 +
arch/riscv/kernel/vdso.c | 5 --
arch/riscv/kernel/vdso/vdso.lds.S | 7 ++
arch/riscv/kvm/tlb.c | 3 +-
17 files changed, 176 insertions(+), 164 deletions(-)
--
2.38.1
riscv_cpufeature_patch_func() currently only scans a limited set of
cpufeatures, explicitly defined with macros. Extend it to probe for all
ISA extensions.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/errata_list.h | 9 ++--
arch/riscv/kernel/cpufeature.c | 64 ++++------------------------
2 files changed, 11 insertions(+), 62 deletions(-)
diff --git a/arch/riscv/include/asm/errata_list.h b/arch/riscv/include/asm/errata_list.h
index 4180312d2a70..274c6f889602 100644
--- a/arch/riscv/include/asm/errata_list.h
+++ b/arch/riscv/include/asm/errata_list.h
@@ -7,6 +7,7 @@
#include <asm/alternative.h>
#include <asm/csr.h>
+#include <asm/hwcap.h>
#include <asm/vendorid_list.h>
#ifdef CONFIG_ERRATA_SIFIVE
@@ -22,10 +23,6 @@
#define ERRATA_THEAD_NUMBER 3
#endif
-#define CPUFEATURE_SVPBMT 0
-#define CPUFEATURE_ZICBOM 1
-#define CPUFEATURE_NUMBER 2
-
#ifdef __ASSEMBLY__
#define ALT_INSN_FAULT(x) \
@@ -55,7 +52,7 @@ asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID, \
#define ALT_SVPBMT(_val, prot) \
asm(ALTERNATIVE_2("li %0, 0\t\nnop", \
"li %0, %1\t\nslli %0,%0,%3", 0, \
- CPUFEATURE_SVPBMT, CONFIG_RISCV_ISA_SVPBMT, \
+ RISCV_ISA_EXT_SVPBMT, CONFIG_RISCV_ISA_SVPBMT, \
"li %0, %2\t\nslli %0,%0,%4", THEAD_VENDOR_ID, \
ERRATA_THEAD_PBMT, CONFIG_ERRATA_THEAD_PBMT) \
: "=r"(_val) \
@@ -129,7 +126,7 @@ asm volatile(ALTERNATIVE_2( \
"add a0, a0, %0\n\t" \
"2:\n\t" \
"bltu a0, %2, 3b\n\t" \
- "nop", 0, CPUFEATURE_ZICBOM, CONFIG_RISCV_ISA_ZICBOM, \
+ "nop", 0, RISCV_ISA_EXT_ZICBOM, CONFIG_RISCV_ISA_ZICBOM, \
"mv a0, %1\n\t" \
"j 2f\n\t" \
"3:\n\t" \
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 59e20cad1b3d..6193f401f0c5 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -276,59 +276,11 @@ void __init riscv_fill_hwcap(void)
}
#ifdef CONFIG_RISCV_ALTERNATIVE
-static bool __init_or_module cpufeature_probe_svpbmt(unsigned int stage)
-{
- if (!IS_ENABLED(CONFIG_RISCV_ISA_SVPBMT))
- return false;
-
- if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
- return false;
-
- return riscv_isa_extension_available(NULL, SVPBMT);
-}
-
-static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
-{
- if (!IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM))
- return false;
-
- if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
- return false;
-
- if (!riscv_isa_extension_available(NULL, ZICBOM))
- return false;
-
- return true;
-}
-
-/*
- * Probe presence of individual extensions.
- *
- * This code may also be executed before kernel relocation, so we cannot use
- * addresses generated by the address-of operator as they won't be valid in
- * this context.
- * Tests, unless otherwise required, are to be added in alphabetical order.
- */
-static u32 __init_or_module cpufeature_probe(unsigned int stage)
-{
- u32 cpu_req_feature = 0;
-
- if (cpufeature_probe_svpbmt(stage))
- cpu_req_feature |= BIT(CPUFEATURE_SVPBMT);
-
- if (cpufeature_probe_zicbom(stage))
- cpu_req_feature |= BIT(CPUFEATURE_ZICBOM);
-
- return cpu_req_feature;
-}
-
void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
struct alt_entry *end,
unsigned int stage)
{
- u32 cpu_req_feature = cpufeature_probe(stage);
struct alt_entry *alt;
- u32 tmp;
if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
return;
@@ -336,18 +288,18 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
for (alt = begin; alt < end; alt++) {
if (alt->vendor_id != 0)
continue;
- if (alt->errata_id >= CPUFEATURE_NUMBER) {
- WARN(1, "This feature id:%d is not in kernel cpufeature list",
+ if (alt->errata_id >= RISCV_ISA_EXT_MAX) {
+ WARN(1, "This extension id:%d is not in ISA extension list",
alt->errata_id);
continue;
}
- tmp = (1U << alt->errata_id);
- if (cpu_req_feature & tmp) {
- patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
- riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len,
- alt->old_ptr - alt->alt_ptr);
- }
+ if (!__riscv_isa_extension_available(NULL, alt->errata_id))
+ continue;
+
+ patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
+ riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len,
+ alt->old_ptr - alt->alt_ptr);
}
}
#endif
--
2.38.1
Move find_section() to module.h so that the implementation can be shared
by the alternatives code. This will allow us to use alternatives in
the vdso.
Reviewed-by: Conor Dooley <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Signed-off-by: Jisheng Zhang <[email protected]>
---
arch/riscv/include/asm/module.h | 16 ++++++++++++++++
arch/riscv/kernel/module.c | 15 ---------------
2 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/arch/riscv/include/asm/module.h b/arch/riscv/include/asm/module.h
index 76aa96a9fc08..0f3baaa6a9a8 100644
--- a/arch/riscv/include/asm/module.h
+++ b/arch/riscv/include/asm/module.h
@@ -5,6 +5,7 @@
#define _ASM_RISCV_MODULE_H
#include <asm-generic/module.h>
+#include <linux/elf.h>
struct module;
unsigned long module_emit_got_entry(struct module *mod, unsigned long val);
@@ -111,4 +112,19 @@ static inline struct plt_entry *get_plt_entry(unsigned long val,
#endif /* CONFIG_MODULE_SECTIONS */
+static inline const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
+ const Elf_Shdr *sechdrs,
+ const char *name)
+{
+ const Elf_Shdr *s, *se;
+ const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
+
+ for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
+ if (strcmp(name, secstrs + s->sh_name) == 0)
+ return s;
+ }
+
+ return NULL;
+}
+
#endif /* _ASM_RISCV_MODULE_H */
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 91fe16bfaa07..76f4b9c2ec5b 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -429,21 +429,6 @@ void *module_alloc(unsigned long size)
}
#endif
-static const Elf_Shdr *find_section(const Elf_Ehdr *hdr,
- const Elf_Shdr *sechdrs,
- const char *name)
-{
- const Elf_Shdr *s, *se;
- const char *secstrs = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
-
- for (s = sechdrs, se = sechdrs + hdr->e_shnum; s < se; s++) {
- if (strcmp(name, secstrs + s->sh_name) == 0)
- return s;
- }
-
- return NULL;
-}
-
int module_finalize(const Elf_Ehdr *hdr,
const Elf_Shdr *sechdrs,
struct module *me)
--
2.38.1
It's a bit weird to call riscv_noncoherent_supported() each time when
insmoding a module. Move the calling out of feature patch func.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/kernel/cpufeature.c | 1 -
arch/riscv/kernel/setup.c | 3 +++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index dde0e91d7668..62443fd32fa7 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -298,7 +298,6 @@ static bool __init_or_module cpufeature_probe_zicbom(unsigned int stage)
if (!riscv_isa_extension_available(NULL, ZICBOM))
return false;
- riscv_noncoherent_supported();
return true;
}
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c
index 86acd690d529..376d2827e736 100644
--- a/arch/riscv/kernel/setup.c
+++ b/arch/riscv/kernel/setup.c
@@ -300,6 +300,9 @@ void __init setup_arch(char **cmdline_p)
riscv_init_cbom_blocksize();
riscv_fill_hwcap();
apply_boot_alternatives();
+ if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) &&
+ riscv_isa_extension_available(NULL, ZICBOM))
+ riscv_noncoherent_supported();
}
static int __init topology_init(void)
--
2.38.1
Currently riscv_cpufeature_patch_func() does nothing at the
RISCV_ALTERNATIVES_EARLY_BOOT stage. Add a check to detect whether we
are in this stage and exit early. This will allow us to use
riscv_cpufeature_patch_func() for scanning of all ISA extensions.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/kernel/cpufeature.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 62443fd32fa7..59e20cad1b3d 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -330,6 +330,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
struct alt_entry *alt;
u32 tmp;
+ if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
+ return;
+
for (alt = begin; alt < end; alt++) {
if (alt->vendor_id != 0)
continue;
--
2.38.1
So that ISA extensions can be used in assembly files, convert the
multi-letter RISC-V ISA extension IDs enums to macros.
In order to make them visible, move the #ifndef __ASSEMBLY__ guard
to a later point in the header
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 45 ++++++++++++++++------------------
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 57439da71c77..8e0ee841fa77 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -12,20 +12,6 @@
#include <linux/bits.h>
#include <uapi/asm/hwcap.h>
-#ifndef __ASSEMBLY__
-#include <linux/jump_label.h>
-/*
- * This yields a mask that user programs can use to figure out what
- * instruction set this cpu supports.
- */
-#define ELF_HWCAP (elf_hwcap)
-
-enum {
- CAP_HWCAP = 1,
-};
-
-extern unsigned long elf_hwcap;
-
#define RISCV_ISA_EXT_a ('a' - 'a')
#define RISCV_ISA_EXT_c ('c' - 'a')
#define RISCV_ISA_EXT_d ('d' - 'a')
@@ -46,23 +32,34 @@ extern unsigned long elf_hwcap;
#define RISCV_ISA_EXT_BASE 26
/*
- * This enum represent the logical ID for each multi-letter RISC-V ISA extension.
+ * These macros represent the logical ID for each multi-letter RISC-V ISA extension.
* The logical ID should start from RISCV_ISA_EXT_BASE and must not exceed
* RISCV_ISA_EXT_MAX. 0-25 range is reserved for single letter
* extensions while all the multi-letter extensions should define the next
* available logical extension id.
* Entries are sorted alphabetically.
*/
-enum riscv_isa_ext_id {
- RISCV_ISA_EXT_SSCOFPMF = RISCV_ISA_EXT_BASE,
- RISCV_ISA_EXT_SSTC,
- RISCV_ISA_EXT_SVINVAL,
- RISCV_ISA_EXT_SVPBMT,
- RISCV_ISA_EXT_ZICBOM,
- RISCV_ISA_EXT_ZIHINTPAUSE,
- RISCV_ISA_EXT_ID_MAX
+#define RISCV_ISA_EXT_SSCOFPMF 26
+#define RISCV_ISA_EXT_SSTC 27
+#define RISCV_ISA_EXT_SVINVAL 28
+#define RISCV_ISA_EXT_SVPBMT 29
+#define RISCV_ISA_EXT_ZICBOM 30
+#define RISCV_ISA_EXT_ZIHINTPAUSE 31
+
+#ifndef __ASSEMBLY__
+#include <linux/jump_label.h>
+/*
+ * This yields a mask that user programs can use to figure out what
+ * instruction set this cpu supports.
+ */
+#define ELF_HWCAP (elf_hwcap)
+
+enum {
+ CAP_HWCAP = 1,
};
-static_assert(RISCV_ISA_EXT_ID_MAX <= RISCV_ISA_EXT_MAX);
+
+extern unsigned long elf_hwcap;
+
/*
* This enum represents the logical ID for each RISC-V ISA extension static
--
2.38.1
From: Andrew Jones <[email protected]>
To prepare for 16-bit relocation types to be emitted in alternatives
add support for ADD16 and SUB16.
Signed-off-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/kernel/module.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/arch/riscv/kernel/module.c b/arch/riscv/kernel/module.c
index 76f4b9c2ec5b..7c651d55fcbd 100644
--- a/arch/riscv/kernel/module.c
+++ b/arch/riscv/kernel/module.c
@@ -268,6 +268,13 @@ static int apply_r_riscv_align_rela(struct module *me, u32 *location,
return -EINVAL;
}
+static int apply_r_riscv_add16_rela(struct module *me, u32 *location,
+ Elf_Addr v)
+{
+ *(u16 *)location += (u16)v;
+ return 0;
+}
+
static int apply_r_riscv_add32_rela(struct module *me, u32 *location,
Elf_Addr v)
{
@@ -282,6 +289,13 @@ static int apply_r_riscv_add64_rela(struct module *me, u32 *location,
return 0;
}
+static int apply_r_riscv_sub16_rela(struct module *me, u32 *location,
+ Elf_Addr v)
+{
+ *(u16 *)location -= (u16)v;
+ return 0;
+}
+
static int apply_r_riscv_sub32_rela(struct module *me, u32 *location,
Elf_Addr v)
{
@@ -315,8 +329,10 @@ static int (*reloc_handlers_rela[]) (struct module *me, u32 *location,
[R_RISCV_CALL] = apply_r_riscv_call_rela,
[R_RISCV_RELAX] = apply_r_riscv_relax_rela,
[R_RISCV_ALIGN] = apply_r_riscv_align_rela,
+ [R_RISCV_ADD16] = apply_r_riscv_add16_rela,
[R_RISCV_ADD32] = apply_r_riscv_add32_rela,
[R_RISCV_ADD64] = apply_r_riscv_add64_rela,
+ [R_RISCV_SUB16] = apply_r_riscv_sub16_rela,
[R_RISCV_SUB32] = apply_r_riscv_sub32_rela,
[R_RISCV_SUB64] = apply_r_riscv_sub64_rela,
};
--
2.38.1
Switch cpu_relax() from static branch to the new helper
riscv_has_extension_likely()
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/vdso/processor.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/vdso/processor.h b/arch/riscv/include/asm/vdso/processor.h
index fa70cfe507aa..edf0e25e43d1 100644
--- a/arch/riscv/include/asm/vdso/processor.h
+++ b/arch/riscv/include/asm/vdso/processor.h
@@ -10,7 +10,7 @@
static inline void cpu_relax(void)
{
- if (!static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_ZIHINTPAUSE])) {
+ if (!riscv_has_extension_likely(RISCV_ISA_EXT_ZIHINTPAUSE)) {
#ifdef __riscv_muldiv
int dummy;
/* In lieu of a halt instruction, induce a long-latency stall. */
--
2.38.1
Make it possible to use alternatives in the vDSO, so that better
implementations can be used if possible.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
---
arch/riscv/include/asm/vdso.h | 4 ++++
arch/riscv/kernel/alternative.c | 29 +++++++++++++++++++++++++++++
arch/riscv/kernel/vdso.c | 5 -----
arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++
4 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/arch/riscv/include/asm/vdso.h b/arch/riscv/include/asm/vdso.h
index a7644f46d0e5..f891478829a5 100644
--- a/arch/riscv/include/asm/vdso.h
+++ b/arch/riscv/include/asm/vdso.h
@@ -28,8 +28,12 @@
#define COMPAT_VDSO_SYMBOL(base, name) \
(void __user *)((unsigned long)(base) + compat__vdso_##name##_offset)
+extern char compat_vdso_start[], compat_vdso_end[];
+
#endif /* CONFIG_COMPAT */
+extern char vdso_start[], vdso_end[];
+
#endif /* !__ASSEMBLY__ */
#endif /* CONFIG_MMU */
diff --git a/arch/riscv/kernel/alternative.c b/arch/riscv/kernel/alternative.c
index 3d4f1f32c7f6..fc341b69bf62 100644
--- a/arch/riscv/kernel/alternative.c
+++ b/arch/riscv/kernel/alternative.c
@@ -11,7 +11,9 @@
#include <linux/cpu.h>
#include <linux/uaccess.h>
#include <asm/alternative.h>
+#include <asm/module.h>
#include <asm/sections.h>
+#include <asm/vdso.h>
#include <asm/vendorid_list.h>
#include <asm/sbi.h>
#include <asm/csr.h>
@@ -160,6 +162,31 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin,
stage);
}
+#ifdef CONFIG_MMU
+static void __init apply_vdso_alternatives(void)
+{
+ const Elf_Ehdr *hdr;
+ const Elf_Shdr *shdr;
+ const Elf_Shdr *alt;
+ struct alt_entry *begin, *end;
+
+ hdr = (Elf_Ehdr *)vdso_start;
+ shdr = (void *)hdr + hdr->e_shoff;
+ alt = find_section(hdr, shdr, ".alternative");
+ if (!alt)
+ return;
+
+ begin = (void *)hdr + alt->sh_offset,
+ end = (void *)hdr + alt->sh_offset + alt->sh_size,
+
+ _apply_alternatives((struct alt_entry *)begin,
+ (struct alt_entry *)end,
+ RISCV_ALTERNATIVES_BOOT);
+}
+#else
+static void __init apply_vdso_alternatives(void) { }
+#endif
+
void __init apply_boot_alternatives(void)
{
/* If called on non-boot cpu things could go wrong */
@@ -168,6 +195,8 @@ void __init apply_boot_alternatives(void)
_apply_alternatives((struct alt_entry *)__alt_start,
(struct alt_entry *)__alt_end,
RISCV_ALTERNATIVES_BOOT);
+
+ apply_vdso_alternatives();
}
/*
diff --git a/arch/riscv/kernel/vdso.c b/arch/riscv/kernel/vdso.c
index e410275918ac..4e631c098f4d 100644
--- a/arch/riscv/kernel/vdso.c
+++ b/arch/riscv/kernel/vdso.c
@@ -22,11 +22,6 @@ struct vdso_data {
};
#endif
-extern char vdso_start[], vdso_end[];
-#ifdef CONFIG_COMPAT
-extern char compat_vdso_start[], compat_vdso_end[];
-#endif
-
enum vvar_pages {
VVAR_DATA_PAGE_OFFSET,
VVAR_TIMENS_PAGE_OFFSET,
diff --git a/arch/riscv/kernel/vdso/vdso.lds.S b/arch/riscv/kernel/vdso/vdso.lds.S
index 150b1a572e61..4a0606633290 100644
--- a/arch/riscv/kernel/vdso/vdso.lds.S
+++ b/arch/riscv/kernel/vdso/vdso.lds.S
@@ -40,6 +40,13 @@ SECTIONS
. = 0x800;
.text : { *(.text .text.*) } :text
+ . = ALIGN(4);
+ .alternative : {
+ __alt_start = .;
+ *(.alternative)
+ __alt_end = .;
+ }
+
.data : {
*(.got.plt) *(.got)
*(.data .data.* .gnu.linkonce.d.*)
--
2.38.1
Generally, riscv ISA extensions are fixed for any specific hardware
platform, so a hart's features won't change after booting. This
chacteristic makes it straightforward to use a static branch to check
if a specific ISA extension is supported or not to optimize
performance.
However, some ISA extensions such as SVPBMT and ZICBOM are handled
via. the alternative sequences.
Basically, for ease of maintenance, we prefer to use static branches
in C code, but recently, Samuel found that the static branch usage in
cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
Samuel pointed out, "Having a static branch in cpu_relax() is
problematic because that function is widely inlined, including in some
quite complex functions like in the VDSO. A quick measurement shows
this static branch is responsible by itself for around 40% of the jump
table."
Samuel's findings pointed out one of a few downsides of static branches
usage in C code to handle ISA extensions detected at boot time:
static branch's metadata in the __jump_table section, which is not
discarded after ISA extensions are finalized, wastes some space.
I want to try to solve the issue for all possible dynamic handling of
ISA extensions at boot time. Inspired by Mark[2], this patch introduces
riscv_has_extension_*() helpers, which work like static branches but
are patched using alternatives, thus the metadata can be freed after
patching.
Link: https://lore.kernel.org/linux-riscv/[email protected]/ [1]
Link: https://lore.kernel.org/linux-arm-kernel/[email protected]/ [2]
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Acked-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 37 ++++++++++++++++++++++++++++++++++
1 file changed, 37 insertions(+)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 8e0ee841fa77..411ef0fb5c4b 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -8,6 +8,7 @@
#ifndef _ASM_RISCV_HWCAP_H
#define _ASM_RISCV_HWCAP_H
+#include <asm/alternative-macros.h>
#include <asm/errno.h>
#include <linux/bits.h>
#include <uapi/asm/hwcap.h>
@@ -99,6 +100,42 @@ static __always_inline int riscv_isa_ext2key(int num)
}
}
+static __always_inline bool
+riscv_has_extension_likely(const unsigned long ext)
+{
+ compiletime_assert(ext < RISCV_ISA_EXT_MAX,
+ "ext must be < RISCV_ISA_EXT_MAX");
+
+ asm_volatile_goto(
+ ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1)
+ :
+ : [ext] "i" (ext)
+ :
+ : l_no);
+
+ return true;
+l_no:
+ return false;
+}
+
+static __always_inline bool
+riscv_has_extension_unlikely(const unsigned long ext)
+{
+ compiletime_assert(ext < RISCV_ISA_EXT_MAX,
+ "ext must be < RISCV_ISA_EXT_MAX");
+
+ asm_volatile_goto(
+ ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1)
+ :
+ : [ext] "i" (ext)
+ :
+ : l_yes);
+
+ return false;
+l_yes:
+ return true;
+}
+
unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext)
--
2.38.1
From: Andrew Jones <[email protected]>
Switch has_svinval() from static branch to the new helper
riscv_has_extension_unlikely().
Signed-off-by: Andrew Jones <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
Acked-by: Anup Patel <[email protected]>
---
arch/riscv/kvm/tlb.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/riscv/kvm/tlb.c b/arch/riscv/kvm/tlb.c
index 309d79b3e5cd..aa3da18ad873 100644
--- a/arch/riscv/kvm/tlb.c
+++ b/arch/riscv/kvm/tlb.c
@@ -15,8 +15,7 @@
#include <asm/hwcap.h>
#include <asm/insn-def.h>
-#define has_svinval() \
- static_branch_unlikely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_SVINVAL])
+#define has_svinval() riscv_has_extension_unlikely(RISCV_ISA_EXT_SVINVAL)
void kvm_riscv_local_hfence_gvma_vmid_gpa(unsigned long vmid,
gpa_t gpa, gpa_t gpsz,
--
2.38.1
Switch has_fpu() from static branch to the new helper
riscv_has_extension_likely().
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/include/asm/switch_to.h | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
index 11463489fec6..60f8ca01d36e 100644
--- a/arch/riscv/include/asm/switch_to.h
+++ b/arch/riscv/include/asm/switch_to.h
@@ -59,7 +59,8 @@ static inline void __switch_to_aux(struct task_struct *prev,
static __always_inline bool has_fpu(void)
{
- return static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_FPU]);
+ return riscv_has_extension_likely(RISCV_ISA_EXT_f) ||
+ riscv_has_extension_likely(RISCV_ISA_EXT_d);
}
#else
static __always_inline bool has_fpu(void) { return false; }
--
2.38.1
All users have switched to riscv_has_extension_*, remove unused
definitions, vars and related setting code.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Heiko Stuebner <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
Reviewed-by: Guo Ren <[email protected]>
---
arch/riscv/include/asm/hwcap.h | 32 --------------------------------
arch/riscv/kernel/cpufeature.c | 9 ---------
2 files changed, 41 deletions(-)
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 411ef0fb5c4b..7936ae6f7bdf 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -61,20 +61,6 @@ enum {
extern unsigned long elf_hwcap;
-
-/*
- * This enum represents the logical ID for each RISC-V ISA extension static
- * keys. We can use static key to optimize code path if some ISA extensions
- * are available.
- * Entries are sorted alphabetically.
- */
-enum riscv_isa_ext_key {
- RISCV_ISA_EXT_KEY_FPU, /* For 'F' and 'D' */
- RISCV_ISA_EXT_KEY_SVINVAL,
- RISCV_ISA_EXT_KEY_ZIHINTPAUSE,
- RISCV_ISA_EXT_KEY_MAX,
-};
-
struct riscv_isa_ext_data {
/* Name of the extension displayed to userspace via /proc/cpuinfo */
char uprop[RISCV_ISA_EXT_NAME_LEN_MAX];
@@ -82,24 +68,6 @@ struct riscv_isa_ext_data {
unsigned int isa_ext_id;
};
-extern struct static_key_false riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_MAX];
-
-static __always_inline int riscv_isa_ext2key(int num)
-{
- switch (num) {
- case RISCV_ISA_EXT_f:
- return RISCV_ISA_EXT_KEY_FPU;
- case RISCV_ISA_EXT_d:
- return RISCV_ISA_EXT_KEY_FPU;
- case RISCV_ISA_EXT_SVINVAL:
- return RISCV_ISA_EXT_KEY_SVINVAL;
- case RISCV_ISA_EXT_ZIHINTPAUSE:
- return RISCV_ISA_EXT_KEY_ZIHINTPAUSE;
- default:
- return -EINVAL;
- }
-}
-
static __always_inline bool
riscv_has_extension_likely(const unsigned long ext)
{
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 18ea518f9e68..a4f737bc7530 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -29,9 +29,6 @@ unsigned long elf_hwcap __read_mostly;
/* Host ISA bitmap */
static DECLARE_BITMAP(riscv_isa, RISCV_ISA_EXT_MAX) __read_mostly;
-DEFINE_STATIC_KEY_ARRAY_FALSE(riscv_isa_ext_keys, RISCV_ISA_EXT_KEY_MAX);
-EXPORT_SYMBOL(riscv_isa_ext_keys);
-
/**
* riscv_isa_extension_base() - Get base extension word
*
@@ -267,12 +264,6 @@ void __init riscv_fill_hwcap(void)
if (elf_hwcap & BIT_MASK(i))
print_str[j++] = (char)('a' + i);
pr_info("riscv: ELF capabilities %s\n", print_str);
-
- for_each_set_bit(i, riscv_isa, RISCV_ISA_EXT_MAX) {
- j = riscv_isa_ext2key(i);
- if (j >= 0)
- static_branch_enable(&riscv_isa_ext_keys[j]);
- }
}
#ifdef CONFIG_RISCV_ALTERNATIVE
--
2.38.1
Instead of using absolute addresses for both the old instrucions and
the alternative instructions, use offsets relative to the alt_entry
values. So this not only cuts the size of the alternative entry, but
also meets the prerequisite for patching alternatives in the vDSO,
since absolute alternative entries are subject to dynamic relocation,
which is incompatible with the vDSO building.
Signed-off-by: Jisheng Zhang <[email protected]>
Reviewed-by: Andrew Jones <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
arch/riscv/errata/sifive/errata.c | 3 ++-
arch/riscv/errata/thead/errata.c | 11 ++++++++---
arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++----------
arch/riscv/include/asm/alternative.h | 17 +++++++++++------
arch/riscv/kernel/cpufeature.c | 8 +++++---
5 files changed, 36 insertions(+), 23 deletions(-)
diff --git a/arch/riscv/errata/sifive/errata.c b/arch/riscv/errata/sifive/errata.c
index 1031038423e7..ef9a4eec0dba 100644
--- a/arch/riscv/errata/sifive/errata.c
+++ b/arch/riscv/errata/sifive/errata.c
@@ -107,7 +107,8 @@ void __init_or_module sifive_errata_patch_func(struct alt_entry *begin,
tmp = (1U << alt->errata_id);
if (cpu_req_errata & tmp) {
- patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
+ patch_text_nosync(ALT_OLD_PTR(alt), ALT_ALT_PTR(alt),
+ alt->alt_len);
cpu_apply_errata |= tmp;
}
}
diff --git a/arch/riscv/errata/thead/errata.c b/arch/riscv/errata/thead/errata.c
index fac5742d1c1e..c0bea5c94128 100644
--- a/arch/riscv/errata/thead/errata.c
+++ b/arch/riscv/errata/thead/errata.c
@@ -87,6 +87,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct al
struct alt_entry *alt;
u32 cpu_req_errata = thead_errata_probe(stage, archid, impid);
u32 tmp;
+ void *oldptr, *altptr;
for (alt = begin; alt < end; alt++) {
if (alt->vendor_id != THEAD_VENDOR_ID)
@@ -96,12 +97,16 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct al
tmp = (1U << alt->errata_id);
if (cpu_req_errata & tmp) {
+ oldptr = ALT_OLD_PTR(alt);
+ altptr = ALT_ALT_PTR(alt);
+
/* On vm-alternatives, the mmu isn't running yet */
if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
- memcpy((void *)__pa_symbol(alt->old_ptr),
- (void *)__pa_symbol(alt->alt_ptr), alt->alt_len);
+ memcpy((void *)__pa_symbol(oldptr),
+ (void *)__pa_symbol(altptr),
+ alt->alt_len);
else
- patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
+ patch_text_nosync(oldptr, altptr, alt->alt_len);
}
}
diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index 7226e2462584..cc6a81c00f2f 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -7,11 +7,11 @@
#ifdef __ASSEMBLY__
.macro ALT_ENTRY oldptr newptr vendor_id errata_id new_len
- RISCV_PTR \oldptr
- RISCV_PTR \newptr
- REG_ASM \vendor_id
- REG_ASM \new_len
- .word \errata_id
+ .4byte \oldptr - .
+ .4byte \newptr - .
+ .2byte \vendor_id
+ .2byte \new_len
+ .4byte \errata_id
.endm
.macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg
@@ -59,11 +59,11 @@
#include <linux/stringify.h>
#define ALT_ENTRY(oldptr, newptr, vendor_id, errata_id, newlen) \
- RISCV_PTR " " oldptr "\n" \
- RISCV_PTR " " newptr "\n" \
- REG_ASM " " vendor_id "\n" \
- REG_ASM " " newlen "\n" \
- ".word " errata_id "\n"
+ ".4byte ((" oldptr ") - .) \n" \
+ ".4byte ((" newptr ") - .) \n" \
+ ".2byte " vendor_id "\n" \
+ ".2byte " newlen "\n" \
+ ".4byte " errata_id "\n"
#define ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c) \
".if " __stringify(enable) " == 1\n" \
diff --git a/arch/riscv/include/asm/alternative.h b/arch/riscv/include/asm/alternative.h
index 1bd4027d34ca..b8648d4f2ac1 100644
--- a/arch/riscv/include/asm/alternative.h
+++ b/arch/riscv/include/asm/alternative.h
@@ -23,6 +23,11 @@
#define RISCV_ALTERNATIVES_MODULE 1 /* alternatives applied during module-init */
#define RISCV_ALTERNATIVES_EARLY_BOOT 2 /* alternatives applied before mmu start */
+/* add the relative offset to the address of the offset to get the absolute address */
+#define __ALT_PTR(a, f) ((void *)&(a)->f + (a)->f)
+#define ALT_OLD_PTR(a) __ALT_PTR(a, old_offset)
+#define ALT_ALT_PTR(a) __ALT_PTR(a, alt_offset)
+
void __init apply_boot_alternatives(void);
void __init apply_early_boot_alternatives(void);
void apply_module_alternatives(void *start, size_t length);
@@ -31,12 +36,12 @@ void riscv_alternative_fix_offsets(void *alt_ptr, unsigned int len,
int patch_offset);
struct alt_entry {
- void *old_ptr; /* address of original instruciton or data */
- void *alt_ptr; /* address of replacement instruction or data */
- unsigned long vendor_id; /* cpu vendor id */
- unsigned long alt_len; /* The replacement size */
- unsigned int errata_id; /* The errata id */
-} __packed;
+ s32 old_offset; /* offset relative to original instruction or data */
+ s32 alt_offset; /* offset relative to replacement instruction or data */
+ u16 vendor_id; /* cpu vendor id */
+ u16 alt_len; /* The replacement size */
+ u32 errata_id; /* The errata id */
+};
struct errata_checkfunc_id {
unsigned long vendor_id;
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 6193f401f0c5..18ea518f9e68 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -281,6 +281,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
unsigned int stage)
{
struct alt_entry *alt;
+ void *oldptr, *altptr;
if (stage == RISCV_ALTERNATIVES_EARLY_BOOT)
return;
@@ -297,9 +298,10 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,
if (!__riscv_isa_extension_available(NULL, alt->errata_id))
continue;
- patch_text_nosync(alt->old_ptr, alt->alt_ptr, alt->alt_len);
- riscv_alternative_fix_offsets(alt->old_ptr, alt->alt_len,
- alt->old_ptr - alt->alt_ptr);
+ oldptr = ALT_OLD_PTR(alt);
+ altptr = ALT_ALT_PTR(alt);
+ patch_text_nosync(oldptr, altptr, alt->alt_len);
+ riscv_alternative_fix_offsets(oldptr, alt->alt_len, oldptr - altptr);
}
}
#endif
--
2.38.1
On Sun, 29 Jan 2023 01:28:43 +0800, Jisheng Zhang wrote:
> Generally, riscv ISA extensions are fixed for any specific hardware
> platform, so a hart's features won't change after booting, this
> chacteristic makes it straightforward to use a static branch to check
> a specific ISA extension is supported or not to optimize performance.
>
> However, some ISA extensions such as SVPBMT and ZICBOM are handled
> via. the alternative sequences.
>
> [...]
Applied, thanks!
[01/13] riscv: move riscv_noncoherent_supported() out of ZICBOM probe
https://git.kernel.org/palmer/c/abcc445acdbe
[02/13] riscv: cpufeature: detect RISCV_ALTERNATIVES_EARLY_BOOT earlier
https://git.kernel.org/palmer/c/191b27c7c0e8
[03/13] riscv: hwcap: make ISA extension ids can be used in asm
https://git.kernel.org/palmer/c/d8a3d8a75206
[04/13] riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA extensions
https://git.kernel.org/palmer/c/4bf8860760d9
[05/13] riscv: introduce riscv_has_extension_[un]likely()
https://git.kernel.org/palmer/c/bdda5d554e43
[06/13] riscv: fpu: switch has_fpu() to riscv_has_extension_likely()
https://git.kernel.org/palmer/c/702e64550b12
[07/13] riscv: module: move find_section to module.h
https://git.kernel.org/palmer/c/e0c267e03b0c
[08/13] riscv: module: Add ADD16 and SUB16 rela types
https://git.kernel.org/palmer/c/1bc400ffb52b
[09/13] riscv: switch to relative alternative entries
https://git.kernel.org/palmer/c/8d23e94a4433
[10/13] riscv: alternative: patch alternatives in the vDSO
https://git.kernel.org/palmer/c/cabfd146b371
[11/13] riscv: cpu_relax: switch to riscv_has_extension_likely()
https://git.kernel.org/palmer/c/95bc69a47be2
[12/13] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
https://git.kernel.org/palmer/c/e8ad17d2b5f3
[13/13] riscv: remove riscv_isa_ext_keys[] array and related usage
https://git.kernel.org/palmer/c/03966594e117
Best regards,
--
Palmer Dabbelt <[email protected]>
Hello:
This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <[email protected]>:
On Sun, 29 Jan 2023 01:28:43 +0800 you wrote:
> Generally, riscv ISA extensions are fixed for any specific hardware
> platform, so a hart's features won't change after booting, this
> chacteristic makes it straightforward to use a static branch to check
> a specific ISA extension is supported or not to optimize performance.
>
> However, some ISA extensions such as SVPBMT and ZICBOM are handled
> via. the alternative sequences.
>
> [...]
Here is the summary with links:
- [v5,01/13] riscv: move riscv_noncoherent_supported() out of ZICBOM probe
https://git.kernel.org/riscv/c/abcc445acdbe
- [v5,02/13] riscv: cpufeature: detect RISCV_ALTERNATIVES_EARLY_BOOT earlier
https://git.kernel.org/riscv/c/191b27c7c0e8
- [v5,03/13] riscv: hwcap: make ISA extension ids can be used in asm
https://git.kernel.org/riscv/c/d8a3d8a75206
- [v5,04/13] riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA extensions
https://git.kernel.org/riscv/c/4bf8860760d9
- [v5,05/13] riscv: introduce riscv_has_extension_[un]likely()
https://git.kernel.org/riscv/c/bdda5d554e43
- [v5,06/13] riscv: fpu: switch has_fpu() to riscv_has_extension_likely()
https://git.kernel.org/riscv/c/702e64550b12
- [v5,07/13] riscv: module: move find_section to module.h
https://git.kernel.org/riscv/c/e0c267e03b0c
- [v5,08/13] riscv: module: Add ADD16 and SUB16 rela types
https://git.kernel.org/riscv/c/1bc400ffb52b
- [v5,09/13] riscv: switch to relative alternative entries
https://git.kernel.org/riscv/c/8d23e94a4433
- [v5,10/13] riscv: alternative: patch alternatives in the vDSO
https://git.kernel.org/riscv/c/cabfd146b371
- [v5,11/13] riscv: cpu_relax: switch to riscv_has_extension_likely()
https://git.kernel.org/riscv/c/95bc69a47be2
- [v5,12/13] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
https://git.kernel.org/riscv/c/e8ad17d2b5f3
- [v5,13/13] riscv: remove riscv_isa_ext_keys[] array and related usage
https://git.kernel.org/riscv/c/03966594e117
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hi,
On Sun, Jan 29, 2023 at 01:28:43AM +0800, Jisheng Zhang wrote:
> Generally, riscv ISA extensions are fixed for any specific hardware
> platform, so a hart's features won't change after booting, this
> chacteristic makes it straightforward to use a static branch to check
> a specific ISA extension is supported or not to optimize performance.
>
> However, some ISA extensions such as SVPBMT and ZICBOM are handled
> via. the alternative sequences.
>
> Basically, for ease of maintenance, we prefer to use static branches
> in C code, but recently, Samuel found that the static branch usage in
> cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
> Samuel pointed out, "Having a static branch in cpu_relax() is
> problematic because that function is widely inlined, including in some
> quite complex functions like in the VDSO. A quick measurement shows
> this static branch is responsible by itself for around 40% of the jump
> table."
>
> Samuel's findings pointed out one of a few downsides of static branches
> usage in C code to handle ISA extensions detected at boot time:
> static branch's metadata in the __jump_table section, which is not
> discarded after ISA extensions are finalized, wastes some space.
>
> I want to try to solve the issue for all possible dynamic handling of
> ISA extensions at boot time. Inspired by Mark[2], this patch introduces
> riscv_has_extension_*() helpers, which work like static branches but
> are patched using alternatives, thus the metadata can be freed after
> patching.
>
This patch series results in boot failures when trying to boot the
qemu sifive_u emulation. There are many log messages along the line of
[ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x222/0x2f6
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G W 6.2.0-rc7-next-20230210 #1
[ 0.000000] Hardware name: SiFive HiFive Unleashed A00 (DT)
[ 0.000000] epc : patch_insn_write+0x222/0x2f6
[ 0.000000] ra : patch_insn_write+0x21e/0x2f6
[ 0.000000] epc : ffffffff800068c6 ra : ffffffff800068c2 sp : ffffffff81803df0
[ 0.000000] gp : ffffffff81a1ab78 tp : ffffffff81814f80 t0 : ffffffffffffe000
[ 0.000000] t1 : 0000000000000001 t2 : 4c45203a76637369 s0 : ffffffff81803e40
[ 0.000000] s1 : 0000000000000004 a0 : 0000000000000000 a1 : ffffffffffffffff
[ 0.000000] a2 : 0000000000000004 a3 : 0000000000000000 a4 : 0000000000000001
[ 0.000000] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000052464e43
[ 0.000000] s2 : ffffffff80b4889c s3 : 000000000000082c s4 : ffffffff80b48828
[ 0.000000] s5 : 0000000000000828 s6 : ffffffff8131a0a0 s7 : 0000000000000fff
[ 0.000000] s8 : 0000000008000200 s9 : ffffffff8131a520 s10: 0000000000000018
[ 0.000000] s11: 000000000000000b t3 : 0000000000000001 t4 : 000000000000000d
[ 0.000000] t5 : ffffffffd8180000 t6 : ffffffff81803bc8
[ 0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[ 0.000000] [<ffffffff800068c6>] patch_insn_write+0x222/0x2f6
[ 0.000000] [<ffffffff80006a36>] patch_text_nosync+0xc/0x2a
[ 0.000000] [<ffffffff80003b86>] riscv_cpufeature_patch_func+0x52/0x98
[ 0.000000] [<ffffffff80003348>] _apply_alternatives+0x46/0x86
[ 0.000000] [<ffffffff80c02d36>] apply_boot_alternatives+0x3c/0xfa
[ 0.000000] [<ffffffff80c03ad8>] setup_arch+0x584/0x5b8
[ 0.000000] [<ffffffff80c0075a>] start_kernel+0xa2/0x8f8
[ 0.000000] irq event stamp: 0
[ 0.000000] hardirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] ------------[ cut here ]------------
then qemu hangs until the session is aborted.
Similar messages are also seen with the "virt" emulation, but there the boot
does not hang but fails to find a root device.
Guenter
---
bisect:
# bad: [6ba8a227fd19d19779005fb66ad7562608e1df83] Add linux-next specific files for 20230210
# good: [4ec5183ec48656cec489c49f989c508b68b518e3] Linux 6.2-rc7
git bisect start 'HEAD' 'v6.2-rc7'
# bad: [94613f0efc69ed41f9229ef5c294db3ec37145da] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
git bisect bad 94613f0efc69ed41f9229ef5c294db3ec37145da
# bad: [8928ece68de4371dc6e1d3336d3029c1e18ae3b4] Merge branch 'for_next' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
git bisect bad 8928ece68de4371dc6e1d3336d3029c1e18ae3b4
# good: [78a9f460e33d103256f3abb38f02f4759710c7dc] soc: document merges
git bisect good 78a9f460e33d103256f3abb38f02f4759710c7dc
# good: [b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
git bisect good b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619
# bad: [57a87a64b520c37dd49f5fde84d09a4adb391180] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git
git bisect bad 57a87a64b520c37dd49f5fde84d09a4adb391180
# good: [cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581] Merge branch 'clk-next' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
git bisect good cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581
# good: [6acecfa485d3de955c35a18730c106ddf1e7600e] powerpc/kcsan: Add KCSAN Support
git bisect good 6acecfa485d3de955c35a18730c106ddf1e7600e
# good: [8a16dea453dbc3e834b162640028e505882cd11e] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
git bisect good 8a16dea453dbc3e834b162640028e505882cd11e
# good: [6be1ff430dab9fc047762b10b2c9669399ea1f37] riscv: pgtable: Fixup comment for KERN_VIRT_SIZE
git bisect good 6be1ff430dab9fc047762b10b2c9669399ea1f37
# good: [e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f] riscv: module: move find_section to module.h
git bisect good e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f
# good: [e8ad17d2b5f38e595d597a3e2419d6d7cc727b17] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
git bisect good e8ad17d2b5f38e595d597a3e2419d6d7cc727b17
# good: [75ab93a244a516d1d3c03c4e27d5d0deff76ebfb] Merge patch series "Zbb string optimizations"
git bisect good 75ab93a244a516d1d3c03c4e27d5d0deff76ebfb
# bad: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
git bisect bad 9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd
# good: [03966594e1170303c037b0cded35c464a13a4a45] riscv: remove riscv_isa_ext_keys[] array and related usage
git bisect good 03966594e1170303c037b0cded35c464a13a4a45
# first bad commit: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
Hey Guenter,
On Sun, Feb 12, 2023 at 07:43:33AM -0800, Guenter Roeck wrote:
> On Sun, Jan 29, 2023 at 01:28:43AM +0800, Jisheng Zhang wrote:
> > Generally, riscv ISA extensions are fixed for any specific hardware
> > platform, so a hart's features won't change after booting, this
> > chacteristic makes it straightforward to use a static branch to check
> > a specific ISA extension is supported or not to optimize performance.
> >
> > However, some ISA extensions such as SVPBMT and ZICBOM are handled
> > via. the alternative sequences.
> >
> > Basically, for ease of maintenance, we prefer to use static branches
> > in C code, but recently, Samuel found that the static branch usage in
> > cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
> > Samuel pointed out, "Having a static branch in cpu_relax() is
> > problematic because that function is widely inlined, including in some
> > quite complex functions like in the VDSO. A quick measurement shows
> > this static branch is responsible by itself for around 40% of the jump
> > table."
> >
> > Samuel's findings pointed out one of a few downsides of static branches
> > usage in C code to handle ISA extensions detected at boot time:
> > static branch's metadata in the __jump_table section, which is not
> > discarded after ISA extensions are finalized, wastes some space.
> >
> > I want to try to solve the issue for all possible dynamic handling of
> > ISA extensions at boot time. Inspired by Mark[2], this patch introduces
> > riscv_has_extension_*() helpers, which work like static branches but
> > are patched using alternatives, thus the metadata can be freed after
> > patching.
> >
>
> This patch series results in boot failures when trying to boot the
> qemu sifive_u emulation. There are many log messages along the line of
>
> [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x222/0x2f6
> [ 0.000000] Modules linked in:
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G W 6.2.0-rc7-next-20230210 #1
> [ 0.000000] Hardware name: SiFive HiFive Unleashed A00 (DT)
> [ 0.000000] epc : patch_insn_write+0x222/0x2f6
> [ 0.000000] ra : patch_insn_write+0x21e/0x2f6
> [ 0.000000] epc : ffffffff800068c6 ra : ffffffff800068c2 sp : ffffffff81803df0
> [ 0.000000] gp : ffffffff81a1ab78 tp : ffffffff81814f80 t0 : ffffffffffffe000
> [ 0.000000] t1 : 0000000000000001 t2 : 4c45203a76637369 s0 : ffffffff81803e40
> [ 0.000000] s1 : 0000000000000004 a0 : 0000000000000000 a1 : ffffffffffffffff
> [ 0.000000] a2 : 0000000000000004 a3 : 0000000000000000 a4 : 0000000000000001
> [ 0.000000] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000052464e43
> [ 0.000000] s2 : ffffffff80b4889c s3 : 000000000000082c s4 : ffffffff80b48828
> [ 0.000000] s5 : 0000000000000828 s6 : ffffffff8131a0a0 s7 : 0000000000000fff
> [ 0.000000] s8 : 0000000008000200 s9 : ffffffff8131a520 s10: 0000000000000018
> [ 0.000000] s11: 000000000000000b t3 : 0000000000000001 t4 : 000000000000000d
> [ 0.000000] t5 : ffffffffd8180000 t6 : ffffffff81803bc8
> [ 0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> [ 0.000000] [<ffffffff800068c6>] patch_insn_write+0x222/0x2f6
> [ 0.000000] [<ffffffff80006a36>] patch_text_nosync+0xc/0x2a
> [ 0.000000] [<ffffffff80003b86>] riscv_cpufeature_patch_func+0x52/0x98
> [ 0.000000] [<ffffffff80003348>] _apply_alternatives+0x46/0x86
> [ 0.000000] [<ffffffff80c02d36>] apply_boot_alternatives+0x3c/0xfa
> [ 0.000000] [<ffffffff80c03ad8>] setup_arch+0x584/0x5b8
> [ 0.000000] [<ffffffff80c0075a>] start_kernel+0xa2/0x8f8
> [ 0.000000] irq event stamp: 0
> [ 0.000000] hardirqs last enabled at (0): [<0000000000000000>] 0x0
> [ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
> [ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
> [ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
> [ 0.000000] ---[ end trace 0000000000000000 ]---
> [ 0.000000] ------------[ cut here ]------------
>
> then qemu hangs until the session is aborted.
I have come across the same issue on PolarFire SoC while looking at
Samuel's fixes for Zbb & the D1:
https://lore.kernel.org/all/[email protected]/
Boot over NFS still works, which I think points to a hole in how my CI
is operating - it assumed the completed boot + bootrr being happy to
mean that there was nothing wrong!
On the VisionFive 2 & D1 Nezha I don't see these splats though.
It appears to be config related as the config I build for Icicle sees
these splats in QEMU but the D1 config does not.
I'll go do some digging!
>
> Similar messages are also seen with the "virt" emulation, but there the boot
> does not hang but fails to find a root device.
>
> Guenter
>
>
>
> ---
> bisect:
>
> # bad: [6ba8a227fd19d19779005fb66ad7562608e1df83] Add linux-next specific files for 20230210
> # good: [4ec5183ec48656cec489c49f989c508b68b518e3] Linux 6.2-rc7
> git bisect start 'HEAD' 'v6.2-rc7'
> # bad: [94613f0efc69ed41f9229ef5c294db3ec37145da] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> git bisect bad 94613f0efc69ed41f9229ef5c294db3ec37145da
> # bad: [8928ece68de4371dc6e1d3336d3029c1e18ae3b4] Merge branch 'for_next' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
> git bisect bad 8928ece68de4371dc6e1d3336d3029c1e18ae3b4
> # good: [78a9f460e33d103256f3abb38f02f4759710c7dc] soc: document merges
> git bisect good 78a9f460e33d103256f3abb38f02f4759710c7dc
> # good: [b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
> git bisect good b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619
> # bad: [57a87a64b520c37dd49f5fde84d09a4adb391180] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git
> git bisect bad 57a87a64b520c37dd49f5fde84d09a4adb391180
> # good: [cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581] Merge branch 'clk-next' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
> git bisect good cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581
> # good: [6acecfa485d3de955c35a18730c106ddf1e7600e] powerpc/kcsan: Add KCSAN Support
> git bisect good 6acecfa485d3de955c35a18730c106ddf1e7600e
> # good: [8a16dea453dbc3e834b162640028e505882cd11e] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
> git bisect good 8a16dea453dbc3e834b162640028e505882cd11e
> # good: [6be1ff430dab9fc047762b10b2c9669399ea1f37] riscv: pgtable: Fixup comment for KERN_VIRT_SIZE
> git bisect good 6be1ff430dab9fc047762b10b2c9669399ea1f37
> # good: [e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f] riscv: module: move find_section to module.h
> git bisect good e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f
> # good: [e8ad17d2b5f38e595d597a3e2419d6d7cc727b17] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
> git bisect good e8ad17d2b5f38e595d597a3e2419d6d7cc727b17
> # good: [75ab93a244a516d1d3c03c4e27d5d0deff76ebfb] Merge patch series "Zbb string optimizations"
> git bisect good 75ab93a244a516d1d3c03c4e27d5d0deff76ebfb
> # bad: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
> git bisect bad 9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd
> # good: [03966594e1170303c037b0cded35c464a13a4a45] riscv: remove riscv_isa_ext_keys[] array and related usage
> git bisect good 03966594e1170303c037b0cded35c464a13a4a45
> # first bad commit: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 07:43:33AM -0800, Guenter Roeck wrote:
> > On Sun, Jan 29, 2023 at 01:28:43AM +0800, Jisheng Zhang wrote:
> > > Generally, riscv ISA extensions are fixed for any specific hardware
> > > platform, so a hart's features won't change after booting, this
> > > chacteristic makes it straightforward to use a static branch to check
> > > a specific ISA extension is supported or not to optimize performance.
> > >
> > > However, some ISA extensions such as SVPBMT and ZICBOM are handled
> > > via. the alternative sequences.
> > >
> > > Basically, for ease of maintenance, we prefer to use static branches
> > > in C code, but recently, Samuel found that the static branch usage in
> > > cpu_relax() breaks building with CONFIG_CC_OPTIMIZE_FOR_SIZE[1]. As
> > > Samuel pointed out, "Having a static branch in cpu_relax() is
> > > problematic because that function is widely inlined, including in some
> > > quite complex functions like in the VDSO. A quick measurement shows
> > > this static branch is responsible by itself for around 40% of the jump
> > > table."
> > >
> > > Samuel's findings pointed out one of a few downsides of static branches
> > > usage in C code to handle ISA extensions detected at boot time:
> > > static branch's metadata in the __jump_table section, which is not
> > > discarded after ISA extensions are finalized, wastes some space.
> > >
> > > I want to try to solve the issue for all possible dynamic handling of
> > > ISA extensions at boot time. Inspired by Mark[2], this patch introduces
> > > riscv_has_extension_*() helpers, which work like static branches but
> > > are patched using alternatives, thus the metadata can be freed after
> > > patching.
> > >
> >
> > This patch series results in boot failures when trying to boot the
> > qemu sifive_u emulation. There are many log messages along the line of
> >
> > [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x222/0x2f6
> > [ 0.000000] Modules linked in:
> > [ 0.000000] CPU: 0 PID: 0 Comm: swapper Tainted: G W 6.2.0-rc7-next-20230210 #1
> > [ 0.000000] Hardware name: SiFive HiFive Unleashed A00 (DT)
> > [ 0.000000] epc : patch_insn_write+0x222/0x2f6
> > [ 0.000000] ra : patch_insn_write+0x21e/0x2f6
> > [ 0.000000] epc : ffffffff800068c6 ra : ffffffff800068c2 sp : ffffffff81803df0
> > [ 0.000000] gp : ffffffff81a1ab78 tp : ffffffff81814f80 t0 : ffffffffffffe000
> > [ 0.000000] t1 : 0000000000000001 t2 : 4c45203a76637369 s0 : ffffffff81803e40
> > [ 0.000000] s1 : 0000000000000004 a0 : 0000000000000000 a1 : ffffffffffffffff
> > [ 0.000000] a2 : 0000000000000004 a3 : 0000000000000000 a4 : 0000000000000001
> > [ 0.000000] a5 : 0000000000000000 a6 : 0000000000000000 a7 : 0000000052464e43
> > [ 0.000000] s2 : ffffffff80b4889c s3 : 000000000000082c s4 : ffffffff80b48828
> > [ 0.000000] s5 : 0000000000000828 s6 : ffffffff8131a0a0 s7 : 0000000000000fff
> > [ 0.000000] s8 : 0000000008000200 s9 : ffffffff8131a520 s10: 0000000000000018
> > [ 0.000000] s11: 000000000000000b t3 : 0000000000000001 t4 : 000000000000000d
> > [ 0.000000] t5 : ffffffffd8180000 t6 : ffffffff81803bc8
> > [ 0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
> > [ 0.000000] [<ffffffff800068c6>] patch_insn_write+0x222/0x2f6
> > [ 0.000000] [<ffffffff80006a36>] patch_text_nosync+0xc/0x2a
> > [ 0.000000] [<ffffffff80003b86>] riscv_cpufeature_patch_func+0x52/0x98
> > [ 0.000000] [<ffffffff80003348>] _apply_alternatives+0x46/0x86
> > [ 0.000000] [<ffffffff80c02d36>] apply_boot_alternatives+0x3c/0xfa
> > [ 0.000000] [<ffffffff80c03ad8>] setup_arch+0x584/0x5b8
> > [ 0.000000] [<ffffffff80c0075a>] start_kernel+0xa2/0x8f8
> > [ 0.000000] irq event stamp: 0
> > [ 0.000000] hardirqs last enabled at (0): [<0000000000000000>] 0x0
> > [ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
> > [ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
> > [ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
> > [ 0.000000] ---[ end trace 0000000000000000 ]---
> > [ 0.000000] ------------[ cut here ]------------
> >
> > then qemu hangs until the session is aborted.
Hmm, so this appears to be us attempting to patch in alternatives where
none actually exists - seemingly F & D.
>
> I have come across the same issue on PolarFire SoC while looking at
> Samuel's fixes for Zbb & the D1:
> https://lore.kernel.org/all/[email protected]/
>
> Boot over NFS still works, which I think points to a hole in how my CI
> is operating - it assumed the completed boot + bootrr being happy to
> mean that there was nothing wrong!
>
> On the VisionFive 2 & D1 Nezha I don't see these splats though.
> It appears to be config related as the config I build for Icicle sees
> these splats in QEMU but the D1 config does not.
> I'll go do some digging!
>
> >
> > Similar messages are also seen with the "virt" emulation, but there the boot
> > does not hang but fails to find a root device.
> >
> > Guenter
> >
> >
> >
> > ---
> > bisect:
> >
> > # bad: [6ba8a227fd19d19779005fb66ad7562608e1df83] Add linux-next specific files for 20230210
> > # good: [4ec5183ec48656cec489c49f989c508b68b518e3] Linux 6.2-rc7
> > git bisect start 'HEAD' 'v6.2-rc7'
> > # bad: [94613f0efc69ed41f9229ef5c294db3ec37145da] Merge branch 'master' of git://git.kernel.org/pub/scm/linux/kernel/git/herbert/cryptodev-2.6.git
> > git bisect bad 94613f0efc69ed41f9229ef5c294db3ec37145da
> > # bad: [8928ece68de4371dc6e1d3336d3029c1e18ae3b4] Merge branch 'for_next' of git://git.kernel.org/pub/scm/linux/kernel/git/jack/linux-fs.git
> > git bisect bad 8928ece68de4371dc6e1d3336d3029c1e18ae3b4
> > # good: [78a9f460e33d103256f3abb38f02f4759710c7dc] soc: document merges
> > git bisect good 78a9f460e33d103256f3abb38f02f4759710c7dc
> > # good: [b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git
> > git bisect good b35b2472ebafa29d0bbe79fbee1da6ef3c4ec619
> > # bad: [57a87a64b520c37dd49f5fde84d09a4adb391180] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/riscv/linux.git
> > git bisect bad 57a87a64b520c37dd49f5fde84d09a4adb391180
> > # good: [cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581] Merge branch 'clk-next' of git://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
> > git bisect good cfc8ba01cc84b24ec6eb293ec9fba893f7cd4581
> > # good: [6acecfa485d3de955c35a18730c106ddf1e7600e] powerpc/kcsan: Add KCSAN Support
> > git bisect good 6acecfa485d3de955c35a18730c106ddf1e7600e
> > # good: [8a16dea453dbc3e834b162640028e505882cd11e] Merge branch 'next' of git://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git
> > git bisect good 8a16dea453dbc3e834b162640028e505882cd11e
> > # good: [6be1ff430dab9fc047762b10b2c9669399ea1f37] riscv: pgtable: Fixup comment for KERN_VIRT_SIZE
> > git bisect good 6be1ff430dab9fc047762b10b2c9669399ea1f37
> > # good: [e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f] riscv: module: move find_section to module.h
> > git bisect good e0c267e03b0c77c9ac79ac08eada41ba8eb1b95f
> > # good: [e8ad17d2b5f38e595d597a3e2419d6d7cc727b17] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
> > git bisect good e8ad17d2b5f38e595d597a3e2419d6d7cc727b17
> > # good: [75ab93a244a516d1d3c03c4e27d5d0deff76ebfb] Merge patch series "Zbb string optimizations"
> > git bisect good 75ab93a244a516d1d3c03c4e27d5d0deff76ebfb
> > # bad: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
> > git bisect bad 9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd
> > # good: [03966594e1170303c037b0cded35c464a13a4a45] riscv: remove riscv_isa_ext_keys[] array and related usage
> > git bisect good 03966594e1170303c037b0cded35c464a13a4a45
> > # first bad commit: [9daca9a5b9ac35361ce2d8d5ec10b19b7048d6cd] Merge patch series "riscv: improve boot time isa extensions handling"
On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
So as not to lead anyone up the garden path, let me correct myself:
> Hmm, so this appears to be us attempting to patch in alternatives where
> none actually exists - seemingly F & D.
And of course that's not true, riscv_has_extension_likely() now uses
alternatives as of:
bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
From a quick look, it just happens that the only users are F & D.
On Sun, Feb 12, 2023 at 05:06:09PM +0000, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
> > On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
>
> So as not to lead anyone up the garden path, let me correct myself:
>
> > Hmm, so this appears to be us attempting to patch in alternatives where
> > none actually exists - seemingly F & D.
>
> And of course that's not true, riscv_has_extension_likely() now uses
> alternatives as of:
> bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
>
> From a quick look, it just happens that the only users are F & D.
>
Samuel pointed out that this is a lockdep splat on irc.
There's a patch on the list that removes the lockdep annotation
entirely:
https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
So ye, no surprises that it was config based!
Palmer posted a "better" fix for that lockdep warning a while ago:
https://lore.kernel.org/all/[email protected]/
So we'd have to duplicate/reuse that for cpufeature/errata patching.
On 2/12/23 10:06, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 05:06:09PM +0000, Conor Dooley wrote:
>> On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
>>> On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
>>
>> So as not to lead anyone up the garden path, let me correct myself:
>>
>>> Hmm, so this appears to be us attempting to patch in alternatives where
>>> none actually exists - seemingly F & D.
>>
>> And of course that's not true, riscv_has_extension_likely() now uses
>> alternatives as of:
>> bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
>>
>> From a quick look, it just happens that the only users are F & D.
>>
>
> Samuel pointed out that this is a lockdep splat on irc.
> There's a patch on the list that removes the lockdep annotation
> entirely:
> https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
>
> So ye, no surprises that it was config based!
>
> Palmer posted a "better" fix for that lockdep warning a while ago:
> https://lore.kernel.org/all/[email protected]/
>
> So we'd have to duplicate/reuse that for cpufeature/errata patching.
>
>
This does not (only) happen in stop_machine().
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:63 patch_insn_write+0x222/0x2f6
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper Not tainted 6.2.0-rc7-next-20230210 #1
[ 0.000000] Hardware name: riscv-virtio,qemu (DT)
[ 0.000000] epc : patch_insn_write+0x222/0x2f6
[ 0.000000] ra : patch_insn_write+0x21e/0x2f6
[ 0.000000] epc : ffffffff800068c6 ra : ffffffff800068c2 sp : ffffffff81803df0
[ 0.000000] gp : ffffffff81a1ab78 tp : ffffffff81814f80 t0 : ffffffffffffe000
[ 0.000000] t1 : fffffffffafdfb03 t2 : 4c45203a76637369 s0 : ffffffff81803e40
[ 0.000000] s1 : 0000000000000004 a0 : 0000000000000000 a1 : ffffffffffffffff
[ 0.000000] a2 : 0000000000000004 a3 : 0000000000000000 a4 : 0000000000000001
[ 0.000000] a5 : 0000000000000000 a6 : 0000000000000006 a7 : 0000000000000010
[ 0.000000] s2 : ffffffff8000431a s3 : 00000000000000b2 s4 : ffffffff800040ae
[ 0.000000] s5 : 00000000000000ae s6 : ffffffff8131a0a0 s7 : 0000000000000fff
[ 0.000000] s8 : 0000000008000200 s9 : ffffffff8131a520 s10: 0000000000000018
[ 0.000000] s11: 0000000000000008 t3 : 0000000022e125d2 t4 : 000000000000000d
[ 0.000000] t5 : ffffffffd8180000 t6 : ffffffff81803bc8
[ 0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[ 0.000000] [<ffffffff800068c6>] patch_insn_write+0x222/0x2f6
[ 0.000000] [<ffffffff80006a36>] patch_text_nosync+0xc/0x2a
[ 0.000000] [<ffffffff80003b86>] riscv_cpufeature_patch_func+0x52/0x98
[ 0.000000] [<ffffffff80003348>] _apply_alternatives+0x46/0x86
[ 0.000000] [<ffffffff80c02d36>] apply_boot_alternatives+0x3c/0xfa
[ 0.000000] [<ffffffff80c03ad8>] setup_arch+0x584/0x5b8
[ 0.000000] [<ffffffff80c0075a>] start_kernel+0xa2/0x8f8
[ 0.000000] irq event stamp: 0
[ 0.000000] hardirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] ---[ end trace 0000000000000000 ]---
Guenter
On Sun, Feb 12, 2023 at 10:14:13AM -0800, Guenter Roeck wrote:
> On 2/12/23 10:06, Conor Dooley wrote:
> > On Sun, Feb 12, 2023 at 05:06:09PM +0000, Conor Dooley wrote:
> > > On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
> > > > On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
> > >
> > > So as not to lead anyone up the garden path, let me correct myself:
> > >
> > > > Hmm, so this appears to be us attempting to patch in alternatives where
> > > > none actually exists - seemingly F & D.
> > >
> > > And of course that's not true, riscv_has_extension_likely() now uses
> > > alternatives as of:
> > > bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
> > >
> > > From a quick look, it just happens that the only users are F & D.
> > >
> >
> > Samuel pointed out that this is a lockdep splat on irc.
> > There's a patch on the list that removes the lockdep annotation
> > entirely:
> > https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
> >
> > So ye, no surprises that it was config based!
> >
> > Palmer posted a "better" fix for that lockdep warning a while ago:
> > https://lore.kernel.org/all/[email protected]/
> >
> > So we'd have to duplicate/reuse that for cpufeature/errata patching.
> >
> >
>
> This does not (only) happen in stop_machine().
Yah, sorry I meant that it's the same lockdep splat as is being
addressed there.
The first patch deletes the lockdep stuff entirely, so removes the
splat. I was thinking that we'd need to take Palmer's (IMO better)
patch and do the same thing for patching alternatives, but I figure we
can just take the text_mutex itself for alternatives & not have to
dance around the lock.
I'll go do that I suppose!
On 2/12/23 10:20, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 10:14:13AM -0800, Guenter Roeck wrote:
>> On 2/12/23 10:06, Conor Dooley wrote:
>>> On Sun, Feb 12, 2023 at 05:06:09PM +0000, Conor Dooley wrote:
>>>> On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
>>>>> On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
>>>>
>>>> So as not to lead anyone up the garden path, let me correct myself:
>>>>
>>>>> Hmm, so this appears to be us attempting to patch in alternatives where
>>>>> none actually exists - seemingly F & D.
>>>>
>>>> And of course that's not true, riscv_has_extension_likely() now uses
>>>> alternatives as of:
>>>> bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
>>>>
>>>> From a quick look, it just happens that the only users are F & D.
>>>>
>>>
>>> Samuel pointed out that this is a lockdep splat on irc.
>>> There's a patch on the list that removes the lockdep annotation
>>> entirely:
>>> https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
>>>
>>> So ye, no surprises that it was config based!
>>>
>>> Palmer posted a "better" fix for that lockdep warning a while ago:
>>> https://lore.kernel.org/all/[email protected]/
>>>
>>> So we'd have to duplicate/reuse that for cpufeature/errata patching.
>>>
>>>
>>
>> This does not (only) happen in stop_machine().
>
> Yah, sorry I meant that it's the same lockdep splat as is being
> addressed there.
> The first patch deletes the lockdep stuff entirely, so removes the
> splat. I was thinking that we'd need to take Palmer's (IMO better)
> patch and do the same thing for patching alternatives, but I figure we
> can just take the text_mutex itself for alternatives & not have to
> dance around the lock.
>
> I'll go do that I suppose!
Thanks a lot for the clarification. That sounds like the backtrace
can be largely ignored. However, I still see that the patch series
results in boot hangs with the sifive_u qemu emulation, where
the log ends with "Oops - illegal instruction". Is that problem
being addressed as well ?
Thanks,
Guenter
On Sun, Feb 12, 2023 at 10:38:26AM -0800, Guenter Roeck wrote:
> On 2/12/23 10:20, Conor Dooley wrote:
> > On Sun, Feb 12, 2023 at 10:14:13AM -0800, Guenter Roeck wrote:
> > > On 2/12/23 10:06, Conor Dooley wrote:
> > > > On Sun, Feb 12, 2023 at 05:06:09PM +0000, Conor Dooley wrote:
> > > > > On Sun, Feb 12, 2023 at 04:33:58PM +0000, Conor Dooley wrote:
> > > > > > On Sun, Feb 12, 2023 at 03:59:59PM +0000, Conor Dooley wrote:
> > > > >
> > > > > So as not to lead anyone up the garden path, let me correct myself:
> > > > >
> > > > > > Hmm, so this appears to be us attempting to patch in alternatives where
> > > > > > none actually exists - seemingly F & D.
> > > > >
> > > > > And of course that's not true, riscv_has_extension_likely() now uses
> > > > > alternatives as of:
> > > > > bdda5d554e43 ("riscv: introduce riscv_has_extension_[un]likely()")
> > > > >
> > > > > From a quick look, it just happens that the only users are F & D.
> > > > >
> > > >
> > > > Samuel pointed out that this is a lockdep splat on irc.
> > > > There's a patch on the list that removes the lockdep annotation
> > > > entirely:
> > > > https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
> > > >
> > > > So ye, no surprises that it was config based!
> > > >
> > > > Palmer posted a "better" fix for that lockdep warning a while ago:
> > > > https://lore.kernel.org/all/[email protected]/
> > > >
> > > > So we'd have to duplicate/reuse that for cpufeature/errata patching.
> > > >
> > > >
> > >
> > > This does not (only) happen in stop_machine().
> >
> > Yah, sorry I meant that it's the same lockdep splat as is being
> > addressed there.
> > The first patch deletes the lockdep stuff entirely, so removes the
> > splat. I was thinking that we'd need to take Palmer's (IMO better)
> > patch and do the same thing for patching alternatives, but I figure we
> > can just take the text_mutex itself for alternatives & not have to
> > dance around the lock.
> >
> > I'll go do that I suppose!
>
> Thanks a lot for the clarification. That sounds like the backtrace
> can be largely ignored.
Yeah, sorry that I phrased that confusingly in the first place.
> However, I still see that the patch series
> results in boot hangs with the sifive_u qemu emulation, where
> the log ends with "Oops - illegal instruction". Is that problem
> being addressed as well ?
Hmm, if it died on the last commit in this series, then I am not sure.
If you meant with riscv/for-next or linux-next that's fixed by a patch
from Samuel:
https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
Cheers,
Conor.
On 2/12/23 10:45, Conor Dooley wrote:
...
>
>> However, I still see that the patch series
>> results in boot hangs with the sifive_u qemu emulation, where
>> the log ends with "Oops - illegal instruction". Is that problem
>> being addressed as well ?
>
> Hmm, if it died on the last commit in this series, then I am not sure.
> If you meant with riscv/for-next or linux-next that's fixed by a patch
> from Samuel:
> https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
>
It failed after the merge, so it looks like it may have been merge damage.
Anyway, I applied
RISC-V: Don't check text_mutex during stop_machine
riscv: Fix early alternative patching
riscv: Fix Zbb alternative IDs
and the sifive_u emulation no longer crashes. However, I still get
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:71 patch_insn_write+0x222/0x2f6
repeated several times.
I then also tested
riscv: patch: Fixup lockdep warning in stop_machine
riscv: Fix early alternative patching
riscv: Fix Zbb alternative IDs
which works fine (no warning backtrace) for sifive_u, but gives me
WARNING: CPU: 0 PID: 0 at kernel/trace/trace_events.c:433 trace_event_raw_init+0xde/0x642
and a whole lot of
event btrfs_clear_extent_bit has unsafe dereference of argument 1
and similar messages when running the "virt" emulation. That was there before,
but drowned in the noise. Ok, guess I'll need another round of bisect.
Guenter
On Sun, Feb 12, 2023 at 12:27:10PM -0800, Guenter Roeck wrote:
> On 2/12/23 10:45, Conor Dooley wrote:
> ...
> >
> > > However, I still see that the patch series
> > > results in boot hangs with the sifive_u qemu emulation, where
> > > the log ends with "Oops - illegal instruction". Is that problem
> > > being addressed as well ?
> >
> > Hmm, if it died on the last commit in this series, then I am not sure.
> > If you meant with riscv/for-next or linux-next that's fixed by a patch
> > from Samuel:
> > https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
> >
>
> It failed after the merge, so it looks like it may have been merge damage.
>
> Anyway, I applied
>
> RISC-V: Don't check text_mutex during stop_machine
That being:
https://lore.kernel.org/all/[email protected]/
Which handles the lockdep assertion during stop_machine...
> riscv: Fix early alternative patching
> riscv: Fix Zbb alternative IDs
>
> and the sifive_u emulation no longer crashes. However, I still get
>
> [ 0.000000] ------------[ cut here ]------------
> [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:71 patch_insn_write+0x222/0x2f6
...but doesn't prevent the early "spam" of assertion failures from the
code patching for alternatives. I sent a patch to take the lock during
the alternative patching which should get rid of them for you. It did
for me at least!
https://lore.kernel.org/all/[email protected]
> repeated several times.
>
> I then also tested
>
> riscv: patch: Fixup lockdep warning in stop_machine
This one just deletes the lockdep check, so I would expect it to remove
the complaints.
> riscv: Fix early alternative patching
> riscv: Fix Zbb alternative IDs
>
> which works fine (no warning backtrace) for sifive_u, but gives me
>
> WARNING: CPU: 0 PID: 0 at kernel/trace/trace_events.c:433 trace_event_raw_init+0xde/0x642
Hmm, do you have the full splat for this one handy?
> and a whole lot of
>
> event btrfs_clear_extent_bit has unsafe dereference of argument 1
>
> and similar messages when running the "virt" emulation. That was there before,
> but drowned in the noise. Ok, guess I'll need another round of bisect.
Thanks for all of your testing :)
On 2/12/23 12:39, Conor Dooley wrote:
> On Sun, Feb 12, 2023 at 12:27:10PM -0800, Guenter Roeck wrote:
>> On 2/12/23 10:45, Conor Dooley wrote:
>> ...
>>>
>>>> However, I still see that the patch series
>>>> results in boot hangs with the sifive_u qemu emulation, where
>>>> the log ends with "Oops - illegal instruction". Is that problem
>>>> being addressed as well ?
>>>
>>> Hmm, if it died on the last commit in this series, then I am not sure.
>>> If you meant with riscv/for-next or linux-next that's fixed by a patch
>>> from Samuel:
>>> https://patchwork.kernel.org/project/linux-riscv/patch/[email protected]/
>>>
>>
>> It failed after the merge, so it looks like it may have been merge damage.
>>
>> Anyway, I applied
>>
>> RISC-V: Don't check text_mutex during stop_machine
>
> That being:
> https://lore.kernel.org/all/[email protected]/
> Which handles the lockdep assertion during stop_machine...
>
>> riscv: Fix early alternative patching
>> riscv: Fix Zbb alternative IDs
>>
>> and the sifive_u emulation no longer crashes. However, I still get
>>
>> [ 0.000000] ------------[ cut here ]------------
>> [ 0.000000] WARNING: CPU: 0 PID: 0 at arch/riscv/kernel/patch.c:71 patch_insn_write+0x222/0x2f6
>
> ...but doesn't prevent the early "spam" of assertion failures from the
> code patching for alternatives. I sent a patch to take the lock during
> the alternative patching which should get rid of them for you. It did
> for me at least!
> https://lore.kernel.org/all/[email protected]
>
>> repeated several times.
>>
>> I then also tested
>>
>> riscv: patch: Fixup lockdep warning in stop_machine
>
> This one just deletes the lockdep check, so I would expect it to remove
> the complaints.
>
>> riscv: Fix early alternative patching
>> riscv: Fix Zbb alternative IDs
>>
>> which works fine (no warning backtrace) for sifive_u, but gives me
>>
>> WARNING: CPU: 0 PID: 0 at kernel/trace/trace_events.c:433 trace_event_raw_init+0xde/0x642
>
> Hmm, do you have the full splat for this one handy?
>
[ 0.000000] ------------[ cut here ]------------
[ 0.000000] WARNING: CPU: 0 PID: 0 at kernel/trace/trace_events.c:433 trace_event_raw_init+0xde/0x642
[ 0.000000] Modules linked in:
[ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Tainted: G W 6.2.0-rc7-next-20230210 #1
[ 0.000000] Hardware name: riscv-virtio,qemu (DT)
[ 0.000000] epc : trace_event_raw_init+0xde/0x642
[ 0.000000] ra : trace_event_raw_init+0x45a/0x642
[ 0.000000] epc : ffffffff8010571a ra : ffffffff80105a96 sp : ffffffff81803e60
[ 0.000000] gp : ffffffff81a1ab78 tp : ffffffff81814f80 t0 : 0000000000000000
[ 0.000000] t1 : 5245432d3e000000 t2 : 0000000000000000 s0 : ffffffff81803f20
[ 0.000000] s1 : 000000000000045f a0 : 0000000000000000 a1 : ffffffff81331ef0
[ 0.000000] a2 : 000000000000025c a3 : 0000000000000001 a4 : ffffffff801056fa
[ 0.000000] a5 : 000000000000002c a6 : ffffffff8192e4d8 a7 : ffffffff81157a90
[ 0.000000] s2 : 0000000000000000 s3 : ffffffff81922870 s4 : ffffffff8192e4d8
[ 0.000000] s5 : ffffffff81011c30 s6 : 000000000000000a s7 : 0000000000000021
[ 0.000000] s8 : 000000000000005c s9 : ffffffff81331ee8 s10: 0000000000000001
[ 0.000000] s11: 0000000000000000 t3 : 0000000000000007 t4 : 0000000000000070
[ 0.000000] t5 : 0000000000000025 t6 : 0000000000000009
[ 0.000000] status: 0000000200000100 badaddr: 0000000000000000 cause: 0000000000000003
[ 0.000000] [<ffffffff8010571a>] trace_event_raw_init+0xde/0x642
[ 0.000000] [<ffffffff80104d32>] event_init+0x28/0x84
[ 0.000000] [<ffffffff80c0f7ca>] trace_event_init+0x9e/0x2ae
[ 0.000000] [<ffffffff80c0f3a0>] trace_init+0x10/0x18
[ 0.000000] [<ffffffff80c00bc6>] start_kernel+0x50e/0x8f8
[ 0.000000] irq event stamp: 0
[ 0.000000] hardirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] hardirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last enabled at (0): [<0000000000000000>] 0x0
[ 0.000000] softirqs last disabled at (0): [<0000000000000000>] 0x0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] event btrfs_clear_extent_bit has unsafe dereference of argument 1
[ 0.000000] print_fmt: "%pU: io_tree=%s ino=%llu root=%llu start=%llu len=%llu clear_bits=%s", REC->fsid, __print_symbolic(REC->owner, {IO_TREE_FS_PINNED_EXTENTS, "PINNED_EXTENTS"}, {IO_TREE_FS_EXCLUDED_EXTENTS, "EXCLUDED_EXTENTS"}, {IO_TREE_BTREE_INODE_IO, "BTREE_INODE_IO"}, {IO_TREE_INODE_IO, "INODE_IO"}, {IO_TREE_RELOC_BLOCKS, "RELOC_BLOCKS"}, {IO_TREE_TRANS_DIRTY_PAGES, "TRANS_DIRTY_PAGES"}, {IO_TREE_ROOT_DIRTY_LOG_PAGES, "ROOT_DIRTY_LOG_PAGES"}, {IO_TREE_INODE_FILE_EXTENT, "INODE_FILE_EXTENT"}, {IO_TREE_LOG_CSUM_RANGE, "LOG_CSUM_RANGE"}, {IO_TREE_SELFTEST, "SELFTEST"}), REC->ino, REC->rootid, REC->start, REC->len, __print_flags(REC->clear_bits, "|", { EXTENT_DIRTY, "DIRTY"}, { EXTENT_UPTODATE, "UPTODATE"}, { EXTENT_LOCKED, "LOCKED"}, { EXTENT_NEW, "NEW"}, { EXTENT_DELALLOC, "DELALLOC"}, { EXTENT_DEFRAG, "DEFRAG"}, { EXTENT_BOUNDARY, "BOUNDARY"}, { EXTENT_NODATASUM, "NODATASUM"}, { EXTENT_CLEAR_META_RESV, "CLEAR_META_RESV"}, { EXTENT_NEED_WAIT, "NEED_WAIT"}, { EXTENT_NORESERVE,
"NORESERVE"}, { EXTENT_QGROUP_RESERV
[ 0.000000] event btrfs_ordered_sched has unsafe dereference of argument 1
[ 0.000000] print_fmt: "%pU: work=%p (normal_work=%p) wq=%p func=%ps ordered_func=%p ordered_free=%p", REC->fsid, REC->work, REC->normal_work, REC->wq, REC->func, REC->ordered_func, REC->ordered_free
[ 0.000000] event btrfs_work_sched has unsafe dereference of argument 1
[ 0.000000] print_fmt: "%pU: work=%p (normal_work=%p) wq=%p func=%ps ordered_func=%p ordered_free=%p", REC->fsid, REC->work, REC->normal_work, REC->wq, REC->func, REC->ordered_func, REC->ordered_free
[ 0.000000] event btrfs_work_queued has unsafe dereference of argument 1
[ 0.000000] print_fmt: "%pU: work=%p (normal_work=%p) wq=%p func=%ps ordered_func=%p ordered_free=%p", REC->fsid, REC->work, REC->normal_work, REC->wq, REC->func, REC->ordered_func, REC->ordered_free
[ 0.000000] event find_free_extent_search_loop has unsafe dereference of argument 1
and so on.
It bisects to "RISC-V: add zbb support to string functions", which also seems
to cause various boot failures. Unfortunately that patch is difficult to revert,
but marking TOOLCHAIN_HAS_ZBB as broken "fixes" it. I don't know if there is
a problem with the patch or with qemu. I'll disable RISCV_ISA_ZBB in my tests
for the time being to work around it.
Guenter
>> and a whole lot of
>>
>> event btrfs_clear_extent_bit has unsafe dereference of argument 1
>>
>> and similar messages when running the "virt" emulation. That was there before,
>> but drowned in the noise. Ok, guess I'll need another round of bisect.
>
> Thanks for all of your testing :)
>
Hi,
On Sun, Jan 29, 2023 at 01:28:49AM +0800, Jisheng Zhang wrote:
> Switch has_fpu() from static branch to the new helper
> riscv_has_extension_likely().
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> Reviewed-by: Andrew Jones <[email protected]>
> Reviewed-by: Heiko Stuebner <[email protected]>
> Reviewed-by: Conor Dooley <[email protected]>
> ---
> arch/riscv/include/asm/switch_to.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/switch_to.h b/arch/riscv/include/asm/switch_to.h
> index 11463489fec6..60f8ca01d36e 100644
> --- a/arch/riscv/include/asm/switch_to.h
> +++ b/arch/riscv/include/asm/switch_to.h
> @@ -59,7 +59,8 @@ static inline void __switch_to_aux(struct task_struct *prev,
>
> static __always_inline bool has_fpu(void)
> {
> - return static_branch_likely(&riscv_isa_ext_keys[RISCV_ISA_EXT_KEY_FPU]);
> + return riscv_has_extension_likely(RISCV_ISA_EXT_f) ||
> + riscv_has_extension_likely(RISCV_ISA_EXT_d);
This causes programs to crash on kernels that are compiled with
CONFIG_RISCV_ALTERNATIVE=n. Since CONFIG_RISCV_ALTERNATIVE isn't
selectable, this is a problem.
You can try this out for yourself using the WireGuard test suite:
ARCH=riscv64 make -C tools/testing/selftests/wireguard/qemu -j$(nproc)
And you'll see the crash:
[ 2.172093] init.sh[45]: unhandled signal 4 code 0x1 at 0x00ffffff945a2170 in libc.so[ffffff94562000+8c000]
[ 2.174306] CPU: 0 PID: 45 Comm: init.sh Not tainted 6.3.0-rc3+ #1
[ 2.174981] Hardware name: riscv-virtio,qemu (DT)
[ 2.175639] epc : 00ffffff945a2170 ra : 00aaaaaae7332820 sp : 00fffffffd3e6c00
[ 2.176287] gp : 00aaaaaae73aff40 tp : 00ffffff945f1a50 t0 : 0000000000000000
[ 2.176858] t1 : 00aaaaaae7331f9c t2 : 0000000000000002 s0 : 00fffffffd3e6de0
[ 2.177427] s1 : 0000000000000002 a0 : 00aaaaaae73b7380 a1 : 00fffffffd3e6dc8
[ 2.177990] a2 : 00fffffffd3e6de0 a3 : 0000000000000000 a4 : 0000000000000000
[ 2.178524] a5 : 0000000000000002 a6 : 000000000000008b a7 : 0000000000000010
[ 2.179081] s2 : 00aaaaaae73327f0 s3 : 00ffffff945ef990 s4 : 00ffffff945f1988
[ 2.179796] s5 : 00ffffff945f1b48 s6 : 0000000000000000 s7 : 00000000000000e0
[ 2.180366] s8 : 00ffffff945f1d58 s9 : 00ffffff945ecb88 s10: 00ffffff945f17e0
[ 2.185464] s11: 0000000000000001 t3 : 00ffffff945a213c t4 : 0000000300000000
[ 2.186106] t5 : 0000000000000003 t6 : ffffffffffffffff
[ 2.186520] status: 0000000200000020 badaddr: 000000000000b920 cause: 0000000000000002
I bisected it to this commit:
702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()")
Thanks,
Jason
When moving switch_to's has_fpu() over to using riscv_has_extension_
likely() rather than static branchs, the FPU code gained a dependency on
the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected
when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not
work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is
selected.
Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()")
Link: https://lore.kernel.org/all/[email protected]/
Cc: Jisheng Zhang <[email protected]>
Cc: Andrew Jones <[email protected]>
Cc: Heiko Stuebner <[email protected]>
Cc: Conor Dooley <[email protected]>
Signed-off-by: Jason A. Donenfeld <[email protected]>
---
arch/riscv/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index c5e42cc37604..0f59350c699d 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE
config FPU
bool "FPU support"
default y
+ select RISCV_ALTERNATIVE
help
Say N here if you want to disable all floating-point related procedure
in the kernel.
--
2.40.0
On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote:
> When moving switch_to's has_fpu() over to using riscv_has_extension_
> likely() rather than static branchs, the FPU code gained a dependency on
> the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected
> when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not
> work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is
> selected.
>
> Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()")
> Link: https://lore.kernel.org/all/[email protected]/
> Cc: Jisheng Zhang <[email protected]>
> Cc: Andrew Jones <[email protected]>
> Cc: Heiko Stuebner <[email protected]>
> Cc: Conor Dooley <[email protected]>
> Signed-off-by: Jason A. Donenfeld <[email protected]>
> ---
> arch/riscv/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index c5e42cc37604..0f59350c699d 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE
> config FPU
> bool "FPU support"
> default y
> + select RISCV_ALTERNATIVE
> help
> Say N here if you want to disable all floating-point related procedure
> in the kernel.
> --
> 2.40.0
>
Reviewed-by: Andrew Jones <[email protected]>
I took a look to see if we missed anything else and see that we should
do the same patch for KVM. I'll send one.
(It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
can defer that wedding a bit longer.)
Thanks,
drew
On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
> On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote:
> > When moving switch_to's has_fpu() over to using riscv_has_extension_
> > likely() rather than static branchs, the FPU code gained a dependency on
> > the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected
> > when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not
> > work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is
> > selected.
> >
> > Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()")
> > Link: https://lore.kernel.org/all/[email protected]/
> > Cc: Jisheng Zhang <[email protected]>
> > Cc: Andrew Jones <[email protected]>
> > Cc: Heiko Stuebner <[email protected]>
> > Cc: Conor Dooley <[email protected]>
Thanks for fixing it!
Reviewed-by: Conor Dooley <[email protected]>
> > Signed-off-by: Jason A. Donenfeld <[email protected]>
> > ---
> > arch/riscv/Kconfig | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index c5e42cc37604..0f59350c699d 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE
> > config FPU
> > bool "FPU support"
> > default y
> > + select RISCV_ALTERNATIVE
> > help
> > Say N here if you want to disable all floating-point related procedure
> > in the kernel.
> > --
> > 2.40.0
> >
>
> Reviewed-by: Andrew Jones <[email protected]>
>
> I took a look to see if we missed anything else and see that we should
> do the same patch for KVM. I'll send one.
>
> (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> can defer that wedding a bit longer.)
At that point, the config option should just go away entirely, no?
On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote:
> On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
> > On Wed, Mar 22, 2023 at 01:09:07PM +0100, Jason A. Donenfeld wrote:
> > > When moving switch_to's has_fpu() over to using riscv_has_extension_
> > > likely() rather than static branchs, the FPU code gained a dependency on
> > > the alternatives framework. If CONFIG_RISCV_ALTERNATIVE isn't selected
> > > when CONFIG_FPU is, then has_fpu() returns false, and switch_to does not
> > > work as intended. So select CONFIG_RISCV_ALTERNATIVE when CONFIG_FPU is
> > > selected.
> > >
> > > Fixes: 702e64550b12 ("riscv: fpu: switch has_fpu() to riscv_has_extension_likely()")
> > > Link: https://lore.kernel.org/all/[email protected]/
> > > Cc: Jisheng Zhang <[email protected]>
> > > Cc: Andrew Jones <[email protected]>
> > > Cc: Heiko Stuebner <[email protected]>
> > > Cc: Conor Dooley <[email protected]>
>
> Thanks for fixing it!
> Reviewed-by: Conor Dooley <[email protected]>
>
> > > Signed-off-by: Jason A. Donenfeld <[email protected]>
> > > ---
> > > arch/riscv/Kconfig | 1 +
> > > 1 file changed, 1 insertion(+)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index c5e42cc37604..0f59350c699d 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -467,6 +467,7 @@ config TOOLCHAIN_HAS_ZIHINTPAUSE
> > > config FPU
> > > bool "FPU support"
> > > default y
> > > + select RISCV_ALTERNATIVE
> > > help
> > > Say N here if you want to disable all floating-point related procedure
> > > in the kernel.
> > > --
> > > 2.40.0
> > >
> >
> > Reviewed-by: Andrew Jones <[email protected]>
> >
> > I took a look to see if we missed anything else and see that we should
> > do the same patch for KVM. I'll send one.
> >
> > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> > can defer that wedding a bit longer.)
>
> At that point, the config option should just go away entirely, no?
Ah, yes, and that makes the idea even more attractive, as we could remove
several ifdefs.
Thanks,
drew
On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote:
> On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote:
> > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
> > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> > > can defer that wedding a bit longer.)
> >
> > At that point, the config option should just go away entirely, no?
>
> Ah, yes, and that makes the idea even more attractive, as we could remove
> several ifdefs.
I went and did the cursory check, it's not compatible with XIP_KERNEL so
dropping the option entirely probably isn't a possibility :/
On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote:
> On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote:
> > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote:
> > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
>
> > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> > > > can defer that wedding a bit longer.)
> > >
> > > At that point, the config option should just go away entirely, no?
> >
> > Ah, yes, and that makes the idea even more attractive, as we could remove
> > several ifdefs.
>
> I went and did the cursory check, it's not compatible with XIP_KERNEL so
> dropping the option entirely probably isn't a possibility :/
What I said is only now sinking in. We're now going to be disabling FPU
support on XIP kernels with this patch.
Well, technically not this patch since it wouldn't have built without
Jason's changes, but that doesn't seem like the right thing to do...
On Wed, Mar 22, 2023 at 9:05 PM Conor Dooley <[email protected]> wrote:
>
> On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote:
> > On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote:
> > > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote:
> > > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
> >
> > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> > > > > can defer that wedding a bit longer.)
> > > >
> > > > At that point, the config option should just go away entirely, no?
> > >
> > > Ah, yes, and that makes the idea even more attractive, as we could remove
> > > several ifdefs.
> >
> > I went and did the cursory check, it's not compatible with XIP_KERNEL so
> > dropping the option entirely probably isn't a possibility :/
>
> What I said is only now sinking in. We're now going to be disabling FPU
> support on XIP kernels with this patch.
> Well, technically not this patch since it wouldn't have built without
> Jason's changes, but that doesn't seem like the right thing to do...
I suppose you could have riscv_has_extension_*() fall back to
something that doesn't use alternatives on XIP kernels.
On Wed, Mar 22, 2023 at 09:19:50PM +0100, Jason A. Donenfeld wrote:
> On Wed, Mar 22, 2023 at 9:05 PM Conor Dooley <[email protected]> wrote:
> >
> > On Wed, Mar 22, 2023 at 07:44:13PM +0000, Conor Dooley wrote:
> > > On Wed, Mar 22, 2023 at 08:26:10PM +0100, Andrew Jones wrote:
> > > > On Wed, Mar 22, 2023 at 03:17:13PM +0000, Conor Dooley wrote:
> > > > > On Wed, Mar 22, 2023 at 01:46:31PM +0100, Andrew Jones wrote:
> > >
> > > > > > (It's tempting to just select RISCV_ALTERNATIVE from RISCV, but maybe we
> > > > > > can defer that wedding a bit longer.)
> > > > >
> > > > > At that point, the config option should just go away entirely, no?
> > > >
> > > > Ah, yes, and that makes the idea even more attractive, as we could remove
> > > > several ifdefs.
> > >
> > > I went and did the cursory check, it's not compatible with XIP_KERNEL so
> > > dropping the option entirely probably isn't a possibility :/
> >
> > What I said is only now sinking in. We're now going to be disabling FPU
> > support on XIP kernels with this patch.
> > Well, technically not this patch since it wouldn't have built without
> > Jason's changes, but that doesn't seem like the right thing to do...
>
> I suppose you could have riscv_has_extension_*() fall back to
> something that doesn't use alternatives on XIP kernels.
Yah, something like the below I guess? Probably overlooking something
silly & it's lost the benefit of the static branch that it used to have,
but with the infra that we have at the moment this seemed like the
sanest thing to do?
This would requiring picking up your patch Jason, but with an
"if !XIP_KERNEL" added to the select.
It's only had the lightest of build tests, but I can go make it a real
patch if there's not something obviously amiss.
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index e3021b2590de..6263a0de1c6a 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -57,18 +57,31 @@ struct riscv_isa_ext_data {
unsigned int isa_ext_id;
};
+unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
+
+#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext)
+
+bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);
+#define riscv_isa_extension_available(isa_bitmap, ext) \
+ __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext)
+
static __always_inline bool
riscv_has_extension_likely(const unsigned long ext)
{
compiletime_assert(ext < RISCV_ISA_EXT_MAX,
"ext must be < RISCV_ISA_EXT_MAX");
- asm_volatile_goto(
- ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1)
- :
- : [ext] "i" (ext)
- :
- : l_no);
+ if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
+ asm_volatile_goto(
+ ALTERNATIVE("j %l[l_no]", "nop", 0, %[ext], 1)
+ :
+ : [ext] "i" (ext)
+ :
+ : l_no);
+ } else {
+ if (!__riscv_isa_extension_available(NULL, ext))
+ goto l_no;
+ }
return true;
l_no:
@@ -81,26 +94,23 @@ riscv_has_extension_unlikely(const unsigned long ext)
compiletime_assert(ext < RISCV_ISA_EXT_MAX,
"ext must be < RISCV_ISA_EXT_MAX");
- asm_volatile_goto(
- ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1)
- :
- : [ext] "i" (ext)
- :
- : l_yes);
+ if (IS_ENABLED(CONFIG_RISCV_ALTERNATIVE)) {
+ asm_volatile_goto(
+ ALTERNATIVE("nop", "j %l[l_yes]", 0, %[ext], 1)
+ :
+ : [ext] "i" (ext)
+ :
+ : l_yes);
+ } else {
+ if (__riscv_isa_extension_available(NULL, ext))
+ goto l_yes;
+ }
return false;
l_yes:
return true;
}
-unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
-
-#define riscv_isa_extension_mask(ext) BIT_MASK(RISCV_ISA_EXT_##ext)
-
-bool __riscv_isa_extension_available(const unsigned long *isa_bitmap, int bit);
-#define riscv_isa_extension_available(isa_bitmap, ext) \
- __riscv_isa_extension_available(isa_bitmap, RISCV_ISA_EXT_##ext)
-
#endif
#endif /* _ASM_RISCV_HWCAP_H */
On Thu, Mar 23, 2023 at 02:49:34PM +0000, Conor Dooley wrote:
> This would requiring picking up your patch Jason, but with an
> "if !XIP_KERNEL" added to the select.
So the risk of making this all work is that we wind up forgetting to add
`select alternatives if !xip` to various places that need it (fpu, kvm,
maybe others? future others?), because it appears to work, thanks to the
code in your patch.
But making it work is also probably a good thing, since we obviously
want the fpu and maybe other things to work on xip kernels.
So maybe we should get rid of the CONFIG_RISCV_ALTERNATIVES knob
entirely, making it "always enabled", and then conditonalize the
alternatives code to BUILD_BUG_ON when called with CONFIG_XIP_KERNEL=y.
Then, this build bug will get hit immediately by
riscv_has_extension_*(), which will then require your patch, which can
run in a `if (IS_ENABLED(XIP_KERNEL))` block or similar.
The result of that will be:
- !xip kernels properly use the fast riscv_has_extension_*() code and
any alternatives code needed, since it's always selected.
- xip kernels get a BUILD_BUG_ON if they use any alternatives-based code
that doesn't have a xip fallback yet.
What do you think of that approach?
A "lighter weight" version of that approach would be to just remove all of
the `select RISCV_ALTERNATIVES` lines, and instead make
RISCV_ALTERNATIVES specify `default !XIP_KERNEL`. That would more or
less amount to the above too, though with weirder error cases.
Jason
Hey Jason,
I read this mail before I left work today & had a think about it on the
bike home, and had a whole response thought out, got distracted and
forgot it all.. Hopefully I've remembered everything I had to say!
On Thu, Mar 23, 2023 at 04:56:24PM +0100, Jason A. Donenfeld wrote:
> On Thu, Mar 23, 2023 at 02:49:34PM +0000, Conor Dooley wrote:
> > This would requiring picking up your patch Jason, but with an
> > "if !XIP_KERNEL" added to the select.
>
> So the risk of making this all work is that we wind up forgetting to add
> `select alternatives if !xip` to various places that need it (fpu, kvm,
> maybe others? future others?), because it appears to work, thanks to the
> code in your patch.
>
> But making it work is also probably a good thing, since we obviously
> want the fpu and maybe other things to work on xip kernels.
I'm not super pushed about the "maybe other things", since the "maybe
other things" that are in my head (errata and recently added extensions)
have never worked on xip kernels, and losing them isn't a regression.
Since XIP_KERNEL is deemed to be a "NONPORTABLE" option, we wouldn't
need alternatives to enable it for them, but changes would be required
for that to make the alternatives collapse to a build time thing.
Can deal with that iff someone actually does come along wanting it.
We do need to fix this so that situations like the one you hit can't
happen, while not regressing the level of support for xip, so some level
of "making it work" is needed, but I do agree that it needs to be done
in a less footgun way.
> So maybe we should get rid of the CONFIG_RISCV_ALTERNATIVES knob
> entirely, making it "always enabled", and then conditonalize the
> alternatives code to BUILD_BUG_ON when called with CONFIG_XIP_KERNEL=y.
> Then, this build bug will get hit immediately by
> riscv_has_extension_*(), which will then require your patch, which can
> run in a `if (IS_ENABLED(XIP_KERNEL))` block or similar.
>
> The result of that will be:
> - !xip kernels properly use the fast riscv_has_extension_*() code and
> any alternatives code needed, since it's always selected.
> - xip kernels get a BUILD_BUG_ON if they use any alternatives-based code
> that doesn't have a xip fallback yet.
>
> What do you think of that approach?
Initially I thought "great, lets always enable the alternatives
framework" but I don't think we can do that.
For the has_extension() stuff a fallback is fine, but I don't think that
applies to using alternatives for either errata or enabling extensions
at runtime.
I just don't really want to go through and modify the alternative macros
so that they're evaluated at build time for xip unless that is
absolutely required down the line. (I'd rather not even do it at all.)
Most of the things that are currently selecting RISCV_ALTERNATIVE do so
to patch in support for extensions or enable errata, and I don't think
we should expose those config options if the alternatives that they rely
on cannot be used. I think that means something like...
> A "lighter weight" version of that approach would be to just remove all of
> the `select RISCV_ALTERNATIVES` lines, and instead make
> RISCV_ALTERNATIVES specify `default !XIP_KERNEL`. That would more or
> less amount to the above too, though with weirder error cases.
...adding a "select RISCV_ALTERNATIVE if !XIP_KERNEL" to the
CONFIG_RISCV entry, and similarly to what you suggest here, swapping all
of the instances of "select RISCV_ALTERNATIVE" for "depends on
RISCV_ALTERNATIVE". That does still mean we can drop all of the
"depends on !XIP_KERNEL" that are littered around the place whereever we
are using alternatives & should only get the slow path for extension
checking for xip kernels.
That'd handle the issue that you pointed out, where if the select is
missing, my suggested change makes it appear to work if alternatives are
not enabled too.
The BUILD_BUG_ON idea is good too, probably not fixes material, but
might be worth having to prevent alternatives somehow being used when
XIP_KERNEL is set.
I'll try to whip something up tomorrow...
Thanks Jason,
Conor.