Add unsafe accessors for the region for reading or writing large
blocks of data.
Reviewed-by: Andreas Hindborg <a.hindborg@kernel.org>
Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.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.
+ ///
+ /// # 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 {
+ self.validate_range(offset, src.len())?;
+ // 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 + 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.
+ unsafe {
+ core::ptr::copy_nonoverlapping(src.as_ptr(), self.cpu_addr.add(offset), src.len())
+ };
+ Ok(())
+ }
+
/// Returns a pointer to an element from the region with bounds checking. `offset` is in
/// units of `T`, not the number of bytes.
///
--
2.43.0On 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@kernel.org>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
A couple remaining nits/questions below, but FWIW:
Reviewed-by: Alexandre Courbot <acourbot@nvidia.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?)
On 02/06/2025 16:05, Alexandre Courbot wrote:
> 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@kernel.org>
>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>
> A couple remaining nits/questions below, but FWIW:
>
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>> +
>> + /// 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?
Thanks! I don't have any strong opinion about this, I think it's enough
that the subsequent paragraph makes it clear that the unit is in bytes
unit? In any case, this could this be updated later after the merge?
>> + /// ```
>> + pub unsafe fn write(&self, src: &[T], offset: usize) -> Result {
>
> Can this function be written by leveraging `as_slice_mut` and
> `clone_from_slice`?
using `slice::clone_from_slice` would enforce the length of the coherent
allocation to be always the same as src data. Not sure if that is what
we want. Also, instead of just a straight memcpy, this would go through
a 2-step layer (a call to `slice::from_raw_parts_mut` and then the
`slice::clone_from_slice` itself)?.
Regards,
Abdiel
On Fri Jun 13, 2025 at 6:45 PM JST, Abdiel Janulgue wrote:
> On 02/06/2025 16:05, Alexandre Courbot wrote:
>> 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@kernel.org>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
>>
>> A couple remaining nits/questions below, but FWIW:
>>
>> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>
>>> +
>>> + /// 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?
>
> Thanks! I don't have any strong opinion about this, I think it's enough
> that the subsequent paragraph makes it clear that the unit is in bytes
> unit? In any case, this could this be updated later after the merge?
I agree this can be its own follow-up change, especially since `offset`
is already used elsewhere in the code and this patch is consistent with
the existing nomenclature - let's fix them all together as a separate
patch.
>
>>> + /// ```
>>> + pub unsafe fn write(&self, src: &[T], offset: usize) -> Result {
>>
>> Can this function be written by leveraging `as_slice_mut` and
>> `clone_from_slice`?
>
> using `slice::clone_from_slice` would enforce the length of the coherent
> allocation to be always the same as src data. Not sure if that is what
> we want. Also, instead of just a straight memcpy, this would go through
> a 2-step layer (a call to `slice::from_raw_parts_mut` and then the
> `slice::clone_from_slice` itself)?.
Ack, thanks for the explanation!
© 2016 - 2025 Red Hat, Inc.