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 <[email protected]>
>> ---
>> 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
