Re: [PATCH v3 1/2] rust: add initial scatterlist abstraction

[Andreas Hindborg <a.hindborg@kernel.org>]

"Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes:

> Add the rust abstraction for scatterlist. This allows use of the C
> scatterlist within Rust code which the caller can allocate themselves
> or to wrap existing kernel sg_table objects.
>
> Co-developed-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---

<cut>

> +    /// Obtain the raw `struct scatterlist *`.
> +    pub(crate) fn as_raw(&self) -> *mut bindings::scatterlist {
> +        self.0.get()
> +    }
> +
> +    /// Returns the DMA address of this SG entry.

In what address space? Device or CPU?

> +    pub fn dma_address(&self) -> bindings::dma_addr_t {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_address(self.0.get()) }
> +    }
> +
> +    /// Returns the length of this SG entry.
> +    pub fn dma_len(&self) -> u32 {
> +        // SAFETY: By the type invariant of `SGEntry`, ptr is valid.
> +        unsafe { bindings::sg_dma_len(self.0.get()) }
> +    }
> +
> +    /// Internal constructor helper to set this entry to point at a given page. Not to be used directly.
> +    fn set_page(&mut self, page: &Page, length: u32, offset: u32) {

Is it safe to call this with invalid length?

> +        let c: *mut bindings::scatterlist = self.0.get();
> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
> +        // `Page` invariant also ensure the pointer is valid.
> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
> +    }
> +}
> +
> +/// Trait implemented by all mapping states.
> +pub trait MappingState {}
> +
> +/// Trait implemented by all mapping states representing the fact that a `struct sg_table` is
> +/// mapped (and thus its DMA addresses are valid).
> +pub trait MappedState: MappingState {}
> +
> +/// Represents the fact that a `struct sg_table` is not DMA-mapped.

Could you explain what "DMA-mapped" means? Does it mean that everything
is set up so that a device can access the memory?

> +pub struct Unmapped;
> +impl MappingState for Unmapped {}
> +
> +/// Represents the fact that a `struct sg_table` is DMA-mapped by an external entity.

What is external entity?

> +pub struct BorrowedMapping;
> +impl MappingState for BorrowedMapping {}
> +impl MappedState for BorrowedMapping {}
> +
> +/// A managed DMA mapping of a `struct sg_table` to a given device.
> +///
> +/// The mapping is cleared when this object is dropped.
> +///
> +/// # Invariants
> +///
> +/// - The `scatterlist` pointer is valid for the lifetime of a `ManagedMapping` instance.
> +/// - The `Device` instance is within a [`kernel::device::Bound`] context.
> +pub struct ManagedMapping {
> +    dev: ARef<Device>,
> +    dir: DmaDataDirection,
> +    // This works because the `sgl` member of `struct sg_table` never moves, and the fact we can
> +    // build this implies that we have an exclusive reference to the `sg_table`, thus it cannot be
> +    // modified by anyone else.
> +    sgl: *mut bindings::scatterlist,
> +    orig_nents: ffi::c_uint,

Could you use a more descriptive name for this field? We don't _have_ to
contract all words to the least possible amount of letters.

> +}
> +

<cut>

> +
> +/// Provides a list of pages that can be used to build a `SGTable`.
> +pub trait SGTablePages {
> +    /// Returns an iterator to the pages providing the backing memory of `self`.
> +    ///
> +    /// Implementers should return an iterator which provides information regarding each page entry to
> +    /// build the `SGTable`. The first element in the tuple is a reference to the Page, the second element
> +    /// as the offset into the page, and the third as the length of data. The fields correspond to the
> +    /// first three fields of the C `struct scatterlist`.
> +    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)>;

Would it make sense to use a struct with proper names here, rather than
a tuple?

<cut>

> +    /// let mut pages = KVec::new();
> +    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +    /// let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +    /// let sgt = SGTable::new_owned(PagesArray(pages), GFP_KERNEL)?;
> +    /// # Ok::<(), Error>(())
> +    /// ```
> +    pub fn new_owned(pages: P, flags: kernel::alloc::Flags) -> Result<Self> {
> +        // SAFETY: `sgt` is not a reference.
> +        let mut sgt: bindings::sg_table = unsafe { core::mem::zeroed() };

I think this unsafe goes away with recent pin-init patches.

https://lore.kernel.org/r/20250523145125.523275-1-lossin@kernel.org

> +
> +        // SAFETY: The sgt pointer is from the Opaque-wrapped `sg_table` object hence is valid.
> +        let ret =
> +            unsafe { bindings::sg_alloc_table(&mut sgt, pages.entries() as u32, flags.as_raw()) };
> +        if ret != 0 {
> +            return Err(Error::from_errno(ret));
> +        }
> +        // SAFETY: We just successfully allocated `sgt`, hence the pointer is valid and have sole access to

nit: I would prefer "exclusive" over "sole".


Best regards,
Andreas Hindborg