Re: Overlapping ioremap() calls, set_memory_*() semantics

["Luis R. Rodriguez" <mcgrof@kernel.org>]

On Thu, Mar 17, 2016 at 04:44:53PM -0600, Toshi Kani wrote:
> On Wed, 2016-03-16 at 02:45 +0100, Luis R. Rodriguez wrote:
> > On Fri, Mar 11, 2016 at 03:13:52PM -0700, Toshi Kani wrote:
> > > On Wed, 2016-03-09 at 10:15 +0100, Ingo Molnar wrote:
> > > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > > > 
> > > > > On Tue, 2016-03-08 at 13:16 +0100, Ingo Molnar wrote:
> > > > > > * Toshi Kani <toshi.kani@hpe.com> wrote:
> > > > > > 
>  :
> > > > > Did you mean 'aliased' or 'aliased with different cache attribute'?
> > > > >  The former check might be too strict.
> > > > 
> > > > I'd say even 'same attribute' aliasing is probably relatively rare.
> > > > 
> > > > And 'different but compatible cache attribute' is in fact more of a
> > > > sign that the driver author does the aliasing for a valid _reason_:
> > > > to have two different types of access methods to the same piece of
> > > > physical address space...
> > > 
> > > Right.  So, if we change to fail ioremap() on aliased cases, it'd be
> > > easier to start with the different attribute case first.  This case
> > > should be rare enough that we can manage to identify such callers and
> > > make them use a new API as necessary.  If we go ahead to fail any
> > > aliased cases, it'd be challenging to manage without a regression or
> > > two.
> > 
> > From my experience on the ioremap_wc() crusade, I found that the need for
> > aliasing with different cache types would have been needed in only 3
> > drivers. For these 3, the atyfb driver I did the proper split in MMIO and
> > framebuffer, but that was significant work.  I did this work to demo and
> > document such work. It wasn't easy. For other two, ivtv and ipath we left
> > as requiring "nopat" to be used. The ipath driver is on its way out of
> > the kenrel now through staging, and ivtv, well I am not aware of single
> > human being claiming to use it. The architecture of ivtv actually
> > prohibits us from ever using PAT for write-combining on the framebuffer
> > as the firmware is the only one who knows the write-combining area and
> > hides it from us.
> 
> At glace, there are 863 references to ioremap(), 329 references to
> ioremap_nocache(), and only 68 references to ioremap_wc() on x86.  There
> are many more ioremap callers with UC mappings than WC mappings, and it is
> hard to say that they never get aliased.

We need to start somewhere. If we really want to vet / white list aliasing
we probably will need both semantic analysis but perhaps also manual vetting,
and finally a phase where we help WARN on uses we did not white-list.

> > We might be able to use tools like Coccinelle to perhaps hunt for
> > the use of aliasing on drivers with different cache attribute types
> > to do a full assessment but I really think that will be really hard
> > to accomplish.
> > 
> > If we can learn anything from the ioremap_wc() crusade I'd say its that
> > the need for aliasing with different cache types obviously implies we
> > should disable such drivers with PAT as what we'd really need is a proper
> > split in maps, but history shows the split can be really hard. It sounded
> > like you guys were confirming we currently do not allow for aliasing with
> > different attributes on x86, is that the case for all architectures?
> > 
> > If aliasing with different cache attributes is not allowed for x86 and
> > if its also rare for other architectures that just leaves the hunt for
> > valid aliasing uses. That still may be hard to hunt for, but I also
> > suspect it may be rare.
> 
> Yes, I'd fail the different cache attribute case if we are to place more
> strict check.

OK it seems this is a good starting point. How can we get a general
architecture consensus aliasing with different cache attributes is a terrible
idea ? Perhaps a patch to WARN/error out and let architectures opt in to this
piece of code?

> > > I think the "set_memory_" prefix implies that their target is regular
> > > memory only.
> > 
> > I did not find any driver using set_memory_wc() on MMIO, its a good thing
> > as that does not work it seems even if it returns no error.  I'm not sure
> > of the use of other set_memory_*() on MMIO but I would suspect its not
> > used. A manual hunt may suffice to rule these out.
> 
> It's good to know that you did not find any case on MMIO.  The thing is,
> set_memory_wc() actually works on MMIO today... This is because __pa()
> returns a bogus address, which skips the alias check in the memtype.

Ingo, are you happy with that ? I honestly do not see the need for
use of set_memory_wc() for the cases I reviewed, I think the case for
write-combining can simply be addressed currently with ioremap_wc().

> > I guess what I'm trying to say is I am not sure we have a need for
> > set_cache_attr_*() APIs, unless of course we find such valid use.
> > 
> > > > And at that point we could definitely argue that set_cache_attr_*()
> > > > APIs should probably generate a warning for _RAM_, because they
> > > > mostly make sense for MMIO type of physical addresses, right? Regular
> > > > RAM should always be WB.
> > > > 
> > > > Are there cases where we change the caching attribute of RAM for
> > > > valid reasons, outside of legacy quirks?
> > > 
> > > ati_create_page_map() is one example that it gets a RAM page
> > > by __get_free_page(), and changes it to UC by calling set_memory_uc().
> > 
> > Should we instead have an API that lets it ask for RAM and of UC type?
> > That would seem a bit cleaner. BTW do you happen to know *why* it needs
> > UC RAM types?
> 
> This RAM page is then shared between graphic card and CPU.  I think this is
> because graphic card cannot snoop the cache.

Was this reason alone sufficient to open such APIs broadly for RAM?

> > > > >  - It only supports attribute transition of {WB -> NewType -> WB}
> > > > > for RAM.  RAM is tracked differently that WB is treated as "no
> > > > > map".  So, this transition does not cause a conflict on RAM.  This
> > > > > will causes a conflict on MMIO when it is tracked correctly.   
> > > > 
> > > > That looks like a bug?
> > > 
> > > This is by design since set_memory_xx was introduced for RAM only.  If
> > > we extend it to MMIO, then we need to change how memtype manages MMIO.
> > 
> > I'd be afraid to *want* to support this on MMIO as I would only expect
> > hacks from drivers.
> 
> Agreed, with the hope that they are not used on MMIO already...

OK we'll need to review this.

  Luis