Message-Id: <3202F69F-397E-4BC4-8DD8-E2D4B0AB056F@collabora.com>
Date: Mon, 24 Feb 2025 20:12:05 -0300
From: Daniel Almeida <daniel.almeida@...labora.com>
To: Benno Lossin <benno.lossin@...ton.me>
Cc: Abdiel Janulgue <abdiel.janulgue@...il.com>,
 aliceryhl@...gle.com,
 dakr@...nel.org,
 robin.murphy@....com,
 rust-for-linux@...r.kernel.org,
 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>,
 Andreas Hindborg <a.hindborg@...nel.org>,
 Trevor Gross <tmgross@...ch.edu>,
 Valentin Obst <kernel@...entinobst.de>,
 linux-kernel@...r.kernel.org,
 Christoph Hellwig <hch@....de>,
 Marek Szyprowski <m.szyprowski@...sung.com>,
 airlied@...hat.com,
 iommu@...ts.linux.dev
Subject: Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

Hi Benno,

>> +pub struct CoherentAllocation<T: AsBytes + FromBytes> {
>> +    dev: ARef<Device>,
>> +    dma_handle: bindings::dma_addr_t,
>> +    count: usize,
>> +    cpu_addr: *mut T,
>> +    dma_attrs: Attrs,
>> +}
>> +
>> +impl<T: AsBytes + FromBytes> CoherentAllocation<T> {
>> +    /// Allocates a region of `size_of::<T> * count` of consistent memory.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// use kernel::device::Device;
>> +    /// use kernel::dma::{attrs::*, CoherentAllocation};
>> +    ///
>> +    /// # fn test(dev: &Device) -> Result {
>> +    /// let c: CoherentAllocation<u64> = CoherentAllocation::alloc_attrs(dev.into(), 4, GFP_KERNEL,
>> +    ///                                                                  DMA_ATTR_NO_WARN)?;
>> +    /// # Ok::<(), Error>(()) }
>> +    /// ```
>> +    pub fn alloc_attrs(
>> +        dev: ARef<Device>,
>> +        count: usize,
>> +        gfp_flags: kernel::alloc::Flags,
>> +        dma_attrs: Attrs,
>> +    ) -> Result<CoherentAllocation<T>> {
>> +        build_assert!(
>> +            core::mem::size_of::<T>() > 0,
>> +            "It doesn't make sense for the allocated type to be a ZST"
>> +        );
> 
> Is this a safety requirement? I.e. the `dma_alloc_attrs` function cannot
> handle a size of 0?

It doesn’t make any sense to have a ZST here. At the very minimum we want to be able to read and
write bytes using this code, or preferably some larger T if applicable. The region also has to be allocated and we
need a size for that too.

This was discussed in an early iteration of this patch. I think a build failure is warranted.

> 
>> +    /// r/w access or use-cases where the pointer to the live data is needed, `start_ptr()` or
>> +    /// `start_ptr_mut()` could be used instead.
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// Callers must ensure that no hardware operations that involve the buffer are currently
>> +    /// taking place while the returned slice is live.
>> +    pub unsafe fn as_slice(&self, offset: usize, count: usize) -> Result<&[T]> {
>> +        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>> +        if end >= self.count {
>> +            return Err(EINVAL);
>> +        }
>> +        // 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
>> +        // of data is also guaranteed by the safety requirements of the function.
>> +        // - `offset` 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 `as_slice`, except that a mutable slice is returned.
>> +    /// See that method for documentation and safety requirements.
> 
> I don't think this is good documentation style. I think copy-pasting the
> first line and second paragraph is better.
> 
>> +    ///
>> +    /// # Safety
>> +    ///
>> +    /// It is the callers responsibility to avoid separate read and write accesses to the region
>> +    /// while the returned slice is live.
> 
> This safety requirement is worded quite differently compared to the one
> on `as_slice`, why?

This was discussed in an earlier iteration of this patch too. If you call this function, you must make
sure that some hw doesn’t change the memory contents while the slice is alive.

This is device-specific. e.g.: I know that for video codecs this is possible. Therefore this API
can be used there.

On the other hand, some people may use the API to share ring buffers with the hw or even implement
some polling logic where the CPU is waiting for a given memory location to be written by the HW. 

If you’re trying to do this, you cannot use this API, that’s what the safety requirement is about.

Although I’d word this differently to be honest, i.e.:

 /// It is the callers responsibility to avoid concurrent access to the region by the CPU and any other device
 /// while the slice is alive.

This is also needs a bit of work, but at least it makes the point clearer.

> 
>> +    pub unsafe fn as_slice_mut(&self, offset: usize, count: usize) -> Result<&mut [T]> {
>> +        let end = offset.checked_add(count).ok_or(EOVERFLOW)?;
>> +        if end >= self.count {
>> +            return Err(EINVAL);
>> +        }
>> +        // 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
>> +        // of data is also guaranteed by the safety requirements of the function.
>> +        // - `offset` 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.
>> +    ///
>> +    /// # Examples
>> +    ///
>> +    /// ```
>> +    /// # fn test(alloc: &mut kernel::dma::CoherentAllocation<u8>) -> Result {
>> +    /// let somedata: [u8; 4] = [0xf; 4];
>> +    /// let buf: &[u8] = &somedata;
>> +    /// alloc.write(buf, 0)?;
>> +    /// # Ok::<(), Error>(()) }
>> +    /// ```
>> +    pub fn write(&self, src: &[T], offset: usize) -> Result {
>> +        let end = offset.checked_add(src.len()).ok_or(EOVERFLOW)?;
>> +        if end >= self.count {
>> +            return Err(EINVAL);
>> +        }
>> +        // SAFETY:
>> +        // - The pointer is valid due to type invariant on `CoherentAllocation`
>> +        // and we've just checked that the range and index is within bounds.
>> +        // - `offset` can't overflow since it is smaller than `self.count` and we've checked
>> +        // that `self.count` won't overflow early in the constructor.
>> +        unsafe {
>> +            core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
> 
> Why are there no concurrent write or read operations on `cpu_addr`?

Sorry, can you rephrase this question?

— Daniel

Powered by blists - more mailing lists