dma::Device trait and DMA mask

Implement dma_set_mask(), dma_set_coherent_mask() and dma_set_mask_and_coherent() in the dma::Device trait.

Those methods are used to set up the device's DMA addressing capabilities.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/helpers/dma.c |  5 +++
 rust/kernel/dma.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 3 deletions(-)

rust/helpers/dma.c +5 -0

index df8b8a77355a..6e741c197242 100644
@@ -14,3 +14,8 @@ void rust_helper_dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 {
 	dma_free_attrs(dev, size, cpu_addr, dma_handle, attrs);
 }
+
+int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
+{
+	return dma_set_mask_and_coherent(dev, mask);
+}

rust/kernel/dma.rs +79 -3

index f0af23d08e8d..4b27b8279941 100644
@@ -6,9 +6,9 @@
 
 use crate::{
     bindings, build_assert, device,
-    device::Bound,
+    device::{Bound, Core},
     error::code::*,
-    error::Result,
+    error::{to_result, Result},
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };

@@ -18,7 +18,83 @@
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
 /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
 /// [`platform::Device`](::kernel::platform::Device).
-pub trait Device: AsRef<device::Device<Core>> {}
+pub trait Device: AsRef<device::Device<Core>> {
+    /// Set up the device's DMA streaming addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_mask(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_mask(self.as_ref().as_raw(), mask) })
+    }
+
+    /// Set up the device's DMA coherent addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_coherent_mask(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_coherent_mask(self.as_ref().as_raw(), mask) })
+    }
+
+    /// Set up the device's DMA addressing capabilities.
+    ///
+    /// This is a combination of [`Device::dma_set_mask`] and [`Device::dma_set_coherent_mask`].
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety
+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].
+    unsafe fn dma_set_mask_and_coherent(&self, mask: u64) -> Result {
+        // SAFETY:
+        // - By the type invariant of `device::Device`, `self.as_ref().as_raw()` is valid.
+        // - The safety requirement of this function guarantees that there are no concurrent calls
+        //   to DMA allocation and mapping primitives using this mask.
+        to_result(unsafe { bindings::dma_set_mask_and_coherent(self.as_ref().as_raw(), mask) })
+    }
+}
+
+/// Returns a bitmask with the lowest `n` bits set to `1`.
+///
+/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
+/// For `n > 64`, returns `u64::MAX` (all bits set).
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::dma_bit_mask;
+///
+/// assert_eq!(dma_bit_mask(0), 0);
+/// assert_eq!(dma_bit_mask(1), 0b1);
+/// assert_eq!(dma_bit_mask(64), u64::MAX);
+/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
+/// ```
+pub const fn dma_bit_mask(n: usize) -> u64 {
+    match n {
+        0 => 0,
+        1..=64 => u64::MAX >> (64 - n),
+        _ => u64::MAX,
+    }
+}
 
 /// Possible attributes associated with a DMA mapping.
 ///

Signature · 1 line

2.50.0

It looks good to me. A few high-level comments:

1. If we don't expect the concurrency issue for this in C code, why do we expect it to happen in rust?

2. Since the Rust code is wrapping around the C code, the data race is happening entirely on the C side right? So can we just rely on KCSAN to catch concurrency issues instead of marking the callers as unsafe? I feel the unsafe { } really might make the driver code ugly.

3. Maybe we could document this issue than enforce it via unsafe? My concern is wrapping unsafe { } makes the calling code ugly.

4. Are there other kernel APIs where Rust wraps potentially racy C code as unsafe?

5. In theory, all rust bindings wrappers are unsafe and we do mark it around the bindings call, right? But in this case, we're also making the calling code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust PoV but I am not sure we worry about the internal implementation-unsafety of the C code because then maybe most bindings wrappers would need to be unsafe, not only these DMA ones.

Quoted 4 lines · Joel Fernandes

It looks good to me. A few high-level comments:

1. If we don't expect the concurrency issue for this in C code, why do we expect it to happen in rust?

The race can happen in C as well, but people would probably argue that no one ever calls the mask setter function concurrently to DMA allocation and mapping primitives.

2. Since the Rust code is wrapping around the C code, the data race is happening entirely on the C side right? So can we just rely on KCSAN to catch concurrency issues instead of marking the callers as unsafe? I feel the unsafe { } really might make the driver code ugly.

3. Maybe we could document this issue than enforce it via unsafe? My concern is wrapping unsafe { } makes the calling code ugly.

5. In theory, all rust bindings wrappers are unsafe and we do mark it around the bindings call, right? But in this case, we're also making the calling code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust PoV but I am not sure we worry about the internal implementation-unsafety of the C code because then maybe most bindings wrappers would need to be unsafe, not only these DMA ones.

Quoted 8 lines · Joel Fernandes

2. Since the Rust code is wrapping around the C code, the data race is happening entirely on the C side right? So can we just rely on KCSAN to catch concurrency issues instead of marking the callers as unsafe? I feel the unsafe { } really might make the driver code ugly.

3. Maybe we could document this issue than enforce it via unsafe? My concern is wrapping unsafe { } makes the calling code ugly.

Yeah, this sort of dilemma comes up from time to time, yeah, e.g. see:

https://lore.kernel.org/rust-for-linux/CANiq72k_NNFsQ=GGCsur34CTYhSFC0m=mHS83mTB8HQCDBcW=w@mail.gmail.com/
https://lore.kernel.org/rust-for-linux/CANiq72m3WFj9Eb2iRUM3mLFibWW+cupAoNQt+cqtNa4O9=jq7Q@mail.gmail.com/

In short: that is the job of unsafe {} -- if we start to avoid it just to make code prettier, then it loses its power.

There are few alternatives/notes, though:

- If there is a way to somehow guarantee or check that something is
safe, perhaps with a tool like Klint, then that could allow us to avoid unsafe.

- If the blocks are very repetitive in a single user and don't add
any value, then one could consider having users write a single unsafe {} where they promise to uphold X instead of requiring it in further calls.

- Worst case, we can promote something to the potential ASH list
idea ("Acknowledged Soundness Holes"): a list where we document things that do not require unsafe {} that should require it but don't for strong practical reasons.

Quoted 6 lines

5. In theory, all rust bindings wrappers are unsafe and we do mark it around the bindings call, right? But in this case, we're also making the calling code of the unsafe caller as unsafe. C code is 'unsafe' obviously from Rust PoV but I am not sure we worry about the internal implementation-unsafety of the C code because then maybe most bindings wrappers would need to be unsafe, not only these DMA ones.

It is orthogonal -- you may have a safe function that uses unsafe {} inside (e.g. to call a C function), but also you will see unsafe functions with just safe code inside. And, of course, safe functions with only safe code inside and unsafe functions with unsafe {} blocks inside.

In other words, a safe function does not mean unsafe code is used or not inside. Similarly, an unsafe function does not mean unsafe code is used (or not) inside either.

This is the usual "two meaning of unsafe" -- that of e.g. functions (where it means there are safety preconditions for calling a function) and that of e.g. unsafe {} blocks (where it means the caller must play by the rules, e.g. satisfy the safety preconditions to call an unsafe function).

So a Rust function that calls C functions (and thus uses unsafe {}) may or may not need to be unsafe -- it all depends on the case. That is, it depends on whether callers can cause UB or not. And similarly, a Rust function that does not use unsafe (including not calling C functions) can still be very much unsafe, because other code may rely on that code for soundness (e.g. an unsafe method that assigns to an internal pointer which then other safe methods rely on).

Implement dma_set_mask(), dma_set_coherent_mask() and dma_set_mask_and_coherent() in the dma::Device trait.

Those methods are used to set up the device's DMA addressing capabilities.

Signed-off-by Danilo Krummrich <[email protected]>

---
 rust/helpers/dma.c |  5 +++
 rust/kernel/dma.rs | 82 ++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 84 insertions(+), 3 deletions(-)

rust/helpers/dma.c +5 -0

index df8b8a77355a..6e741c197242 100644
@@ -14,3 +14,8 @@ void rust_helper_dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
 {
 	dma_free_attrs(dev, size, cpu_addr, dma_handle, attrs);
 }
+
+int rust_helper_dma_set_mask_and_coherent(struct device *dev, u64 mask)
+{
+	return dma_set_mask_and_coherent(dev, mask);
+}

rust/kernel/dma.rs +2 -3

index f0af23d08e8d..4b27b8279941 100644
@@ -6,9 +6,9 @@
 
 use crate::{
     bindings, build_assert, device,
-    device::Bound,
+    device::{Bound, Core},
     error::code::*,
-    error::Result,
+    error::{to_result, Result},
     transmute::{AsBytes, FromBytes},
     types::ARef,
 };

@@ -18,7 +18,83 @@
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
 /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
 /// [`platform::Device`](::kernel::platform::Device).
-pub trait Device: AsRef<device::Device<Core>> {}

Quoted 12 lines · Danilo Krummrich

rust/kernel/dma.rs +7 -1

@@ -18,7 +18,83 @@
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
 /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
 /// [`platform::Device`](::kernel::platform::Device).
-pub trait Device: AsRef<device::Device<Core>> {}
+pub trait Device: AsRef<device::Device<Core>> {
+    /// Set up the device's DMA streaming addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety

rust/kernel/dma.rs line 30

Quoted 3 lines

rust/kernel/dma.rs +3 -0

+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].

I'm a bit confused by the use of "concurrently" in this sentence. Do you mean that it must be called *before* any DMA allocation of mapping primitives? In this case, wouldn't it be clearer to make the order explicit?

rust/kernel/dma.rs line 31

Quoted 1 line

rust/kernel/dma.rs +1 -0

+    unsafe fn dma_set_mask(&self, mask: u64) -> Result {

Do we want to allow any u64 as a valid mask? If not, shall we restrict the accepted values by taking either the parameter to give to dma_bit_mask, or a bit range (similarly to Daniel's bitmask series [1], which it might make sense to leverage)?

[1]
https://lore.kernel.org/rust-for-linux/[email protected]/

Quoted 13 lines · Alexandre Courbot

Quoted 12 lines · Danilo Krummrich

rust/kernel/dma.rs +7 -1

@@ -18,7 +18,83 @@
 /// The [`dma::Device`](Device) trait should be implemented by bus specific device representations,
 /// where the underlying bus is DMA capable, such as [`pci::Device`](::kernel::pci::Device) or
 /// [`platform::Device`](::kernel::platform::Device).
-pub trait Device: AsRef<device::Device<Core>> {}
+pub trait Device: AsRef<device::Device<Core>> {
+    /// Set up the device's DMA streaming addressing capabilities.
+    ///
+    /// This method is usually called once from `probe()` as soon as the device capabilities are
+    /// known.
+    ///
+    /// # Safety

Quoted 8 lines

rust/kernel/dma.rs line 30

Quoted 3 lines

rust/kernel/dma.rs +3 -0

+    ///
+    /// This method must not be called concurrently with any DMA allocation or mapping primitives,
+    /// such as [`CoherentAllocation::alloc_attrs`].

I'm a bit confused by the use of "concurrently" in this sentence. Do you mean that it must be called *before* any DMA allocation of mapping primitives? In this case, wouldn't it be clearer to make the order explicit?

Setting the mask before any DMA allocations might only be relevant from a semantical point of view, but not safety wise.

We need to prevent concurrent accesses to dev->dma_mask and dev->coherent_dma_mask.

Quoted 9 lines

rust/kernel/dma.rs line 31

Quoted 1 line

rust/kernel/dma.rs +1 -0

+    unsafe fn dma_set_mask(&self, mask: u64) -> Result {

Do we want to allow any u64 as a valid mask? If not, shall we restrict the accepted values by taking either the parameter to give to dma_bit_mask, or a bit range (similarly to Daniel's bitmask series [1], which it might make sense to leverage)?

[1]
https://lore.kernel.org/rust-for-linux/[email protected]/

I think it would make sense to make dma_bit_mask() return a new type, e.g. DmaMask and take this instead.

Taking the parameter dma_bit_mask() takes directly in dma_set_mask() etc. makes sense, but changes the semantics of the mask parameter *subtly* compared to the C versions, which I want to avoid.

Using the infrastructure in [1] doesn't seem to provide much value, since we don't want a range [M..N], but [0..N], so we should rather only ask for N.

rust/kernel/dma.rs +12 -0

+/// Returns a bitmask with the lowest `n` bits set to `1`.
+///
+/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
+/// For `n > 64`, returns `u64::MAX` (all bits set).
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::dma_bit_mask;
+///
+/// assert_eq!(dma_bit_mask(0), 0);
+/// assert_eq!(dma_bit_mask(1), 0b1);

Quoted 17 lines · Danilo Krummrich

rust/kernel/dma.rs +17 -0

+/// Returns a bitmask with the lowest `n` bits set to `1`.
+///
+/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
+/// For `n > 64`, returns `u64::MAX` (all bits set).
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::dma_bit_mask;
+///
+/// assert_eq!(dma_bit_mask(0), 0);
+/// assert_eq!(dma_bit_mask(1), 0b1);
+/// assert_eq!(dma_bit_mask(64), u64::MAX);
+/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
+/// ```
+pub const fn dma_bit_mask(n: usize) -> u64 {
+    match n {

rust/kernel/dma.rs line 97

Quoted 5 lines

rust/kernel/dma.rs +5 -0

+        0 => 0,
+        1..=64 => u64::MAX >> (64 - n),
+        _ => u64::MAX,
+    }
+}

This is just the C macro DMA_BIT_MASK(), right? If so, can that be said here somewhere?

Or, how about turning DMA_BIT_MASK() into an inline function which could then be just called by the rust code directly instead?

Quoted 19 lines · Danilo Krummrich

rust/kernel/dma.rs +19 -0

+/// Returns a bitmask with the lowest `n` bits set to `1`.
+///
+/// For `n` in `0..=64`, returns a mask with the lowest `n` bits set.
+/// For `n > 64`, returns `u64::MAX` (all bits set).
+///
+/// # Examples
+///
+/// ```
+/// use kernel::dma::dma_bit_mask;
+///
+/// assert_eq!(dma_bit_mask(0), 0);
+/// assert_eq!(dma_bit_mask(1), 0b1);
+/// assert_eq!(dma_bit_mask(64), u64::MAX);
+/// assert_eq!(dma_bit_mask(100), u64::MAX); // Saturates at all bits set.
+/// ```
+pub const fn dma_bit_mask(n: usize) -> u64 {
+    match n {
+        0 => 0,
+        1..=64 => u64::MAX >> (64 - n),

rust/kernel/dma.rs line 97

Quoted 3 lines

rust/kernel/dma.rs +3 -0

+        _ => u64::MAX,
+    }
+}

This is just the C macro DMA_BIT_MASK(), right? If so, can that be said here somewhere? Or, how about turning DMA_BIT_MASK() into an inline function which could then be just called by the rust code directly instead?

This patch series adds the dma::Device trait to be implemented by bus devices on DMA capable busses.

The dma::Device trait implements methods to set the DMA mask for for such devices.

The first two bus devices implementing the trait are PCI and platform.

Unfortunately, the DMA mask setters have to be unsafe for now, since, with reasonable effort, we can't prevent drivers from data races writing and reading the DMA mask fields concurrently (see also [1]).

Link https://lore.kernel.org/lkml/[email protected]/ [1] open here

Link https://git.kernel.org/pub/scm/linux/kernel/git/dakr/linux.git/log/?h=rust/dma-mask

Danilo Krummrich (5):
rust: dma: implement dma::Device trait
rust: dma: add DMA addressing capabilities
rust: pci: implement the dma::Device trait
rust: platform: implement the dma::Device trait
rust: samples: dma: set DMA mask

  rust/helpers/dma.c       |  5 +++
  rust/kernel/dma.rs       | 95 +++++++++++++++++++++++++++++++++++++---
  rust/kernel/pci.rs       |  2 +
  rust/kernel/platform.rs  |  2 +
  samples/rust/rust_dma.rs | 12 ++++-
  5 files changed, 109 insertions(+), 7 deletions(-)

base-commit: d49ac7744f578bcc8708a845cce24d3b91f86260