Re: [PATCH 2/2] rust: scatterlist: honor the device's maximum segment size

["Alexandre Courbot" <acourbot@nvidia.com>]

On Mon Sep 14, 2026 at 2:57 PM BST, Danilo Krummrich wrote:
> On Mon Sep 14, 2026 at 3:17 PM CEST, Gary Guo wrote:
>> On Mon Sep 14, 2026 at 1:17 PM BST, Robin Murphy wrote:
>>> On 14/09/2026 11:22 am, Gary Guo wrote:
>>>> On Mon Sep 14, 2026 at 10:58 AM BST, Danilo Krummrich wrote:
>>>>> On Mon Sep 14, 2026 at 2:45 AM CEST, Alexandre Courbot wrote:
>>>>>> On Mon Sep 14, 2026 at 6:08 AM JST, Matteo Kloiber wrote:
>>>>>>> On Mon Sep 7, 2026 at 11:43 AM JST, Alexandre Courbot wrote:
>>>>>>>> It also means that without patch 1, nova-core would split the firmware
>>>>>>>> into hundreds of 64KB SG entries, which is not breaking but still
>>>>>>>> something we want to avoid. The correct fix is to make sure that
>>>>>>>> `dma_set_max_seg_size` is called by the driver, and while we are at it
>>>>>>>> we also want every driver to call `dma_set_mask_and_coherent`. Ideally
>>>>>>>> we would use the type system to make sure that both functions are called
>>>>>>>> before any DMA operation can take place (using a safe interface), but
>>>>>>>> I'm not quite sure yet how we can do this.
>>>>>>>
>>>>>>> This sounds sensible indeed. Should I open a thread regarding that on Zulip?
>>>>>>
>>>>>> Probably not necessary, the mailing-list has a larger audience and is
>>>>>> the right place for this. I expect people will jump in here with their
>>>>>> thoughts.
>>>>>
>>>>> The problem with those is not that they must strictly be called before
>>>>> allocating DMA memory, but they must not be called concurrently with other DMA
>>>>> operations, such as allocating DMA memory, as it would technically be a data
>>>>> race.
>>>> 
>>>> Do they really have to be called *before* allocating DMA memory, not do they
>>>> just need not be called *concurrent* to DMA memory allocation?
>>>
>>> Similar to DMA masks, the segment parameters should be set appropriately 
>>> before any dma_map_sg() operation. Plus since they likely influence 
>>> scatterlist geometry, that means typically they're also going to need to 
>>> be set before building the scatterlist to be mapped in the first place.
>>
>> Right, in that case I think we have the following options:
>>
>> 1.  Have setters these being unsafe, with precondition that they must be done
>>     before creation of DMA mappings.
>
> As mentioned, I don't think the safety requirement has to be before using other
> DMA APIs, but just requiring to not do it concurrently. Not doing it would be a
> semantic bug, but it shouldn't have any safety implications.

Agreed on the safety requirement; but Robin's reply does suggest that we
really have no reason to not go the extra step and require these
parameters to be set before any DMA op takes place. Which is honestly
the answer I was hoping for as it makes things much simpler (see below).

>
>> 2.  Have a `dma_info()` method like Danilo mentioned. However, we cannot expose
>>     `&Device<Core<'_>>` inside such callbacks, because otherwise driver would be
>>     able create DMA mappings. This however is too restrictive to be the only way
>>     of setting DMA masks, because Nova needs to access the bar before setting the
>>     DMA masks.
>
> Just to clarify, I mentioned this to be something we could do in addition to (1)
> for drivers where those values are statically known.
>
>> 3.  Use wrapper type instead of generics for typestate. Then, we can express the
>>     pattern of typestate transformation (generics cannot do this because we
>>     always place it behind a reference).
>>
>>     For example, we can have `Device` to mean `Device<Normal>`, and then have
>>     `Bound<Device>` to mean `Device<Bound>` (both of which are behind a shared
>>     reference, like today, and `Bound<D>` can deref to `&D`).
>>
>>     Then, for `Core` typestate, we can instead have
>>
>>         struct Core<'a, D>(&'a Bound<Device>);
>>
>>     where it's passed by value. We can then define the DMA setters methods to
>>     operate on `Core` like we did today, but safely. Instead of providing a
>>     `Deref` impl that turns `&Core<'_, D>` to `&Bound<D>`, we only implement
>>     forwarding functions for APIs that can work without DMA configured (e.g. PCI
>>     bar).
>>
>>     Then, provide a `into_bound()` method which *consumes* `Core<'a, D>` and
>>     give out `&'a Bound<Device>` (this is why typestate generics cannot work,
>>     because `&'a Device<Core<'_>>` is Copy and we cannot represent the ownership
>>     consumption).
>
> Yeah, we had this discussion in the past with similar ideas; it is the kind of
> complication that we concluded not to be worth it and that I mentioned in my
> initial reply.

How about this: 

  fn probe<'bound>(
      pdev: &'bound pci::Device<Core<'_>>,
      dma: dma::Setup<'bound>,
      _info: Option<&'bound Self::IdInfo>,
  ) -> ...

`dma::Setup` is a token carrying the DMA capability. You turn it into a
`dma::Handle<'bound>` by acknowledging the DMA masks and segment
parameters using setters and a `finish` method. Then all DMA operations
are performed on that `dma::Handle`. This proves the DMA setup has been
performed before any operation takes place.

It doesn't complicate `Device` any further, and drivers that don't need
DMA can just drop the token and don't need to think about it anymore.

The DMA token is not opened from the `Device` because it needs to be
unique, so this adds one extra argument to probe, but this removes the
need to use `unsafe` while strictly enforcing the ordering (and staying
trivial to use).