Hi Tvrtko,
On Wed, Apr 12, 2023 at 04:45:13PM +0100, Tvrtko Ursulin wrote:
[snip]
> >
> > [snip]
> > > However I am unsure if disabling pagefaulting is needed or not. Thomas,
> > > Matt, being the last to touch this area, perhaps you could have a look?
> > > Because I notice we have a fallback iomap path which still uses
> > > io_mapping_map_atomic_wc. So if kmap_atomic to kmap_local conversion is
> > > safe, does the iomap side also needs converting to
> > > io_mapping_map_local_wc? Or they have separate requirements?
> >
> > AFAIK, the requirements for io_mapping_map_local_wc() are the same as for
> > kmap_local_page(): the kernel virtual address is _only_ valid in the caller
> > context, and map/unmap nesting must be done in stack-based ordering (LIFO).
> >
> > I think a follow up patch could safely switch to io_mapping_map_local_wc() /
> > io_mapping_unmap_local_wc since the address is local to context.
> >
> > However, not being an expert, reading your note now I suspect that I'm
> > missing
> > something. Can I ask why you think that page-faults disabling might be
> > necessary?
>
> I am not saying it is, was just unsure and wanted some people who worked on
> this code most recently to take a look and confirm.
>
> I guess it will work since the copying is done like this anyway:
>
> /*
> * This is the fast path and we cannot handle a pagefault
> * whilst holding the struct mutex lest the user pass in the
> * relocations contained within a mmaped bo. For in such a case
> * we, the page fault handler would call i915_gem_fault() and
> * we would try to acquire the struct mutex again. Obviously
> * this is bad and so lockdep complains vehemently.
> */
> pagefault_disable();
> copied = __copy_from_user_inatomic(r, urelocs, count *
> sizeof(r[0]));
> pagefault_enable();
> if (unlikely(copied)) {
> remain = -EFAULT;
> goto out;
> }
>
> Comment is a bit outdated since we don't use that global "struct mutex" any
> longer, but in any case, if there is a page fault on the mapping where we
> need to recurse into i915 again to satisfy if, we seem to have code already
> to handle it. So kmap_local conversion I *think* can't regress anything.
Thanks for your explanation!
>
> Patch to convert the io_mapping_map_atomic_wc can indeed come later.
Okay, I will also look at this.
>
> In terms of logistics - if we landed this series to out branch it would be
> queued only for 6.5. Would that work for you?
Yeah, it's ok for me. But could I ask, did I miss the 6.4 merge time?
Thanks,
Zhao
>
> Regards,
>
> Tvrtko
