Re: [PATCH 1/2] rust: add initial scatterlist bindings

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

On Thu Jun 5, 2025 at 10:30 PM JST, Abdiel Janulgue wrote:
>
>
> On 05/06/2025 08:51, Alexandre Courbot wrote:
>> On Thu Jun 5, 2025 at 3:21 AM JST, Lyude Paul wrote:
>>> On Fri, 2025-05-30 at 23:02 +0900, Alexandre Courbot wrote:
>>>> On Thu May 29, 2025 at 9:45 AM JST, Jason Gunthorpe wrote:
>>>>> On Thu, May 29, 2025 at 01:14:05AM +0300, Abdiel Janulgue wrote:
>>>>>> +impl SGEntry<Unmapped> {
>>>>>> +    /// Set this entry to point at a given page.
>>>>>> +    pub fn set_page(&mut self, page: &Page, length: u32, offset: u32) {
>>>>>> +        let c: *mut bindings::scatterlist = self.0.get();
>>>>>> +        // SAFETY: according to the `SGEntry` invariant, the scatterlist pointer is valid.
>>>>>> +        // `Page` invariant also ensures the pointer is valid.
>>>>>> +        unsafe { bindings::sg_set_page(c, page.as_ptr(), length, offset) };
>>>>>> +    }
>>>>>> +}
>>>>>
>>>>> Wrong safety statement. sg_set_page captures the page.as_ptr() inside
>>>>> the C datastructure so the caller must ensure it holds a reference on
>>>>> the page while it is contained within the scatterlist.
>>>>>
>>>>> Which this API doesn't force to happen.
>>>>>
>>>>> Most likely for this to work for rust you have to take a page
>>>>> reference here and ensure the page reference is put back during sg
>>>>> destruction. A typical normal pattern would 'move' the reference from
>>>>> the caller into the scatterlist.
>>>>
>>>> As Jason mentioned, we need to make sure that the backing pages don't get
>>>> dropped while the `SGTable` is alive. The example provided unfortunately fails
>>>> to do that:
>>>>
>>>>      let sgt = SGTable::alloc_table(4, GFP_KERNEL)?;
>>>>      let sgt = sgt.init(|iter| {
>>>>          for sg in iter {
>>>>              sg.set_page(&Page::alloc_page(GFP_KERNEL)?, PAGE_SIZE as u32, 0);
>>>>          }
>>>>          Ok(())
>>>>      })?;
>>>>
>>>> Here the allocated `Page`s are dropped immediately after their address is
>>>> written by `set_page`, giving the device access to memory that may now be used
>>>> for completely different purposes. As long as the `SGTable` exists, the memory
>>>> it points to must not be released or reallocated in any way.
>>>>
>>>> To that effect, we could simply store the `Page`s into the `SGTable`, but that
>>>> would cover only one of the many ways they can be constructed. For instance we
>>>> may want to share a `VVec` with a device and this just won't allow doing it.
>>>>
>>>> So we need a way to keep the provider of the pages alive into the `SGTable`,
>>>> while also having a convenient way to get its list of pages. Here is rough idea
>>>> for doing this, it is very crude and probably not bulletproof but hopefully it
>>>> can constitute a start.
>>>>
>>>> You would have a trait for providing the pages and their range:
>>>>
>>>>      /// Provides a list of pages that can be used to build a `SGTable`.
>>>>      trait SGTablePages {
>>>>          /// Returns an iterator to the pages providing the backing memory of `self`.
>>>>          fn pages_iter<'a>(&'a self) -> impl Iterator<Item = &'a bindings::page>;
>>>>          /// Returns the effective range of the mapping.
>>>>          fn range(&self) -> Range<usize>;
>>>>      }
>>>>
>>>> The `SGTable` becomes something like:
>>>>
>>>>      struct SGTable<P: SGTablePages, T: MapState>
>>>>      {
>>>>          table: Opaque<bindings::sg_table>,
>>>>          pages: P,
>>>>          _s: PhantomData<T>,
>>>>      }
>>>
>>> Hopefully I'm not missing anything here but - I'm not sure how I feel about
>>> this making assumptions about the memory layout of an sg_table beyond just
>>> being a struct sg_table. For instance, in the gem shmem helpers I had this for
>>> exposing the SGTable that is setup for gem shmem objects:
>>>
>>> struct OwnedSGTable<T: drm::gem::shmem::DriverObject> {
>>>      sg_table: NonNull<SGTable>
>>>      _owner: ARef<Object<T>>
>>> }
>>>
>>> So, I'm not really sure we have any reasonable representation for P here as we
>>> don't handle the memory allocation for the SGTable.
>> 
>> Maybe I need more context to understand your problem, but the point of
>> this design is precisely that it doesn't make any assumption about the
>> memory layout - all `P` needs to do is provide the pages describing the
>> memory backing.
>> 
>> Assuming that `_owner` here is the owner of the memory, couldn't you
>> flip your data layout and pass `_owner` (or rather a newtype wrapping
>> it) to `SGTable`, thus removing the need for a custom type?
>
> I think what Lyude has in mind here (Lyude, correct me if I'm wrong) is 
> for cases where we need to have a rust SGTable instances for those 
> struct sg_table that we didn't allocate ourselves for instance in the 
> gem shmem bindings. So memory layout needs to match for
> #[repr(transparent)] to work

Thanks, I think I am starting to understand and this is a problem
indeed. I should probably take a look at the DRM code to get my answers,
but IIUC in `OwnedSGTable`, `sg_table` is already provided by the C side
and is backed by `_owner`?