rust-osdev/xhci

A Rust library which is useful to handle xHCI

xhci's Issues

chore: rewrite the Context code with const generics

The only difference is the number of arrays. There is no need to create two modules.

This is a breaking change.

There should be an easy access to extended capabilities

Consistent port and interrupter register API with accessor `.set_at(i)` syntax

Currently, there are two register set arrays(ports and interrupters) but their designs are different and both have several issues:

• Since #142, interrupter type became a struct of accessors so that each field can be accessed independently. Port register set is still a struct of data fields and can't be updated field-wise. More consistent design between two register set arrays is required.
• Making new PortRegisterSet to pair with Port (just like InterrupterRegisterSet pairs with Interrupter) is inefficient and violates DRY principle.
• Current indexing method of interrupters (InterrupterRegisterSet::interrupter(&self, i)) actually maps and unmaps 5 physical addresses per indexing. I'm concerned about that if (un)mapping cost is more than zero, this would not be the optimal implementation.

---

I've crated a PR(toku-sa-n/accessor#44) for accessor .at(i) and .set_at(i) syntax for this purpose. Applying those changes in xhci crate, I've made a commit but I can't make it a PR until the accessor crate PR has been merged, so I opened this issue.

Changes

• Removed InterrupterRegisterSet (as non-mapping array-like type as it is now) and Interrupter (as struct of accessors)
• The name InterrupterRegisterSet is used for the indexed item type(struct of data field) of IRS array.
• InterrupterRegisterSet and PortRegisterSet both implements accessor::array::BoundSetGeneric trait to use .set_at(i) method.

implementation detail

Internally `accessor::array::BoundSetGeneric` trait is implemented by deriving `BoundSetGenericOfInterrupterRegisterSet` and `BoundSetGenericOfPortRegisterSet` types.

Usecase

• (before)

use xhci::Registers;

// let regs: Registers<M>;
let erdp = regs.interrupter_register_set.interrupter(i).erdp.read_volatile();
let portsc = regs.port_register_set.read_volatile_at(i).portsc;

• (after)

use xhci::Registers;
use xhci::accessor::array::BoundSetGeneric; // for `.set_at(i)` method

// let regs: Registers<M>;
let erdp = regs.interrupter_register_set.set_at(i).erdp.read_volatile();
let portsc = regs.port_register_set.set_at(i).portsc.read_volatile();

refactor: remove the "capability" suffix from modules of the Extended Capabilities

This is a breaking change.

Setters of the register should return mutable references to `Self`

So that the users can create method chains.

This is a breaking change.

feat: add the missing definitions

• Extended Capabilities
• Registers
  • Operational
  • Capability
  • Doorbell
  • Runtime
• TRBs
  • Event
  • Command
  • Transfer
• Contexts

Support for a 'Common raw TRB' type

Currently, individual TRB types are defined as transparent wrappers of [u32;4] with some impls.
However speaking about their union types, there are Allowed types which I feel inconvenient for several reasons:

• Allowed types are not transparently [u32;4], so we can't directly push that type into a ring.
• Several methods shared among all TRB types, such as extracting raw [u32;4] block or set/clear cycle bits,
  are implemented in Allowed types by matching discriminator arms.
  Although I didn't benchmark, this seems quite redundant to me. This possibly is a bottleneck given that the actual methods perform simple enough tasks like setting a bit or return the block itself.
• Since this crate doesn't have a Event Ring Segment Table Entry type, I had to build it in my own. (I'll open another issue for this)
  For that entry type, we need a method which takes a Event Ring buffer reference and convert it into an entry.
  What would be the type of that buffer argument? Definitely not &[Allowed], and &[[u32;4]] seems a little bit unclear.

Is it okay that an allocated buffer has the type &[[u32;4]]?

If not, I suggest to add a transparent wrapper for [u32; 4], say pub struct Block(pub(crate) [u32; 4]);, and define some methods like set/clear cycle bits on it.
Then the ERST Entry type will accept &[Block] type buffers, and concrete TRB types would wrap Block rather than raw [u32; 4]. (e.g. pub struct NoOp(Block); instead of pub struct NoOp([u32;4]);)

Should `InterruptRegisterSet` have `Single` members?

Currently, InterruptRegisterSet represents the actual structure of the Interrupt Register Set. This causes all of the registers are rewritten when one of the fields of these registers is changed.

I think the impact of rewriting is fairly small because none of the registers returns 0 when read, so the inconsistency of values before and after rewriting doesn't happen. However, I'm not sure.

Should RW1C bit fields have `set_xxx` and `clear_xxx` which do actually *set* and *clear* bits?

Currently, there are two methods for the RW1C bit: clear_xxx and set_0_xxx. The former sets the bit to 1, the latter sets it to 0.

Originally, only the former existed, because I thought there was no need to write 0 in the RW1C bit. However, as pointed out in #146, there are actually situations where it is necessary to write 0 to prevent the bit from being erased, which is why the latter was added in #148.

I didn't think anything when I merged #148, but later I thought this was obviously confusing. For example, the same clear_xxx method exists for RW bits, but this method sets the corresponding bit to 0.

Should I fix this and have the RW1C bits also have set_xxx/clear_xxx methods that actually set the bits to 1/0?

One problem is that the clear_xxx method will have the exact opposite meaning in a later version of this change, and thus must be clarified in the changelog.

Misc suggestions

Apologies for spamming bunch of issues.

• Mark capability registers as read-only.
  Currently they are read-write but they're supposed to be read-only according to the spec sheet.
• Rename doorbell::Register into doorbell::Doorbell (with an alias of doorbell::Register).
  There are lots of other registers and it is clearer to say Doorbell than to keep prefixing doorbell::Register.
• Chain-able context setters, just like register bitmap setters ( as in #83 )

I'll add a PR if these changes are appropriate.

Consider using the `register` crate

Currently, this crate has lots of pairs of getters and setters. The register crate is for defining MMIO registers, and this might help to reduce the number of lines.

Rewriting this with `volatile` crate?

Although I'm adding features of accessor crate now,
I noticed that my concept is similar to that of existing volatile crate.

e.g. introducing new bound accessor types over unbounded accessors are somewhat tiresome,
so we make a generic lifetime parameter on accessor types (and set default lifetime to 'static) etc..

The thing that lacks in volatile ptr is that there are no mappers, however.

(I should go to work shortly so I will add details more tomorrow)

Enforce Input and Output(Device) context their alignments

For input contexts, the input context pointer should be 16-byte aligned.
So the proper definition of input context would be:

#[repr(C, align(16))]
pub struct Input<const N: usize> {
    control: InputControl<N>,
    device: Device<N>,
}

I'm not sure if the Rust type layout guarantees this, since all contexts are mere wrappers of [u32;8] or other contexts which means we only have 4-byte alignment.

For output(device) context, DCBAA spec requires that the context pointer should be 64-byte aligned, but naively decorating #[repr(C, align(64)] to Device<N> results in 32-byte padding between Input control context and device context fields (assuming 32-byte contexts).

For this, I think we have two choices:

1. Add a Output context type which is a mere wrapper for the device context, except with 64-bit alignment guarantee.
   The Output context here should be defined like:

#[repr(C, align(64))]
#[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)]
pub struct Output<const N: usize> {
    device: Device<N>,
}
impl_constructor!(Output, "Output");
impl<const N: usize> Output<N> {
    const fn new() -> Self {
        Self {
            device: Device::new(),
        }
    }
}
impl<const N: usize> OutputHandler for Output<N> {
    fn device(&self) -> &dyn DeviceHandler {
        &self.device
    }

    fn device_mut(&mut self) -> &mut dyn DeviceHandler {
        &mut self.device
    }
}

/// A trait to handle Output Context.
pub trait OutputHandler {
    /// Returns a handler of Device Context.
    fn device(&self) -> &dyn DeviceHandler;

    /// Returns a mutable handler of Device Context.
    fn device_mut(&mut self) -> &mut dyn DeviceHandler;
}

It is questionable that whether we really need OutputHandler (impl DeviceHandler for Output will do all works) though.

A thought about the last line, irrelevant to this issue

`InputHandler` itself is something redundant just as the above `OutputHandler` is, since we can implement `InputControlHandler` and `DeviceHandler` directly for input contexts.

impl<const N: usize> InputControlHandler for Input<N> {} // already impl'ed in trait def. Needs impl `AsRef<[u32]>` and `AsMut<[u32]>` for `Input<N>`.
impl<const N: usize> DeviceHandler for Input<N> {
    fn slot(&self) -> &dyn SlotHandler {
        &self.device.slot
    }

    fn slot_mut(&mut self) -> &mut dyn SlotHandler {
        &mut self.device.slot
    }

    fn endpoint(&self, dci: usize) -> &dyn EndpointHandler {
        Self::assert_dci(dci);

        &self.device.endpoints[dci - 1]
    }

    fn endpoint_mut(&mut self, dci: usize) -> &mut dyn EndpointHandler {
        Self::assert_dci(dci);

        &mut self.device.endpoints[dci - 1]
    }
}

2. Split slot and EP contexts out of device context in Input context definition. This is a breaking change.
   In this case, the definitions should be like this:

#[repr(C, align(16))]
pub struct Input<const N: usize> {
    control: InputControl<N>,
    slot: Slot<N>,
    endpoints: [Endpoint<N>; NUM_OF_ENDPOINT_CONTEXTS],
}
#[repr(C, align(64))]
pub struct Device<const N: usize> {
    slot: Slot<N>,
    endpoints: [Endpoint<N>; NUM_OF_ENDPOINT_CONTEXTS],
}

Panic when unrecoverable errors happen

For example, we cannot handle the error of the unaligned address. This should cause a panic.

Add ERST Entry structure

It'd be nice to add a support to Event Ring Segment Table Entry structure.
The implementation would be something like this:
(I'll make an actual PR once we determine whether the ring buffer type should be &[[u32;4]] or wrapped-type like &[Block]. This is mentioned in #161 )

use super::block::Block;
use bit_field::BitField;
use core::ops::{Index, IndexMut};

/// The Event Ring Segment Table entry.
/// This plays the same role as an array pointer, and require special care to guarantee memory safety.
///
/// For example, the entry do not implement `Drop` trait, so the user should manually free its memory.
#[repr(transparent)]
#[derive(Clone, Copy, Debug)]
pub struct EventRingSegmentTableEntry([u32; 4]);

impl EventRingSegmentTableEntry {
    /// Create new segment table entry from a block buffer.
    /// 
    /// # Panics
    ///
    /// This method will panic if `len >= 4096`.
    pub unsafe fn from_buf(buf: &[Block]) -> Self {
        assert!(buf.len() <= u16::MAX as usize);

        let mut entry = Self([0; 4]);
        entry
            .set_ring_segment_base_address(buf.as_ptr() as usize as u64)
            .set_ring_segment_size(buf.len() as u16);
        entry
    }

    /// Returns the entry count of the segment.
    pub fn len(&self) -> usize {
        return self.ring_segment_size() as usize;
    }

    /// Returns the slice that this entry is representing.
    pub fn as_slice(&self) -> &[Block] {
        unsafe {
            let base = self.ring_segment_base_address() as *const _;
            let len = self.len();

            core::slice::from_raw_parts(base, len)
        }
    }

    /// Returns the mutable slice that this entry is representing.
    pub fn as_mut_slice(&mut self) -> &mut [Block] {
        unsafe {
            let base = self.ring_segment_base_address() as *mut _;
            let len = self.len();

            core::slice::from_raw_parts_mut(base, len)
        }
    }
}

impl EventRingSegmentTableEntry {
    /// Returns the value of the Ring Segment Base Address field.
    pub unsafe fn ring_segment_base_address(&self) -> u64 {
        let l: u64 = self.0[0].into();
        let u: u64 = self.0[1].into();

        (u << 32) | l
    }

    /// Sets the value of the Ring Segment Base Address field.
    ///
    /// # Panics
    ///
    /// This method panics if `p` is not 64-byte aligned.
    pub unsafe fn set_ring_segment_base_address(&mut self, p: u64) -> &mut Self {
        assert_eq!(
            p % 64,
            0,
            "The Ring Segment Base Address must be 64-byte aligned."
        );

        let l = p.get_bits(0..32);
        let u = p.get_bits(32..64);

        self.0[0] = l.try_into().unwrap();
        self.0[1] = u.try_into().unwrap();
        self
    }

    /// Returns the value of the Ring Segment Size field.
    ///
    /// This field represents entry count.
    pub fn ring_segment_size(&self) -> u16 {
        self.0[2].get_bits(0..16) as _
    }
    /// Sets the value of the Ring Segment Size field.
    ///
    /// The value should be entry count.
    pub unsafe fn set_ring_segment_size(&mut self, v: u16) -> &mut Self {
        self.0[2].set_bits(0..16, v.into());
        self
    }
    // rw_field!([2](0..16), ring_segment_size, "Ring Segment Size", u16);

    /// Returns the value of the ring segment end address.
    pub unsafe fn ring_segment_bound_address(&self) -> u64 {
        self.ring_segment_base_address() + (core::mem::size_of::<Block>() * self.ring_segment_size() as usize) as u64
    }
}

About implementing Mapper

This example leaves map/unmap unimplemented. Is it allowed to do so? At least it seems to me that it will panic in Single::new.

    /// #[derive(Clone)]
    /// struct MemoryMapper;
    /// impl Mapper for MemoryMapper {
    ///     unsafe fn map(&mut self, phys_base: usize, bytes: usize) -> NonZeroUsize {
    ///         unimplemented!()
    ///     }
    ///
    ///     fn unmap(&mut self, virt_base: usize, bytes: usize) {
    ///         unimplemented!()
    ///     }
    /// }
    ///
    /// let mapper = MemoryMapper;
    /// let r = unsafe { xhci::Registers::new(MMIO_BASE, mapper) };

https://github.com/rust-osdev/xhci/blob/main/src/registers/mod.rs

chore: split `set_xxx(bit)` into `set_xxx` and `clear_xxx`

Taking a bool value makes it unclear what the method does, even if it represents a bit.

This is a breaking change.