Re: [PATCH v5 2/6] arch: unify ioremap prototypes and macro aliases

Toshi Kani
2015-07-09T23:44:37Z

On Thu, 2015-07-09 at 03:40 +0200, Luis R. Rodriguez wrote:
> On Tue, Jul 07, 2015 at 05:10:58PM -0600, Toshi Kani wrote:
> > On Tue, 2015-07-07 at 18:07 +0200, Luis R. Rodriguez wrote:
> > > On Tue, Jul 07, 2015 at 11:13:30AM +0100, Russell King - ARM
> > > Linux
> > > wrote:
> > :
> > > > On ARM, we (probably) have a lot of cases where ioremap() is
> > > > used
> > > > multiple
> > > > times for the same physical address space, so we shouldn't rule
> > > > out
> > > > having
> > > > multiple mappings of the same type.
> > >
> > > Why is that done? Don't worry if you are not sure why but only
> > > speculate of the
> > > practice's existence (sloppy drivers or lazy driver developers).
> > > FWIW
> > > for x86
> > > IIRC I ended up concluding that overlapping ioremap() calls with
> > > the
> > > same type
> > > would work but not if they differ in type. Although I haven't
> > > written a
> > > grammer rule to hunt down overlapping ioremap() I suspected its
> > > use
> > > was likely
> > > odd and likely should be reconsidered. Would this be true for ARM
> > > too
> > > ? Or are
> > > you saying this should be a feature ? I don't expect an answer
> > > now
> > > but I'm
> > > saying we *should* all together decide on this, and if you're
> > > inclined to
> > > believe that this should ideally be avoided I'd like to hear
> > > that. If
> > > you feel
> > > strongly though this should be a feature I would like to know
> > > why.
> >
> > There are multiple mapping interfaces, and overlapping can happen
> > among
> > them as well. For instance, remap_pfn_range() (and
> > io_remap_pfn_range(), which is the same as remap_pfn_range() on
> > x86)
> > creates a mapping to user space. The same physical ranges may be
> > mapped to kernel and user spaces. /dev/mem is one example that may
> > create a user space mapping to a physical address that is already
> > mapped with ioremap() by other module.
>
> Thanks for the feedback. The restriction seems to be differing cache
> types
> requirements, other than this, are there any other concerns ? For
> instance are
> we completley happy with aliasing so long as cache types match
> everywhere? I'd
> expect no architecture would want cache types to differ when
> aliasing, what
> should differ then I think would just be how to verify this and it
> doesn't seem
> we may be doing this for all architectures.
>
> Even for userspace we seem to be covered -- we enable userspace
> mmap() calls to
> get their mapped space with a cache type, on the kernel we'd say use
> pgprot_writecombine() on the vma->vm_page_prot prior to the
> io_remap_pfn_range() -- that maps to remap_pfn_range() on x86 and as
> you note
> that checks cache type via reserve_memtype() -- but only on x86...
>
> Other than this differing cache type concern are we OK with aliasing
> in
> userspace all the time ?
>
> If we want to restrict aliasing either for the kernel or userspace
> mapping
> we might be able to do it, I just want to know if we want to or not
> care
> at all.

Yes, we allow to create multiple mappings to a same physical page as
long as their cache type is the same. There are multiple use-cases
that depend on this ability.

> > pmem and DAX also create mappings to the same NVDIMM ranges. DAX
> > calls
> > vm_insert_mixed(), which is particularly a problematic since
> > vm_insert_mixed() does not verify aliasing. ioremap() and
> > remap_pfn_range()
> > call reserve_memtype() to verify aliasing on x86.
> > reserve_memtype() is
> > x86-specific and there is no arch-generic wrapper for such check.
>
> As clarified by Matthew Wilcox via commit d92576f1167cacf7844 ("dax:
> does not
> work correctly with virtual aliasing caches") caches are virtually
> mapped for
> some architectures, it seems it should be possible to fix this for
> DAX somehow
> though.

I simply described this DAX case as an example of how two modules might
request different cache types. Yes, we should be able to fix this
case.

> > I think DAX could get a cache type from pmem to keep them in sync,
> > though.
>
> pmem is x86 specific right now, are other folks going to expose
> something
> similar ? Otherwise we seem to only be addressing these deep concerns
> for
> x86 so far.

pmem is a generic driver and is not x86-specific.

Thanks,
-Toshi