Hi,

Here are the scatterlist bindings that has been brewing for a while in my
local tree while working with Nova code. The bindings are used mostly to
build the radix3 table from the GSP firmware which is loaded via dma.
This interface can be used on top of existing kernel scatterlist objects
or to allocate a new one from scratch.

Some questions still need to be resolved, which mostly come from
the DeviceSGTable::dma_map() function. Primarily, what if you call
bindings::dma_map_sgtable() on an already mapped sg_table? From my
experiments it doesn't seem to do anything and no indication is returned if
the call succeeded or not. Should we save the "mapping info" to a list
everytime we call DeviceSGTable::dma_map more than once?

Hoping this initial submission will generate some discussion. I'd like to
acknowledge valuable feedback from Danilo Krummrich and Lyude
Paul in shaping this up.

Abdiel Janulgue (2):
  rust: add initial scatterlist bindings
  samples: rust: add sample code for scatterlist bindings

 rust/bindings/bindings_helper.h |   1 +
 rust/helpers/helpers.c          |   1 +
 rust/helpers/scatterlist.c      |  25 +++
 rust/kernel/lib.rs              |   1 +
 rust/kernel/scatterlist.rs      | 275 ++++++++++++++++++++++++++++++++
 samples/rust/rust_dma.rs        |  14 +-
 6 files changed, 316 insertions(+), 1 deletion(-)
 create mode 100644 rust/helpers/scatterlist.c
 create mode 100644 rust/kernel/scatterlist.rs


base-commit: dd21715de3dfa6f6457432ce909e5f7eb142a7d2
-- 
2.43.0

On Mon, May 12, 2025 at 12:53:26PM +0300, Abdiel Janulgue wrote:
>
> Some questions still need to be resolved, which mostly come from
> the DeviceSGTable::dma_map() function. Primarily, what if you call
> bindings::dma_map_sgtable() on an already mapped sg_table? From my
> experiments it doesn't seem to do anything and no indication is returned if
> the call succeeded or not. Should we save the "mapping info" to a list
> everytime we call DeviceSGTable::dma_map more than once?

You should cc Christoph Hellwig on the DMA mapping API.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Tue, May 13, 2025 at 10:19:45AM +0800, Herbert Xu wrote:
> You should cc Christoph Hellwig on the DMA mapping API.

I'm not going to use my scare time to let this cancer creep further.

On Mon, 12 May 2025 22:50:29 -0700
Christoph Hellwig <hch@infradead.org> wrote:

> On Tue, May 13, 2025 at 10:19:45AM +0800, Herbert Xu wrote:
> > You should cc Christoph Hellwig on the DMA mapping API.  
> 
> I'm not going to use my scare time to let this cancer creep further.

No, please do not scare us with cancer again. We do respect your
position on the Rust acceptance scale (where this end is in fact named
after you).

It's fine; you don't have to do anything here.

Petr T

Hi Abdiel,

> On 12 May 2025, at 06:53, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
> 
> Hi,
> 
> Here are the scatterlist bindings that has been brewing for a while in my
> local tree while working with Nova code. The bindings are used mostly to
> build the radix3 table from the GSP firmware which is loaded via dma.
> This interface can be used on top of existing kernel scatterlist objects
> or to allocate a new one from scratch.
> 
> Some questions still need to be resolved, which mostly come from
> the DeviceSGTable::dma_map() function. Primarily, what if you call
> bindings::dma_map_sgtable() on an already mapped sg_table? From my

Perhaps we should introduce a type for buffers which are known to be mapped. Then
we can simply not offer the option to map for that type.

> experiments it doesn't seem to do anything and no indication is returned if
> the call succeeded or not. Should we save the "mapping info" to a list
> everytime we call DeviceSGTable::dma_map more than once?

What mapping info are you referring to?

> 
> Hoping this initial submission will generate some discussion. I'd like to
> acknowledge valuable feedback from Danilo Krummrich and Lyude
> Paul in shaping this up.
> 
> Abdiel Janulgue (2):
>  rust: add initial scatterlist bindings
>  samples: rust: add sample code for scatterlist bindings
> 
> rust/bindings/bindings_helper.h |   1 +
> rust/helpers/helpers.c          |   1 +
> rust/helpers/scatterlist.c      |  25 +++
> rust/kernel/lib.rs              |   1 +
> rust/kernel/scatterlist.rs      | 275 ++++++++++++++++++++++++++++++++
> samples/rust/rust_dma.rs        |  14 +-
> 6 files changed, 316 insertions(+), 1 deletion(-)
> create mode 100644 rust/helpers/scatterlist.c
> create mode 100644 rust/kernel/scatterlist.rs
> 
> 
> base-commit: dd21715de3dfa6f6457432ce909e5f7eb142a7d2
> -- 
> 2.43.0
> 
> 

— Daniel 

On 12/05/2025 14:19, Daniel Almeida wrote:
> Hi Abdiel,
> 
>> On 12 May 2025, at 06:53, Abdiel Janulgue <abdiel.janulgue@gmail.com> wrote:
>>
>> Hi,
>>
>> Here are the scatterlist bindings that has been brewing for a while in my
>> local tree while working with Nova code. The bindings are used mostly to
>> build the radix3 table from the GSP firmware which is loaded via dma.
>> This interface can be used on top of existing kernel scatterlist objects
>> or to allocate a new one from scratch.
>>
>> Some questions still need to be resolved, which mostly come from
>> the DeviceSGTable::dma_map() function. Primarily, what if you call
>> bindings::dma_map_sgtable() on an already mapped sg_table? From my
> 
> Perhaps we should introduce a type for buffers which are known to be mapped. Then
> we can simply not offer the option to map for that type.
> 
>> experiments it doesn't seem to do anything and no indication is returned if
>> the call succeeded or not. Should we save the "mapping info" to a list
>> everytime we call DeviceSGTable::dma_map more than once?
> 
> What mapping info are you referring to?
> 
Basically the dma_data_direction enum and possibly `Device`, if we 
decouple SGTable from the device. So this approach would mean that 
every-time SGTable::dma_map() is called, unique mapping object(s) would 
be created, and which would get unmapped later on the destructor:

struct SgtDmaMap {
     dev: ARef<Device>,
     dir: DmaDataDirection,
}

impl SgtDmaMap {
     /// Creates a new mapping object
     fn new(dev: &Device, dir: DmaDataDirection) -> Self {
         Self { dev: dev.into(), dir, }
     }
}
...
...

impl SGTable {
     pub fn dma_map(dev: &Device, dir: DmaDataDirection) -> 
Result<SgtDmaMap>

But I'm not sure if there is any point to that as the C 
`dma_map_sgtable()` doesn't seem to care anyway (I could be wrong with 
this) if the sg_table gets mapped more than once?

Regards,
Abdiel

On 14.05.2025 09:00, Abdiel Janulgue wrote:
>
> On 12/05/2025 14:19, Daniel Almeida wrote:
>> Hi Abdiel,
>>
>>> On 12 May 2025, at 06:53, Abdiel Janulgue 
>>> <abdiel.janulgue@gmail.com> wrote:
>>>
>>> Hi,
>>>
>>> Here are the scatterlist bindings that has been brewing for a while 
>>> in my
>>> local tree while working with Nova code. The bindings are used 
>>> mostly to
>>> build the radix3 table from the GSP firmware which is loaded via dma.
>>> This interface can be used on top of existing kernel scatterlist 
>>> objects
>>> or to allocate a new one from scratch.
>>>
>>> Some questions still need to be resolved, which mostly come from
>>> the DeviceSGTable::dma_map() function. Primarily, what if you call
>>> bindings::dma_map_sgtable() on an already mapped sg_table? From my
>>
>> Perhaps we should introduce a type for buffers which are known to be 
>> mapped. Then
>> we can simply not offer the option to map for that type.
>>
>>> experiments it doesn't seem to do anything and no indication is 
>>> returned if
>>> the call succeeded or not. Should we save the "mapping info" to a list
>>> everytime we call DeviceSGTable::dma_map more than once?
>>
>> What mapping info are you referring to?
>>
> Basically the dma_data_direction enum and possibly `Device`, if we 
> decouple SGTable from the device. So this approach would mean that 
> every-time SGTable::dma_map() is called, unique mapping object(s) 
> would be created, and which would get unmapped later on the destructor:
>
> struct SgtDmaMap {
>     dev: ARef<Device>,
>     dir: DmaDataDirection,
> }
>
> impl SgtDmaMap {
>     /// Creates a new mapping object
>     fn new(dev: &Device, dir: DmaDataDirection) -> Self {
>         Self { dev: dev.into(), dir, }
>     }
> }
> ...
> ...
>
> impl SGTable {
>     pub fn dma_map(dev: &Device, dir: DmaDataDirection) -> 
> Result<SgtDmaMap>
>
> But I'm not sure if there is any point to that as the C 
> `dma_map_sgtable()` doesn't seem to care anyway (I could be wrong with 
> this) if the sg_table gets mapped more than once?


Standard DMA-mapping C api doesn't have the notion of the object, 
although in case of sgtable structure, one might add some flags might 
there. Originally the sgtable based helpers were just trivial wrappers 
for dma_sync_sg_*() and dma_unmap_sg() ensuring proper parameters (and 
avoiding the confusion which nents to pass).

It is generally assumed that caller uses the DMA API properly and there 
are no checks for double dma_map calls. It is only correct to call 
dma_map_sgtable() for the same sgtable structure after earlier call to 
dma_unmap_sgtable().


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

On 14/05/2025 15:12, Marek Szyprowski wrote:
> On 14.05.2025 09:00, Abdiel Janulgue wrote:
>>
>> On 12/05/2025 14:19, Daniel Almeida wrote:
>>> Hi Abdiel,
>>>
>>>> On 12 May 2025, at 06:53, Abdiel Janulgue
>>>> <abdiel.janulgue@gmail.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Here are the scatterlist bindings that has been brewing for a while
>>>> in my
>>>> local tree while working with Nova code. The bindings are used
>>>> mostly to
>>>> build the radix3 table from the GSP firmware which is loaded via dma.
>>>> This interface can be used on top of existing kernel scatterlist
>>>> objects
>>>> or to allocate a new one from scratch.
>>>>
>>>> Some questions still need to be resolved, which mostly come from
>>>> the DeviceSGTable::dma_map() function. Primarily, what if you call
>>>> bindings::dma_map_sgtable() on an already mapped sg_table? From my
>>>
>>> Perhaps we should introduce a type for buffers which are known to be
>>> mapped. Then
>>> we can simply not offer the option to map for that type.
>>>
>>>> experiments it doesn't seem to do anything and no indication is
>>>> returned if
>>>> the call succeeded or not. Should we save the "mapping info" to a list
>>>> everytime we call DeviceSGTable::dma_map more than once?
>>>
>>> What mapping info are you referring to?
>>>
>> Basically the dma_data_direction enum and possibly `Device`, if we
>> decouple SGTable from the device. So this approach would mean that
>> every-time SGTable::dma_map() is called, unique mapping object(s)
>> would be created, and which would get unmapped later on the destructor:
>>
>> struct SgtDmaMap {
>>      dev: ARef<Device>,
>>      dir: DmaDataDirection,
>> }
>>
>> impl SgtDmaMap {
>>      /// Creates a new mapping object
>>      fn new(dev: &Device, dir: DmaDataDirection) -> Self {
>>          Self { dev: dev.into(), dir, }
>>      }
>> }
>> ...
>> ...
>>
>> impl SGTable {
>>      pub fn dma_map(dev: &Device, dir: DmaDataDirection) ->
>> Result<SgtDmaMap>
>>
>> But I'm not sure if there is any point to that as the C
>> `dma_map_sgtable()` doesn't seem to care anyway (I could be wrong with
>> this) if the sg_table gets mapped more than once?
> 
> 
> Standard DMA-mapping C api doesn't have the notion of the object,
> although in case of sgtable structure, one might add some flags might
> there. Originally the sgtable based helpers were just trivial wrappers
> for dma_sync_sg_*() and dma_unmap_sg() ensuring proper parameters (and
> avoiding the confusion which nents to pass).
> 
> It is generally assumed that caller uses the DMA API properly and there
> are no checks for double dma_map calls. It is only correct to call
> dma_map_sgtable() for the same sgtable structure after earlier call to
> dma_unmap_sgtable().

Thanks for the clarification! I think this double mapping issue can be 
solved by the suggestion presented by Alexander Courbot.

/Abdiel
> 
> 
> Best regards