Re: [PATCH v3 2/2] samples: rust: add sample code for scatterlist abstraction

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Abdiel, Alex,

> On 18 Jul 2025, at 07:33, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Add simple excercises to test the scatterlist abstraction.
> 
> 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>
> ---
> samples/rust/rust_dma.rs | 49 +++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/samples/rust/rust_dma.rs b/samples/rust/rust_dma.rs
> index 9e05d5c0cdae..1fa278e8e29a 100644
> --- a/samples/rust/rust_dma.rs
> +++ b/samples/rust/rust_dma.rs
> @@ -4,11 +4,33 @@
> //!
> //! To make this driver probe, QEMU must be run with `-device pci-testdev`.
> 
> -use kernel::{bindings, device::Core, dma::CoherentAllocation, pci, prelude::*, types::ARef};
> +use kernel::{
> +    bindings, device::Core, dma::CoherentAllocation, page::*, pci, prelude::*, scatterlist::*,
> +    sync::Arc, types::ARef,
> +};
> 
> struct DmaSampleDriver {
>     pdev: ARef<pci::Device>,
>     ca: CoherentAllocation<MyStruct>,
> +    _sgt: SGTable<OwnedSgt<PagesArray>, ManagedMapping>,
> +}
> +
> +struct PagesArray(KVec<Page>);
> +impl SGTablePages for PagesArray {
> +    fn iter<'a>(&'a self) -> impl Iterator<Item = (&'a Page, usize, usize)> {
> +        self.0.iter().map(|page| (page, kernel::page::PAGE_SIZE, 0))

The order seems to also be inverted here (see comment on the previous patch)

> +    }
> +
> +    fn entries(&self) -> usize {
> +        self.0.len()
> +    }
> +}
> +
> +struct WrappedArc(Arc<kernel::bindings::sg_table>);
> +impl core::borrow::Borrow<kernel::bindings::sg_table> for WrappedArc {
> +    fn borrow(&self) -> &kernel::bindings::sg_table {
> +        &self.0
> +    }
> }

I assume there is no way to get around this without compromising somewhere
else, right?

> 
> const TEST_VALUES: [(u32, u32); 5] = [
> @@ -58,10 +80,35 @@ fn probe(pdev: &pci::Device<Core>, _info: &Self::IdInfo) -> Result<Pin<KBox<Self
>             kernel::dma_write!(ca[i] = MyStruct::new(value.0, value.1))?;
>         }
> 
> +        let mut pages = KVec::new();
> +        for _ in TEST_VALUES.into_iter() {
> +            let _ = pages.push(Page::alloc_page(GFP_KERNEL)?, GFP_KERNEL);
> +        }
> +
> +        // Let's pretend this is valid...

I’d reword this.

> +        // SAFETY: `sg_table` is not a reference.
> +        let sg_table: bindings::sg_table = unsafe { core::mem::zeroed() };
> +
> +        // `borrowed_sgt` cannot outlive `sg_table`.
> +        // SAFETY: From above, we assume that `sg_table` is initialized and valid.
> +        let _borrowed_sgt = unsafe { SGTable::new_unmapped(&sg_table) };

Wait, zero-initialization is considered “initialized and valid” here? i.e.:

struct sg_table {
	struct scatterlist *sgl;	/* the list */
	unsigned int nents;		/* number of mapped entries */
	unsigned int orig_nents;	/* original size of list */
};

> +
> +        let sg_table = WrappedArc(Arc::new(sg_table, GFP_KERNEL)?);
> +        // `refcounted_sgt` keeps a refcounted reference to the `sg_table` and is thus not
> +        // tied by a compile-time lifetime.
> +        // SAFETY: From above, we assume that `sg_table` is initialized and valid.
> +        let _refcounted_sgt = unsafe { SGTable::new_unmapped(sg_table) };

Ah, this is cool, though the Borrow implementation is a bit of a downside :/

> +
> +        // `owned_sgt` carries and owns the data it represents.
> +        let owned_sgt = SGTable::new_owned(PagesArray(pages), GFP_KERNEL)?;
> +        let sgt = owned_sgt.dma_map(pdev.as_ref(), kernel::dma::DmaDataDirection::DmaToDevice)?;
> +
>         let drvdata = KBox::new(
>             Self {
>                 pdev: pdev.into(),
>                 ca,
> +                // excercise the destructor
> +                _sgt: sgt,
>             },
>             GFP_KERNEL,
>         )?;
> -- 
> 2.43.0
> 

— Daniel