From: "Gary Guo" <gary@garyguo.net>
To: "Alexandre Courbot" <acourbot@nvidia.com>, "Gary Guo" <gary@garyguo.net>
Cc: "Danilo Krummrich" <dakr@kernel.org>,
"Alice Ryhl" <aliceryhl@google.com>,
"Daniel Almeida" <daniel.almeida@collabora.com>,
"Miguel Ojeda" <ojeda@kernel.org>,
"Boqun Feng" <boqun@kernel.org>,
"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
"Benno Lossin" <lossin@kernel.org>,
"Andreas Hindborg" <a.hindborg@kernel.org>,
"Trevor Gross" <tmgross@umich.edu>,
"Tamir Duberstein" <tamird@kernel.org>,
"Onur Özkan" <work@onurozkan.dev>,
"David Airlie" <airlied@gmail.com>,
"Simona Vetter" <simona@ffwll.ch>,
"Bjorn Helgaas" <bhelgaas@google.com>,
"Krzysztof Wilczyński" <kwilczynski@kernel.org>,
driver-core@lists.linux.dev, rust-for-linux@vger.kernel.org,
linux-kernel@vger.kernel.org, nova-gpu@lists.linux.dev,
dri-devel@lists.freedesktop.org, linux-pci@vger.kernel.org
Subject: Re: [PATCH v2 02/16] rust: io: add `IoRepr` trait
Date: Mon, 10 Aug 2026 12:21:38 +0100 [thread overview]
Message-ID: <DKL837PVU38R.3OONERSO8RMOW@garyguo.net> (raw)
In-Reply-To: <DKL5Q2P868GQ.29WSPCWX4HTHJ@nvidia.com>
On Mon Aug 10, 2026 at 10:30 AM BST, Alexandre Courbot wrote:
> On Thu Aug 6, 2026 at 1:35 AM JST, Gary Guo wrote:
>> For types that are layout-compatible with an I/O capable type, we would
>> want the ability to use them directly for I/O operations. E.g.
>>
>> bitfield! {
>> pub struct Foo(u32) {
>> ...
>> }
>> }
>>
>> #[repr(C)]
>> struct Bar {
>> foo: Foo,
>> }
>>
>> let mmio: Mmio<'_, Bar> = ...;
>> io_read!(mmio, .foo)
>>
>> Currently this feature is available from `register!()` macro but not
>> otherwise available with `io_read!`, `io_write!`. Support this by adding a
>> `IoRepr` type to denote the underlying I/O type to use for a specific type.
>>
>> This makes the `IoLoc::IoType` and `Register::Storage` redundant; thus
>> remove them; also convert register methods to use the `read_val` and
>> `write_val` instead.
>
> Ah that's nice, I was a bit bothered that `IoType` and `Storage`
> basically defined the same thing.
>
>>
>> Signed-off-by: Gary Guo <gary@garyguo.net>
>> ---
>> rust/kernel/bitfield.rs | 5 ++
>> rust/kernel/io.rs | 203 ++++++++++++++++++++++++++++++++-------------
>> rust/kernel/io/register.rs | 59 +++++--------
>> 3 files changed, 170 insertions(+), 97 deletions(-)
>>
>> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs
>> index 35ede53f2b8e..4bc92c62da06 100644
>> --- a/rust/kernel/bitfield.rs
>> +++ b/rust/kernel/bitfield.rs
>> @@ -308,6 +308,7 @@ macro_rules! bitfield {
>> $(#[$attr])*
>> #[repr(transparent)]
>> #[derive(Clone, Copy, PartialEq, Eq)]
>> + #[derive($crate::prelude::FromBytes, $crate::prelude::IntoBytes)]
>> $vis struct $name {
>> inner: $storage,
>> }
>> @@ -346,6 +347,10 @@ fn from(val: $storage) -> $name {
>> Self::from_raw(val)
>> }
>> }
>> +
>> + impl $crate::io::IoRepr for $name {
>> + type Repr = $storage;
>> + }
>
> This introduces a dependency from `bitfield` to `io`, which looks like
> inconsistent layering. But thankfully there is an easy fix - see below.
>
>> };
>>
>> // Definitions requiring knowledge of individual fields: private and public field accessors,
>> diff --git a/rust/kernel/io.rs b/rust/kernel/io.rs
>> index adfc555de7d0..71c6180ed745 100644
>> --- a/rust/kernel/io.rs
>> +++ b/rust/kernel/io.rs
>> @@ -276,6 +276,91 @@ pub trait IoCapable<T>: IoBackend {
>> fn io_write<'a>(view: Self::View<'a, T>, value: T);
>> }
>>
>> +/// Safe transmute that performs size check on monomorphization-time.
>> +///
>> +/// Can be considered as generic version of [`zerocopy::transmute!`] macro but using the unstable
>> +/// `core::mem::transmute_neo` instead of [`core::mem::transmute`].
>> +#[inline(always)] // This is a no-op.
>> +fn transmute_neo<Src: IntoBytes, Dst: FromBytes>(val: Src) -> Dst {
>> + const_assert!(size_of::<Src>() == size_of::<Dst>());
>> +
>> + // SAFETY: `Src: IntoBytes` and `Dst: FromBytes` and we've checked size is the same.
>> + unsafe { core::mem::transmute_copy(&core::mem::ManuallyDrop::new(val)) }
>> +}
>
> This is universally useful, so let's move this to the `transmute` module?
I think we can just have a `kernel::mem` and put it there so it's consistent
with std naming. The `transmute` module just contains two types that are going
away.
Ideally we have this function in `zerocopy`. But my understanding is that this
depends on inline const which is stable since 1.79 but zerocopy's MSRV is 1.56.
>
>> +
>> +/// Trait indicating the underlying primitive types to be used for I/O operations.
>> +///
>> +/// Implementing trait allows arbitrary types to be used for I/O operations, not just raw
>> +/// primitives.
>> +///
>> +/// The layout of the type and the underlying primitive must match; this is enforced via const
>> +/// assertions when I/O methods are used, as the type system cannot represent this.
>> +/// [`IoRepr::from_repr`] and [`IoRepr::into_expr`] can be overridden for conversions, however it
>> +/// should be noted that they are only invoked on value read/write operations and are not invoked
>> +/// on byte operations such as [`Io::copy_read`].
>> +///
>> +/// # Examples
>> +///
>> +/// ```
>> +/// # use kernel::io::*;
>> +/// #[repr(transparent)]
>> +/// #[derive(FromBytes, IntoBytes)]
>> +/// pub struct MyNewType(u32);
>> +///
>> +/// impl IoRepr for MyNewType {
>> +/// type Repr = u32;
>> +/// }
>> +///
>> +/// #[repr(C)]
>> +/// pub struct MyStruct {
>> +/// raw: u32,
>> +/// new_type: MyNewType,
>> +/// }
>> +///
>> +/// # fn test(mmio: Mmio<'_, MyStruct>) {
>> +/// // let mmio: Mmio<'_, MyStruct>;
>> +/// let val: u32 = io_read!(mmio, .raw); // Raw primitive read
>> +/// io_write!(mmio, .raw, val); // Raw primitve write
>> +/// let val: MyNewType = io_read!(mmio, .new_type); // Read via `IoRepr`.
>> +/// io_write!(mmio, .new_type, val); // Write via `IoRepr`.
>> +/// # }
>> +/// ```
>> +pub trait IoRepr: FromBytes + IntoBytes + Sized {
>> + /// The backing I/O capable type.
>> + type Repr: FromBytes + IntoBytes;
>> +
>> + /// Convert from [`IoRepr::Repr`] to `Self`.
>> + #[inline(always)]
>> + fn from_repr(repr: Self::Repr) -> Self {
>> + transmute_neo(repr)
>> + }
>> +
>> + /// Convert from `Self` to [`IoRepr::Repr`].
>> + #[inline(always)]
>> + fn into_repr(this: Self) -> Self::Repr {
>> + transmute_neo(this)
>> + }
>> +}
>> +
>> +macro_rules! impl_io_repr {
>> + ($($ty:ty => $backing:ty,)*) => {
>> + $(impl IoRepr for $ty {
>> + type Repr = $backing;
>> + })*
>> + };
>> +}
>> +
>> +impl_io_repr! {
>> + u8 => u8,
>> + u16 => u16,
>> + u32 => u32,
>> + u64 => u64,
>> + i8 => u8,
>> + i16 => u16,
>> + i32 => u32,
>> + i64 => u64,
>> +}
>
> ... and `IoRepr` and its implementations for primitive types should also
> be part of `transmute` IMHO (after being renamed to e.g. `Repr`), for
> there is nothing I/O exclusive to it. It just indicates that one type
> can be represented by another, a property that is again useful outside
> of I/O.
I tried to be a little bit forward looking in designing this, so I ended up
with a design that provides conversion functions instead of just transmutation.
If we're moving it we should probably just get rid of these and just require a
transmutability with the raw repr.
Note that there is `AtomicType` which has similar (but not equivalent
requirement). `AtomicType` requires a round-trip transmutability only, and
`IoRepr` needs both directions. So `repr(C)` enums (that is not used up all its
variant reprs) can be `AtomicType` but not `IoRepr`.
>
> That way `bitfield` gets a dependency on `transmute` rather than `io`,
> which doesn't break layering.
We can also avoid breaking layering by requiring user to need to specify
`#[derive(IoRepr)]` when declaring bitfield.
Best,
Gary
>
> In order to avoid `Repr::Repr` we can also rename the associated type to
> `Raw` and update the method names accordingly to `from_raw`/`into_raw` -
> which would have allowed us to remove the `bitfield` methods of the same
> name if they weren't needed in const context! But at least it makes
> things align nicely.
>
> <...>
>> @@ -831,8 +816,8 @@ macro_rules! register {
>> { $($fields:tt)* }
>> ) => {
>> $crate::register!(@bitfield $(#[$attr])* $vis struct $name($storage) { $($fields)* });
>> - $crate::register!(@io_base $name($storage) @ $offset);
>> - $crate::register!(@io_fixed $(#[$attr])* $vis $name($storage));
>> + $crate::register!(@io_base $name @ $offset);
>> + $crate::register!(@io_fixed $(#[$attr])* $vis $name);
>> };
>>
>> };
>> [snip]
>>
>> // Implementations of register arrays.
>> - (@io_array $vis:vis $name:ident ($storage:ty) [ $size:expr, stride = $stride:expr ]) => {
>> + (@io_array $vis:vis $name:ident [ $size:expr, stride = $stride:expr ]) => {
>> impl $crate::io::register::Array for $name {}
>>
>> impl $crate::io::register::RegisterArray for $name {
>> @@ -1008,7 +991,7 @@ impl $crate::io::register::RegisterArray for $name {
>>
>> // Implementations of relative array registers.
>> (
>> - @io_relative_array $vis:vis $name:ident ($storage:ty) [ $size:expr, stride = $stride:expr ]
>> + @io_relative_array $vis:vis $name:ident [ $size:expr, stride = $stride:expr ]
>> @ $base:ident + $offset:literal
>> ) => {
>> impl $crate::io::register::WithBase for $name {
>
> These appear to repeat quite a bit of [1] which is already in
> `driver-core-next`. You'll want to rebase, I think the only difference
> between the two is the removal of $storage from @io_base.
I'm thinking of moving this entire thing to syn :)
Best,
Gary
next prev parent reply other threads:[~2026-08-10 11:21 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-08-05 16:35 [PATCH v2 00/16] rust: io: support register projections and remove relative registers Gary Guo
2026-08-05 16:35 ` [PATCH v2 01/16] rust: io: add static `cast()` method for views Gary Guo
2026-08-10 9:29 ` Alexandre Courbot
2026-08-05 16:35 ` [PATCH v2 02/16] rust: io: add `IoRepr` trait Gary Guo
2026-08-10 9:30 ` Alexandre Courbot
2026-08-10 11:21 ` Gary Guo [this message]
2026-08-05 16:35 ` [PATCH v2 03/16] rust: io: support register projections Gary Guo
2026-08-10 9:30 ` Alexandre Courbot
2026-08-10 11:23 ` Gary Guo
2026-08-05 16:35 ` [PATCH v2 04/16] rust: io: register: handle one register at a time Gary Guo
2026-08-10 9:30 ` Alexandre Courbot
2026-08-05 16:35 ` [PATCH v2 05/16] rust: io: register extract offset computation to helper rules Gary Guo
2026-08-10 9:31 ` Alexandre Courbot
2026-08-05 16:35 ` [PATCH v2 06/16] rust: io: register: allow explicit base type specification Gary Guo
2026-08-10 9:32 ` Alexandre Courbot
2026-08-05 16:35 ` [PATCH v2 07/16] gpu: nova-core: specify base type for registers Gary Guo
2026-08-05 16:35 ` [PATCH v2 08/16] drm/tyr: " Gary Guo
2026-08-05 16:59 ` Gary Guo
2026-08-05 16:35 ` [PATCH v2 09/16] samples: rust: pci: " Gary Guo
2026-08-05 16:35 ` [PATCH v2 10/16] rust: io: register: make register have a typed base Gary Guo
2026-08-05 16:35 ` [PATCH v2 11/16] rust: io: register: support fixed offset register without bitfield Gary Guo
2026-08-05 16:35 ` [PATCH v2 12/16] gpu: nova-core: use projection for PFALCON and PFALCON2 registers Gary Guo
2026-08-05 16:35 ` [PATCH v2 13/16] gpu: nova-core: convert hshub0 from relative register to projection Gary Guo
2026-08-05 16:35 ` [PATCH v2 14/16] rust: io: register: remove relative registers Gary Guo
2026-08-05 16:35 ` [PATCH v2 15/16] rust: io: register: remove `Register` trait and cleanup macro Gary Guo
2026-08-05 16:35 ` [PATCH v2 16/16] rust: io: register: unify handling of register with/without bitfields Gary Guo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=DKL837PVU38R.3OONERSO8RMOW@garyguo.net \
--to=gary@garyguo.net \
--cc=a.hindborg@kernel.org \
--cc=acourbot@nvidia.com \
--cc=airlied@gmail.com \
--cc=aliceryhl@google.com \
--cc=bhelgaas@google.com \
--cc=bjorn3_gh@protonmail.com \
--cc=boqun@kernel.org \
--cc=dakr@kernel.org \
--cc=daniel.almeida@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=driver-core@lists.linux.dev \
--cc=kwilczynski@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=lossin@kernel.org \
--cc=nova-gpu@lists.linux.dev \
--cc=ojeda@kernel.org \
--cc=rust-for-linux@vger.kernel.org \
--cc=simona@ffwll.ch \
--cc=tamird@kernel.org \
--cc=tmgross@umich.edu \
--cc=work@onurozkan.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox