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

Alice Ryhl
2025-01-27T13:25:16Z

On Mon, Jan 27, 2025 at 1:14 PM Danilo Krummrich <dakr@kernel.org> wrote:
>
> On Mon, Jan 27, 2025 at 11:43:39AM +0100, Alice Ryhl wrote:
> > On Mon, Jan 27, 2025 at 11:37 AM Danilo Krummrich <dakr@kernel.org> wrote:
> > >
> > > On Fri, Jan 24, 2025 at 08:27:36AM +0100, Alice Ryhl wrote:
> > > > On Thu, Jan 23, 2025 at 11:43 AM Abdiel Janulgue
> > > > <abdiel.janulgue@gmail.com> wrote:
> > > > > + /// Reads data from the region starting from `offset` as a slice.
> > > > > + /// `offset` and `count` are in units of `T`, not the number of bytes.
> > > > > + ///
> > > > > + /// Due to the safety requirements of slice, the data returned should be regarded by the
> > > > > + /// caller as a snapshot of the region when this function is called, as the region could
> > > > > + /// be modified by the device at anytime. For ringbuffer type of 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]> {
> > > >
> > > > You were asked to rename this function because it returns a slice, but
> > > > I wonder if it's better to take an `&mut [T]` argument and to have
> > > > this function copy data into that argument. That way, we could make
> > > > the function itself safe. Perhaps the actual copy needs to be
> > > > volatile?
> > >
> > > Why do we consider the existing one unsafe?
> > >
> > > Surely, it's not desirable that the contents of the buffer are modified by the
> > > HW unexpectedly, but is this a concern in terms of Rust safety requirements?
> > >
> > > And if so, how does this go away with the proposed approach?
> >
> > In Rust, it is undefined behavior if the value behind an immutable
> > reference changes (unless the type uses UnsafeCell / Opaque or
> > similar). That is, any two consecutive reads of the same immutable
> > reference must return the same byte value no matter what happened in
> > between those reads.
>
> Undefined as in the sense of anything is allowed to happen?

Yes.

> I thought undefined
> as in you might still see the old value on two consecutive reads.

That is the optimization that motivates this being UB, but it's
defined as full UB.

> Do you have a pointer to the corresponding docs?

Sure, it's on the "behavior considered undefined" page:
Moreover, the bytes pointed to by a shared reference, including
transitively through other references (both shared and mutable) and
Boxes, are immutable; transitivity includes those references stored in
fields of compound types.

https://doc.rust-lang.org/reference/behavior-considered-undefined.html#r-undefined.immutable

> > If we manually perform the read as a volatile read, then it is
> > arguably allowed for the value to be modified by the hardware while we
> > read the value.
>
> From read_volatile() [1]: "In particular, a race between a read_volatile and any
> write operation to the same location is undefined behavior."

I mean, ultimately we are a bit on our own here. In C code you just
use an ordinary read / write, so we could use the ordinary
core::ptr::copy_nonoverlapping method to mirror that. We've been told
from the Rust project that we should just do these kinds of things
like we do in C - technically these things aren't okay in C either,
but because LLVM will try to avoid breaking patterns used in the
kernel, they shouldn't break in Rust either.

But using an immutable reference should be avoided because that gives
LLVM optimization hints that we are not giving to LLVM in C code.

> Also, what if the hardware put a value that is invalid for the type?

The trait bounds require that T is FromBytes, which has a safety
requirement that all bit-patterns must be valid for the type.

Alice