Re: [RFC PATCH 0/2] scatterlist rust bindings

[Abdiel Janulgue <abdiel.janulgue@gmail.com>]

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