Re: [PATCH v13 2/7] rust: add dma coherent allocator abstraction.

Andreas Hindborg
2025-03-07T20:40:42Z

"Abdiel Janulgue" <abdiel.janulgue@gmail.com> writes:

> Add a simple dma coherent allocator rust abstraction. Based on
> Andreas Hindborg's dma abstractions from the rnvme driver, which
> was also based on earlier work by Wedson Almeida Filho.
>
> A CoherentAllocation is wrapped in Devres which basically guarantees
> that a driver can't make a CoherentAllocation out-live driver unbind.
> This is needed, since DMA allocations potentially also result in
> programming of the IOMMU. IOMMU mappings are device resources and
> hence the device / driver lifecycle needs to be enforced.
>
> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@gmail.com>
> ---
> +
> + /// Reads the value of `field` and ensures that its type is [`FromBytes`].
> + ///
> + /// # Safety
> + ///
> + /// This must be called from the [`dma_read`] macro which ensures that the `field` pointer is
> + /// validated beforehand.
> + ///
> + /// Public but hidden since it should only be used from [`dma_read`] macro.
> + #[doc(hidden)]
> + pub unsafe fn field_read<F: FromBytes>(&self, field: *const F) -> F {
> + // SAFETY: By the safety requirements field is valid.
> + unsafe { field.read_volatile() }

From the docs of `read_volatile`:

Just like in C, whether an operation is volatile has no bearing
whatsoever on questions involving concurrent access from multiple
threads. Volatile accesses behave exactly like non-atomic accesses in
that regard. In particular, a race between a read_volatile and any
write operation to the same location is undefined behavior.

So the safety requirement is not sufficient. Alice responded to my
question on v12:

> Volatile reads/writes that race are OK?

I will not give a blanket yes to that. If you read their docs, you
will find that they claim to not allow it. But they are the correct
choice for DMA memory, and there's no way in practice to get
miscompilations on memory locations that are only accessed with
volatile operations, and never have references to them created.

In general, this will fall into the exception that we've been given
from the Rust people. In cases such as this where the Rust language
does not give us the operation we want, do it like you do in C. Since
Rust uses LLVM which does not miscompile the C part of the kernel, it
should not miscompile the Rust part either.

Since we have an exception for the safety requirements in the
documentation, we should make that clear in the safety comment at the
call site.

Best regards,
Andreas Hindborg