On 2025-12-02 at 13:28 +1100, Jordan Niethe <[email protected]> wrote...
> Hi,
>
> On 29/11/25 04:51, Jason Gunthorpe wrote:
> > On Fri, Nov 28, 2025 at 03:41:46PM +1100, Jordan Niethe wrote:
> > > Introduce helpers:
> > >
> > > - device_private_page_to_offset()
> > > - device_private_folio_to_offset()
> > >
> > > to take a given device private page / folio and return its offset within
> > > the device private address space (this is essentially a PFN within the
> > > device private address space).
> >
> > It would be nice if we rarely/never needed to see number space outside
> > the pte itself or the internal helpers..
>
> Outside of the PTE itself, one of the use cases for the PFNs themselves
> is range checking. Like we see in mm/page_vma_mapped.c:check_pte().
>
> >
> > Like, I don't think there should be stuff like this:
> >
> > > entry =
> > > make_writable_migration_device_private_entry(
> > > -
> > > page_to_pfn(page));
> > > +
> > > device_private_page_to_offset(page));
> >
> > make_writable_migration_device_private_entry() should accept the
> > struct page as the handle?
>
> That would be more clean - I'll give it a try.
>
> >
> > If it really is needed I think it should have its own dedicated type
> > and not be intermixed with normal pfns..
>
> One consideration here is for things like range checking the PFNs, the
> logic remains the same for device PFNs and the normal PFNs.
> If we represent the device PFNs as a unique type, ideally we'd like to
> still avoid introducing too much special handling.
Right, Jordan and I went back and forth on this a little bit prior to posting
but in the end I thought it wasn't worth the overhead of a new type for such a
limited number of use cases for which the actual logic ends up being the same
anyway.
Getting rid of passing the pfn to make_writable_migration_device_private_entry()
makes sense though and should address most of these cases.
> Potentially I could see something like a tagged union for memory indices
> like ...
>
> enum memory_index_type {
> MEMORY_INDEX_TYPE_PFN,
> MEMORY_INDEX_TYPE_DEVICE_MEMORY_INDEX,
> };
>
> union memory_index {
> unsigned long pfn;
> unsigned long device_memory_index;
> enum memory_index_type type;
> };
>
> ... if we wanted to introduce a dedicated type.
>
> Another possibility could be to avoid exposing the PFN for cases like
> this.
>
> For example if we went back to struct page_vma_mapped_walk containing a
> folio / struct page instead of a passing in a pfn then we could introduce
> some helper
> like ...
>
> bool swap_entry_contains_folio(struct folio *folio, swp_entry_t
> entry);
>
> ... that handles both device memory and normal memory and use that in
> check_pte().
>
> Thanks,
> Jordan.
>
>
> >
> > Jason
>
