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

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

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