Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enforce Input and Output(Device) context their alignments #163

Open
paulsohn opened this issue Dec 1, 2023 · 0 comments
Open

Enforce Input and Output(Device) context their alignments #163

paulsohn opened this issue Dec 1, 2023 · 0 comments

Comments

@paulsohn
Copy link
Contributor

paulsohn commented Dec 1, 2023

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]
    }
}

  1. 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],
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant