| Message ID | 20240705111455.142790-1-nmi@metaspace.dk (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | rust: add `module_params` macro | expand |
Hi Adam, "Andreas Hindborg" <nmi@metaspace.dk> writes: > 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. > > This code is a reduced and updated version of code by Adam available in the > original `rust` branch [1]. > > [1] https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f > > Cc: Adam Bratschi-Kaye <ark.email@gmail.com> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> Two questions for you: a) Can I put your sign off on this patch, and b) how would you prefer attribution on this patch? Best regards, Andreas
On Fri, Jul 05, 2024 at 11:15:11AM +0000, Andreas Hindborg 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. > > This code is a reduced and updated version of code by Adam available in the > original `rust` branch [1]. > > [1] https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f > > Cc: Adam Bratschi-Kaye <ark.email@gmail.com> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > This poses an interesting challenge I'd like to take up with the Rust community. I'm fine with Rust bindings, however many C maintainers neither don't speak / write Rust, and in many cases many don't even want to touch Rust at all. In my case I want to get there.. but just haven't had time yet. So we have to live with that world. But to help with a Rust world, clearly we need to allow for some Rust bindings. As a compromise, I recently suggested for example for the firmware_loader Rust bindings to be acceptable if and only if we could get the developer doing those changes to be willing to commit to also being *both* a C and rust maintainer for the firmware loader. I'm starting to feel the same way about modules, but modules requires more work than the firmware loader. And since I also know Andreas has already a lot on his plate, I'm at a cross roads. My above request for the firmware loader made sense to the person working on the firmware loader changes, but who would help on the modules side of things? And does this request make sense to help scale? The rationale here is that a rust binding means commitment then also from fresh blood to help co-maintain review C / Rust for exising code when there is will / desire to collaborate from an existing C maintainer. I realize this may be a lot to ask, but I think this is one of the responsible ways to ask to scale here. Luis
Hi Luis, On Monday, July 8th, 2024 at 23:42, Luis Chamberlain <mcgrof@kernel.org> wrote: > I'm starting to feel the same way about modules, but modules requires > more work than the firmware loader. And since I also know Andreas has > already a lot on his plate, I'm at a cross roads. My above request for > the firmware loader made sense to the person working on the firmware > loader changes, but who would help on the modules side of things? And > does this request make sense to help scale? > > The rationale here is that a rust binding means commitment then also > from fresh blood to help co-maintain review C / Rust for exising code > when there is will / desire to collaborate from an existing C maintainer. > > I realize this may be a lot to ask, but I think this is one of the > responsible ways to ask to scale here. I am not sure I am the right person for the task, because as you say, I have a lot on my plate. But perhaps lets schedule a call so I can get a sense of the required effort. Best regards, Andreas
On Tue, Jul 09, 2024 at 06:00:46AM +0000, nmi wrote: > Hi Luis, > > On Monday, July 8th, 2024 at 23:42, Luis Chamberlain <mcgrof@kernel.org> wrote: > > > I'm starting to feel the same way about modules, but modules requires > > more work than the firmware loader. And since I also know Andreas has > > already a lot on his plate, I'm at a cross roads. My above request for > > the firmware loader made sense to the person working on the firmware > > loader changes, but who would help on the modules side of things? And > > does this request make sense to help scale? > > > > The rationale here is that a rust binding means commitment then also > > from fresh blood to help co-maintain review C / Rust for exising code > > when there is will / desire to collaborate from an existing C maintainer. > > > > I realize this may be a lot to ask, but I think this is one of the > > responsible ways to ask to scale here. > > I am not sure I am the right person for the task, because as you say, > I have a lot on my plate. But perhaps lets schedule a call so I can > get a sense of the required effort. Kernel development is done through emails, not calls :) If a submitter isn't willing to maintain the code they submit, then it should be rejected as maintance is the most important part. Sorry, greg k-h
Hi Greg, On Tuesday, 9 July 2024 at 10:27, Greg KH <gregkh@linuxfoundation.org> wrote: > On Tue, Jul 09, 2024 at 06:00:46AM +0000, nmi wrote: > > > Hi Luis, > > > > On Monday, July 8th, 2024 at 23:42, Luis Chamberlain mcgrof@kernel.org wrote: > > > > > I'm starting to feel the same way about modules, but modules requires > > > more work than the firmware loader. And since I also know Andreas has > > > already a lot on his plate, I'm at a cross roads. My above request for > > > the firmware loader made sense to the person working on the firmware > > > loader changes, but who would help on the modules side of things? And > > > does this request make sense to help scale? > > > > > > The rationale here is that a rust binding means commitment then also > > > from fresh blood to help co-maintain review C / Rust for exising code > > > when there is will / desire to collaborate from an existing C maintainer. > > > > > > I realize this may be a lot to ask, but I think this is one of the > > > responsible ways to ask to scale here. > > > > I am not sure I am the right person for the task, because as you say, > > I have a lot on my plate. But perhaps lets schedule a call so I can > > get a sense of the required effort. > > > Kernel development is done through emails, not calls :) This is such a confusing statement to me. Of course people are using video calls and even meeting up in real life, to discuss matters of kernel development and communicate technical details? Perhaps you could clarify a bit more what you are trying to communicate? > > If a submitter isn't willing to maintain the code they submit, then it > should be rejected as maintance is the most important part. Unless I misunderstand something, Luis requests is about the C code that is already in the kernel, not the code I submitted now. Of course I will maintain the code I submit, if required. Best regards, Andreas
On Mon, Jul 8, 2024 at 11:42 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > The rationale here is that a rust binding means commitment then also > from fresh blood to help co-maintain review C / Rust for exising code > when there is will / desire to collaborate from an existing C maintainer. > > I realize this may be a lot to ask, but I think this is one of the > responsible ways to ask to scale here. Yeah, there have been different approaches for this taken by different subsystems -- it depends on their constraints and how much the submitter can commit to. For instance, some maintainers may want to keep being the maintainers of both Rust and C. Some want that the submitter becomes a new co-maintainer in the subsystem and eventually maintainers both C and Rust. Some prefer to have a new maintainer for the Rust side only, i.e. considering Rust as a new section of the subsystem with a new `MAINTAINERS` entry and all. On top of that, some allow the C and Rust sides to be independent, to the point of allowing temporary breakage on the Rust side if the new maintainers commits to be quick fixing it (though I have my reservations about how well that would eventually work if more core/common subsystems start doing that -- linux-next could be broken a lot of the time for the Rust side). But, yes, I think Rust is a great opportunity to get new co-maintainers, as well as getting new developers involved with kernel maintenance in general, which could help with other issues too. Cheers, Miguel
On Tue, Jul 09, 2024 at 12:08:16PM +0200, Miguel Ojeda wrote: > On Mon, Jul 8, 2024 at 11:42 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > The rationale here is that a rust binding means commitment then also > > from fresh blood to help co-maintain review C / Rust for exising code > > when there is will / desire to collaborate from an existing C maintainer. > > > > I realize this may be a lot to ask, but I think this is one of the > > responsible ways to ask to scale here. > > But, yes, I think Rust is a great opportunity to get new > co-maintainers, as well as getting new developers involved with kernel > maintenance in general, which could help with other issues too. Great well then my preference is to not have Rust bindings for modules unless the Rust community can commit to not only a co-maintianer for both C And Rust but also commit to not ditching the role; if a C/Rust co-maintainer gets hits by a bus the Rust community would strive to look for someone else to step in. This would proactively help with upstream responsibilities understood by companies who hire developers in this context. It is why I brought up Andreas's work, I already know he has a lot of work to do and responsibilities. If not Andreas, who else can step up to help with this, Sami? While each company varies in accepting a developer's roles in the community, I think we would stand to gain to consider the long term aspects of this before it becomes an issue, so we get employers to understand / accept this as part of our work. I don't think this is an unreasonable for companies or developers interested in Rust advancements. This includes testing, helping improve tests and using existing tests or automation tools for them so we don't regress. Clearly, this isn't just about a module_params macro, for example I'm starting to see other module related code I need to review and having to be very careful to ensure all of what is ongoing with modules like Kris's work on kbuild CONFIG_BUILTIN_MODULE_RANGES will still work in a Rust modules world with Sami's work on module modversions. [0] https://lkml.kernel.org/r/20240716031045.1781332-1-kris.van.hees@oracle.com Luis
Hi Luis, On Thu, Jul 18, 2024 at 5:15 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > On Tue, Jul 09, 2024 at 12:08:16PM +0200, Miguel Ojeda wrote: > > On Mon, Jul 8, 2024 at 11:42 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > The rationale here is that a rust binding means commitment then also > > > from fresh blood to help co-maintain review C / Rust for exising code > > > when there is will / desire to collaborate from an existing C maintainer. > > > > > > I realize this may be a lot to ask, but I think this is one of the > > > responsible ways to ask to scale here. > > > > But, yes, I think Rust is a great opportunity to get new > > co-maintainers, as well as getting new developers involved with kernel > > maintenance in general, which could help with other issues too. > > Great well then my preference is to not have Rust bindings for modules > unless the Rust community can commit to not only a co-maintianer for > both C And Rust but also commit to not ditching the role; if a C/Rust > co-maintainer gets hits by a bus the Rust community would strive to > look for someone else to step in. This would proactively help with > upstream responsibilities understood by companies who hire developers > in this context. It is why I brought up Andreas's work, I already know > he has a lot of work to do and responsibilities. If not Andreas, who else > can step up to help with this, Sami? I agree, having a co-maintainer from the Rust community sounds like a good idea. It would be great if someone actually working on the bindings could step up, but if there are no other volunteers, I can certainly help with this. Sami
On 05.07.24 13:15, Andreas Hindborg 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. > > This code is a reduced and updated version of code by Adam available in the > original `rust` branch [1]. > > [1] https://github.com/Rust-for-Linux/linux/tree/bc22545f38d74473cfef3e9fd65432733435b79f > > Cc: Adam Bratschi-Kaye <ark.email@gmail.com> > Signed-off-by: Andreas Hindborg <a.hindborg@samsung.com> > > --- > > I have patches queued up for `rnull` that depend on this patch. > --- > rust/bindings/bindings_helper.h | 1 + > rust/kernel/lib.rs | 2 + > rust/kernel/module_param.rs | 339 ++++++++++++++++++++++++++++++++ > rust/macros/helpers.rs | 8 + > rust/macros/module.rs | 230 ++++++++++++++++++++-- > samples/rust/rust_minimal.rs | 10 + > 6 files changed, 571 insertions(+), 19 deletions(-) > create mode 100644 rust/kernel/module_param.rs > > diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h > index ddb5644d4fd9..767169f46f10 100644 > --- a/rust/bindings/bindings_helper.h > +++ b/rust/bindings/bindings_helper.h > @@ -25,3 +25,4 @@ const gfp_t RUST_CONST_HELPER_GFP_KERNEL = GFP_KERNEL; > const gfp_t RUST_CONST_HELPER_GFP_KERNEL_ACCOUNT = GFP_KERNEL_ACCOUNT; > const gfp_t RUST_CONST_HELPER_GFP_NOWAIT = GFP_NOWAIT; > const gfp_t RUST_CONST_HELPER___GFP_ZERO = __GFP_ZERO; > +const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE; > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index fbd91a48ff8b..7cc2730e572f 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -33,6 +33,8 @@ > pub mod ioctl; > #[cfg(CONFIG_KUNIT)] > pub mod kunit; > +#[doc(hidden)] > +pub mod module_param; > #[cfg(CONFIG_NET)] > pub mod net; > pub mod prelude; > diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs > new file mode 100644 > index 000000000000..dc1108d20723 > --- /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::{error::code::*, str::CStr}; > + > +/// 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; > + > + /// Whether the parameter is allowed to be set without an argument. > + /// > + /// Setting this to `true` allows the parameter to be passed without an > + /// argument (e.g. just `module.param` instead of `module.param=foo`). > + const NOARG_ALLOWED: bool; I think, there is a better way of doing this. Instead of this bool, we do the following: 1. have a `const DEFAULT: Option<Self>` 2. change the type of the argument of `try_from_param_arg` to `&'static [u8]` That way we don't have the weird behavior of `try_from_param_arg` that for params that don't have a default value. > + > + /// Convert a parameter argument into the parameter value. > + /// > + /// `None` should be returned when parsing of the argument fails. > + /// `arg == None` indicates that the parameter was passed without an > + /// argument. If `NOARG_ALLOWED` is set to `false` then `arg` is guaranteed > + /// to always be `Some(_)`. > + /// > + /// 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: Option<&'static [u8]>) -> Option<Self>; Do we want this to return `Result` instead? Or rather, why is this returning `Option`? > + > + /// 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 Self` 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 `Self`. > + /// > + /// # 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( > + val: *const core::ffi::c_char, > + param: *const crate::bindings::kernel_param, > + ) -> core::ffi::c_int { > + let arg = if val.is_null() { > + None > + } else { > + // 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. > + Some(unsafe { CStr::from_char_ptr(val).as_bytes() }) > + }; > + match Self::try_from_param_arg(arg) { > + Some(new_value) => { > + // SAFETY: `param` is guaranteed to be valid by C API contract > + // and `arg` is guaranteed to point to an instance of `Self`. > + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut Self }; > + // 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. > + let _ = unsafe { core::ptr::replace(old_value, new_value) }; > + 0 > + } > + None => EINVAL.to_errno(), > + } > + } Do implementers need to override this function? If not, then I would move it out of the trait. > + > + /// 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. > + unsafe extern "C" fn get_param( > + _buf: *mut core::ffi::c_char, > + _param: *const crate::bindings::kernel_param, > + ) -> core::ffi::c_int { > + 0 > + } Ditto. > + > + /// 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(arg: *mut core::ffi::c_void) { > + // SAFETY: By function safety requirement, `arg` is an initialized > + // instance of `Self`. By C API contract, `arg` will not be used after > + // this function returns. > + unsafe { core::ptr::drop_in_place(arg as *mut Self) }; > + } Ditto. > +} [...] > diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs > index 563dcd2b7ace..dc0b47879a8c 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 get_string(it: &mut token_stream::IntoIter, expected_name: &str) -> String { This name is rather weird, `get_field` makes more sense IMO. > + assert_eq!(expect_ident(it), expected_name); > + assert_eq!(expect_punct(it), ':'); > + let string = expect_string(it); > + assert_eq!(expect_punct(it), ','); Why do we require a trailing comma? > + 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/module.rs b/rust/macros/module.rs > index acd0393b5095..fc794855b99e 100644 > --- a/rust/macros/module.rs > +++ b/rust/macros/module.rs > @@ -1,7 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > > use crate::helpers::*; > -use proc_macro::{token_stream, Delimiter, Literal, TokenStream, TokenTree}; > +use proc_macro::{token_stream, Delimiter, Group, Literal, TokenStream, TokenTree}; > use std::fmt::Write; > > fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> { > @@ -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,190 @@ 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) { This parses the parameters, but shouldn't that happen in `ModuleInfo::parse`? I agree that it should be a different function, but I don't think that we should store the params as a `Group` and fail when we emit the params. > + if let Some(params) = &info.params { > + assert_eq!(params.delimiter(), Delimiter::Brace); > + > + let mut it = params.stream().into_iter(); > + > + 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!(expect_punct(&mut it), ','); Why do we require a trailing comma? > + > + assert_eq!(group.delimiter(), Delimiter::Brace); This check will should be immediately after expecting the group. > + > + let mut param_it = group.stream().into_iter(); > + let param_default = get_param_default(&mut param_it); I don't understand why this is put into its own (5 LOC) function and other parts of this function are not. If you can separate parsing from emitting better, this will probably improve. > + let param_description = get_string(&mut param_it, "description"); > + expect_end(&mut param_it); > + > + let (param_kernel_type, ops): (String, _) = ( Why do you need to specify the type like this? > + param_type.to_string(), > + param_ops_path(¶m_type).to_string(), > + ); > + > + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); Is the spelling intentional? "parmtype"? > + self.emit_param("parm", ¶m_name, ¶m_description); > + let param_type_internal = param_type.clone(); > + > + let read_func = format!( > + " > + pub(crate) fn read(&self) > + -> &<{param_type_internal} > + as kernel::module_param::ModuleParam>::Value {{ Please add a `::` in front of `kernel::module_param::ModuleParam`. There are more instances below. > + // 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_internal} as kernel::module_param::ModuleParam>::value( > + &__{name}_{param_name}_value > + ) > + }} > + }} > + ", > + name = info.name, > + param_name = param_name, > + param_type_internal = param_type_internal, > + ); > + > + let kparam = format!( > + " > + kernel::bindings::kernel_param__bindgen_ty_1 {{ > + // SAFETY: Access through the resulting pointer is > + // serialized by C side and only happens before module > + // `init` or after module `drop` is called. > + arg: unsafe {{ &__{name}_{param_name}_value }} > + as *const _ as *mut core::ffi::c_void, Here you should use `addr_of[_mut]!` instead of taking a reference. Also will this pointer be used to write to the static, in that case you need `_mut!`. > + }}, > + ", > + name = info.name, > + param_name = param_name, > + ); What is the reason for putting `kparam` and `read_func` outside of the `write!` below? I think it would be easier to read if they are inlined. > + write!( > + self.param_buffer, > + " > + static mut __{name}_{param_name}_value: {param_type_internal} = {param_default}; > + > + pub(crate) struct __{name}_{param_name}; > + > + impl __{name}_{param_name} {{ {read_func} }} > + > + pub(crate) const {param_name}: __{name}_{param_name} = __{name}_{param_name}; Why do we need a unit struct as a constant? I think it would make more sense to have a unit struct/empty enum as the type and the `read` function be without a receiver. > + > + // Note: the C macro that generates the static structs for the `__param` section > + // asks for them to be `aligned(sizeof(void *))`. However, that was put in place > + // in 2003 in commit 38d5b085d2a0 (\"[PATCH] Fix over-alignment problem on x86-64\") > + // to undo GCC over-alignment of static structs of >32 bytes. It seems that is > + // not the case anymore, so we simplify to a transparent representation here > + // in the expectation that it is not needed anymore. > + // TODO: Revisit this to confirm the above comment and remove it if it happened. Should this TODO be fixed before this is merged? Or do you intend for it to stay? If this is indeed correct, should this also be changed in the C side (of course a different patch)? > + /// 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 {{ > + }} Any reason to put the `}` on the next line? > + > + #[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(), > + // SAFETY: This static is actually constant as seen by > + // module code. But we need a unique address for it, so it > + // must be static. This safety comment makes no sense, should it be a normal comment? > + ops: unsafe {{ &{ops} }} as *const kernel::bindings::kernel_param_ops, Why is this `unsafe` block needed, the `make_param_ops` macro declares a non-mut static. > + perm: 0, // Will not appear in sysfs > + level: -1, Why this value? > + flags: 0, > + __bindgen_anon_1: {kparam} > + }}); > + ", > + name = info.name, > + param_type_internal = param_type_internal, > + read_func = read_func, > + param_default = param_default, > + param_name = param_name, > + ops = ops, > + kparam = kparam, > + ) > + .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!("Unrecognized type {}", t), I would write "Unsupported parameter type `{}`!". > + } > +} > + > +fn get_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 > +} > + Could you also add documentation in rust/macros/lib.rs on the `module!` macro how parameters can be declared/used? I took a look and it seems that there is out-of-date docs there also. --- Cheers, Benno
Hi Benno, Thanks for the comments! "Benno Lossin" <benno.lossin@proton.me> writes: > On 05.07.24 13:15, Andreas Hindborg wrote: [...] >> + >> +/// 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; >> + >> + /// Whether the parameter is allowed to be set without an argument. >> + /// >> + /// Setting this to `true` allows the parameter to be passed without an >> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >> + const NOARG_ALLOWED: bool; > > I think, there is a better way of doing this. Instead of this bool, we > do the following: > 1. have a `const DEFAULT: Option<Self>` > 2. change the type of the argument of `try_from_param_arg` to > `&'static [u8]` > > That way we don't have the weird behavior of `try_from_param_arg` that > for params that don't have a default value. Since we have no parameter types for which `NOARG_ALLOWED` is true in this patch set, it is effectively dead code. I will remove it. > >> + >> + /// Convert a parameter argument into the parameter value. >> + /// >> + /// `None` should be returned when parsing of the argument fails. >> + /// `arg == None` indicates that the parameter was passed without an >> + /// argument. If `NOARG_ALLOWED` is set to `false` then `arg` is guaranteed >> + /// to always be `Some(_)`. >> + /// >> + /// 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: Option<&'static [u8]>) -> Option<Self>; > > Do we want this to return `Result` instead? > Or rather, why is this returning `Option`? Some legacy cruft is going on here I think. At some point in the past the patch set supported parameters without value, and this option was supposed to represent the presence of a value. It looks like it then got aliased with parse errors. Since we do not support parameters without value in this patch set, I will update the return type and documentation to handle parse errors with `Result`. > >> + >> + /// 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 Self` 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 `Self`. >> + /// >> + /// # 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( >> + val: *const core::ffi::c_char, >> + param: *const crate::bindings::kernel_param, >> + ) -> core::ffi::c_int { >> + let arg = if val.is_null() { >> + None >> + } else { >> + // 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. >> + Some(unsafe { CStr::from_char_ptr(val).as_bytes() }) >> + }; >> + match Self::try_from_param_arg(arg) { >> + Some(new_value) => { >> + // SAFETY: `param` is guaranteed to be valid by C API contract >> + // and `arg` is guaranteed to point to an instance of `Self`. >> + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut Self }; >> + // 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. >> + let _ = unsafe { core::ptr::replace(old_value, new_value) }; >> + 0 >> + } >> + None => EINVAL.to_errno(), >> + } >> + } > > Do implementers need to override this function? If not, then I would > move it out of the trait. Ok, makes sense
On 01.08.24 13:29, Andreas Hindborg wrote: > > Hi Benno, > > Thanks for the comments! > > "Benno Lossin" <benno.lossin@proton.me> writes: > >> On 05.07.24 13:15, Andreas Hindborg wrote: > > [...] > >>> + >>> +/// 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; >>> + >>> + /// Whether the parameter is allowed to be set without an argument. >>> + /// >>> + /// Setting this to `true` allows the parameter to be passed without an >>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>> + const NOARG_ALLOWED: bool; >> >> I think, there is a better way of doing this. Instead of this bool, we >> do the following: >> 1. have a `const DEFAULT: Option<Self>` >> 2. change the type of the argument of `try_from_param_arg` to >> `&'static [u8]` >> >> That way we don't have the weird behavior of `try_from_param_arg` that >> for params that don't have a default value. > > Since we have no parameter types for which `NOARG_ALLOWED` is true in > this patch set, it is effectively dead code. I will remove it. Hmm what parameters actually are optional? I looked at the old rust branch and only `bool` is marked as optional. Are there others? If it is used commonly for custom parameters (I could imagine that Rust modules have enums as parameters and specifying nothing could mean the default value), then it might be a good idea to just include it now. (otherwise we might forget the design later) >>> + >>> + /// Convert a parameter argument into the parameter value. >>> + /// >>> + /// `None` should be returned when parsing of the argument fails. >>> + /// `arg == None` indicates that the parameter was passed without an >>> + /// argument. If `NOARG_ALLOWED` is set to `false` then `arg` is guaranteed >>> + /// to always be `Some(_)`. [...] >>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs >>> index 563dcd2b7ace..dc0b47879a8c 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 get_string(it: &mut token_stream::IntoIter, expected_name: &str) -> String { >> >> This name is rather weird, `get_field` makes more sense IMO. > > Looking at this, the `get` prefix is not aligned with other helpers. How > about `expect_string_field` ? SGTM >>> + assert_eq!(expect_ident(it), expected_name); >>> + assert_eq!(expect_punct(it), ':'); >>> + let string = expect_string(it); >>> + assert_eq!(expect_punct(it), ','); >> >> Why do we require a trailing comma? > > For consistency with existing module macro. All keys must be terminated > with comma. Hmm I think that might be a bit unexpected, since everywhere else in Rust you are allowed to omit the trailing comma. But I guess if the entire `module!` macro does that currently, then we can change that when we move to `syn`. [...] >>> + param_type.to_string(), >>> + param_ops_path(¶m_type).to_string(), >>> + ); >>> + >>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); >> >> Is the spelling intentional? "parmtype"? > > This is intentional. I don't think the kernel is ever parsing this, but > it is parsed by the `modinfo` tool. Hmm, why is it not `paramtype`? Does that conflict with something? >>> + self.emit_param("parm", ¶m_name, ¶m_description); >>> + let param_type_internal = param_type.clone(); >>> + >>> + let read_func = format!( >>> + " >>> + pub(crate) fn read(&self) >>> + -> &<{param_type_internal} >>> + as kernel::module_param::ModuleParam>::Value {{ >> >> Please add a `::` in front of `kernel::module_param::ModuleParam`. There >> are more instances below. > > Thanks. > >> >>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>> + &__{name}_{param_name}_value >>> + ) >>> + }} >>> + }} >>> + ", >>> + name = info.name, >>> + param_name = param_name, >>> + param_type_internal = param_type_internal, >>> + ); >>> + >>> + let kparam = format!( >>> + " >>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>> + // SAFETY: Access through the resulting pointer is >>> + // serialized by C side and only happens before module >>> + // `init` or after module `drop` is called. >>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>> + as *const _ as *mut core::ffi::c_void, >> >> Here you should use `addr_of[_mut]!` instead of taking a reference. > > This is a static initializer, so it would be evaluated in const context. > At that time, this is going to be the only reference to > `&__{name}_{param_name}_value` which would be const. So it should be > fine? When compiling this [1] with a sufficiently new Rust version, you will get an error: warning: creating a shared reference to mutable static is discouraged --> src/main.rs:4:22 | 4 | let x = unsafe { &foo }; | ^^^^ shared reference to mutable static | = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> = note: this will be a hard error in the 2024 edition = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior = note: `#[warn(static_mut_refs)]` on by default help: use `addr_of!` instead to create a raw pointer | 4 | let x = unsafe { addr_of!(foo) }; | ~~~~~~~~~~~~~ [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 So I think we should start using `addr_of!` for mutable static now. > The safety comment is wrong though. > >> Also >> will this pointer be used to write to the static, in that case you need >> `_mut!`. > > Not in this version of the patch set, but potentially in future iterations. All the more reason to use `addr_of!` IMO. >>> + }}, >>> + ", >>> + name = info.name, >>> + param_name = param_name, >>> + ); >> >> What is the reason for putting `kparam` and `read_func` outside of the >> `write!` below? I think it would be easier to read if they are inlined. > > It had different shapes based on other options in the original patch > set. I guess I can just inline it in this version. > >> >>> + write!( >>> + self.param_buffer, >>> + " >>> + static mut __{name}_{param_name}_value: {param_type_internal} = {param_default}; >>> + >>> + pub(crate) struct __{name}_{param_name}; >>> + >>> + impl __{name}_{param_name} {{ {read_func} }} >>> + >>> + pub(crate) const {param_name}: __{name}_{param_name} = __{name}_{param_name}; >> >> Why do we need a unit struct as a constant? I think it would make more >> sense to have a unit struct/empty enum as the type and the `read` >> function be without a receiver. > > To be able to call `module_parameters::my_parameter.read()`. Other > options would be `module_parameters::my_parameter::read()` or > `module_parameters::my_parameter_read()`. > > I don't think there will be a difference in the generated machine code. > I also don't have any particular preference. Probably > `module_parameters::my_parameter::read()` is the most idiomatic one. Yeah, I would prefer if we can avoid having both a constant and a type. The type then also can be an empty enum, so no value can be constructed. >>> + >>> + // Note: the C macro that generates the static structs for the `__param` section >>> + // asks for them to be `aligned(sizeof(void *))`. However, that was put in place >>> + // in 2003 in commit 38d5b085d2a0 (\"[PATCH] Fix over-alignment problem on x86-64\") >>> + // to undo GCC over-alignment of static structs of >32 bytes. It seems that is >>> + // not the case anymore, so we simplify to a transparent representation here >>> + // in the expectation that it is not needed anymore. >>> + // TODO: Revisit this to confirm the above comment and remove it if it happened. >> >> Should this TODO be fixed before this is merged? Or do you intend for it >> to stay? >> If this is indeed correct, should this also be changed in the C side (of >> course a different patch)? > > I dug into this. The original code in this patch must be quite old, > because that the code the comment refers to was changed in Nov 2020 from > `aligned(sizeof(void *))` to `__aligned(__alignof__(struct > kernel_param))`. The commit message says that the rationale for not > removing the alignment completely is to prevent the compiler from > increasing the alignment, as this would mess up the array stride used in > the `__param` section. > > So I think we can remove the comment and keep `repr(transparent)`, right? > I think `rustc` would not increase the alignment of a `repr(C)` struct > for optimization purposes? I don't know that, maybe Gary or someone else knows how this works. >>> + /// 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 {{ >>> + }} >> >> Any reason to put the `}` on the next line? > > No. Do you have any tricks for formatting multi line strings of code like this? Not really, I don't think that this is a big deal, since this will eventually be replaced by `syn`, which can be formatted more easily. >>> + >>> + #[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(), >>> + // SAFETY: This static is actually constant as seen by >>> + // module code. But we need a unique address for it, so it >>> + // must be static. >> >> This safety comment makes no sense, should it be a normal comment? > > I removed the unsafe block and the safety comment as unsafe is not > required here. > >> >>> + ops: unsafe {{ &{ops} }} as *const kernel::bindings::kernel_param_ops, >> >> Why is this `unsafe` block needed, the `make_param_ops` macro declares a >> non-mut static. >> >>> + perm: 0, // Will not appear in sysfs >>> + level: -1, >> >> Why this value? > > The kernel has 8 initcall levels. Parameters can be assigned one of > these levels to have the parameter initialized just before the init > functions for that level are executed. -1 has no effect for loadable modules, but > for built-in modules it looks like the args will be initialized just after early > boot args (level 0). > > At any rate, this is what C side does. I see, I was just wondering where the magic value comes from (especially since the `perm` value has a comment explaining what it does). --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 01.08.24 13:29, Andreas Hindborg wrote: >> >> Hi Benno, >> >> Thanks for the comments! >> >> "Benno Lossin" <benno.lossin@proton.me> writes: >> >>> On 05.07.24 13:15, Andreas Hindborg wrote: >> >> [...] >> >>>> + >>>> +/// 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; >>>> + >>>> + /// Whether the parameter is allowed to be set without an argument. >>>> + /// >>>> + /// Setting this to `true` allows the parameter to be passed without an >>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>> + const NOARG_ALLOWED: bool; >>> >>> I think, there is a better way of doing this. Instead of this bool, we >>> do the following: >>> 1. have a `const DEFAULT: Option<Self>` >>> 2. change the type of the argument of `try_from_param_arg` to >>> `&'static [u8]` >>> >>> That way we don't have the weird behavior of `try_from_param_arg` that >>> for params that don't have a default value. >> >> Since we have no parameter types for which `NOARG_ALLOWED` is true in >> this patch set, it is effectively dead code. I will remove it. > > Hmm what parameters actually are optional? I looked at the old rust > branch and only `bool` is marked as optional. Are there others? > > If it is used commonly for custom parameters (I could imagine that Rust > modules have enums as parameters and specifying nothing could mean the > default value), then it might be a good idea to just include it now. > (otherwise we might forget the design later) As far as I can tell from the C code, all parameters are able to have the `NOARG` flag set. We get a null pointer in the callback in that case. If we want to handle this now, we could drop the `default` field in the Rust module macro. There is no equivalent in the C macros. And then use an `Option<Option<_>>` to represent the value. `None` would be an unset parameter. `Some(None)` would be a parameter without a value. `Some(Some(_))` would be a set parameter with a value. We could probably fix the types so that only parameters with the `NOARG` flag use the double option, others use a single option. Or we could just not adopt this feature in the Rust abstractions. > >>>> + >>>> + /// Convert a parameter argument into the parameter value. >>>> + /// >>>> + /// `None` should be returned when parsing of the argument fails. >>>> + /// `arg == None` indicates that the parameter was passed without an >>>> + /// argument. If `NOARG_ALLOWED` is set to `false` then `arg` is guaranteed >>>> + /// to always be `Some(_)`. > > [...] > >>>> diff --git a/rust/macros/helpers.rs b/rust/macros/helpers.rs >>>> index 563dcd2b7ace..dc0b47879a8c 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 get_string(it: &mut token_stream::IntoIter, expected_name: &str) -> String { >>> >>> This name is rather weird, `get_field` makes more sense IMO. >> >> Looking at this, the `get` prefix is not aligned with other helpers. How >> about `expect_string_field` ? > > SGTM > >>>> + assert_eq!(expect_ident(it), expected_name); >>>> + assert_eq!(expect_punct(it), ':'); >>>> + let string = expect_string(it); >>>> + assert_eq!(expect_punct(it), ','); >>> >>> Why do we require a trailing comma? >> >> For consistency with existing module macro. All keys must be terminated >> with comma. > > Hmm I think that might be a bit unexpected, since everywhere else in > Rust you are allowed to omit the trailing comma. But I guess if the > entire `module!` macro does that currently, then we can change that when > we move to `syn`. Yes, I agree. > > [...] > >>>> + param_type.to_string(), >>>> + param_ops_path(¶m_type).to_string(), >>>> + ); >>>> + >>>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); >>> >>> Is the spelling intentional? "parmtype"? >> >> This is intentional. I don't think the kernel is ever parsing this, but >> it is parsed by the `modinfo` tool. > > Hmm, why is it not `paramtype`? Does that conflict with something? You would have to take that up with the maintainer(s) of the `modinfo` tool. The name is externally dictated [1]. > >>>> + self.emit_param("parm", ¶m_name, ¶m_description); >>>> + let param_type_internal = param_type.clone(); >>>> + >>>> + let read_func = format!( >>>> + " >>>> + pub(crate) fn read(&self) >>>> + -> &<{param_type_internal} >>>> + as kernel::module_param::ModuleParam>::Value {{ >>> >>> Please add a `::` in front of `kernel::module_param::ModuleParam`. There >>> are more instances below. >> >> Thanks. >> >>> >>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>> + &__{name}_{param_name}_value >>>> + ) >>>> + }} >>>> + }} >>>> + ", >>>> + name = info.name, >>>> + param_name = param_name, >>>> + param_type_internal = param_type_internal, >>>> + ); >>>> + >>>> + let kparam = format!( >>>> + " >>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>> + // SAFETY: Access through the resulting pointer is >>>> + // serialized by C side and only happens before module >>>> + // `init` or after module `drop` is called. >>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>> + as *const _ as *mut core::ffi::c_void, >>> >>> Here you should use `addr_of[_mut]!` instead of taking a reference. >> >> This is a static initializer, so it would be evaluated in const context. >> At that time, this is going to be the only reference to >> `&__{name}_{param_name}_value` which would be const. So it should be >> fine? > > When compiling this [1] with a sufficiently new Rust version, you will > get an error: > > warning: creating a shared reference to mutable static is discouraged > --> src/main.rs:4:22 > | > 4 | let x = unsafe { &foo }; > | ^^^^ shared reference to mutable static > | > = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> > = note: this will be a hard error in the 2024 edition > = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior > = note: `#[warn(static_mut_refs)]` on by default > help: use `addr_of!` instead to create a raw pointer > | > 4 | let x = unsafe { addr_of!(foo) }; > | ~~~~~~~~~~~~~ > > [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 > > So I think we should start using `addr_of!` for mutable static now. Oh. Thanks for the pointer. Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes away as well with the feature you linked as well. This also requires `const_mut_refs`, but as I recall that is going to be stabilized soon. > >> The safety comment is wrong though. >> >>> Also >>> will this pointer be used to write to the static, in that case you need >>> `_mut!`. >> >> Not in this version of the patch set, but potentially in future iterations. > > All the more reason to use `addr_of!` IMO. > >>>> + }}, >>>> + ", >>>> + name = info.name, >>>> + param_name = param_name, >>>> + ); >>> >>> What is the reason for putting `kparam` and `read_func` outside of the >>> `write!` below? I think it would be easier to read if they are inlined. >> >> It had different shapes based on other options in the original patch >> set. I guess I can just inline it in this version. >> >>> >>>> + write!( >>>> + self.param_buffer, >>>> + " >>>> + static mut __{name}_{param_name}_value: {param_type_internal} = {param_default}; >>>> + >>>> + pub(crate) struct __{name}_{param_name}; >>>> + >>>> + impl __{name}_{param_name} {{ {read_func} }} >>>> + >>>> + pub(crate) const {param_name}: __{name}_{param_name} = __{name}_{param_name}; >>> >>> Why do we need a unit struct as a constant? I think it would make more >>> sense to have a unit struct/empty enum as the type and the `read` >>> function be without a receiver. >> >> To be able to call `module_parameters::my_parameter.read()`. Other >> options would be `module_parameters::my_parameter::read()` or >> `module_parameters::my_parameter_read()`. >> >> I don't think there will be a difference in the generated machine code. >> I also don't have any particular preference. Probably >> `module_parameters::my_parameter::read()` is the most idiomatic one. > > Yeah, I would prefer if we can avoid having both a constant and a type. > The type then also can be an empty enum, so no value can be constructed. Nice trick
On 01.08.24 15:40, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: >> On 01.08.24 13:29, Andreas Hindborg wrote: >>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>> + >>>>> +/// 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; >>>>> + >>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>> + /// >>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>> + const NOARG_ALLOWED: bool; >>>> >>>> I think, there is a better way of doing this. Instead of this bool, we >>>> do the following: >>>> 1. have a `const DEFAULT: Option<Self>` >>>> 2. change the type of the argument of `try_from_param_arg` to >>>> `&'static [u8]` >>>> >>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>> for params that don't have a default value. >>> >>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>> this patch set, it is effectively dead code. I will remove it. >> >> Hmm what parameters actually are optional? I looked at the old rust >> branch and only `bool` is marked as optional. Are there others? >> >> If it is used commonly for custom parameters (I could imagine that Rust >> modules have enums as parameters and specifying nothing could mean the >> default value), then it might be a good idea to just include it now. >> (otherwise we might forget the design later) > > As far as I can tell from the C code, all parameters are able to have > the `NOARG` flag set. We get a null pointer in the callback in that > case. > > If we want to handle this now, we could drop the `default` field > in the Rust module macro. There is no equivalent in the C macros. > And then use an `Option<Option<_>>` to represent the value. `None` would > be an unset parameter. `Some(None)` would be a parameter without a > value. `Some(Some(_))` would be a set parameter with a value. We could > probably fix the types so that only parameters with the `NOARG` flag use > the double option, others use a single option. What did you think of my approach that I detailed above? I would like to avoid `Option<Option<_>>` if we can. > Or we could just not adopt this feature in the Rust abstractions. Depends on how common this is and if people need to use it. I think that what I proposed above isn't that complex, so it should be easy to implement. >>>>> + param_type.to_string(), >>>>> + param_ops_path(¶m_type).to_string(), >>>>> + ); >>>>> + >>>>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); >>>> >>>> Is the spelling intentional? "parmtype"? >>> >>> This is intentional. I don't think the kernel is ever parsing this, but >>> it is parsed by the `modinfo` tool. >> >> Hmm, why is it not `paramtype`? Does that conflict with something? > > You would have to take that up with the maintainer(s) of the `modinfo` > tool. The name is externally dictated [1]. Thanks for the pointer that's what I wanted to know (is it given from somewhere else? or is it a name that we came up with), then it's fine :) >>>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>>> + &__{name}_{param_name}_value >>>>> + ) >>>>> + }} >>>>> + }} >>>>> + ", >>>>> + name = info.name, >>>>> + param_name = param_name, >>>>> + param_type_internal = param_type_internal, >>>>> + ); >>>>> + >>>>> + let kparam = format!( >>>>> + " >>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>>> + // SAFETY: Access through the resulting pointer is >>>>> + // serialized by C side and only happens before module >>>>> + // `init` or after module `drop` is called. >>>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>>> + as *const _ as *mut core::ffi::c_void, >>>> >>>> Here you should use `addr_of[_mut]!` instead of taking a reference. >>> >>> This is a static initializer, so it would be evaluated in const context. >>> At that time, this is going to be the only reference to >>> `&__{name}_{param_name}_value` which would be const. So it should be >>> fine? >> >> When compiling this [1] with a sufficiently new Rust version, you will >> get an error: >> >> warning: creating a shared reference to mutable static is discouraged >> --> src/main.rs:4:22 >> | >> 4 | let x = unsafe { &foo }; >> | ^^^^ shared reference to mutable static >> | >> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> >> = note: this will be a hard error in the 2024 edition >> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior >> = note: `#[warn(static_mut_refs)]` on by default >> help: use `addr_of!` instead to create a raw pointer >> | >> 4 | let x = unsafe { addr_of!(foo) }; >> | ~~~~~~~~~~~~~ >> >> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 >> >> So I think we should start using `addr_of!` for mutable static now. > > Oh. Thanks for the pointer. > > Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes > away as well with the feature you linked as well. I think that will take some time until it is gone. > This also requires `const_mut_refs`, but as I recall that is going to be > stabilized soon. That should only be needed if you need `addr_of_mut!`, but IIUC, you only need `addr_of!`, right? --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 01.08.24 15:40, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@proton.me> writes: >>> On 01.08.24 13:29, Andreas Hindborg wrote: >>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>>> + >>>>>> +/// 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; >>>>>> + >>>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>>> + /// >>>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>>> + const NOARG_ALLOWED: bool; >>>>> >>>>> I think, there is a better way of doing this. Instead of this bool, we >>>>> do the following: >>>>> 1. have a `const DEFAULT: Option<Self>` >>>>> 2. change the type of the argument of `try_from_param_arg` to >>>>> `&'static [u8]` >>>>> >>>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>>> for params that don't have a default value. >>>> >>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>>> this patch set, it is effectively dead code. I will remove it. >>> >>> Hmm what parameters actually are optional? I looked at the old rust >>> branch and only `bool` is marked as optional. Are there others? >>> >>> If it is used commonly for custom parameters (I could imagine that Rust >>> modules have enums as parameters and specifying nothing could mean the >>> default value), then it might be a good idea to just include it now. >>> (otherwise we might forget the design later) >> >> As far as I can tell from the C code, all parameters are able to have >> the `NOARG` flag set. We get a null pointer in the callback in that >> case. >> >> If we want to handle this now, we could drop the `default` field >> in the Rust module macro. There is no equivalent in the C macros. >> And then use an `Option<Option<_>>` to represent the value. `None` would >> be an unset parameter. `Some(None)` would be a parameter without a >> value. `Some(Some(_))` would be a set parameter with a value. We could >> probably fix the types so that only parameters with the `NOARG` flag use >> the double option, others use a single option. > > What did you think of my approach that I detailed above? I would like to > avoid `Option<Option<_>>` if we can. How would you represent the case when the parameter is passed without a value and a default is given in `module!`? I think we need to drop the default value if we adopt the arg without value scheme. > >> Or we could just not adopt this feature in the Rust abstractions. > > Depends on how common this is and if people need to use it. I think that > what I proposed above isn't that complex, so it should be easy to > implement. Rust modules would just force people to add "my_module.param=1" instead of just "my_module.param". I think that is reasonable. > >>>>>> + param_type.to_string(), >>>>>> + param_ops_path(¶m_type).to_string(), >>>>>> + ); >>>>>> + >>>>>> + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); >>>>> >>>>> Is the spelling intentional? "parmtype"? >>>> >>>> This is intentional. I don't think the kernel is ever parsing this, but >>>> it is parsed by the `modinfo` tool. >>> >>> Hmm, why is it not `paramtype`? Does that conflict with something? >> >> You would have to take that up with the maintainer(s) of the `modinfo` >> tool. The name is externally dictated [1]. > > Thanks for the pointer that's what I wanted to know (is it given from > somewhere else? or is it a name that we came up with), then it's fine :) > >>>>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>>>> + &__{name}_{param_name}_value >>>>>> + ) >>>>>> + }} >>>>>> + }} >>>>>> + ", >>>>>> + name = info.name, >>>>>> + param_name = param_name, >>>>>> + param_type_internal = param_type_internal, >>>>>> + ); >>>>>> + >>>>>> + let kparam = format!( >>>>>> + " >>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>>>> + // SAFETY: Access through the resulting pointer is >>>>>> + // serialized by C side and only happens before module >>>>>> + // `init` or after module `drop` is called. >>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>>>> + as *const _ as *mut core::ffi::c_void, >>>>> >>>>> Here you should use `addr_of[_mut]!` instead of taking a reference. >>>> >>>> This is a static initializer, so it would be evaluated in const context. >>>> At that time, this is going to be the only reference to >>>> `&__{name}_{param_name}_value` which would be const. So it should be >>>> fine? >>> >>> When compiling this [1] with a sufficiently new Rust version, you will >>> get an error: >>> >>> warning: creating a shared reference to mutable static is discouraged >>> --> src/main.rs:4:22 >>> | >>> 4 | let x = unsafe { &foo }; >>> | ^^^^ shared reference to mutable static >>> | >>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> >>> = note: this will be a hard error in the 2024 edition >>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior >>> = note: `#[warn(static_mut_refs)]` on by default >>> help: use `addr_of!` instead to create a raw pointer >>> | >>> 4 | let x = unsafe { addr_of!(foo) }; >>> | ~~~~~~~~~~~~~ >>> >>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 >>> >>> So I think we should start using `addr_of!` for mutable static now. >> >> Oh. Thanks for the pointer. >> >> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes >> away as well with the feature you linked as well. > > I think that will take some time until it is gone. > >> This also requires `const_mut_refs`, but as I recall that is going to be >> stabilized soon. > > That should only be needed if you need `addr_of_mut!`, but IIUC, you > only need `addr_of!`, right? The pointer we create here is the one passed to `free` in module_param.rs, so it will eventually be used as `&mut T`. Best regards, Andreas
On 01.08.24 17:11, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: >> On 01.08.24 15:40, Andreas Hindborg wrote: >>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>> On 01.08.24 13:29, Andreas Hindborg wrote: >>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>>>> + >>>>>>> +/// 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; >>>>>>> + >>>>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>>>> + /// >>>>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>>>> + const NOARG_ALLOWED: bool; >>>>>> >>>>>> I think, there is a better way of doing this. Instead of this bool, we >>>>>> do the following: >>>>>> 1. have a `const DEFAULT: Option<Self>` >>>>>> 2. change the type of the argument of `try_from_param_arg` to >>>>>> `&'static [u8]` >>>>>> >>>>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>>>> for params that don't have a default value. >>>>> >>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>>>> this patch set, it is effectively dead code. I will remove it. >>>> >>>> Hmm what parameters actually are optional? I looked at the old rust >>>> branch and only `bool` is marked as optional. Are there others? >>>> >>>> If it is used commonly for custom parameters (I could imagine that Rust >>>> modules have enums as parameters and specifying nothing could mean the >>>> default value), then it might be a good idea to just include it now. >>>> (otherwise we might forget the design later) >>> >>> As far as I can tell from the C code, all parameters are able to have >>> the `NOARG` flag set. We get a null pointer in the callback in that >>> case. >>> >>> If we want to handle this now, we could drop the `default` field >>> in the Rust module macro. There is no equivalent in the C macros. >>> And then use an `Option<Option<_>>` to represent the value. `None` would >>> be an unset parameter. `Some(None)` would be a parameter without a >>> value. `Some(Some(_))` would be a set parameter with a value. We could >>> probably fix the types so that only parameters with the `NOARG` flag use >>> the double option, others use a single option. >> >> What did you think of my approach that I detailed above? I would like to >> avoid `Option<Option<_>>` if we can. > > How would you represent the case when the parameter is passed without a > value and a default is given in `module!`? I am a bit confused, there are two default values here: (1) the value returned by `try_from_param_arg(None)`. (2) the value given by the user to the `module!` macro. I am talking about changing the definition of ModuleParam, so (1). I get the feeling that you are talking about (2), is that correct? > I think we need to drop the default value if we adopt the arg without > value scheme. Yes definitely. I don't see anything in the code doing this currently, right? We could also only allow `Copy` values to be used as Parameters (but then `str` cannot be used as a parameter...), since they can't implement `Drop`. >>> Or we could just not adopt this feature in the Rust abstractions. >> >> Depends on how common this is and if people need to use it. I think that >> what I proposed above isn't that complex, so it should be easy to >> implement. > > Rust modules would just force people to add "my_module.param=1" instead > of just "my_module.param". I think that is reasonable. Eh, why do we want to give that up? I don't think it's difficult to do. >>>>>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>>>>> + &__{name}_{param_name}_value >>>>>>> + ) >>>>>>> + }} >>>>>>> + }} >>>>>>> + ", >>>>>>> + name = info.name, >>>>>>> + param_name = param_name, >>>>>>> + param_type_internal = param_type_internal, >>>>>>> + ); >>>>>>> + >>>>>>> + let kparam = format!( >>>>>>> + " >>>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>>>>> + // SAFETY: Access through the resulting pointer is >>>>>>> + // serialized by C side and only happens before module >>>>>>> + // `init` or after module `drop` is called. >>>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>>>>> + as *const _ as *mut core::ffi::c_void, >>>>>> >>>>>> Here you should use `addr_of[_mut]!` instead of taking a reference. >>>>> >>>>> This is a static initializer, so it would be evaluated in const context. >>>>> At that time, this is going to be the only reference to >>>>> `&__{name}_{param_name}_value` which would be const. So it should be >>>>> fine? >>>> >>>> When compiling this [1] with a sufficiently new Rust version, you will >>>> get an error: >>>> >>>> warning: creating a shared reference to mutable static is discouraged >>>> --> src/main.rs:4:22 >>>> | >>>> 4 | let x = unsafe { &foo }; >>>> | ^^^^ shared reference to mutable static >>>> | >>>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> >>>> = note: this will be a hard error in the 2024 edition >>>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior >>>> = note: `#[warn(static_mut_refs)]` on by default >>>> help: use `addr_of!` instead to create a raw pointer >>>> | >>>> 4 | let x = unsafe { addr_of!(foo) }; >>>> | ~~~~~~~~~~~~~ >>>> >>>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 >>>> >>>> So I think we should start using `addr_of!` for mutable static now. >>> >>> Oh. Thanks for the pointer. >>> >>> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes >>> away as well with the feature you linked as well. >> >> I think that will take some time until it is gone. >> >>> This also requires `const_mut_refs`, but as I recall that is going to be >>> stabilized soon. >> >> That should only be needed if you need `addr_of_mut!`, but IIUC, you >> only need `addr_of!`, right? > > The pointer we create here is the one passed to `free` in > module_param.rs, so it will eventually be used as `&mut T`. Oh then the original code is definitely wrong, since it creates a shared reference. Yeah then you should use `addr_of_mut!`. --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 01.08.24 17:11, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@proton.me> writes: >>> On 01.08.24 15:40, Andreas Hindborg wrote: >>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>> On 01.08.24 13:29, Andreas Hindborg wrote: >>>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>>>>> + >>>>>>>> +/// 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; >>>>>>>> + >>>>>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>>>>> + /// >>>>>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>>>>> + const NOARG_ALLOWED: bool; >>>>>>> >>>>>>> I think, there is a better way of doing this. Instead of this bool, we >>>>>>> do the following: >>>>>>> 1. have a `const DEFAULT: Option<Self>` >>>>>>> 2. change the type of the argument of `try_from_param_arg` to >>>>>>> `&'static [u8]` >>>>>>> >>>>>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>>>>> for params that don't have a default value. >>>>>> >>>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>>>>> this patch set, it is effectively dead code. I will remove it. >>>>> >>>>> Hmm what parameters actually are optional? I looked at the old rust >>>>> branch and only `bool` is marked as optional. Are there others? >>>>> >>>>> If it is used commonly for custom parameters (I could imagine that Rust >>>>> modules have enums as parameters and specifying nothing could mean the >>>>> default value), then it might be a good idea to just include it now. >>>>> (otherwise we might forget the design later) >>>> >>>> As far as I can tell from the C code, all parameters are able to have >>>> the `NOARG` flag set. We get a null pointer in the callback in that >>>> case. >>>> >>>> If we want to handle this now, we could drop the `default` field >>>> in the Rust module macro. There is no equivalent in the C macros. >>>> And then use an `Option<Option<_>>` to represent the value. `None` would >>>> be an unset parameter. `Some(None)` would be a parameter without a >>>> value. `Some(Some(_))` would be a set parameter with a value. We could >>>> probably fix the types so that only parameters with the `NOARG` flag use >>>> the double option, others use a single option. >>> >>> What did you think of my approach that I detailed above? I would like to >>> avoid `Option<Option<_>>` if we can. >> >> How would you represent the case when the parameter is passed without a >> value and a default is given in `module!`? > > I am a bit confused, there are two default values here: > (1) the value returned by `try_from_param_arg(None)`. > (2) the value given by the user to the `module!` macro. > > I am talking about changing the definition of ModuleParam, so (1). I get > the feeling that you are talking about (2), is that correct? I confused myself as well I think. I am talking about (1). Let me try again. If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I should change the flag name in Rust), modules can optionally support some parameters where users can pass parameters either as `my_module.param=value` or `my_module.param`. Thus, at the level of `try_from_param_arg`, we need to represent two cases: parameter passed without value, and parameter passed with value. A third case, parameter not passed at all, is equivalent to `try_from_param_arg` never being called. In C this is undetectable for the predefined parameter types. I wanted the double option to detect this. But I guess it does not make sense. At a higher level where the bindings supply the parsing functions, we can decide that passing an argument without a value yields a default parameter value. C does this for the predefined `bool` type. The predefined integer types does not support omitting the value. This patch only supports the higher level predefined parameter types, and does not allow modules to supply their own parameter parsing functions. None of the types we implement in this patch support passing the argument without a value. This is intentional to mirror the C implementation. To that end, I removed `NOARG_ALLOWED`, and changed the parsing function trait to: fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; If/when we start supporting types like `bool` or custom parsing functions provided by the module, we will have to update the signature to take an `Option` to represent the case where the user passed an argument without a value. However, to mimic C, the function must always return a value if successful, even if the user did not supply a value to the argument. Two different default values are in flight here. 1) the value that the parameter will have before the kernel calls `try_from_param_arg` via `set_param` and 2) the value to return from `try_from_param_arg` if the user did not pass a value with the argument. For a `bool` 1) would usually be `false` and 2) would always be `true`. For predefined types the module would not customize 2), but 1) is useful to customize. For custom types where the module supplies the parsing function, 2) would be implicitly given by the module in the parsing function. In this patch set, we only have 1 default value, namely 1). We do not need 2) because we do not support parameters without values. > >> I think we need to drop the default value if we adopt the arg without >> value scheme. > > Yes definitely. I don't see anything in the code doing this currently, > right? The current patch uses the default value given in the `module!` macro to initialize the parameter value. > We could also only allow `Copy` values to be used as Parameters (but > then `str` cannot be used as a parameter...), since they can't implement > `Drop`. We should plan on eventually supporting `String`. > >>>> Or we could just not adopt this feature in the Rust abstractions. >>> >>> Depends on how common this is and if people need to use it. I think that >>> what I proposed above isn't that complex, so it should be easy to >>> implement. >> >> Rust modules would just force people to add "my_module.param=1" instead >> of just "my_module.param". I think that is reasonable. > > Eh, why do we want to give that up? I don't think it's difficult to do. I just don't see the point. Just have the user pass `my_module.param=1` instead of omitting the value. Having multiple ways of specifying for instance a true boolean just leads to confusion. Some boolean parameters have a default value of `true`, for instance `nvme.use_cmb_sqes`. In this case specifying `nvme.use_cmb_sqes` has no effect, even though one might think it has. Of course, if we are going to do things the same as C, we have to support it. > >>>>>>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>>>>>> + &__{name}_{param_name}_value >>>>>>>> + ) >>>>>>>> + }} >>>>>>>> + }} >>>>>>>> + ", >>>>>>>> + name = info.name, >>>>>>>> + param_name = param_name, >>>>>>>> + param_type_internal = param_type_internal, >>>>>>>> + ); >>>>>>>> + >>>>>>>> + let kparam = format!( >>>>>>>> + " >>>>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>>>>>> + // SAFETY: Access through the resulting pointer is >>>>>>>> + // serialized by C side and only happens before module >>>>>>>> + // `init` or after module `drop` is called. >>>>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>>>>>> + as *const _ as *mut core::ffi::c_void, >>>>>>> >>>>>>> Here you should use `addr_of[_mut]!` instead of taking a reference. >>>>>> >>>>>> This is a static initializer, so it would be evaluated in const context. >>>>>> At that time, this is going to be the only reference to >>>>>> `&__{name}_{param_name}_value` which would be const. So it should be >>>>>> fine? >>>>> >>>>> When compiling this [1] with a sufficiently new Rust version, you will >>>>> get an error: >>>>> >>>>> warning: creating a shared reference to mutable static is discouraged >>>>> --> src/main.rs:4:22 >>>>> | >>>>> 4 | let x = unsafe { &foo }; >>>>> | ^^^^ shared reference to mutable static >>>>> | >>>>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> >>>>> = note: this will be a hard error in the 2024 edition >>>>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior >>>>> = note: `#[warn(static_mut_refs)]` on by default >>>>> help: use `addr_of!` instead to create a raw pointer >>>>> | >>>>> 4 | let x = unsafe { addr_of!(foo) }; >>>>> | ~~~~~~~~~~~~~ >>>>> >>>>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 >>>>> >>>>> So I think we should start using `addr_of!` for mutable static now. >>>> >>>> Oh. Thanks for the pointer. >>>> >>>> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes >>>> away as well with the feature you linked as well. >>> >>> I think that will take some time until it is gone. >>> >>>> This also requires `const_mut_refs`, but as I recall that is going to be >>>> stabilized soon. >>> >>> That should only be needed if you need `addr_of_mut!`, but IIUC, you >>> only need `addr_of!`, right? >> >> The pointer we create here is the one passed to `free` in >> module_param.rs, so it will eventually be used as `&mut T`. > > Oh then the original code is definitely wrong, since it creates a shared > reference. Yeah then you should use `addr_of_mut!`. Right. I agree the right thing is to change to `addr_of_mut!`. But I am curious. If the original code was ```rust arg: unsafe {{ &mut __{name}_{param_name}_value }} as *mut _ as *mut ::core::ffi::c_void, ``` Then it would be fine? Because we have the only mutable reference in existence when the code is evaluated. Best regards, Andreas
On 02.08.24 12:27, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: >> On 01.08.24 17:11, Andreas Hindborg wrote: >>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>> On 01.08.24 15:40, Andreas Hindborg wrote: >>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>> On 01.08.24 13:29, Andreas Hindborg wrote: >>>>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>>>>>> + >>>>>>>>> +/// 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; >>>>>>>>> + >>>>>>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>>>>>> + /// >>>>>>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>>>>>> + const NOARG_ALLOWED: bool; >>>>>>>> >>>>>>>> I think, there is a better way of doing this. Instead of this bool, we >>>>>>>> do the following: >>>>>>>> 1. have a `const DEFAULT: Option<Self>` >>>>>>>> 2. change the type of the argument of `try_from_param_arg` to >>>>>>>> `&'static [u8]` >>>>>>>> >>>>>>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>>>>>> for params that don't have a default value. >>>>>>> >>>>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>>>>>> this patch set, it is effectively dead code. I will remove it. >>>>>> >>>>>> Hmm what parameters actually are optional? I looked at the old rust >>>>>> branch and only `bool` is marked as optional. Are there others? >>>>>> >>>>>> If it is used commonly for custom parameters (I could imagine that Rust >>>>>> modules have enums as parameters and specifying nothing could mean the >>>>>> default value), then it might be a good idea to just include it now. >>>>>> (otherwise we might forget the design later) >>>>> >>>>> As far as I can tell from the C code, all parameters are able to have >>>>> the `NOARG` flag set. We get a null pointer in the callback in that >>>>> case. >>>>> >>>>> If we want to handle this now, we could drop the `default` field >>>>> in the Rust module macro. There is no equivalent in the C macros. >>>>> And then use an `Option<Option<_>>` to represent the value. `None` would >>>>> be an unset parameter. `Some(None)` would be a parameter without a >>>>> value. `Some(Some(_))` would be a set parameter with a value. We could >>>>> probably fix the types so that only parameters with the `NOARG` flag use >>>>> the double option, others use a single option. >>>> >>>> What did you think of my approach that I detailed above? I would like to >>>> avoid `Option<Option<_>>` if we can. >>> >>> How would you represent the case when the parameter is passed without a >>> value and a default is given in `module!`? >> >> I am a bit confused, there are two default values here: >> (1) the value returned by `try_from_param_arg(None)`. >> (2) the value given by the user to the `module!` macro. >> >> I am talking about changing the definition of ModuleParam, so (1). I get >> the feeling that you are talking about (2), is that correct? > > I confused myself as well I think. I am talking about (1). Let me try > again. > > If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I > should change the flag name in Rust), modules can optionally support > some parameters where users can pass parameters either as > `my_module.param=value` or `my_module.param`. Thus, at the level of > `try_from_param_arg`, we need to represent two cases: parameter passed > without value, and parameter passed with value. A third case, parameter > not passed at all, is equivalent to `try_from_param_arg` never being > called. In C this is undetectable for the predefined parameter types. I > wanted the double option to detect this. But I guess it does not make > sense. My idea was to have an `const DEFAULT: Option<Self>` to represent the following: (1) if `DEFAULT == None`, then `KERNEL_PARAM_OPS_FL_NOARG` is not set and thus either the `module!` user-specified default value is used, or it is specified via `my_module.param=value` and `try_from_param_arg` is called. (2) if `DEFAULT == Some(d)`, then `KERNEL_PARAM_OPS_FL_NOARG` is set and when `NULL` is given to `kernel_param_ops.set`, the parameter value is set to `d`, otherwise `try_from_param_arg` is called. But I think I agree with you removing `NOARG_ALLOWED`, see below. > At a higher level where the bindings supply the parsing functions, we > can decide that passing an argument without a value yields a default > parameter value. C does this for the predefined `bool` type. The > predefined integer types does not support omitting the value. > > This patch only supports the higher level predefined parameter types, > and does not allow modules to supply their own parameter parsing > functions. None of the types we implement in this patch support passing > the argument without a value. This is intentional to mirror the C > implementation. > > To that end, I removed `NOARG_ALLOWED`, and changed the parsing function > trait to: > > fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; > > If/when we start supporting types like `bool` or custom parsing > functions provided by the module, we will have to update the signature > to take an `Option` to represent the case where the user passed an > argument without a value. However, to mimic C, the function must always > return a value if successful, even if the user did not supply a value to > the argument. > > Two different default values are in flight here. 1) the value that the > parameter will have before the kernel calls `try_from_param_arg` via > `set_param` and 2) the value to return from `try_from_param_arg` if the > user did not pass a value with the argument. > > For a `bool` 1) would usually be `false` and 2) would always be `true`. > > For predefined types the module would not customize 2), but 1) is useful > to customize. For custom types where the module supplies the parsing > function, 2) would be implicitly given by the module in the parsing > function. > > In this patch set, we only have 1 default value, namely 1). We do not > need 2) because we do not support parameters without values. I am not sure that putting the default value of `my_module.param` into the `ModuleParam` trait is a good idea. It feels more correct to me to add an optional field to the part in `module!` that can be set to denote this default value -- we might also want to change the name of `default`, what do you think of `default_inactive` and `default_active`? Since one might want an option by default be `true` and when one writes `my_module.param`, it should be `false`. Also as the C side shows, having default values for integer types is not really a good idea, since you might want a non-zero default value. If one does not specify the `default_active` value, then the `KERNEL_PARAM_OPS_FL_NOARG` is not set. If you don't want to implement this (which I can fully understand, since we might get `syn` before anyone needs params with default values), then we should write this idea down (maybe in an issue?). But regardless, I would like to know your opinion on this topic. >>> I think we need to drop the default value if we adopt the arg without >>> value scheme. >> >> Yes definitely. I don't see anything in the code doing this currently, >> right? > > The current patch uses the default value given in the `module!` macro to > initialize the parameter value. But what drops the default value, when an actual value is specified via `my_module.param=value`? (or is the default value only assigned when nothing is specified?) >> We could also only allow `Copy` values to be used as Parameters (but >> then `str` cannot be used as a parameter...), since they can't implement >> `Drop`. > > We should plan on eventually supporting `String`. Yes. >>>>> Or we could just not adopt this feature in the Rust abstractions. >>>> >>>> Depends on how common this is and if people need to use it. I think that >>>> what I proposed above isn't that complex, so it should be easy to >>>> implement. >>> >>> Rust modules would just force people to add "my_module.param=1" instead >>> of just "my_module.param". I think that is reasonable. >> >> Eh, why do we want to give that up? I don't think it's difficult to do. > > I just don't see the point. Just have the user pass `my_module.param=1` > instead of omitting the value. Having multiple ways of specifying for > instance a true boolean just leads to confusion. Some boolean parameters > have a default value of `true`, for instance `nvme.use_cmb_sqes`. In > this case specifying `nvme.use_cmb_sqes` has no effect, even though one > might think it has. This just shows to me that a "global" default in `ModuleParam` is wrong, since for `use_cmb_sqes` one could either have a negated flag, or no default value, forcing the user to write `nvme.use_cmb_sqes=false`. > Of course, if we are going to do things the same as C, we have to > support it. I think eventually this will be useful, but as you said it's not a feature that you *need*. >>>>>>>>> + // 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_internal} as kernel::module_param::ModuleParam>::value( >>>>>>>>> + &__{name}_{param_name}_value >>>>>>>>> + ) >>>>>>>>> + }} >>>>>>>>> + }} >>>>>>>>> + ", >>>>>>>>> + name = info.name, >>>>>>>>> + param_name = param_name, >>>>>>>>> + param_type_internal = param_type_internal, >>>>>>>>> + ); >>>>>>>>> + >>>>>>>>> + let kparam = format!( >>>>>>>>> + " >>>>>>>>> + kernel::bindings::kernel_param__bindgen_ty_1 {{ >>>>>>>>> + // SAFETY: Access through the resulting pointer is >>>>>>>>> + // serialized by C side and only happens before module >>>>>>>>> + // `init` or after module `drop` is called. >>>>>>>>> + arg: unsafe {{ &__{name}_{param_name}_value }} >>>>>>>>> + as *const _ as *mut core::ffi::c_void, >>>>>>>> >>>>>>>> Here you should use `addr_of[_mut]!` instead of taking a reference. >>>>>>> >>>>>>> This is a static initializer, so it would be evaluated in const context. >>>>>>> At that time, this is going to be the only reference to >>>>>>> `&__{name}_{param_name}_value` which would be const. So it should be >>>>>>> fine? >>>>>> >>>>>> When compiling this [1] with a sufficiently new Rust version, you will >>>>>> get an error: >>>>>> >>>>>> warning: creating a shared reference to mutable static is discouraged >>>>>> --> src/main.rs:4:22 >>>>>> | >>>>>> 4 | let x = unsafe { &foo }; >>>>>> | ^^^^ shared reference to mutable static >>>>>> | >>>>>> = note: for more information, see issue #114447 <https://github.com/rust-lang/rust/issues/114447> >>>>>> = note: this will be a hard error in the 2024 edition >>>>>> = note: this shared reference has lifetime `'static`, but if the static ever gets mutated, or a mutable reference is created, then any further use of this shared reference is Undefined Behavior >>>>>> = note: `#[warn(static_mut_refs)]` on by default >>>>>> help: use `addr_of!` instead to create a raw pointer >>>>>> | >>>>>> 4 | let x = unsafe { addr_of!(foo) }; >>>>>> | ~~~~~~~~~~~~~ >>>>>> >>>>>> [1]: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=c914a438938be6f5fc643ee277efa1d1 >>>>>> >>>>>> So I think we should start using `addr_of!` for mutable static now. >>>>> >>>>> Oh. Thanks for the pointer. >>>>> >>>>> Hmm, `addr_of_mut!` still requires the unsafe block. Hopefully that goes >>>>> away as well with the feature you linked as well. >>>> >>>> I think that will take some time until it is gone. >>>> >>>>> This also requires `const_mut_refs`, but as I recall that is going to be >>>>> stabilized soon. >>>> >>>> That should only be needed if you need `addr_of_mut!`, but IIUC, you >>>> only need `addr_of!`, right? >>> >>> The pointer we create here is the one passed to `free` in >>> module_param.rs, so it will eventually be used as `&mut T`. >> >> Oh then the original code is definitely wrong, since it creates a shared >> reference. Yeah then you should use `addr_of_mut!`. > > Right. I agree the right thing is to change to `addr_of_mut!`. But I am > curious. If the original code was > > ```rust > arg: unsafe {{ &mut __{name}_{param_name}_value }} as *mut _ as *mut ::core::ffi::c_void, > ``` > > Then it would be fine? Because we have the only mutable reference in > existence when the code is evaluated. That *might* be fine, but I don't know if there is anything that would guarantee you that it is. Note that the code above uses a shared reference, which definitely isn't OK. --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 02.08.24 12:27, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@proton.me> writes: >>> On 01.08.24 17:11, Andreas Hindborg wrote: >>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>> On 01.08.24 15:40, Andreas Hindborg wrote: >>>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>>> On 01.08.24 13:29, Andreas Hindborg wrote: >>>>>>>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>>>>>>> On 05.07.24 13:15, Andreas Hindborg wrote: >>>>>>>>>> + >>>>>>>>>> +/// 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; >>>>>>>>>> + >>>>>>>>>> + /// Whether the parameter is allowed to be set without an argument. >>>>>>>>>> + /// >>>>>>>>>> + /// Setting this to `true` allows the parameter to be passed without an >>>>>>>>>> + /// argument (e.g. just `module.param` instead of `module.param=foo`). >>>>>>>>>> + const NOARG_ALLOWED: bool; >>>>>>>>> >>>>>>>>> I think, there is a better way of doing this. Instead of this bool, we >>>>>>>>> do the following: >>>>>>>>> 1. have a `const DEFAULT: Option<Self>` >>>>>>>>> 2. change the type of the argument of `try_from_param_arg` to >>>>>>>>> `&'static [u8]` >>>>>>>>> >>>>>>>>> That way we don't have the weird behavior of `try_from_param_arg` that >>>>>>>>> for params that don't have a default value. >>>>>>>> >>>>>>>> Since we have no parameter types for which `NOARG_ALLOWED` is true in >>>>>>>> this patch set, it is effectively dead code. I will remove it. >>>>>>> >>>>>>> Hmm what parameters actually are optional? I looked at the old rust >>>>>>> branch and only `bool` is marked as optional. Are there others? >>>>>>> >>>>>>> If it is used commonly for custom parameters (I could imagine that Rust >>>>>>> modules have enums as parameters and specifying nothing could mean the >>>>>>> default value), then it might be a good idea to just include it now. >>>>>>> (otherwise we might forget the design later) >>>>>> >>>>>> As far as I can tell from the C code, all parameters are able to have >>>>>> the `NOARG` flag set. We get a null pointer in the callback in that >>>>>> case. >>>>>> >>>>>> If we want to handle this now, we could drop the `default` field >>>>>> in the Rust module macro. There is no equivalent in the C macros. >>>>>> And then use an `Option<Option<_>>` to represent the value. `None` would >>>>>> be an unset parameter. `Some(None)` would be a parameter without a >>>>>> value. `Some(Some(_))` would be a set parameter with a value. We could >>>>>> probably fix the types so that only parameters with the `NOARG` flag use >>>>>> the double option, others use a single option. >>>>> >>>>> What did you think of my approach that I detailed above? I would like to >>>>> avoid `Option<Option<_>>` if we can. >>>> >>>> How would you represent the case when the parameter is passed without a >>>> value and a default is given in `module!`? >>> >>> I am a bit confused, there are two default values here: >>> (1) the value returned by `try_from_param_arg(None)`. >>> (2) the value given by the user to the `module!` macro. >>> >>> I am talking about changing the definition of ModuleParam, so (1). I get >>> the feeling that you are talking about (2), is that correct? >> >> I confused myself as well I think. I am talking about (1). Let me try >> again. >> >> If we support `NOARG_ALLOWED` (`KERNEL_PARAM_OPS_FL_NOARG` in C. I >> should change the flag name in Rust), modules can optionally support >> some parameters where users can pass parameters either as >> `my_module.param=value` or `my_module.param`. Thus, at the level of >> `try_from_param_arg`, we need to represent two cases: parameter passed >> without value, and parameter passed with value. A third case, parameter >> not passed at all, is equivalent to `try_from_param_arg` never being >> called. In C this is undetectable for the predefined parameter types. I >> wanted the double option to detect this. But I guess it does not make >> sense. > > My idea was to have an `const DEFAULT: Option<Self>` to represent the > following: > (1) if `DEFAULT == None`, then `KERNEL_PARAM_OPS_FL_NOARG` is not set > and thus either the `module!` user-specified default value is used, > or it is specified via `my_module.param=value` and > `try_from_param_arg` is called. > (2) if `DEFAULT == Some(d)`, then `KERNEL_PARAM_OPS_FL_NOARG` is set and > when `NULL` is given to `kernel_param_ops.set`, the parameter value > is set to `d`, otherwise `try_from_param_arg` is called. > > But I think I agree with you removing `NOARG_ALLOWED`, see below. Great! > >> At a higher level where the bindings supply the parsing functions, we >> can decide that passing an argument without a value yields a default >> parameter value. C does this for the predefined `bool` type. The >> predefined integer types does not support omitting the value. >> >> This patch only supports the higher level predefined parameter types, >> and does not allow modules to supply their own parameter parsing >> functions. None of the types we implement in this patch support passing >> the argument without a value. This is intentional to mirror the C >> implementation. >> >> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >> trait to: >> >> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >> >> If/when we start supporting types like `bool` or custom parsing >> functions provided by the module, we will have to update the signature >> to take an `Option` to represent the case where the user passed an >> argument without a value. However, to mimic C, the function must always >> return a value if successful, even if the user did not supply a value to >> the argument. >> >> Two different default values are in flight here. 1) the value that the >> parameter will have before the kernel calls `try_from_param_arg` via >> `set_param` and 2) the value to return from `try_from_param_arg` if the >> user did not pass a value with the argument. >> >> For a `bool` 1) would usually be `false` and 2) would always be `true`. >> >> For predefined types the module would not customize 2), but 1) is useful >> to customize. For custom types where the module supplies the parsing >> function, 2) would be implicitly given by the module in the parsing >> function. >> >> In this patch set, we only have 1 default value, namely 1). We do not >> need 2) because we do not support parameters without values. > > I am not sure that putting the default value of `my_module.param` into > the `ModuleParam` trait is a good idea. It feels more correct to me to > add an optional field to the part in `module!` that can be set to denote > this default value -- we might also want to change the name of > `default`, what do you think of `default_inactive` and `default_active`? For all the predefined parameter types, the module code would never set the `default_active` value. It should be part of the data parsing specification for the predefined argument types. For custom parsing functions implemented in the module, it makes sense specifying this value in the trait. > Since one might want an option by default be `true` and when one writes > `my_module.param`, it should be `false`. I think this would be confusing, since the default C parameter types do not have this semantic. Specifying a default true boolean as an argument does not make it false in C land. TBH this also feels like the most sane thing to do. > Also as the C side shows, having default values for integer types is not > really a good idea, since you might want a non-zero default value. > If one does not specify the `default_active` value, then the > `KERNEL_PARAM_OPS_FL_NOARG` is not set. I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait implementation like C does. We would set the flag for `bool` but not for integer types. For custom implementations of the trait, the developer can do whatever they want. > If you don't want to implement this (which I can fully understand, since > we might get `syn` before anyone needs params with default values), then > we should write this idea down (maybe in an issue?). But regardless, I > would like to know your opinion on this topic. We can create he issue if this series is accepted without the feature. > >>>> I think we need to drop the default value if we adopt the arg without >>>> value scheme. >>> >>> Yes definitely. I don't see anything in the code doing this currently, >>> right? >> >> The current patch uses the default value given in the `module!` macro to >> initialize the parameter value. > > But what drops the default value, when an actual value is specified via > `my_module.param=value`? (or is the default value only assigned when > nothing is specified?) Some more confusion maybe. When I wrote "drop the default value", I meant remove it from the code, not specify it. I take it back though. We would use the value given in the `module!` macro `default` field as `default_inactive`. `default_active` is not required for integer types, since they always require a value for the argument. But the drop would happen in `set_param` when the return value of `core::ptr::replace` is dropped. > >>> We could also only allow `Copy` values to be used as Parameters (but >>> then `str` cannot be used as a parameter...), since they can't implement >>> `Drop`. >> >> We should plan on eventually supporting `String`. > > Yes. > >>>>>> Or we could just not adopt this feature in the Rust abstractions. >>>>> >>>>> Depends on how common this is and if people need to use it. I think that >>>>> what I proposed above isn't that complex, so it should be easy to >>>>> implement. >>>> >>>> Rust modules would just force people to add "my_module.param=1" instead >>>> of just "my_module.param". I think that is reasonable. >>> >>> Eh, why do we want to give that up? I don't think it's difficult to do. >> >> I just don't see the point. Just have the user pass `my_module.param=1` >> instead of omitting the value. Having multiple ways of specifying for >> instance a true boolean just leads to confusion. Some boolean parameters >> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In >> this case specifying `nvme.use_cmb_sqes` has no effect, even though one >> might think it has. > > This just shows to me that a "global" default in `ModuleParam` is wrong, > since for `use_cmb_sqes` one could either have a negated flag, or no > default value, forcing the user to write `nvme.use_cmb_sqes=false`. I do not think it is a good idea to be able to override the value assigned to a parameter when it is given without a value. The more predictable a user interface is, the better. Best regards, Andreas
On 05.08.24 12:55, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: >> On 02.08.24 12:27, Andreas Hindborg wrote: >>> At a higher level where the bindings supply the parsing functions, we >>> can decide that passing an argument without a value yields a default >>> parameter value. C does this for the predefined `bool` type. The >>> predefined integer types does not support omitting the value. >>> >>> This patch only supports the higher level predefined parameter types, >>> and does not allow modules to supply their own parameter parsing >>> functions. None of the types we implement in this patch support passing >>> the argument without a value. This is intentional to mirror the C >>> implementation. >>> >>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >>> trait to: >>> >>> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >>> >>> If/when we start supporting types like `bool` or custom parsing >>> functions provided by the module, we will have to update the signature >>> to take an `Option` to represent the case where the user passed an >>> argument without a value. However, to mimic C, the function must always >>> return a value if successful, even if the user did not supply a value to >>> the argument. >>> >>> Two different default values are in flight here. 1) the value that the >>> parameter will have before the kernel calls `try_from_param_arg` via >>> `set_param` and 2) the value to return from `try_from_param_arg` if the >>> user did not pass a value with the argument. >>> >>> For a `bool` 1) would usually be `false` and 2) would always be `true`. >>> >>> For predefined types the module would not customize 2), but 1) is useful >>> to customize. For custom types where the module supplies the parsing >>> function, 2) would be implicitly given by the module in the parsing >>> function. >>> >>> In this patch set, we only have 1 default value, namely 1). We do not >>> need 2) because we do not support parameters without values. >> >> I am not sure that putting the default value of `my_module.param` into >> the `ModuleParam` trait is a good idea. It feels more correct to me to >> add an optional field to the part in `module!` that can be set to denote >> this default value -- we might also want to change the name of >> `default`, what do you think of `default_inactive` and `default_active`? > > For all the predefined parameter types, the module code would never set > the `default_active` value. It should be part of the data parsing > specification for the predefined argument types. So if your module has an i32 parameter, you can't set a default value to eg 1000? > For custom parsing functions implemented in the module, it makes sense > specifying this value in the trait. Couldn't I just emulate the behavior from above by creating a `struct MyI32(i32)` and having a default value? In that case, why make it more difficult instead of providing a simple way to do it? >> Since one might want an option by default be `true` and when one writes >> `my_module.param`, it should be `false`. > > I think this would be confusing, since the default C parameter types do > not have this semantic. Specifying a default true boolean as an argument > does not make it false in C land. TBH this also feels like the most sane > thing to do. Can't you also do the custom parameter parsing from above? ie have an integer parameter with a default value? Or is that frowned upon (ie not allowed through review)? >> Also as the C side shows, having default values for integer types is not >> really a good idea, since you might want a non-zero default value. >> If one does not specify the `default_active` value, then the >> `KERNEL_PARAM_OPS_FL_NOARG` is not set. > > I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait > implementation like C does. We would set the flag for `bool` but not for > integer types. For custom implementations of the trait, the developer > can do whatever they want. So we are only going to use it for `bool`? >> If you don't want to implement this (which I can fully understand, since >> we might get `syn` before anyone needs params with default values), then >> we should write this idea down (maybe in an issue?). But regardless, I >> would like to know your opinion on this topic. > > We can create he issue if this series is accepted without the feature. > >> >>>>> I think we need to drop the default value if we adopt the arg without >>>>> value scheme. >>>> >>>> Yes definitely. I don't see anything in the code doing this currently, >>>> right? >>> >>> The current patch uses the default value given in the `module!` macro to >>> initialize the parameter value. >> >> But what drops the default value, when an actual value is specified via >> `my_module.param=value`? (or is the default value only assigned when >> nothing is specified?) > > Some more confusion maybe. When I wrote "drop the default value", I > meant remove it from the code, not specify it. I take it back though. We > would use the value given in the `module!` macro `default` field as > `default_inactive`. `default_active` is not required for integer types, > since they always require a value for the argument. > > But the drop would happen in `set_param` when the return value of > `core::ptr::replace` is dropped. Ah I see, I missed that. >>>>> We could also only allow `Copy` values to be used as Parameters (but >>>> then `str` cannot be used as a parameter...), since they can't implement >>>> `Drop`. >>> >>> We should plan on eventually supporting `String`. >> >> Yes. >> >>>>>>> Or we could just not adopt this feature in the Rust abstractions. >>>>>> >>>>>> Depends on how common this is and if people need to use it. I think that >>>>>> what I proposed above isn't that complex, so it should be easy to >>>>>> implement. >>>>> >>>>> Rust modules would just force people to add "my_module.param=1" instead >>>>> of just "my_module.param". I think that is reasonable. >>>> >>>> Eh, why do we want to give that up? I don't think it's difficult to do. >>> >>> I just don't see the point. Just have the user pass `my_module.param=1` >>> instead of omitting the value. Having multiple ways of specifying for >>> instance a true boolean just leads to confusion. Some boolean parameters >>> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In >>> this case specifying `nvme.use_cmb_sqes` has no effect, even though one >>> might think it has. >> >> This just shows to me that a "global" default in `ModuleParam` is wrong, >> since for `use_cmb_sqes` one could either have a negated flag, or no >> default value, forcing the user to write `nvme.use_cmb_sqes=false`. > > I do not think it is a good idea to be able to override the value > assigned to a parameter when it is given without a value. The more > predictable a user interface is, the better. By that argument, we should also not permit custom implementations to specify a default value. --- Cheers, Benno
"Benno Lossin" <benno.lossin@proton.me> writes: > On 05.08.24 12:55, Andreas Hindborg wrote: >> "Benno Lossin" <benno.lossin@proton.me> writes: >>> On 02.08.24 12:27, Andreas Hindborg wrote: >>>> At a higher level where the bindings supply the parsing functions, we >>>> can decide that passing an argument without a value yields a default >>>> parameter value. C does this for the predefined `bool` type. The >>>> predefined integer types does not support omitting the value. >>>> >>>> This patch only supports the higher level predefined parameter types, >>>> and does not allow modules to supply their own parameter parsing >>>> functions. None of the types we implement in this patch support passing >>>> the argument without a value. This is intentional to mirror the C >>>> implementation. >>>> >>>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >>>> trait to: >>>> >>>> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >>>> >>>> If/when we start supporting types like `bool` or custom parsing >>>> functions provided by the module, we will have to update the signature >>>> to take an `Option` to represent the case where the user passed an >>>> argument without a value. However, to mimic C, the function must always >>>> return a value if successful, even if the user did not supply a value to >>>> the argument. >>>> >>>> Two different default values are in flight here. 1) the value that the >>>> parameter will have before the kernel calls `try_from_param_arg` via >>>> `set_param` and 2) the value to return from `try_from_param_arg` if the >>>> user did not pass a value with the argument. >>>> >>>> For a `bool` 1) would usually be `false` and 2) would always be `true`. >>>> >>>> For predefined types the module would not customize 2), but 1) is useful >>>> to customize. For custom types where the module supplies the parsing >>>> function, 2) would be implicitly given by the module in the parsing >>>> function. >>>> >>>> In this patch set, we only have 1 default value, namely 1). We do not >>>> need 2) because we do not support parameters without values. >>> >>> I am not sure that putting the default value of `my_module.param` into >>> the `ModuleParam` trait is a good idea. It feels more correct to me to >>> add an optional field to the part in `module!` that can be set to denote >>> this default value -- we might also want to change the name of >>> `default`, what do you think of `default_inactive` and `default_active`? >> >> For all the predefined parameter types, the module code would never set >> the `default_active` value. It should be part of the data parsing >> specification for the predefined argument types. > > So if your module has an i32 parameter, you can't set a default value to > eg 1000? You _would_ be able to set the `default_inactive` value, which is the value assigned to the static at initialization time. It would make sense to default this to 0 for integer types and make it overridable in the `module!` macro. You would not be to set the `default_active` value, which is the value assigned to the parameter static variable, when the parameter is passed without value. The reason being that we want to mirror C, so we prohibit this for predefined integer parameter types. > >> For custom parsing functions implemented in the module, it makes sense >> specifying this value in the trait. > > Couldn't I just emulate the behavior from above by creating a > `struct MyI32(i32)` and having a default value? > In that case, why make it more difficult instead of providing a simple > way to do it? The way parameter parsing works in C (that we try to replicate) is that we are able to parse a set of predefined types. For each predefined type we have a set of parser functions. If a module author want to parse something outside of the predefined set, they can provide custom parser functions. I would advocate we do the same in Rust. This patch provides a set of predefined types that can be parsed. For parsing to other types, we would eventually expand the features to allow the module authors to implement the parsing trait on their own types. So to answer your question: No, not in this patch set. But let's plan to add it in the future. > >>> Since one might want an option by default be `true` and when one writes >>> `my_module.param`, it should be `false`. >> >> I think this would be confusing, since the default C parameter types do >> not have this semantic. Specifying a default true boolean as an argument >> does not make it false in C land. TBH this also feels like the most sane >> thing to do. > > Can't you also do the custom parameter parsing from above? ie have an > integer parameter with a default value? > Or is that frowned upon (ie not allowed through review)? In C, you cannot, if you utilize the predefined parameter parsing functions. Your can supply your own parser, and then you can do whatever you want. I would assume that it is best practice to reuse the predefined parsing functions. For Rust code I would argue that we should have module authors use the predefined parsers, unless they have very special requirements. > >>> Also as the C side shows, having default values for integer types is not >>> really a good idea, since you might want a non-zero default value. >>> If one does not specify the `default_active` value, then the >>> `KERNEL_PARAM_OPS_FL_NOARG` is not set. >> >> I would rather predefine `KERNEL_PARAM_OPS_FL_NOARG` in the trait >> implementation like C does. We would set the flag for `bool` but not for >> integer types. For custom implementations of the trait, the developer >> can do whatever they want. > > So we are only going to use it for `bool`? I think so. As far as I can tell, the predefined C parsers only allow this for bool. Outside of the predefined parsers, I see only 4 custom argument parsers that allow parameters without values. > >>> If you don't want to implement this (which I can fully understand, since >>> we might get `syn` before anyone needs params with default values), then >>> we should write this idea down (maybe in an issue?). But regardless, I >>> would like to know your opinion on this topic. >> >> We can create he issue if this series is accepted without the feature. >> >>> >>>>>> I think we need to drop the default value if we adopt the arg without >>>>>> value scheme. >>>>> >>>>> Yes definitely. I don't see anything in the code doing this currently, >>>>> right? >>>> >>>> The current patch uses the default value given in the `module!` macro to >>>> initialize the parameter value. >>> >>> But what drops the default value, when an actual value is specified via >>> `my_module.param=value`? (or is the default value only assigned when >>> nothing is specified?) >> >> Some more confusion maybe. When I wrote "drop the default value", I >> meant remove it from the code, not specify it. I take it back though. We >> would use the value given in the `module!` macro `default` field as >> `default_inactive`. `default_active` is not required for integer types, >> since they always require a value for the argument. >> >> But the drop would happen in `set_param` when the return value of >> `core::ptr::replace` is dropped. > > Ah I see, I missed that. > >>>>>> We could also only allow `Copy` values to be used as Parameters (but >>>>> then `str` cannot be used as a parameter...), since they can't implement >>>>> `Drop`. >>>> >>>> We should plan on eventually supporting `String`. >>> >>> Yes. >>> >>>>>>>> Or we could just not adopt this feature in the Rust abstractions. >>>>>>> >>>>>>> Depends on how common this is and if people need to use it. I think that >>>>>>> what I proposed above isn't that complex, so it should be easy to >>>>>>> implement. >>>>>> >>>>>> Rust modules would just force people to add "my_module.param=1" instead >>>>>> of just "my_module.param". I think that is reasonable. >>>>> >>>>> Eh, why do we want to give that up? I don't think it's difficult to do. >>>> >>>> I just don't see the point. Just have the user pass `my_module.param=1` >>>> instead of omitting the value. Having multiple ways of specifying for >>>> instance a true boolean just leads to confusion. Some boolean parameters >>>> have a default value of `true`, for instance `nvme.use_cmb_sqes`. In >>>> this case specifying `nvme.use_cmb_sqes` has no effect, even though one >>>> might think it has. >>> >>> This just shows to me that a "global" default in `ModuleParam` is wrong, >>> since for `use_cmb_sqes` one could either have a negated flag, or no >>> default value, forcing the user to write `nvme.use_cmb_sqes=false`. >> >> I do not think it is a good idea to be able to override the value >> assigned to a parameter when it is given without a value. The more >> predictable a user interface is, the better. > > By that argument, we should also not permit custom implementations to > specify a default value. If we provide a predefined set of parsing functions, most uses will use those, and we will get a similar user experience across most of the modules. If we allow said customization of the predefined parsers, people will use it those customizations, and we will not get a good user experience. If we omit the customizations, I think most will still stick to predefined semantics, rather than implement their own parsers. For those that actually need to have other semantics (or types), we will have the option of user provided parsers, just as C. Best regards, Andreas
On 15.08.24 15:11, Andreas Hindborg wrote: > "Benno Lossin" <benno.lossin@proton.me> writes: >> On 05.08.24 12:55, Andreas Hindborg wrote: >>> "Benno Lossin" <benno.lossin@proton.me> writes: >>>> On 02.08.24 12:27, Andreas Hindborg wrote: >>>>> At a higher level where the bindings supply the parsing functions, we >>>>> can decide that passing an argument without a value yields a default >>>>> parameter value. C does this for the predefined `bool` type. The >>>>> predefined integer types does not support omitting the value. >>>>> >>>>> This patch only supports the higher level predefined parameter types, >>>>> and does not allow modules to supply their own parameter parsing >>>>> functions. None of the types we implement in this patch support passing >>>>> the argument without a value. This is intentional to mirror the C >>>>> implementation. >>>>> >>>>> To that end, I removed `NOARG_ALLOWED`, and changed the parsing function >>>>> trait to: >>>>> >>>>> fn try_from_param_arg(arg: &'static [u8]) -> Result<Self>; >>>>> >>>>> If/when we start supporting types like `bool` or custom parsing >>>>> functions provided by the module, we will have to update the signature >>>>> to take an `Option` to represent the case where the user passed an >>>>> argument without a value. However, to mimic C, the function must always >>>>> return a value if successful, even if the user did not supply a value to >>>>> the argument. >>>>> >>>>> Two different default values are in flight here. 1) the value that the >>>>> parameter will have before the kernel calls `try_from_param_arg` via >>>>> `set_param` and 2) the value to return from `try_from_param_arg` if the >>>>> user did not pass a value with the argument. >>>>> >>>>> For a `bool` 1) would usually be `false` and 2) would always be `true`. >>>>> >>>>> For predefined types the module would not customize 2), but 1) is useful >>>>> to customize. For custom types where the module supplies the parsing >>>>> function, 2) would be implicitly given by the module in the parsing >>>>> function. >>>>> >>>>> In this patch set, we only have 1 default value, namely 1). We do not >>>>> need 2) because we do not support parameters without values. >>>> >>>> I am not sure that putting the default value of `my_module.param` into >>>> the `ModuleParam` trait is a good idea. It feels more correct to me to >>>> add an optional field to the part in `module!` that can be set to denote >>>> this default value -- we might also want to change the name of >>>> `default`, what do you think of `default_inactive` and `default_active`? >>> >>> For all the predefined parameter types, the module code would never set >>> the `default_active` value. It should be part of the data parsing >>> specification for the predefined argument types. >> >> So if your module has an i32 parameter, you can't set a default value to >> eg 1000? > > You _would_ be able to set the `default_inactive` value, which is the > value assigned to the static at initialization time. It would make sense > to default this to 0 for integer types and make it overridable in the > `module!` macro. Hmm, I would say it makes more sense to have the user always specify a default in `module!`, since integers can mean a lot of different things. > You would not be to set the `default_active` value, which is the value > assigned to the parameter static variable, when the parameter is passed > without value. The reason being that we want to mirror C, so we prohibit > this for predefined integer parameter types. Gotcha, I wasn't 100% sure in our previous emails if we wanted to exactly mirror C or not. Should've asked that. Thanks for taking the time to write this clarification (and the other ones as well!), I think I now finally understand why you do it this way. I agree with your approach. --- Cheers, Benno
On Wed, Jul 24, 2024 at 05:04:25PM GMT, Sami Tolvanen wrote: > Hi Luis, > > On Thu, Jul 18, 2024 at 5:15 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Tue, Jul 09, 2024 at 12:08:16PM +0200, Miguel Ojeda wrote: > > > On Mon, Jul 8, 2024 at 11:42 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > > > The rationale here is that a rust binding means commitment then also > > > > from fresh blood to help co-maintain review C / Rust for exising code > > > > when there is will / desire to collaborate from an existing C maintainer. > > > > > > > > I realize this may be a lot to ask, but I think this is one of the > > > > responsible ways to ask to scale here. > > > > > > But, yes, I think Rust is a great opportunity to get new > > > co-maintainers, as well as getting new developers involved with kernel > > > maintenance in general, which could help with other issues too. > > > > Great well then my preference is to not have Rust bindings for modules > > unless the Rust community can commit to not only a co-maintianer for > > both C And Rust but also commit to not ditching the role; if a C/Rust > > co-maintainer gets hits by a bus the Rust community would strive to > > look for someone else to step in. This would proactively help with > > upstream responsibilities understood by companies who hire developers > > in this context. It is why I brought up Andreas's work, I already know > > he has a lot of work to do and responsibilities. If not Andreas, who else > > can step up to help with this, Sami? > > I agree, having a co-maintainer from the Rust community sounds like a > good idea. It would be great if someone actually working on the > bindings could step up, but if there are no other volunteers, I can > certainly help with this. I recently started learning Rust, and as I continue with that, I’m also interested in joining and volunteering. > > Sami
On Wed, Jul 24, 2024 at 05:04:25PM +0000, Sami Tolvanen wrote: > Hi Luis, > > On Thu, Jul 18, 2024 at 5:15 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > On Tue, Jul 09, 2024 at 12:08:16PM +0200, Miguel Ojeda wrote: > > > On Mon, Jul 8, 2024 at 11:42 PM Luis Chamberlain <mcgrof@kernel.org> wrote: > > > > > > > > The rationale here is that a rust binding means commitment then also > > > > from fresh blood to help co-maintain review C / Rust for exising code > > > > when there is will / desire to collaborate from an existing C maintainer. > > > > > > > > I realize this may be a lot to ask, but I think this is one of the > > > > responsible ways to ask to scale here. > > > > > > But, yes, I think Rust is a great opportunity to get new > > > co-maintainers, as well as getting new developers involved with kernel > > > maintenance in general, which could help with other issues too. > > > > Great well then my preference is to not have Rust bindings for modules > > unless the Rust community can commit to not only a co-maintianer for > > both C And Rust but also commit to not ditching the role; if a C/Rust > > co-maintainer gets hits by a bus the Rust community would strive to > > look for someone else to step in. This would proactively help with > > upstream responsibilities understood by companies who hire developers > > in this context. It is why I brought up Andreas's work, I already know > > he has a lot of work to do and responsibilities. If not Andreas, who else > > can step up to help with this, Sami? > > I agree, having a co-maintainer from the Rust community sounds like a > good idea. It would be great if someone actually working on the > bindings could step up, but if there are no other volunteers, I can > certainly help with this. Excelent thank you for this, yes we can certainly use your help with this. After this went out we also had Daniel Gomez express interest, and Petr Pavlu expressed interest as well. I think with all of us we can likely grow a strong base to cover both C / Rust side of modules. I'll send a follow up maintainers patch up for this and we can move on forward with review / future integration of Rust module code. Luis
diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h index ddb5644d4fd9..767169f46f10 100644 --- a/rust/bindings/bindings_helper.h +++ b/rust/bindings/bindings_helper.h @@ -25,3 +25,4 @@ const gfp_t RUST_CONST_HELPER_GFP_KERNEL = GFP_KERNEL; const gfp_t RUST_CONST_HELPER_GFP_KERNEL_ACCOUNT = GFP_KERNEL_ACCOUNT; const gfp_t RUST_CONST_HELPER_GFP_NOWAIT = GFP_NOWAIT; const gfp_t RUST_CONST_HELPER___GFP_ZERO = __GFP_ZERO; +const size_t RUST_CONST_HELPER_PAGE_SIZE = PAGE_SIZE; diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index fbd91a48ff8b..7cc2730e572f 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -33,6 +33,8 @@ pub mod ioctl; #[cfg(CONFIG_KUNIT)] pub mod kunit; +#[doc(hidden)] +pub mod module_param; #[cfg(CONFIG_NET)] pub mod net; pub mod prelude; diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs new file mode 100644 index 000000000000..dc1108d20723 --- /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::{error::code::*, str::CStr}; + +/// 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; + + /// Whether the parameter is allowed to be set without an argument. + /// + /// Setting this to `true` allows the parameter to be passed without an + /// argument (e.g. just `module.param` instead of `module.param=foo`). + const NOARG_ALLOWED: bool; + + /// Convert a parameter argument into the parameter value. + /// + /// `None` should be returned when parsing of the argument fails. + /// `arg == None` indicates that the parameter was passed without an + /// argument. If `NOARG_ALLOWED` is set to `false` then `arg` is guaranteed + /// to always be `Some(_)`. + /// + /// 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: Option<&'static [u8]>) -> Option<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 Self` 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 `Self`. + /// + /// # 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( + val: *const core::ffi::c_char, + param: *const crate::bindings::kernel_param, + ) -> core::ffi::c_int { + let arg = if val.is_null() { + None + } else { + // 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. + Some(unsafe { CStr::from_char_ptr(val).as_bytes() }) + }; + match Self::try_from_param_arg(arg) { + Some(new_value) => { + // SAFETY: `param` is guaranteed to be valid by C API contract + // and `arg` is guaranteed to point to an instance of `Self`. + let old_value = unsafe { (*param).__bindgen_anon_1.arg as *mut Self }; + // 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. + let _ = unsafe { core::ptr::replace(old_value, new_value) }; + 0 + } + None => EINVAL.to_errno(), + } + } + + /// 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. + unsafe extern "C" fn get_param( + _buf: *mut core::ffi::c_char, + _param: *const crate::bindings::kernel_param, + ) -> core::ffi::c_int { + 0 + } + + /// 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(arg: *mut core::ffi::c_void) { + // SAFETY: By function safety requirement, `arg` is an initialized + // instance of `Self`. By C API contract, `arg` will not be used after + // this function returns. + unsafe { core::ptr::drop_in_place(arg as *mut Self) }; + } +} + +/// 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>; + 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) -> Option<Self> { + match src.bytes().next() { + None => None, + Some(b'-') => Self::from_str_unsigned(&src[1..]).ok()?.checked_neg(), + Some(b'+') => Some(Self::from_str_unsigned(&src[1..]).ok()?), + Some(_) => Some(Self::from_str_unsigned(src).ok()?), + } + } +} + +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; + + const NOARG_ALLOWED: bool = false; + + fn try_from_param_arg(arg: Option<&'static [u8]>) -> Option<Self> { + let bytes = arg?; + let utf8 = core::str::from_utf8(bytes).ok()?; + <$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: if <$ty as $crate::module_param::ModuleParam>::NOARG_ALLOWED { + $crate::bindings::KERNEL_PARAM_OPS_FL_NOARG + } else { + 0 + }, + set: Some(<$ty as $crate::module_param::ModuleParam>::set_param), + get: Some(<$ty as $crate::module_param::ModuleParam>::get_param), + free: Some(<$ty as $crate::module_param::ModuleParam>::free), + }; + }; +} + +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..dc0b47879a8c 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 get_string(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/module.rs b/rust/macros/module.rs index acd0393b5095..fc794855b99e 100644 --- a/rust/macros/module.rs +++ b/rust/macros/module.rs @@ -1,7 +1,7 @@ // SPDX-License-Identifier: GPL-2.0 use crate::helpers::*; -use proc_macro::{token_stream, Delimiter, Literal, TokenStream, TokenTree}; +use proc_macro::{token_stream, Delimiter, Group, Literal, TokenStream, TokenTree}; use std::fmt::Write; fn expect_string_array(it: &mut token_stream::IntoIter) -> Vec<String> { @@ -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,190 @@ 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 { + assert_eq!(params.delimiter(), Delimiter::Brace); + + let mut it = params.stream().into_iter(); + + 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!(expect_punct(&mut it), ','); + + assert_eq!(group.delimiter(), Delimiter::Brace); + + let mut param_it = group.stream().into_iter(); + let param_default = get_param_default(&mut param_it); + let param_description = get_string(&mut param_it, "description"); + expect_end(&mut param_it); + + let (param_kernel_type, ops): (String, _) = ( + param_type.to_string(), + param_ops_path(¶m_type).to_string(), + ); + + self.emit_param("parmtype", ¶m_name, ¶m_kernel_type); + self.emit_param("parm", ¶m_name, ¶m_description); + let param_type_internal = param_type.clone(); + + let read_func = format!( + " + pub(crate) fn read(&self) + -> &<{param_type_internal} + 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_internal} as kernel::module_param::ModuleParam>::value( + &__{name}_{param_name}_value + ) + }} + }} + ", + name = info.name, + param_name = param_name, + param_type_internal = param_type_internal, + ); + + let kparam = format!( + " + kernel::bindings::kernel_param__bindgen_ty_1 {{ + // SAFETY: Access through the resulting pointer is + // serialized by C side and only happens before module + // `init` or after module `drop` is called. + arg: unsafe {{ &__{name}_{param_name}_value }} + as *const _ as *mut core::ffi::c_void, + }}, + ", + name = info.name, + param_name = param_name, + ); + write!( + self.param_buffer, + " + static mut __{name}_{param_name}_value: {param_type_internal} = {param_default}; + + pub(crate) struct __{name}_{param_name}; + + impl __{name}_{param_name} {{ {read_func} }} + + pub(crate) const {param_name}: __{name}_{param_name} = __{name}_{param_name}; + + // Note: the C macro that generates the static structs for the `__param` section + // asks for them to be `aligned(sizeof(void *))`. However, that was put in place + // in 2003 in commit 38d5b085d2a0 (\"[PATCH] Fix over-alignment problem on x86-64\") + // to undo GCC over-alignment of static structs of >32 bytes. It seems that is + // not the case anymore, so we simplify to a transparent representation here + // in the expectation that it is not needed anymore. + // TODO: Revisit this to confirm the above comment and remove it if it happened. + /// 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(), + // SAFETY: This static is actually constant as seen by + // module code. But we need a unique address for it, so it + // must be static. + ops: unsafe {{ &{ops} }} as *const kernel::bindings::kernel_param_ops, + perm: 0, // Will not appear in sysfs + level: -1, + flags: 0, + __bindgen_anon_1: {kparam} + }}); + ", + name = info.name, + param_type_internal = param_type_internal, + read_func = read_func, + param_default = param_default, + param_name = param_name, + ops = ops, + kparam = kparam, + ) + .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!("Unrecognized type {}", t), + } +} + +fn get_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, @@ -97,14 +275,22 @@ struct ModuleInfo { author: Option<String>, description: Option<String>, alias: Option<Vec<String>>, + params: Option<Group>, } impl ModuleInfo { fn parse(it: &mut token_stream::IntoIter) -> Self { let mut info = ModuleInfo::default(); - const EXPECTED_KEYS: &[&str] = - &["type", "name", "author", "description", "license", "alias"]; + const EXPECTED_KEYS: &[&str] = &[ + "type", + "name", + "author", + "description", + "license", + "alias", + "params", + ]; const REQUIRED_KEYS: &[&str] = &["type", "name", "license"]; let mut seen_keys = Vec::new(); @@ -131,6 +317,7 @@ fn parse(it: &mut token_stream::IntoIter) -> Self { "description" => info.description = Some(expect_string(it)), "license" => info.license = expect_string_ascii(it), "alias" => info.alias = Some(expect_string_array(it)), + "params" => info.params = Some(expect_group(it)), _ => panic!( "Unknown key \"{}\". Valid keys are: {:?}.", key, EXPECTED_KEYS @@ -174,23 +361,25 @@ 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); } } // 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!( " @@ -332,14 +521,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..359e6d8ec983 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)?;