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

[Andreas Hindborg <a.hindborg@kernel.org>]

"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