[PATCH v4 0/2] Tracepoints and static branch in Rust

[PATCH v4 0/2] Tracepoints and static branch in Rust

[Alice Ryhl]

An important part of a production ready Linux kernel driver is
tracepoints. So to write production ready Linux kernel drivers in Rust,
we must be able to call tracepoints from Rust code. This patch series
adds support for calling tracepoints declared in C from Rust.

To use the tracepoint support, you must:

1. Declare the tracepoint in a C header file as usual.

2. Add #define CREATE_RUST_TRACE_POINTS next to your
   #define CREATE_TRACE_POINTS.

2. Make sure that the header file is visible to bindgen.

3. Use the declare_trace! macro in your Rust code to generate Rust
   functions that call into the tracepoint.

For example, the kernel has a tracepoint called `sched_kthread_stop`. It
is declared like this:

	TRACE_EVENT(sched_kthread_stop,
		TP_PROTO(struct task_struct *t),
		TP_ARGS(t),
		TP_STRUCT__entry(
			__array(	char,	comm,	TASK_COMM_LEN	)
			__field(	pid_t,	pid			)
		),
		TP_fast_assign(
			memcpy(__entry->comm, t->comm, TASK_COMM_LEN);
			__entry->pid	= t->pid;
		),
		TP_printk("comm=%s pid=%d", __entry->comm, __entry->pid)
	);

To call the above tracepoint from Rust code, you must first ensure that
the Rust helper for the tracepoint is generated. To do this, you would
modify kernel/sched/core.c by adding #define CREATE_RUST_TRACE_POINTS.

Next, you would include include/trace/events/sched.h in
rust/bindings/bindings_helper.h so that the exported C functions are
visible to Rust, and then you would declare the tracepoint in Rust:

	declare_trace! {
	    fn sched_kthread_stop(task: *mut task_struct);
	}

This will define an inline Rust function that checks the static key,
calling into rust_do_trace_##name if the tracepoint is active. Since
these tracepoints often take raw pointers as arguments, it may be
convenient to wrap it in a safe wrapper:

	mod raw {
	    declare_trace! {
	        fn sched_kthread_stop(task: *mut task_struct);
	    }
	}
	
	#[inline]
	pub fn trace_sched_kthread_stop(task: &Task) {
	    // SAFETY: The pointer to `task` is valid.
	    unsafe { raw::sched_kthread_stop(task.as_raw()) }
	}

A future expansion of the tracepoint support could generate these safe
versions automatically, but that is left as future work for now.

This is intended for use in the Rust Binder driver, which was originally
sent as an RFC [1]. The RFC did not include tracepoint support, but you
can see how it will be used in Rust Binder at [2]. The author has
verified that the tracepoint support works on Android devices.

This implementation implements support for static keys in Rust so that
the actual static branch happens in the Rust object file. However, the
__DO_TRACE body remains in C code. See v1 for an implementation where
__DO_TRACE is also implemented in Rust.

Link: https://lore.kernel.org/rust-for-linux/20231101-rust-binder-v1-0-08ba9197f637@google.com/ [1]
Link: https://r.android.com/3119993 [2]
Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
Changes in v4:
- Move arch-specific code into rust/kernel/arch.
- Restore DEFINE_RUST_DO_TRACE at end of define_trace.h
- Link to v3: https://lore.kernel.org/r/20240621-tracepoint-v3-0-9e44eeea2b85@google.com

Changes in v3:
- Support for Rust static_key on loongarch64 and riscv64.
- Avoid failing compilation on architectures that are missing Rust
  static_key support when the archtectures does not actually use it.
- Link to v2: https://lore.kernel.org/r/20240610-tracepoint-v2-0-faebad81b355@google.com

Changes in v2:
- Call into C code for __DO_TRACE.
- Drop static_call patch, as it is no longer needed.
- Link to v1: https://lore.kernel.org/r/20240606-tracepoint-v1-0-6551627bf51b@google.com

---
Alice Ryhl (2):
      rust: add static_key_false
      rust: add tracepoint support

 include/linux/tracepoint.h               | 18 +++++++++++-
 include/trace/define_trace.h             | 12 ++++++++
 rust/bindings/bindings_helper.h          |  1 +
 rust/kernel/arch/arm64/jump_label.rs     | 34 +++++++++++++++++++++++
 rust/kernel/arch/loongarch/jump_label.rs | 35 ++++++++++++++++++++++++
 rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++
 rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++
 rust/kernel/arch/x86/jump_label.rs       | 35 ++++++++++++++++++++++++
 rust/kernel/lib.rs                       |  3 ++
 rust/kernel/static_key.rs                | 32 ++++++++++++++++++++++
 rust/kernel/tracepoint.rs                | 47 ++++++++++++++++++++++++++++++++
 scripts/Makefile.build                   |  2 +-
 12 files changed, 279 insertions(+), 2 deletions(-)
---
base-commit: 1613e604df0cd359cf2a7fbd9be7a0bcfacfabd0
change-id: 20240606-tracepoint-31e15b90e471

Best regards,
-- 
Alice Ryhl <aliceryhl@google.com>

[PATCH v4 1/2] rust: add static_key_false

[Alice Ryhl]

Add just enough support for static key so that we can use it from
tracepoints. Tracepoints rely on `static_key_false` even though it is
deprecated, so we add the same functionality to Rust.

It is not possible to use the existing C implementation of
arch_static_branch because it passes the argument `key` to inline
assembly as an 'i' parameter, so any attempt to add a C helper for this
function will fail to compile because the value of `key` must be known
at compile-time.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
 rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
 rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
 rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
 rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
 rust/kernel/lib.rs                       |  2 ++
 rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
 scripts/Makefile.build                   |  2 +-
 8 files changed, 201 insertions(+), 1 deletion(-)

diff --git a/rust/kernel/arch/arm64/jump_label.rs b/rust/kernel/arch/arm64/jump_label.rs
new file mode 100644
index 000000000000..5eede2245718
--- /dev/null
+++ b/rust/kernel/arch/arm64/jump_label.rs
@@ -0,0 +1,34 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Arm64 Rust implementation of jump_label.h
+
+/// arm64 implementation of arch_static_branch
+#[macro_export]
+#[cfg(target_arch = "aarch64")]
+macro_rules! arch_static_branch {
+    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
+        core::arch::asm!(
+            r#"
+            1: nop
+
+            .pushsection __jump_table,  "aw"
+            .align 3
+            .long 1b - ., {0} - .
+            .quad {1} + {2} + {3} - .
+            .popsection
+            "#,
+            label {
+                break 'my_label true;
+            },
+            sym $key,
+            const ::core::mem::offset_of!($keytyp, $field),
+            const $crate::arch::bool_to_int($branch),
+        );
+
+        break 'my_label false;
+    }};
+}
+
+pub use arch_static_branch;
diff --git a/rust/kernel/arch/loongarch/jump_label.rs b/rust/kernel/arch/loongarch/jump_label.rs
new file mode 100644
index 000000000000..8d31318aeb11
--- /dev/null
+++ b/rust/kernel/arch/loongarch/jump_label.rs
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Loongarch Rust implementation of jump_label.h
+
+/// loongarch implementation of arch_static_branch
+#[doc(hidden)]
+#[macro_export]
+#[cfg(target_arch = "loongarch64")]
+macro_rules! arch_static_branch {
+    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
+        core::arch::asm!(
+            r#"
+            1: nop
+
+            .pushsection __jump_table,  "aw"
+            .align 3
+            .long 1b - ., {0} - .
+            .quad {1} + {2} + {3} - .
+            .popsection
+            "#,
+            label {
+                break 'my_label true;
+            },
+            sym $key,
+            const ::core::mem::offset_of!($keytyp, $field),
+            const $crate::arch::bool_to_int($branch),
+        );
+
+        break 'my_label false;
+    }};
+}
+
+pub use arch_static_branch;
diff --git a/rust/kernel/arch/mod.rs b/rust/kernel/arch/mod.rs
new file mode 100644
index 000000000000..14271d2530e9
--- /dev/null
+++ b/rust/kernel/arch/mod.rs
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Architecture specific code.
+
+#[cfg_attr(target_arch = "aarch64", path = "arm64")]
+#[cfg_attr(target_arch = "x86_64", path = "x86")]
+#[cfg_attr(target_arch = "loongarch64", path = "loongarch")]
+#[cfg_attr(target_arch = "riscv64", path = "riscv")]
+mod inner {
+    pub mod jump_label;
+}
+
+pub use self::inner::*;
+
+/// A helper used by inline assembly to pass a boolean to as a `const` parameter.
+///
+/// Using this function instead of a cast lets you assert that the input is a boolean, rather than
+/// some other type that can be cast to an integer.
+#[doc(hidden)]
+pub const fn bool_to_int(b: bool) -> i32 {
+    b as i32
+}
diff --git a/rust/kernel/arch/riscv/jump_label.rs b/rust/kernel/arch/riscv/jump_label.rs
new file mode 100644
index 000000000000..2672e0c6f033
--- /dev/null
+++ b/rust/kernel/arch/riscv/jump_label.rs
@@ -0,0 +1,38 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! RiscV Rust implementation of jump_label.h
+
+/// riscv implementation of arch_static_branch
+#[macro_export]
+#[cfg(target_arch = "riscv64")]
+macro_rules! arch_static_branch {
+    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
+        core::arch::asm!(
+            r#"
+            .align  2
+            .option push
+            .option norelax
+            .option norvc
+            1: nop
+            .option pop
+            .pushsection __jump_table,  "aw"
+            .align 3
+            .long 1b - ., {0} - .
+            .dword {1} + {2} + {3} - .
+            .popsection
+            "#,
+            label {
+                break 'my_label true;
+            },
+            sym $key,
+            const ::core::mem::offset_of!($keytyp, $field),
+            const $crate::arch::bool_to_int($branch),
+        );
+
+        break 'my_label false;
+    }};
+}
+
+pub use arch_static_branch;
diff --git a/rust/kernel/arch/x86/jump_label.rs b/rust/kernel/arch/x86/jump_label.rs
new file mode 100644
index 000000000000..383bed273c50
--- /dev/null
+++ b/rust/kernel/arch/x86/jump_label.rs
@@ -0,0 +1,35 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! X86 Rust implementation of jump_label.h
+
+/// x86 implementation of arch_static_branch
+#[macro_export]
+#[cfg(target_arch = "x86_64")]
+macro_rules! arch_static_branch {
+    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
+        core::arch::asm!(
+            r#"
+            1: .byte 0x0f,0x1f,0x44,0x00,0x00
+
+            .pushsection __jump_table,  "aw"
+            .balign 8
+            .long 1b - .
+            .long {0} - .
+            .quad {1} + {2} + {3} - .
+            .popsection
+            "#,
+            label {
+                break 'my_label true;
+            },
+            sym $key,
+            const ::core::mem::offset_of!($keytyp, $field),
+            const $crate::arch::bool_to_int($branch),
+        );
+
+        break 'my_label false;
+    }};
+}
+
+pub use arch_static_branch;
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fbd91a48ff8b..fffd4e1dd1c1 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -27,6 +27,7 @@
 extern crate self as kernel;
 
 pub mod alloc;
+pub mod arch;
 mod build_assert;
 pub mod error;
 pub mod init;
@@ -38,6 +39,7 @@
 pub mod prelude;
 pub mod print;
 mod static_assert;
+pub mod static_key;
 #[doc(hidden)]
 pub mod std_vendor;
 pub mod str;
diff --git a/rust/kernel/static_key.rs b/rust/kernel/static_key.rs
new file mode 100644
index 000000000000..32cf027ef091
--- /dev/null
+++ b/rust/kernel/static_key.rs
@@ -0,0 +1,32 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Logic for static keys.
+
+use crate::bindings::*;
+
+/// Branch based on a static key.
+///
+/// Takes three arguments:
+///
+/// * `key` - the path to the static variable containing the `static_key`.
+/// * `keytyp` - the type of `key`.
+/// * `field` - the name of the field of `key` that contains the `static_key`.
+#[macro_export]
+macro_rules! static_key_false {
+    ($key:path, $keytyp:ty, $field:ident) => {{
+        // Assert that `$key` has type `$keytyp` and that `$key.$field` has type `static_key`.
+        //
+        // SAFETY: We know that `$key` is a static because otherwise the inline assembly will not
+        // compile. The raw pointers created in this block are in-bounds of `$key`.
+        static _TY_ASSERT: () = unsafe {
+            let key: *const $keytyp = ::core::ptr::addr_of!($key);
+            let _: *const $crate::bindings::static_key = ::core::ptr::addr_of!((*key).$field);
+        };
+
+        $crate::arch::jump_label::arch_static_branch! { $key, $keytyp, $field, false }
+    }};
+}
+
+pub use static_key_false;
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index efacca63c897..60197c1c063f 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
 # Compile Rust sources (.rs)
 # ---------------------------------------------------------------------------
 
-rust_allowed_features := new_uninit
+rust_allowed_features := asm_const,asm_goto,new_uninit
 
 # `--out-dir` is required to avoid temporaries being created by `rustc` in the
 # current working directory, which may be not accessible in the out-of-tree

-- 
2.45.2.803.g4e1b14247a-goog

[PATCH v4 2/2] rust: add tracepoint support

[Alice Ryhl]

Make it possible to have Rust code call into tracepoints defined by C
code. It is still required that the tracepoint is declared in a C
header, and that this header is included in the input to bindgen.

Signed-off-by: Alice Ryhl <aliceryhl@google.com>
---
 include/linux/tracepoint.h      | 18 +++++++++++++++-
 include/trace/define_trace.h    | 12 +++++++++++
 rust/bindings/bindings_helper.h |  1 +
 rust/kernel/lib.rs              |  1 +
 rust/kernel/tracepoint.rs       | 47 +++++++++++++++++++++++++++++++++++++++++
 5 files changed, 78 insertions(+), 1 deletion(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 689b6d71590e..d82af4d77c9f 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -238,6 +238,20 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #define __DECLARE_TRACE_RCU(name, proto, args, cond)
 #endif
 
+/*
+ * Declare an exported function that Rust code can call to trigger this
+ * tracepoint. This function does not include the static branch; that is done
+ * in Rust to avoid a function call when the tracepoint is disabled.
+ */
+#define DEFINE_RUST_DO_TRACE(name, proto, args)
+#define DEFINE_RUST_DO_TRACE_REAL(name, proto, args)			\
+	notrace void rust_do_trace_##name(proto)			\
+	{								\
+		__DO_TRACE(name,					\
+			TP_ARGS(args),					\
+			cpu_online(raw_smp_processor_id()), 0);		\
+	}
+
 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the
@@ -253,6 +267,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	extern int __traceiter_##name(data_proto);			\
 	DECLARE_STATIC_CALL(tp_func_##name, __traceiter_##name);	\
 	extern struct tracepoint __tracepoint_##name;			\
+	extern void rust_do_trace_##name(proto);			\
 	static inline void trace_##name(proto)				\
 	{								\
 		if (static_key_false(&__tracepoint_##name.key))		\
@@ -337,7 +352,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	void __probestub_##_name(void *__data, proto)			\
 	{								\
 	}								\
-	DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
+	DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);	\
+	DEFINE_RUST_DO_TRACE(_name, TP_PROTO(proto), TP_ARGS(args))
 
 #define DEFINE_TRACE(name, proto, args)		\
 	DEFINE_TRACE_FN(name, NULL, NULL, PARAMS(proto), PARAMS(args));
diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
index 00723935dcc7..08ed5ce63a96 100644
--- a/include/trace/define_trace.h
+++ b/include/trace/define_trace.h
@@ -72,6 +72,13 @@
 #define DECLARE_TRACE(name, proto, args)	\
 	DEFINE_TRACE(name, PARAMS(proto), PARAMS(args))
 
+/* If requested, create helpers for calling these tracepoints from Rust. */
+#ifdef CREATE_RUST_TRACE_POINTS
+#undef DEFINE_RUST_DO_TRACE
+#define DEFINE_RUST_DO_TRACE(name, proto, args)	\
+	DEFINE_RUST_DO_TRACE_REAL(name, PARAMS(proto), PARAMS(args))
+#endif
+
 #undef TRACE_INCLUDE
 #undef __TRACE_INCLUDE
 
@@ -129,6 +136,11 @@
 # undef UNDEF_TRACE_INCLUDE_PATH
 #endif
 
+#ifdef CREATE_RUST_TRACE_POINTS
+# undef DEFINE_RUST_DO_TRACE
+# define DEFINE_RUST_DO_TRACE(name, proto, args)
+#endif
+
 /* We may be processing more files */
 #define CREATE_TRACE_POINTS
 
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index ddb5644d4fd9..d442f9ccfc2c 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -15,6 +15,7 @@
 #include <linux/refcount.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
+#include <linux/tracepoint.h>
 #include <linux/wait.h>
 #include <linux/workqueue.h>
 
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index fffd4e1dd1c1..9ae90eb69020 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -46,6 +46,7 @@
 pub mod sync;
 pub mod task;
 pub mod time;
+pub mod tracepoint;
 pub mod types;
 pub mod workqueue;
 
diff --git a/rust/kernel/tracepoint.rs b/rust/kernel/tracepoint.rs
new file mode 100644
index 000000000000..1005f09e0330
--- /dev/null
+++ b/rust/kernel/tracepoint.rs
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0
+
+// Copyright (C) 2024 Google LLC.
+
+//! Logic for tracepoints.
+
+/// Declare the Rust entry point for a tracepoint.
+#[macro_export]
+macro_rules! declare_trace {
+    ($($(#[$attr:meta])* $pub:vis fn $name:ident($($argname:ident : $argtyp:ty),* $(,)?);)*) => {$(
+        $( #[$attr] )*
+        #[inline(always)]
+        $pub unsafe fn $name($($argname : $argtyp),*) {
+            #[cfg(CONFIG_TRACEPOINTS)]
+            {
+                use $crate::bindings::*;
+
+                // SAFETY: It's always okay to query the static key for a tracepoint.
+                let should_trace = unsafe {
+                    $crate::macros::paste! {
+                        $crate::static_key::static_key_false!(
+                            [< __tracepoint_ $name >],
+                            $crate::bindings::tracepoint,
+                            key
+                        )
+                    }
+                };
+
+                if should_trace {
+                    $crate::macros::paste! {
+                        // SAFETY: The caller guarantees that it is okay to call this tracepoint.
+                        unsafe { [< rust_do_trace_ $name >]($($argname),*) };
+                    }
+                }
+            }
+
+            #[cfg(not(CONFIG_TRACEPOINTS))]
+            {
+                // If tracepoints are disabled, insert a trivial use of each argument
+                // to avoid unused argument warnings.
+                $( let _unused = $argname; )*
+            }
+        }
+    )*}
+}
+
+pub use declare_trace;

-- 
2.45.2.803.g4e1b14247a-goog

Re: [PATCH v4 1/2] rust: add static_key_false

[hev]

Hi Alice

On Fri, Jun 28, 2024 at 9:24 PM Alice Ryhl <aliceryhl@google.com> wrote:
>
> Add just enough support for static key so that we can use it from
> tracepoints. Tracepoints rely on `static_key_false` even though it is
> deprecated, so we add the same functionality to Rust.
>
> It is not possible to use the existing C implementation of
> arch_static_branch because it passes the argument `key` to inline
> assembly as an 'i' parameter, so any attempt to add a C helper for this
> function will fail to compile because the value of `key` must be known
> at compile-time.
>
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
>  rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
>  rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
>  rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
>  rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                       |  2 ++
>  rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
>  scripts/Makefile.build                   |  2 +-
>  8 files changed, 201 insertions(+), 1 deletion(-)
>
> diff --git a/rust/kernel/arch/arm64/jump_label.rs b/rust/kernel/arch/arm64/jump_label.rs
> new file mode 100644
> index 000000000000..5eede2245718
> --- /dev/null
> +++ b/rust/kernel/arch/arm64/jump_label.rs
> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Arm64 Rust implementation of jump_label.h
> +
> +/// arm64 implementation of arch_static_branch
> +#[macro_export]
> +#[cfg(target_arch = "aarch64")]
> +macro_rules! arch_static_branch {
> +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> +        core::arch::asm!(
> +            r#"
> +            1: nop
> +
> +            .pushsection __jump_table,  "aw"
> +            .align 3
> +            .long 1b - ., {0} - .
> +            .quad {1} + {2} + {3} - .
> +            .popsection
> +            "#,
> +            label {
> +                break 'my_label true;
> +            },
> +            sym $key,
> +            const ::core::mem::offset_of!($keytyp, $field),
> +            const $crate::arch::bool_to_int($branch),
> +        );
> +
> +        break 'my_label false;
> +    }};
> +}
> +
> +pub use arch_static_branch;
> diff --git a/rust/kernel/arch/loongarch/jump_label.rs b/rust/kernel/arch/loongarch/jump_label.rs
> new file mode 100644
> index 000000000000..8d31318aeb11
> --- /dev/null
> +++ b/rust/kernel/arch/loongarch/jump_label.rs
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Loongarch Rust implementation of jump_label.h
> +
> +/// loongarch implementation of arch_static_branch
> +#[doc(hidden)]
> +#[macro_export]
> +#[cfg(target_arch = "loongarch64")]
> +macro_rules! arch_static_branch {
> +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> +        core::arch::asm!(
> +            r#"
> +            1: nop
> +
> +            .pushsection __jump_table,  "aw"
> +            .align 3
> +            .long 1b - ., {0} - .
> +            .quad {1} + {2} + {3} - .
> +            .popsection
> +            "#,
> +            label {
> +                break 'my_label true;
> +            },
> +            sym $key,
> +            const ::core::mem::offset_of!($keytyp, $field),
> +            const $crate::arch::bool_to_int($branch),
> +        );
> +
> +        break 'my_label false;
> +    }};
> +}

I have tested these patches on LoongArch and it works as expected.

Tested-by: WANG Rui <wangrui@loongson.cn>

Thanks,
-Rui

> +
> +pub use arch_static_branch;
> diff --git a/rust/kernel/arch/mod.rs b/rust/kernel/arch/mod.rs
> new file mode 100644
> index 000000000000..14271d2530e9
> --- /dev/null
> +++ b/rust/kernel/arch/mod.rs
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Architecture specific code.
> +
> +#[cfg_attr(target_arch = "aarch64", path = "arm64")]
> +#[cfg_attr(target_arch = "x86_64", path = "x86")]
> +#[cfg_attr(target_arch = "loongarch64", path = "loongarch")]
> +#[cfg_attr(target_arch = "riscv64", path = "riscv")]
> +mod inner {
> +    pub mod jump_label;
> +}
> +
> +pub use self::inner::*;
> +
> +/// A helper used by inline assembly to pass a boolean to as a `const` parameter.
> +///
> +/// Using this function instead of a cast lets you assert that the input is a boolean, rather than
> +/// some other type that can be cast to an integer.
> +#[doc(hidden)]
> +pub const fn bool_to_int(b: bool) -> i32 {
> +    b as i32
> +}
> diff --git a/rust/kernel/arch/riscv/jump_label.rs b/rust/kernel/arch/riscv/jump_label.rs
> new file mode 100644
> index 000000000000..2672e0c6f033
> --- /dev/null
> +++ b/rust/kernel/arch/riscv/jump_label.rs
> @@ -0,0 +1,38 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! RiscV Rust implementation of jump_label.h
> +
> +/// riscv implementation of arch_static_branch
> +#[macro_export]
> +#[cfg(target_arch = "riscv64")]
> +macro_rules! arch_static_branch {
> +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> +        core::arch::asm!(
> +            r#"
> +            .align  2
> +            .option push
> +            .option norelax
> +            .option norvc
> +            1: nop
> +            .option pop
> +            .pushsection __jump_table,  "aw"
> +            .align 3
> +            .long 1b - ., {0} - .
> +            .dword {1} + {2} + {3} - .
> +            .popsection
> +            "#,
> +            label {
> +                break 'my_label true;
> +            },
> +            sym $key,
> +            const ::core::mem::offset_of!($keytyp, $field),
> +            const $crate::arch::bool_to_int($branch),
> +        );
> +
> +        break 'my_label false;
> +    }};
> +}
> +
> +pub use arch_static_branch;
> diff --git a/rust/kernel/arch/x86/jump_label.rs b/rust/kernel/arch/x86/jump_label.rs
> new file mode 100644
> index 000000000000..383bed273c50
> --- /dev/null
> +++ b/rust/kernel/arch/x86/jump_label.rs
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! X86 Rust implementation of jump_label.h
> +
> +/// x86 implementation of arch_static_branch
> +#[macro_export]
> +#[cfg(target_arch = "x86_64")]
> +macro_rules! arch_static_branch {
> +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> +        core::arch::asm!(
> +            r#"
> +            1: .byte 0x0f,0x1f,0x44,0x00,0x00
> +
> +            .pushsection __jump_table,  "aw"
> +            .balign 8
> +            .long 1b - .
> +            .long {0} - .
> +            .quad {1} + {2} + {3} - .
> +            .popsection
> +            "#,
> +            label {
> +                break 'my_label true;
> +            },
> +            sym $key,
> +            const ::core::mem::offset_of!($keytyp, $field),
> +            const $crate::arch::bool_to_int($branch),
> +        );
> +
> +        break 'my_label false;
> +    }};
> +}
> +
> +pub use arch_static_branch;
> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
> index fbd91a48ff8b..fffd4e1dd1c1 100644
> --- a/rust/kernel/lib.rs
> +++ b/rust/kernel/lib.rs
> @@ -27,6 +27,7 @@
>  extern crate self as kernel;
>
>  pub mod alloc;
> +pub mod arch;
>  mod build_assert;
>  pub mod error;
>  pub mod init;
> @@ -38,6 +39,7 @@
>  pub mod prelude;
>  pub mod print;
>  mod static_assert;
> +pub mod static_key;
>  #[doc(hidden)]
>  pub mod std_vendor;
>  pub mod str;
> diff --git a/rust/kernel/static_key.rs b/rust/kernel/static_key.rs
> new file mode 100644
> index 000000000000..32cf027ef091
> --- /dev/null
> +++ b/rust/kernel/static_key.rs
> @@ -0,0 +1,32 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Logic for static keys.
> +
> +use crate::bindings::*;
> +
> +/// Branch based on a static key.
> +///
> +/// Takes three arguments:
> +///
> +/// * `key` - the path to the static variable containing the `static_key`.
> +/// * `keytyp` - the type of `key`.
> +/// * `field` - the name of the field of `key` that contains the `static_key`.
> +#[macro_export]
> +macro_rules! static_key_false {
> +    ($key:path, $keytyp:ty, $field:ident) => {{
> +        // Assert that `$key` has type `$keytyp` and that `$key.$field` has type `static_key`.
> +        //
> +        // SAFETY: We know that `$key` is a static because otherwise the inline assembly will not
> +        // compile. The raw pointers created in this block are in-bounds of `$key`.
> +        static _TY_ASSERT: () = unsafe {
> +            let key: *const $keytyp = ::core::ptr::addr_of!($key);
> +            let _: *const $crate::bindings::static_key = ::core::ptr::addr_of!((*key).$field);
> +        };
> +
> +        $crate::arch::jump_label::arch_static_branch! { $key, $keytyp, $field, false }
> +    }};
> +}
> +
> +pub use static_key_false;
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index efacca63c897..60197c1c063f 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -263,7 +263,7 @@ $(obj)/%.lst: $(obj)/%.c FORCE
>  # Compile Rust sources (.rs)
>  # ---------------------------------------------------------------------------
>
> -rust_allowed_features := new_uninit
> +rust_allowed_features := asm_const,asm_goto,new_uninit
>
>  # `--out-dir` is required to avoid temporaries being created by `rustc` in the
>  # current working directory, which may be not accessible in the out-of-tree
>
> --
> 2.45.2.803.g4e1b14247a-goog
>
>

Re: [PATCH v4 2/2] rust: add tracepoint support

[Carlos Llamas]

On Fri, Jun 28, 2024 at 01:23:32PM +0000, Alice Ryhl wrote:
> Make it possible to have Rust code call into tracepoints defined by C
> code. It is still required that the tracepoint is declared in a C
> header, and that this header is included in the input to bindgen.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>
> ---
>  include/linux/tracepoint.h      | 18 +++++++++++++++-
>  include/trace/define_trace.h    | 12 +++++++++++
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/lib.rs              |  1 +
>  rust/kernel/tracepoint.rs       | 47 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 689b6d71590e..d82af4d77c9f 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -238,6 +238,20 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  #define __DECLARE_TRACE_RCU(name, proto, args, cond)
>  #endif
>  
> +/*
> + * Declare an exported function that Rust code can call to trigger this
> + * tracepoint. This function does not include the static branch; that is done
> + * in Rust to avoid a function call when the tracepoint is disabled.
> + */
> +#define DEFINE_RUST_DO_TRACE(name, proto, args)
> +#define DEFINE_RUST_DO_TRACE_REAL(name, proto, args)			\
nit: IMO using a __* prefix would be a better option to describe the
internal use of the macro instead of the _REAL suffix.

Other than that, this patch looks good to me:

Reviewed-by: Carlos Llamas <cmllamas@google.com>

Re: [PATCH v4 1/2] rust: add static_key_false

[Gary Guo]

On Fri, 28 Jun 2024 13:23:31 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Add just enough support for static key so that we can use it from
> tracepoints. Tracepoints rely on `static_key_false` even though it is
> deprecated, so we add the same functionality to Rust.
> 
> It is not possible to use the existing C implementation of
> arch_static_branch because it passes the argument `key` to inline
> assembly as an 'i' parameter, so any attempt to add a C helper for this
> function will fail to compile because the value of `key` must be known
> at compile-time.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

Reviewed-by: Gary Guo <gary@garyguo.net>

> ---
>  rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
>  rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
>  rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
>  rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
>  rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                       |  2 ++
>  rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
>  scripts/Makefile.build                   |  2 +-
>  8 files changed, 201 insertions(+), 1 deletion(-)

Re: [PATCH v4 2/2] rust: add tracepoint support

[Gary Guo]

On Fri, 28 Jun 2024 13:23:32 +0000
Alice Ryhl <aliceryhl@google.com> wrote:

> Make it possible to have Rust code call into tracepoints defined by C
> code. It is still required that the tracepoint is declared in a C
> header, and that this header is included in the input to bindgen.
> 
> Signed-off-by: Alice Ryhl <aliceryhl@google.com>

The Rust part LGTM. Some comment about the C part.

> ---
>  include/linux/tracepoint.h      | 18 +++++++++++++++-
>  include/trace/define_trace.h    | 12 +++++++++++
>  rust/bindings/bindings_helper.h |  1 +
>  rust/kernel/lib.rs              |  1 +
>  rust/kernel/tracepoint.rs       | 47 +++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 78 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 689b6d71590e..d82af4d77c9f 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -238,6 +238,20 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  #define __DECLARE_TRACE_RCU(name, proto, args, cond)
>  #endif
>  
> +/*
> + * Declare an exported function that Rust code can call to trigger this
> + * tracepoint. This function does not include the static branch; that is done
> + * in Rust to avoid a function call when the tracepoint is disabled.
> + */
> +#define DEFINE_RUST_DO_TRACE(name, proto, args)
> +#define DEFINE_RUST_DO_TRACE_REAL(name, proto, args)			\
> +	notrace void rust_do_trace_##name(proto)			\
> +	{								\
> +		__DO_TRACE(name,					\
> +			TP_ARGS(args),					\
> +			cpu_online(raw_smp_processor_id()), 0);		\

I guess this doesn't support conditions. Currently the conditions are
specified during declaration and not during definition.

Would it make sense to have

	static inline void do_trace_##name(proto)
	{
		__DO_TRACE(name, TP_ARGS(args), TP_CONDITION(cond), 0);
	}

in `__DECLARE_TRACE` and then simply call it in `rust_do_trace_##name`?

> +	}
> +
>  /*
>   * Make sure the alignment of the structure in the __tracepoints section will
>   * not add unwanted padding between the beginning of the section and the
> @@ -253,6 +267,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  	extern int __traceiter_##name(data_proto);			\
>  	DECLARE_STATIC_CALL(tp_func_##name, __traceiter_##name);	\
>  	extern struct tracepoint __tracepoint_##name;			\
> +	extern void rust_do_trace_##name(proto);			\
>  	static inline void trace_##name(proto)				\
>  	{								\
>  		if (static_key_false(&__tracepoint_##name.key))		\
> @@ -337,7 +352,8 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
>  	void __probestub_##_name(void *__data, proto)			\
>  	{								\
>  	}								\
> -	DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);
> +	DEFINE_STATIC_CALL(tp_func_##_name, __traceiter_##_name);	\
> +	DEFINE_RUST_DO_TRACE(_name, TP_PROTO(proto), TP_ARGS(args))
>  
>  #define DEFINE_TRACE(name, proto, args)		\
>  	DEFINE_TRACE_FN(name, NULL, NULL, PARAMS(proto), PARAMS(args));
> diff --git a/include/trace/define_trace.h b/include/trace/define_trace.h
> index 00723935dcc7..08ed5ce63a96 100644
> --- a/include/trace/define_trace.h
> +++ b/include/trace/define_trace.h
> @@ -72,6 +72,13 @@
>  #define DECLARE_TRACE(name, proto, args)	\
>  	DEFINE_TRACE(name, PARAMS(proto), PARAMS(args))
>  
> +/* If requested, create helpers for calling these tracepoints from Rust. */
> +#ifdef CREATE_RUST_TRACE_POINTS
> +#undef DEFINE_RUST_DO_TRACE
> +#define DEFINE_RUST_DO_TRACE(name, proto, args)	\
> +	DEFINE_RUST_DO_TRACE_REAL(name, PARAMS(proto), PARAMS(args))
> +#endif
> +
>  #undef TRACE_INCLUDE
>  #undef __TRACE_INCLUDE
>  
> @@ -129,6 +136,11 @@
>  # undef UNDEF_TRACE_INCLUDE_PATH
>  #endif
>  
> +#ifdef CREATE_RUST_TRACE_POINTS
> +# undef DEFINE_RUST_DO_TRACE
> +# define DEFINE_RUST_DO_TRACE(name, proto, args)
> +#endif
> +
>  /* We may be processing more files */
>  #define CREATE_TRACE_POINTS
>  

Re: [PATCH v4 2/2] rust: add tracepoint support

[Steven Rostedt]

On Tue, 30 Jul 2024 11:35:27 +0100
Gary Guo <gary@garyguo.net> wrote:

> > +/*
> > + * Declare an exported function that Rust code can call to trigger this
> > + * tracepoint. This function does not include the static branch; that is done
> > + * in Rust to avoid a function call when the tracepoint is disabled.
> > + */
> > +#define DEFINE_RUST_DO_TRACE(name, proto, args)
> > +#define DEFINE_RUST_DO_TRACE_REAL(name, proto, args)			\
> > +	notrace void rust_do_trace_##name(proto)			\
> > +	{								\
> > +		__DO_TRACE(name,					\
> > +			TP_ARGS(args),					\
> > +			cpu_online(raw_smp_processor_id()), 0);		\  
> 
> I guess this doesn't support conditions. Currently the conditions are
> specified during declaration and not during definition.
> 
> Would it make sense to have
> 
> 	static inline void do_trace_##name(proto)
> 	{
> 		__DO_TRACE(name, TP_ARGS(args), TP_CONDITION(cond), 0);

But where is the "cond" passed in from?

I guess in the future if you want to add conditions, you would then just
add:

#define DEFINE_RUST_DO_TRACE_REAL_CONDITION(name, proto, args, cond)			\
	notrace void rust_do_trace_##name(proto)			\
	{								\
		__DO_TRACE(name,					\
			TP_ARGS(args),					\
			cpu_online(raw_smp_processor_id()) &&		\
			TP_CONDITION(cond), 0);				\  

-- Steve

> 	}
> 
> in `__DECLARE_TRACE` and then simply call it in `rust_do_trace_##name`?
> 
> > +	}
> > +
> >  /*

Re: [PATCH v4 1/2] rust: add static_key_false

[Peter Zijlstra]

On Fri, Jun 28, 2024 at 01:23:31PM +0000, Alice Ryhl wrote:

>  rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
>  rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
>  rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
>  rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
>  rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
>  rust/kernel/lib.rs                       |  2 ++
>  rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
>  scripts/Makefile.build                   |  2 +-
>  8 files changed, 201 insertions(+), 1 deletion(-)

So I really find the amount of duplicated asm offensive. Is is far too
easy for any of this to get out of sync.

> diff --git a/rust/kernel/arch/x86/jump_label.rs b/rust/kernel/arch/x86/jump_label.rs
> new file mode 100644
> index 000000000000..383bed273c50
> --- /dev/null
> +++ b/rust/kernel/arch/x86/jump_label.rs
> @@ -0,0 +1,35 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! X86 Rust implementation of jump_label.h
> +
> +/// x86 implementation of arch_static_branch
> +#[macro_export]
> +#[cfg(target_arch = "x86_64")]
> +macro_rules! arch_static_branch {
> +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> +        core::arch::asm!(
> +            r#"
> +            1: .byte 0x0f,0x1f,0x44,0x00,0x00
> +
> +            .pushsection __jump_table,  "aw"
> +            .balign 8
> +            .long 1b - .
> +            .long {0} - .
> +            .quad {1} + {2} + {3} - .
> +            .popsection
> +            "#,
> +            label {
> +                break 'my_label true;
> +            },
> +            sym $key,
> +            const ::core::mem::offset_of!($keytyp, $field),
> +            const $crate::arch::bool_to_int($branch),
> +        );
> +
> +        break 'my_label false;
> +    }};
> +}

Note that this uses the forced 5 byte version, and not the dynamic sized
one. On top of that it hard-codes the nop5 string :/

Please work harder to not have to duplicate stuff like this.

NAK.

Re: [PATCH v4 2/2] rust: add tracepoint support

[Benno Lossin]

On 28.06.24 15:23, Alice Ryhl wrote:
> diff --git a/rust/kernel/tracepoint.rs b/rust/kernel/tracepoint.rs
> new file mode 100644
> index 000000000000..1005f09e0330
> --- /dev/null
> +++ b/rust/kernel/tracepoint.rs
> @@ -0,0 +1,47 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +// Copyright (C) 2024 Google LLC.
> +
> +//! Logic for tracepoints.
> +
> +/// Declare the Rust entry point for a tracepoint.
> +#[macro_export]
> +macro_rules! declare_trace {
> +    ($($(#[$attr:meta])* $pub:vis fn $name:ident($($argname:ident : $argtyp:ty),* $(,)?);)*) => {$(
> +        $( #[$attr] )*
> +        #[inline(always)]
> +        $pub unsafe fn $name($($argname : $argtyp),*) {
> +            #[cfg(CONFIG_TRACEPOINTS)]
> +            {
> +                use $crate::bindings::*;

Why is this needed, can't you put this into the invocation of `paste!`?
ie `[< $crate::bindings::__tracepoint_ $name >]`?

> +
> +                // SAFETY: It's always okay to query the static key for a tracepoint.
> +                let should_trace = unsafe {
> +                    $crate::macros::paste! {
> +                        $crate::static_key::static_key_false!(
> +                            [< __tracepoint_ $name >],
> +                            $crate::bindings::tracepoint,
> +                            key
> +                        )
> +                    }
> +                };
> +
> +                if should_trace {
> +                    $crate::macros::paste! {
> +                        // SAFETY: The caller guarantees that it is okay to call this tracepoint.

Can you add this on the docs of `$name`? ie add a Safety section.
The docs should still appear when creating them/when LSPs show them to
you.

---
Cheers,
Benno

> +                        unsafe { [< rust_do_trace_ $name >]($($argname),*) };
> +                    }
> +                }
> +            }
> +
> +            #[cfg(not(CONFIG_TRACEPOINTS))]
> +            {
> +                // If tracepoints are disabled, insert a trivial use of each argument
> +                // to avoid unused argument warnings.
> +                $( let _unused = $argname; )*
> +            }
> +        }
> +    )*}
> +}
> +
> +pub use declare_trace;
> 
> --
> 2.45.2.803.g4e1b14247a-goog
> 

Re: [PATCH v4 1/2] rust: add static_key_false

[Alice Ryhl]

On Wed, Jul 31, 2024 at 7:05 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jun 28, 2024 at 01:23:31PM +0000, Alice Ryhl wrote:
>
> >  rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
> >  rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
> >  rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
> >  rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
> >  rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs                       |  2 ++
> >  rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
> >  scripts/Makefile.build                   |  2 +-
> >  8 files changed, 201 insertions(+), 1 deletion(-)
>
> So I really find the amount of duplicated asm offensive. Is is far too
> easy for any of this to get out of sync.
>
> > diff --git a/rust/kernel/arch/x86/jump_label.rs b/rust/kernel/arch/x86/jump_label.rs
> > new file mode 100644
> > index 000000000000..383bed273c50
> > --- /dev/null
> > +++ b/rust/kernel/arch/x86/jump_label.rs
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! X86 Rust implementation of jump_label.h
> > +
> > +/// x86 implementation of arch_static_branch
> > +#[macro_export]
> > +#[cfg(target_arch = "x86_64")]
> > +macro_rules! arch_static_branch {
> > +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> > +        core::arch::asm!(
> > +            r#"
> > +            1: .byte 0x0f,0x1f,0x44,0x00,0x00
> > +
> > +            .pushsection __jump_table,  "aw"
> > +            .balign 8
> > +            .long 1b - .
> > +            .long {0} - .
> > +            .quad {1} + {2} + {3} - .
> > +            .popsection
> > +            "#,
> > +            label {
> > +                break 'my_label true;
> > +            },
> > +            sym $key,
> > +            const ::core::mem::offset_of!($keytyp, $field),
> > +            const $crate::arch::bool_to_int($branch),
> > +        );
> > +
> > +        break 'my_label false;
> > +    }};
> > +}
>
> Note that this uses the forced 5 byte version, and not the dynamic sized
> one. On top of that it hard-codes the nop5 string :/
>
> Please work harder to not have to duplicate stuff like this.

I really didn't want to duplicate it, but it's very hard to find a
performant alternative. Is there any way we could accept duplication
only in the cases where an 'i' parameter is used? I don't have the
choice of using a Rust helper for 'i' parameters.

Perhaps one option could be to put the Rust code inside jump_label.h
and have the header file evaluate to either C or Rust depending on the
value of some #ifdefs?

#ifndef RUST_ASM
/* existing C code goes here */
#endif
#ifdef RUST_ASM
// rust code goes here
#endif

That way the duplication is all in a single file. It would also avoid
the need for duplicating the nop5 string, as the Rust case is still
going through the C preprocessor and can use the existing #define.

I'm also open to other alternatives. But I don't have infinite
resources to drive major language changes.

Alice

Re: [PATCH v4 1/2] rust: add static_key_false

[Peter Zijlstra]

On Wed, Jul 31, 2024 at 11:34:13PM +0200, Alice Ryhl wrote:

> > Please work harder to not have to duplicate stuff like this.
> 
> I really didn't want to duplicate it, but it's very hard to find a
> performant alternative. Is there any way we could accept duplication
> only in the cases where an 'i' parameter is used? I don't have the
> choice of using a Rust helper for 'i' parameters.
> 
> Perhaps one option could be to put the Rust code inside jump_label.h
> and have the header file evaluate to either C or Rust depending on the
> value of some #ifdefs?
> 
> #ifndef RUST_ASM
> /* existing C code goes here */
> #endif
> #ifdef RUST_ASM
> // rust code goes here
> #endif
> 
> That way the duplication is all in a single file. It would also avoid
> the need for duplicating the nop5 string, as the Rust case is still
> going through the C preprocessor and can use the existing #define.

I suppose that is slightly better, but ideally you generate the whole of
the Rust thing from the C version. After all, Clang can already parse
this.

That said, with the below patch, I think you should be able to reuse the
JUMP_TABLE_ENTRY macro like:

	JUMP_TABLE_ENTRY({0}, {1}, {2} + {3})

> I'm also open to other alternatives. But I don't have infinite
> resources to drive major language changes.

Yes, well, you all picked this terrible language full well knowing it
hated C/C++ and had not a single effort put into integration with
existing code bases.

I find it very hard to care for the problems you've created for
yourself.


---
diff --git a/arch/x86/include/asm/jump_label.h b/arch/x86/include/asm/jump_label.h
index cbbef32517f0..6cff7bf5e779 100644
--- a/arch/x86/include/asm/jump_label.h
+++ b/arch/x86/include/asm/jump_label.h
@@ -12,12 +12,12 @@
 #include <linux/stringify.h>
 #include <linux/types.h>
 
-#define JUMP_TABLE_ENTRY				\
-	".pushsection __jump_table,  \"aw\" \n\t"	\
-	_ASM_ALIGN "\n\t"				\
-	".long 1b - . \n\t"				\
-	".long %l[l_yes] - . \n\t"			\
-	_ASM_PTR "%c0 + %c1 - .\n\t"			\
+#define JUMP_TABLE_ENTRY(l_yes, key, branch)				\
+	".pushsection __jump_table,  \"aw\" \n\t"			\
+	_ASM_ALIGN "\n\t"						\
+	".long 1b - . \n\t"						\
+	".long " __stringify(l_yes) "- . \n\t"				\
+	_ASM_PTR " " __stringify(key) " + " __stringify(branch) " - . \n\t" \
 	".popsection \n\t"
 
 #ifdef CONFIG_HAVE_JUMP_LABEL_HACK
@@ -26,7 +26,7 @@ static __always_inline bool arch_static_branch(struct static_key *key, bool bran
 {
 	asm goto("1:"
 		"jmp %l[l_yes] # objtool NOPs this \n\t"
-		JUMP_TABLE_ENTRY
+		JUMP_TABLE_ENTRY(%[l_yes], %c0, %c1)
 		: :  "i" (key), "i" (2 | branch) : : l_yes);
 
 	return false;
@@ -40,7 +40,7 @@ static __always_inline bool arch_static_branch(struct static_key * const key, co
 {
 	asm goto("1:"
 		".byte " __stringify(BYTES_NOP5) "\n\t"
-		JUMP_TABLE_ENTRY
+		JUMP_TABLE_ENTRY(%[l_yes], %c0, %c1)
 		: :  "i" (key), "i" (branch) : : l_yes);
 
 	return false;
@@ -54,7 +54,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke
 {
 	asm goto("1:"
 		"jmp %l[l_yes]\n\t"
-		JUMP_TABLE_ENTRY
+		JUMP_TABLE_ENTRY(%[l_yes], %c0, %c1)
 		: :  "i" (key), "i" (branch) : : l_yes);
 
 	return false;

Re: [PATCH v4 1/2] rust: add static_key_false

[Alice Ryhl]

On Thu, Aug 1, 2024 at 12:28 PM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jul 31, 2024 at 11:34:13PM +0200, Alice Ryhl wrote:
>
> > > Please work harder to not have to duplicate stuff like this.
> >
> > I really didn't want to duplicate it, but it's very hard to find a
> > performant alternative. Is there any way we could accept duplication
> > only in the cases where an 'i' parameter is used? I don't have the
> > choice of using a Rust helper for 'i' parameters.
> >
> > Perhaps one option could be to put the Rust code inside jump_label.h
> > and have the header file evaluate to either C or Rust depending on the
> > value of some #ifdefs?
> >
> > #ifndef RUST_ASM
> > /* existing C code goes here */
> > #endif
> > #ifdef RUST_ASM
> > // rust code goes here
> > #endif
> >
> > That way the duplication is all in a single file. It would also avoid
> > the need for duplicating the nop5 string, as the Rust case is still
> > going through the C preprocessor and can use the existing #define.
>
> I suppose that is slightly better, but ideally you generate the whole of
> the Rust thing from the C version. After all, Clang can already parse
> this.
>
> That said, with the below patch, I think you should be able to reuse the
> JUMP_TABLE_ENTRY macro like:
>
>         JUMP_TABLE_ENTRY({0}, {1}, {2} + {3})

Yeah, I think this can work. I will submit a follow-up patch that
removes the duplication soon.

Alice