Message-Id: <DAC2ILD2DSIZ.3TF8W39X5DDH8@nvidia.com>
Date: Mon, 02 Jun 2025 22:05:53 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Abdiel Janulgue" <abdiel.janulgue@...il.com>, <dakr@...nel.org>
Cc: "Miguel Ojeda" <ojeda@...nel.org>, "Alex Gaynor"
 <alex.gaynor@...il.com>, "Boqun Feng" <boqun.feng@...il.com>, "Gary Guo"
 <gary@...yguo.net>, Björn Roy Baron
 <bjorn3_gh@...tonmail.com>, "Benno Lossin" <benno.lossin@...ton.me>,
 "Andreas Hindborg" <a.hindborg@...nel.org>, "Alice Ryhl"
 <aliceryhl@...gle.com>, "Trevor Gross" <tmgross@...ch.edu>, "Valentin Obst"
 <kernel@...entinobst.de>, "open list" <linux-kernel@...r.kernel.org>,
 "Marek Szyprowski" <m.szyprowski@...sung.com>, "Robin Murphy"
 <robin.murphy@....com>, <airlied@...hat.com>,
 <rust-for-linux@...r.kernel.org>, "open list:DMA MAPPING HELPERS"
 <iommu@...ts.linux.dev>, "Petr Tesarik" <petr@...arici.cz>, "Andrew Morton"
 <akpm@...ux-foundation.org>, "Herbert Xu" <herbert@...dor.apana.org.au>,
 "Sui Jingfeng" <sui.jingfeng@...ux.dev>, "Randy Dunlap"
 <rdunlap@...radead.org>, "Michael Kelley" <mhklinux@...look.com>
Subject: Re: [PATCH v4 3/3] rust: dma: add as_slice/write functions for
 CoherentAllocation

On Mon Jun 2, 2025 at 5:53 PM JST, Abdiel Janulgue wrote:
> Add unsafe accessors for the region for reading or writing large
> blocks of data.
>
> Reviewed-by: Andreas Hindborg <a.hindborg@...nel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>

A couple remaining nits/questions below, but FWIW:

Reviewed-by: Alexandre Courbot <acourbot@...dia.com>

> ---
>  rust/kernel/dma.rs | 86 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 86 insertions(+)
>
> diff --git a/rust/kernel/dma.rs b/rust/kernel/dma.rs
> index 5a690e5f1e66..b486f63c1d3a 100644
> --- a/rust/kernel/dma.rs
> +++ b/rust/kernel/dma.rs
> @@ -218,6 +218,92 @@ pub fn dma_handle(&self) -> bindings::dma_addr_t {
>          self.dma_handle
>      }
>  
> +    /// Common helper to validate a range applied from the allocated region in the CPU's virtual
> +    /// address space.
> +    fn validate_range(&self, offset: usize, count: usize) -> Result
> +    {
> +        if offset.checked_add(count).ok_or(EOVERFLOW)? > self.count {
> +            return Err(EINVAL);
> +        }
> +        Ok(())
> +    }
> +
> +    /// Returns the data from the region starting from `offset` as a slice.
> +    /// `offset` and `count` are in units of `T`, not the number of bytes.
> +    ///
> +    /// For ringbuffer type of r/w access or use-cases where the pointer to the live data is needed,
> +    /// [`CoherentAllocation::start_ptr`] or [`CoherentAllocation::start_ptr_mut`] could be used instead.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
> +    ///   slice is live.
> +    /// * Callers must ensure that this call does not race with a write to the same region while
> +    ///   the returned slice is live.
> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
> +        self.validate_range(offset, count)?;
> +        // SAFETY:
> +        // - The pointer is valid due to type invariant on `CoherentAllocation`,
> +        //   we've just checked that the range and index is within bounds. The immutability of the
> +        //   data is also guaranteed by the safety requirements of the function.
> +        // - `offset + count` can't overflow since it is smaller than `self.count` and we've checked
> +        //   that `self.count` won't overflow early in the constructor.
> +        Ok(unsafe { core::slice::from_raw_parts(self.cpu_addr.add(offset), count) })
> +    }
> +
> +    /// Performs the same functionality as [`CoherentAllocation::as_slice`], except that a mutable
> +    /// slice is returned.
> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
> +    ///   slice is live.
> +    /// * Callers must ensure that this call does not race with a read or write to the same region
> +    ///   while the returned slice is live.
> +    pub unsafe fn as_slice_mut(&self, offset: usize, count: usize) -> Result<&mut [T]> {
> +        self.validate_range(offset, count)?;
> +        // SAFETY:
> +        // - The pointer is valid due to type invariant on `CoherentAllocation`,
> +        //   we've just checked that the range and index is within bounds. The immutability of the
> +        //   data is also guaranteed by the safety requirements of the function.
> +        // - `offset + count` can't overflow since it is smaller than `self.count` and we've checked
> +        //   that `self.count` won't overflow early in the constructor.
> +        Ok(unsafe { core::slice::from_raw_parts_mut(self.cpu_addr.add(offset), count) })
> +    }
> +
> +    /// Writes data to the region starting from `offset`. `offset` is in units of `T`, not the
> +    /// number of bytes.

Reading this sentence it occured to me that `offset` may be ambiguous
here, as in my mind it rings as being in bytes unit. How about using
`index` throughout the file?

> +    ///
> +    /// # Safety
> +    ///
> +    /// * Callers must ensure that the device does not read/write to/from memory while the returned
> +    ///   slice is live.
> +    /// * Callers must ensure that this call does not race with a read or write to the same region
> +    ///   that overlaps with this write.
> +    ///
> +    /// # Examples
> +    ///
> +    /// ```
> +    /// # fn test(alloc: &mut kernel::dma::CoherentAllocation<u8>) -> Result {
> +    /// let somedata: [u8; 4] = [0xf; 4];
> +    /// let buf: &[u8] = &somedata;
> +    /// // SAFETY: No hw operation on the device and no other r/w access to the region at this point.
> +    /// unsafe { alloc.write(buf, 0)?; }
> +    /// # Ok::<(), Error>(()) }
> +    /// ```
> +    pub unsafe fn write(&self, src: &[T], offset: usize) -> Result {

Can this function be written by leveraging `as_slice_mut` and
`clone_from_slice`? But doing so might require `T` to implement Clone,
so maybe not a good idea (OTOH, aren't types implementing `AsBytes`
implicitly Cloneable?)

Powered by blists - more mailing lists