| Message ID | 20250217-nova_timer-v1-1-78c5ace2d987@nvidia.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | gpu: nova-core: add basic timer subdevice implementation | expand |
On Mon, 17 Feb 2025 at 15:07, Alexandre Courbot <acourbot@nvidia.com> wrote: > > It is common to build a u64 from its high and low parts obtained from > two 32-bit registers. Conversely, it is also common to split a u64 into > two u32s to write them into registers. Add an extension trait for u64 > that implement these methods in a new `num` module. > > It is expected that this trait will be extended with other useful > operations, and similar extension traits implemented for other types. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -59,6 +59,7 @@ > pub mod miscdevice; > #[cfg(CONFIG_NET)] > pub mod net; > +pub mod num; > pub mod of; > pub mod page; > #[cfg(CONFIG_PCI)] > diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b > --- /dev/null > +++ b/rust/kernel/num.rs > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Numerical and binary utilities for primitive types. > + > +/// Useful operations for `u64`. > +pub trait U64Ext { > + /// Build a `u64` by combining its `high` and `low` parts. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); > + /// ``` > + fn from_u32s(high: u32, low: u32) -> Self; > + > + /// Returns the `(high, low)` u32s that constitute `self`. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); > + /// ``` > + fn into_u32s(self) -> (u32, u32); > +} > + > +impl U64Ext for u64 { > + fn from_u32s(high: u32, low: u32) -> Self { > + ((high as u64) << u32::BITS) | low as u64 > + } > + > + fn into_u32s(self) -> (u32, u32) { > + ((self >> u32::BITS) as u32, self as u32) > + } > +} > > -- > 2.48.1 > > Looks good :) Reviewed-by: Sergio González Collado <sergio.collado@gmail.com>
Hi Alex, > On 17 Feb 2025, at 11:04, Alexandre Courbot <acourbot@nvidia.com> wrote: > > It is common to build a u64 from its high and low parts obtained from > two 32-bit registers. Conversely, it is also common to split a u64 into > two u32s to write them into registers. Add an extension trait for u64 > that implement these methods in a new `num` module. Thank you for working on that. I find myself doing this manually extremely often indeed. > > It is expected that this trait will be extended with other useful > operations, and similar extension traits implemented for other types. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -59,6 +59,7 @@ > pub mod miscdevice; > #[cfg(CONFIG_NET)] > pub mod net; > +pub mod num; > pub mod of; > pub mod page; > #[cfg(CONFIG_PCI)] > diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b > --- /dev/null > +++ b/rust/kernel/num.rs > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Numerical and binary utilities for primitive types. > + > +/// Useful operations for `u64`. > +pub trait U64Ext { > + /// Build a `u64` by combining its `high` and `low` parts. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); > + /// ``` > + fn from_u32s(high: u32, low: u32) -> Self; > + > + /// Returns the `(high, low)` u32s that constitute `self`. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); > + /// ``` > + fn into_u32s(self) -> (u32, u32); > +} > + > +impl U64Ext for u64 { > + fn from_u32s(high: u32, low: u32) -> Self { > + ((high as u64) << u32::BITS) | low as u64 > + } > + > + fn into_u32s(self) -> (u32, u32) { I wonder if a struct would make more sense here. Just recently I had to debug an issue where I forgot the right order for code I had just written. Something like: let (pgcount, pgsize) = foo(); where the function actually returned (pgsize, pgcount). A proper struct with `high` and `low` might be more verbose, but it rules out this issue. > + ((self >> u32::BITS) as u32, self as u32) > + } > +} > > -- > 2.48.1 > — Daniel >
On 17/02/2025 15:04, Alexandre Courbot wrote: > It is common to build a u64 from its high and low parts obtained from > two 32-bit registers. Conversely, it is also common to split a u64 into > two u32s to write them into registers. Add an extension trait for u64 > that implement these methods in a new `num` module. > > It is expected that this trait will be extended with other useful > operations, and similar extension traits implemented for other types. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > --- > rust/kernel/lib.rs | 1 + > rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ > 2 files changed, 33 insertions(+) > > diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs > index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 > --- a/rust/kernel/lib.rs > +++ b/rust/kernel/lib.rs > @@ -59,6 +59,7 @@ > pub mod miscdevice; > #[cfg(CONFIG_NET)] > pub mod net; > +pub mod num; > pub mod of; > pub mod page; > #[cfg(CONFIG_PCI)] > diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs > new file mode 100644 > index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b > --- /dev/null > +++ b/rust/kernel/num.rs > @@ -0,0 +1,32 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +//! Numerical and binary utilities for primitive types. > + > +/// Useful operations for `u64`. > +pub trait U64Ext { > + /// Build a `u64` by combining its `high` and `low` parts. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); > + /// ``` > + fn from_u32s(high: u32, low: u32) -> Self; > + > + /// Returns the `(high, low)` u32s that constitute `self`. > + /// > + /// ``` > + /// use kernel::num::U64Ext; > + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); > + /// ``` > + fn into_u32s(self) -> (u32, u32); > +} > + > +impl U64Ext for u64 { > + fn from_u32s(high: u32, low: u32) -> Self { > + ((high as u64) << u32::BITS) | low as u64 > + } > + > + fn into_u32s(self) -> (u32, u32) { > + ((self >> u32::BITS) as u32, self as u32) > + } > +} Just as a question: Would it make sense to make this more generic? For example u64 -> u32, u32 / u32, u32 -> u64 (as done here) u32 -> u16, u16 / u16, u16 -> u32 u16 -> u8, u8 / u8, u8 -> u16 Additionally, I wonder if this might be combined with the Integer trait [1]? But the usize and signed ones might not make sense here... Dirk [1] E.g. https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0
On Tue Feb 18, 2025 at 7:07 PM JST, Dirk Behme wrote: > On 17/02/2025 15:04, Alexandre Courbot wrote: >> It is common to build a u64 from its high and low parts obtained from >> two 32-bit registers. Conversely, it is also common to split a u64 into >> two u32s to write them into registers. Add an extension trait for u64 >> that implement these methods in a new `num` module. >> >> It is expected that this trait will be extended with other useful >> operations, and similar extension traits implemented for other types. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> rust/kernel/lib.rs | 1 + >> rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -59,6 +59,7 @@ >> pub mod miscdevice; >> #[cfg(CONFIG_NET)] >> pub mod net; >> +pub mod num; >> pub mod of; >> pub mod page; >> #[cfg(CONFIG_PCI)] >> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs >> new file mode 100644 >> index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b >> --- /dev/null >> +++ b/rust/kernel/num.rs >> @@ -0,0 +1,32 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! Numerical and binary utilities for primitive types. >> + >> +/// Useful operations for `u64`. >> +pub trait U64Ext { >> + /// Build a `u64` by combining its `high` and `low` parts. >> + /// >> + /// ``` >> + /// use kernel::num::U64Ext; >> + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); >> + /// ``` >> + fn from_u32s(high: u32, low: u32) -> Self; >> + >> + /// Returns the `(high, low)` u32s that constitute `self`. >> + /// >> + /// ``` >> + /// use kernel::num::U64Ext; >> + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); >> + /// ``` >> + fn into_u32s(self) -> (u32, u32); >> +} >> + >> +impl U64Ext for u64 { >> + fn from_u32s(high: u32, low: u32) -> Self { >> + ((high as u64) << u32::BITS) | low as u64 >> + } >> + >> + fn into_u32s(self) -> (u32, u32) { >> + ((self >> u32::BITS) as u32, self as u32) >> + } >> +} > Just as a question: Would it make sense to make this more generic? > > For example > > u64 -> u32, u32 / u32, u32 -> u64 (as done here) > u32 -> u16, u16 / u16, u16 -> u32 > u16 -> u8, u8 / u8, u8 -> u16 > > Additionally, I wonder if this might be combined with the Integer trait > [1]? But the usize and signed ones might not make sense here... > > Dirk > > [1] E.g. > > https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0 I agree something more generic would be nice. One drawback I see though is that it would have to use more generic (and lengthy) method names - i.e. `from_components(u32, u32)` instead of `from_u32s`. I quickly tried to write a completely generic trait where the methods are auto-implemented from constants and associated types, but got stuck by the impossibility to use `as` in that context without a macro. Regardless, I was looking for an already existing trait/module to leverage instead of introducing a whole new one, maybe the one you linked is what I was looking for?
Hi Daniel! On Tue Feb 18, 2025 at 6:10 AM JST, Daniel Almeida wrote: > Hi Alex, > >> On 17 Feb 2025, at 11:04, Alexandre Courbot <acourbot@nvidia.com> wrote: >> >> It is common to build a u64 from its high and low parts obtained from >> two 32-bit registers. Conversely, it is also common to split a u64 into >> two u32s to write them into registers. Add an extension trait for u64 >> that implement these methods in a new `num` module. > > Thank you for working on that. I find myself doing this manually extremely often indeed. Are you aware of existing upstream code that could benefit from this? This would allow me to split that patch out of this series. > > >> >> It is expected that this trait will be extended with other useful >> operations, and similar extension traits implemented for other types. >> >> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >> --- >> rust/kernel/lib.rs | 1 + >> rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ >> 2 files changed, 33 insertions(+) >> >> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >> index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 >> --- a/rust/kernel/lib.rs >> +++ b/rust/kernel/lib.rs >> @@ -59,6 +59,7 @@ >> pub mod miscdevice; >> #[cfg(CONFIG_NET)] >> pub mod net; >> +pub mod num; >> pub mod of; >> pub mod page; >> #[cfg(CONFIG_PCI)] >> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs >> new file mode 100644 >> index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b >> --- /dev/null >> +++ b/rust/kernel/num.rs >> @@ -0,0 +1,32 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> + >> +//! Numerical and binary utilities for primitive types. >> + >> +/// Useful operations for `u64`. >> +pub trait U64Ext { >> + /// Build a `u64` by combining its `high` and `low` parts. >> + /// >> + /// ``` >> + /// use kernel::num::U64Ext; >> + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); >> + /// ``` >> + fn from_u32s(high: u32, low: u32) -> Self; >> + >> + /// Returns the `(high, low)` u32s that constitute `self`. >> + /// >> + /// ``` >> + /// use kernel::num::U64Ext; >> + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); >> + /// ``` >> + fn into_u32s(self) -> (u32, u32); >> +} >> + >> +impl U64Ext for u64 { >> + fn from_u32s(high: u32, low: u32) -> Self { >> + ((high as u64) << u32::BITS) | low as u64 >> + } >> + >> + fn into_u32s(self) -> (u32, u32) { > > I wonder if a struct would make more sense here. > > Just recently I had to debug an issue where I forgot the > right order for code I had just written. Something like: > > let (pgcount, pgsize) = foo(); where the function actually > returned (pgsize, pgcount). > > A proper struct with `high` and `low` might be more verbose, but > it rules out this issue. Mmm indeed, so we would have client code looking like: let SplitU64 { high, low } = some_u64.into_u32(); instead of let (high, low) = some_u64.into_u32(); which is correct, and let (low, high) = some_u64.into_u32(); which is incorrect, but is likely to not be caught. Since the point of these methods is to avoid potential errors in what otherwise appears to be trivial code, I agree it would be better to avoid introducing a new trap because the elements of the returned tuple are not clearly named. Not sure which is more idiomatic here.
On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote: > > A proper struct with `high` and `low` might be more verbose, but > > it rules out this issue. > > Mmm indeed, so we would have client code looking like: > > let SplitU64 { high, low } = some_u64.into_u32(); > > instead of > > let (high, low) = some_u64.into_u32(); > > which is correct, and > > let (low, high) = some_u64.into_u32(); > > which is incorrect, but is likely to not be caught. I'm new to Rust, so let me see if I get this right. struct SplitU64 { high: u32, low: u32 } So if you want to extract the upper 32 bits of a u64, you have to do this: let split = some_u64.into_u32s(); let some_u32 = split.high; as opposed to your original design: let (some_u32, _) = some_u64.into_u32s(); Personally, I prefer the latter. The other advantage is that into_u32s and from_u32s are reciprocal: assert_eq!(u64::from_u32s(u64::into_u32s(some_u64)), some_u64); (or something like that)
On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote: > On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote: >> > A proper struct with `high` and `low` might be more verbose, but >> > it rules out this issue. >> >> Mmm indeed, so we would have client code looking like: >> >> let SplitU64 { high, low } = some_u64.into_u32(); >> >> instead of >> >> let (high, low) = some_u64.into_u32(); >> >> which is correct, and >> >> let (low, high) = some_u64.into_u32(); >> >> which is incorrect, but is likely to not be caught. > > I'm new to Rust, so let me see if I get this right. > > struct SplitU64 { > high: u32, > low: u32 > } > > So if you want to extract the upper 32 bits of a u64, you have to do this: > > let split = some_u64.into_u32s(); > let some_u32 = split.high; More likely this would be something like: let SplitU64 { high: some_u32, .. } = some_u64; Which is still a bit verbose, but a single-liner. Actually. How about adding methods to this trait that return either component? let some_u32 = some_u64.high_half(); let another_u32 = some_u64.low_half(); These should be used most of the times, and using destructuring/tuple would only be useful for a few select cases. > > as opposed to your original design: > > let (some_u32, _) = some_u64.into_u32s(); > > Personally, I prefer the latter. The other advantage is that into_u32s and > from_u32s are reciprocal: > > assert_eq!(u64::from_u32s(u64::into_u32s(some_u64)), some_u64); > > (or something like that) Yeah, having symmetry is definitely nice. OTOH there are no safeguards against mixing up the order and the high and low components, so a compromise will have to be made one way or the other. But if we also add the methods I proposed above, that question should matter less.
On 2/18/25 5:21 PM, Alexandre Courbot wrote: > On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote: >> On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote: ... > More likely this would be something like: > > let SplitU64 { high: some_u32, .. } = some_u64; > > Which is still a bit verbose, but a single-liner. > > Actually. How about adding methods to this trait that return either > component? > > let some_u32 = some_u64.high_half(); > let another_u32 = some_u64.low_half(); > > These should be used most of the times, and using destructuring/tuple > would only be useful for a few select cases. I think I like this approach best so far, because that is actually how drivers tend to use these values: one or the other 32 bits at a time. Registers are often grouped into 32-bit named registers, and driver code wants to refer to them one at a time (before breaking some of them down into smaller named fields)> The .high_half() and .low_half() approach matches that very closely. And it's simpler to read than the SplitU64 API, without losing anything we need, right? thanks,
On Wed Feb 19, 2025 at 12:24 PM JST, John Hubbard wrote: > On 2/18/25 5:21 PM, Alexandre Courbot wrote: >> On Wed Feb 19, 2025 at 5:51 AM JST, Timur Tabi wrote: >>> On Tue, 2025-02-18 at 22:16 +0900, Alexandre Courbot wrote: > ... >> More likely this would be something like: >> >> let SplitU64 { high: some_u32, .. } = some_u64; >> >> Which is still a bit verbose, but a single-liner. >> >> Actually. How about adding methods to this trait that return either >> component? >> >> let some_u32 = some_u64.high_half(); >> let another_u32 = some_u64.low_half(); >> >> These should be used most of the times, and using destructuring/tuple >> would only be useful for a few select cases. > > I think I like this approach best so far, because that is actually how > drivers tend to use these values: one or the other 32 bits at a time. > Registers are often grouped into 32-bit named registers, and driver code > wants to refer to them one at a time (before breaking some of them down > into smaller named fields)> > > The .high_half() and .low_half() approach matches that very closely. > And it's simpler to read than the SplitU64 API, without losing anything > we need, right? Yes, that looks like the optimal way to do this actually. It also doesn't introduce any overhead as the destructuring was doing both high_half() and low_half() in sequence, so in some cases it might even be more efficient. I'd just like to find a better naming. high() and low() might be enough? Or are there other suggestions?
> Actually. How about adding methods to this trait that return either > component? > > let some_u32 = some_u64.high_half(); > let another_u32 = some_u64.low_half(); > > These should be used most of the times, and using destructuring/tuple > would only be useful for a few select cases. > Indeed very nice!
On 2/19/25 4:51 AM, Alexandre Courbot wrote: > Yes, that looks like the optimal way to do this actually. It also > doesn't introduce any overhead as the destructuring was doing both > high_half() and low_half() in sequence, so in some cases it might > even be more efficient. > > I'd just like to find a better naming. high() and low() might be enough? > Or are there other suggestions? > Maybe use "32" instead of "half": .high_32() / .low_32() .upper_32() / .lower_32() thanks,
On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote: > > On 2/19/25 4:51 AM, Alexandre Courbot wrote: > > Yes, that looks like the optimal way to do this actually. It also > > doesn't introduce any overhead as the destructuring was doing both > > high_half() and low_half() in sequence, so in some cases it might > > even be more efficient. > > > > I'd just like to find a better naming. high() and low() might be enough? > > Or are there other suggestions? > > > > Maybe use "32" instead of "half": > > .high_32() / .low_32() > .upper_32() / .lower_32() > The C code currently does upper_32_bits and lower_32_bits, do we want to align or diverge here? Dave.
> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote: > > On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote: >> >> On 2/19/25 4:51 AM, Alexandre Courbot wrote: >>> Yes, that looks like the optimal way to do this actually. It also >>> doesn't introduce any overhead as the destructuring was doing both >>> high_half() and low_half() in sequence, so in some cases it might >>> even be more efficient. >>> >>> I'd just like to find a better naming. high() and low() might be enough? >>> Or are there other suggestions? >>> >> >> Maybe use "32" instead of "half": >> >> .high_32() / .low_32() >> .upper_32() / .lower_32() >> > > The C code currently does upper_32_bits and lower_32_bits, do we want > to align or diverge here? > > Dave. My humble suggestion here is to use the same nomenclature. `upper_32_bits` and `lower_32_bits` immediately and succinctly informs the reader of what is going on. — Daniel
On 2/19/25 3:13 PM, Daniel Almeida wrote: >> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote: >> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote: >>> On 2/19/25 4:51 AM, Alexandre Courbot wrote: >>>> Yes, that looks like the optimal way to do this actually. It also >>>> doesn't introduce any overhead as the destructuring was doing both >>>> high_half() and low_half() in sequence, so in some cases it might >>>> even be more efficient. >>>> >>>> I'd just like to find a better naming. high() and low() might be enough? >>>> Or are there other suggestions? >>>> >>> >>> Maybe use "32" instead of "half": >>> >>> .high_32() / .low_32() >>> .upper_32() / .lower_32() >>> >> >> The C code currently does upper_32_bits and lower_32_bits, do we want >> to align or diverge here? This sounds like a trick question, so I'm going to go with..."align". haha :) >> >> Dave. > > > My humble suggestion here is to use the same nomenclature. `upper_32_bits` and > `lower_32_bits` immediately and succinctly informs the reader of what is going on. > Yes. I missed the pre-existing naming in C, but since we have it and it's well-named as well, definitely this is the way to go. thanks,
On 18/02/2025 14:07, Alexandre Courbot wrote: > On Tue Feb 18, 2025 at 7:07 PM JST, Dirk Behme wrote: >> On 17/02/2025 15:04, Alexandre Courbot wrote: >>> It is common to build a u64 from its high and low parts obtained from >>> two 32-bit registers. Conversely, it is also common to split a u64 into >>> two u32s to write them into registers. Add an extension trait for u64 >>> that implement these methods in a new `num` module. >>> >>> It is expected that this trait will be extended with other useful >>> operations, and similar extension traits implemented for other types. >>> >>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> >>> --- >>> rust/kernel/lib.rs | 1 + >>> rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ >>> 2 files changed, 33 insertions(+) >>> >>> diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs >>> index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 >>> --- a/rust/kernel/lib.rs >>> +++ b/rust/kernel/lib.rs >>> @@ -59,6 +59,7 @@ >>> pub mod miscdevice; >>> #[cfg(CONFIG_NET)] >>> pub mod net; >>> +pub mod num; >>> pub mod of; >>> pub mod page; >>> #[cfg(CONFIG_PCI)] >>> diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b >>> --- /dev/null >>> +++ b/rust/kernel/num.rs >>> @@ -0,0 +1,32 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> + >>> +//! Numerical and binary utilities for primitive types. >>> + >>> +/// Useful operations for `u64`. >>> +pub trait U64Ext { >>> + /// Build a `u64` by combining its `high` and `low` parts. >>> + /// >>> + /// ``` >>> + /// use kernel::num::U64Ext; >>> + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); >>> + /// ``` >>> + fn from_u32s(high: u32, low: u32) -> Self; >>> + >>> + /// Returns the `(high, low)` u32s that constitute `self`. >>> + /// >>> + /// ``` >>> + /// use kernel::num::U64Ext; >>> + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); >>> + /// ``` >>> + fn into_u32s(self) -> (u32, u32); >>> +} >>> + >>> +impl U64Ext for u64 { >>> + fn from_u32s(high: u32, low: u32) -> Self { >>> + ((high as u64) << u32::BITS) | low as u64 >>> + } >>> + >>> + fn into_u32s(self) -> (u32, u32) { >>> + ((self >> u32::BITS) as u32, self as u32) >>> + } >>> +} >> Just as a question: Would it make sense to make this more generic? >> >> For example >> >> u64 -> u32, u32 / u32, u32 -> u64 (as done here) >> u32 -> u16, u16 / u16, u16 -> u32 >> u16 -> u8, u8 / u8, u8 -> u16 >> >> Additionally, I wonder if this might be combined with the Integer trait >> [1]? But the usize and signed ones might not make sense here... >> >> Dirk >> >> [1] E.g. >> >> https://github.com/senekor/linux/commit/7291dcc98e8ab74e34c1600784ec9ff3e2fa32d0 > > I agree something more generic would be nice. One drawback I see though > is that it would have to use more generic (and lengthy) method names - > i.e. `from_components(u32, u32)` instead of `from_u32s`. > > I quickly tried to write a completely generic trait where the methods > are auto-implemented from constants and associated types, but got stuck > by the impossibility to use `as` in that context without a macro. Being inspired by the Integer trait example [1] above, just as an idea, I wonder if anything like impl_split_merge! { (u64, u32), (u32, u16), (u16, u8), } would be implementable? > Regardless, I was looking for an already existing trait/module to > leverage instead of introducing a whole new one, maybe the one you > linked is what I was looking for? Cheers, Dirk
On Thu Feb 20, 2025 at 9:14 AM JST, John Hubbard wrote: > On 2/19/25 3:13 PM, Daniel Almeida wrote: >>> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote: >>> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote: >>>> On 2/19/25 4:51 AM, Alexandre Courbot wrote: >>>>> Yes, that looks like the optimal way to do this actually. It also >>>>> doesn't introduce any overhead as the destructuring was doing both >>>>> high_half() and low_half() in sequence, so in some cases it might >>>>> even be more efficient. >>>>> >>>>> I'd just like to find a better naming. high() and low() might be enough? >>>>> Or are there other suggestions? >>>>> >>>> >>>> Maybe use "32" instead of "half": >>>> >>>> .high_32() / .low_32() >>>> .upper_32() / .lower_32() >>>> >>> >>> The C code currently does upper_32_bits and lower_32_bits, do we want >>> to align or diverge here? > > This sounds like a trick question, so I'm going to go with..."align". haha :) > >>> >>> Dave. >> >> >> My humble suggestion here is to use the same nomenclature. `upper_32_bits` and >> `lower_32_bits` immediately and succinctly informs the reader of what is going on. >> > > Yes. I missed the pre-existing naming in C, but since we have it and it's > well-named as well, definitely this is the way to go. Agreed, I wasn't aware of the C equivalents either, but since they exist we should definitely use the same naming scheme.
On Fri, Feb 21, 2025 at 08:35:54PM +0900, Alexandre Courbot wrote: > On Thu Feb 20, 2025 at 9:14 AM JST, John Hubbard wrote: > > On 2/19/25 3:13 PM, Daniel Almeida wrote: > >>> On 19 Feb 2025, at 17:23, Dave Airlie <airlied@gmail.com> wrote: > >>> On Thu, 20 Feb 2025 at 06:22, John Hubbard <jhubbard@nvidia.com> wrote: > >>>> On 2/19/25 4:51 AM, Alexandre Courbot wrote: > >>>>> Yes, that looks like the optimal way to do this actually. It also > >>>>> doesn't introduce any overhead as the destructuring was doing both > >>>>> high_half() and low_half() in sequence, so in some cases it might > >>>>> even be more efficient. > >>>>> > >>>>> I'd just like to find a better naming. high() and low() might be enough? > >>>>> Or are there other suggestions? > >>>>> > >>>> > >>>> Maybe use "32" instead of "half": > >>>> > >>>> .high_32() / .low_32() > >>>> .upper_32() / .lower_32() > >>>> > >>> > >>> The C code currently does upper_32_bits and lower_32_bits, do we want > >>> to align or diverge here? > > > > This sounds like a trick question, so I'm going to go with..."align". haha :) > > > >>> > >>> Dave. > >> > >> > >> My humble suggestion here is to use the same nomenclature. `upper_32_bits` and > >> `lower_32_bits` immediately and succinctly informs the reader of what is going on. > >> > > > > Yes. I missed the pre-existing naming in C, but since we have it and it's > > well-named as well, definitely this is the way to go. > > Agreed, I wasn't aware of the C equivalents either, but since they exist > we should definitely use the same naming scheme. IIUC, we're still talking about extending the u64 primitive type. Hence, I think there is no necessity to do align with the corresponding C nameing scheme. I think this would only be the case if we'd write an abstraction for the C API. In this case though we extend an existing Rust type, so we should do something that aligns with the corresponding Rust type. In this specific case I think it goes hand in hand though. - Danilo
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs index 496ed32b0911a9fdbce5d26738b9cf7ef910b269..8c0c7c20a16aa96e3d3e444be3e03878650ddf77 100644 --- a/rust/kernel/lib.rs +++ b/rust/kernel/lib.rs @@ -59,6 +59,7 @@ pub mod miscdevice; #[cfg(CONFIG_NET)] pub mod net; +pub mod num; pub mod of; pub mod page; #[cfg(CONFIG_PCI)] diff --git a/rust/kernel/num.rs b/rust/kernel/num.rs new file mode 100644 index 0000000000000000000000000000000000000000..5e714cbda4575b8d74f50660580dc4c5683f8c2b --- /dev/null +++ b/rust/kernel/num.rs @@ -0,0 +1,32 @@ +// SPDX-License-Identifier: GPL-2.0 + +//! Numerical and binary utilities for primitive types. + +/// Useful operations for `u64`. +pub trait U64Ext { + /// Build a `u64` by combining its `high` and `low` parts. + /// + /// ``` + /// use kernel::num::U64Ext; + /// assert_eq!(u64::from_u32s(0x01234567, 0x89abcdef), 0x01234567_89abcdef); + /// ``` + fn from_u32s(high: u32, low: u32) -> Self; + + /// Returns the `(high, low)` u32s that constitute `self`. + /// + /// ``` + /// use kernel::num::U64Ext; + /// assert_eq!(u64::into_u32s(0x01234567_89abcdef), (0x1234567, 0x89abcdef)); + /// ``` + fn into_u32s(self) -> (u32, u32); +} + +impl U64Ext for u64 { + fn from_u32s(high: u32, low: u32) -> Self { + ((high as u64) << u32::BITS) | low as u64 + } + + fn into_u32s(self) -> (u32, u32) { + ((self >> u32::BITS) as u32, self as u32) + } +}
It is common to build a u64 from its high and low parts obtained from two 32-bit registers. Conversely, it is also common to split a u64 into two u32s to write them into registers. Add an extension trait for u64 that implement these methods in a new `num` module. It is expected that this trait will be extended with other useful operations, and similar extension traits implemented for other types. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- rust/kernel/lib.rs | 1 + rust/kernel/num.rs | 32 ++++++++++++++++++++++++++++++++ 2 files changed, 33 insertions(+)