Message-ID: <Z5fYAGxXymV1xTFL@tardis.local>
Date: Mon, 27 Jan 2025 11:01:20 -0800
From: Boqun Feng <boqun.feng@...il.com>
To: Alice Ryhl <aliceryhl@...gle.com>
Cc: Danilo Krummrich <dakr@...nel.org>,
	Abdiel Janulgue <abdiel.janulgue@...il.com>,
	rust-for-linux@...r.kernel.org, daniel.almeida@...labora.com,
	robin.murphy@....com, Miguel Ojeda <ojeda@...nel.org>,
	Alex Gaynor <alex.gaynor@...il.com>, Gary Guo <gary@...yguo.net>,
	Björn Roy Baron <bjorn3_gh@...tonmail.com>,
	Benno Lossin <benno.lossin@...ton.me>,
	Andreas Hindborg <a.hindborg@...nel.org>,
	Trevor Gross <tmgross@...ch.edu>,
	Valentin Obst <kernel@...entinobst.de>,
	open list <linux-kernel@...r.kernel.org>,
	Christoph Hellwig <hch@....de>,
	Marek Szyprowski <m.szyprowski@...sung.com>, airlied@...hat.com,
	"open list:DMA MAPPING HELPERS" <iommu@...ts.linux.dev>
Subject: Re: [PATCH v11 2/3] rust: add dma coherent allocator abstraction.

On Mon, Jan 27, 2025 at 07:46:07PM +0100, Alice Ryhl wrote:
> On Mon, Jan 27, 2025 at 7:38 PM Boqun Feng <boqun.feng@...il.com> wrote:
> >
> > On Mon, Jan 27, 2025 at 07:32:29PM +0100, Alice Ryhl wrote:
> > > On Mon, Jan 27, 2025 at 5:59 PM Boqun Feng <boqun.feng@...il.com> 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@...nel.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@...il.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.
> > > > >
> > > > > 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.
> > > > >
> > > >
> > > > Do you also assume that volatile read/write provide some sort of
> > > > atomicity? Because otherwise even though the read/write may not be
> > > > considered as UB, then results can be load/store teared.
> > > >
> > > > I asked because I think in case that we need atomicity, we should just
> > > > use atomic APIs.
> > >
> > > No, I'm not assuming that. I think it's like uaccess. Under normal
> > > cases, it's not going to be concurrently modified, but it shouldn't
> > > trigger UB if it is.
> > >
> >
> > Let's say my_alloc[7].foo is a (u64, u64), would
> >
> >         dma_read!(my_alloc[7].foo)
> >
> > and
> >
> >         dma_write!(my_alloc[7].foo, (1u64, 2u64))
> >
> > trigger any UB when they are concurrent? (Of course, the example here is
> > a bit inpropriate because it's DMA buff, but still the question is more
> > on whatever atomic expectation we want from read_volatile() and
> > write_volatile()).
> 
> I imagine that it would be most convenient for it to not be UB, but I
> also don't think people would have an expectation for that to not
> involve tearing.
> 

Depending on the granularity that tearing can happen, if .foo is an enum
(or any other type that not all bit combinations are valid) and tearing
can happen at byte levels, then a racing dma_read() may read invalid
data.

I think it's fine to expect read_volatile() and write_volatile()
themselves don't trigger UB, but we will need to be careful about the
atomic granularity that we can expect on them. It would be more clear if
we use the atomic API here (and implementation can be read_volatile()
and write_volatile()), and it can avoid coding based on tribal knowledge
such as "in kernel, read_volatile() and write_volatile() imply atomic".

Regards,
Boqun

> Alice

Powered by blists - more mailing lists