riscv: improve boot time isa extensions handling
Subsystems (4)
v2 cover riscv: improve boot time isa extensions handling
Generally, riscv ISA extensions are fixed for any specific hardware platform, that's to say, the hart features won't change any more after booting, this chacteristic make it straightforward to use static branch to check one 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 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/20220922060958.44203-1-samuel at sholland.org/ [2]https://lore.kernel.org/linux-arm-kernel/20220912162210.3626215-8-mark.rutland at arm.com/ [3]https://lore.kernel.org/linux-riscv/20221130225614.1594256-1-heiko at sntech.de/
Andrew Jones (1):
riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
Jisheng Zhang (12):
riscv: fix jal offsets in patched alternatives
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 | 4 +- arch/riscv/errata/thead/errata.c | 11 ++- arch/riscv/include/asm/alternative-macros.h | 20 ++--- arch/riscv/include/asm/alternative.h | 14 +-- arch/riscv/include/asm/errata_list.h | 9 +- arch/riscv/include/asm/hwcap.h | 96 +++++++++++---------- arch/riscv/include/asm/module.h | 15 ++++ 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 | 63 ++++++++++++++ arch/riscv/kernel/cpufeature.c | 82 +++--------------- arch/riscv/kernel/module.c | 15 ---- arch/riscv/kernel/setup.c | 2 + arch/riscv/kernel/vdso.c | 5 -- arch/riscv/kernel/vdso/vdso.lds.S | 7 ++ arch/riscv/kvm/tlb.c | 3 +- 17 files changed, 191 insertions(+), 164 deletions(-)
Signature · 1 line
2.37.2
PATCH v2 1/13 riscv: fix jal offsets in patched alternatives
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)
arch/riscv/include/asm/alternative.h +2 -0
@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */
arch/riscv/kernel/alternative.c +38 -0
@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } } +#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \ + ((value & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) | \ + ((value & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF)) + +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ; + +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit@31:12 */ + call &= ~GENMASK (31 ,12 ); + +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs
arch/riscv/kernel/cpufeature.c +3 -0
@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }
Signature · 1 line
2.37.2
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 30 lines · Jisheng Zhang
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 129
Quoted 5 lines
arch/riscv/kernel/alternative.c +2
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } } +#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \
^ RV_J_IMM_10_1_OPOFF
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 132
Quoted 3 lines
arch/riscv/kernel/alternative.c +3
-0+ ((value & (RV_J_IMM_11_MASK <<RV_J_IMM_11_OFF )) <<RV_J_IMM_11_OPOFF ) | \ + ((value & (RV_J_IMM_19_12_OPOFF <<RV_J_IMM_19_12_OFF )) <<RV_J_IMM_19_12_OPOFF ) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF))
Should put () around value
Quoted 14 lines
arch/riscv/kernel/alternative.c +14
-0+ +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ;
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 156
Quoted 10 lines
arch/riscv/kernel/alternative.c +10
-0+ +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 );
It'd be nice if this had a define.
Quoted 29 lines
arch/riscv/kernel/alternative.c +9
-0+ +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }Signature · 1 line
2.37.2
Thanks,
drew
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 34 lines · Andrew Jones
Quoted 33 lines · Jisheng Zhang
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }
Quoted 3 lines
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 129
Quoted 2 lines
arch/riscv/kernel/alternative.c +2
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \^ RV_J_IMM_10_1_OPOFF
Good catch! I was lucky when testing the whole series ;) will fix it in newer version.
Quoted 6 lines
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 132
Quoted 4 lines
arch/riscv/kernel/alternative.c +3
-0+ ((value & (RV_J_IMM_11_MASK <<RV_J_IMM_11_OFF )) <<RV_J_IMM_11_OPOFF ) | \ + ((value & (RV_J_IMM_19_12_OPOFF <<RV_J_IMM_19_12_OFF )) <<RV_J_IMM_19_12_OPOFF ) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF))Should put () around value
good idea, previously, I just follow to_jalr_imm(), will update it in newer version.
Quoted 60 lines
Quoted 15 lines
arch/riscv/kernel/alternative.c +14
-0+ +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ;arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 156
Quoted 10 lines
arch/riscv/kernel/alternative.c +10
-0+ +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 );It'd be nice if this had a define.
Quoted 29 lines
arch/riscv/kernel/alternative.c +9
-0+ +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }Signature · 1 line
2.37.2Thanks,
drew
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
In reply to Andrew JonesQuoted 34 lines · Andrew Jones
Quoted 33 lines · Jisheng Zhang
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }
Quoted 7 lines
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 129
Quoted 2 lines
arch/riscv/kernel/alternative.c +2
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \^ RV_J_IMM_10_1_OPOFFarch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 132
Quoted 3 lines
arch/riscv/kernel/alternative.c +3
-0+ ((value & (RV_J_IMM_11_MASK <<RV_J_IMM_11_OFF )) <<RV_J_IMM_11_OPOFF ) | \ + ((value & (RV_J_IMM_19_12_OPOFF <<RV_J_IMM_19_12_OFF )) <<RV_J_IMM_19_12_OPOFF ) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF))
Hi all,
I believe there's bug in the to_jal_imm() macro implementation, the correct one should be like this:
#define to_jal_imm(value) \
((RV_X(value, RV_J_IMM_10_1_OFF, RV_J_IMM_10_1_MASK) << RV_J_IMM_10_1_OPOFF) | \
(RV_X(value, RV_J_IMM_11_OFF, RV_J_IMM_11_MASK) << RV_J_IMM_11_OPOFF) | \
(RV_X(value, RV_J_IMM_19_12_OFF, RV_J_IMM_19_12_MASK) << RV_J_IMM_19_12_OPOFF) | \
(RV_X(value, RV_J_IMM_SIGN_OFF, 1) << RV_J_IMM_SIGN_OPOFF))Will fix it in next version.
Thanks
Quoted 62 lines
Should put () around value
Quoted 14 lines
arch/riscv/kernel/alternative.c +14
-0+ +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ;arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 156
Quoted 10 lines
arch/riscv/kernel/alternative.c +10
-0+ +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 );It'd be nice if this had a define.
Quoted 29 lines
arch/riscv/kernel/alternative.c +9
-0+ +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }Signature · 1 line
2.37.2Thanks,
drew
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 45 lines · Jisheng Zhang
Quoted 34 lines · Andrew Jones
Quoted 33 lines · Jisheng Zhang
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }Quoted 7 lines
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 129
Quoted 2 lines
arch/riscv/kernel/alternative.c +2
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \^ RV_J_IMM_10_1_OPOFFarch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 132
Quoted 3 lines
arch/riscv/kernel/alternative.c +3
-0+ ((value & (RV_J_IMM_11_MASK <<RV_J_IMM_11_OFF )) <<RV_J_IMM_11_OPOFF ) | \ + ((value & (RV_J_IMM_19_12_OPOFF <<RV_J_IMM_19_12_OFF )) <<RV_J_IMM_19_12_OPOFF ) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF))Hi all,
Quoted 8 lines
I believe there's bug in the
to_jal_imm()macro implementation, the correct one should be like this:#define to_jal_imm(value) \ ((RV_X(value, RV_J_IMM_10_1_OFF, RV_J_IMM_10_1_MASK) << RV_J_IMM_10_1_OPOFF) | \ (RV_X(value, RV_J_IMM_11_OFF, RV_J_IMM_11_MASK) << RV_J_IMM_11_OPOFF) | \ (RV_X(value, RV_J_IMM_19_12_OFF, RV_J_IMM_19_12_MASK) << RV_J_IMM_19_12_OPOFF) | \ (RV_X(value, RV_J_IMM_SIGN_OFF, 1) << RV_J_IMM_SIGN_OPOFF))
And I just tested to_jal_imm() vs RV_EXTRACT_JTYPE_IMM(), they match perfectly. E.g:
RV_EXTRACT_JTYPE_IMM(to_jal_imm(imm)) == imm is alway true when imm is a jal valid offset.
Quoted 66 lines
Will fix it in next version.
Thanks
Quoted 62 lines
Should put () around value
Quoted 14 lines
arch/riscv/kernel/alternative.c +14
-0+ +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ;arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 156
Quoted 10 lines
arch/riscv/kernel/alternative.c +10
-0+ +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 );It'd be nice if this had a define.
Quoted 29 lines
arch/riscv/kernel/alternative.c +9
-0+ +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }Signature · 1 line
2.37.2Thanks,
drew
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 55 lines · Jisheng Zhang
Quoted 45 lines · Jisheng Zhang
Quoted 34 lines · Andrew Jones
Quoted 33 lines · Jisheng Zhang
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }Quoted 7 lines
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 129
Quoted 2 lines
arch/riscv/kernel/alternative.c +2
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \^ RV_J_IMM_10_1_OPOFFarch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 132
Quoted 3 lines
arch/riscv/kernel/alternative.c +3
-0+ ((value & (RV_J_IMM_11_MASK <<RV_J_IMM_11_OFF )) <<RV_J_IMM_11_OPOFF ) | \ + ((value & (RV_J_IMM_19_12_OPOFF <<RV_J_IMM_19_12_OFF )) <<RV_J_IMM_19_12_OPOFF ) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF))Hi all,
Quoted 8 lines
I believe there's bug in the
to_jal_imm()macro implementation, the correct one should be like this:#define to_jal_imm(value) \ ((RV_X(value, RV_J_IMM_10_1_OFF, RV_J_IMM_10_1_MASK) << RV_J_IMM_10_1_OPOFF) | \ (RV_X(value, RV_J_IMM_11_OFF, RV_J_IMM_11_MASK) << RV_J_IMM_11_OPOFF) | \ (RV_X(value, RV_J_IMM_19_12_OFF, RV_J_IMM_19_12_MASK) << RV_J_IMM_19_12_OPOFF) | \ (RV_X(value, RV_J_IMM_SIGN_OFF, 1) << RV_J_IMM_SIGN_OPOFF))
Quoted 4 lines
And I just tested
to_jal_imm()vsRV_EXTRACT_JTYPE_IMM(), they match perfectly. E.g:
RV_EXTRACT_JTYPE_IMM(to_jal_imm(imm)) == imm is alway true when imm is a jal valid offset.
I think we should define deposit() functions for each type, as well as the extract() functions, (and I'd prefer we use static inlines to macros to get some type checking). See [1] for my proposal.
[1] https://lore.kernel.org/all/20221205145323.l2dro6dt7muumqpn at kamzik/
Thanks,
drew
Quoted 67 lines
Quoted 67 lines
Will fix it in next version.
Thanks
Quoted 62 lines
Should put () around value
Quoted 14 lines
arch/riscv/kernel/alternative.c +14
-0+ +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{ +int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ;arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 156
Quoted 10 lines
arch/riscv/kernel/alternative.c +10
-0+ +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 );It'd be nice if this had a define.
Quoted 29 lines
arch/riscv/kernel/alternative.c +9
-0+ +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }Signature · 1 line
2.37.2Thanks,
drew
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
In reply to Jisheng ZhangAm Sonntag, 4. Dezember 2022, 18:46:20 CET schrieb Jisheng Zhang:
Quoted 32 lines
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }
arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 136
Quoted 10 lines
arch/riscv/kernel/alternative.c +9
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \ + ((value & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) | \ + ((value & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF)) + +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{
I think we might want to unfiy this into a common function like
riscv_alternative_fix_offsets(...)
so that we only run through the code block once
for (i = 0; i < num_instr; i++) {
if (riscv_insn_is_auipc_jalr(inst1, inst2)) {
riscv_alternative_fix_auipc_jalr(...)
continue;
}
if (riscv_insn_is_jal(inst)) {
riscv_alternative_fix_jal(...)
continue;
}
}This would also remove the need from calling multiple functions after patching alternatives.
Thoughts?
Heiko
Quoted 47 lines
arch/riscv/kernel/alternative.c +29
-0+int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ; + +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 ); + +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 62 lines · Heiko St?bner
Am Sonntag, 4. Dezember 2022, 18:46:20 CET schrieb Jisheng Zhang:
Quoted 32 lines
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 136
Quoted 10 lines
arch/riscv/kernel/alternative.c +9
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \ + ((value & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) | \ + ((value & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF)) + +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{I think we might want to unfiy this into a common function like
riscv_alternative_fix_offsets(...)so that we only run through the code block once
for (i = 0; i < num_instr; i++) { if (riscv_insn_is_auipc_jalr(inst1, inst2)) { riscv_alternative_fix_auipc_jalr(...) continue; } if (riscv_insn_is_jal(inst)) { riscv_alternative_fix_jal(...) continue; } }
Quoted 2 lines
This would also remove the need from calling multiple functions after patching alternatives.
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the riscv_alternative_fix_auipc_jalr() to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.
Quoted 57 lines
Thoughts?
Heiko
Quoted 47 lines
arch/riscv/kernel/alternative.c +29
-0+int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ; + +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 ); + +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); } } }
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Heiko, Jisheng,
Quoted 66 lines · Jisheng Zhang
Quoted 62 lines · Heiko St?bner
Am Sonntag, 4. Dezember 2022, 18:46:20 CET schrieb Jisheng Zhang:
Quoted 32 lines
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 136
Quoted 10 lines
arch/riscv/kernel/alternative.c +9
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \ + ((value & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) | \ + ((value & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF)) + +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{I think we might want to unfiy this into a common function like
riscv_alternative_fix_offsets(...)so that we only run through the code block once
for (i = 0; i < num_instr; i++) { if (riscv_insn_is_auipc_jalr(inst1, inst2)) { riscv_alternative_fix_auipc_jalr(...) continue; } if (riscv_insn_is_jal(inst)) { riscv_alternative_fix_jal(...) continue; } }Quoted 2 lines
This would also remove the need from calling multiple functions after patching alternatives.
Quoted 7 lines
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the
riscv_alternative_fix_auipc_jalr()to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.
Heiko, perhaps you can correct me here:
Last Wednesday you & Palmer agreed that it was too late in the cycle to apply any of the stuff touching alternatives?
If I do recall correctly, gives plenty of time to sort out any interdependent changes here.
Could easily be misremembering, wouldn't be the first time!
Thanks,
Conor.
Quoted 43 lines
Quoted 43 lines
Quoted 43 lines
arch/riscv/kernel/alternative.c +29
-0+int num_instr = len /sizeof (u32); +unsigned int call; +int i; +int imm; + +for (i =0 ; i < num_instr; i++) { +u32 inst =riscv_instruction_at (alt_ptr, i); + +if (!riscv_insn_is_jal (inst)) +continue ; + +/* get and adjust new target address */ + imm =RV_EXTRACT_JTYPE_IMM (inst); + imm -= patch_offset; + +/* pick the original jal */ + call = inst; + +/* drop the old IMMs, all jal imm bits sit at 31:12 */ + call &= ~GENMASK (31 ,12 ); + +/* add the adapted IMMs */ + call |=to_jal_imm (imm); + +/* patch the call place again */ +patch_text_nosync (alt_ptr + i *sizeof (u32), &call,4 ); + } +} +/* * This is called very early in the boot process (directly after we run * a feature detect on the boot CPU). No need to worry about other CPUs arch/riscv/kernel/cpufeature.c +3
-0@@ -324,6 +324,9 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin,riscv_alternative_fix_auipc_jalr (alt->old_ptr, alt->alt_len, alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr);
Quoted 8 lines
Quoted 8 lines
arch/riscv/kernel/cpufeature.c void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, · line 332
Quoted 4 lines
arch/riscv/kernel/cpufeature.c +0
-0} } }
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/a1b9d266/attachment-0001.sig>
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Am Montag, 5. Dezember 2022, 19:36:45 CET schrieb Conor Dooley:
Quoted 77 lines
Heiko, Jisheng,
Quoted 66 lines · Jisheng Zhang
Quoted 62 lines · Heiko St?bner
Am Sonntag, 4. Dezember 2022, 18:46:20 CET schrieb Jisheng Zhang:
Quoted 32 lines
Alternatives live in a different section, so offsets used by jal instruction will point to wrong locations after the patch got applied.
Similar to arm64, adjust the location to consider that offset.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/alternative.h | 2 ++ arch/riscv/kernel/alternative.c | 38 ++++++++++++++++++++++++++++ arch/riscv/kernel/cpufeature.c | 3 +++ 3 files changed, 43 insertions(+)arch/riscv/include/asm/alternative.h +2
-0@@ -29,6 +29,8 @@ void apply_module_alternatives(void *start, size_t length);void riscv_alternative_fix_auipc_jalr (void *alt_ptr,unsigned int len,int patch_offset); +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset);struct alt_entry {void *old_ptr;/* address of original instruciton or data */ arch/riscv/kernel/alternative.c +0
-0@@ -125,6 +125,44 @@ void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, } }arch/riscv/kernel/alternative.c void riscv_alternative_fix_auipc_jalr(void *alt_ptr, unsigned int len, · line 136
Quoted 10 lines
arch/riscv/kernel/alternative.c +9
-0+#define to_jal_imm (value) \ + (((value & (RV_J_IMM_10_1_MASK << RV_J_IMM_10_1_OFF)) << RV_I_IMM_11_0_OPOFF) | \ + ((value & (RV_J_IMM_11_MASK << RV_J_IMM_11_OFF)) << RV_J_IMM_11_OPOFF) | \ + ((value & (RV_J_IMM_19_12_OPOFF << RV_J_IMM_19_12_OFF)) << RV_J_IMM_19_12_OPOFF) | \ + ((value & (1 << RV_J_IMM_SIGN_OFF)) << RV_J_IMM_SIGN_OPOFF)) + +void riscv_alternative_fix_jal (void *alt_ptr,unsigned int len, +int patch_offset) +{I think we might want to unfiy this into a common function like
riscv_alternative_fix_offsets(...)so that we only run through the code block once
for (i = 0; i < num_instr; i++) { if (riscv_insn_is_auipc_jalr(inst1, inst2)) { riscv_alternative_fix_auipc_jalr(...) continue; } if (riscv_insn_is_jal(inst)) { riscv_alternative_fix_jal(...) continue; } }Quoted 2 lines
This would also remove the need from calling multiple functions after patching alternatives.
Quoted 7 lines
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the
riscv_alternative_fix_auipc_jalr()to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.
Quoted 8 lines
Heiko, perhaps you can correct me here:
Last Wednesday you & Palmer agreed that it was too late in the cycle to apply any of the stuff touching alternatives?
If I do recall correctly, gives plenty of time to sort out any interdependent changes here.Could easily be misremembering, wouldn't be the first time!
You slightly misremembered, but are still correct with the above ;-) .
I.e. what we talked about was stuff for fixes for 6.1-rc, were Palmers wisely wanted to limit additions to really easy fixes for the remaining last rc, to not upset any existing boards.
But you are still correct that we also shouldn't target the 6.2 merge window anymore :-) .
We're after -rc8 now (which is in itself uncommon) and in his -rc7 announcement [0], Linus stated
"[...] the usual rule is that things that I get sent for the merge window should have been all ready _before_ the merge window opened. But with the merge window happening largely during the holiday season, I'll just be enforcing that pretty strictly."
That means new stuff should be reviewed and in linux-next _way before_ the merge window opens next weekend. Taking into account that people need to review stuff (and maybe the series needing another round), I really don't see this happening this week and everything else will get us shouted at from atop a christmas tree ;-) .
That's the reason most maintainer-trees stop accepting stuff after -rc7
Heiko
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 18 lines · Heiko St?bner
Am Montag, 5. Dezember 2022, 19:36:45 CET schrieb Conor Dooley:
Quoted 17 lines
Heiko, Jisheng,
Quoted 7 lines · Jisheng Zhang
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the
riscv_alternative_fix_auipc_jalr()to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.Heiko, perhaps you can correct me here:
Last Wednesday you & Palmer agreed that it was too late in the cycle to apply any of the stuff touching alternatives?
If I do recall correctly, gives plenty of time to sort out any interdependent changes here.
Quoted 7 lines
Quoted 1 line
Could easily be misremembering, wouldn't be the first time!
You slightly misremembered, but are still correct with the above ;-) .
I.e. what we talked about was stuff for fixes for 6.1-rc, were Palmers wisely wanted to limit additions to really easy fixes for the remaining last rc, to not upset any existing boards.
Ahh right. I was 50-50 on whether something like that was said so at least I am not going crazy.
Quoted 10 lines
But you are still correct that we also shouldn't target the 6.2 merge window anymore :-) .
We're after -rc8 now (which is in itself uncommon) and in his -rc7 announcement [0], Linus stated
"[...] the usual rule is that things that I get sent for the merge window should have been all ready _before_ the merge window opened. But with the merge window happening largely during the holiday season, I'll just be enforcing that pretty strictly."
Yah, of all the windows to land patchsets that are being re-spun a few days before it opens this probably isn't the best one to pick!
Quoted 7 lines
That means new stuff should be reviewed and in linux-next
_waybefore_the merge window opens next weekend. Taking into account that people need to review stuff (and maybe the series needing another round), I really don't see this happening this week and everything else will get us shouted at from atop a christmas tree ;-) .That's the reason most maintainer-trees stop accepting stuff after -rc7
Aye, in RISC-V land maybe we will get there one day :)
For the original question though, breaking them up into 3 or 4 smaller bits that could get applied on their own is probably a good idea?
Between yourselves, Drew and Prabhakar there's a couple series touching the same bits. Certainly don't want to seem like I am speaking for the Higher Powers here, but some sort of logical ordering would probably be a good idea so as not to hold each other up?
The non-string bit of your series has been fairly well reviewed & would, in theory, be mergeable once the tree re-opens? Timing aside, Jisheng's idea seems like a good one, no?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/2e948fc5/attachment.sig>
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Am Montag, 5. Dezember 2022, 20:49:26 CET schrieb Conor Dooley:
Quoted 57 lines · Heiko St?bner
Quoted 18 lines
Am Montag, 5. Dezember 2022, 19:36:45 CET schrieb Conor Dooley:
Quoted 17 lines
Heiko, Jisheng,
Quoted 7 lines · Jisheng Zhang
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the
riscv_alternative_fix_auipc_jalr()to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.Heiko, perhaps you can correct me here:
Last Wednesday you & Palmer agreed that it was too late in the cycle to apply any of the stuff touching alternatives?
If I do recall correctly, gives plenty of time to sort out any interdependent changes here.Quoted 7 lines
Quoted 1 line
Could easily be misremembering, wouldn't be the first time!
You slightly misremembered, but are still correct with the above ;-) .
I.e. what we talked about was stuff for fixes for 6.1-rc, were Palmers wisely wanted to limit additions to really easy fixes for the remaining last rc, to not upset any existing boards.
Ahh right. I was 50-50 on whether something like that was said so at least I am not going crazy.
Quoted 10 lines
But you are still correct that we also shouldn't target the 6.2 merge window anymore :-) .
We're after -rc8 now (which is in itself uncommon) and in his -rc7 announcement [0], Linus stated
"[...] the usual rule is that things that I get sent for the merge window should have been all ready _before_ the merge window opened. But with the merge window happening largely during the holiday season, I'll just be enforcing that pretty strictly."
Yah, of all the windows to land patchsets that are being re-spun a few days before it opens this probably isn't the best one to pick!
Quoted 7 lines
That means new stuff should be reviewed and in linux-next
_waybefore_the merge window opens next weekend. Taking into account that people need to review stuff (and maybe the series needing another round), I really don't see this happening this week and everything else will get us shouted at from atop a christmas tree ;-) .That's the reason most maintainer-trees stop accepting stuff after -rc7
Aye, in RISC-V land maybe we will get there one day :)
For the original question though, breaking them up into 3 or 4 smaller bits that could get applied on their own is probably a good idea?
Quoted 7 lines
Between yourselves, Drew and Prabhakar there's a couple series touching the same bits. Certainly don't want to seem like I am speaking for the Higher Powers here, but some sort of logical ordering would probably be a good idea so as not to hold each other up?
The non-string bit of your series has been fairly well reviewed & would, in theory, be mergeable once the tree re-opens? Timing aside, Jisheng's idea seems like a good one, no?
yeah, I had that same thought over the weekend - with the generic part being pretty good in the review and only the string part needing more work and thus ideally splitting the series [0] .
Jisheng's series just made that even more important to do :-)
Heiko
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 52 lines · Heiko St?bner
Am Montag, 5. Dezember 2022, 20:49:26 CET schrieb Conor Dooley:
Quoted 51 lines · Heiko St?bner
Quoted 18 lines
Am Montag, 5. Dezember 2022, 19:36:45 CET schrieb Conor Dooley:
Quoted 17 lines
Heiko, Jisheng,
Quoted 7 lines · Jisheng Zhang
Yesterday, I also wanted to unify the two instruction fix into one. But that would need to roll back the
riscv_alternative_fix_auipc_jalr()to your v1 version. And IMHO, it's better if you can split the Zbb string optimizations series into two: one for alternative improvements, another for Zbb. Then we may get the alternative improvements and this inst extension series merged in v6.2-rc1.Heiko, perhaps you can correct me here:
Last Wednesday you & Palmer agreed that it was too late in the cycle to apply any of the stuff touching alternatives?
If I do recall correctly, gives plenty of time to sort out any interdependent changes here.Quoted 7 lines
Quoted 1 line
Could easily be misremembering, wouldn't be the first time!
You slightly misremembered, but are still correct with the above ;-) .
I.e. what we talked about was stuff for fixes for 6.1-rc, were Palmers wisely wanted to limit additions to really easy fixes for the remaining last rc, to not upset any existing boards.
Ahh right. I was 50-50 on whether something like that was said so at least I am not going crazy.
Quoted 10 lines
But you are still correct that we also shouldn't target the 6.2 merge window anymore :-) .
We're after -rc8 now (which is in itself uncommon) and in his -rc7 announcement [0], Linus stated
"[...] the usual rule is that things that I get sent for the merge window should have been all ready _before_ the merge window opened. But with the merge window happening largely during the holiday season, I'll just be enforcing that pretty strictly."
Yah, of all the windows to land patchsets that are being re-spun a few days before it opens this probably isn't the best one to pick!
Quoted 7 lines
That means new stuff should be reviewed and in linux-next
_waybefore_the merge window opens next weekend. Taking into account that people need to review stuff (and maybe the series needing another round), I really don't see this happening this week and everything else will get us shouted at from atop a christmas tree ;-) .That's the reason most maintainer-trees stop accepting stuff after -rc7
Thanks for the information, then we have more time to test and review this series.
Quoted 8 lines
Quoted 8 lines
Aye, in RISC-V land maybe we will get there one day :)
For the original question though, breaking them up into 3 or 4 smaller bits that could get applied on their own is probably a good idea?
Between yourselves, Drew and Prabhakar there's a couple series touching the same bits. Certainly don't want to seem like I am speaking for the
Because alternative is the best solution to riscv extensions while still keep one unified kernel Image ;)
Quoted 5 lines
Quoted 5 lines
Higher Powers here, but some sort of logical ordering would probably be a good idea so as not to hold each other up?
The non-string bit of your series has been fairly well reviewed & would, in theory, be mergeable once the tree re-opens? Timing aside, Jisheng's idea seems like a good one, no?
IMHO, it will be better if Palmer can merge Heiko's alternative improvements into riscv-next once well reviewed and the window is reopen. Then Drew, Prabhakar and I can rebase on that tree.
Quoted 11 lines
yeah, I had that same thought over the weekend - with the generic part being pretty good in the review and only the string part needing more work and thus ideally splitting the series [0] .
Jisheng's series just made that even more important to do :-)
Heiko
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 9 lines · Jisheng Zhang
Quoted 5 lines
Quoted 5 lines
Higher Powers here, but some sort of logical ordering would probably be a good idea so as not to hold each other up?
The non-string bit of your series has been fairly well reviewed & would, in theory, be mergeable once the tree re-opens? Timing aside, Jisheng's idea seems like a good one, no?IMHO, it will be better if Palmer can merge Heiko's alternative improvements into riscv-next once well reviewed and the window is reopen. Then Drew, Prabhakar and I can rebase on that tree.
Unless I missed something, we're saying the same thing in different ways :)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221206/80f68058/attachment-0001.sig>
Re: [PATCH v2 01/13] riscv: fix jal offsets in patched alternatives
Quoted 8 lines · Conor Dooley
Quoted 6 lines · Jisheng Zhang
Quoted 5 lines
Quoted 5 lines
Higher Powers here, but some sort of logical ordering would probably be a good idea so as not to hold each other up?
The non-string bit of your series has been fairly well reviewed & would, in theory, be mergeable once the tree re-opens? Timing aside, Jisheng's idea seems like a good one, no?
Quoted 6 lines
Quoted 3 lines
IMHO, it will be better if Palmer can merge Heiko's alternative improvements into riscv-next once well reviewed and the window is reopen. Then Drew, Prabhakar and I can rebase on that tree.
Unless I missed something, we're saying the same thing in different ways :)
Hey Jisheng,
FYI I'm gonna mark this version of the patchset as "Changes Requested" in patchwork. Palmer's said he'll apply Heiko's patchset that this depends on once rc1 is out so I am expecting that you'll rebase on top of that with the various comments fixed.
Thanks,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221219/23be95fc/attachment.sig>
PATCH v2 2/13 riscv: move riscv_noncoherent_supported() out of ZICBOM probe
In reply to Jisheng ZhangIt's a bit weird to call riscv_noncoherent_supported() each time when insmoding a module. Move the calling out of feature patch func.
--- arch/riscv/kernel/cpufeature.c | 1 - arch/riscv/kernel/setup.c | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)
arch/riscv/kernel/cpufeature.c +0 -1
@@ -274,7 +274,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; }
arch/riscv/kernel/setup.c +2 -0
@@ -300,6 +300,8 @@ void __init setup_arch(char **cmdline_p)riscv_init_cbom_blocksize ();riscv_fill_hwcap ();apply_boot_alternatives (); +if (riscv_isa_extension_available (NULL ,ZICBOM )) +riscv_noncoherent_supported (); }static int __init topology_init (void )
Signature · 1 line
2.37.2
Re: [PATCH v2 02/13] riscv: move riscv_noncoherent_supported() out of ZICBOM probe
Am Sonntag, 4. Dezember 2022, 18:46:21 CET schrieb Jisheng Zhang:
Quoted 28 lines
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 | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)arch/riscv/kernel/cpufeature.c +0
-1@@ -274,7 +274,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; }arch/riscv/kernel/setup.c +0
-0
arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 304
Quoted 5 lines
arch/riscv/kernel/setup.c +2
-0@@ -300,6 +300,8 @@ void __init setup_arch(char **cmdline_p)riscv_init_cbom_blocksize ();riscv_fill_hwcap ();apply_boot_alternatives (); +if (riscv_isa_extension_available (NULL ,ZICBOM )) +riscv_noncoherent_supported ();
hmm, this changes the behaviour slightly. In the probe function there is the
if (!IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM)) return false;
at the top, so with this change the second WARN_TAINT in arch_setup_dma_ops will behave differently
Heiko
Re: [PATCH v2 02/13] riscv: move riscv_noncoherent_supported() out of ZICBOM probe
Quoted 37 lines · Heiko St?bner
Am Sonntag, 4. Dezember 2022, 18:46:21 CET schrieb Jisheng Zhang:
Quoted 28 lines
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 | 2 ++ 2 files changed, 2 insertions(+), 1 deletion(-)arch/riscv/kernel/cpufeature.c +0
-1@@ -274,7 +274,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; }arch/riscv/kernel/setup.c +0
-0arch/riscv/kernel/setup.c void __init setup_arch(char **cmdline_p) · line 304
Quoted 5 lines
arch/riscv/kernel/setup.c +2
-0@@ -300,6 +300,8 @@ void __init setup_arch(char **cmdline_p)riscv_init_cbom_blocksize ();riscv_fill_hwcap ();apply_boot_alternatives (); +if (riscv_isa_extension_available (NULL ,ZICBOM )) +riscv_noncoherent_supported ();hmm, this changes the behaviour slightly. In the probe function there is the
Quoted 4 lines
if (!IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM)) return false;at the top, so with this change the second
WARN_TAINTinarch_setup_dma_opswill behave differently
thanks for the information. below code can keep the behavior:
if (IS_ENABLED(CONFIG_RISCV_ISA_ZICBOM) && riscv_isa_extension_available(NULL, ZICBOM)) riscv_noncoherent_supported();
will wait for one more day for more review comments, then will send out a v3
Quoted 5 lines
Heiko
Re: [PATCH v2 02/13] riscv: move riscv_noncoherent_supported() out of ZICBOM probe
Quoted 2 lines · Jisheng Zhang
will wait for one more day for more review comments, then will send out a v3
AFAICT, the patches were only sent yesterday? It'd be nice if you could give us more than 2 days between versions of something please :/
Thanks
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/4e2c4111/attachment-0001.sig>
PATCH v2 3/13 riscv: cpufeature: detect RISCV_ALTERNATIVES_EARLY_BOOT earlier
In reply to Jisheng ZhangNow, the riscv_cpufeature_patch_func() do nothing in the stage of RISCV_ALTERNATIVES_EARLY_BOOT. We can move the detection of "early boot" stage earlier.
In following patch, we will make riscv_cpufeature_patch_func() scans all ISA extensions.
--- arch/riscv/kernel/cpufeature.c | 3 +++ 1 file changed, 3 insertions(+)
arch/riscv/kernel/cpufeature.c +3 -0
@@ -305,6 +305,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 ;
Signature · 1 line
2.37.2
Re: [PATCH v2 03/13] riscv: cpufeature: detect RISCV_ALTERNATIVES_EARLY_BOOT earlier
Quoted 6 lines · Jisheng Zhang
Now, the
riscv_cpufeature_patch_func()do nothing in the stage ofRISCV_ALTERNATIVES_EARLY_BOOT. We can move the detection of "early boot" stage earlier.In following patch, we will make
riscv_cpufeature_patch_func()scans all ISA extensions.
I'm not 100% on the motivation here. Let me try and regurgitate the changelog in a way that makes more sense to me and maybe you can confirm it.
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.
Would that be a correct summary?
Thanks,
Conor.
Quoted 25 lines
Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Andrew Jones <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>--- arch/riscv/kernel/cpufeature.c | 3 +++ 1 file changed, 3 insertions(+)arch/riscv/kernel/cpufeature.c +3
-0@@ -305,6 +305,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 ;Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/6cba4cad/attachment.sig>
PATCH v2 4/13 riscv: hwcap: make ISA extension ids can be used in asm
In reply to Jisheng ZhangWe will make use of ISA extension in asm files, so make the multi-letter RISC-V ISA extension IDs macros rather than enums and move them and those base ISA extension IDs to suitable place.
--- arch/riscv/include/asm/hwcap.h | 43 ++++++++++++++++------------------ 1 file changed, 20 insertions(+), 23 deletions(-)
arch/riscv/include/asm/hwcap.h +20 -23
@@ -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,22 +32,33 @@ 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. */ -enum riscv_isa_ext_id { -RISCV_ISA_EXT_SSCOFPMF =RISCV_ISA_EXT_BASE , -RISCV_ISA_EXT_SVPBMT , -RISCV_ISA_EXT_ZICBOM , -RISCV_ISA_EXT_ZIHINTPAUSE , -RISCV_ISA_EXT_SSTC , -RISCV_ISA_EXT_SVINVAL , -RISCV_ISA_EXT_ID_MAX =RISCV_ISA_EXT_MAX , +#define RISCV_ISA_EXT_SSCOFPMF 26 +#define RISCV_ISA_EXT_SVPBMT 27 +#define RISCV_ISA_EXT_ZICBOM 28 +#define RISCV_ISA_EXT_ZIHINTPAUSE 29 +#define RISCV_ISA_EXT_SSTC 30 +#define RISCV_ISA_EXT_SVINVAL 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 , }; +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
Signature · 1 line
2.37.2
Re: [PATCH v2 04/13] riscv: hwcap: make ISA extension ids can be used in asm
Hey Jisheng,
Quoted 3 lines · Jisheng Zhang
We will make use of ISA extension in asm files, so make the multi-letter RISC-V ISA extension IDs macros rather than enums and move them and those base ISA extension IDs to suitable place.
Which base ISA extension IDs? Changelog should match the patch contents, and it's a little unclear here since the base ISA extension IDs are visible here but in the context not the diff.
Quoted 48 lines
Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>Reviewed-by Andrew Jones <[email protected]>--- arch/riscv/include/asm/hwcap.h | 43 ++++++++++++++++------------------ 1 file changed, 20 insertions(+), 23 deletions(-)arch/riscv/include/asm/hwcap.h +1
-19@@ -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,22 +32,33 @@ 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. */ -enum riscv_isa_ext_id { -RISCV_ISA_EXT_SSCOFPMF =RISCV_ISA_EXT_BASE , -RISCV_ISA_EXT_SVPBMT , -RISCV_ISA_EXT_ZICBOM ,
arch/riscv/include/asm/hwcap.h extern unsigned long elf_hwcap; · line 46
Quoted 10 lines
arch/riscv/include/asm/hwcap.h +6
-4-RISCV_ISA_EXT_ZIHINTPAUSE , -RISCV_ISA_EXT_SSTC , -RISCV_ISA_EXT_SVINVAL , - RISCV_ISA_EXT_ID_MAX = RISCV_ISA_EXT_MAX, +#define RISCV_ISA_EXT_SSCOFPMF 26 +#define RISCV_ISA_EXT_SVPBMT 27 +#define RISCV_ISA_EXT_ZICBOM 28 +#define RISCV_ISA_EXT_ZIHINTPAUSE 29 +#define RISCV_ISA_EXT_SSTC 30 +#define RISCV_ISA_EXT_SVINVAL 31
Could you re-order these alphabetically when you move them please?
Thanks,
Conor.
Quoted 21 lines
arch/riscv/include/asm/hwcap.h +13
-0+ +#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; +/* * 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 Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/bf094017/attachment-0001.sig>
Re: [PATCH v2 04/13] riscv: hwcap: make ISA extension ids can be used in asm
Quoted 63 lines · Conor Dooley
Hey Jisheng,
Quoted 3 lines · Jisheng Zhang
We will make use of ISA extension in asm files, so make the multi-letter RISC-V ISA extension IDs macros rather than enums and move them and those base ISA extension IDs to suitable place.
Which base ISA extension IDs? Changelog should match the patch contents, and it's a little unclear here since the base ISA extension IDs are visible here but in the context not the diff.
Quoted 52 lines
Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>Reviewed-by Andrew Jones <[email protected]>--- arch/riscv/include/asm/hwcap.h | 43 ++++++++++++++++------------------ 1 file changed, 20 insertions(+), 23 deletions(-)arch/riscv/include/asm/hwcap.h +1
-23@@ -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,22 +32,33 @@ 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. */ -enum riscv_isa_ext_id { -RISCV_ISA_EXT_SSCOFPMF =RISCV_ISA_EXT_BASE , -RISCV_ISA_EXT_SVPBMT , -RISCV_ISA_EXT_ZICBOM , -RISCV_ISA_EXT_ZIHINTPAUSE , -RISCV_ISA_EXT_SSTC , -RISCV_ISA_EXT_SVINVAL , -RISCV_ISA_EXT_ID_MAX =RISCV_ISA_EXT_MAX ,
Quoted 8 lines
arch/riscv/include/asm/hwcap.h extern unsigned long elf_hwcap; · line 46
Quoted 6 lines
arch/riscv/include/asm/hwcap.h +6
-0+#define RISCV_ISA_EXT_SSCOFPMF 26 +#define RISCV_ISA_EXT_SVPBMT 27 +#define RISCV_ISA_EXT_ZICBOM 28 +#define RISCV_ISA_EXT_ZIHINTPAUSE 29 +#define RISCV_ISA_EXT_SSTC 30 +#define RISCV_ISA_EXT_SVINVAL 31Could you re-order these alphabetically when you move them please?
On reflection, this is a horrific idea - don't bother. It'd only be temporary anyway as it'd need massaging when the next extension comes along.
Either people will have a slightly harder seeing if something is added, or a "slightly" harder time finding which is the next free number or have to reshuffle the whole thing.
The latter sounds like a prime bug breeding ground, while the former is just a search away.
So yeah, don't bother & apologies for the noise!
Quoted 13 lines
Quoted 13 lines
arch/riscv/include/asm/hwcap.h +11
-0+ +#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 , };
Quoted 8 lines
Quoted 8 lines
arch/riscv/include/asm/hwcap.h +2
-0+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 Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221222/57b9f567/attachment.sig>
PATCH v2 5/13 riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA extensions
In reply to Jisheng Zhangmake the riscv_cpufeature_patch_func() scan all ISA extensions rather than limited feature macros.
--- arch/riscv/include/asm/errata_list.h | 9 ++-- arch/riscv/kernel/cpufeature.c | 73 +++++----------------------- 2 files changed, 15 insertions(+), 67 deletions(-)
arch/riscv/include/asm/errata_list.h +3 -6
@@ -6,6 +6,7 @@#define ASM_ERRATA_LIST_H #include <asm/alternative.h> +#include <asm/hwcap.h> #include <asm/vendorid_list.h> #ifdef CONFIG_ERRATA_SIFIVE
@@ -20,10 +21,6 @@#define ERRATA_THEAD_NUMBER 2#endif -#define CPUFEATURE_SVPBMT 0 -#define CPUFEATURE_ZICBOM 1 -#define CPUFEATURE_NUMBER 2 -#ifdef __ASSEMBLY__#define ALT_INSN_FAULT (x) \
@@ -53,7 +50,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) \
@@ -127,7 +124,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" \
arch/riscv/kernel/cpufeature.c +12 -61
95 lines not on this page · raw message
Signature · 1 line
2.37.2
Re: [PATCH v2 05/13] riscv: cpufeature: extend riscv_cpufeature_patch_func to all ISA extensions
Quoted 2 lines · Jisheng Zhang
make the
riscv_cpufeature_patch_func()scan all ISA extensions rather than limited feature macros.
Certainly looks like a nice cleanup. Perhaps for the changelog, something along the lines of:
"riscv_cpufeature_patch_func() currently only scans a limited set of cpufeatures, explicitly defined with macros. Extend it to probe for all ISA extensions"
Quoted 34 lines
Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Andrew Jones <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>--- arch/riscv/include/asm/errata_list.h | 9 ++-- arch/riscv/kernel/cpufeature.c | 73 +++++----------------------- 2 files changed, 15 insertions(+), 67 deletions(-)arch/riscv/kernel/cpufeature.c +4
-12@@ -311,25 +264,23 @@ 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) { -/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, - alt->alt_len); +if (!__riscv_isa_extension_available (NULL , alt->errata_id)) +continue ; -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); - riscv_alternative_fix_jal(alt->old_ptr, - alt->alt_len,
arch/riscv/kernel/cpufeature.c void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, · line 283
Quoted 10 lines
arch/riscv/kernel/cpufeature.c +8
-2- alt->old_ptr - alt->alt_ptr); - } +/* do the basic patching */ +patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr); +riscv_alternative_fix_jal (alt->old_ptr, + alt->alt_len, + alt->old_ptr - alt->alt_ptr);
nit:
Now that you've dropped a level of indent, can alt->alt_len move up a line?
Thanks,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221205/a8392d07/attachment.sig>
PATCH v2 6/13 riscv: introduce riscv_has_extension_[un]likely()
In reply to Jisheng ZhangGenerally, riscv ISA extensions are fixed for any specific hardware platform, that's to say, the hart features won't change any more after booting, this chacteristic make it straightforward to use static branch to check one 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.
[1]https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel at sholland.org/ [2]https://lore.kernel.org/linux-arm-kernel/20220912162210.3626215-8-mark.rutland at arm.com/
--- arch/riscv/include/asm/hwcap.h | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
arch/riscv/include/asm/hwcap.h +37 -0
@@ -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>
@@ -96,6 +97,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)
Signature · 1 line
2.37.2
Re: [PATCH v2 06/13] riscv: introduce riscv_has_extension_[un]likely()
Hey Jisheng,
Just a couple really minor bits here...
Quoted 2 lines · Jisheng Zhang
Generally, riscv ISA extensions are fixed for any specific hardware platform, that's to say, the hart features won't change any more
s/that's to say, the hart/so a hart's/
s/any more//
Quoted 1 line
after booting, this chacteristic make it straightforward to use
"booting. This characteristic makes it"
Quoted 1 line
static branch to check one specific ISA extension is supported or not
"a static branch to check if a"
Quoted 19 lines
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 incpu_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_tablesection, which is not discarded after ISA extensions are finalized, wastes some space.
Quoted 8 lines
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.
[1]https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel at sholland.org/ [2]https://lore.kernel.org/linux-arm-kernel/20220912162210.3626215-8-mark.rutland at arm.com/
Can you make these into Link: tags please (and drop the line between the and the SoB)? So:
Quoted 2 lines
Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Andrew Jones <[email protected]>
Changes themselves look grand, no comments there :)
Thanks!
Conor.
Quoted 62 lines
--- arch/riscv/include/asm/hwcap.h | 37 ++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)arch/riscv/include/asm/hwcap.h +37
-0@@ -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> @@ -96,6 +97,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)Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221206/0fb45920/attachment-0001.sig>
PATCH v2 7/13 riscv: fpu: switch has_fpu() to riscv_has_extension_likely()
In reply to Jisheng ZhangSwitch has_fpu() from statich branch to the new helper riscv_has_extension_likely().
--- arch/riscv/include/asm/switch_to.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
arch/riscv/include/asm/switch_to.h +2 -1
@@ -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; }
Signature · 1 line
2.37.2
PATCH v2 8/13 riscv: module: move find_section to module.h
In reply to Jisheng ZhangMove it to the header so that the implementation can be shared by the alternatives code.
--- arch/riscv/include/asm/module.h | 15 +++++++++++++++ arch/riscv/kernel/module.c | 15 --------------- 2 files changed, 15 insertions(+), 15 deletions(-)
arch/riscv/include/asm/module.h +15 -0
@@ -111,4 +111,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 */
arch/riscv/kernel/module.c +0 -15
@@ -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)
Signature · 1 line
2.37.2
Re: [PATCH v2 08/13] riscv: module: move find_section to module.h
Quoted 14 lines · Jisheng Zhang
Move it to the header so that the implementation can be shared by the alternatives code.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/module.h | 15 +++++++++++++++ arch/riscv/kernel/module.c | 15 --------------- 2 files changed, 15 insertions(+), 15 deletions(-)arch/riscv/include/asm/module.h +0
-0
arch/riscv/include/asm/module.h static inline struct plt_entry *get_plt_entry(unsigned long val, · line 112
Quoted 2 lines
arch/riscv/include/asm/module.h +0
-0@@ -111,4 +111,19 @@ static inline struct plt_entry *get_plt_entry(unsigned long val,#endif /* CONFIG_MODULE_SECTIONS */
Should probably add an explicit #include <linux/elf.h>
Otherwise
Thanks,
drew
Quoted 46 lines
arch/riscv/include/asm/module.h +15
-0+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 */ arch/riscv/kernel/module.c +0
-15@@ -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)Signature · 1 line
2.37.2
Re: [PATCH v2 08/13] riscv: module: move find_section to module.h
In reply to Jisheng ZhangQuoted 2 lines · Jisheng Zhang
Move it to the header so that the implementation can be shared by the alternatives code.
I'm not a super big fan of these perfunctory commit messages. Maybe I'm being a bit ornery, but a few words about why the alternatives could benefit from this would be nice. Also, s/it/find_section()/ please.
How about:
Quoted 9 lines
Move
find_section()tomodule.hso that the implementation can be shared by the alternatives code. This will allow us to use alternatives in the vdso.Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/include/asm/module.h | 15 +++++++++++++++ arch/riscv/kernel/module.c | 15 ---------------
Silly question maybe, but is module.h the right place to put this? But I have nothing better to suggest, and I hate bikeshedding stuff when I have no suggestion to make.
Rationale behind the movement seems grand to me though, so I suppose:
Thanks,
Conor.
Quoted 55 lines
2 files changed, 15 insertions(+), 15 deletions(-)arch/riscv/include/asm/module.h +15
-0@@ -111,4 +111,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 */ arch/riscv/kernel/module.c +0
-15@@ -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)Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221206/09a0e220/attachment-0001.sig>
PATCH v2 9/13 riscv: switch to relative alternative entries
In reply to Jisheng ZhangInstead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the alt_entry values. So we can not only cut the size of the alternative entry, but also meet the prerequisite for patching alternatives in the vDSO, since absolute alternative entries are subject to dynamic relocation, which is incompatible with the vDSO building.
--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)
arch/riscv/errata/sifive/errata.c +3 -1
@@ -107,7 +107,9 @@ 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 ((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset, + alt->alt_len); cpu_apply_errata |= tmp; } }
arch/riscv/errata/thead/errata.c +8 -3
@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )
@@ -77,12 +78,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 = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset; +/* 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 (updptr), + alt->alt_len);else -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +patch_text_nosync (oldptr, updptr, alt->alt_len); } }
arch/riscv/include/asm/alternative-macros.h +10 -10
@@ -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 + .long \oldptr - . + .long \newptr - . + .short \vendor_id + .short \new_len + .long \errata_id .endm .macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg
@@ -75,11 +75,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" + ".long ((" oldptr ") - .) \n" \ + ".long ((" newptr ") - .) \n" \ + ".short " vendor_id "\n" \ + ".short " newlen "\n" \ + ".long " errata_id "\n"#define ALT_NEW_CONTENT (vendor_id, errata_id, enable, new_c) \ ".if " __stringify(enable) " == 1\n" \
arch/riscv/include/asm/alternative.h +6 -6
@@ -33,12 +33,12 @@ void riscv_alternative_fix_jal(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 to original instruciton or data */ +s32 alt_offset;/* offset 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;
arch/riscv/kernel/cpufeature.c +6 -7
@@ -257,6 +257,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, unsigned int stage) {struct alt_entry *alt; +void *oldptr, *updptr;if (stage ==RISCV_ALTERNATIVES_EARLY_BOOT )return ;
@@ -270,17 +271,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, continue; } + oldptr = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset;if (!__riscv_isa_extension_available (NULL , alt->errata_id))continue ;/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); -riscv_alternative_fix_jal (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); +patch_text_nosync (oldptr, updptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (oldptr, alt->alt_len, oldptr - updptr); +riscv_alternative_fix_jal (oldptr, alt->alt_len, oldptr - updptr); } }#endif
Signature · 1 line
2.37.2
Re: [PATCH v2 09/13] riscv: switch to relative alternative entries
Quoted 22 lines · Jisheng Zhang
Instead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the
alt_entryvalues. So we can not only cut the size of the alternative entry, but also meet 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]>--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)arch/riscv/errata/sifive/errata.c +0
-0
arch/riscv/errata/sifive/errata.c void __init_or_module sifive_errata_patch_func(struct alt_entry *begin, · line 111
Quoted 6 lines
arch/riscv/errata/sifive/errata.c +2
-1@@ -107,7 +107,9 @@ 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((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset,
(void *)&alt->alt_offset + alt->alt_offset. ??!!
Quoted 135 lines
arch/riscv/errata/sifive/errata.c +1
-0+ alt->alt_len); cpu_apply_errata |= tmp; } }arch/riscv/errata/thead/errata.c +8
-3@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )@@ -77,12 +78,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 = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset; +/* 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 (updptr), + alt->alt_len);else -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +patch_text_nosync (oldptr, updptr, alt->alt_len); } }arch/riscv/include/asm/alternative-macros.h +10
-10@@ -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 + .long \oldptr - . + .long \newptr - . + .short \vendor_id + .short \new_len + .long \errata_id .endm .macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg@@ -75,11 +75,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" + ".long ((" oldptr ") - .) \n" \ + ".long ((" newptr ") - .) \n" \ + ".short " vendor_id "\n" \ + ".short " newlen "\n" \ + ".long " errata_id "\n"#define ALT_NEW_CONTENT (vendor_id, errata_id, enable, new_c) \ ".if " __stringify(enable) " == 1\n" \arch/riscv/include/asm/alternative.h +6
-6@@ -33,12 +33,12 @@ void riscv_alternative_fix_jal(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 to original instruciton or data */ +s32 alt_offset;/* offset 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;arch/riscv/kernel/cpufeature.c +6
-8@@ -257,6 +257,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, unsigned int stage) {struct alt_entry *alt; +void *oldptr, *updptr;if (stage ==RISCV_ALTERNATIVES_EARLY_BOOT )return ;@@ -270,17 +271,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, continue; } + oldptr = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset;if (!__riscv_isa_extension_available (NULL , alt->errata_id))continue ;/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); -riscv_alternative_fix_jal (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); +patch_text_nosync (oldptr, updptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (oldptr, alt->alt_len, oldptr - updptr); +riscv_alternative_fix_jal (oldptr, alt->alt_len, oldptr - updptr); } }#endif --2.37.2
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 09/13] riscv: switch to relative alternative entries
Quoted 24 lines · Guo Ren
Quoted 23 lines · Jisheng Zhang
Instead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the
alt_entryvalues. So we can not only cut the size of the alternative entry, but also meet 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]>--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)arch/riscv/errata/sifive/errata.c +0
-0@@ -107,7 +107,9 @@ void __init_or_module sifive_errata_patch_func(struct alt_entry *begin,
Quoted 6 lines
arch/riscv/errata/sifive/errata.c void __init_or_module sifive_errata_patch_func(struct alt_entry *begin, · line 111
Quoted 5 lines
arch/riscv/errata/sifive/errata.c +2
-1tmp = (1U << alt->errata_id);if (cpu_req_errata & tmp) { -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); + patch_text_nosync((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset,(void *)&alt->alt_offset +
alt->alt_offset. ??!!
Hi Guo,
what's the problem? I can't catch your meaning, could you please proide more details?
Thanks
Quoted 141 lines
Quoted 136 lines
arch/riscv/errata/sifive/errata.c +1
-0+ alt->alt_len); cpu_apply_errata |= tmp; } }arch/riscv/errata/thead/errata.c +8
-3@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )@@ -77,12 +78,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 = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset; +/* 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 (updptr), + alt->alt_len);else -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +patch_text_nosync (oldptr, updptr, alt->alt_len); } }arch/riscv/include/asm/alternative-macros.h +10
-10@@ -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 + .long \oldptr - . + .long \newptr - . + .short \vendor_id + .short \new_len + .long \errata_id .endm .macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg@@ -75,11 +75,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" + ".long ((" oldptr ") - .) \n" \ + ".long ((" newptr ") - .) \n" \ + ".short " vendor_id "\n" \ + ".short " newlen "\n" \ + ".long " errata_id "\n"#define ALT_NEW_CONTENT (vendor_id, errata_id, enable, new_c) \ ".if " __stringify(enable) " == 1\n" \arch/riscv/include/asm/alternative.h +6
-6@@ -33,12 +33,12 @@ void riscv_alternative_fix_jal(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 to original instruciton or data */ +s32 alt_offset;/* offset 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;arch/riscv/kernel/cpufeature.c +6
-8@@ -257,6 +257,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, unsigned int stage) {struct alt_entry *alt; +void *oldptr, *updptr;if (stage ==RISCV_ALTERNATIVES_EARLY_BOOT )return ;@@ -270,17 +271,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, continue; } + oldptr = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset;if (!__riscv_isa_extension_available (NULL , alt->errata_id))continue ;/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); -riscv_alternative_fix_jal (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); +patch_text_nosync (oldptr, updptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (oldptr, alt->alt_len, oldptr - updptr); +riscv_alternative_fix_jal (oldptr, alt->alt_len, oldptr - updptr); } }#endif --2.37.2
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 09/13] riscv: switch to relative alternative entries
Quoted 33 lines · Jisheng Zhang
Quoted 24 lines · Guo Ren
Quoted 23 lines · Jisheng Zhang
Instead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the
alt_entryvalues. So we can not only cut the size of the alternative entry, but also meet 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]>--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)arch/riscv/errata/sifive/errata.c +0
-0@@ -107,7 +107,9 @@ void __init_or_module sifive_errata_patch_func(struct alt_entry *begin,Quoted 6 lines
arch/riscv/errata/sifive/errata.c void __init_or_module sifive_errata_patch_func(struct alt_entry *begin, · line 111
Quoted 5 lines
arch/riscv/errata/sifive/errata.c +2
-1tmp = (1U << alt->errata_id);if (cpu_req_errata & tmp) { -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); + patch_text_nosync((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset,(void *)&alt->alt_offset +
alt->alt_offset. ??!!
Quoted 4 lines
Hi Guo,
what's the problem? I can't catch your meaning, could you please proide more details?
Can you explain why:
alt->old_ptr = (void *)&alt->old_offset + alt->old_offset
| offset | <- &offset | ... | | value | <- ptr = &offset + offset
I don't make sense of the above.
Quoted 144 lines
Thanks
Quoted 141 lines
Quoted 134 lines
arch/riscv/errata/sifive/errata.c +1
-0+ alt->alt_len); cpu_apply_errata |= tmp; } }arch/riscv/errata/thead/errata.c +8
-3@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )@@ -77,12 +78,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 = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset; +/* 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 (updptr), + alt->alt_len);else -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +patch_text_nosync (oldptr, updptr, alt->alt_len); } }arch/riscv/include/asm/alternative-macros.h +10
-10@@ -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 + .long \oldptr - . + .long \newptr - . + .short \vendor_id + .short \new_len + .long \errata_id .endm .macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg@@ -75,11 +75,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" + ".long ((" oldptr ") - .) \n" \ + ".long ((" newptr ") - .) \n" \ + ".short " vendor_id "\n" \ + ".short " newlen "\n" \ + ".long " errata_id "\n"#define ALT_NEW_CONTENT (vendor_id, errata_id, enable, new_c) \ ".if " __stringify(enable) " == 1\n" \arch/riscv/include/asm/alternative.h +6
-6@@ -33,12 +33,12 @@ void riscv_alternative_fix_jal(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 to original instruciton or data */ +s32 alt_offset;/* offset 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;arch/riscv/kernel/cpufeature.c +6
-8@@ -257,6 +257,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, unsigned int stage) {struct alt_entry *alt; +void *oldptr, *updptr;if (stage ==RISCV_ALTERNATIVES_EARLY_BOOT )return ;@@ -270,17 +271,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, continue; } + oldptr = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset;if (!__riscv_isa_extension_available (NULL , alt->errata_id))continue ;/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); -riscv_alternative_fix_jal (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); +patch_text_nosync (oldptr, updptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (oldptr, alt->alt_len, oldptr - updptr); +riscv_alternative_fix_jal (oldptr, alt->alt_len, oldptr - updptr); } }#endif --Quoted 2 lines
2.37.2
--
Best Regards
Guo Ren
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 09/13] riscv: switch to relative alternative entries
Quoted 34 lines · Guo Ren
Quoted 33 lines · Jisheng Zhang
Quoted 30 lines · Guo Ren
Quoted 22 lines · Jisheng Zhang
Instead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the
alt_entryvalues. So we can not only cut the size of the alternative entry, but also meet 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]>--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)arch/riscv/errata/sifive/errata.c +0
-0arch/riscv/errata/sifive/errata.c void __init_or_module sifive_errata_patch_func(struct alt_entry *begin, · line 111
Quoted 6 lines
arch/riscv/errata/sifive/errata.c +2
-1@@ -107,7 +107,9 @@ 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((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset,(void *)&alt->alt_offset +
alt->alt_offset. ??!!
Quoted 7 lines
Quoted 4 lines
Hi Guo,
what's the problem? I can't catch your meaning, could you please proide more details?
Can you explain why:
alt->old_ptr= (void *)&alt->old_offset +alt->old_offset
Hi,
when constructing the alt entry, we save the offset in then entry as below:
.long \oldptr - .
So we can restore the old_ptr by &alt->old_offset + alt->old_offset
Thanks
Quoted 157 lines
| offset | <- &offset | ... | | value | <- ptr = &offset + offsetI don't make sense of the above.
Quoted 144 lines
Thanks
Quoted 141 lines
Quoted 134 lines
arch/riscv/errata/sifive/errata.c +1
-0+ alt->alt_len); cpu_apply_errata |= tmp; } }arch/riscv/errata/thead/errata.c +8
-3@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )@@ -77,12 +78,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 = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset; +/* 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 (updptr), + alt->alt_len);else -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); +patch_text_nosync (oldptr, updptr, alt->alt_len); } }arch/riscv/include/asm/alternative-macros.h +10
-10@@ -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 + .long \oldptr - . + .long \newptr - . + .short \vendor_id + .short \new_len + .long \errata_id .endm .macro ALT_NEW_CONTENT vendor_id, errata_id, enable = 1, new_c : vararg@@ -75,11 +75,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" + ".long ((" oldptr ") - .) \n" \ + ".long ((" newptr ") - .) \n" \ + ".short " vendor_id "\n" \ + ".short " newlen "\n" \ + ".long " errata_id "\n"#define ALT_NEW_CONTENT (vendor_id, errata_id, enable, new_c) \ ".if " __stringify(enable) " == 1\n" \arch/riscv/include/asm/alternative.h +6
-6@@ -33,12 +33,12 @@ void riscv_alternative_fix_jal(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 to original instruciton or data */ +s32 alt_offset;/* offset 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;arch/riscv/kernel/cpufeature.c +6
-8@@ -257,6 +257,7 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, unsigned int stage) {struct alt_entry *alt; +void *oldptr, *updptr;if (stage ==RISCV_ALTERNATIVES_EARLY_BOOT )return ;@@ -270,17 +271,15 @@ void __init_or_module riscv_cpufeature_patch_func(struct alt_entry *begin, continue; } + oldptr = (void *)&alt->old_offset + alt->old_offset; + updptr = (void *)&alt->alt_offset + alt->alt_offset;if (!__riscv_isa_extension_available (NULL , alt->errata_id))continue ;/* do the basic patching */ -patch_text_nosync (alt->old_ptr, alt->alt_ptr, alt->alt_len); -riscv_alternative_fix_auipc_jalr (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); -riscv_alternative_fix_jal (alt->old_ptr, - alt->alt_len, - alt->old_ptr - alt->alt_ptr); +patch_text_nosync (oldptr, updptr, alt->alt_len); +riscv_alternative_fix_auipc_jalr (oldptr, alt->alt_len, oldptr - updptr); +riscv_alternative_fix_jal (oldptr, alt->alt_len, oldptr - updptr); } }#endif --Quoted 2 lines
2.37.2
--
Best Regards
Guo RenSignature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 09/13] riscv: switch to relative alternative entries
Hey Jisheng, Guo Ren,
Quoted 8 lines · Jisheng Zhang
Quoted 7 lines · Guo Ren
Quoted 6 lines · Jisheng Zhang
Quoted 5 lines · Guo Ren
Quoted 4 lines · Jisheng Zhang
Instead of using absolute addresses for both the old instrucions and the alternative instructions, use offsets relative to the
alt_entryvalues. So we can not only cut the size of the alternative entry, but
"This not only cuts"
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
also meet the prerequisite for patching alternatives in the vDSO, since absolute alternative entries are subject to dynamic relocation, which is incompatible with the vDSO building.
I do this this is in the wrong order though, saving on size is secondary to enabling their use in the vdso?
Quoted 31 lines
Quoted 23 lines
Quoted 23 lines
Quoted 22 lines
Quoted 15 lines
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/errata/sifive/errata.c | 4 +++- arch/riscv/errata/thead/errata.c | 11 ++++++++--- arch/riscv/include/asm/alternative-macros.h | 20 ++++++++++---------- arch/riscv/include/asm/alternative.h | 12 ++++++------ arch/riscv/kernel/cpufeature.c | 13 ++++++------- 5 files changed, 33 insertions(+), 27 deletions(-)arch/riscv/errata/sifive/errata.c +0
-0arch/riscv/errata/sifive/errata.c void __init_or_module sifive_errata_patch_func(struct alt_entry *begin, · line 111
Quoted 6 lines
arch/riscv/errata/sifive/errata.c +2
-1@@ -107,7 +107,9 @@ 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((void *)&alt->old_offset + alt->old_offset, + (void *)&alt->alt_offset + alt->alt_offset,(void *)&alt->alt_offset +
alt->alt_offset. ??!!Quoted 7 lines
Quoted 4 lines
Hi Guo,
what's the problem? I can't catch your meaning, could you please proide more details?
Can you explain why:
alt->old_ptr= (void *)&alt->old_offset +alt->old_offset
Quoted 8 lines
Hi,
when constructing the alt entry, we save the offset in then entry as below:
.long \oldptr - .
So we can restore the
old_ptrby&alt->old_offset+alt->old_offset
Please correct me if I have misunderstood, but for stuff like this I find it useful to kinda summarise a bit and figure out for myself what is going on..
As things stand, we have absolute "locations" for the alternative and "old" instructions/data/functions. Your commit is converting us over to use offsets. The code that patches in the alternatives needs to have absolute addresses though, so you need to be able to, effectively, reverse engineer those from the offset.
You do this by taking the address of the offset & adding the offset to the address before casting to (void *). This works, because the offset is the offset from the alt_entry data structure to the alternative?
I hope I am in the right ballpark there haha, but I do think that this really needs a comment explaining what it is doing. Maybe extract that operation into some sort of macro in alternatives.h so the operation is done in a central location & you can leave the comment there?
That'd make it at least more manageable for us mere mortals who can just do something like patch_text_nosync(ALT_OFFSET_ADDRESS(alt->old_offset),
ALT_OFFSET_ADDRESS(alt->alt_offset), alt->alt_len);
when we have to go an add some alternatives..
Quoted 11 lines
Quoted 11 lines
| offset | <- &offset | ... | | value | <- ptr = &offset + offsetI don't make sense of the above.
Quoted 4 lines
Thanks
Quoted 1 line
Quoted 13 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
arch/riscv/errata/sifive/errata.c +1
-0+ alt->alt_len); cpu_apply_errata |= tmp; } }arch/riscv/errata/thead/errata.c +0
-0Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
arch/riscv/errata/thead/errata.c void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct al · line 71
Quoted 8 lines
index
21546937db39..2a6e335b5a32100644arch/riscv/errata/thead/errata.c +1
-0@@ -68,6 +68,7 @@ void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct alstruct alt_entry *alt;u32 cpu_req_errata =thead_errata_probe (stage, archid, impid);u32 tmp; +void *oldptr, *updptr;
Why mix the terminology with "upd" instead of "alt"?
Quoted 4 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
arch/riscv/errata/thead/errata.c +0
-0for (alt = begin; alt < end; alt++) {if (alt->vendor_id !=THEAD_VENDOR_ID )
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
arch/riscv/errata/thead/errata.c void __init_or_module thead_errata_patch_func(struct alt_entry *begin, struct al
Quoted 9 lines
arch/riscv/errata/thead/errata.c +2
-6struct 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 to original instruciton or data */ +s32 alt_offset;/* offset to replacement instruction or data */
Perhaps also this comment could be expanded on to specify what it is an offset *from* as well as to?
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
arch/riscv/errata/thead/errata.c
Quoted 4 lines
arch/riscv/errata/thead/errata.c +4
-0+u16 vendor_id;/* cpu vendor id */ +u16 alt_len;/* The replacement size */ +u32 errata_id;/* The errata id */ +};
I hope I didn't make a hames of trying to understand what you were doing, but please let me know what I have undoubtedly got mixed up on! Thanks,
Conor.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221206/fc08ff0d/attachment-0001.sig>
PATCH v2 10/13 riscv: alternative: patch alternatives in the vDSO
In reply to Jisheng ZhangMake it possible to use alternatives in the vDSO, so that better implementations can be used if possible.
--- arch/riscv/include/asm/vdso.h | 4 ++++ arch/riscv/kernel/alternative.c | 25 +++++++++++++++++++++++++ arch/riscv/kernel/vdso.c | 5 ----- arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++ 4 files changed, 36 insertions(+), 5 deletions(-)
arch/riscv/include/asm/vdso.h +4 -0
@@ -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 */
arch/riscv/kernel/alternative.c +25 -0
@@ -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>
@@ -187,6 +189,27 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin, stage); } +static void __init apply_vdso_alternatives (void ) +{ +const struct elf64_hdr *hdr; +const struct elf64_shdr *shdr; +const struct elf64_shdr *alt; +struct alt_entry *begin, *end; + + hdr = (struct elf64_hdr *)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 ); +} +void __init apply_boot_alternatives (void ) {/* If called on non-boot cpu things could go wrong */
@@ -195,6 +218,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 (); }/*
arch/riscv/kernel/vdso.c +0 -5
@@ -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 ,
arch/riscv/kernel/vdso/vdso.lds.S +7 -0
@@ -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.*)
Signature · 1 line
2.37.2
Re: [PATCH v2 10/13] riscv: alternative: patch alternatives in the vDSO
Are there any patches that depend on it? Any existing utilization? My first idea is to let __vdso_flush_icache use it, the standard implementation is so heavy for user space JIT scenario, maybe vendors could give a custom one.
Quoted 117 lines · Jisheng Zhang
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]>--- arch/riscv/include/asm/vdso.h | 4 ++++ arch/riscv/kernel/alternative.c | 25 +++++++++++++++++++++++++ arch/riscv/kernel/vdso.c | 5 ----- arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++ 4 files changed, 36 insertions(+), 5 deletions(-)arch/riscv/include/asm/vdso.h +4
-0@@ -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 */ arch/riscv/kernel/alternative.c +25
-0@@ -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> @@ -187,6 +189,27 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin, stage); } +static void __init apply_vdso_alternatives (void ) +{ +const struct elf64_hdr *hdr; +const struct elf64_shdr *shdr; +const struct elf64_shdr *alt; +struct alt_entry *begin, *end; + + hdr = (struct elf64_hdr *)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 ); +} +void __init apply_boot_alternatives (void ) {/* If called on non-boot cpu things could go wrong */ @@ -195,6 +218,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 (); }/* arch/riscv/kernel/vdso.c +0
-5@@ -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 ,arch/riscv/kernel/vdso/vdso.lds.S +7
-1@@ -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.37.2
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 10/13] riscv: alternative: patch alternatives in the vDSO
Quoted 4 lines · Guo Ren
Are there any patches that depend on it? Any existing utilization? My first idea is to let
__vdso_flush_icacheuse it, the standard implementation is so heavy for user space JIT scenario, maybe vendors could give a custom one.
Hi Guo,
the 11th patch of swtiching cpu_relax() to riscv_has_extension_likely() depends on this patch, the gettimeofday implementation calls cpu_relax() which may use ZIHINTPAUSE extension, so we need to patch vDSO.
Thanks
Quoted 124 lines · Jisheng Zhang
Quoted 117 lines
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]>--- arch/riscv/include/asm/vdso.h | 4 ++++ arch/riscv/kernel/alternative.c | 25 +++++++++++++++++++++++++ arch/riscv/kernel/vdso.c | 5 ----- arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++ 4 files changed, 36 insertions(+), 5 deletions(-)arch/riscv/include/asm/vdso.h +4
-0@@ -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 */ arch/riscv/kernel/alternative.c +25
-0@@ -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> @@ -187,6 +189,27 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin, stage); } +static void __init apply_vdso_alternatives (void ) +{ +const struct elf64_hdr *hdr; +const struct elf64_shdr *shdr; +const struct elf64_shdr *alt; +struct alt_entry *begin, *end; + + hdr = (struct elf64_hdr *)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 ); +} +void __init apply_boot_alternatives (void ) {/* If called on non-boot cpu things could go wrong */ @@ -195,6 +218,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 (); }/* arch/riscv/kernel/vdso.c +0
-5@@ -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 ,arch/riscv/kernel/vdso/vdso.lds.S +7
-1@@ -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.37.2
Signature · 2 lines
Best Regards Guo Ren
Guo Ren
Reviewed-byRe: [PATCH v2 10/13] riscv: alternative: patch alternatives in the vDSO
Quoted 7 lines · Jisheng Zhang
Quoted 4 lines · Guo Ren
Are there any patches that depend on it? Any existing utilization? My first idea is to let
__vdso_flush_icacheuse it, the standard implementation is so heavy for user space JIT scenario, maybe vendors could give a custom one.
Quoted 5 lines
Hi Guo,
the 11th patch of swtiching
cpu_relax()toriscv_has_extension_likely()depends on this patch, the gettimeofday implementation callscpu_relax()which may use ZIHINTPAUSE extension, so we need to patch vDSO.
Oh, I missed that. Thx.
Quoted 127 lines
Thanks
Quoted 124 lines · Jisheng Zhang
Quoted 115 lines
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]>--- arch/riscv/include/asm/vdso.h | 4 ++++ arch/riscv/kernel/alternative.c | 25 +++++++++++++++++++++++++ arch/riscv/kernel/vdso.c | 5 ----- arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++ 4 files changed, 36 insertions(+), 5 deletions(-)arch/riscv/include/asm/vdso.h +4
-0@@ -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 */ arch/riscv/kernel/alternative.c +25
-0@@ -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> @@ -187,6 +189,27 @@ static void __init_or_module _apply_alternatives(struct alt_entry *begin, stage); } +static void __init apply_vdso_alternatives (void ) +{ +const struct elf64_hdr *hdr; +const struct elf64_shdr *shdr; +const struct elf64_shdr *alt; +struct alt_entry *begin, *end; + + hdr = (struct elf64_hdr *)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 ); +} +void __init apply_boot_alternatives (void ) {/* If called on non-boot cpu things could go wrong */ @@ -195,6 +218,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 (); }/* arch/riscv/kernel/vdso.c +0
-5@@ -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 ,arch/riscv/kernel/vdso/vdso.lds.S +7
-1@@ -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.*) --Quoted 2 lines
2.37.2
--
Best Regards
Guo Ren
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 10/13] riscv: alternative: patch alternatives in the vDSO
In reply to Jisheng ZhangQuoted 11 lines · Jisheng Zhang
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]>--- arch/riscv/include/asm/vdso.h | 4 ++++ arch/riscv/kernel/alternative.c | 25 +++++++++++++++++++++++++ arch/riscv/kernel/vdso.c | 5 ----- arch/riscv/kernel/vdso/vdso.lds.S | 7 +++++++ 4 files changed, 36 insertions(+), 5 deletions(-)
PATCH v2 11/13 riscv: cpu_relax: switch to riscv_has_extension_likely()
In reply to Jisheng ZhangSwitch cpu_relax() from statich branch to the new helper riscv_has_extension_likely()
--- arch/riscv/include/asm/vdso/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
arch/riscv/include/asm/vdso/processor.h +1 -1
@@ -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_muldivint dummy;/* In lieu of a halt instruction, induce a long-latency stall. */
Signature · 1 line
2.37.2
Re: [PATCH v2 11/13] riscv: cpu_relax: switch to riscv_has_extension_likely()
Quoted 27 lines · Jisheng Zhang
Switch
cpu_relax()from statich branch to the new helperriscv_has_extension_likely()Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Andrew Jones <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>--- arch/riscv/include/asm/vdso/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/vdso/processor.h +1
-2@@ -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_muldivint dummy;/* In lieu of a halt instruction, induce a long-latency stall. */ --2.37.2
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 11/13] riscv: cpu_relax: switch to riscv_has_extension_likely()
In reply to Jisheng ZhangQuoted 1 line · Jisheng Zhang
Switch
cpu_relax()from statich branch to the new helper
The tiniest nit: static
Quoted 25 lines
riscv_has_extension_likely()Signed-off-by Jisheng Zhang <[email protected]>Reviewed-by Andrew Jones <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>--- arch/riscv/include/asm/vdso/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)arch/riscv/include/asm/vdso/processor.h +1
-1@@ -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_muldivint dummy;/* In lieu of a halt instruction, induce a long-latency stall. */ Signature · 1 line
2.37.2
PATCH v2 12/13 riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
In reply to Jisheng ZhangFrom: Andrew Jones <[email protected]>
Switch has_svinval() from static branch to the new helper riscv_has_extension_unlikely().
--- arch/riscv/kvm/tlb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
arch/riscv/kvm/tlb.c +1 -2
@@ -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,
Signature · 1 line
2.37.2
Re: [PATCH v2 12/13] riscv: KVM: Switch has_svinval() to riscv_has_extension_unlikely()
Quoted 28 lines · Jisheng Zhang
From: Andrew Jones <[email protected]>
Switch
has_svinval()from static branch to the new helperriscv_has_extension_unlikely().Signed-off-by Andrew Jones <[email protected]>--- arch/riscv/kvm/tlb.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)arch/riscv/kvm/tlb.c +1
-3@@ -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.37.2
Signature · 2 lines
Best Regards Guo Ren
PATCH v2 13/13 riscv: remove riscv_isa_ext_keys[] array and related usage
In reply to Jisheng ZhangAll users have switched to riscv_has_extension_*, removed unused definitions, vars and related setting code.
--- arch/riscv/include/asm/hwcap.h | 30 ------------------------------ arch/riscv/kernel/cpufeature.c | 9 --------- 2 files changed, 39 deletions(-)
arch/riscv/include/asm/hwcap.h +0 -30
@@ -60,18 +60,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. -*/ -enum riscv_isa_ext_key { -RISCV_ISA_EXT_KEY_FPU ,/* For 'F' and 'D' */ -RISCV_ISA_EXT_KEY_ZIHINTPAUSE , -RISCV_ISA_EXT_KEY_SVINVAL , -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 ];
@@ -79,24 +67,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_ZIHINTPAUSE : -return RISCV_ISA_EXT_KEY_ZIHINTPAUSE ; -case RISCV_ISA_EXT_SVINVAL : -return RISCV_ISA_EXT_KEY_SVINVAL ; -default : -return -EINVAL ; - } -} - static __always_inline boolriscv_has_extension_likely (const unsigned long ext) {
arch/riscv/kernel/cpufeature.c +0 -9
@@ -28,9 +28,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 *
@@ -243,12 +240,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
Signature · 1 line
2.37.2
Re: [PATCH v2 13/13] riscv: remove riscv_isa_ext_keys[] array and related usage
Quoted 47 lines · Jisheng Zhang
All users have switched to riscv_has_extension_*, removed 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]>--- arch/riscv/include/asm/hwcap.h | 30 ------------------------------ arch/riscv/kernel/cpufeature.c | 9 --------- 2 files changed, 39 deletions(-)arch/riscv/include/asm/hwcap.h +0
-20@@ -60,18 +60,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. -*/ -enum riscv_isa_ext_key { -RISCV_ISA_EXT_KEY_FPU ,/* For 'F' and 'D' */ -RISCV_ISA_EXT_KEY_ZIHINTPAUSE , -RISCV_ISA_EXT_KEY_SVINVAL , -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 ];@@ -79,24 +67,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:
arch/riscv/include/asm/hwcap.h struct riscv_isa_ext_data { · line 99
Quoted 10 lines
arch/riscv/include/asm/hwcap.h +0
-10-return RISCV_ISA_EXT_KEY_FPU ; -case RISCV_ISA_EXT_ZIHINTPAUSE : -return RISCV_ISA_EXT_KEY_ZIHINTPAUSE ; -case RISCV_ISA_EXT_SVINVAL : -return RISCV_ISA_EXT_KEY_SVINVAL ; -default : -return -EINVAL ; - } -} -
Quoted 18 lines
arch/riscv/include/asm/hwcap.h +0
-0static __always_inline boolriscv_has_extension_likely (const unsigned long ext) {arch/riscv/kernel/cpufeature.c +0
-3@@ -28,9 +28,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 *
arch/riscv/kernel/cpufeature.c void __init riscv_fill_hwcap(void) · line 251
Quoted 9 lines
arch/riscv/kernel/cpufeature.c +0
-6@@ -243,12 +240,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]); - }
Quoted 6 lines
arch/riscv/kernel/cpufeature.c +0
-1}#ifdef CONFIG_RISCV_ALTERNATIVE --2.37.2
Signature · 2 lines
Best Regards Guo Ren
Re: [PATCH v2 13/13] riscv: remove riscv_isa_ext_keys[] array and related usage
In reply to Jisheng ZhangQuoted 1 line · Jisheng Zhang
All users have switched to riscv_has_extension_*, removed unused
minor nit: remove
Quoted 5 lines
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]>
May as well join the R-b club here, the removal of 2 places where extensions need to be kept ordered is especially appreciated!
Thanks!
Quoted 83 lines
--- arch/riscv/include/asm/hwcap.h | 30 ------------------------------ arch/riscv/kernel/cpufeature.c | 9 --------- 2 files changed, 39 deletions(-)arch/riscv/include/asm/hwcap.h +0
-30@@ -60,18 +60,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. -*/ -enum riscv_isa_ext_key { -RISCV_ISA_EXT_KEY_FPU ,/* For 'F' and 'D' */ -RISCV_ISA_EXT_KEY_ZIHINTPAUSE , -RISCV_ISA_EXT_KEY_SVINVAL , -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 ];@@ -79,24 +67,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_ZIHINTPAUSE : -return RISCV_ISA_EXT_KEY_ZIHINTPAUSE ; -case RISCV_ISA_EXT_SVINVAL : -return RISCV_ISA_EXT_KEY_SVINVAL ; -default : -return -EINVAL ; - } -} - static __always_inline boolriscv_has_extension_likely (const unsigned long ext) {arch/riscv/kernel/cpufeature.c +0
-9@@ -28,9 +28,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 * @@ -243,12 +240,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 Signature · 1 line
2.37.2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/kvm-riscv/attachments/20221206/f876de99/attachment-0001.sig>