Re: [PATCH v12 2/3] rust: add dma coherent allocator abstraction.

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

Hi,

On 24/02/2025 22:07, Benno Lossin wrote:
>> +    ///
>> +    /// # 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?
> 
>> +    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: &mutkernel::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`?

Thanks for the feedback! I noticed an additional safety requirement in 
slice::from_raw_parts_mut:

"The memory referenced by the returned slice must not be accessed 
through any other pointer (not derived from the return value) for the 
duration of lifetime 'a. Both read and write accesses are forbidden."

I can see now though why both as_slice and as_slice_mut docs needs more 
clarity. i.e., they could be worded similarly and add the additional 
safety requirement of slice::from_raw_parts_mut of having no other r/w 
access while the slice is live?

Regards,
Abdiel