[RFC PATCH 00/16] arm64: make alternative patching callbacks safe

[RFC PATCH 00/16] arm64: make alternative patching callbacks safe

[Ada Couprie Diaz]

Hi all,

This started as an optimization of an alternative (patch 16) but quickly
devolved into a patching rabbit hole, raising more questions than answers.
Hence my looking for comments and some of the patches being
"half-done", as per the notes you will find in patches 7 and 12.


Currently, we use a few callbacks to patch the kernel code, mostly
for KVM and Spectre mitigation handling.
However, almost all of those callbacks are instrumentable or call
instrumentable functions : which means they can be patched themselves.

While applying alternatives, there is no guarantee that a function
might be in a consistent state if it is called by the patching callback,
nor is there a guarantee that the patching of a function calling another
will be consistent between both.
Further, `__apply_alternatives()` doesn't flush the instruction cache
until having applied all alternatives, so there is a possibility that
a patching callback or one of its callees might have been patched
while the I-cache contains an unpatched or partially patched version.

This has (mostly[0]) not blown up so far, but it is mechanically unsound :
we cannot be sure it will not happen in the future.


The goal of this series is to gather feedback on how we can make sure
that the patching callbacks we use are safe and not instrumentable,
as well as all of their callees.
Details on the thought process and the results follow, with open questions
at then end.


Callbacks are made safe when all their callees are, current callbacks
are covered by patches 1-12.
Patches 13-16 are illustrative of what might be required to implement
a new callback using functions not yet covered.


Reasoning
===

I felt that the safest way to be sure that the callbacks would not be
instrumented would be if they were `noinstr` and if all of their callees
were `__always_inline`, or `noinstr` if that were not possible.
That way, if the callback itself is not patchable neither would be any
of the functions it calls, nor any of those that they call, etc.
(Marking all patching callbacks `noinstr` would also make them easily
indentifialbe as internal callbacks, re:[1])

I noted the following alternative callbacks, and went through all of their
callees recursively :
 - kvm_compute_final_ctr_el0
 - kvm_get_kimage_voffset
 - kvm_update_va_mask
 - kvm_patch_vector_branch
 - spectre_bhb_patch_loop_iter (noinstr)
 - spectre_bhb_patch_loop_mitigation_enable (safe, noinstr)
 - spectre_bhb_patch_clearbhb
 - spectre_bhb_patch_wa3 (noinstr)
 - spectre_v4_patch_fw_mitigation_enable
 - smccc_patch_fw_mitigation_conduit
 - kasan_hw_tags_enable
 - alt_cb_patch_nops (safe, noinstr)

Only a couple of them are already safe, and a few more `noinstr` but
calling not inlined nor `noinstr` functions.


The largest source of unsafe functions being called is the
`aarch64_insn_...` functions. There is a large number of them, but only
a few are used in alternative callbacks (directly or transitively).
As they are usually quite simple it made sense to `__always_inline`
those few used in callbacks, which also limits the scope of a complete
`insn` rework.
All the `...get_<insn>_value()`/`is_<insn>()` are `__always_inline`
already, which reduces the number of functions to take care of.


The second one is calls to `printk`, throug `WARN`s or `pr_...` functions.
This is something that we cannot make `__always_inline` or `noinstr`,
so we must either remove them entirely or find another way to make
the information available.

`aarch64_insn_...` functions call `pr_err` a lot to denote invalid
input data, but it is often a dynamically provided argument : if not
in the callbacks, in other use cases (mostly, the BPF JIT compiler).
I removed those as they should always lead to an a break fault instruction
being generated, though the source of the issue becomes less clear.
In cases where the arguments are all available at compile time, I replaced
the runtime `pr_err()` by a `compiletime_assert()`, as a way to preserve
some of the error messages.


Outcome
===

With this series, most of the callbacks are deemede "safe", with
the following exceptions :
 - kvm_patch_vector_branch
 - spectre_bhb_patch_wa3
 - spectre_v4_patch_fw_mitigation_enable
 - smccc_patch_fw_mitigation_conduit

This is due to the use of `WARN`s which I do not know if
they can be safely removed and calling into non-arch code.
There is a bit more info on the Spectre and KVM ones in patches 7 and 12.

This also doesn't (currently, I think it would make sense to do it)
apply the same fixes to the functions called by `patch_alternative()`,
which thus remains "unsound".

There is no size difference in the final image after forcing all those
new inlines with the base defconfig.
A clean compilation on my machine is about 1% faster wall clock time,
using 1% more total CPU time. (20 samples for each, -j110, 125GB of RAM)

This also allows safely introducing a new callback which handles the
Cortex-A57 erratum 832075 (Patch 16), which would be sent separately
after discussion on the RFC.


Open questions
===

There are quite a few things that I am unsure about and would appreciate
some input on :

 - I do prefer when we have error messages, but the current series
   removes a lot of them and fully completing the goal requires the
   removal of more yet.
   - Instead of removing all of them, would it make sense to gate them
     behind a config option (depending on `CONFIG_EXPERT`) ? For example
     `CONFIG_ARM64_UNSAFE_ALTERNATIVE` ? But that would only help for
     developpers or when actively trying to debug.
   - Alternatively, would a command line option make sense ? But then,
     I'm afraid that it would call into more instrumentable/patchable
     functions, leading us back to the beginning. 
   - Are the `compiletime_assert` messages a useful alternative ?
     Are they more limiting than needed ? (Given the arguments _need_
     to be decidable at compile time, that would limit new users that
     create them dynamically)

 - Some alternative callbacks are `__init`. This makes them incompatible
   with the default `noinstr`, as they place functions in different
   text sections. I worked around that for now by using
   `__noinstr_section(".init.text")`, which adds all the `noinstr`
   attributes, but maintains the function in the init section.
   However, it seems to me that Kprobes do not care about the attributes
   and only look at the section to block instrumentation, which could
   be an issue.
   What to do with `__init` callbacks then ? Would this be "good enough" ?
   Is there a proper way to have non-instrumented `__init` functions ?
   What would be the impact of not marking them `__init` anymore ?

 - Given all the limitations and issues above, is this the right way to go
   about it ?

 - `__always_inline`'ing seems to make sense here, but does create
   a disparity in the `aarch64_insn_...` functions,
   but marking everything `noinstr` instead would work as well. Given
   that there is no size difference with and without the patches,
   I would assume that the compiler already inlines them all,
   given we compile with -O2 which considers all functions for inlining.

 - It also means a change of visibility for a few helper functions.
   I have tried to add relevant checks when needed, but I assume there
   were reasons for them to be static to the C file, which they cannot
   be anymore if the functions that need them are `__always_inline`



Thanks very much for taking the time and apologies for another
lengthy cover.
Best,
Ada

Based on v6.17-rc4

[0]: https://lore.kernel.org/all/aNF0gb1iZndz0-be@J2N7QTR9R3/
[1]: https://lore.kernel.org/all/aJnccgC5E-ui2Oqo@willie-the-truck/

Ada Couprie Diaz (16):
  kasan: mark kasan_(hw_)tags_enabled() __always_inline
  arm64: kasan: make kasan_hw_tags_enable() callback safe
  arm64/insn: always inline aarch64_insn_decode_register()
  arm64/insn: always inline aarch64_insn_encode_register()
  arm64/insn: always inline aarch64_insn_encode_immediate()
  arm64/insn: always inline aarch64_insn_gen_movewide()
  arm64/proton-pack: make alternative callbacks safe
  arm64/insn: always inline aarch64_insn_gen_logical_immediate()
  arm64/insn: always inline aarch64_insn_gen_add_sub_imm()
  arm64/insn: always inline aarch64_insn_gen_branch_reg()
  arm64/insn: always inline aarch64_insn_gen_extr()
  kvm/arm64: make alternative callbacks safe
  arm64/insn: introduce missing is_store/is_load helpers
  arm64/insn: always inline aarch64_insn_encode_ldst_size()
  arm64/insn: always inline aarch64_insn_gen_load_acq_store_rel()
  arm64/io: rework Cortex-A57 erratum 832075 to use callback

 arch/arm64/include/asm/insn.h   | 632 ++++++++++++++++++++++++++++++--
 arch/arm64/include/asm/io.h     |  27 +-
 arch/arm64/kernel/image-vars.h  |   1 +
 arch/arm64/kernel/io.c          |  21 ++
 arch/arm64/kernel/mte.c         |   1 +
 arch/arm64/kernel/proton-pack.c |   1 +
 arch/arm64/kvm/va_layout.c      |  12 +-
 arch/arm64/lib/insn.c           | 530 +-------------------------
 include/linux/kasan-enabled.h   |   6 +-
 9 files changed, 657 insertions(+), 574 deletions(-)


base-commit: b320789d6883cc00ac78ce83bccbfe7ed58afcf0
-- 
2.43.0

[RFC PATCH 01/16] kasan: mark kasan_(hw_)tags_enabled() __always_inline

[Ada Couprie Diaz]

`kasan_hw_tags_enabled()` and `kasan_tags_enabled()` are marked inline,
except `kasan_enabled()` with `CONFIG_KASAN_HW_TAGS`,
which is marked `__always_inline`.

Those functions are called in the arm64 alternative callback
`kasan_hw_tags_enable()`, which requires them to be inlined to avoid
being instrumented and safe for patching.

For consistency between the four declarations and to make the arm64
alternative callback safe, mark them `__always_inline`.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 include/linux/kasan-enabled.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/kasan-enabled.h b/include/linux/kasan-enabled.h
index 6f612d69ea0c..d3d5a2327e11 100644
--- a/include/linux/kasan-enabled.h
+++ b/include/linux/kasan-enabled.h
@@ -13,19 +13,19 @@ static __always_inline bool kasan_enabled(void)
 	return static_branch_likely(&kasan_flag_enabled);
 }
 
-static inline bool kasan_hw_tags_enabled(void)
+static __always_inline bool kasan_hw_tags_enabled(void)
 {
 	return kasan_enabled();
 }
 
 #else /* CONFIG_KASAN_HW_TAGS */
 
-static inline bool kasan_enabled(void)
+static __always_inline bool kasan_enabled(void)
 {
 	return IS_ENABLED(CONFIG_KASAN);
 }
 
-static inline bool kasan_hw_tags_enabled(void)
+static __always_inline bool kasan_hw_tags_enabled(void)
 {
 	return false;
 }
-- 
2.43.0

[RFC PATCH 02/16] arm64: kasan: make kasan_hw_tags_enable() callback safe

[Ada Couprie Diaz]

Alternative callback functions are regular functions, which means they
or any function they call could get patched or instrumented
by alternatives or other parts of the kernel.
Given that applying alternatives does not guarantee a consistent state
while patching, only once done, and handles cache maintenance manually,
it could lead to nasty corruptions and execution of bogus code.

Make `kasan_hw_tags_enable()` safe by preventing its instrumentation.
This is possible thanks to a previous commit making
`kasan_hw_tags_enabled()` always inlined, preventing any instrumentation
in the callback.

As `kasan_hw_tags_enable()` is already marked as `__init`,
which has its own text section conflicting with the `noinstr` one,
use `__no_instr_section(".noinstr.text")` to add
all the function attributes added by `noinstr`, without the section
conflict.
This can be an issue, as kprobes seems to only block the text sections,
not based on function attributes.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/kernel/mte.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/mte.c b/arch/arm64/kernel/mte.c
index e5e773844889..a525c1d0c26d 100644
--- a/arch/arm64/kernel/mte.c
+++ b/arch/arm64/kernel/mte.c
@@ -239,6 +239,7 @@ static void mte_update_gcr_excl(struct task_struct *task)
 void __init kasan_hw_tags_enable(struct alt_instr *alt, __le32 *origptr,
 				 __le32 *updptr, int nr_inst);
 
+__noinstr_section(".init.text")
 void __init kasan_hw_tags_enable(struct alt_instr *alt, __le32 *origptr,
 				 __le32 *updptr, int nr_inst)
 {
-- 
2.43.0

[RFC PATCH 03/16] arm64/insn: always inline aarch64_insn_decode_register()

[Ada Couprie Diaz]

As it is always called with an explicit register type, we can
check for its validity at compile time and remove the runtime error print.

This makes `aarch64_insn_decode_register()` self-contained and safe
for inlining and usage from patching callbacks.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 32 ++++++++++++++++++++++++++++++--
 arch/arm64/lib/insn.c         | 29 -----------------------------
 2 files changed, 30 insertions(+), 31 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 18c7811774d3..f6bce1a62dda 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -7,6 +7,7 @@
  */
 #ifndef	__ASM_INSN_H
 #define	__ASM_INSN_H
+#include <linux/bits.h>
 #include <linux/build_bug.h>
 #include <linux/types.h>
 
@@ -558,8 +559,35 @@ enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
 u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 				  u32 insn, u64 imm);
-u32 aarch64_insn_decode_register(enum aarch64_insn_register_type type,
-					 u32 insn);
+static __always_inline u32 aarch64_insn_decode_register(
+				 enum aarch64_insn_register_type type, u32 insn)
+{
+	compiletime_assert(type >= AARCH64_INSN_REGTYPE_RT &&
+		type <= AARCH64_INSN_REGTYPE_RS, "unknown register type encoding");
+	int shift;
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:
+		shift = 10;
+		break;
+	case AARCH64_INSN_REGTYPE_RM:
+	case AARCH64_INSN_REGTYPE_RS:
+		shift = 16;
+		break;
+	default:
+		return 0;
+	}
+
+	return (insn >> shift) & GENMASK(4, 0);
+}
 u32 aarch64_insn_gen_branch_imm(unsigned long pc, unsigned long addr,
 				enum aarch64_insn_branch_type type);
 u32 aarch64_insn_gen_comp_branch_imm(unsigned long pc, unsigned long addr,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 4e298baddc2e..0fac78e542cf 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -144,35 +144,6 @@ u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 	return insn;
 }
 
-u32 aarch64_insn_decode_register(enum aarch64_insn_register_type type,
-					u32 insn)
-{
-	int shift;
-
-	switch (type) {
-	case AARCH64_INSN_REGTYPE_RT:
-	case AARCH64_INSN_REGTYPE_RD:
-		shift = 0;
-		break;
-	case AARCH64_INSN_REGTYPE_RN:
-		shift = 5;
-		break;
-	case AARCH64_INSN_REGTYPE_RT2:
-	case AARCH64_INSN_REGTYPE_RA:
-		shift = 10;
-		break;
-	case AARCH64_INSN_REGTYPE_RM:
-		shift = 16;
-		break;
-	default:
-		pr_err("%s: unknown register type encoding %d\n", __func__,
-		       type);
-		return 0;
-	}
-
-	return (insn >> shift) & GENMASK(4, 0);
-}
-
 static u32 aarch64_insn_encode_register(enum aarch64_insn_register_type type,
 					u32 insn,
 					enum aarch64_insn_register reg)
-- 
2.43.0

[RFC PATCH 04/16] arm64/insn: always inline aarch64_insn_encode_register()

[Ada Couprie Diaz]

As it is always called with an explicit register type, we can
check for its validity at compile time and remove the runtime error print.
The register and instruction checks cannot be made at compile time,
as they are dynamically created. However, we can remove the error print
as it should never appear in normal operation and will still lead to
a fault BRK.

This makes `aarch64_insn_encode_register()` self-contained and safe
for inlining and usage from patching callbacks.

This is a change of visiblity, as previously the function was private to
lib/insn.c.
However, in order to inline more `aarch64_insn_...` functions and make
patching callbacks safe, it needs to be accessible by those functions.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 42 +++++++++++++++++++++++++++++++++++
 arch/arm64/lib/insn.c         | 42 -----------------------------------
 2 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index f6bce1a62dda..90f271483e5b 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -559,6 +559,48 @@ enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
 u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 				  u32 insn, u64 imm);
+static __always_inline u32 aarch64_insn_encode_register(
+				 enum aarch64_insn_register_type type,
+				 u32 insn,
+				 enum aarch64_insn_register reg)
+{
+	compiletime_assert(type >= AARCH64_INSN_REGTYPE_RT &&
+		type <= AARCH64_INSN_REGTYPE_RS, "unknown register type encoding");
+	int shift;
+
+	if (insn == AARCH64_BREAK_FAULT)
+		return AARCH64_BREAK_FAULT;
+
+	if (reg < AARCH64_INSN_REG_0 || reg > AARCH64_INSN_REG_SP) {
+		return AARCH64_BREAK_FAULT;
+	}
+
+	switch (type) {
+	case AARCH64_INSN_REGTYPE_RT:
+	case AARCH64_INSN_REGTYPE_RD:
+		shift = 0;
+		break;
+	case AARCH64_INSN_REGTYPE_RN:
+		shift = 5;
+		break;
+	case AARCH64_INSN_REGTYPE_RT2:
+	case AARCH64_INSN_REGTYPE_RA:
+		shift = 10;
+		break;
+	case AARCH64_INSN_REGTYPE_RM:
+	case AARCH64_INSN_REGTYPE_RS:
+		shift = 16;
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn &= ~(GENMASK(4, 0) << shift);
+	insn |= reg << shift;
+
+	return insn;
+}
+
 static __always_inline u32 aarch64_insn_decode_register(
 				 enum aarch64_insn_register_type type, u32 insn)
 {
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 0fac78e542cf..1810e1ea64a7 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -144,48 +144,6 @@ u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
 	return insn;
 }
 
-static u32 aarch64_insn_encode_register(enum aarch64_insn_register_type type,
-					u32 insn,
-					enum aarch64_insn_register reg)
-{
-	int shift;
-
-	if (insn == AARCH64_BREAK_FAULT)
-		return AARCH64_BREAK_FAULT;
-
-	if (reg < AARCH64_INSN_REG_0 || reg > AARCH64_INSN_REG_SP) {
-		pr_err("%s: unknown register encoding %d\n", __func__, reg);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	switch (type) {
-	case AARCH64_INSN_REGTYPE_RT:
-	case AARCH64_INSN_REGTYPE_RD:
-		shift = 0;
-		break;
-	case AARCH64_INSN_REGTYPE_RN:
-		shift = 5;
-		break;
-	case AARCH64_INSN_REGTYPE_RT2:
-	case AARCH64_INSN_REGTYPE_RA:
-		shift = 10;
-		break;
-	case AARCH64_INSN_REGTYPE_RM:
-	case AARCH64_INSN_REGTYPE_RS:
-		shift = 16;
-		break;
-	default:
-		pr_err("%s: unknown register type encoding %d\n", __func__,
-		       type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	insn &= ~(GENMASK(4, 0) << shift);
-	insn |= reg << shift;
-
-	return insn;
-}
-
 static const u32 aarch64_insn_ldst_size[] = {
 	[AARCH64_INSN_SIZE_8] = 0,
 	[AARCH64_INSN_SIZE_16] = 1,
-- 
2.43.0

[RFC PATCH 05/16] arm64/insn: always inline aarch64_insn_encode_immediate()

[Ada Couprie Diaz]

As type is passed dynamically at runtime we cannot check at compile time
that is valid.
However, in practice this should not happen and will still result in a
fault BRK, so remove the error print.

Pull `aarch64_get_imm_shift_mask()` in the header as well and make it
`__always_inline` as it is needed for `aarch64_insn_encode_immediate()`
and is already safe to inline.
This is a change of visibility, so make sure to check the input pointers
in case it is used in other places.
Current callers do not care about -EINVAL, they just check for an error,
so change the return to a boolean.

This makes `aarch64_insn_encode_immediate()` safe for inlining and
usage from patching callbacks.

As both functions are now `__always_inline`, they do not need
their `__kprobes` annotation anymore.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 103 +++++++++++++++++++++++++++++++++-
 arch/arm64/lib/insn.c         | 102 +--------------------------------
 2 files changed, 102 insertions(+), 103 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 90f271483e5b..5f5f6a125b4e 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -9,6 +9,7 @@
 #define	__ASM_INSN_H
 #include <linux/bits.h>
 #include <linux/build_bug.h>
+#include <linux/sizes.h>
 #include <linux/types.h>
 
 #include <asm/insn-def.h>
@@ -555,10 +556,108 @@ static __always_inline bool aarch64_insn_uses_literal(u32 insn)
 	       aarch64_insn_is_prfm_lit(insn);
 }
 
+static __always_inline bool aarch64_get_imm_shift_mask(
+				 enum aarch64_insn_imm_type type, u32 *maskp, int *shiftp)
+{
+	u32 mask;
+	int shift;
+
+	if (maskp == NULL || shiftp == NULL)
+		return false;
+
+	switch (type) {
+	case AARCH64_INSN_IMM_26:
+		mask = BIT(26) - 1;
+		shift = 0;
+		break;
+	case AARCH64_INSN_IMM_19:
+		mask = BIT(19) - 1;
+		shift = 5;
+		break;
+	case AARCH64_INSN_IMM_16:
+		mask = BIT(16) - 1;
+		shift = 5;
+		break;
+	case AARCH64_INSN_IMM_14:
+		mask = BIT(14) - 1;
+		shift = 5;
+		break;
+	case AARCH64_INSN_IMM_12:
+		mask = BIT(12) - 1;
+		shift = 10;
+		break;
+	case AARCH64_INSN_IMM_9:
+		mask = BIT(9) - 1;
+		shift = 12;
+		break;
+	case AARCH64_INSN_IMM_7:
+		mask = BIT(7) - 1;
+		shift = 15;
+		break;
+	case AARCH64_INSN_IMM_6:
+	case AARCH64_INSN_IMM_S:
+		mask = BIT(6) - 1;
+		shift = 10;
+		break;
+	case AARCH64_INSN_IMM_R:
+		mask = BIT(6) - 1;
+		shift = 16;
+		break;
+	case AARCH64_INSN_IMM_N:
+		mask = 1;
+		shift = 22;
+		break;
+	default:
+		return false;
+	}
+
+	*maskp = mask;
+	*shiftp = shift;
+
+	return true;
+}
+
+#define ADR_IMM_HILOSPLIT	2
+#define ADR_IMM_SIZE		SZ_2M
+#define ADR_IMM_LOMASK		((1 << ADR_IMM_HILOSPLIT) - 1)
+#define ADR_IMM_HIMASK		((ADR_IMM_SIZE >> ADR_IMM_HILOSPLIT) - 1)
+#define ADR_IMM_LOSHIFT		29
+#define ADR_IMM_HISHIFT		5
+
 enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
-u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
-				  u32 insn, u64 imm);
+
+static __always_inline u32 aarch64_insn_encode_immediate(
+				 enum aarch64_insn_imm_type type, u32 insn, u64 imm)
+{
+	u32 immlo, immhi, mask;
+	int shift;
+
+	if (insn == AARCH64_BREAK_FAULT)
+		return AARCH64_BREAK_FAULT;
+
+	switch (type) {
+	case AARCH64_INSN_IMM_ADR:
+		shift = 0;
+		immlo = (imm & ADR_IMM_LOMASK) << ADR_IMM_LOSHIFT;
+		imm >>= ADR_IMM_HILOSPLIT;
+		immhi = (imm & ADR_IMM_HIMASK) << ADR_IMM_HISHIFT;
+		imm = immlo | immhi;
+		mask = ((ADR_IMM_LOMASK << ADR_IMM_LOSHIFT) |
+			(ADR_IMM_HIMASK << ADR_IMM_HISHIFT));
+		break;
+	default:
+		if (aarch64_get_imm_shift_mask(type, &mask, &shift) == false) {
+			return AARCH64_BREAK_FAULT;
+		}
+	}
+
+	/* Update the immediate field. */
+	insn &= ~(mask << shift);
+	insn |= (imm & mask) << shift;
+
+	return insn;
+}
 static __always_inline u32 aarch64_insn_encode_register(
 				 enum aarch64_insn_register_type type,
 				 u32 insn,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 1810e1ea64a7..d77aef7f84f1 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -13,7 +13,6 @@
 #include <linux/types.h>
 
 #include <asm/debug-monitors.h>
-#include <asm/errno.h>
 #include <asm/insn.h>
 #include <asm/kprobes.h>
 
@@ -21,71 +20,6 @@
 #define AARCH64_INSN_N_BIT	BIT(22)
 #define AARCH64_INSN_LSL_12	BIT(22)
 
-static int __kprobes aarch64_get_imm_shift_mask(enum aarch64_insn_imm_type type,
-						u32 *maskp, int *shiftp)
-{
-	u32 mask;
-	int shift;
-
-	switch (type) {
-	case AARCH64_INSN_IMM_26:
-		mask = BIT(26) - 1;
-		shift = 0;
-		break;
-	case AARCH64_INSN_IMM_19:
-		mask = BIT(19) - 1;
-		shift = 5;
-		break;
-	case AARCH64_INSN_IMM_16:
-		mask = BIT(16) - 1;
-		shift = 5;
-		break;
-	case AARCH64_INSN_IMM_14:
-		mask = BIT(14) - 1;
-		shift = 5;
-		break;
-	case AARCH64_INSN_IMM_12:
-		mask = BIT(12) - 1;
-		shift = 10;
-		break;
-	case AARCH64_INSN_IMM_9:
-		mask = BIT(9) - 1;
-		shift = 12;
-		break;
-	case AARCH64_INSN_IMM_7:
-		mask = BIT(7) - 1;
-		shift = 15;
-		break;
-	case AARCH64_INSN_IMM_6:
-	case AARCH64_INSN_IMM_S:
-		mask = BIT(6) - 1;
-		shift = 10;
-		break;
-	case AARCH64_INSN_IMM_R:
-		mask = BIT(6) - 1;
-		shift = 16;
-		break;
-	case AARCH64_INSN_IMM_N:
-		mask = 1;
-		shift = 22;
-		break;
-	default:
-		return -EINVAL;
-	}
-
-	*maskp = mask;
-	*shiftp = shift;
-
-	return 0;
-}
-
-#define ADR_IMM_HILOSPLIT	2
-#define ADR_IMM_SIZE		SZ_2M
-#define ADR_IMM_LOMASK		((1 << ADR_IMM_HILOSPLIT) - 1)
-#define ADR_IMM_HIMASK		((ADR_IMM_SIZE >> ADR_IMM_HILOSPLIT) - 1)
-#define ADR_IMM_LOSHIFT		29
-#define ADR_IMM_HISHIFT		5
-
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
 {
 	u32 immlo, immhi, mask;
@@ -100,7 +34,7 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
 		mask = ADR_IMM_SIZE - 1;
 		break;
 	default:
-		if (aarch64_get_imm_shift_mask(type, &mask, &shift) < 0) {
+		if (aarch64_get_imm_shift_mask(type, &mask, &shift) == false) {
 			pr_err("%s: unknown immediate encoding %d\n", __func__,
 			       type);
 			return 0;
@@ -110,40 +44,6 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
 	return (insn >> shift) & mask;
 }
 
-u32 __kprobes aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
-				  u32 insn, u64 imm)
-{
-	u32 immlo, immhi, mask;
-	int shift;
-
-	if (insn == AARCH64_BREAK_FAULT)
-		return AARCH64_BREAK_FAULT;
-
-	switch (type) {
-	case AARCH64_INSN_IMM_ADR:
-		shift = 0;
-		immlo = (imm & ADR_IMM_LOMASK) << ADR_IMM_LOSHIFT;
-		imm >>= ADR_IMM_HILOSPLIT;
-		immhi = (imm & ADR_IMM_HIMASK) << ADR_IMM_HISHIFT;
-		imm = immlo | immhi;
-		mask = ((ADR_IMM_LOMASK << ADR_IMM_LOSHIFT) |
-			(ADR_IMM_HIMASK << ADR_IMM_HISHIFT));
-		break;
-	default:
-		if (aarch64_get_imm_shift_mask(type, &mask, &shift) < 0) {
-			pr_err("%s: unknown immediate encoding %d\n", __func__,
-			       type);
-			return AARCH64_BREAK_FAULT;
-		}
-	}
-
-	/* Update the immediate field. */
-	insn &= ~(mask << shift);
-	insn |= (imm & mask) << shift;
-
-	return insn;
-}
-
 static const u32 aarch64_insn_ldst_size[] = {
 	[AARCH64_INSN_SIZE_8] = 0,
 	[AARCH64_INSN_SIZE_16] = 1,
-- 
2.43.0

[RFC PATCH 06/16] arm64/insn: always inline aarch64_insn_gen_movewide()

[Ada Couprie Diaz]

As it is always called with an explicit movewide type, we can
check for its validity at compile time and remove the runtime error print.

The other error prints cannot be verified at compile time, but should not
occur in practice and will still lead to a fault BRK, so remove them.

This makes `aarch64_insn_gen_movewide()` safe for inlining
and usage from patching callbacks, as both
`aarch64_insn_encode_register()` and `aarch64_insn_encode_immediate()`
have been made safe in previous commits.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 58 ++++++++++++++++++++++++++++++++---
 arch/arm64/lib/insn.c         | 56 ---------------------------------
 2 files changed, 54 insertions(+), 60 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 5f5f6a125b4e..5a25e311717f 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -624,6 +624,8 @@ static __always_inline bool aarch64_get_imm_shift_mask(
 #define ADR_IMM_LOSHIFT		29
 #define ADR_IMM_HISHIFT		5
 
+#define AARCH64_INSN_SF_BIT	BIT(31)
+
 enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
 
@@ -796,10 +798,58 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
 			      int immr, int imms,
 			      enum aarch64_insn_variant variant,
 			      enum aarch64_insn_bitfield_type type);
-u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
-			      int imm, int shift,
-			      enum aarch64_insn_variant variant,
-			      enum aarch64_insn_movewide_type type);
+
+static __always_inline u32 aarch64_insn_gen_movewide(
+				 enum aarch64_insn_register dst,
+				 int imm, int shift,
+				 enum aarch64_insn_variant variant,
+				 enum aarch64_insn_movewide_type type)
+{
+	compiletime_assert(type >=  AARCH64_INSN_MOVEWIDE_ZERO &&
+		type <= AARCH64_INSN_MOVEWIDE_INVERSE, "unknown movewide encoding");
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_MOVEWIDE_ZERO:
+		insn = aarch64_insn_get_movz_value();
+		break;
+	case AARCH64_INSN_MOVEWIDE_KEEP:
+		insn = aarch64_insn_get_movk_value();
+		break;
+	case AARCH64_INSN_MOVEWIDE_INVERSE:
+		insn = aarch64_insn_get_movn_value();
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	if (imm & ~(SZ_64K - 1)) {
+		return AARCH64_BREAK_FAULT;
+	}
+
+	switch (variant) {
+	case AARCH64_INSN_VARIANT_32BIT:
+		if (shift != 0 && shift != 16) {
+			return AARCH64_BREAK_FAULT;
+		}
+		break;
+	case AARCH64_INSN_VARIANT_64BIT:
+		insn |= AARCH64_INSN_SF_BIT;
+		if (shift != 0 && shift != 16 && shift != 32 && shift != 48) {
+			return AARCH64_BREAK_FAULT;
+		}
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn |= (shift >> 4) << 21;
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
+
+	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
+}
+
 u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
 					 enum aarch64_insn_register src,
 					 enum aarch64_insn_register reg,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index d77aef7f84f1..7530d51f9b2a 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -16,7 +16,6 @@
 #include <asm/insn.h>
 #include <asm/kprobes.h>
 
-#define AARCH64_INSN_SF_BIT	BIT(31)
 #define AARCH64_INSN_N_BIT	BIT(22)
 #define AARCH64_INSN_LSL_12	BIT(22)
 
@@ -702,61 +701,6 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
 	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_S, insn, imms);
 }
 
-u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
-			      int imm, int shift,
-			      enum aarch64_insn_variant variant,
-			      enum aarch64_insn_movewide_type type)
-{
-	u32 insn;
-
-	switch (type) {
-	case AARCH64_INSN_MOVEWIDE_ZERO:
-		insn = aarch64_insn_get_movz_value();
-		break;
-	case AARCH64_INSN_MOVEWIDE_KEEP:
-		insn = aarch64_insn_get_movk_value();
-		break;
-	case AARCH64_INSN_MOVEWIDE_INVERSE:
-		insn = aarch64_insn_get_movn_value();
-		break;
-	default:
-		pr_err("%s: unknown movewide encoding %d\n", __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	if (imm & ~(SZ_64K - 1)) {
-		pr_err("%s: invalid immediate encoding %d\n", __func__, imm);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	switch (variant) {
-	case AARCH64_INSN_VARIANT_32BIT:
-		if (shift != 0 && shift != 16) {
-			pr_err("%s: invalid shift encoding %d\n", __func__,
-			       shift);
-			return AARCH64_BREAK_FAULT;
-		}
-		break;
-	case AARCH64_INSN_VARIANT_64BIT:
-		insn |= AARCH64_INSN_SF_BIT;
-		if (shift != 0 && shift != 16 && shift != 32 && shift != 48) {
-			pr_err("%s: invalid shift encoding %d\n", __func__,
-			       shift);
-			return AARCH64_BREAK_FAULT;
-		}
-		break;
-	default:
-		pr_err("%s: unknown variant encoding %d\n", __func__, variant);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	insn |= (shift >> 4) << 21;
-
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
-
-	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, imm);
-}
-
 u32 aarch64_insn_gen_add_sub_shifted_reg(enum aarch64_insn_register dst,
 					 enum aarch64_insn_register src,
 					 enum aarch64_insn_register reg,
-- 
2.43.0

[RFC PATCH 07/16] arm64/proton-pack: make alternative callbacks safe

[Ada Couprie Diaz]

Alternative callback functions are regular functions, which means they
or any function they call could get patched or instrumented
by alternatives or other parts of the kernel.
Given that applying alternatives does not guarantee a consistent state
while patching, only once done, and handles cache maintenance manually,
it could lead to nasty corruptions and execution of bogus code.

Make the Spectre mitigations alternative callbacks safe by marking them
`noinstr` when they are not.
This is possible thanks to previous commits making `aarch64_insn_...`
functions used in the callbacks safe to inline.

`spectre_bhb_patch_clearbhb()` is already marked as `__init`,
which has its own text section conflicting with the `noinstr` one.
Instead, use `__no_instr_section(".noinstr.text")` to add
all the function attributes added by `noinstr`, without the section
conflict.
This can be an issue, as kprobes seems to only block the text sections,
not based on function attributes.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
This is missing `spectre_bhb_patch_wa3()` and
`spectre_v4_patch_fw_mitigation_enable()` callbacks, which would need
some more work :
- `spectre_bhb_patch_wa3()` uses `WARN` which is instrumented, and
  I am not sure if it is safe to remove. It feels like something else
  should be done there ?
- `spectre_v4_patch_fw_mitigation_enable()` calls into
  `spectre_v4_mitigations_off()` which calls `pr_info_once()` to notice
  the disabling of the mitigations on the command line, which is
  instrumentable but feels important to keep. I am not sure if there
  would be a better place to generate that message ?
  Interestingly, this was brought up recently[0].
  It also calls `cpu_mitigations_off()` which checks a static variable
  against a static enum, in a common code C file, and is instrumentable.
  This one feels like it could be `__always_inline`'d, but given it is
  common code and the static nature of operands in the check, maybe
  marking it `noinstr` would be acceptable ?

[0]: https://lore.kernel.org/all/aNF0gb1iZndz0-be@J2N7QTR9R3/
---
 arch/arm64/kernel/proton-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kernel/proton-pack.c b/arch/arm64/kernel/proton-pack.c
index edf1783ffc81..4ba8d24bf7ef 100644
--- a/arch/arm64/kernel/proton-pack.c
+++ b/arch/arm64/kernel/proton-pack.c
@@ -1174,6 +1174,7 @@ void noinstr spectre_bhb_patch_wa3(struct alt_instr *alt,
 }
 
 /* Patched to NOP when not supported */
+__noinstr_section(".init.text")
 void __init spectre_bhb_patch_clearbhb(struct alt_instr *alt,
 				   __le32 *origptr, __le32 *updptr, int nr_inst)
 {
-- 
2.43.0

[RFC PATCH 08/16] arm64/insn: always inline aarch64_insn_gen_logical_immediate()

[Ada Couprie Diaz]

As it is always called with an explicit logic instruction type, we can
check for its validity at compile time and remove the runtime error print.

Pull its helper functions, `aarch64_encode_immediate()` and
`range_of_ones()`, into the header and make them `__always_inline`
as well.
This is safe as they only call other `__always_inline` functions.

This makes `aarch64_insn_gen_logical_immediate()` safe for inlining
and usage from patching callbacks.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 149 ++++++++++++++++++++++++++++++++--
 arch/arm64/lib/insn.c         | 136 -------------------------------
 2 files changed, 144 insertions(+), 141 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 5a25e311717f..a94ecc9140f1 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -16,6 +16,8 @@
 
 #ifndef __ASSEMBLY__
 
+#include <linux/bitops.h>
+
 enum aarch64_insn_hint_cr_op {
 	AARCH64_INSN_HINT_NOP	= 0x0 << 5,
 	AARCH64_INSN_HINT_YIELD	= 0x1 << 5,
@@ -880,11 +882,148 @@ u32 aarch64_insn_gen_logical_shifted_reg(enum aarch64_insn_register dst,
 u32 aarch64_insn_gen_move_reg(enum aarch64_insn_register dst,
 			      enum aarch64_insn_register src,
 			      enum aarch64_insn_variant variant);
-u32 aarch64_insn_gen_logical_immediate(enum aarch64_insn_logic_type type,
-				       enum aarch64_insn_variant variant,
-				       enum aarch64_insn_register Rn,
-				       enum aarch64_insn_register Rd,
-				       u64 imm);
+
+static __always_inline bool range_of_ones(u64 val)
+{
+	/* Doesn't handle full ones or full zeroes */
+	u64 sval = val >> __ffs64(val);
+
+	/* One of Sean Eron Anderson's bithack tricks */
+	return ((sval + 1) & (sval)) == 0;
+}
+
+static __always_inline u32 aarch64_encode_immediate(u64 imm,
+				 enum aarch64_insn_variant variant,
+				 u32 insn)
+{
+	unsigned int immr, imms, n, ones, ror, esz, tmp;
+	u64 mask;
+
+	switch (variant) {
+	case AARCH64_INSN_VARIANT_32BIT:
+		esz = 32;
+		break;
+	case AARCH64_INSN_VARIANT_64BIT:
+		insn |= AARCH64_INSN_SF_BIT;
+		esz = 64;
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	mask = GENMASK(esz - 1, 0);
+
+	/* Can't encode full zeroes, full ones, or value wider than the mask */
+	if (!imm || imm == mask || imm & ~mask)
+		return AARCH64_BREAK_FAULT;
+
+	/*
+	 * Inverse of Replicate(). Try to spot a repeating pattern
+	 * with a pow2 stride.
+	 */
+	for (tmp = esz / 2; tmp >= 2; tmp /= 2) {
+		u64 emask = BIT(tmp) - 1;
+
+		if ((imm & emask) != ((imm >> tmp) & emask))
+			break;
+
+		esz = tmp;
+		mask = emask;
+	}
+
+	/* N is only set if we're encoding a 64bit value */
+	n = esz == 64;
+
+	/* Trim imm to the element size */
+	imm &= mask;
+
+	/* That's how many ones we need to encode */
+	ones = hweight64(imm);
+
+	/*
+	 * imms is set to (ones - 1), prefixed with a string of ones
+	 * and a zero if they fit. Cap it to 6 bits.
+	 */
+	imms  = ones - 1;
+	imms |= 0xf << ffs(esz);
+	imms &= BIT(6) - 1;
+
+	/* Compute the rotation */
+	if (range_of_ones(imm)) {
+		/*
+		 * Pattern: 0..01..10..0
+		 *
+		 * Compute how many rotate we need to align it right
+		 */
+		ror = __ffs64(imm);
+	} else {
+		/*
+		 * Pattern: 0..01..10..01..1
+		 *
+		 * Fill the unused top bits with ones, and check if
+		 * the result is a valid immediate (all ones with a
+		 * contiguous ranges of zeroes).
+		 */
+		imm |= ~mask;
+		if (!range_of_ones(~imm))
+			return AARCH64_BREAK_FAULT;
+
+		/*
+		 * Compute the rotation to get a continuous set of
+		 * ones, with the first bit set at position 0
+		 */
+		ror = fls64(~imm);
+	}
+
+	/*
+	 * immr is the number of bits we need to rotate back to the
+	 * original set of ones. Note that this is relative to the
+	 * element size...
+	 */
+	immr = (esz - ror) % esz;
+
+	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_N, insn, n);
+	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_R, insn, immr);
+	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_S, insn, imms);
+}
+
+static __always_inline u32 aarch64_insn_gen_logical_immediate(
+					 enum aarch64_insn_logic_type type,
+					 enum aarch64_insn_variant variant,
+					 enum aarch64_insn_register Rn,
+					 enum aarch64_insn_register Rd,
+					 u64 imm)
+{
+	compiletime_assert(type == AARCH64_INSN_LOGIC_AND ||
+		type == AARCH64_INSN_LOGIC_ORR ||
+		type == AARCH64_INSN_LOGIC_EOR ||
+		type == AARCH64_INSN_LOGIC_AND_SETFLAGS,
+		"unknown logical encoding");
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_LOGIC_AND:
+		insn = aarch64_insn_get_and_imm_value();
+		break;
+	case AARCH64_INSN_LOGIC_ORR:
+		insn = aarch64_insn_get_orr_imm_value();
+		break;
+	case AARCH64_INSN_LOGIC_EOR:
+		insn = aarch64_insn_get_eor_imm_value();
+		break;
+	case AARCH64_INSN_LOGIC_AND_SETFLAGS:
+		insn = aarch64_insn_get_ands_imm_value();
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, Rd);
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, Rn);
+	return aarch64_encode_immediate(imm, variant, insn);
+}
+
+
 u32 aarch64_insn_gen_extr(enum aarch64_insn_variant variant,
 			  enum aarch64_insn_register Rm,
 			  enum aarch64_insn_register Rn,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 7530d51f9b2a..15634094de05 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -1106,142 +1106,6 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
 	return insn & CRM_MASK;
 }
 
-static bool range_of_ones(u64 val)
-{
-	/* Doesn't handle full ones or full zeroes */
-	u64 sval = val >> __ffs64(val);
-
-	/* One of Sean Eron Anderson's bithack tricks */
-	return ((sval + 1) & (sval)) == 0;
-}
-
-static u32 aarch64_encode_immediate(u64 imm,
-				    enum aarch64_insn_variant variant,
-				    u32 insn)
-{
-	unsigned int immr, imms, n, ones, ror, esz, tmp;
-	u64 mask;
-
-	switch (variant) {
-	case AARCH64_INSN_VARIANT_32BIT:
-		esz = 32;
-		break;
-	case AARCH64_INSN_VARIANT_64BIT:
-		insn |= AARCH64_INSN_SF_BIT;
-		esz = 64;
-		break;
-	default:
-		pr_err("%s: unknown variant encoding %d\n", __func__, variant);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	mask = GENMASK(esz - 1, 0);
-
-	/* Can't encode full zeroes, full ones, or value wider than the mask */
-	if (!imm || imm == mask || imm & ~mask)
-		return AARCH64_BREAK_FAULT;
-
-	/*
-	 * Inverse of Replicate(). Try to spot a repeating pattern
-	 * with a pow2 stride.
-	 */
-	for (tmp = esz / 2; tmp >= 2; tmp /= 2) {
-		u64 emask = BIT(tmp) - 1;
-
-		if ((imm & emask) != ((imm >> tmp) & emask))
-			break;
-
-		esz = tmp;
-		mask = emask;
-	}
-
-	/* N is only set if we're encoding a 64bit value */
-	n = esz == 64;
-
-	/* Trim imm to the element size */
-	imm &= mask;
-
-	/* That's how many ones we need to encode */
-	ones = hweight64(imm);
-
-	/*
-	 * imms is set to (ones - 1), prefixed with a string of ones
-	 * and a zero if they fit. Cap it to 6 bits.
-	 */
-	imms  = ones - 1;
-	imms |= 0xf << ffs(esz);
-	imms &= BIT(6) - 1;
-
-	/* Compute the rotation */
-	if (range_of_ones(imm)) {
-		/*
-		 * Pattern: 0..01..10..0
-		 *
-		 * Compute how many rotate we need to align it right
-		 */
-		ror = __ffs64(imm);
-	} else {
-		/*
-		 * Pattern: 0..01..10..01..1
-		 *
-		 * Fill the unused top bits with ones, and check if
-		 * the result is a valid immediate (all ones with a
-		 * contiguous ranges of zeroes).
-		 */
-		imm |= ~mask;
-		if (!range_of_ones(~imm))
-			return AARCH64_BREAK_FAULT;
-
-		/*
-		 * Compute the rotation to get a continuous set of
-		 * ones, with the first bit set at position 0
-		 */
-		ror = fls64(~imm);
-	}
-
-	/*
-	 * immr is the number of bits we need to rotate back to the
-	 * original set of ones. Note that this is relative to the
-	 * element size...
-	 */
-	immr = (esz - ror) % esz;
-
-	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_N, insn, n);
-	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_R, insn, immr);
-	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_S, insn, imms);
-}
-
-u32 aarch64_insn_gen_logical_immediate(enum aarch64_insn_logic_type type,
-				       enum aarch64_insn_variant variant,
-				       enum aarch64_insn_register Rn,
-				       enum aarch64_insn_register Rd,
-				       u64 imm)
-{
-	u32 insn;
-
-	switch (type) {
-	case AARCH64_INSN_LOGIC_AND:
-		insn = aarch64_insn_get_and_imm_value();
-		break;
-	case AARCH64_INSN_LOGIC_ORR:
-		insn = aarch64_insn_get_orr_imm_value();
-		break;
-	case AARCH64_INSN_LOGIC_EOR:
-		insn = aarch64_insn_get_eor_imm_value();
-		break;
-	case AARCH64_INSN_LOGIC_AND_SETFLAGS:
-		insn = aarch64_insn_get_ands_imm_value();
-		break;
-	default:
-		pr_err("%s: unknown logical encoding %d\n", __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, Rd);
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, Rn);
-	return aarch64_encode_immediate(imm, variant, insn);
-}
-
 u32 aarch64_insn_gen_extr(enum aarch64_insn_variant variant,
 			  enum aarch64_insn_register Rm,
 			  enum aarch64_insn_register Rn,
-- 
2.43.0

[RFC PATCH 09/16] arm64/insn: always inline aarch64_insn_gen_add_sub_imm()

[Ada Couprie Diaz]

As it is always called with an explicit instruction adsb type and variant,
we can check for their validity at compile time and remove
the runtime error print.

This is not the case for the immediate error print, as it checks
dynamically. However, it should not occur in practice and will still
generate a fault BRK, so remove it.

This makes `aarch64_insn_gen_add_sub_imm()` safe for inlining
and usage from patching callbacks, as both
`aarch64_insn_encode_register()` and `aarch64_insn_encode_immediate()`
have been made safe in previous commits.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 68 +++++++++++++++++++++++++++++++++--
 arch/arm64/lib/insn.c         | 62 --------------------------------
 2 files changed, 66 insertions(+), 64 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index a94ecc9140f1..a7caafd6f02b 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -627,6 +627,8 @@ static __always_inline bool aarch64_get_imm_shift_mask(
 #define ADR_IMM_HISHIFT		5
 
 #define AARCH64_INSN_SF_BIT	BIT(31)
+#define AARCH64_INSN_LSL_12	BIT(22)
+
 
 enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
@@ -788,10 +790,72 @@ u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
 				   enum aarch64_insn_register state,
 				   enum aarch64_insn_size_type size,
 				   enum aarch64_insn_ldst_type type);
-u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
+
+static __always_inline u32 aarch64_insn_gen_add_sub_imm(
+				 enum aarch64_insn_register dst,
 				 enum aarch64_insn_register src,
 				 int imm, enum aarch64_insn_variant variant,
-				 enum aarch64_insn_adsb_type type);
+				 enum aarch64_insn_adsb_type type)
+{
+	compiletime_assert(type >= AARCH64_INSN_ADSB_ADD &&
+		type <= AARCH64_INSN_ADSB_SUB_SETFLAGS,
+		"unknown add/sub encoding");
+	compiletime_assert(variant == AARCH64_INSN_VARIANT_32BIT ||
+		variant == AARCH64_INSN_VARIANT_64BIT,
+		"unknown variant encoding");
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_ADSB_ADD:
+		insn = aarch64_insn_get_add_imm_value();
+		break;
+	case AARCH64_INSN_ADSB_SUB:
+		insn = aarch64_insn_get_sub_imm_value();
+		break;
+	case AARCH64_INSN_ADSB_ADD_SETFLAGS:
+		insn = aarch64_insn_get_adds_imm_value();
+		break;
+	case AARCH64_INSN_ADSB_SUB_SETFLAGS:
+		insn = aarch64_insn_get_subs_imm_value();
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	switch (variant) {
+	case AARCH64_INSN_VARIANT_32BIT:
+		break;
+	case AARCH64_INSN_VARIANT_64BIT:
+		insn |= AARCH64_INSN_SF_BIT;
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	/* We can't encode more than a 24bit value (12bit + 12bit shift) */
+	if (imm & ~(BIT(24) - 1))
+		goto out;
+
+	/* If we have something in the top 12 bits... */
+	if (imm & ~(SZ_4K - 1)) {
+		/* ... and in the low 12 bits -> error */
+		if (imm & (SZ_4K - 1))
+			goto out;
+
+		imm >>= 12;
+		insn |= AARCH64_INSN_LSL_12;
+	}
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, src);
+
+	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_12, insn, imm);
+
+out:
+	return AARCH64_BREAK_FAULT;
+}
+
 u32 aarch64_insn_gen_adr(unsigned long pc, unsigned long addr,
 			 enum aarch64_insn_register reg,
 			 enum aarch64_insn_adr_type type);
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 15634094de05..34b6f1c692b4 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -17,7 +17,6 @@
 #include <asm/kprobes.h>
 
 #define AARCH64_INSN_N_BIT	BIT(22)
-#define AARCH64_INSN_LSL_12	BIT(22)
 
 u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
 {
@@ -585,67 +584,6 @@ u32 aarch64_insn_gen_cas(enum aarch64_insn_register result,
 }
 #endif
 
-u32 aarch64_insn_gen_add_sub_imm(enum aarch64_insn_register dst,
-				 enum aarch64_insn_register src,
-				 int imm, enum aarch64_insn_variant variant,
-				 enum aarch64_insn_adsb_type type)
-{
-	u32 insn;
-
-	switch (type) {
-	case AARCH64_INSN_ADSB_ADD:
-		insn = aarch64_insn_get_add_imm_value();
-		break;
-	case AARCH64_INSN_ADSB_SUB:
-		insn = aarch64_insn_get_sub_imm_value();
-		break;
-	case AARCH64_INSN_ADSB_ADD_SETFLAGS:
-		insn = aarch64_insn_get_adds_imm_value();
-		break;
-	case AARCH64_INSN_ADSB_SUB_SETFLAGS:
-		insn = aarch64_insn_get_subs_imm_value();
-		break;
-	default:
-		pr_err("%s: unknown add/sub encoding %d\n", __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	switch (variant) {
-	case AARCH64_INSN_VARIANT_32BIT:
-		break;
-	case AARCH64_INSN_VARIANT_64BIT:
-		insn |= AARCH64_INSN_SF_BIT;
-		break;
-	default:
-		pr_err("%s: unknown variant encoding %d\n", __func__, variant);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	/* We can't encode more than a 24bit value (12bit + 12bit shift) */
-	if (imm & ~(BIT(24) - 1))
-		goto out;
-
-	/* If we have something in the top 12 bits... */
-	if (imm & ~(SZ_4K - 1)) {
-		/* ... and in the low 12 bits -> error */
-		if (imm & (SZ_4K - 1))
-			goto out;
-
-		imm >>= 12;
-		insn |= AARCH64_INSN_LSL_12;
-	}
-
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, dst);
-
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, src);
-
-	return aarch64_insn_encode_immediate(AARCH64_INSN_IMM_12, insn, imm);
-
-out:
-	pr_err("%s: invalid immediate encoding %d\n", __func__, imm);
-	return AARCH64_BREAK_FAULT;
-}
-
 u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
 			      enum aarch64_insn_register src,
 			      int immr, int imms,
-- 
2.43.0

[RFC PATCH 10/16] arm64/insn: always inline aarch64_insn_gen_branch_reg()

[Ada Couprie Diaz]

As it is always called with an explicit instruction branch type, we can
check for its validity at compile time and remove the runtime error print.

This makes `aarch64_insn_gen_branch_reg()` safe for inlining
and usage from patching callbacks, as `aarch64_insn_encode_register()`
has been made safe in a previous commit.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 28 ++++++++++++++++++++++++++--
 arch/arm64/lib/insn.c         | 23 -----------------------
 2 files changed, 26 insertions(+), 25 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index a7caafd6f02b..6e6a53d4d750 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -760,8 +760,32 @@ static __always_inline bool aarch64_insn_is_nop(u32 insn)
 	return insn == aarch64_insn_gen_nop();
 }
 
-u32 aarch64_insn_gen_branch_reg(enum aarch64_insn_register reg,
-				enum aarch64_insn_branch_type type);
+static __always_inline u32 aarch64_insn_gen_branch_reg(
+			 enum aarch64_insn_register reg,
+			 enum aarch64_insn_branch_type type)
+{
+	compiletime_assert(type >= AARCH64_INSN_BRANCH_NOLINK &&
+		type <= AARCH64_INSN_BRANCH_RETURN,
+		"unknown branch encoding");
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_BRANCH_NOLINK:
+		insn = aarch64_insn_get_br_value();
+		break;
+	case AARCH64_INSN_BRANCH_LINK:
+		insn = aarch64_insn_get_blr_value();
+		break;
+	case AARCH64_INSN_BRANCH_RETURN:
+		insn = aarch64_insn_get_ret_value();
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, reg);
+}
+
 u32 aarch64_insn_gen_load_store_reg(enum aarch64_insn_register reg,
 				    enum aarch64_insn_register base,
 				    enum aarch64_insn_register offset,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 34b6f1c692b4..8d38bf4bf203 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -178,29 +178,6 @@ u32 aarch64_insn_gen_cond_branch_imm(unsigned long pc, unsigned long addr,
 					     offset >> 2);
 }
 
-u32 aarch64_insn_gen_branch_reg(enum aarch64_insn_register reg,
-				enum aarch64_insn_branch_type type)
-{
-	u32 insn;
-
-	switch (type) {
-	case AARCH64_INSN_BRANCH_NOLINK:
-		insn = aarch64_insn_get_br_value();
-		break;
-	case AARCH64_INSN_BRANCH_LINK:
-		insn = aarch64_insn_get_blr_value();
-		break;
-	case AARCH64_INSN_BRANCH_RETURN:
-		insn = aarch64_insn_get_ret_value();
-		break;
-	default:
-		pr_err("%s: unknown branch encoding %d\n", __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, reg);
-}
-
 u32 aarch64_insn_gen_load_store_reg(enum aarch64_insn_register reg,
 				    enum aarch64_insn_register base,
 				    enum aarch64_insn_register offset,
-- 
2.43.0

[RFC PATCH 11/16] arm64/insn: always inline aarch64_insn_gen_extr()

[Ada Couprie Diaz]

As it is always called with an explicit variant, we can check for
its validity at compile time and remove the runtime error print.

This makes `aarch64_insn_gen_extr()` safe for inlining
and usage from patching callbacks, as both
`aarch64_insn_encode_immediate()` and `aarch64_insn_encode_register()`
have been made safe in previous commits.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 39 ++++++++++++++++++++++++++++++-----
 arch/arm64/lib/insn.c         | 32 ----------------------------
 2 files changed, 34 insertions(+), 37 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 6e6a53d4d750..4ba4d5c50137 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -1111,12 +1111,41 @@ static __always_inline u32 aarch64_insn_gen_logical_immediate(
 	return aarch64_encode_immediate(imm, variant, insn);
 }
 
+static __always_inline u32 aarch64_insn_gen_extr(
+			 enum aarch64_insn_variant variant,
+			 enum aarch64_insn_register Rm,
+			 enum aarch64_insn_register Rn,
+			 enum aarch64_insn_register Rd,
+			 u8 lsb)
+{
+	compiletime_assert(variant == AARCH64_INSN_VARIANT_32BIT ||
+		variant == AARCH64_INSN_VARIANT_64BIT,
+		"unknown variant encoding");
+	u32 insn;
+
+	insn = aarch64_insn_get_extr_value();
+
+	switch (variant) {
+	case AARCH64_INSN_VARIANT_32BIT:
+		if (lsb > 31)
+			return AARCH64_BREAK_FAULT;
+		break;
+	case AARCH64_INSN_VARIANT_64BIT:
+		if (lsb > 63)
+			return AARCH64_BREAK_FAULT;
+		insn |= AARCH64_INSN_SF_BIT;
+		insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_N, insn, 1);
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_S, insn, lsb);
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, Rd);
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, Rn);
+	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RM, insn, Rm);
+}
 
-u32 aarch64_insn_gen_extr(enum aarch64_insn_variant variant,
-			  enum aarch64_insn_register Rm,
-			  enum aarch64_insn_register Rn,
-			  enum aarch64_insn_register Rd,
-			  u8 lsb);
 #ifdef CONFIG_ARM64_LSE_ATOMICS
 u32 aarch64_insn_gen_atomic_ld_op(enum aarch64_insn_register result,
 				  enum aarch64_insn_register address,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 8d38bf4bf203..71df4d72ac81 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -1021,38 +1021,6 @@ u32 aarch32_insn_mcr_extract_crm(u32 insn)
 	return insn & CRM_MASK;
 }
 
-u32 aarch64_insn_gen_extr(enum aarch64_insn_variant variant,
-			  enum aarch64_insn_register Rm,
-			  enum aarch64_insn_register Rn,
-			  enum aarch64_insn_register Rd,
-			  u8 lsb)
-{
-	u32 insn;
-
-	insn = aarch64_insn_get_extr_value();
-
-	switch (variant) {
-	case AARCH64_INSN_VARIANT_32BIT:
-		if (lsb > 31)
-			return AARCH64_BREAK_FAULT;
-		break;
-	case AARCH64_INSN_VARIANT_64BIT:
-		if (lsb > 63)
-			return AARCH64_BREAK_FAULT;
-		insn |= AARCH64_INSN_SF_BIT;
-		insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_N, insn, 1);
-		break;
-	default:
-		pr_err("%s: unknown variant encoding %d\n", __func__, variant);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_S, insn, lsb);
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RD, insn, Rd);
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn, Rn);
-	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RM, insn, Rm);
-}
-
 static u32 __get_barrier_crm_val(enum aarch64_insn_mb_type type)
 {
 	switch (type) {
-- 
2.43.0

[RFC PATCH 12/16] kvm/arm64: make alternative callbacks safe

[Ada Couprie Diaz]

Alternative callback functions are regular functions, which means they
or any function they call could get patched or instrumented
by alternatives or other parts of the kernel.
Given that applying alternatives does not guarantee a consistent state
while patching, only once done, and handles cache maintenance manually,
it could lead to nasty corruptions and execution of bogus code.

Make the KVM alternative callbacks safe by marking them `noinstr` and
`__always_inline`'ing their helpers.
This is possible thanks to previous commits making `aarch64_insn_...`
functions used in the callbacks safe to inline.

`kvm_update_va_mask()` is already marked as `__init`, which has its own
text section conflicting with the `noinstr` one.
Instead, use `__no_instr_section(".noinstr.text")` to add
all the function attributes added by `noinstr`, without the section
conflict.
This can be an issue, as kprobes seems to only block the text sections,
not based on function attributes.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
This is missing `kvm_patch_vector_branch()`, which could receive the same
treatment, but the `WARN_ON_ONCE` in the early-exit check would make it
call into instrumentable code.
I do not currently know if this `WARN` can safely be removed or if it
has some importance.
---
 arch/arm64/kvm/va_layout.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
index 91b22a014610..3ebb7e0074f6 100644
--- a/arch/arm64/kvm/va_layout.c
+++ b/arch/arm64/kvm/va_layout.c
@@ -109,7 +109,7 @@ __init void kvm_apply_hyp_relocations(void)
 	}
 }
 
-static u32 compute_instruction(int n, u32 rd, u32 rn)
+static __always_inline u32 compute_instruction(int n, u32 rd, u32 rn)
 {
 	u32 insn = AARCH64_BREAK_FAULT;
 
@@ -151,6 +151,7 @@ static u32 compute_instruction(int n, u32 rd, u32 rn)
 	return insn;
 }
 
+__noinstr_section(".init.text")
 void __init kvm_update_va_mask(struct alt_instr *alt,
 			       __le32 *origptr, __le32 *updptr, int nr_inst)
 {
@@ -241,7 +242,8 @@ void kvm_patch_vector_branch(struct alt_instr *alt,
 	*updptr++ = cpu_to_le32(insn);
 }
 
-static void generate_mov_q(u64 val, __le32 *origptr, __le32 *updptr, int nr_inst)
+static __always_inline void generate_mov_q(u64 val, __le32 *origptr,
+				 __le32 *updptr, int nr_inst)
 {
 	u32 insn, oinsn, rd;
 
@@ -284,15 +286,15 @@ static void generate_mov_q(u64 val, __le32 *origptr, __le32 *updptr, int nr_inst
 	*updptr++ = cpu_to_le32(insn);
 }
 
-void kvm_get_kimage_voffset(struct alt_instr *alt,
+noinstr void kvm_get_kimage_voffset(struct alt_instr *alt,
 			    __le32 *origptr, __le32 *updptr, int nr_inst)
 {
 	generate_mov_q(kimage_voffset, origptr, updptr, nr_inst);
 }
 
-void kvm_compute_final_ctr_el0(struct alt_instr *alt,
+noinstr void kvm_compute_final_ctr_el0(struct alt_instr *alt,
 			       __le32 *origptr, __le32 *updptr, int nr_inst)
 {
-	generate_mov_q(read_sanitised_ftr_reg(SYS_CTR_EL0),
+	generate_mov_q(arm64_ftr_reg_ctrel0.sys_val,
 		       origptr, updptr, nr_inst);
 }
-- 
2.43.0

[RFC PATCH 13/16] arm64/insn: introduce missing is_store/is_load helpers

[Ada Couprie Diaz]

The current helpers only cover single and pair load/stores.
Introduce new helpers to cover exclusive, load acquire, store release
and the LSE atomics, as they both load and store.

To gather all of them in one call : introduce `aarch64_insn_is_load()`,
`aarch64_insn_is_store()`, and `aarch64_insn_is_ldst()` helpers which
check if the instruction is a load, store or either.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
Note: I made the LSE atomics part of the is_{load,store} helpers
as they are used as such by `aarch64_insn_encode_ldst_size()`,
but it could make sense to not have them in the helpers and just
call them together where neeeded.
---
 arch/arm64/include/asm/insn.h | 53 +++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 4ba4d5c50137..44435eede1f3 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -520,6 +520,23 @@ static __always_inline bool aarch64_insn_is_barrier(u32 insn)
 	       aarch64_insn_is_pssbb(insn);
 }
 
+#ifdef CONFIG_ARM64_LSE_ATOMICS
+static __always_inline bool aarch64_insn_is_lse_atomic(u32 insn)
+{
+	return aarch64_insn_is_ldadd(insn) ||
+	       aarch64_insn_is_ldclr(insn) ||
+	       aarch64_insn_is_ldeor(insn) ||
+		   aarch64_insn_is_ldset(insn) ||
+		   aarch64_insn_is_swp(insn) ||
+		   aarch64_insn_is_cas(insn);
+}
+#else /* CONFIG_ARM64_LSE_ATOMICS */
+static __always_inline bool aarch64_insn_is_lse_atomic(u32 insn)
+{
+	return false;
+}
+#endif /* CONFIG_ARM64_LSE_ATOMICS */
+
 static __always_inline bool aarch64_insn_is_store_single(u32 insn)
 {
 	return aarch64_insn_is_store_imm(insn) ||
@@ -534,6 +551,21 @@ static __always_inline bool aarch64_insn_is_store_pair(u32 insn)
 	       aarch64_insn_is_stp_post(insn);
 }
 
+static __always_inline bool aarch64_insn_is_store_ex_or_rel(u32 insn)
+{
+	return aarch64_insn_is_store_ex(insn) ||
+	       aarch64_insn_is_store_ex(insn & (~BIT(15))) ||
+		   aarch64_insn_is_store_rel(insn);
+}
+
+static __always_inline bool aarch64_insn_is_store(u32 insn)
+{
+	return aarch64_insn_is_store_single(insn) ||
+	       aarch64_insn_is_store_pair(insn) ||
+		   aarch64_insn_is_store_ex_or_rel(insn) ||
+		   aarch64_insn_is_lse_atomic(insn);
+}
+
 static __always_inline bool aarch64_insn_is_load_single(u32 insn)
 {
 	return aarch64_insn_is_load_imm(insn) ||
@@ -548,6 +580,27 @@ static __always_inline bool aarch64_insn_is_load_pair(u32 insn)
 	       aarch64_insn_is_ldp_post(insn);
 }
 
+static __always_inline bool aarch64_insn_is_load_ex_or_acq(u32 insn)
+{
+	return aarch64_insn_is_load_ex(insn) ||
+	       aarch64_insn_is_load_ex(insn & (~BIT(15))) ||
+		   aarch64_insn_is_load_acq(insn);
+}
+
+static __always_inline bool aarch64_insn_is_load(u32 insn)
+{
+	return aarch64_insn_is_load_single(insn) ||
+	       aarch64_insn_is_load_pair(insn) ||
+		   aarch64_insn_is_load_ex_or_acq(insn) ||
+		   aarch64_insn_is_lse_atomic(insn);
+}
+
+static __always_inline bool aarch64_insn_is_ldst(u32 insn)
+{
+	return aarch64_insn_is_load(insn) ||
+		   aarch64_insn_is_store(insn);
+}
+
 static __always_inline bool aarch64_insn_uses_literal(u32 insn)
 {
 	/* ldr/ldrsw (literal), prfm */
-- 
2.43.0

[RFC PATCH 14/16] arm64/insn: always inline aarch64_insn_encode_ldst_size()

[Ada Couprie Diaz]

The type and instruction checks cannot be made at compile time,
as they are dynamically created. However, we can remove the error print
as it should never appear in normal operation and will still lead to
a fault BRK.

This makes `aarch64_insn_encode_ldst_size()` safe for inlining
and usage from patching callbacks.

This is a change of visiblity, as previously the function was private to
lib/insn.c.
However, in order to inline more `aarch64_insn_` functions and make
patching callbacks safe, it needs to be accessible by those functions.
As it is more accessible than before, add a check so that only loads
or stores can be affected by the size encoding.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 24 ++++++++++++++++++++++++
 arch/arm64/lib/insn.c         | 19 +------------------
 2 files changed, 25 insertions(+), 18 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 44435eede1f3..46d4d452e2e2 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -717,6 +717,30 @@ static __always_inline u32 aarch64_insn_encode_immediate(
 
 	return insn;
 }
+
+extern const u32 aarch64_insn_ldst_size[];
+static __always_inline u32 aarch64_insn_encode_ldst_size(
+					 enum aarch64_insn_size_type type,
+					 u32 insn)
+{
+	u32 size;
+
+	if (type < AARCH64_INSN_SIZE_8 || type > AARCH64_INSN_SIZE_64) {
+		return AARCH64_BREAK_FAULT;
+	}
+
+	/* Don't corrput the top bits of other instructions which aren't a size. */
+	if (!aarch64_insn_is_ldst(insn)) {
+		return AARCH64_BREAK_FAULT;
+	}
+
+	size = aarch64_insn_ldst_size[type];
+	insn &= ~GENMASK(31, 30);
+	insn |= size << 30;
+
+	return insn;
+}
+
 static __always_inline u32 aarch64_insn_encode_register(
 				 enum aarch64_insn_register_type type,
 				 u32 insn,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 71df4d72ac81..63564d236235 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -42,30 +42,13 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
 	return (insn >> shift) & mask;
 }
 
-static const u32 aarch64_insn_ldst_size[] = {
+const u32 aarch64_insn_ldst_size[] = {
 	[AARCH64_INSN_SIZE_8] = 0,
 	[AARCH64_INSN_SIZE_16] = 1,
 	[AARCH64_INSN_SIZE_32] = 2,
 	[AARCH64_INSN_SIZE_64] = 3,
 };
 
-static u32 aarch64_insn_encode_ldst_size(enum aarch64_insn_size_type type,
-					 u32 insn)
-{
-	u32 size;
-
-	if (type < AARCH64_INSN_SIZE_8 || type > AARCH64_INSN_SIZE_64) {
-		pr_err("%s: unknown size encoding %d\n", __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	size = aarch64_insn_ldst_size[type];
-	insn &= ~GENMASK(31, 30);
-	insn |= size << 30;
-
-	return insn;
-}
-
 static inline long label_imm_common(unsigned long pc, unsigned long addr,
 				     long range)
 {
-- 
2.43.0

[RFC PATCH 15/16] arm64/insn: always inline aarch64_insn_gen_load_acq_store_rel()

[Ada Couprie Diaz]

As it is always called with an explicit instruction type, we can
check for its validity at compile time and remove the runtime error print.

This makes `aarch64_insn_gen_load_acq_store_rel()` safe for inlining
and usage from patching callbacks, as both
`aarch64_insn_encode_ldst_size()` and `aarch64_insn_encode_register()`
have been made safe in previous commits.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/insn.h | 36 +++++++++++++++++++++++++++++++----
 arch/arm64/lib/insn.c         | 29 ----------------------------
 2 files changed, 32 insertions(+), 33 deletions(-)

diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
index 46d4d452e2e2..b7abc9b3e74c 100644
--- a/arch/arm64/include/asm/insn.h
+++ b/arch/arm64/include/asm/insn.h
@@ -882,10 +882,38 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
 				     int offset,
 				     enum aarch64_insn_variant variant,
 				     enum aarch64_insn_ldst_type type);
-u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
-					enum aarch64_insn_register base,
-					enum aarch64_insn_size_type size,
-					enum aarch64_insn_ldst_type type);
+
+static __always_inline u32 aarch64_insn_gen_load_acq_store_rel(
+					 enum aarch64_insn_register reg,
+					 enum aarch64_insn_register base,
+					 enum aarch64_insn_size_type size,
+					 enum aarch64_insn_ldst_type type)
+{
+	compiletime_assert(type == AARCH64_INSN_LDST_LOAD_ACQ ||
+					type == AARCH64_INSN_LDST_STORE_REL,
+		"unknown load-acquire/store-release encoding");
+	u32 insn;
+
+	switch (type) {
+	case AARCH64_INSN_LDST_LOAD_ACQ:
+		insn = aarch64_insn_get_load_acq_value();
+		break;
+	case AARCH64_INSN_LDST_STORE_REL:
+		insn = aarch64_insn_get_store_rel_value();
+		break;
+	default:
+		return AARCH64_BREAK_FAULT;
+	}
+
+	insn = aarch64_insn_encode_ldst_size(size, insn);
+
+	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
+					    reg);
+
+	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn,
+					    base);
+}
+
 u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
 				   enum aarch64_insn_register base,
 				   enum aarch64_insn_register state,
diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
index 63564d236235..6ee298f96d47 100644
--- a/arch/arm64/lib/insn.c
+++ b/arch/arm64/lib/insn.c
@@ -328,35 +328,6 @@ u32 aarch64_insn_gen_load_store_pair(enum aarch64_insn_register reg1,
 					     offset >> shift);
 }
 
-u32 aarch64_insn_gen_load_acq_store_rel(enum aarch64_insn_register reg,
-					enum aarch64_insn_register base,
-					enum aarch64_insn_size_type size,
-					enum aarch64_insn_ldst_type type)
-{
-	u32 insn;
-
-	switch (type) {
-	case AARCH64_INSN_LDST_LOAD_ACQ:
-		insn = aarch64_insn_get_load_acq_value();
-		break;
-	case AARCH64_INSN_LDST_STORE_REL:
-		insn = aarch64_insn_get_store_rel_value();
-		break;
-	default:
-		pr_err("%s: unknown load-acquire/store-release encoding %d\n",
-		       __func__, type);
-		return AARCH64_BREAK_FAULT;
-	}
-
-	insn = aarch64_insn_encode_ldst_size(size, insn);
-
-	insn = aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RT, insn,
-					    reg);
-
-	return aarch64_insn_encode_register(AARCH64_INSN_REGTYPE_RN, insn,
-					    base);
-}
-
 u32 aarch64_insn_gen_load_store_ex(enum aarch64_insn_register reg,
 				   enum aarch64_insn_register base,
 				   enum aarch64_insn_register state,
-- 
2.43.0

[RFC PATCH 16/16] arm64/io: rework Cortex-A57 erratum 832075 to use callback

[Ada Couprie Diaz]

The Cortex-A57 erratum 832075 fix implemented by the kernel
replaces all device memory loads with their load-acquire versions.
By using simple instruction-level alternatives to replace the 13k+
instances of such loads, we add more than 50kB of data
to the `.altinstructions` section, and thus the kernel image.

Implement `alt_cb_patch_ldr_to_ldar()` as the alternative callback
to patch LDRs to device memory into LDARs and use it instead
of the alternative instructions.

This lightens the image by around 50kB as predicted, with the same result.

The new callback is safe to be used for alternatives as it is `noinstr`
and the `aarch64_insn_...` functions it uses have been made safe
in previous commits.

Add `alt_cb_patch_ldr_to_ldar()` to the nVHE namespace as
`__vgic_v2_perform_cpuif_access()` uses one of the patched functions.

Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
---
 arch/arm64/include/asm/io.h    | 27 +++++++++++++++------------
 arch/arm64/kernel/image-vars.h |  1 +
 arch/arm64/kernel/io.c         | 21 +++++++++++++++++++++
 3 files changed, 37 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 9b96840fb979..ec75bd0a9d76 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -50,13 +50,16 @@ static __always_inline void __raw_writeq(u64 val, volatile void __iomem *addr)
 	asm volatile("str %x0, %1" : : "rZ" (val), "Qo" (*ptr));
 }
 
+void noinstr alt_cb_patch_ldr_to_ldar(struct alt_instr *alt,
+			       __le32 *origptr, __le32 *updptr, int nr_inst);
+
 #define __raw_readb __raw_readb
 static __always_inline u8 __raw_readb(const volatile void __iomem *addr)
 {
 	u8 val;
-	asm volatile(ALTERNATIVE("ldrb %w0, [%1]",
-				 "ldarb %w0, [%1]",
-				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+	asm volatile(ALTERNATIVE_CB("ldrb %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
+				 alt_cb_patch_ldr_to_ldar)
 		     : "=r" (val) : "r" (addr));
 	return val;
 }
@@ -66,9 +69,9 @@ static __always_inline u16 __raw_readw(const volatile void __iomem *addr)
 {
 	u16 val;
 
-	asm volatile(ALTERNATIVE("ldrh %w0, [%1]",
-				 "ldarh %w0, [%1]",
-				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+	asm volatile(ALTERNATIVE_CB("ldrh %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
+				 alt_cb_patch_ldr_to_ldar)
 		     : "=r" (val) : "r" (addr));
 	return val;
 }
@@ -77,9 +80,9 @@ static __always_inline u16 __raw_readw(const volatile void __iomem *addr)
 static __always_inline u32 __raw_readl(const volatile void __iomem *addr)
 {
 	u32 val;
-	asm volatile(ALTERNATIVE("ldr %w0, [%1]",
-				 "ldar %w0, [%1]",
-				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+	asm volatile(ALTERNATIVE_CB("ldr %w0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
+				 alt_cb_patch_ldr_to_ldar)
 		     : "=r" (val) : "r" (addr));
 	return val;
 }
@@ -88,9 +91,9 @@ static __always_inline u32 __raw_readl(const volatile void __iomem *addr)
 static __always_inline u64 __raw_readq(const volatile void __iomem *addr)
 {
 	u64 val;
-	asm volatile(ALTERNATIVE("ldr %0, [%1]",
-				 "ldar %0, [%1]",
-				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE)
+	asm volatile(ALTERNATIVE_CB("ldr %0, [%1]",
+				 ARM64_WORKAROUND_DEVICE_LOAD_ACQUIRE,
+				 alt_cb_patch_ldr_to_ldar)
 		     : "=r" (val) : "r" (addr));
 	return val;
 }
diff --git a/arch/arm64/kernel/image-vars.h b/arch/arm64/kernel/image-vars.h
index 714b0b5ec5ac..43ac41f87229 100644
--- a/arch/arm64/kernel/image-vars.h
+++ b/arch/arm64/kernel/image-vars.h
@@ -91,6 +91,7 @@ KVM_NVHE_ALIAS(spectre_bhb_patch_loop_mitigation_enable);
 KVM_NVHE_ALIAS(spectre_bhb_patch_wa3);
 KVM_NVHE_ALIAS(spectre_bhb_patch_clearbhb);
 KVM_NVHE_ALIAS(alt_cb_patch_nops);
+KVM_NVHE_ALIAS(alt_cb_patch_ldr_to_ldar);
 
 /* Global kernel state accessed by nVHE hyp code. */
 KVM_NVHE_ALIAS(kvm_vgic_global_state);
diff --git a/arch/arm64/kernel/io.c b/arch/arm64/kernel/io.c
index fe86ada23c7d..d4dff119f78c 100644
--- a/arch/arm64/kernel/io.c
+++ b/arch/arm64/kernel/io.c
@@ -9,6 +9,27 @@
 #include <linux/types.h>
 #include <linux/io.h>
 
+noinstr void alt_cb_patch_ldr_to_ldar(struct alt_instr *alt,
+			       __le32 *origptr, __le32 *updptr, int nr_inst)
+{
+	u32 rt, rn, size, orinst, altinst;
+
+	BUG_ON(nr_inst != 1);
+
+	orinst = le32_to_cpu(origptr[0]);
+
+	rt = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RT, orinst);
+	rn = aarch64_insn_decode_register(AARCH64_INSN_REGTYPE_RN, orinst);
+	/* The size field (31,30) matches the enum used in gen_load_acq below. */
+	size = orinst >> 30;
+
+	altinst = aarch64_insn_gen_load_acq_store_rel(rt, rn, size,
+		AARCH64_INSN_LDST_LOAD_ACQ);
+
+	updptr[0] = cpu_to_le32(altinst);
+}
+EXPORT_SYMBOL(alt_cb_patch_ldr_to_ldar);
+
 /*
  * This generates a memcpy that works on a from/to address which is aligned to
  * bits. Count is in terms of the number of bits sized quantities to copy. It
-- 
2.43.0

Re: [RFC PATCH 03/16] arm64/insn: always inline aarch64_insn_decode_register()

[Marc Zyngier]

On Tue, 23 Sep 2025 18:48:50 +0100,
Ada Couprie Diaz <ada.coupriediaz@arm.com> wrote:
> 
> As it is always called with an explicit register type, we can
> check for its validity at compile time and remove the runtime error print.
> 
> This makes `aarch64_insn_decode_register()` self-contained and safe
> for inlining and usage from patching callbacks.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/insn.h | 32 ++++++++++++++++++++++++++++++--
>  arch/arm64/lib/insn.c         | 29 -----------------------------
>  2 files changed, 30 insertions(+), 31 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 18c7811774d3..f6bce1a62dda 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -7,6 +7,7 @@
>   */
>  #ifndef	__ASM_INSN_H
>  #define	__ASM_INSN_H
> +#include <linux/bits.h>
>  #include <linux/build_bug.h>
>  #include <linux/types.h>
>  
> @@ -558,8 +559,35 @@ enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
>  u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
>  u32 aarch64_insn_encode_immediate(enum aarch64_insn_imm_type type,
>  				  u32 insn, u64 imm);
> -u32 aarch64_insn_decode_register(enum aarch64_insn_register_type type,
> -					 u32 insn);
> +static __always_inline u32 aarch64_insn_decode_register(
> +				 enum aarch64_insn_register_type type, u32 insn)
> +{
> +	compiletime_assert(type >= AARCH64_INSN_REGTYPE_RT &&
> +		type <= AARCH64_INSN_REGTYPE_RS, "unknown register type encoding");
> +	int shift;
> +
> +	switch (type) {
> +	case AARCH64_INSN_REGTYPE_RT:
> +	case AARCH64_INSN_REGTYPE_RD:
> +		shift = 0;
> +		break;
> +	case AARCH64_INSN_REGTYPE_RN:
> +		shift = 5;
> +		break;
> +	case AARCH64_INSN_REGTYPE_RT2:
> +	case AARCH64_INSN_REGTYPE_RA:
> +		shift = 10;
> +		break;
> +	case AARCH64_INSN_REGTYPE_RM:
> +	case AARCH64_INSN_REGTYPE_RS:
> +		shift = 16;
> +		break;
> +	default:
> +		return 0;

Could you replace the above compiletime_assert() with something in the
default: case instead (BUILD_BUG_ON() or otherwise)?

I'm a bit concerned that if we add an enum entry in the middle of the
current lot, this code becomes broken without us noticing. It would
also rid us of this "return 0" case, which is pretty brittle.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH 06/16] arm64/insn: always inline aarch64_insn_gen_movewide()

[Marc Zyngier]

On Tue, 23 Sep 2025 18:48:53 +0100,
Ada Couprie Diaz <ada.coupriediaz@arm.com> wrote:
> 
> As it is always called with an explicit movewide type, we can
> check for its validity at compile time and remove the runtime error print.
> 
> The other error prints cannot be verified at compile time, but should not
> occur in practice and will still lead to a fault BRK, so remove them.
> 
> This makes `aarch64_insn_gen_movewide()` safe for inlining
> and usage from patching callbacks, as both
> `aarch64_insn_encode_register()` and `aarch64_insn_encode_immediate()`
> have been made safe in previous commits.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/insn.h | 58 ++++++++++++++++++++++++++++++++---
>  arch/arm64/lib/insn.c         | 56 ---------------------------------
>  2 files changed, 54 insertions(+), 60 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 5f5f6a125b4e..5a25e311717f 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -624,6 +624,8 @@ static __always_inline bool aarch64_get_imm_shift_mask(
>  #define ADR_IMM_LOSHIFT		29
>  #define ADR_IMM_HISHIFT		5
>  
> +#define AARCH64_INSN_SF_BIT	BIT(31)
> +
>  enum aarch64_insn_encoding_class aarch64_get_insn_class(u32 insn);
>  u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn);
>  
> @@ -796,10 +798,58 @@ u32 aarch64_insn_gen_bitfield(enum aarch64_insn_register dst,
>  			      int immr, int imms,
>  			      enum aarch64_insn_variant variant,
>  			      enum aarch64_insn_bitfield_type type);
> -u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
> -			      int imm, int shift,
> -			      enum aarch64_insn_variant variant,
> -			      enum aarch64_insn_movewide_type type);
> +
> +static __always_inline u32 aarch64_insn_gen_movewide(
> +				 enum aarch64_insn_register dst,
> +				 int imm, int shift,
> +				 enum aarch64_insn_variant variant,
> +				 enum aarch64_insn_movewide_type type)

nit: I personally find this definition style pretty unreadable, and
would rather see the "static __always_inline" stuff put on a line of
its own:

static __always_inline
u32 aarch64_insn_gen_movewide(enum aarch64_insn_register dst,
			      int imm, int shift,
			      enum aarch64_insn_variant variant,
			      enum aarch64_insn_movewide_type type)

But again, that's a personal preference, nothing else.

> +{
> +	compiletime_assert(type >=  AARCH64_INSN_MOVEWIDE_ZERO &&
> +		type <= AARCH64_INSN_MOVEWIDE_INVERSE, "unknown movewide encoding");
> +	u32 insn;
> +
> +	switch (type) {
> +	case AARCH64_INSN_MOVEWIDE_ZERO:
> +		insn = aarch64_insn_get_movz_value();
> +		break;
> +	case AARCH64_INSN_MOVEWIDE_KEEP:
> +		insn = aarch64_insn_get_movk_value();
> +		break;
> +	case AARCH64_INSN_MOVEWIDE_INVERSE:
> +		insn = aarch64_insn_get_movn_value();
> +		break;
> +	default:
> +		return AARCH64_BREAK_FAULT;

Similar request to one of the previous patches: since you can check
the validity at compile time, place it in the default: case, and drop
the return statement.

> +	}
> +
> +	if (imm & ~(SZ_64K - 1)) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	switch (variant) {
> +	case AARCH64_INSN_VARIANT_32BIT:
> +		if (shift != 0 && shift != 16) {
> +			return AARCH64_BREAK_FAULT;
> +		}
> +		break;
> +	case AARCH64_INSN_VARIANT_64BIT:
> +		insn |= AARCH64_INSN_SF_BIT;
> +		if (shift != 0 && shift != 16 && shift != 32 && shift != 48) {
> +			return AARCH64_BREAK_FAULT;
> +		}
> +		break;
> +	default:
> +		return AARCH64_BREAK_FAULT;

You could also check the variant.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH 12/16] kvm/arm64: make alternative callbacks safe

[Marc Zyngier]

nit: please keep $SUBJECT in keeping with the subsystem you are
patching: "KVM: arm64: Make alternative callbacks safe"

On Tue, 23 Sep 2025 18:48:59 +0100,
Ada Couprie Diaz <ada.coupriediaz@arm.com> wrote:
> 
> Alternative callback functions are regular functions, which means they
> or any function they call could get patched or instrumented
> by alternatives or other parts of the kernel.
> Given that applying alternatives does not guarantee a consistent state
> while patching, only once done, and handles cache maintenance manually,
> it could lead to nasty corruptions and execution of bogus code.
> 
> Make the KVM alternative callbacks safe by marking them `noinstr` and
> `__always_inline`'ing their helpers.
> This is possible thanks to previous commits making `aarch64_insn_...`
> functions used in the callbacks safe to inline.
> 
> `kvm_update_va_mask()` is already marked as `__init`, which has its own
> text section conflicting with the `noinstr` one.
> Instead, use `__no_instr_section(".noinstr.text")` to add
> all the function attributes added by `noinstr`, without the section
> conflict.
> This can be an issue, as kprobes seems to only block the text sections,
> not based on function attributes.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
> This is missing `kvm_patch_vector_branch()`, which could receive the same
> treatment, but the `WARN_ON_ONCE` in the early-exit check would make it
> call into instrumentable code.
> I do not currently know if this `WARN` can safely be removed or if it
> has some importance.

This is only debug code, which could be easily removed. However, I'd
like to suggest that we add a method to indicate that a patching
callback has failed for whatever reason. This doesn't have to be a
complex piece of infrastructure, and can be best effort (you can only
fail a single callback in a single location).

But it would certainly help catching unexpected situations (been
there, done that, ended up with visible scars...).

> ---
>  arch/arm64/kvm/va_layout.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm64/kvm/va_layout.c b/arch/arm64/kvm/va_layout.c
> index 91b22a014610..3ebb7e0074f6 100644
> --- a/arch/arm64/kvm/va_layout.c
> +++ b/arch/arm64/kvm/va_layout.c
> @@ -109,7 +109,7 @@ __init void kvm_apply_hyp_relocations(void)
>  	}
>  }
>  
> -static u32 compute_instruction(int n, u32 rd, u32 rn)
> +static __always_inline u32 compute_instruction(int n, u32 rd, u32 rn)
>  {
>  	u32 insn = AARCH64_BREAK_FAULT;
>  
> @@ -151,6 +151,7 @@ static u32 compute_instruction(int n, u32 rd, u32 rn)
>  	return insn;
>  }
>  
> +__noinstr_section(".init.text")
>  void __init kvm_update_va_mask(struct alt_instr *alt,
>  			       __le32 *origptr, __le32 *updptr, int nr_inst)
>  {
> @@ -241,7 +242,8 @@ void kvm_patch_vector_branch(struct alt_instr *alt,
>  	*updptr++ = cpu_to_le32(insn);
>  }
>  
> -static void generate_mov_q(u64 val, __le32 *origptr, __le32 *updptr, int nr_inst)
> +static __always_inline void generate_mov_q(u64 val, __le32 *origptr,
> +				 __le32 *updptr, int nr_inst)
>  {
>  	u32 insn, oinsn, rd;
>

generate_mov_q() (and others) start with a BUG_ON(), which may not be
recoverable, just like WARN_ON() above. That's where we should be able
to fail (more or less) gracefully and at least indicate the failure.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [RFC PATCH 14/16] arm64/insn: always inline aarch64_insn_encode_ldst_size()

[Marc Zyngier]

On Tue, 23 Sep 2025 18:49:01 +0100,
Ada Couprie Diaz <ada.coupriediaz@arm.com> wrote:
> 
> The type and instruction checks cannot be made at compile time,
> as they are dynamically created. However, we can remove the error print
> as it should never appear in normal operation and will still lead to
> a fault BRK.
> 
> This makes `aarch64_insn_encode_ldst_size()` safe for inlining
> and usage from patching callbacks.
> 
> This is a change of visiblity, as previously the function was private to
> lib/insn.c.
> However, in order to inline more `aarch64_insn_` functions and make
> patching callbacks safe, it needs to be accessible by those functions.
> As it is more accessible than before, add a check so that only loads
> or stores can be affected by the size encoding.
> 
> Signed-off-by: Ada Couprie Diaz <ada.coupriediaz@arm.com>
> ---
>  arch/arm64/include/asm/insn.h | 24 ++++++++++++++++++++++++
>  arch/arm64/lib/insn.c         | 19 +------------------
>  2 files changed, 25 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
> index 44435eede1f3..46d4d452e2e2 100644
> --- a/arch/arm64/include/asm/insn.h
> +++ b/arch/arm64/include/asm/insn.h
> @@ -717,6 +717,30 @@ static __always_inline u32 aarch64_insn_encode_immediate(
>  
>  	return insn;
>  }
> +
> +extern const u32 aarch64_insn_ldst_size[];
> +static __always_inline u32 aarch64_insn_encode_ldst_size(
> +					 enum aarch64_insn_size_type type,
> +					 u32 insn)
> +{
> +	u32 size;
> +
> +	if (type < AARCH64_INSN_SIZE_8 || type > AARCH64_INSN_SIZE_64) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	/* Don't corrput the top bits of other instructions which aren't a size. */
> +	if (!aarch64_insn_is_ldst(insn)) {
> +		return AARCH64_BREAK_FAULT;
> +	}
> +
> +	size = aarch64_insn_ldst_size[type];

Given that we have this:

	enum aarch64_insn_size_type {
		AARCH64_INSN_SIZE_8,
		AARCH64_INSN_SIZE_16,
		AARCH64_INSN_SIZE_32,
		AARCH64_INSN_SIZE_64,
	};

[...]

> +	insn &= ~GENMASK(31, 30);
> +	insn |= size << 30;
> +
> +	return insn;
> +}
> +
>  static __always_inline u32 aarch64_insn_encode_register(
>  				 enum aarch64_insn_register_type type,
>  				 u32 insn,
> diff --git a/arch/arm64/lib/insn.c b/arch/arm64/lib/insn.c
> index 71df4d72ac81..63564d236235 100644
> --- a/arch/arm64/lib/insn.c
> +++ b/arch/arm64/lib/insn.c
> @@ -42,30 +42,13 @@ u64 aarch64_insn_decode_immediate(enum aarch64_insn_imm_type type, u32 insn)
>  	return (insn >> shift) & mask;
>  }
>  
> -static const u32 aarch64_insn_ldst_size[] = {
> +const u32 aarch64_insn_ldst_size[] = {
>  	[AARCH64_INSN_SIZE_8] = 0,
>  	[AARCH64_INSN_SIZE_16] = 1,
>  	[AARCH64_INSN_SIZE_32] = 2,
>  	[AARCH64_INSN_SIZE_64] = 3,
>  };

[...] this array is completely superfluous, and

	size = aarch64_insn_ldst_size[type];

could be replaced by

	size = type;

But that's only a very minor improvement. On the plus side, your
approach definitely makes it impossible to add a patching callback
using aarch64_insn_encode_ldst_size() in a module!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.