[PATCH v3 0/3] Rust scatterlist abstractions

[PATCH v3 0/3] Rust scatterlist abstractions

[Qingsong Chen]

Hi All!

This is a version of scatterlist abstractions for Rust drivers.

Scatterlist is used for efficient management of memory buffers, which is
essential for many kernel-level operations such as Direct Memory Access
(DMA) transfers and crypto APIs.

This patch should be a good start to introduce the crypto APIs for Rust
drivers and to develop cipher algorithms in Rust later.

Changelog:
----------
v2 -> v3:
- Use `addr_of!` to avoid creating a reference to uninit memory.
- Mark `ScatterList::as_ref` and `ScatterList::as_mut` as unsafe.
- Revise some typos and check with `scripts/checkpatch.pl --codespell`.
- Add `# Errors` doc comment to some methods of `SgTable`.

v1 -> v2:
- Split the old patch into smaller parts.
- Remove the selftest module, and place those use cases in the doc.
- Repair some invalid hyperlinks in the doc.
- Put some `cfgs` inside functions to avoid boilerplate.
====================

Qingsong Chen (3):
  rust: kernel: add ScatterList abstraction
  rust: kernel: implement iterators for ScatterList
  rust: kernel: add SgTable abstraction

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  |  14 +
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 549 ++++++++++++++++++++++++++++++++
 4 files changed, 565 insertions(+)
 create mode 100644 rust/kernel/scatterlist.rs

-- 
2.40.1

[PATCH v3 1/3] rust: kernel: add ScatterList abstraction

[Qingsong Chen]

Add a wrapper for `struct scatterlist`.

Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
 rust/bindings/bindings_helper.h |   1 +
 rust/helpers.c                  |  14 ++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 293 ++++++++++++++++++++++++++++++++
 4 files changed, 309 insertions(+)
 create mode 100644 rust/kernel/scatterlist.rs

diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
index 50e7a76d5455..9cfa1ef92a04 100644
--- a/rust/bindings/bindings_helper.h
+++ b/rust/bindings/bindings_helper.h
@@ -10,6 +10,7 @@
 #include <linux/refcount.h>
 #include <linux/wait.h>
 #include <linux/sched.h>
+#include <linux/scatterlist.h>
 
 /* `bindgen` gets confused at certain things. */
 const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
diff --git a/rust/helpers.c b/rust/helpers.c
index 81e80261d597..4eb48f6c926e 100644
--- a/rust/helpers.c
+++ b/rust/helpers.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/sched/signal.h>
 #include <linux/wait.h>
+#include <linux/scatterlist.h>
 
 __noreturn void rust_helper_BUG(void)
 {
@@ -128,6 +129,19 @@ void rust_helper_put_task_struct(struct task_struct *t)
 }
 EXPORT_SYMBOL_GPL(rust_helper_put_task_struct);
 
+void rust_helper_sg_set_buf(struct scatterlist *sgl, const void *buf,
+			    unsigned int buflen)
+{
+	sg_set_buf(sgl, buf, buflen);
+}
+EXPORT_SYMBOL_GPL(rust_helper_sg_set_buf);
+
+void *rust_helper_sg_virt(struct scatterlist *sgl)
+{
+	return sg_virt(sgl);
+}
+EXPORT_SYMBOL_GPL(rust_helper_sg_virt);
+
 /*
  * We use `bindgen`'s `--size_t-is-usize` option to bind the C `size_t` type
  * as the Rust `usize` type, so we can use it in contexts where Rust
diff --git a/rust/kernel/lib.rs b/rust/kernel/lib.rs
index 676995d4e460..d8dbcde4ad5c 100644
--- a/rust/kernel/lib.rs
+++ b/rust/kernel/lib.rs
@@ -40,6 +40,7 @@ pub mod init;
 pub mod ioctl;
 pub mod prelude;
 pub mod print;
+pub mod scatterlist;
 mod static_assert;
 #[doc(hidden)]
 pub mod std_vendor;
diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
new file mode 100644
index 000000000000..7fb8f3326ff3
--- /dev/null
+++ b/rust/kernel/scatterlist.rs
@@ -0,0 +1,293 @@
+// SPDX-License-Identifier: GPL-2.0
+
+//! Scatterlist.
+//!
+//! C header: [`include/linux/scatterlist.h`](../../../../include/linux/scatterlist.h)
+
+use crate::prelude::*;
+use crate::types::Opaque;
+use core::marker::PhantomData;
+use core::ptr::{addr_of, NonNull};
+
+/// Wrap the kernel's `struct scatterlist`.
+///
+/// According to the design of kernel's `struct sg_table`, the `page_link`
+/// field of one `scatterlist` may contain a pointer to another. That is,
+/// there could exist external pointers to it, making it necessary to pin
+/// this struct.
+///
+/// # Invariants
+///
+/// All instances should be valid, either created by the `new` constructor
+/// (see [`pin_init`]), or transmuted from raw pointers (which could be used
+/// to reference a valid entry of `sg_table`).
+///
+/// # Examples
+///
+/// The following are some use cases of [`ScatterList`].
+///
+/// ```rust
+/// use core::pin::pin;
+/// # use kernel::error::Result;
+/// # use kernel::scatterlist::ScatterList;
+///
+/// // Prepare memory buffers.
+/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
+/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
+///
+/// // Allocate an instance on stack.
+/// kernel::stack_pin_init!(let foo = ScatterList::new(&buf0));
+/// let mut foo: Pin<&mut ScatterList<'_>> = foo;
+/// assert_eq!(foo.length(), 512);
+///
+/// // Allocate an instance by Box::pin_init.
+/// let bar: Pin<Box<ScatterList<'_>>> = Box::pin_init(ScatterList::new(&buf1))?;
+/// assert_eq!(bar.length(), 512);
+///
+/// // Assert other attributes of an instance.
+/// assert_eq!(foo.is_dma_bus_address(), false);
+/// assert_eq!(foo.is_chain(), false);
+/// assert_eq!(foo.is_last(), true);
+/// assert_eq!(foo.count(), 1);
+///
+/// // Get an immutable reference to memory buffer.
+/// assert_eq!(foo.get_buf(), [0u8; 512]);
+///
+/// // Reset the memory buffer.
+/// foo.set_buf(&buf1);
+/// assert_eq!(foo.get_buf(), [1u8; 512]);
+///
+/// // Get a mutable reference to memory buffer.
+/// foo.get_mut_buf().fill(2);
+/// assert_eq!(foo.get_buf(), [2u8; 512]);
+/// ```
+#[pin_data]
+pub struct ScatterList<'a> {
+    #[pin]
+    opaque: Opaque<bindings::scatterlist>,
+    _p: PhantomData<&'a mut bindings::scatterlist>,
+}
+
+impl<'a> ScatterList<'a> {
+    /// Construct a new initializer.
+    pub fn new(buf: &'a Pin<&mut [u8]>) -> impl PinInit<ScatterList<'a>> {
+        // SAFETY: `slot` is valid while the closure is called, the memory
+        // buffer is pinned and valid.
+        unsafe {
+            init::pin_init_from_closure(move |slot: *mut Self| {
+                // `slot` contains uninit memory, avoid creating a reference.
+                let opaque = addr_of!((*slot).opaque);
+                let sgl = Opaque::raw_get(opaque);
+
+                bindings::sg_set_buf(sgl, buf.as_ptr() as _, buf.len() as _);
+                (*sgl).page_link |= bindings::SG_END as u64;
+                (*sgl).page_link &= !(bindings::SG_CHAIN as u64);
+                Ok(())
+            })
+        }
+    }
+
+    /// Obtain a pinned reference to an immutable scatterlist from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// The `ptr` is null, or pointed to a valid `scatterlist`.
+    unsafe fn as_ref(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a Self>> {
+        // SAFETY: `sgl` is non-null and valid.
+        NonNull::new(ptr).map(|sgl| Pin::new(unsafe { &*(sgl.as_ptr() as *const ScatterList<'_>) }))
+    }
+
+    /// Obtain a pinned reference to a mutable scatterlist from a raw pointer.
+    ///
+    /// # Safety
+    ///
+    /// The `ptr` is null, or pointed to a valid `scatterlist`.
+    unsafe fn as_mut(ptr: *mut bindings::scatterlist) -> Option<Pin<&'a mut Self>> {
+        // SAFETY: `sgl` is non-null and valid.
+        NonNull::new(ptr)
+            .map(|sgl| Pin::new(unsafe { &mut *(sgl.as_ptr() as *mut ScatterList<'_>) }))
+    }
+}
+
+impl ScatterList<'_> {
+    /// Return the offset of memory buffer into the page.
+    pub fn offset(&self) -> usize {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { (*self.opaque.get()).offset as _ }
+    }
+
+    /// Return the length of memory buffer.
+    pub fn length(&self) -> usize {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { (*self.opaque.get()).length as _ }
+    }
+
+    /// Return the mapped DMA address.
+    ///
+    /// # Safety
+    ///
+    /// It is only valid after this scatterlist has been mapped to some bus
+    /// address and then called `set_dma` method to setup it.
+    pub fn dma_address(&self) -> usize {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { (*self.opaque.get()).dma_address as _ }
+    }
+
+    /// Return the mapped DMA length.
+    ///
+    /// # Safety
+    ///
+    /// It is only valid after this scatterlist has been mapped to some bus
+    /// address and then called `set_dma` method to setup it.
+    pub fn dma_length(&self) -> usize {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
+        unsafe {
+            (*self.opaque.get()).dma_length as _
+        }
+        #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
+        unsafe {
+            (*self.opaque.get()).length as _
+        }
+    }
+
+    /// Setup the DMA address and length.
+    pub fn set_dma(&mut self, addr: usize, len: usize) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        #[cfg(CONFIG_NEED_SG_DMA_LENGTH)]
+        unsafe {
+            (*self.opaque.get()).dma_address = addr as _;
+            (*self.opaque.get()).dma_length = len as _;
+        }
+        #[cfg(not(CONFIG_NEED_SG_DMA_LENGTH))]
+        unsafe {
+            (*self.opaque.get()).dma_address = addr as _;
+            (*self.opaque.get()).length = len as _;
+        }
+        self.dma_mark_bus_address();
+    }
+
+    /// Return `true` if it is mapped with a DMA address.
+    pub fn is_dma_bus_address(&self) -> bool {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        #[cfg(CONFIG_PCI_P2PDMA)]
+        unsafe {
+            ((*self.opaque.get()).dma_flags & bindings::SG_DMA_BUS_ADDRESS) != 0
+        }
+        #[cfg(not(CONFIG_PCI_P2PDMA))]
+        false
+    }
+
+    /// Mark it as mapped to a DMA address.
+    pub fn dma_mark_bus_address(&mut self) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        #[cfg(CONFIG_PCI_P2PDMA)]
+        unsafe {
+            (*self.opaque.get()).dma_flags |= bindings::SG_DMA_BUS_ADDRESS;
+        }
+    }
+
+    /// Mark it as `not` mapped to a DMA address.
+    pub fn dma_unmark_bus_address(&mut self) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        #[cfg(CONFIG_PCI_P2PDMA)]
+        unsafe {
+            (*self.opaque.get()).dma_flags &= !bindings::SG_DMA_BUS_ADDRESS;
+        }
+    }
+
+    /// Return `true` if it is a chainable entry.
+    pub fn is_chain(&self) -> bool {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe {
+            ((*self.opaque.get()).page_link
+                & (bindings::SG_PAGE_LINK_MASK as u64)
+                & (bindings::SG_CHAIN as u64))
+                != 0
+        }
+    }
+
+    /// Transfer this scatterlist as a chainable entry, linked to `sgl`.
+    pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) {
+        let addr = sgl.opaque.get() as u64;
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe {
+            (*self.opaque.get()).offset = 0;
+            (*self.opaque.get()).length = 0;
+            (*self.opaque.get()).page_link = addr | (bindings::SG_CHAIN as u64);
+        }
+        self.unmark_end();
+    }
+
+    /// Return `true` if it is the last normal entry in scatterlist.
+    pub fn is_last(&self) -> bool {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe {
+            ((*self.opaque.get()).page_link
+                & (bindings::SG_PAGE_LINK_MASK as u64)
+                & (bindings::SG_END as u64))
+                != 0
+        }
+    }
+
+    /// Mark it as the last normal entry in scatterlist.
+    pub fn mark_end(&mut self) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe {
+            (*self.opaque.get()).page_link |= bindings::SG_END as u64;
+            (*self.opaque.get()).page_link &= !(bindings::SG_CHAIN as u64);
+        }
+    }
+
+    /// Mark it as `not` the last normal entry in scatterlist.
+    pub fn unmark_end(&mut self) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe {
+            (*self.opaque.get()).page_link &= !(bindings::SG_END as u64);
+        }
+    }
+
+    /// Get an immutable reference to memory buffer.
+    pub fn get_buf(&self) -> &[u8] {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        let addr = unsafe { bindings::sg_virt(self.opaque.get()) };
+        let len = self.length();
+        // SAFETY: `addr` is returned by `sg_virt`, so it is valid. And the memory
+        // buffer is set by `new` constructor or `set_buf` method, so it's pinned
+        // and valid.
+        unsafe { core::slice::from_raw_parts(addr as _, len) }
+    }
+
+    /// Get a mutable reference to memory buffer.
+    pub fn get_mut_buf(&mut self) -> &mut [u8] {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        let addr = unsafe { bindings::sg_virt(self.opaque.get()) };
+        let len = self.length();
+        // SAFETY: `addr` is returned by `sg_virt`, so it is valid. And the memory
+        // buffer is set by `new` constructor or `set_buf` method, so it's pinned
+        // and valid.
+        unsafe { core::slice::from_raw_parts_mut(addr as _, len) }
+    }
+
+    /// Set the memory buffer.
+    pub fn set_buf(&mut self, buf: &Pin<&mut [u8]>) {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        // And `buf` is pinned and valid.
+        unsafe {
+            bindings::sg_set_buf(
+                self.opaque.get(),
+                buf.as_ptr() as *const core::ffi::c_void,
+                buf.len() as core::ffi::c_uint,
+            );
+        }
+    }
+
+    /// Return the total count of normal entries in scatterlist.
+    ///
+    /// It allows to know how many entries are in scatterlist, taking into
+    /// account chaining as well.
+    pub fn count(&self) -> usize {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { bindings::sg_nents(self.opaque.get()) as _ }
+    }
+}
-- 
2.40.1

Re: [PATCH v3 1/3] rust: kernel: add ScatterList abstraction

[Martin Rodriguez Reboredo]

On 6/10/23 07:49, Qingsong Chen wrote:
> Add a wrapper for `struct scatterlist`.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

[PATCH v3 2/3] rust: kernel: implement iterators for ScatterList

[Qingsong Chen]

ScatterList could be transmuted from raw pointers of a valid
`sg_table`. Then we can use those iterators to access the
following normal entries.

Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
 rust/kernel/scatterlist.rs | 58 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
index 7fb8f3326ff3..41e268b93c9e 100644
--- a/rust/kernel/scatterlist.rs
+++ b/rust/kernel/scatterlist.rs
@@ -290,4 +290,62 @@ impl ScatterList<'_> {
         // SAFETY: By the type invariant, we know that `self.opaque` is valid.
         unsafe { bindings::sg_nents(self.opaque.get()) as _ }
     }
+
+    /// Get an iterator for immutable references.
+    pub fn iter(&self) -> Iter<'_> {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { Iter(ScatterList::as_ref(self.opaque.get())) }
+    }
+
+    /// Get an iterator for mutable references.
+    pub fn iter_mut(&mut self) -> IterMut<'_> {
+        // SAFETY: By the type invariant, we know that `self.opaque` is valid.
+        unsafe { IterMut(ScatterList::as_mut(self.opaque.get())) }
+    }
+}
+
+/// An iterator that yields [`Pin<&ScatterList>`].
+///
+/// Only iterate normal scatterlist entries, chainable entry will be skipped.
+pub struct Iter<'a>(Option<Pin<&'a ScatterList<'a>>>);
+
+impl<'a> Iterator for Iter<'a> {
+    type Item = Pin<&'a ScatterList<'a>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let ptr = match &self.0 {
+            None => return None,
+            Some(sgl) => sgl.opaque.get(),
+        };
+        // SAFETY: `ptr` is from `self.opaque`, it is valid by the type invariant.
+        // And `next` is null, or the next valid scatterlist entry.
+        unsafe {
+            let next = bindings::sg_next(ptr);
+            self.0 = ScatterList::as_ref(next);
+            ScatterList::as_ref(ptr)
+        }
+    }
+}
+
+/// An iterator that yields [`Pin<&mut ScatterList>`].
+///
+/// Only iterate normal scatterlist entries, chainable entry will be skipped.
+pub struct IterMut<'a>(Option<Pin<&'a mut ScatterList<'a>>>);
+
+impl<'a> Iterator for IterMut<'a> {
+    type Item = Pin<&'a mut ScatterList<'a>>;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        let ptr = match &self.0 {
+            None => return None,
+            Some(sgl) => sgl.opaque.get(),
+        };
+        // SAFETY: `ptr` is from `self.opaque`, it is valid by the type invariant.
+        // And `next` is null, or the next valid scatterlist entry.
+        unsafe {
+            let next = bindings::sg_next(ptr);
+            self.0 = ScatterList::as_mut(next);
+            ScatterList::as_mut(ptr)
+        }
+    }
 }
-- 
2.40.1

Re: [PATCH v3 2/3] rust: kernel: implement iterators for ScatterList

[Martin Rodriguez Reboredo]

On 6/10/23 07:49, Qingsong Chen wrote:
> ScatterList could be transmuted from raw pointers of a valid
> `sg_table`. Then we can use those iterators to access the
> following normal entries.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

[PATCH v3 3/3] rust: kernel: add SgTable abstraction

[Qingsong Chen]

SgTable is similar to `struct sg_table`, consisted of
scatterlist entries, and could be chained with each other.

Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
---
 rust/kernel/scatterlist.rs | 198 +++++++++++++++++++++++++++++++++++++
 1 file changed, 198 insertions(+)

diff --git a/rust/kernel/scatterlist.rs b/rust/kernel/scatterlist.rs
index 41e268b93c9e..870904d53e81 100644
--- a/rust/kernel/scatterlist.rs
+++ b/rust/kernel/scatterlist.rs
@@ -349,3 +349,201 @@ impl<'a> Iterator for IterMut<'a> {
         }
     }
 }
+
+/// A [`ScatterList`] table of fixed `N` entries.
+///
+/// According to the design of kernel's `struct sg_table`, the `page_link`
+/// field of one `scatterlist` may contain a pointer to another. That is,
+/// there could exist external pointers to it, making it necessary to pin
+/// this struct.
+///
+/// If the table is chainable, the last entry will be reserved for chaining.
+/// The recommended way to create such instances is with the [`pin_init`].
+///
+/// # Examples
+///
+/// The following are some use cases of [`SgTable<N>`].
+///
+/// ```rust
+/// use core::pin::pin;;
+/// # use kernel::error::Result;
+/// # use kernel::scatterlist::{ScatterList, SgTable};
+///
+/// // Prepare memory buffers.
+/// let buf0: Pin<&mut [u8]> = pin!([0u8; 512]);
+/// let buf1: Pin<&mut [u8]> = pin!([1u8; 512]);
+/// let mut entries: Vec<Pin<&mut [u8]>> = Vec::<Pin<&mut [u8]>>::new();
+/// entries.try_push(buf0)?;
+/// entries.try_push(buf1)?;
+///
+/// // Allocate an instance on stack.
+/// kernel::stack_try_pin_init!(let foo =? SgTable::new(entries.as_slice(), false));
+/// let mut foo: Pin<&mut SgTable<'_, 2>> = foo;
+/// assert_eq!(foo.count(), 2);
+///
+/// // Allocate a chainable instance by Box::pin_init.
+/// let mut bar: Pin<Box<SgTable<'_, 3>>> = Box::pin_init(SgTable::new(entries.as_slice(), true))?;
+/// assert_eq!(bar.count(), 2);
+///
+/// // Chain two `SgTable` together.
+/// bar.chain_sgt(foo.as_mut())?;
+/// assert_eq!(bar.count(), 4);
+///
+/// // Get an immutable reference to the entry in the table.
+/// let sgl1: Pin<&ScatterList<'_>> = bar.get(1).ok_or(EINVAL)?;
+/// assert_eq!(sgl1.count(), 3);
+/// assert_eq!(sgl1.get_buf(), [1u8; 512]);
+///
+/// // Get a mutable reference to the entry in the table.
+/// let sgl2: Pin<&mut ScatterList<'_>> = bar.get_mut(2).ok_or(EINVAL)?;
+/// assert_eq!(sgl2.count(), 2);
+/// assert_eq!(sgl2.get_buf(), [0u8; 512]);
+///
+/// // Try to get a non-exist entry from the table.
+/// let sgl4 = bar.get(4);
+/// assert_eq!(sgl4.is_none(), true);
+///
+/// // Split the first table from chained scatterlist.
+/// bar.split()?;
+/// assert_eq!(bar.count(), 2);
+/// ```
+#[pin_data]
+pub struct SgTable<'a, const N: usize> {
+    #[pin]
+    entries: [ScatterList<'a>; N],
+}
+
+impl<'a, const N: usize> SgTable<'a, N> {
+    /// Construct a new initializer.
+    ///
+    /// # Errors
+    ///
+    /// The length of `entries` should exactly be the available size of [`SgTable<N>`],
+    /// or else an error is returned.
+    ///
+    /// If the table is `chainable`, the available size is `N - 1`, because one entry
+    /// should be reserved for chaining.
+    pub fn new(
+        entries: &'a [Pin<&mut [u8]>],
+        chainable: bool,
+    ) -> impl PinInit<SgTable<'a, N>, Error> {
+        build_assert!(N > 0);
+        // SAFETY: `slot` is valid while the closure is called, the `entries` are
+        // pinned and valid.
+        unsafe {
+            init::pin_init_from_closure(move |slot: *mut Self| {
+                let mut nr_entry = N;
+                if chainable {
+                    nr_entry -= 1;
+                }
+                if nr_entry == 0 || entries.len() != nr_entry {
+                    return Err(EINVAL);
+                }
+
+                for i in 0..nr_entry {
+                    // `slot` contains uninit memory, avoid creating a reference.
+                    let opaque = addr_of!((*slot).entries[i].opaque);
+                    let sgl = Opaque::raw_get(opaque);
+
+                    bindings::sg_set_buf(sgl, entries[i].as_ptr() as _, entries[i].len() as _);
+                    if i + 1 == nr_entry {
+                        (*sgl).page_link |= bindings::SG_END as u64;
+                        (*sgl).page_link &= !(bindings::SG_CHAIN as u64);
+                    }
+                }
+                Ok(())
+            })
+        }
+    }
+
+    /// Chain two [`SgTable`] together.
+    ///
+    /// Transfer the last entry of this table as a chainable pointer to the
+    /// first entry of `sgt` SgTable.
+    ///
+    /// # Errors
+    ///
+    /// Return an error if this table is not chainable or has been chained.
+    pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
+        if self.count() != N - 1 {
+            return Err(EINVAL);
+        }
+        self.entries[N - 2].unmark_end();
+
+        // SAFETY: `sgt.entries` are initialized by the `new` constructor,
+        // so it's valid.
+        let next = unsafe { ScatterList::as_mut(sgt.entries[0].opaque.get()).unwrap() };
+        self.entries[N - 1].chain_sgl(next);
+        Ok(())
+    }
+
+    /// Chain [`SgTable`] and [`ScatterList`] together.
+    ///
+    /// Transfer the last entry of this table as a chainable pointer to `sgl`
+    /// scatterlist.
+    ///
+    /// # Errors
+    ///
+    /// Return an error if this table is not chainable or has been chained.
+    pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
+        if self.count() != N - 1 {
+            return Err(EINVAL);
+        }
+        self.entries[N - 2].unmark_end();
+        self.entries[N - 1].chain_sgl(sgl);
+        Ok(())
+    }
+
+    /// Split the first table from chained scatterlist.
+    ///
+    /// # Errors
+    ///
+    /// Return an error if this table is not chainable or has not been chained.
+    pub fn split(&mut self) -> Result {
+        if !self.entries[N - 1].is_chain() {
+            return Err(EINVAL);
+        }
+        self.entries[N - 2].mark_end();
+        Ok(())
+    }
+
+    /// Return the total count of normal entries in the table.
+    ///
+    /// It allows to know how many entries are in the table, taking into account
+    /// chaining as well.
+    pub fn count(&self) -> usize {
+        // SAFETY: `self.entries` are initialized by the `new` constructor,
+        // so it's valid.
+        unsafe { bindings::sg_nents(self.entries[0].opaque.get()) as _ }
+    }
+
+    /// Get an immutable reference to `n`th entry in the table.
+    ///
+    /// Like most indexing operations, the count starts from zero. Return None
+    /// if `n` is greater than or equal to the total count of entries.
+    pub fn get(&self, n: usize) -> Option<Pin<&ScatterList<'_>>> {
+        self.iter().nth(n)
+    }
+
+    /// Get a mutable reference to `n`th entry in the table.
+    ///
+    /// Like most indexing operations, the count starts from zero. Return None
+    /// if `n` is greater than or equal to the number of total entries.
+    pub fn get_mut(&mut self, n: usize) -> Option<Pin<&mut ScatterList<'_>>> {
+        self.iter_mut().nth(n)
+    }
+
+    /// Get an iterator for immutable entries.
+    pub fn iter(&self) -> Iter<'_> {
+        // SAFETY: `self.entries` are initialized by the `new` constructor,
+        // so it's valid.
+        unsafe { Iter(ScatterList::as_ref(self.entries[0].opaque.get())) }
+    }
+
+    /// Get an iterator for mutable entries.
+    pub fn iter_mut(&mut self) -> IterMut<'_> {
+        // SAFETY: `self.entries` are initialized by the `new` constructor,
+        // so it's valid.
+        unsafe { IterMut(ScatterList::as_mut(self.entries[0].opaque.get())) }
+    }
+}
-- 
2.40.1

Re: [PATCH v3 3/3] rust: kernel: add SgTable abstraction

[Martin Rodriguez Reboredo]

On 6/10/23 07:49, Qingsong Chen wrote:
> SgTable is similar to `struct sg_table`, consisted of
> scatterlist entries, and could be chained with each other.
> 
> Signed-off-by: Qingsong Chen <changxian.cqs@antgroup.com>
> ---
> [...]
> +impl<'a, const N: usize> SgTable<'a, N> {
> +    /// Construct a new initializer.
> +    ///
> +    /// # Errors
> +    ///
> +    /// The length of `entries` should exactly be the available size of [`SgTable<N>`],
> +    /// or else an error is returned.
> +    ///
> +    /// If the table is `chainable`, the available size is `N - 1`, because one entry
> +    /// should be reserved for chaining.
> +    pub fn new(
> +        entries: &'a [Pin<&mut [u8]>],
> +        chainable: bool,
> +    ) -> impl PinInit<SgTable<'a, N>, Error> {
> +        build_assert!(N > 0);
> +        // SAFETY: `slot` is valid while the closure is called, the `entries` are
> +        // pinned and valid.
> +        unsafe {
> +            init::pin_init_from_closure(move |slot: *mut Self| {
> +                let mut nr_entry = N;
> +                if chainable {
> +                    nr_entry -= 1;
> +                }
> +                if nr_entry == 0 || entries.len() != nr_entry {
> +                    return Err(EINVAL);
> +                }
> +
> +                for i in 0..nr_entry {
> +                    // `slot` contains uninit memory, avoid creating a reference.
> +                    let opaque = addr_of!((*slot).entries[i].opaque);
> +                    let sgl = Opaque::raw_get(opaque);
> +
> +                    bindings::sg_set_buf(sgl, entries[i].as_ptr() as _, entries[i].len() as _);
> +                    if i + 1 == nr_entry {
> +                        (*sgl).page_link |= bindings::SG_END as u64;
> +                        (*sgl).page_link &= !(bindings::SG_CHAIN as u64);
> +                    }
> +                }
> +                Ok(())
> +            })
> +        }
> +    }
> +
> +    /// Chain two [`SgTable`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to the
> +    /// first entry of `sgt` SgTable.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    pub fn chain_sgt<const M: usize>(&mut self, sgt: Pin<&mut SgTable<'_, M>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +
> +        // SAFETY: `sgt.entries` are initialized by the `new` constructor,
> +        // so it's valid.
> +        let next = unsafe { ScatterList::as_mut(sgt.entries[0].opaque.get()).unwrap() };
> +        self.entries[N - 1].chain_sgl(next);
> +        Ok(())
> +    }
> +
> +    /// Chain [`SgTable`] and [`ScatterList`] together.
> +    ///
> +    /// Transfer the last entry of this table as a chainable pointer to `sgl`
> +    /// scatterlist.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has been chained.
> +    pub fn chain_sgl(&mut self, sgl: Pin<&mut ScatterList<'_>>) -> Result {
> +        if self.count() != N - 1 {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].unmark_end();
> +        self.entries[N - 1].chain_sgl(sgl);
> +        Ok(())
> +    }
> +
> +    /// Split the first table from chained scatterlist.
> +    ///
> +    /// # Errors
> +    ///
> +    /// Return an error if this table is not chainable or has not been chained.
> +    pub fn split(&mut self) -> Result {
> +        if !self.entries[N - 1].is_chain() {
> +            return Err(EINVAL);
> +        }
> +        self.entries[N - 2].mark_end();
> +        Ok(())
> +    }
> +
> [...]
> +}

I'd suggest being more explicit in the `# Errors` section by
mentioning that `EINVAL` is returned, but do as you prefer.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Greg KH]

On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
> Hi All!
> 
> This is a version of scatterlist abstractions for Rust drivers.
> 
> Scatterlist is used for efficient management of memory buffers, which is
> essential for many kernel-level operations such as Direct Memory Access
> (DMA) transfers and crypto APIs.
> 
> This patch should be a good start to introduce the crypto APIs for Rust
> drivers and to develop cipher algorithms in Rust later.

I thought we were getting rid of the scatter list api for the crypto
drivers, so this shouldn't be needed going forward, right?  Why not just
use the direct apis instead?

And what crypto protocols are needed to be done in rust that we are
currently missing in the C versions?

thanks,

greg k-h

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Greg KH]

On Sat, Jun 10, 2023 at 05:33:47PM +0200, Greg KH wrote:
> On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
> > Hi All!
> > 
> > This is a version of scatterlist abstractions for Rust drivers.
> > 
> > Scatterlist is used for efficient management of memory buffers, which is
> > essential for many kernel-level operations such as Direct Memory Access
> > (DMA) transfers and crypto APIs.
> > 
> > This patch should be a good start to introduce the crypto APIs for Rust
> > drivers and to develop cipher algorithms in Rust later.
> 
> I thought we were getting rid of the scatter list api for the crypto
> drivers, so this shouldn't be needed going forward, right?  Why not just
> use the direct apis instead?

See https://lore.kernel.org/r/ZH2hgrV6po9dkxi+@gondor.apana.org.au for
the details of that (sorry I forgot the link first time...)

greg k-h

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Qingsong Chen]

On 6/10/23 11:35 PM, Greg KH wrote:
> On Sat, Jun 10, 2023 at 05:33:47PM +0200, Greg KH wrote:
>> On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
>>> Hi All!
>>>
>>> This is a version of scatterlist abstractions for Rust drivers.
>>>
>>> Scatterlist is used for efficient management of memory buffers, which is
>>> essential for many kernel-level operations such as Direct Memory Access
>>> (DMA) transfers and crypto APIs.
>>>
>>> This patch should be a good start to introduce the crypto APIs for Rust
>>> drivers and to develop cipher algorithms in Rust later.
>>
>> I thought we were getting rid of the scatter list api for the crypto
>> drivers, so this shouldn't be needed going forward, right?  Why not just
>> use the direct apis instead?
> 
> See https://lore.kernel.org/r/ZH2hgrV6po9dkxi+@gondor.apana.org.au for
> the details of that (sorry I forgot the link first time...)

Thanks for the information. I agree that turning simple buffers into
sg-bufs is not a good idea. If I were implementing a new cipher
algorithm, I would definitely follow the `struct cipher_alg`, which
takes simple `u8 *` pointer as parameter. However, if we'd like to
utilize some existing ciphers, such as aead, we still need scatterlists
to construct an `aead_request` for further operations, util changes are
made to the underlying interface.

Qingsong Chen

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Greg KH]

On Mon, Jun 12, 2023 at 11:49:51AM +0800, Qingsong Chen wrote:
> On 6/10/23 11:35 PM, Greg KH wrote:
> > On Sat, Jun 10, 2023 at 05:33:47PM +0200, Greg KH wrote:
> > > On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
> > > > Hi All!
> > > > 
> > > > This is a version of scatterlist abstractions for Rust drivers.
> > > > 
> > > > Scatterlist is used for efficient management of memory buffers, which is
> > > > essential for many kernel-level operations such as Direct Memory Access
> > > > (DMA) transfers and crypto APIs.
> > > > 
> > > > This patch should be a good start to introduce the crypto APIs for Rust
> > > > drivers and to develop cipher algorithms in Rust later.
> > > 
> > > I thought we were getting rid of the scatter list api for the crypto
> > > drivers, so this shouldn't be needed going forward, right?  Why not just
> > > use the direct apis instead?
> > 
> > See https://lore.kernel.org/r/ZH2hgrV6po9dkxi+@gondor.apana.org.au for
> > the details of that (sorry I forgot the link first time...)
> 
> Thanks for the information. I agree that turning simple buffers into
> sg-bufs is not a good idea. If I were implementing a new cipher
> algorithm, I would definitely follow the `struct cipher_alg`, which
> takes simple `u8 *` pointer as parameter. However, if we'd like to
> utilize some existing ciphers, such as aead, we still need scatterlists
> to construct an `aead_request` for further operations, util changes are
> made to the underlying interface.

Why not make the changes to the C code first, and get those changes
merged, making this patch series not needed at all?

thanks,

greg k-h

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Qingsong Chen]

On 6/12/23 1:38 PM, Greg KH wrote:
> On Mon, Jun 12, 2023 at 11:49:51AM +0800, Qingsong Chen wrote:
>> On 6/10/23 11:35 PM, Greg KH wrote:
>>> On Sat, Jun 10, 2023 at 05:33:47PM +0200, Greg KH wrote:
>>>> On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
>>>>> Hi All!
>>>>>
>>>>> This is a version of scatterlist abstractions for Rust drivers.
>>>>>
>>>>> Scatterlist is used for efficient management of memory buffers, which is
>>>>> essential for many kernel-level operations such as Direct Memory Access
>>>>> (DMA) transfers and crypto APIs.
>>>>>
>>>>> This patch should be a good start to introduce the crypto APIs for Rust
>>>>> drivers and to develop cipher algorithms in Rust later.
>>>>
>>>> I thought we were getting rid of the scatter list api for the crypto
>>>> drivers, so this shouldn't be needed going forward, right?  Why not just
>>>> use the direct apis instead?
>>>
>>> See https://lore.kernel.org/r/ZH2hgrV6po9dkxi+@gondor.apana.org.au for
>>> the details of that (sorry I forgot the link first time...)
>>
>> Thanks for the information. I agree that turning simple buffers into
>> sg-bufs is not a good idea. If I were implementing a new cipher
>> algorithm, I would definitely follow the `struct cipher_alg`, which
>> takes simple `u8 *` pointer as parameter. However, if we'd like to
>> utilize some existing ciphers, such as aead, we still need scatterlists
>> to construct an `aead_request` for further operations, util changes are
>> made to the underlying interface.
> 
> Why not make the changes to the C code first, and get those changes
> merged, making this patch series not needed at all?
> 

Yes, you're right. I believe Herbert would work on that. And I would
stop this patch series. Thanks.

Qingsong Chen

Re: [PATCH v3 0/3] Rust scatterlist abstractions

[Greg KH]

On Mon, Jun 12, 2023 at 03:03:14PM +0800, Qingsong Chen wrote:
> On 6/12/23 1:38 PM, Greg KH wrote:
> > On Mon, Jun 12, 2023 at 11:49:51AM +0800, Qingsong Chen wrote:
> > > On 6/10/23 11:35 PM, Greg KH wrote:
> > > > On Sat, Jun 10, 2023 at 05:33:47PM +0200, Greg KH wrote:
> > > > > On Sat, Jun 10, 2023 at 06:49:06PM +0800, Qingsong Chen wrote:
> > > > > > Hi All!
> > > > > > 
> > > > > > This is a version of scatterlist abstractions for Rust drivers.
> > > > > > 
> > > > > > Scatterlist is used for efficient management of memory buffers, which is
> > > > > > essential for many kernel-level operations such as Direct Memory Access
> > > > > > (DMA) transfers and crypto APIs.
> > > > > > 
> > > > > > This patch should be a good start to introduce the crypto APIs for Rust
> > > > > > drivers and to develop cipher algorithms in Rust later.
> > > > > 
> > > > > I thought we were getting rid of the scatter list api for the crypto
> > > > > drivers, so this shouldn't be needed going forward, right?  Why not just
> > > > > use the direct apis instead?
> > > > 
> > > > See https://lore.kernel.org/r/ZH2hgrV6po9dkxi+@gondor.apana.org.au for
> > > > the details of that (sorry I forgot the link first time...)
> > > 
> > > Thanks for the information. I agree that turning simple buffers into
> > > sg-bufs is not a good idea. If I were implementing a new cipher
> > > algorithm, I would definitely follow the `struct cipher_alg`, which
> > > takes simple `u8 *` pointer as parameter. However, if we'd like to
> > > utilize some existing ciphers, such as aead, we still need scatterlists
> > > to construct an `aead_request` for further operations, util changes are
> > > made to the underlying interface.
> > 
> > Why not make the changes to the C code first, and get those changes
> > merged, making this patch series not needed at all?
> > 
> 
> Yes, you're right. I believe Herbert would work on that. And I would
> stop this patch series. Thanks.

No, I mean you work on that now.  There's no reason you should wait for
Herbert, or any one else, submit the patch series this week to that
subsystem that does the conversion and see how it goes!

thanks,

greg k-h