Alexandre Courbot
2025-Oct-02 01:41 UTC
[PATCH v5 6/9] rust: bitfield: Add KUNIT tests for bitfield
On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote:> Add KUNIT tests to make sure the macro is working correctly. > > Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> > --- > rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 321 insertions(+) > > diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs > index fed19918c3b9..9a20bcd2eb60 100644 > --- a/rust/kernel/bitfield.rs > +++ b/rust/kernel/bitfield.rs > @@ -402,3 +402,324 @@ fn default() -> Self { > } > }; > } > + > +#[::kernel::macros::kunit_tests(kernel_bitfield)] > +mod tests { > + use core::convert::TryFrom; > + > + // Enum types for testing => and ?=> conversions > + #[derive(Debug, Clone, Copy, PartialEq)] > + enum MemoryType { > + Unmapped = 0, > + Normal = 1, > + Device = 2, > + Reserved = 3, > + } > + > + impl Default for MemoryType { > + fn default() -> Self { > + MemoryType::Unmapped > + } > + }Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and mark the variant you want as default with `#[default]` instead of providing a full impl block: #[derive(Debug, Default, Clone, Copy, PartialEq)] enum MemoryType { #[default] Unmapped = 0, Normal = 1, Device = 2, Reserved = 3, }> + > + impl TryFrom<u8> for MemoryType { > + type Error = u8; > + fn try_from(value: u8) -> Result<Self, Self::Error> { > + match value { > + 0 => Ok(MemoryType::Unmapped), > + 1 => Ok(MemoryType::Normal), > + 2 => Ok(MemoryType::Device), > + 3 => Ok(MemoryType::Reserved), > + _ => Err(value), > + } > + } > + } > + > + impl From<MemoryType> for u64 { > + fn from(mt: MemoryType) -> u64 { > + mt as u64 > + } > + } > + > + #[derive(Debug, Clone, Copy, PartialEq)] > + enum Priority { > + Low = 0, > + Medium = 1, > + High = 2, > + Critical = 3, > + } > + > + impl Default for Priority { > + fn default() -> Self { > + Priority::Low > + } > + } > + > + impl From<u8> for Priority { > + fn from(value: u8) -> Self { > + match value & 0x3 { > + 0 => Priority::Low, > + 1 => Priority::Medium, > + 2 => Priority::High, > + _ => Priority::Critical, > + } > + } > + } > + > + impl From<Priority> for u16 { > + fn from(p: Priority) -> u16 { > + p as u16 > + } > + } > + > + bitfield! { > + struct TestPageTableEntry(u64) { > + 0:0 present as bool; > + 1:1 writable as bool; > + 11:9 available as u8; > + 13:12 mem_type as u8 ?=> MemoryType; > + 17:14 extended_type as u8 ?=> MemoryType; // For testing failures > + 51:12 pfn as u64; > + 51:12 pfn_overlap as u64; > + 61:52 available2 as u16; > + } > + } > + > + bitfield! { > + struct TestControlRegister(u16) { > + 0:0 enable as bool; > + 3:1 mode as u8; > + 5:4 priority as u8 => Priority; > + 7:4 priority_nibble as u8; > + 15:8 channel as u8; > + } > + } > + > + bitfield! { > + struct TestStatusRegister(u8) { > + 0:0 ready as bool; > + 1:1 error as bool; > + 3:2 state as u8; > + 7:4 reserved as u8; > + 7:0 full_byte as u8; // For entire register > + } > + } > + > + #[test] > + fn test_single_bits() { > + let mut pte = TestPageTableEntry::default(); > + > + assert!(!pte.present()); > + assert!(!pte.writable()); > + > + pte = pte.set_present(true); > + assert!(pte.present()); > + > + pte = pte.set_writable(true); > + assert!(pte.writable()); > + > + pte = pte.set_writable(false); > + assert!(!pte.writable()); > + > + assert_eq!(pte.available(), 0); > + pte = pte.set_available(0x5); > + assert_eq!(pte.available(), 0x5);I'd suggest testing the actual raw value of the register on top of invoking the getter. That way you also test that: - The right field is actually written (i.e. if the offset is off by one, the getter will return the expected result even though the bitfield has the wrong value), - No other field has been affected. So something like: pte = pte.set_present(true); assert!(pte.present()); assert(pte.into(), 0x1u64); pte = pte.set_writable(true); assert!(pte.writable()); assert(pte.into(), 0x3u64); It might look a bit gross, but it is ok since these are not doctests that users are going to take as a reference, so we case improve test coverage at the detriment of readability.
Joel Fernandes
2025-Oct-03 15:23 UTC
[PATCH v5 6/9] rust: bitfield: Add KUNIT tests for bitfield
On 10/1/2025 9:41 PM, Alexandre Courbot wrote:> On Tue Sep 30, 2025 at 11:45 PM JST, Joel Fernandes wrote: >> Add KUNIT tests to make sure the macro is working correctly. >> >> Signed-off-by: Joel Fernandes <joelagnelf at nvidia.com> >> --- >> rust/kernel/bitfield.rs | 321 ++++++++++++++++++++++++++++++++++++++++ >> 1 file changed, 321 insertions(+) >> >> diff --git a/rust/kernel/bitfield.rs b/rust/kernel/bitfield.rs >> index fed19918c3b9..9a20bcd2eb60 100644 >> --- a/rust/kernel/bitfield.rs >> +++ b/rust/kernel/bitfield.rs >> @@ -402,3 +402,324 @@ fn default() -> Self { >> } >> }; >> } >> + >> +#[::kernel::macros::kunit_tests(kernel_bitfield)] >> +mod tests { >> + use core::convert::TryFrom; >> + >> + // Enum types for testing => and ?=> conversions >> + #[derive(Debug, Clone, Copy, PartialEq)] >> + enum MemoryType { >> + Unmapped = 0, >> + Normal = 1, >> + Device = 2, >> + Reserved = 3, >> + } >> + >> + impl Default for MemoryType { >> + fn default() -> Self { >> + MemoryType::Unmapped >> + } >> + } > > Tip: you can add `Default` to the `#[derive]` marker of `MemoryType` and > mark the variant you want as default with `#[default]` instead of > providing a full impl block: > > #[derive(Debug, Default, Clone, Copy, PartialEq)] > enum MemoryType { > #[default] > Unmapped = 0, > Normal = 1, > Device = 2, > Reserved = 3, > } >Good point, changed to this.>> + >> + impl TryFrom<u8> for MemoryType { >> + type Error = u8; >> + fn try_from(value: u8) -> Result<Self, Self::Error> { >> + match value { >> + 0 => Ok(MemoryType::Unmapped), >> + 1 => Ok(MemoryType::Normal), >> + 2 => Ok(MemoryType::Device), >> + 3 => Ok(MemoryType::Reserved), >> + _ => Err(value), >> + } >> + } >> + } >> + >> + impl From<MemoryType> for u64 { >> + fn from(mt: MemoryType) -> u64 { >> + mt as u64 >> + } >> + } >> + >> + #[derive(Debug, Clone, Copy, PartialEq)] >> + enum Priority { >> + Low = 0, >> + Medium = 1, >> + High = 2, >> + Critical = 3, >> + } >> + >> + impl Default for Priority { >> + fn default() -> Self { >> + Priority::Low >> + } >> + } >> + >> + impl From<u8> for Priority { >> + fn from(value: u8) -> Self { >> + match value & 0x3 { >> + 0 => Priority::Low, >> + 1 => Priority::Medium, >> + 2 => Priority::High, >> + _ => Priority::Critical, >> + } >> + } >> + } >> + >> + impl From<Priority> for u16 { >> + fn from(p: Priority) -> u16 { >> + p as u16 >> + } >> + } >> + >> + bitfield! { >> + struct TestPageTableEntry(u64) { >> + 0:0 present as bool; >> + 1:1 writable as bool; >> + 11:9 available as u8; >> + 13:12 mem_type as u8 ?=> MemoryType; >> + 17:14 extended_type as u8 ?=> MemoryType; // For testing failures >> + 51:12 pfn as u64; >> + 51:12 pfn_overlap as u64; >> + 61:52 available2 as u16; >> + } >> + } >> + >> + bitfield! { >> + struct TestControlRegister(u16) { >> + 0:0 enable as bool; >> + 3:1 mode as u8; >> + 5:4 priority as u8 => Priority; >> + 7:4 priority_nibble as u8; >> + 15:8 channel as u8; >> + } >> + } >> + >> + bitfield! { >> + struct TestStatusRegister(u8) { >> + 0:0 ready as bool; >> + 1:1 error as bool; >> + 3:2 state as u8; >> + 7:4 reserved as u8; >> + 7:0 full_byte as u8; // For entire register >> + } >> + } >> + >> + #[test] >> + fn test_single_bits() { >> + let mut pte = TestPageTableEntry::default(); >> + >> + assert!(!pte.present()); >> + assert!(!pte.writable()); >> + >> + pte = pte.set_present(true); >> + assert!(pte.present()); >> + >> + pte = pte.set_writable(true); >> + assert!(pte.writable()); >> + >> + pte = pte.set_writable(false); >> + assert!(!pte.writable()); >> + >> + assert_eq!(pte.available(), 0); >> + pte = pte.set_available(0x5); >> + assert_eq!(pte.available(), 0x5); > > I'd suggest testing the actual raw value of the register on top of > invoking the getter. That way you also test that:Sure, I am actually doing a raw check in a few other tests, but I could do so in this one as well.> > - The right field is actually written (i.e. if the offset is off by one, > the getter will return the expected result even though the bitfield > has the wrong value), > - No other field has been affected. > > So something like: > > pte = pte.set_present(true); > assert!(pte.present()); > assert(pte.into(), 0x1u64); > > pte = pte.set_writable(true); > assert!(pte.writable()); > assert(pte.into(), 0x3u64); > > It might look a bit gross, but it is ok since these are not doctests > that users are going to take as a reference, so we case improve test > coverage at the detriment of readability. >Ack. I will add these. Thanks for the review! (I am assuming with these changes you're Ok with me carrying your Reviewed-by tag on this patch as well, but please let me know if there is a concern.) - Joel
Alexandre Courbot
2025-Oct-04 00:38 UTC
[PATCH v5 6/9] rust: bitfield: Add KUNIT tests for bitfield
On Sat Oct 4, 2025 at 12:23 AM JST, Joel Fernandes wrote:>> - The right field is actually written (i.e. if the offset is off by one, >> the getter will return the expected result even though the bitfield >> has the wrong value), >> - No other field has been affected. >> >> So something like: >> >> pte = pte.set_present(true); >> assert!(pte.present()); >> assert(pte.into(), 0x1u64); >> >> pte = pte.set_writable(true); >> assert!(pte.writable()); >> assert(pte.into(), 0x3u64); >> >> It might look a bit gross, but it is ok since these are not doctests >> that users are going to take as a reference, so we case improve test >> coverage at the detriment of readability. >> > > Ack. I will add these. > > Thanks for the review! (I am assuming with these changes you're Ok with me > carrying your Reviewed-by tag on this patch as well, but please let me know if > there is a concern.)Please do not add tags that haven't been explicitly given. If we start assuming one another's stance about patches, the trust we can have in these tags is significantly reduced. Doing so also doesn't achieve anything in terms of efficiency; if I am ok with v3 I can give my Reviewed-by on it, and the tag can be picked up along with the patch when it is applied.