Message-Id: <DCD2IM4THRWE.4M1IQTEWX6G8@nvidia.com>
Date: Wed, 27 Aug 2025 17:30:31 +0900
From: "Alexandre Courbot" <acourbot@...dia.com>
To: "Danilo Krummrich" <dakr@...nel.org>
Cc: <akpm@...ux-foundation.org>, <ojeda@...nel.org>,
 <alex.gaynor@...il.com>, <boqun.feng@...il.com>, <gary@...yguo.net>,
 <bjorn3_gh@...tonmail.com>, <lossin@...nel.org>, <a.hindborg@...nel.org>,
 <aliceryhl@...gle.com>, <tmgross@...ch.edu>, <abdiel.janulgue@...il.com>,
 <jgg@...pe.ca>, <lyude@...hat.com>, <robin.murphy@....com>,
 <daniel.almeida@...labora.com>, <rust-for-linux@...r.kernel.org>,
 <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 3/5] rust: scatterlist: Add abstraction for sg_table

On Wed Aug 27, 2025 at 12:18 AM JST, Danilo Krummrich wrote:
> On Tue Aug 26, 2025 at 4:36 PM CEST, Alexandre Courbot wrote:
>> On Mon Aug 25, 2025 at 10:24 PM JST, Danilo Krummrich wrote:
>>> Add a safe Rust abstraction for the kernel's scatter-gather list
>>> facilities (`struct scatterlist` and `struct sg_table`).
>>>
>>> This commit introduces `SGTable<T>`, a wrapper that uses a generic
>>> parameter to provide compile-time guarantees about ownership and lifetime.
>>>
>>> The abstraction provides two primary states:
>>> - `SGTable<Owned<P>>`: Represents a table whose resources are fully
>>>   managed by Rust. It takes ownership of a page provider `P`, allocates
>>>   the underlying `struct sg_table`, maps it for DMA, and handles all
>>>   cleanup automatically upon drop. The DMA mapping's lifetime is tied to
>>>   the associated device using `Devres`, ensuring it is correctly unmapped
>>>   before the device is unbound.
>>> - `SGTable<Borrowed>` (or just `SGTable`): A zero-cost representation of
>>>   an externally managed `struct sg_table`. It is created from a raw
>>>   pointer using `SGTable::as_ref()` and provides a lifetime-bound
>>>   reference (`&'a SGTable`) for operations like iteration.
>>>
>>> The API exposes a safe iterator that yields `&SGEntry` references,
>>> allowing drivers to easily access the DMA address and length of each
>>> segment in the list.
>>>
>>> Co-developed-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
>>> Signed-off-by: Abdiel Janulgue <abdiel.janulgue@...il.com>
>>> Signed-off-by: Danilo Krummrich <dakr@...nel.org>
>>
>> A few minor things below, but:
>>
>> Reviewed-by: Alexandre Courbot <acourbot@...dia.com>
>>
>> Used successfully on nova-core:
>>
>> Tested-by: Alexandre Courbot <acourbot@...dia.com>
>
> Thanks for re-testing!
>
>> I still see mentions of "type state" in the code (and the commit
>> message), is this on purpose? I still think this is a misleading use of
>> the term, but your call.
>
> I think I changed everything in the commit message, but there are indeed two or
> three mentions in the code still.
>
> I'm happy to replace them with "generic parameter", but I do not agree that the
> term "type state" is misleading.
>
> It may not be the classical typestate pattern,

And that's what is confusing. :)

>
> yet we are representing two
> distinct states of a type.

How about saying these are "variants" of a type, rather than a state?

>
>> <snip>
>>> +impl SGEntry {
>>> +    /// Convert a raw `struct scatterlist *` to a `&'a SGEntry`.
>>> +    ///
>>> +    /// # Safety
>>> +    ///
>>> +    /// Callers must ensure that the `struct scatterlist` pointed to by `ptr` is valid for the
>>> +    /// lifetime `'a`.
>>
>> Shouldn't the scatterlist also have valid a dma_address and dma_len?
>
> I don't think this is safety relevant from the perspective of Rust.
>
> Also note that if we want to provide this guarantee, we need the caller to
> provide the &Device<Bound> in SGTable::iter() the SGTable has been created with.
>
> For the Owned generic parameter this is easy, for the Borrowed one we have no
> way to ensure that the &Device<Bound> matches the device the SGTable has been
> mapped for.
>
> However, I don't think we have to provide this guarantee, since at this point
> all device resources (such as I/O memory) have been revoked from the driver
> already. So, effectively, even if a driver would attempt to program invalid DMA
> addresses, the driver would be uncapable of doing so anyways.

Right, I forgot this can be used even after the device has dropped. We
even discussed this point.

I was worried of what could happen on a setup without a IOMMU, if the
hardware goes berserk after being provided random memory addresses that
it can effectively write to due to the lack of a IOMMU to control the
accesses. Not quite sure whether this can happen, but in any case you
are right that we cannot uphold the guarantee that the DMA address and
length will remain valid even if they initially are anyway.

Powered by blists - more mailing lists