| Message ID | 20240819133345.3438739-1-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | [v2] rust: add `module_params` macro | expand |
On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: > > From: Andreas Hindborg <a.hindborg@samsung.com> > > This patch includes changes required for Rust kernel modules to utilize > module parameters. This code implements read only support for integer > types without `sysfs` support. > diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs > new file mode 100644 > index 000000000000..9dfee0311d65 > --- /dev/null > +++ b/rust/kernel/module_param.rs > @@ -0,0 +1,339 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Types for module parameters. > +//! > +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h) > + > +use crate::prelude::*; > + > +/// Types that can be used for module parameters. > +/// > +/// Note that displaying the type in `sysfs` will fail if > +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`] > +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null > +/// terminator). I'm a bit confused where `str::from_utf8` comes into play - maybe just: Note that displaying the type in `sysfs` will fail if the [`Display`] (core::fmt::Display) implementation would write more than [`PAGE_SIZE`] - 1 bytes. > +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE` > +pub trait ModuleParam: core::fmt::Display + core::marker::Sized { I don't think `Sized` should need `core::marker` > + /// The `ModuleParam` will be used by the kernel module through this type. > + /// > + /// This may differ from `Self` if, for example, `Self` needs to track > + /// ownership without exposing it or allocate extra space for other possible > + /// parameter values. This is required to support string parameters in the > + /// future. > + type Value: ?Sized; ^ proof of the above :) "This is required... in the future" could probably be moved to a non-doc comment or just dropped. > +/// Write a string representation of the current parameter value to `buf`. > +/// > +/// # Safety > +/// > +/// Must not be called. > +/// > +/// # Note > +/// > +/// This should not be called as we declare all parameters as read only. > +#[allow(clippy::extra_unused_type_parameters)] > +unsafe extern "C" fn get_param<T>( I think this could make sense being a safe function. I get that calling it would mean something is wrong - but that's a problem of broken invariants elsewhere and not just calling this, no? > +/// Trait for parsing integers. > +/// > +/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or > +/// binary respectively. Strings beginning with `0` otherwise are parsed as > +/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also > +/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be > +/// successfully parsed. > +/// > +/// [`kstrtol()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtol > +/// [`kstrtoul()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtoul > +trait ParseInt: Sized { Since this is standalone functionality, it might be better moved to a different module for reuse (and probably its own patch). I'm not sure about the name either, would something like `FromKStr` with a method `from_kstr` make more sense? Also because the current trait will conflict if `core::str::FromStr` is also in scope. The methods could use docs, and the trait could probably get a doctest and/or some unit tests. > + fn from_str(src: &str) -> Result<Self> { > + match src.bytes().next() { > + None => Err(EINVAL), > + Some(b'-') => Self::from_str_unsigned(&src[1..]) > + .map_err(|_| EINVAL)? > + .checked_neg() > + .ok_or(EINVAL), > + Some(b'+') => Self::from_str_unsigned(&src[1..]).map_err(|_| EINVAL), > + Some(_) => Self::from_str_unsigned(src).map_err(|_| EINVAL), > + } > + } kstrtol returns -ERANGE on overflow, this might need to check the `.kind()` of parse errors to match this > +macro_rules! impl_parse_int { > + ($ty:ident) => { Nit: `$ty` could be a `:ty` here (makes no difference) > +macro_rules! impl_module_param { Nit: maybe `impl_int_module_param` since it doesn't handle other types > +#[doc(hidden)] > +#[macro_export] > +/// Generate a static [`kernel_param_ops`](../../../include/linux/moduleparam.h) struct. > +/// > +/// # Examples > +/// > +/// ```ignore > +/// make_param_ops!( > +/// /// Documentation for new param ops. > +/// PARAM_OPS_MYTYPE, // Name for the static. > +/// MyType // A type which implements [`ModuleParam`]. > +/// ); > +/// ``` Nit: move attributes to after the docs > +macro_rules! make_param_ops { > + ($ops:ident, $ty:ty) => { > + $crate::make_param_ops!( > + #[doc=""] > + $ops, > + $ty > + ); > + }; I don't think you need this first pattern, since the meta in the below pattern is optional. But... > + ($(#[$meta:meta])* $ops:ident, $ty:ty) => { > + $(#[$meta])* ...you could probably just drop the `$meta` and remove docs from the invocation by adding the following here /// Rust implementation of [`kernel_param_ops`] /// (../../../include/linux/moduleparam.h) #[doc(concat!("for [`", stringify!($ty), "`].))] Since the docs are the same except the type > + /// > + /// Static [`kernel_param_ops`](../../../include/linux/moduleparam.h) > + /// struct generated by [`make_param_ops`]. > + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops { > + flags: 0, > + set: Some(set_param::<$ty>), > + get: Some(get_param::<$ty>), > + free: Some(free::<$ty>), > + }; Could this be a const rather than a static? > +impl_module_param!(i8); > +impl_module_param!(u8); > +impl_module_param!(i16); > +impl_module_param!(u16); > +impl_module_param!(i32); > +impl_module_param!(u32); > +impl_module_param!(i64); > +impl_module_param!(u64); > +impl_module_param!(isize); > +impl_module_param!(usize); Nit: these could probably go above `impl_module_param` to be next to their macro definition. > diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs > index 563dcd2b7ace..49388907370d 100644 > --- a/rust/macros/helpers.rs > +++ b/rust/macros/helpers.rs > @@ -107,6 +107,14 @@ pub(crate) struct Generics { > pub(crate) ty_generics: Vec<TokenTree>, > } > > +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String { Just to make it obvious what it is looking for: /// Expect `expected_name: "value",` > diff --git a/rust/macros/module.rs b/rust/macros/module.rs > index 411dc103d82e..2fa9ed8e78ff 100644 > --- a/rust/macros/module.rs > +++ b/rust/macros/module.rs > + fn emit_params(&mut self, info: &ModuleInfo) { > + if let Some(params) = &info.params { Switching to let Some(params) = &info.params else { return; }; would save an indentation level > + for param in params { > + let ops = param_ops_path(¶m.ptype); > + > + self.emit_param("parmtype", ¶m.name, ¶m.ptype); > + self.emit_param("parm", ¶m.name, ¶m.description); > + (Referring to below) could the template string body be indented another level, so it doesn't hang left of the parent `write!` macro? Also - maybe `let pfx = format!("__{name}_{param_name}") here then use `{pfx}` in the template, since that sequence is repeated a lot. > + write!( > + self.param_buffer, > + " > + static mut __{name}_{param_name}_value: {param_type} = {param_default}; Ah.. we need to migrate from `static mut` to `UnsafeCell` wrappers at some point. Since `module!` already uses `static mut`, this may need to happen separately - meaning I don't think we need to block on making any change here. This would mean adding an `UnsafeSyncCell` / `RacyCell` (just a wrapper around `UnsafeCell` that always implements `Sync`), using `UnsafeSyncCell<{param_type}>` as the type here, and changing from `static mut` to just `static`. (I can take a look at doing this change for existing `static mut` in the near future). > + /// Newtype to make `bindings::kernel_param` `Sync`. > + #[repr(transparent)] > + struct __{name}_{param_name}_RacyKernelParam(::kernel::bindings::kernel_param); > + > + // SAFETY: C kernel handles serializing access to this type. We > + // never access from Rust module. > + unsafe impl Sync for __{name}_{param_name}_RacyKernelParam {{ }} Could there just be a type for this in the kernel crate rather than creating one as part of the macro? > + #[cfg(not(MODULE))] > + const __{name}_{param_name}_name: *const ::core::ffi::c_char = > + b\"{name}.{param_name}\\0\" as *const _ as *const ::core::ffi::c_char; This should work as `c\"{name}.{param_name}\".as_ptr()` now > + > + #[cfg(MODULE)] > + const __{name}_{param_name}_name: *const ::core::ffi::c_char = > + b\"{param_name}\\0\" as *const _ as *const ::core::ffi::c_char; Same as above. > + #[link_section = \"__param\"] > + #[used] > + static __{name}_{param_name}_struct: __{name}_{param_name}_RacyKernelParam = > + __{name}_{param_name}_RacyKernelParam(::kernel::bindings::kernel_param {{ > + name: __{name}_{param_name}_name, You could eliminate the above `__{name}_{param_name}_name` consts by using: name: if cfg!(MODULE) { c\"{param_name}\\" } else { c\"{name}.{param_name}\\" }.as_ptr(), > + // SAFETY: `__this_module` is constructed by the kernel at load time > + // and will not be freed until the module is unloaded. > + #[cfg(MODULE)] > + mod_: unsafe {{ &::kernel::bindings::__this_module as *const _ as *mut _ }}, Prefer `.cast::<T>().cast_mut()` > + #[cfg(not(MODULE))] > + mod_: ::core::ptr::null_mut(), > + ops: &{ops} as *const ::kernel::bindings::kernel_param_ops, > + perm: 0, // Will not appear in sysfs For my knowledge, what would be needed to make sysfs work? > + level: -1, > + flags: 0, > + __bindgen_anon_1: > + ::kernel::bindings::kernel_param__bindgen_ty_1 {{ > + // SAFETY: As this is evaluated in const context, it is > + // safe to take a reference to a mut static. > + arg: unsafe {{ > + ::core::ptr::addr_of_mut!(__{name}_{param_name}_value) > + }}.cast::<::core::ffi::c_void>(), A note on this toward the end > + }}, > + }}); > + ", > @@ -216,12 +387,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { Is a lot of below just some rewrapping and leading `::`? It may be good to split off tweaks to the existing code > // SAFETY: `__this_module` is constructed by the kernel at load time and will not be > // freed until the module is unloaded. > #[cfg(MODULE)] > - static THIS_MODULE: kernel::ThisModule = unsafe {{ > - kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _) > + static THIS_MODULE: ::kernel::ThisModule = unsafe {{ > + ::kernel::ThisModule::from_ptr( > + &::kernel::bindings::__this_module as *const _ as *mut _ > + ) > }}; > #[cfg(not(MODULE))] > - static THIS_MODULE: kernel::ThisModule = unsafe {{ > - kernel::ThisModule::from_ptr(core::ptr::null_mut()) > + static THIS_MODULE: ::kernel::ThisModule = unsafe {{ > + ::kernel::ThisModule::from_ptr(::core::ptr::null_mut()) > }}; > > // Double nested modules, since then nobody can access the public items inside. > @@ -276,7 +449,8 @@ mod __module_init {{ > #[doc(hidden)] > #[link_section = \"{initcall_section}\"] > #[used] > - pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init; > + pub static __{name}_initcall: extern \"C\" fn() > + -> ::core::ffi::c_int = __{name}_init; > > #[cfg(not(MODULE))] > #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)] > @@ -291,7 +465,7 @@ mod __module_init {{ > #[cfg(not(MODULE))] > #[doc(hidden)] > #[no_mangle] > - pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{ > + pub extern \"C\" fn __{name}_init() -> ::core::ffi::c_int {{ > // SAFETY: This function is inaccessible to the outside due to the double > // module wrapping it. It is called exactly once by the C side via its > // placement above in the initcall section. > @@ -314,8 +488,8 @@ mod __module_init {{ > /// # Safety > /// > /// This function must only be called once. > - unsafe fn __init() -> core::ffi::c_int {{ > - match <{type_} as kernel::Module>::init(&super::super::THIS_MODULE) {{ > + unsafe fn __init() -> ::core::ffi::c_int {{ > + match <{type_} as ::kernel::Module>::init(&super::super::THIS_MODULE) {{ > Ok(m) => {{ > // SAFETY: No data race, since `__MOD` can only be accessed by this > // module and there only `__init` and `__exit` access it. These > @@ -346,14 +520,17 @@ unsafe fn __exit() {{ > __MOD = None; > }} > }} > - > {modinfo} > }} > }} > + mod module_parameters {{ > + {params} > + }} > ", > type_ = info.type_, > name = info.name, > modinfo = modinfo.buffer, > + params = modinfo.param_buffer, > initcall_section = ".initcall6.init" > ) > .parse() > diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs > index 2a9eaab62d1c..d9bc2218d504 100644 > --- a/samples/rust/rust_minimal.rs > +++ b/samples/rust/rust_minimal.rs > @@ -10,6 +10,12 @@ > author: "Rust for Linux Contributors", > description: "Rust minimal sample", > license: "GPL", > + params: { > + test_parameter: i64 { > + default: 1, > + description: "This parameter has a default of 1", > + }, > + }, > } I was going to suggest updating the sample then saw you did that already, thanks :) > diff --git a/scripts/Makefile.build b/scripts/Makefile.build > index efacca63c897..a65bd0233843 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 := new_uninit,const_mut_refs We shouldn't enable `const_mut_refs`. It is indeed close to stabilization, but it is still kind of churny right now and we don't want to enable the sharp edges everywhere. If the change from `static mut` to `UnsafeCell` that I mentioned above happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the place of `addr_of_mut!` and doesn't require this feature (and also isn't unsafe). If you prefer not to make that change, I think `addr_of!(...).cast_mut()` might be the best solution. --- Other thought: would a wrapper type make more sense here? Something like this: ``` /* implementation */ struct ModParam<T>(UnsafeCell<T>); // `Parameter` is the existing `ModParameter` (could be // any name). It could be sealed. impl<T: Parameter> ModParam<T> { #[doc(hidden)] // used in the macro fn new(value: T) -> Self { ... } fn get(&self) -> T::Value { ... } fn set(&self, v: T::Value) { ... } } ``` With usage: ``` module! { // ... // instantiated as: // `static MY_PARAM: ModParam<i64> = ModParam::new(1);` MY_PARAM: i64 { default: 1, description: "foo", }, } fn foo() { pr_info!("My param is {}", MY_PARAM.get()); } ``` Advantages I see: - You bring your own name, rather than being scoped and needing to remember the module name - You can check `ModParam` in the docs to see the API, rather than needing to refer to source for the exact signatures of `read` and `write` - The interface gets a bit more like a mutex or atomic - Trevor
On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: > > From: Andreas Hindborg <a.hindborg@samsung.com> > > This patch includes changes required for Rust kernel modules to utilize > module parameters. This code implements read only support for integer > types without `sysfs` support. Also, I think the subject line needs an update ("rust: add `module_params` macro")
On 24.08.24 13:27, Trevor Gross wrote: > On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: >> From: Andreas Hindborg <a.hindborg@samsung.com> >> + write!( >> + self.param_buffer, >> + " >> + static mut __{name}_{param_name}_value: {param_type} = {param_default}; > > Ah.. we need to migrate from `static mut` to `UnsafeCell` wrappers at > some point. Since `module!` already uses `static mut`, this may need IIRC Alice wanted to do something about that. > to happen separately - meaning I don't think we need to block on > making any change here. > > This would mean adding an `UnsafeSyncCell` / `RacyCell` (just a > wrapper around `UnsafeCell` that always implements `Sync`), using > `UnsafeSyncCell<{param_type}>` as the type here, and changing from > `static mut` to just `static`. > > (I can take a look at doing this change for existing `static mut` in > the near future). > >> diff --git a/scripts/Makefile.build b/scripts/Makefile.build >> index efacca63c897..a65bd0233843 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 := new_uninit,const_mut_refs > > We shouldn't enable `const_mut_refs`. It is indeed close to > stabilization, but it is still kind of churny right now and we don't > want to enable the sharp edges everywhere. > > If the change from `static mut` to `UnsafeCell` that I mentioned above > happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the > place of `addr_of_mut!` and doesn't require this feature (and also > isn't unsafe). I think this is a good idea. There might only be a problem with not being `Sync` though... So probably need to use `SyncUnsafeCell` instead. > If you prefer not to make that change, I think > `addr_of!(...).cast_mut()` might be the best solution. Won't that be resulting in the wrong provenance? I.e. the pointer won't be allowed to write to that location? I just checked with miri, it doesn't complain (even with `strict-provenance`), so I guess it's fine? It feels rather wrong to me to allow writing through a pointer obtained via `addr_of!`. > --- > > Other thought: would a wrapper type make more sense here? Something like this: > > ``` > /* implementation */ > struct ModParam<T>(UnsafeCell<T>); > > // `Parameter` is the existing `ModParameter` (could be > // any name). It could be sealed. > impl<T: Parameter> ModParam<T> { > #[doc(hidden)] // used in the macro > fn new(value: T) -> Self { ... } > > fn get(&self) -> T::Value { ... } > fn set(&self, v: T::Value) { ... } > } > ``` > > With usage: > > ``` > module! { > // ... > // instantiated as: > // `static MY_PARAM: ModParam<i64> = ModParam::new(1);` We used to do this, but it lead to problems: normally the parameter has a lower case name, since that's the convention in the kernel. But then pattern matching prioritises the static instead of introducing it as a local parameter: let MY_PARAM = ...; would fail, since you can't match MY_PARAM. This is also the reason why they live in their own module. But you can still do the modification of creating `ModParam` and using that as the type of the static. --- Cheers, Benno > MY_PARAM: i64 { > default: 1, > description: "foo", > }, > } > > fn foo() { > pr_info!("My param is {}", MY_PARAM.get()); > } > ``` > > Advantages I see: > - You bring your own name, rather than being scoped and needing to > remember the module name > - You can check `ModParam` in the docs to see the API, rather than > needing to refer to source for the exact signatures of `read` and > `write` > - The interface gets a bit more like a mutex or atomic > > - Trevor
On Sat, Aug 24, 2024 at 8:16 AM Benno Lossin <benno.lossin@proton.me> wrote: > > We shouldn't enable `const_mut_refs`. It is indeed close to > > stabilization, but it is still kind of churny right now and we don't > > want to enable the sharp edges everywhere. > > > > If the change from `static mut` to `UnsafeCell` that I mentioned above > > happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the > > place of `addr_of_mut!` and doesn't require this feature (and also > > isn't unsafe). > > I think this is a good idea. There might only be a problem with not > being `Sync` though... So probably need to use `SyncUnsafeCell` instead. Ah whoops, yeah that is what I meant. > > If you prefer not to make that change, I think > > `addr_of!(...).cast_mut()` might be the best solution. > > Won't that be resulting in the wrong provenance? I.e. the pointer won't > be allowed to write to that location? > > I just checked with miri, it doesn't complain (even with > `strict-provenance`), so I guess it's fine? It feels rather wrong to me > to allow writing through a pointer obtained via `addr_of!`. I think that `static mut` gets the interior mutability rules that `UnsafeCell` has, that *const and *mut become interchangeable. Quick demo for the `UnsafeCell` at [1]. We would probably have to ask opsem to clarify. Coincidentally I had been talking to Ralf about this very pattern before seeing this, at [2]. > > Other thought: would a wrapper type make more sense here? Something like this: > > > > ``` > > /* implementation */ > > struct ModParam<T>(UnsafeCell<T>); > > [...] > > module! { > > // ... > > // instantiated as: > > // `static MY_PARAM: ModParam<i64> = ModParam::new(1);` > > We used to do this, but it lead to problems: normally the parameter has > a lower case name, since that's the convention in the kernel. [...] To me it seemed logical to keep the uppercase names here since it matches the convention for what they are (statics), and the macro could lowercase it for the bits exposed to the kernel. But I absolutely get the consistency argument here. > [...] But then > pattern matching prioritises the static instead of introducing it as a > local parameter: > > let MY_PARAM = ...; > > would fail, since you can't match MY_PARAM. > > This is also the reason why they live in their own module. I'm not sure I follow the example here. It looks like it is shadowing a static's name as a local, why would you want that? Or if you meant the other way around `let SomePat(...) = MY_PARAM`, wouldn't it just be `let SomePat(...) = MY_PARAM.get()`? (Sorry if I missed some context here). > But you can still do the modification of creating `ModParam` and using > that as the type of the static. Do you mean an expansion like this? // module_parameters is kind of a long name mod mod_params { #[allow(non_upper_case_globals)] static my_param: ModParam<i32> = ModParam::new(...); } I don't mind that, even if the name is a bit weird by rust conventions. (For what it's worth, I used this wrapper type pattern for a plugin project that does shared variables in a similar way. I have been quite happy with it.) - Trevor [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43664620f50384b7a3d5bf74ce7c3e39 [2]: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/More.20accidental.20stabilization
On 24.08.24 23:23, Trevor Gross wrote: > On Sat, Aug 24, 2024 at 8:16 AM Benno Lossin <benno.lossin@proton.me> wrote: >>> We shouldn't enable `const_mut_refs`. It is indeed close to >>> stabilization, but it is still kind of churny right now and we don't >>> want to enable the sharp edges everywhere. >>> >>> If the change from `static mut` to `UnsafeCell` that I mentioned above >>> happens, `addr_of_mut!` turns into a `.get().cast::<...>()` takes the >>> place of `addr_of_mut!` and doesn't require this feature (and also >>> isn't unsafe). >> >> I think this is a good idea. There might only be a problem with not >> being `Sync` though... So probably need to use `SyncUnsafeCell` instead. > > Ah whoops, yeah that is what I meant. > >>> If you prefer not to make that change, I think >>> `addr_of!(...).cast_mut()` might be the best solution. >> >> Won't that be resulting in the wrong provenance? I.e. the pointer won't >> be allowed to write to that location? >> >> I just checked with miri, it doesn't complain (even with >> `strict-provenance`), so I guess it's fine? It feels rather wrong to me >> to allow writing through a pointer obtained via `addr_of!`. > > I think that `static mut` gets the interior mutability rules that > `UnsafeCell` has, that *const and *mut become interchangeable. Quick > demo for the `UnsafeCell` at [1]. We would probably have to ask opsem > to clarify. I see. I suggested to Andreas to change it to the current version, seems like I was wrong on this... I feel like the `SyncUnsafeCell` version is still better, since we can avoid a `static mut`. > Coincidentally I had been talking to Ralf about this very pattern > before seeing this, at [2]. > >>> Other thought: would a wrapper type make more sense here? Something like this: >>> >>> ``` >>> /* implementation */ >>> struct ModParam<T>(UnsafeCell<T>); >>> [...] >>> module! { >>> // ... >>> // instantiated as: >>> // `static MY_PARAM: ModParam<i64> = ModParam::new(1);` >> >> We used to do this, but it lead to problems: normally the parameter has >> a lower case name, since that's the convention in the kernel. [...] > > To me it seemed logical to keep the uppercase names here since it > matches the convention for what they are (statics), and the macro > could lowercase it for the bits exposed to the kernel. But I > absolutely get the consistency argument here. That would also be an option, but it might be confusing for people, they enter a SCREAMING_CASE_SNAKE name, but when they start the kernel it doesn't work. >> [...] But then >> pattern matching prioritises the static instead of introducing it as a >> local parameter: >> >> let MY_PARAM = ...; >> >> would fail, since you can't match MY_PARAM. >> >> This is also the reason why they live in their own module. > > I'm not sure I follow the example here. It looks like it is shadowing > a static's name as a local, why would you want that? Or if you meant > the other way around `let SomePat(...) = MY_PARAM`, wouldn't it just > be `let SomePat(...) = MY_PARAM.get()`? (Sorry if I missed some > context here). Yeah I expressed this poorly, the problem before was that you would write: module! { /* ... */ params: { foo: i64 { default: 1, description: "foo", } } } pub struct MyDriver { foo: i64, } impl Module for MyDriver { fn init(_: &'static ThisModule) -> Result<Self> { let foo = foo.read(); // ^^^ cannot be named the same as a static Ok(Self { foo }) } } >> But you can still do the modification of creating `ModParam` and using >> that as the type of the static. > > Do you mean an expansion like this? > > // module_parameters is kind of a long name > mod mod_params { > #[allow(non_upper_case_globals)] > static my_param: ModParam<i32> = ModParam::new(...); > } Yes that's what I meant, although `my_param` should be `pub(crate)`. > I don't mind that, even if the name is a bit weird by rust conventions. Yeah, but I think since they are in their own module, it's fine. One thing that we need to decide would be if we want mod_params::my_param::read() or mod_params::my_param.read() I slightly prefer the first one, which would mean that the expansion would have to be: mod mod_params { pub(crate) enum my_param {} static my_param_value: ModParam<i32> = ModParam::new(...); impl my_param { pub(crate) fn read() { my_param_value.read() } /* ... */ } } > (For what it's worth, I used this wrapper type pattern for a plugin > project that does shared variables in a similar way. I have been quite > happy with it.) Good to know! --- Cheers, Benno > > - Trevor > > [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=43664620f50384b7a3d5bf74ce7c3e39 > [2]: https://rust-lang.zulipchat.com/#narrow/stream/146212-t-compiler.2Fconst-eval/topic/More.20accidental.20stabilization
"Trevor Gross" <tmgross@umich.edu> writes: > On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: >> >> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> This patch includes changes required for Rust kernel modules to utilize >> module parameters. This code implements read only support for integer >> types without `sysfs` support. > > Also, I think the subject line needs an update ("rust: add > `module_params` macro") Well, it is still what it does. Plus few support types. You think it is not descriptive enough? BR Andreas
On Tue, Aug 27, 2024 at 10:00 AM Andreas Hindborg <nmi@metaspace.dk> wrote: > > "Trevor Gross" <tmgross@umich.edu> writes: > > > On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: > >> > >> From: Andreas Hindborg <a.hindborg@samsung.com> > >> > >> This patch includes changes required for Rust kernel modules to utilize > >> module parameters. This code implements read only support for integer > >> types without `sysfs` support. > > > > Also, I think the subject line needs an update ("rust: add > > `module_params` macro") > > Well, it is still what it does. Plus few support types. You think it is > not descriptive enough? Maybe it should just say 'Add parameter support to the `module!` macro'? The text `module_params` doesn't seem to appear in the patch, I was looking for something like `module_params!`. - Trevor > BR Andreas
"Trevor Gross" <tmgross@umich.edu> writes: > On Tue, Aug 27, 2024 at 10:00 AM Andreas Hindborg <nmi@metaspace.dk> wrote: >> >> "Trevor Gross" <tmgross@umich.edu> writes: >> >> > On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: >> >> >> >> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> >> >> This patch includes changes required for Rust kernel modules to utilize >> >> module parameters. This code implements read only support for integer >> >> types without `sysfs` support. >> > >> > Also, I think the subject line needs an update ("rust: add >> > `module_params` macro") >> >> Well, it is still what it does. Plus few support types. You think it is >> not descriptive enough? > > Maybe it should just say 'Add parameter support to the `module!` > macro'? The text `module_params` doesn't seem to appear in the patch, > I was looking for something like `module_params!`. Right, I'll change it. BR Andreas
Hi Trevor, Thanks a lot for the thorough review! "Trevor Gross" <tmgross@umich.edu> writes: > On Mon, Aug 19, 2024 at 8:35 AM Andreas Hindborg <nmi@metaspace.dk> wrote: >> >> From: Andreas Hindborg <a.hindborg@samsung.com> >> >> This patch includes changes required for Rust kernel modules to utilize >> module parameters. This code implements read only support for integer >> types without `sysfs` support. > >> diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs >> new file mode 100644 >> index 000000000000..9dfee0311d65 >> --- /dev/null >> +++ b/rust/kernel/module_param.rs >> @@ -0,0 +1,339 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! Types for module parameters. >> +//! >> +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h) >> + >> +use crate::prelude::*; >> + >> +/// Types that can be used for module parameters. >> +/// >> +/// Note that displaying the type in `sysfs` will fail if >> +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`] >> +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null >> +/// terminator). > > I'm a bit confused where `str::from_utf8` comes into play - maybe just: > > Note that displaying the type in `sysfs` will fail if the [`Display`] > (core::fmt::Display) implementation would write more than > [`PAGE_SIZE`] - 1 bytes. I think it is a hint for where to look if things go wrong for `core` types. But it makes sense to cut it to cut `Display`. >> +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE` >> +pub trait ModuleParam: core::fmt::Display + core::marker::Sized { > > I don't think `Sized` should need `core::marker` Thanks. > >> + /// The `ModuleParam` will be used by the kernel module through this type. >> + /// >> + /// This may differ from `Self` if, for example, `Self` needs to track >> + /// ownership without exposing it or allocate extra space for other possible >> + /// parameter values. This is required to support string parameters in the >> + /// future. >> + type Value: ?Sized; > > ^ proof of the above :) > > "This is required... in the future" could probably be moved to a > non-doc comment or just dropped. OK. > >> +/// Write a string representation of the current parameter value to `buf`. >> +/// >> +/// # Safety >> +/// >> +/// Must not be called. >> +/// >> +/// # Note >> +/// >> +/// This should not be called as we declare all parameters as read only. >> +#[allow(clippy::extra_unused_type_parameters)] >> +unsafe extern "C" fn get_param<T>( > > I think this could make sense being a safe function. I get that > calling it would mean something is wrong - but that's a problem of > broken invariants elsewhere and not just calling this, no? The "must not be called" requirement will be dropped when we support parameters writable through sysfs. Once we allow it to be called, it is going to have safety requirements and it will have to be unsafe. So I see no point it not making it unsafe now. > >> +/// Trait for parsing integers. >> +/// >> +/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or >> +/// binary respectively. Strings beginning with `0` otherwise are parsed as >> +/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also >> +/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be >> +/// successfully parsed. >> +/// >> +/// [`kstrtol()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtol >> +/// [`kstrtoul()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtoul >> +trait ParseInt: Sized { > > Since this is standalone functionality, it might be better moved to a > different module for reuse (and probably its own patch). > > I'm not sure about the name either, would something like `FromKStr` > with a method `from_kstr` make more sense? Also because the current > trait will conflict if `core::str::FromStr` is also in scope. I see, that makes sense. I can rename it and move it to a separate patch and module. > > The methods could use docs, and the trait could probably get a doctest > and/or some unit tests. I will add that. > >> + fn from_str(src: &str) -> Result<Self> { >> + match src.bytes().next() { >> + None => Err(EINVAL), >> + Some(b'-') => Self::from_str_unsigned(&src[1..]) >> + .map_err(|_| EINVAL)? >> + .checked_neg() >> + .ok_or(EINVAL), >> + Some(b'+') => Self::from_str_unsigned(&src[1..]).map_err(|_| EINVAL), >> + Some(_) => Self::from_str_unsigned(src).map_err(|_| EINVAL), >> + } >> + } > > kstrtol returns -ERANGE on overflow, this might need to check the > `.kind()` of parse errors to match this Good idea. > >> +macro_rules! impl_parse_int { >> + ($ty:ident) => { > > Nit: `$ty` could be a `:ty` here (makes no difference)
diff --git a/rust/kernel/error.rs b/rust/kernel/error.rs index 145f5c397009..8532a09947d4 100644 --- a/rust/kernel/error.rs +++ b/rust/kernel/error.rs @@ -312,8 +312,6 @@ pub(crate) fn from_err_ptr<T>(ptr: *mut T) -> Result<*mut T> { /// }) /// } /// ``` -// TODO: Remove `dead_code` marker once an in-kernel client is available. -#[allow(dead_code)] pub(crate) fn from_result<T, F>(f: F) -> T where T: From<i16>, diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 274bdc1b0a82..2840237eb73b 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -38,6 +38,7 @@ pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit; +pub mod module_param; #[cfg(CONFIG_NET)] pub mod net; pub mod page; diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs new file mode 100644 index 000000000000..9dfee0311d65 --- /dev/null +++ b/rust/kernel/module_param.rs @@ -0,0 +1,339 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Types for module parameters. +//! +//! C header: [`include/linux/moduleparam.h`](../../../include/linux/moduleparam.h) + +use crate::prelude::*; + +/// Types that can be used for module parameters. +/// +/// Note that displaying the type in `sysfs` will fail if +/// [`core::str::from_utf8`] (as implemented through the [`core::fmt::Display`] +/// trait) writes more than [`PAGE_SIZE`] bytes (including an additional null +/// terminator). +/// +/// [`PAGE_SIZE`]: `bindings::PAGE_SIZE` +pub trait ModuleParam: core::fmt::Display + core::marker::Sized { + /// The `ModuleParam` will be used by the kernel module through this type. + /// + /// This may differ from `Self` if, for example, `Self` needs to track + /// ownership without exposing it or allocate extra space for other possible + /// parameter values. This is required to support string parameters in the + /// future. + type Value: ?Sized; + + /// Parse a parameter argument into the parameter value. + /// + /// `Err(_)` should be returned when parsing of the argument fails. + /// + /// Parameters passed at boot time will be set before [`kmalloc`] is + /// available (even if the module is loaded at a later time). However, in + /// this case, the argument buffer will be valid for the entire lifetime of + /// the kernel. So implementations of this method which need to allocate + /// should first check that the allocator is available (with + /// [`crate::bindings::slab_is_available`]) and when it is not available + /// provide an alternative implementation which doesn't allocate. In cases + /// where the allocator is not available it is safe to save references to + /// `arg` in `Self`, but in other cases a copy should be made. + /// + /// [`kmalloc`]: ../../../include/linux/slab.h + fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; + + /// Get the current value of the parameter for use in the kernel module. + /// + /// This function should not be used directly. Instead use the wrapper + /// `read` which will be generated by [`macros::module`]. + fn value(&self) -> &Self::Value; +} + +/// Set the module parameter from a string. +/// +/// Used to set the parameter value at kernel initialization, when loading +/// the module or when set through `sysfs`. +/// +/// `param.arg` is a pointer to `*mut T` as set up by the [`module!`] +/// macro. +/// +/// See `struct kernel_param_ops.set`. +/// +/// # Safety +/// +/// If `val` is non-null then it must point to a valid null-terminated +/// string. The `arg` field of `param` must be an instance of `T`. +/// +/// # Invariants +/// +/// Currently, we only support read-only parameters that are not readable +/// from `sysfs`. Thus, this function is only called at kernel +/// initialization time, or at module load time, and we have exclusive +/// access to the parameter for the duration of the function. +/// +/// [`module!`]: macros::module +unsafe extern "C" fn set_param<T>( + val: *const core::ffi::c_char, + param: *const crate::bindings::kernel_param, +) -> core::ffi::c_int +where + T: ModuleParam, +{ + // NOTE: If we start supporting arguments without values, val _is_ allowed + // to be null here. + assert!(!val.is_null()); + + // SAFETY: By function safety requirement, val is non-null and + // null-terminated. By C API contract, `val` is live and valid for reads + // for the duration of this function. + let arg = unsafe { CStr::from_char_ptr(val).as_bytes() }; + + crate::error::from_result(|| { + let new_value = T::try_from_param_arg(arg)?; + + // SAFETY: `param` is guaranteed to be valid by C API contract + // and `arg` is guaranteed to point to an instance of `T`. + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut T }; + + // SAFETY: `old_value` is valid for writes, as we have exclusive + // access. `old_value` is pointing to an initialized static, an + // so it is properly initialized. + unsafe { core::ptr::replace(old_value, new_value) }; + Ok(0) + }) +} + +/// Write a string representation of the current parameter value to `buf`. +/// +/// # Safety +/// +/// Must not be called. +/// +/// # Note +/// +/// This should not be called as we declare all parameters as read only. +#[allow(clippy::extra_unused_type_parameters)] +unsafe extern "C" fn get_param<T>( + _buf: *mut core::ffi::c_char, + _param: *const crate::bindings::kernel_param, +) -> core::ffi::c_int +where + T: ModuleParam, +{ + unreachable!("Parameters are not readable"); +} + +/// Drop the parameter. +/// +/// Called when unloading a module. +/// +/// # Safety +/// +/// The `arg` field of `param` must be an initialized instance of `Self`. +unsafe extern "C" fn free<T>(arg: *mut core::ffi::c_void) +where + T: ModuleParam, +{ + // SAFETY: By function safety requirement, `arg` is an initialized + // instance of `T`. By C API contract, `arg` will not be used after + // this function returns. + unsafe { core::ptr::drop_in_place(arg as *mut T) }; +} + +/// Trait for parsing integers. +/// +/// Strings beginning with `0x`, `0o`, or `0b` are parsed as hex, octal, or +/// binary respectively. Strings beginning with `0` otherwise are parsed as +/// octal. Anything else is parsed as decimal. A leading `+` or `-` is also +/// permitted. Any string parsed by [`kstrtol()`] or [`kstrtoul()`] will be +/// successfully parsed. +/// +/// [`kstrtol()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtol +/// [`kstrtoul()`]: https://www.kernel.org/doc/html/latest/core-api/kernel-api.html#c.kstrtoul +trait ParseInt: Sized { + fn from_str_radix(src: &str, radix: u32) -> Result<Self, core::num::ParseIntError>; + + // NOTE: Required because `checked_neg` is not provided by any trait. + fn checked_neg(self) -> Option<Self>; + + fn from_str_unsigned(src: &str) -> Result<Self, core::num::ParseIntError> { + let (radix, digits) = if let Some(n) = src.strip_prefix("0x") { + (16, n) + } else if let Some(n) = src.strip_prefix("0X") { + (16, n) + } else if let Some(n) = src.strip_prefix("0o") { + (8, n) + } else if let Some(n) = src.strip_prefix("0O") { + (8, n) + } else if let Some(n) = src.strip_prefix("0b") { + (2, n) + } else if let Some(n) = src.strip_prefix("0B") { + (2, n) + } else if src.starts_with('0') { + (8, src) + } else { + (10, src) + }; + Self::from_str_radix(digits, radix) + } + + fn from_str(src: &str) -> Result<Self> { + match src.bytes().next() { + None => Err(EINVAL), + Some(b'-') => Self::from_str_unsigned(&src[1..]) + .map_err(|_| EINVAL)? + .checked_neg() + .ok_or(EINVAL), + Some(b'+') => Self::from_str_unsigned(&src[1..]).map_err(|_| EINVAL), + Some(_) => Self::from_str_unsigned(src).map_err(|_| EINVAL), + } + } +} + +macro_rules! impl_parse_int { + ($ty:ident) => { + impl ParseInt for $ty { + fn from_str_radix(src: &str, radix: u32) -> Result<Self, core::num::ParseIntError> { + $ty::from_str_radix(src, radix) + } + + fn checked_neg(self) -> Option<Self> { + self.checked_neg() + } + } + }; +} + +impl_parse_int!(i8); +impl_parse_int!(u8); +impl_parse_int!(i16); +impl_parse_int!(u16); +impl_parse_int!(i32); +impl_parse_int!(u32); +impl_parse_int!(i64); +impl_parse_int!(u64); +impl_parse_int!(isize); +impl_parse_int!(usize); + +macro_rules! impl_module_param { + ($ty:ident) => { + impl ModuleParam for $ty { + type Value = $ty; + + fn try_from_param_arg(arg: &'static [u8]) -> Result<Self> { + let utf8 = core::str::from_utf8(arg)?; + <$ty as crate::module_param::ParseInt>::from_str(utf8) + } + + #[inline(always)] + fn value(&self) -> &Self::Value { + self + } + } + }; +} + +#[doc(hidden)] +#[macro_export] +/// Generate a static [`kernel_param_ops`](../../../include/linux/moduleparam.h) struct. +/// +/// # Examples +/// +/// ```ignore +/// make_param_ops!( +/// /// Documentation for new param ops. +/// PARAM_OPS_MYTYPE, // Name for the static. +/// MyType // A type which implements [`ModuleParam`]. +/// ); +/// ``` +macro_rules! make_param_ops { + ($ops:ident, $ty:ty) => { + $crate::make_param_ops!( + #[doc=""] + $ops, + $ty + ); + }; + ($(#[$meta:meta])* $ops:ident, $ty:ty) => { + $(#[$meta])* + /// + /// Static [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// struct generated by [`make_param_ops`]. + pub static $ops: $crate::bindings::kernel_param_ops = $crate::bindings::kernel_param_ops { + flags: 0, + set: Some(set_param::<$ty>), + get: Some(get_param::<$ty>), + free: Some(free::<$ty>), + }; + }; +} + +impl_module_param!(i8); +impl_module_param!(u8); +impl_module_param!(i16); +impl_module_param!(u16); +impl_module_param!(i32); +impl_module_param!(u32); +impl_module_param!(i64); +impl_module_param!(u64); +impl_module_param!(isize); +impl_module_param!(usize); + +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`i8`]. + PARAM_OPS_I8, + i8 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`u8`]. + PARAM_OPS_U8, + u8 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`i16`]. + PARAM_OPS_I16, + i16 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`u16`]. + PARAM_OPS_U16, + u16 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`i32`]. + PARAM_OPS_I32, + i32 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`u32`]. + PARAM_OPS_U32, + u32 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`i64`]. + PARAM_OPS_I64, + i64 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`u64`]. + PARAM_OPS_U64, + u64 +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`isize`]. + PARAM_OPS_ISIZE, + isize +); +make_param_ops!( + /// Rust implementation of [`kernel_param_ops`](../../../include/linux/moduleparam.h) + /// for [`usize`]. + PARAM_OPS_USIZE, + usize +); diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs index 563dcd2b7ace..49388907370d 100644 --- a/rust/macros/helpers.rs +++ b/rust/macros/helpers.rs @@ -107,6 +107,14 @@ pub(crate) struct Generics { pub(crate) ty_generics: Vec<TokenTree>, } +pub(crate) fn expect_string_field(it: &mut token_stream::IntoIter, expected_name: &str) -> String { + assert_eq!(expect_ident(it), expected_name); + assert_eq!(expect_punct(it), ':'); + let string = expect_string(it); + assert_eq!(expect_punct(it), ','); + string +} + /// Parses the given `TokenStream` into `Generics` and the rest. /// /// The generics are not present in the rest, but a where clause might remain. diff --git a/rust/macros/lib.rs b/rust/macros/lib.rs index 159e75292970..1d7bc99ec5e0 100644 --- a/rust/macros/lib.rs +++ b/rust/macros/lib.rs @@ -20,6 +20,30 @@ /// The `type` argument should be a type which implements the [`Module`] /// trait. Also accepts various forms of kernel metadata. /// +/// The `params` field describe module parameters. Each entry has the form +/// +/// ```ignore +/// parameter_name: type { +/// default: default_value, +/// description: "Description", +/// } +/// ``` +/// +/// `type` may be one of +/// +/// - `i8` +/// - `u8` +/// - `i8` +/// - `u8` +/// - `i16` +/// - `u16` +/// - `i32` +/// - `u32` +/// - `i64` +/// - `u64` +/// - `isize` +/// - `usize` +/// /// C header: [`include/linux/moduleparam.h`](srctree/include/linux/moduleparam.h) /// /// [`Module`]: ../kernel/trait.Module.html @@ -36,21 +60,19 @@ /// description: "My very own kernel module!", /// license: "GPL", /// alias: ["alternate_module_name"], +/// params: { +/// my_parameter: i64 { +/// default: 1, +/// description: "This parameter has a default of 1", +/// }, +/// }, /// } /// /// struct MyModule; /// /// impl kernel::Module for MyModule { /// fn init() -> Result<Self> { -/// // If the parameter is writeable, then the kparam lock must be -/// // taken to read the parameter: -/// { -/// let lock = THIS_MODULE.kernel_param_lock(); -/// pr_info!("i32 param is: {}\n", writeable_i32.read(&lock)); -/// } -/// // If the parameter is read only, it can be read without locking -/// // the kernel parameters: -/// pr_info!("i32 param is: {}\n", my_i32.read()); +/// pr_info!("i32 param is: {}\n", module_parameters::my_parameter::read()); /// Ok(Self) /// } /// } diff --git a/rust/macros/module.rs b/rust/macros/module.rs index 411dc103d82e..2fa9ed8e78ff 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -26,6 +26,7 @@ struct ModInfoBuilder<'a> { module: &'a str, counter: usize, buffer: String, + param_buffer: String, } impl<'a> ModInfoBuilder<'a> { @@ -34,10 +35,11 @@ fn new(module: &'a str) -> Self { module, counter: 0, buffer: String::new(), + param_buffer: String::new(), } } - fn emit_base(&mut self, field: &str, content: &str, builtin: bool) { + fn emit_base(&mut self, field: &str, content: &str, builtin: bool, param: bool) { let string = if builtin { // Built-in modules prefix their modinfo strings by `module.`. format!( @@ -51,8 +53,14 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) { format!("{field}={content}\0", field = field, content = content) }; + let buffer = if param { + &mut self.param_buffer + } else { + &mut self.buffer + }; + write!( - &mut self.buffer, + buffer, " {cfg} #[doc(hidden)] @@ -75,20 +83,135 @@ fn emit_base(&mut self, field: &str, content: &str, builtin: bool) { self.counter += 1; } - fn emit_only_builtin(&mut self, field: &str, content: &str) { - self.emit_base(field, content, true) + fn emit_only_builtin(&mut self, field: &str, content: &str, param: bool) { + self.emit_base(field, content, true, param) } - fn emit_only_loadable(&mut self, field: &str, content: &str) { - self.emit_base(field, content, false) + fn emit_only_loadable(&mut self, field: &str, content: &str, param: bool) { + self.emit_base(field, content, false, param) } fn emit(&mut self, field: &str, content: &str) { - self.emit_only_builtin(field, content); - self.emit_only_loadable(field, content); + self.emit_internal(field, content, false); + } + + fn emit_internal(&mut self, field: &str, content: &str, param: bool) { + self.emit_only_builtin(field, content, param); + self.emit_only_loadable(field, content, param); + } + + fn emit_param(&mut self, field: &str, param: &str, content: &str) { + let content = format!("{param}:{content}", param = param, content = content); + self.emit_internal(field, &content, true); + } + + fn emit_params(&mut self, info: &ModuleInfo) { + if let Some(params) = &info.params { + for param in params { + let ops = param_ops_path(¶m.ptype); + + self.emit_param("parmtype", ¶m.name, ¶m.ptype); + self.emit_param("parm", ¶m.name, ¶m.description); + + write!( + self.param_buffer, + " + static mut __{name}_{param_name}_value: {param_type} = {param_default}; + + pub(crate) enum {param_name} {{}} + + impl {param_name} {{ + pub(crate) fn read<'a>() + -> &'a <{param_type} as ::kernel::module_param::ModuleParam>::Value {{ + // Note: when we enable r/w parameters, we need to lock here. + + // SAFETY: Parameters do not need to be locked because they are + // read only or sysfs is not enabled. + unsafe {{ + <{param_type} as ::kernel::module_param::ModuleParam>::value( + &__{name}_{param_name}_value + ) + }} + }} + }} + + /// Newtype to make `bindings::kernel_param` `Sync`. + #[repr(transparent)] + struct __{name}_{param_name}_RacyKernelParam(::kernel::bindings::kernel_param); + + // SAFETY: C kernel handles serializing access to this type. We + // never access from Rust module. + unsafe impl Sync for __{name}_{param_name}_RacyKernelParam {{ }} + + #[cfg(not(MODULE))] + const __{name}_{param_name}_name: *const ::core::ffi::c_char = + b\"{name}.{param_name}\\0\" as *const _ as *const ::core::ffi::c_char; + + #[cfg(MODULE)] + const __{name}_{param_name}_name: *const ::core::ffi::c_char = + b\"{param_name}\\0\" as *const _ as *const ::core::ffi::c_char; + + #[link_section = \"__param\"] + #[used] + static __{name}_{param_name}_struct: __{name}_{param_name}_RacyKernelParam = + __{name}_{param_name}_RacyKernelParam(::kernel::bindings::kernel_param {{ + name: __{name}_{param_name}_name, + // SAFETY: `__this_module` is constructed by the kernel at load time + // and will not be freed until the module is unloaded. + #[cfg(MODULE)] + mod_: unsafe {{ &::kernel::bindings::__this_module as *const _ as *mut _ }}, + #[cfg(not(MODULE))] + mod_: ::core::ptr::null_mut(), + ops: &{ops} as *const ::kernel::bindings::kernel_param_ops, + perm: 0, // Will not appear in sysfs + level: -1, + flags: 0, + __bindgen_anon_1: + ::kernel::bindings::kernel_param__bindgen_ty_1 {{ + // SAFETY: As this is evaluated in const context, it is + // safe to take a reference to a mut static. + arg: unsafe {{ + ::core::ptr::addr_of_mut!(__{name}_{param_name}_value) + }}.cast::<::core::ffi::c_void>(), + }}, + }}); + ", + name = info.name, + param_type = param.ptype, + param_default = param.default, + param_name = param.name, + ops = ops, + ) + .unwrap(); + } + } + } +} + +fn param_ops_path(param_type: &str) -> &'static str { + match param_type { + "i8" => "::kernel::module_param::PARAM_OPS_I8", + "u8" => "::kernel::module_param::PARAM_OPS_U8", + "i16" => "::kernel::module_param::PARAM_OPS_I16", + "u16" => "::kernel::module_param::PARAM_OPS_U16", + "i32" => "::kernel::module_param::PARAM_OPS_I32", + "u32" => "::kernel::module_param::PARAM_OPS_U32", + "i64" => "::kernel::module_param::PARAM_OPS_I64", + "u64" => "::kernel::module_param::PARAM_OPS_U64", + "isize" => "::kernel::module_param::PARAM_OPS_ISIZE", + "usize" => "::kernel::module_param::PARAM_OPS_USIZE", + t => panic!("Unsupported parameter type {}", t), } } +fn expect_param_default(param_it: &mut token_stream::IntoIter) -> String { + assert_eq!(expect_ident(param_it), "default"); + assert_eq!(expect_punct(param_it), ':'); + let default = try_literal(param_it).expect("Expected default param value"); + assert_eq!(expect_punct(param_it), ','); + default +} + #[derive(Debug, Default)] struct ModuleInfo { type_: String, @@ -98,6 +221,50 @@ struct ModuleInfo { description: Option<String>, alias: Option<Vec<String>>, firmware: Option<Vec<String>>, + params: Option<Vec<Parameter>>, +} + +#[derive(Debug)] +struct Parameter { + name: String, + ptype: String, + default: String, + description: String, +} + +fn expect_params(it: &mut token_stream::IntoIter) -> Vec<Parameter> { + let params = expect_group(it); + assert_eq!(params.delimiter(), Delimiter::Brace); + let mut it = params.stream().into_iter(); + let mut parsed = Vec::new(); + + loop { + let param_name = match it.next() { + Some(TokenTree::Ident(ident)) => ident.to_string(), + Some(_) => panic!("Expected Ident or end"), + None => break, + }; + + assert_eq!(expect_punct(&mut it), ':'); + let param_type = expect_ident(&mut it); + let group = expect_group(&mut it); + assert_eq!(group.delimiter(), Delimiter::Brace); + assert_eq!(expect_punct(&mut it), ','); + + let mut param_it = group.stream().into_iter(); + let param_default = expect_param_default(&mut param_it); + let param_description = expect_string_field(&mut param_it, "description"); + expect_end(&mut param_it); + + parsed.push(Parameter { + name: param_name, + ptype: param_type, + default: param_default, + description: param_description, + }) + } + + parsed } impl ModuleInfo { @@ -112,6 +279,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self { "license", "alias", "firmware", + "params", ]; const REQUIRED_KEYS: &[&str] = &["type", "name", "license"]; let mut seen_keys = Vec::new(); @@ -140,6 +308,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self { "license" => info.license = expect_string_ascii(it), "alias" => info.alias = Some(expect_string_array(it)), "firmware" => info.firmware = Some(expect_string_array(it)), + "params" => info.params = Some(expect_params(it)), _ => panic!( "Unknown key \"{}\". Valid keys are: {:?}.", key, EXPECTED_KEYS @@ -183,28 +352,30 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { let info = ModuleInfo::parse(&mut it); let mut modinfo = ModInfoBuilder::new(info.name.as_ref()); - if let Some(author) = info.author { - modinfo.emit("author", &author); + if let Some(author) = &info.author { + modinfo.emit("author", author); } - if let Some(description) = info.description { - modinfo.emit("description", &description); + if let Some(description) = &info.description { + modinfo.emit("description", description); } modinfo.emit("license", &info.license); - if let Some(aliases) = info.alias { + if let Some(aliases) = &info.alias { for alias in aliases { - modinfo.emit("alias", &alias); + modinfo.emit("alias", alias); } } - if let Some(firmware) = info.firmware { + if let Some(firmware) = &info.firmware { for fw in firmware { - modinfo.emit("firmware", &fw); + modinfo.emit("firmware", fw); } } // Built-in modules also export the `file` modinfo string. let file = std::env::var("RUST_MODFILE").expect("Unable to fetch RUST_MODFILE environmental variable"); - modinfo.emit_only_builtin("file", &file); + modinfo.emit_only_builtin("file", &file, false); + + modinfo.emit_params(&info); format!( " @@ -216,12 +387,14 @@ pub(crate) fn module(ts: TokenStream) -> TokenStream { // SAFETY: `__this_module` is constructed by the kernel at load time and will not be // freed until the module is unloaded. #[cfg(MODULE)] - static THIS_MODULE: kernel::ThisModule = unsafe {{ - kernel::ThisModule::from_ptr(&kernel::bindings::__this_module as *const _ as *mut _) + static THIS_MODULE: ::kernel::ThisModule = unsafe {{ + ::kernel::ThisModule::from_ptr( + &::kernel::bindings::__this_module as *const _ as *mut _ + ) }}; #[cfg(not(MODULE))] - static THIS_MODULE: kernel::ThisModule = unsafe {{ - kernel::ThisModule::from_ptr(core::ptr::null_mut()) + static THIS_MODULE: ::kernel::ThisModule = unsafe {{ + ::kernel::ThisModule::from_ptr(::core::ptr::null_mut()) }}; // Double nested modules, since then nobody can access the public items inside. @@ -276,7 +449,8 @@ mod __module_init {{ #[doc(hidden)] #[link_section = \"{initcall_section}\"] #[used] - pub static __{name}_initcall: extern \"C\" fn() -> core::ffi::c_int = __{name}_init; + pub static __{name}_initcall: extern \"C\" fn() + -> ::core::ffi::c_int = __{name}_init; #[cfg(not(MODULE))] #[cfg(CONFIG_HAVE_ARCH_PREL32_RELOCATIONS)] @@ -291,7 +465,7 @@ mod __module_init {{ #[cfg(not(MODULE))] #[doc(hidden)] #[no_mangle] - pub extern \"C\" fn __{name}_init() -> core::ffi::c_int {{ + pub extern \"C\" fn __{name}_init() -> ::core::ffi::c_int {{ // SAFETY: This function is inaccessible to the outside due to the double // module wrapping it. It is called exactly once by the C side via its // placement above in the initcall section. @@ -314,8 +488,8 @@ mod __module_init {{ /// # Safety /// /// This function must only be called once. - unsafe fn __init() -> core::ffi::c_int {{ - match <{type_} as kernel::Module>::init(&super::super::THIS_MODULE) {{ + unsafe fn __init() -> ::core::ffi::c_int {{ + match <{type_} as ::kernel::Module>::init(&super::super::THIS_MODULE) {{ Ok(m) => {{ // SAFETY: No data race, since `__MOD` can only be accessed by this // module and there only `__init` and `__exit` access it. These @@ -346,14 +520,17 @@ unsafe fn __exit() {{ __MOD = None; }} }} - {modinfo} }} }} + mod module_parameters {{ + {params} + }} ", type_ = info.type_, name = info.name, modinfo = modinfo.buffer, + params = modinfo.param_buffer, initcall_section = ".initcall6.init" ) .parse() diff --git a/samples/rust/rust_minimal.rs b/samples/rust/rust_minimal.rs index 2a9eaab62d1c..d9bc2218d504 100644 --- a/samples/rust/rust_minimal.rs +++ b/samples/rust/rust_minimal.rs @@ -10,6 +10,12 @@ author: "Rust for Linux Contributors", description: "Rust minimal sample", license: "GPL", + params: { + test_parameter: i64 { + default: 1, + description: "This parameter has a default of 1", + }, + }, } struct RustMinimal { @@ -20,6 +26,10 @@ impl kernel::Module for RustMinimal { fn init(_module: &'static ThisModule) -> Result<Self> { pr_info!("Rust minimal sample (init)\n"); pr_info!("Am I built-in? {}\n", !cfg!(MODULE)); + pr_info!( + "My parameter: {}\n", + *module_parameters::test_parameter::read() + ); let mut numbers = Vec::new(); numbers.push(72, GFP_KERNEL)?; diff --git a/scripts/Makefile.build b/scripts/Makefile.build index efacca63c897..a65bd0233843 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 := new_uninit,const_mut_refs # `--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