On Wed, Jul 31, 2024 at 7:05 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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
