lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d238d9b7-8ec5-4063-8217-885d951d2f0c@weathered-steel.dev>
Date: Thu, 2 Oct 2025 02:16:16 +0000
From: Elle Rhumsaa <elle@...thered-steel.dev>
To: Alexandre Courbot <acourbot@...dia.com>,
 Joel Fernandes <joelagnelf@...dia.com>, linux-kernel@...r.kernel.org,
 rust-for-linux@...r.kernel.org, dri-devel@...ts.freedesktop.org,
 dakr@...nel.org
Cc: Alistair Popple <apopple@...dia.com>, Miguel Ojeda <ojeda@...nel.org>,
 Alex Gaynor <alex.gaynor@...il.com>, Boqun Feng <boqun.feng@...il.com>,
 Gary Guo <gary@...yguo.net>, bjorn3_gh@...tonmail.com,
 Benno Lossin <lossin@...nel.org>, Andreas Hindborg <a.hindborg@...nel.org>,
 Alice Ryhl <aliceryhl@...gle.com>, Trevor Gross <tmgross@...ch.edu>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 John Hubbard <jhubbard@...dia.com>, Timur Tabi <ttabi@...dia.com>,
 joel@...lfernandes.org, Yury Norov <yury.norov@...il.com>,
 Daniel Almeida <daniel.almeida@...labora.com>,
 Andrea Righi <arighi@...dia.com>, nouveau@...ts.freedesktop.org
Subject: Re: [PATCH v5 6/9] rust: bitfield: Add KUNIT tests for bitfield

On 10/2/25 1:41 AM, 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@...dia.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,
>      }

I would alternatively recommend to provide a `MemoryType::new` impl with 
a `const` definition:

```rust
impl MemoryType {
     pub const fn new() -> Self {

         Self::Unmapped

     }
}

impl Default for MemoryType {
     fn default() -> Self {
         Self::new()
     }
}
```

This pattern allows using `MemoryType::new()` in `const` contexts, while 
also providing the `Default` impl using the same default. It's somewhat 
of a workaround until we get `const` traits.

>> +
>> +    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.
>

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ